diff mbox series

format-patch: teach format.useAutoBase "whenAble" option

Message ID 20200925230701.2814287-1-jacob.e.keller@intel.com
State New
Headers show
Series format-patch: teach format.useAutoBase "whenAble" option | expand

Commit Message

Jacob Keller Sept. 25, 2020, 11:07 p.m. UTC
From: Jacob Keller <jacob.keller@gmail.com>

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 <jacob.keller@gmail.com>
---
 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(-)

Comments

Junio C Hamano Sept. 26, 2020, 10:38 p.m. UTC | #1
Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> 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.

Two minor points.

 * would users get confused choosing between if-able and whenAble to
   use for the configuration and the command line option?

 * what if there is a branch called "if-able"?  The same problem
   already exists with a refname "auto", and this makes it worse.

I do not know what the best approach to solve the latter, but the
former would be easier to solve by just picking one and sticking to
it.

Thanks.
diff mbox series

Patch

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" \