[PATCH] format-patch: teach format.useAutoBase "whenAble" option
From: Jacob Keller <jacob.e.keller@intel.com>
Date: 2020-09-25 23:08:01
Subsystem:
documentation, the rest · Maintainers:
Jonathan Corbet, Linus Torvalds
From: Jacob Keller <redacted>
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. --base also learns the same
mode using the term "if-able".
Add tests to cover the new mode of operation for --base.
Signed-off-by: Jacob Keller <redacted>
---
Documentation/config/format.txt | 5 +-
Documentation/git-format-patch.txt | 4 +-
builtin/log.c | 100 ++++++++++++++++++++++-------
t/t4014-format-patch.sh | 27 ++++++++
4 files changed, 112 insertions(+), 24 deletions(-)
diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt
index 564e8091ba5c..e0760f16d6ad 100644
--- a/Documentation/config/format.txt
+++ b/Documentation/config/format.txt@@ -96,7 +96,10 @@ format.outputDirectory:: format.useAutoBase:: A boolean value which lets you enable the `--base=auto` option of - format-patch by default. + format-patch by default. Can also be set to "whenAble" to set + `--base=if-able`. This causes format-patch to include the base + commit information if it can be determined, but skip it otherwise + without dying. format.notes:: Provides the default value for the `--notes` option to
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 0f81d0437bb6..b58f7cd13382 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt@@ -345,7 +345,9 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`. Record the base tree information to identify the state the patch series applies to. See the BASE TREE INFORMATION section below for details. If <commit> is "auto", a base commit is - automatically chosen. The `--no-base` option overrides a + automatically chosen. If <commit> is "if-able", a base commit is + included if available, however format-patch won't die if it cannot + find a valid base commit. The `--no-base` option overrides a `format.useAutoBase` configuration. --root::
diff --git a/builtin/log.c b/builtin/log.c
index b8824d898f49..3b15d3cb87ea 100644
--- a/builtin/log.c
+++ b/builtin/log.c@@ -805,9 +805,15 @@ enum cover_from_description { COVER_FROM_AUTO }; +enum auto_base_setting { + AUTO_BASE_NEVER, + AUTO_BASE_ALWAYS, + AUTO_BASE_WHEN_ABLE +}; + static enum thread_level thread; static int do_signoff; -static int base_auto; +static enum auto_base_setting auto_base; static char *from; static const char *signature = git_version_string; static const char *signature_file;
@@ -906,7 +912,11 @@ static int git_format_config(const char *var, const char *value, void *cb) if (!strcmp(var, "format.outputdirectory")) return git_config_string(&config_output_directory, var, value); if (!strcmp(var, "format.useautobase")) { - base_auto = git_config_bool(var, value); + if (value && !strcasecmp(value, "whenAble")) { + auto_base = AUTO_BASE_WHEN_ABLE; + return 0; + } + auto_base = git_config_bool(var, value) ? AUTO_BASE_ALWAYS : AUTO_BASE_NEVER; return 0; } if (!strcmp(var, "format.from")) {
@@ -1436,13 +1446,24 @@ static struct commit *get_base_commit(const char *base_commit, { struct commit *base = NULL; struct commit **rev; - int i = 0, rev_nr = 0; + int i = 0, rev_nr = 0, auto_select, die_on_failure; - if (base_commit && strcmp(base_commit, "auto")) { + if (!strcmp(base_commit, "auto")) { + auto_select = 1; + die_on_failure = 1; + } else if (!strcmp(base_commit, "if-able")) { + auto_select = 1; + die_on_failure = 0; + } else { + auto_select = 0; + die_on_failure = 1; + } + + if (!auto_select) { base = lookup_commit_reference_by_name(base_commit); if (!base) die(_("unknown commit %s"), base_commit); - } else if ((base_commit && !strcmp(base_commit, "auto"))) { + } else { struct branch *curr_branch = branch_get(NULL); const char *upstream = branch_get_upstream(curr_branch, NULL); if (upstream) {
@@ -1450,19 +1471,32 @@ static struct commit *get_base_commit(const char *base_commit, struct commit *commit; struct object_id oid; - if (get_oid(upstream, &oid)) - die(_("failed to resolve '%s' as a valid ref"), upstream); + if (get_oid(upstream, &oid)) { + if (die_on_failure) + die(_("failed to resolve '%s' as a valid ref"), upstream); + else + return NULL; + } commit = lookup_commit_or_die(&oid, "upstream base"); base_list = get_merge_bases_many(commit, total, list); /* There should be one and only one merge base. */ - if (!base_list || base_list->next) - die(_("could not find exact merge base")); + if (!base_list || base_list->next) { + if (die_on_failure) { + die(_("could not find exact merge base")); + } else { + free_commit_list(base_list); + return NULL; + } + } base = base_list->item; free_commit_list(base_list); } else { - die(_("failed to get upstream, if you want to record base commit automatically,\n" - "please use git branch --set-upstream-to to track a remote branch.\n" - "Or you could specify base commit by --base=<base-commit-id> manually")); + if (die_on_failure) + die(_("failed to get upstream, if you want to record base commit automatically,\n" + "please use git branch --set-upstream-to to track a remote branch.\n" + "Or you could specify base commit by --base=<base-commit-id> manually")); + else + return NULL; } }
@@ -1479,8 +1513,14 @@ static struct commit *get_base_commit(const char *base_commit, for (i = 0; i < rev_nr / 2; i++) { struct commit_list *merge_base; merge_base = get_merge_bases(rev[2 * i], rev[2 * i + 1]); - if (!merge_base || merge_base->next) - die(_("failed to find exact merge base")); + if (!merge_base || merge_base->next) { + if (die_on_failure) { + die(_("failed to find exact merge base")); + } else { + free(rev); + return NULL; + } + } rev[i] = merge_base->item; }
@@ -1490,12 +1530,24 @@ static struct commit *get_base_commit(const char *base_commit, rev_nr = DIV_ROUND_UP(rev_nr, 2); } - if (!in_merge_bases(base, rev[0])) - die(_("base commit should be the ancestor of revision list")); + if (!in_merge_bases(base, rev[0])) { + if (die_on_failure) { + die(_("base commit should be the ancestor of revision list")); + } else { + free(rev); + return NULL; + } + } for (i = 0; i < total; i++) { - if (base == list[i]) - die(_("base commit shouldn't be in revision list")); + if (base == list[i]) { + if (die_on_failure) { + die(_("base commit shouldn't be in revision list")); + } else { + free(rev); + return NULL; + } + } } free(rev);
@@ -1752,8 +1804,10 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) s_r_opt.def = "HEAD"; s_r_opt.revarg_opt = REVARG_COMMITTISH; - if (base_auto) + if (auto_base == AUTO_BASE_ALWAYS) base_commit = "auto"; + else if (auto_base == AUTO_BASE_WHEN_ABLE) + base_commit = "if-able"; if (default_attach) { rev.mime_boundary = default_attach;
@@ -2020,9 +2074,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) memset(&bases, 0, sizeof(bases)); if (base_commit) { struct commit *base = get_base_commit(base_commit, list, nr); - reset_revision_walk(); - clear_object_flags(UNINTERESTING); - prepare_bases(&bases, base, list, nr); + if (base) { + reset_revision_walk(); + clear_object_flags(UNINTERESTING); + prepare_bases(&bases, base, list, nr); + } } if (in_reply_to || thread || cover_letter)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 958c2da56ec6..0c14619293bb 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh@@ -2037,6 +2037,17 @@ test_expect_success 'format-patch errors out when history involves criss-cross' test_must_fail git format-patch --base=auto -1 ' +test_expect_success 'format-patch disable base=if-able when history involves criss-cross' ' + git format-patch --base=if-able -1 >patch && + ! grep "^base-commit:" patch +' + +test_expect_success 'format-patch format.useAutoBase whenAble history involves criss-cross' ' + test_config format.useAutoBase whenAble && + git format-patch -1 >patch && + ! grep "^base-commit:" patch +' + test_expect_success 'format-patch format.useAutoBase option' ' git checkout local && test_config format.useAutoBase true &&
@@ -2047,6 +2058,16 @@ test_expect_success 'format-patch format.useAutoBase option' ' test_cmp expect actual ' +test_expect_success 'format-patch format.useAutoBase option with whenAble' ' + git checkout local && + test_config format.useAutoBase whenAble && + git format-patch --stdout -1 >patch && + grep "^base-commit:" patch >actual && + git rev-parse upstream >commit-id-base && + echo "base-commit: $(cat commit-id-base)" >expect && + test_cmp expect actual +' + test_expect_success 'format-patch --base overrides format.useAutoBase' ' test_config format.useAutoBase true && git format-patch --stdout --base=HEAD~1 -1 >patch &&
@@ -2062,6 +2083,12 @@ test_expect_success 'format-patch --no-base overrides format.useAutoBase' ' ! grep "^base-commit:" patch ' +test_expect_success 'format-patch --no-base overrides format.useAutoBase whenAble' ' + test_config format.useAutoBase whenAble && + git format-patch --stdout --no-base -1 >patch && + ! grep "^base-commit:" patch +' + test_expect_success 'format-patch --base with --attach' ' git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch && sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
--
2.28.0.497.g54e85e7af1ac