format-patch: teach format.useAutoBase "whenAble" option
The format.useAutoBase configuration option exists to allow users to enable '--base=auto' for format-patch by default. This can sometimes lead to poor workflow, due to unexpected failures when attempting to format an ancient patch: $ git format-patch -1 <an old commit> fatal: base commit shouldn't be in revision list This can be very confusing, as it is not necessarily immediately obvious that the user requested a --base (since this was in the configuration, not on the command line). We do want --base=auto to fail when it cannot provide a suitable base, as it would be equally confusing if a formatted patch did not include the base information when it was requested. Teach format.useAutoBase a new mode, "whenAble". This mode will cause format-patch to attempt to include a base commit when it can. However, if no valid base commit can be found, then format-patch will continue formatting the patch without a base commit. In order to avoid making yet another branch name unusable with --base, do not teach --base=whenAble or --base=whenable. Instead, refactor the base_commit option to use a callback, and rely on the global configuration variable auto_base. This does mean that a user cannot request this optional base commit generation from the command line. However, this is likely not too valuable. If the user requests base information manually, they will be immediately informed of the failure to acquire a suitable base commit. This allows the user to make an informed choice about whether to continue the format. Add tests to cover the new mode of operation for --base. Signed-off-by: Jacob Keller <jacob.keller@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Jacob Keller committed
Oct 1, 2020 at 14:46 UTC
7efba5fa39fa1608fccea51a849b36b0f85825bb
3 files changed
+130
-26
Documentation/config/format.txt
+3
-1
@@ -96,7 +96,9 @@ format.outputDirectory::
96
97
format.useAutoBase::
98
A boolean value which lets you enable the `--base=auto` option of
99
- format-patch by default.
99
+ format-patch by default. Can also be set to "whenAble" to allow
100
+ enabling `--base=auto` if a suitable base is available, but to skip
101
+ adding base info otherwise without the format dying.
102
103
format.notes::
104
Provides the default value for the `--notes` option to
builtin/log.c
+105
-25
@@ -805,9 +805,15 @@ enum cover_from_description {
805
COVER_FROM_AUTO
806
};
807
808
+enum auto_base_setting {
809
+ AUTO_BASE_NEVER,
810
+ AUTO_BASE_ALWAYS,
811
+ AUTO_BASE_WHEN_ABLE
812
+};
813
+
814
static enum thread_level thread;
815
static int do_signoff;
810
-static int base_auto;
816
+static enum auto_base_setting auto_base;
817
static char *from;
818
static const char *signature = git_version_string;
819
static const char *signature_file;
@@ -906,7 +912,11 @@ static int git_format_config(const char *var, const char *value, void *cb)
912
if (!strcmp(var, "format.outputdirectory"))
913
return git_config_string(&config_output_directory, var, value);
914
if (!strcmp(var, "format.useautobase")) {
909
- base_auto = git_config_bool(var, value);
915
+ if (value && !strcasecmp(value, "whenAble")) {
916
+ auto_base = AUTO_BASE_WHEN_ABLE;
917
+ return 0;
918
+ }
919
+ auto_base = git_config_bool(var, value) ? AUTO_BASE_ALWAYS : AUTO_BASE_NEVER;
920
return 0;
921
}
922
if (!strcmp(var, "format.from")) {
@@ -1424,6 +1434,23 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
1434
return 0;
1435
}
1436
1437
+static int base_callback(const struct option *opt, const char *arg, int unset)
1438
+{
1439
+ const char **base_commit = opt->value;
1440
+
1441
+ if (unset) {
1442
+ auto_base = AUTO_BASE_NEVER;
1443
+ *base_commit = NULL;
1444
+ } else if (!strcmp(arg, "auto")) {
1445
+ auto_base = AUTO_BASE_ALWAYS;
1446
+ *base_commit = NULL;
1447
+ } else {
1448
+ auto_base = AUTO_BASE_NEVER;
1449
+ *base_commit = arg;
1450
+ }
1451
+ return 0;
1452
+}
1453
+
1454
struct base_tree_info {
1455
struct object_id base_commit;
1456
int nr_patch_id, alloc_patch_id;
@@ -1436,13 +1463,36 @@ static struct commit *get_base_commit(const char *base_commit,
1463
{
1464
struct commit *base = NULL;
1465
struct commit **rev;
1439
- int i = 0, rev_nr = 0;
1466
+ int i = 0, rev_nr = 0, auto_select, die_on_failure;
1467
1441
- if (base_commit && strcmp(base_commit, "auto")) {
1468
+ switch (auto_base) {
1469
+ case AUTO_BASE_NEVER:
1470
+ if (base_commit) {
1471
+ auto_select = 0;
1472
+ die_on_failure = 1;
1473
+ } else {
1474
+ /* no base information is requested */
1475
+ return NULL;
1476
+ }
1477
+ break;
1478
+ case AUTO_BASE_ALWAYS:
1479
+ case AUTO_BASE_WHEN_ABLE:
1480
+ if (base_commit) {
1481
+ BUG("requested automatic base selection but a commit was provided");
1482
+ } else {
1483
+ auto_select = 1;
1484
+ die_on_failure = auto_base == AUTO_BASE_ALWAYS;
1485
+ }
1486
+ break;
1487
+ default:
1488
+ BUG("unexpected automatic base selection method");
1489
+ }
1490
+
1491
+ if (!auto_select) {
1492
base = lookup_commit_reference_by_name(base_commit);
1493
if (!base)
1494
die(_("unknown commit %s"), base_commit);
1445
- } else if ((base_commit && !strcmp(base_commit, "auto"))) {
1495
+ } else {
1496
struct branch *curr_branch = branch_get(NULL);
1497
const char *upstream = branch_get_upstream(curr_branch, NULL);
1498
if (upstream) {
@@ -1450,19 +1500,32 @@ static struct commit *get_base_commit(const char *base_commit,
1500
struct commit *commit;
1501
struct object_id oid;
1502
1453
- if (get_oid(upstream, &oid))
1454
- die(_("failed to resolve '%s' as a valid ref"), upstream);
1503
+ if (get_oid(upstream, &oid)) {
1504
+ if (die_on_failure)
1505
+ die(_("failed to resolve '%s' as a valid ref"), upstream);
1506
+ else
1507
+ return NULL;
1508
+ }
1509
commit = lookup_commit_or_die(&oid, "upstream base");
1510
base_list = get_merge_bases_many(commit, total, list);
1511
/* There should be one and only one merge base. */
1458
- if (!base_list || base_list->next)
1459
- die(_("could not find exact merge base"));
1512
+ if (!base_list || base_list->next) {
1513
+ if (die_on_failure) {
1514
+ die(_("could not find exact merge base"));
1515
+ } else {
1516
+ free_commit_list(base_list);
1517
+ return NULL;
1518
+ }
1519
+ }
1520
base = base_list->item;
1521
free_commit_list(base_list);
1522
} else {
1463
- die(_("failed to get upstream, if you want to record base commit automatically,\n"
1464
- "please use git branch --set-upstream-to to track a remote branch.\n"
1465
- "Or you could specify base commit by --base=<base-commit-id> manually"));
1523
+ if (die_on_failure)
1524
+ die(_("failed to get upstream, if you want to record base commit automatically,\n"
1525
+ "please use git branch --set-upstream-to to track a remote branch.\n"
1526
+ "Or you could specify base commit by --base=<base-commit-id> manually"));
1527
+ else
1528
+ return NULL;
1529
}
1530
}
1531
@@ -1479,8 +1542,14 @@ static struct commit *get_base_commit(const char *base_commit,
1542
for (i = 0; i < rev_nr / 2; i++) {
1543
struct commit_list *merge_base;
1544
merge_base = get_merge_bases(rev[2 * i], rev[2 * i + 1]);
1482
- if (!merge_base || merge_base->next)
1483
- die(_("failed to find exact merge base"));
1545
+ if (!merge_base || merge_base->next) {
1546
+ if (die_on_failure) {
1547
+ die(_("failed to find exact merge base"));
1548
+ } else {
1549
+ free(rev);
1550
+ return NULL;
1551
+ }
1552
+ }
1553
1554
rev[i] = merge_base->item;
1555
}
@@ -1490,12 +1559,24 @@ static struct commit *get_base_commit(const char *base_commit,
1559
rev_nr = DIV_ROUND_UP(rev_nr, 2);
1560
}
1561
1493
- if (!in_merge_bases(base, rev[0]))
1494
- die(_("base commit should be the ancestor of revision list"));
1562
+ if (!in_merge_bases(base, rev[0])) {
1563
+ if (die_on_failure) {
1564
+ die(_("base commit should be the ancestor of revision list"));
1565
+ } else {
1566
+ free(rev);
1567
+ return NULL;
1568
+ }
1569
+ }
1570
1571
for (i = 0; i < total; i++) {
1497
- if (base == list[i])
1498
- die(_("base commit shouldn't be in revision list"));
1572
+ if (base == list[i]) {
1573
+ if (die_on_failure) {
1574
+ die(_("base commit shouldn't be in revision list"));
1575
+ } else {
1576
+ free(rev);
1577
+ return NULL;
1578
+ }
1579
+ }
1580
}
1581
1582
free(rev);
@@ -1638,6 +1719,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
1719
char *branch_name = NULL;
1720
char *base_commit = NULL;
1721
struct base_tree_info bases;
1722
+ struct commit *base;
1723
int show_progress = 0;
1724
struct progress *progress = NULL;
1725
struct oid_array idiff_prev = OID_ARRAY_INIT;
@@ -1714,8 +1796,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
1796
PARSE_OPT_OPTARG, thread_callback),
1797
OPT_STRING(0, "signature", &signature, N_("signature"),
1798
N_("add a signature")),
1717
- OPT_STRING(0, "base", &base_commit, N_("base-commit"),
1718
- N_("add prerequisite tree info to the patch series")),
1799
+ OPT_CALLBACK_F(0, "base", &base_commit, N_("base-commit"),
1800
+ N_("add prerequisite tree info to the patch series"),
1801
+ 0, base_callback),
1802
OPT_FILENAME(0, "signature-file", &signature_file,
1803
N_("add a signature from a file")),
1804
OPT__QUIET(&quiet, N_("don't print the patch filenames")),
@@ -1752,9 +1835,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
1835
s_r_opt.def = "HEAD";
1836
s_r_opt.revarg_opt = REVARG_COMMITTISH;
1837
1755
- if (base_auto)
1756
- base_commit = "auto";
1757
-
1838
if (default_attach) {
1839
rev.mime_boundary = default_attach;
1840
rev.no_inline = 1;
@@ -2018,8 +2098,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
2098
}
2099
2100
memset(&bases, 0, sizeof(bases));
2021
- if (base_commit) {
2022
- struct commit *base = get_base_commit(base_commit, list, nr);
2101
+ base = get_base_commit(base_commit, list, nr);
2102
+ if (base) {
2103
reset_revision_walk();
2104
clear_object_flags(UNINTERESTING);
2105
prepare_bases(&bases, base, list, nr);
t/t4014-format-patch.sh
+22
@@ -2037,6 +2037,12 @@ test_expect_success 'format-patch errors out when history involves criss-cross'
2037
test_must_fail git format-patch --base=auto -1
2038
'
2039
2040
+test_expect_success 'format-patch format.useAutoBase whenAble history involves criss-cross' '
2041
+ test_config format.useAutoBase whenAble &&
2042
+ git format-patch -1 >patch &&
2043
+ ! grep "^base-commit:" patch
2044
+'
2045
+
2046
test_expect_success 'format-patch format.useAutoBase option' '
2047
git checkout local &&
2048
test_config format.useAutoBase true &&
@@ -2047,6 +2053,16 @@ test_expect_success 'format-patch format.useAutoBase option' '
2053
test_cmp expect actual
2054
'
2055
2056
+test_expect_success 'format-patch format.useAutoBase option with whenAble' '
2057
+ git checkout local &&
2058
+ test_config format.useAutoBase whenAble &&
2059
+ git format-patch --stdout -1 >patch &&
2060
+ grep "^base-commit:" patch >actual &&
2061
+ git rev-parse upstream >commit-id-base &&
2062
+ echo "base-commit: $(cat commit-id-base)" >expect &&
2063
+ test_cmp expect actual
2064
+'
2065
+
2066
test_expect_success 'format-patch --base overrides format.useAutoBase' '
2067
test_config format.useAutoBase true &&
2068
git format-patch --stdout --base=HEAD~1 -1 >patch &&
@@ -2062,6 +2078,12 @@ test_expect_success 'format-patch --no-base overrides format.useAutoBase' '
2078
! grep "^base-commit:" patch
2079
'
2080
2081
+test_expect_success 'format-patch --no-base overrides format.useAutoBase whenAble' '
2082
+ test_config format.useAutoBase whenAble &&
2083
+ git format-patch --stdout --no-base -1 >patch &&
2084
+ ! grep "^base-commit:" patch
2085
+'
2086
+
2087
test_expect_success 'format-patch --base with --attach' '
2088
git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
2089
sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \