[PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Xiaolong Ye]

Thanks for Junio's reviews and suggestions.

This version contains the following changes since v5:

 - Fix a decl-after-statement in patch 3/4.

 - Improve testcases to cover more scenarios and make them more portable and
   readable.

Thanks,
Xiaolong

Xiaolong Ye (4):
  patch-ids: make commit_patch_id() a public helper function
  format-patch: add '--base' option to record base tree info
  format-patch: introduce --base=auto option
  format-patch: introduce format.useAutoBase configuration

 Documentation/config.txt           |   5 ++
 Documentation/git-format-patch.txt |  60 +++++++++++++
 builtin/log.c                      | 168 +++++++++++++++++++++++++++++++++++++
 patch-ids.c                        |   2 +-
 patch-ids.h                        |   2 +
 t/t4014-format-patch.sh            | 105 +++++++++++++++++++++++
 6 files changed, 341 insertions(+), 1 deletion(-)

-- 
2.8.1.343.gda643e5

base-commit: 3ad15fd5e17bbb73fb1161ff4e9c3ed254d5b243

[PATCH v6 1/4] patch-ids: make commit_patch_id() a public helper function

[Xiaolong Ye]

Make commit_patch_id() available to other builtins.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 patch-ids.c | 2 +-
 patch-ids.h | 2 ++
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/patch-ids.c b/patch-ids.c
index b7b3e5a..a4d0016 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -4,7 +4,7 @@
 #include "sha1-lookup.h"
 #include "patch-ids.h"
 
-static int commit_patch_id(struct commit *commit, struct diff_options *options,
+int commit_patch_id(struct commit *commit, struct diff_options *options,
 		    unsigned char *sha1)
 {
 	if (commit->parents)
diff --git a/patch-ids.h b/patch-ids.h
index c8c7ca1..eeb56b3 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -13,6 +13,8 @@ struct patch_ids {
 	struct patch_id_bucket *patches;
 };
 
+int commit_patch_id(struct commit *commit, struct diff_options *options,
+		    unsigned char *sha1);
 int init_patch_ids(struct patch_ids *);
 int free_patch_ids(struct patch_ids *);
 struct patch_id *add_commit_patch_id(struct commit *, struct patch_ids *);
-- 
2.8.1.343.gda643e5

[PATCH v6 2/4] format-patch: add '--base' option to record base tree info

[Xiaolong Ye]

Maintainers or third party testers may want to know the exact base tree
the patch series applies to. Teach git format-patch a '--base' option
to record the base tree info and append it at the end of the first
message (either the cover letter or the first patch in the series).

The base tree info consists of the "base commit", which is a well-known
commit that is part of the stable part of the project history everybody
else works off of, and zero or more "prerequisite patches", which are
well-known patches in flight that is not yet part of the "base commit"
that need to be applied on top of "base commit" in topological order
before the patches can be applied.

The "base commit" is shown as "base-commit: " followed by the 40-hex of
the commit object name.  A "prerequisite patch" is shown as
"prerequisite-patch-id: " followed by the 40-hex "patch id", which can
be obtained by passing the patch through the "git patch-id --stable"
command.

Imagine that on top of the public commit P, you applied well-known
patches X, Y and Z from somebody else, and then built your three-patch
series A, B, C, the history would be like:

---P---X---Y---Z---A---B---C

With "git format-patch --base=P -3 C" (or variants thereof, e.g. with
"--cover-letter" of using "Z..C" instead of "-3 C" to specify the
range), the base tree information block is shown at the end of the
first message the command outputs (either the first patch, or the
cover letter), like this:

base-commit: P
prerequisite-patch-id: X
prerequisite-patch-id: Y
prerequisite-patch-id: Z

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 Documentation/git-format-patch.txt |  54 ++++++++++++++
 builtin/log.c                      | 139 +++++++++++++++++++++++++++++++++++++
 t/t4014-format-patch.sh            |  47 +++++++++++++
 3 files changed, 240 insertions(+)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6821441..1d790f1 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -265,6 +265,11 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
   Output an all-zero hash in each patch's From header instead
   of the hash of the commit.
 
+--base=<commit>::
+	Record the base tree information to identify the state the
+	patch series applies to.  See the BASE TREE INFORMATION section
+	below for details.
+
 --root::
 	Treat the revision argument as a <revision range>, even if it
 	is just a single commit (that would normally be treated as a
@@ -520,6 +525,55 @@ This should help you to submit patches inline using KMail.
 5. Back in the compose window: add whatever other text you wish to the
    message, complete the addressing and subject fields, and press send.
 
+BASE TREE INFORMATION
+---------------------
+
+The base tree information block is used for maintainers or third party
+testers to know the exact state the patch series applies to. It consists
+of the 'base commit', which is a well-known commit that is part of the
+stable part of the project history everybody else works off of, and zero
+or more 'prerequisite patches', which are well-known patches in flight
+that is not yet part of the 'base commit' that need to be applied on top
+of 'base commit' in topological order before the patches can be applied.
+
+The 'base commit' is shown as "base-commit: " followed by the 40-hex of
+the commit object name.  A 'prerequisite patch' is shown as
+"prerequisite-patch-id: " followed by the 40-hex 'patch id', which can
+be obtained by passing the patch through the `git patch-id --stable`
+command.
+
+Imagine that on top of the public commit P, you applied well-known
+patches X, Y and Z from somebody else, and then built your three-patch
+series A, B, C, the history would be like:
+
+................................................
+---P---X---Y---Z---A---B---C
+................................................
+
+With `git format-patch --base=P -3 C` (or variants thereof, e.g. with
+`--cover-letter` of using `Z..C` instead of `-3 C` to specify the
+range), the base tree information block is shown at the end of the
+first message the command outputs (either the first patch, or the
+cover letter), like this:
+
+------------
+base-commit: P
+prerequisite-patch-id: X
+prerequisite-patch-id: Y
+prerequisite-patch-id: Z
+------------
+
+For non-linear topology, such as
+
+................................................
+---P---X---A---M---C
+    \         /
+     Y---Z---B
+................................................
+
+You can also use `git format-patch --base=P -3 C` to generate patches
+for A, B and C, and the identifiers for P, X, Y, Z are appended at the
+end of the first message.
 
 EXAMPLES
 --------
diff --git a/builtin/log.c b/builtin/log.c
index dff3fbb..ee332ab 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1191,6 +1191,131 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+struct base_tree_info {
+	struct object_id base_commit;
+	int nr_patch_id, alloc_patch_id;
+	struct object_id *patch_id;
+};
+
+static struct commit *get_base_commit(const char *base_commit,
+				      struct commit **list,
+				      int total)
+{
+	struct commit *base = NULL;
+	struct commit **rev;
+	int i = 0, rev_nr = 0;
+
+	base = lookup_commit_reference_by_name(base_commit);
+	if (!base)
+		die(_("Unknown commit %s"), base_commit);
+
+	ALLOC_ARRAY(rev, total);
+	for (i = 0; i < total; i++)
+		rev[i] = list[i];
+
+	rev_nr = total;
+	/*
+	 * Get merge base through pair-wise computations
+	 * and store it in rev[0].
+	 */
+	while (rev_nr > 1) {
+		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"));
+
+			rev[i] = merge_base->item;
+		}
+
+		if (rev_nr % 2)
+			rev[i] = rev[2 * i];
+		rev_nr = (rev_nr + 1) / 2;
+	}
+
+	if (!in_merge_bases(base, rev[0]))
+		die(_("base commit should be the ancestor of revision list"));
+
+	for (i = 0; i < total; i++) {
+		if (base == list[i])
+			die(_("base commit shouldn't be in revision list"));
+	}
+
+	free(rev);
+	return base;
+}
+
+static void prepare_bases(struct base_tree_info *bases,
+			  struct commit *base,
+			  struct commit **list,
+			  int total)
+{
+	struct commit *commit;
+	struct rev_info revs;
+	struct diff_options diffopt;
+	int i;
+
+	if (!base)
+		return;
+
+	diff_setup(&diffopt);
+	DIFF_OPT_SET(&diffopt, RECURSIVE);
+	diff_setup_done(&diffopt);
+
+	oidcpy(&bases->base_commit, &base->object.oid);
+
+	init_revisions(&revs, NULL);
+	revs.max_parents = 1;
+	revs.topo_order = 1;
+	for (i = 0; i < total; i++) {
+		list[i]->object.flags &= ~UNINTERESTING;
+		add_pending_object(&revs, &list[i]->object, "rev_list");
+		list[i]->util = (void *)1;
+	}
+	base->object.flags |= UNINTERESTING;
+	add_pending_object(&revs, &base->object, "base");
+
+	if (prepare_revision_walk(&revs))
+		die(_("revision walk setup failed"));
+	/*
+	 * Traverse the commits list, get prerequisite patch ids
+	 * and stuff them in bases structure.
+	 */
+	while ((commit = get_revision(&revs)) != NULL) {
+		unsigned char sha1[20];
+		struct object_id *patch_id;
+		if (commit->util)
+			continue;
+		if (commit_patch_id(commit, &diffopt, sha1))
+			die(_("cannot get patch id"));
+		ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
+		patch_id = bases->patch_id + bases->nr_patch_id;
+		hashcpy(patch_id->hash, sha1);
+		bases->nr_patch_id++;
+	}
+}
+
+static void print_bases(struct base_tree_info *bases)
+{
+	int i;
+
+	/* Only do this once, either for the cover or for the first one */
+	if (is_null_oid(&bases->base_commit))
+		return;
+
+	/* Show the base commit */
+	printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
+
+	/* Show the prerequisite patches */
+	for (i = bases->nr_patch_id - 1; i >= 0; i--)
+		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
+
+	free(bases->patch_id);
+	bases->nr_patch_id = 0;
+	bases->alloc_patch_id = 0;
+	oidclr(&bases->base_commit);
+}
+
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
 {
 	struct commit *commit;
@@ -1215,6 +1340,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	int reroll_count = -1;
 	char *branch_name = NULL;
 	char *from = NULL;
+	char *base_commit = NULL;
+	struct base_tree_info bases;
+
 	const struct option builtin_format_patch_options[] = {
 		{ OPTION_CALLBACK, 'n', "numbered", &numbered, NULL,
 			    N_("use [PATCH n/m] even with a single patch"),
@@ -1277,6 +1405,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, N_("signature"),
 			    N_("add a signature")),
+		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
+			   N_("add prerequisite tree info to the patch series")),
 		OPT_FILENAME(0, "signature-file", &signature_file,
 				N_("add a signature from a file")),
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
@@ -1514,6 +1644,13 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		signature = strbuf_detach(&buf, NULL);
 	}
 
+	memset(&bases, 0, sizeof(bases));
+	if (base_commit) {
+		struct commit *base = get_base_commit(base_commit, list, nr);
+		reset_revision_walk();
+		prepare_bases(&bases, base, list, nr);
+	}
+
 	if (in_reply_to || thread || cover_letter)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
@@ -1527,6 +1664,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			gen_message_id(&rev, "cover");
 		make_cover_letter(&rev, use_stdout,
 				  origin, nr, list, branch_name, quiet);
+		print_bases(&bases);
 		total++;
 		start_number--;
 	}
@@ -1592,6 +1730,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 				       rev.mime_boundary);
 			else
 				print_signature();
+			print_bases(&bases);
 		}
 		if (!use_stdout)
 			fclose(stdout);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index eed2981..5dcf24f 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1460,4 +1460,51 @@ test_expect_success 'format-patch -o overrides format.outputDirectory' '
 	test_path_is_dir patchset
 '
 
+test_expect_success 'format-patch --base' '
+	git checkout side &&
+	git format-patch --stdout --base=HEAD~3 -1 >patch &&
+	grep "^base-commit:" patch >actual &&
+	grep "^prerequisite-patch-id:" patch >>actual &&
+	echo "base-commit: $(git rev-parse HEAD~3)" >expected &&
+	echo "prerequisite-patch-id: $(git show --patch HEAD~2 | git patch-id --stable | awk "{print \$1}")" >>expected &&
+	echo "prerequisite-patch-id: $(git show --patch HEAD~1 | git patch-id --stable | awk "{print \$1}")" >>expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'format-patch --base errors out when base commit is in revision list' '
+	test_must_fail git format-patch --base=HEAD -2 &&
+	test_must_fail git format-patch --base=HEAD~1 -2 &&
+	git format-patch --stdout --base=HEAD~2 -2 >patch &&
+	grep "^base-commit:" patch >actual &&
+	echo "base-commit: $(git rev-parse HEAD~2)" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'format-patch --base errors out when base commit is not ancestor of revision list' '
+	# For history as below:
+	#
+	#    ---Q---P---Z---Y---*---X
+	#	 \             /
+	#	  ------------W
+	#
+	# If "format-patch Z..X" is given, P and Z can not be specified as the base commit
+	git checkout -b topic1 master &&
+	git rev-parse HEAD >commit-id-base &&
+	test_commit P &&
+	git rev-parse HEAD >commit-id-P &&
+	test_commit Z &&
+	git rev-parse HEAD >commit-id-Z &&
+	test_commit Y &&
+	git checkout -b topic2 master &&
+	test_commit W &&
+	git merge topic1 &&
+	test_commit X &&
+	test_must_fail git format-patch --base=$(cat commit-id-P) -3 &&
+	test_must_fail git format-patch --base=$(cat commit-id-Z) -3 &&
+	git format-patch --stdout --base=$(cat commit-id-base) -3 >patch &&
+	grep "^base-commit:" patch >actual &&
+	echo "base-commit: $(cat commit-id-base)" >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.8.1.343.gda643e5

[PATCH v6 3/4] format-patch: introduce --base=auto option

[Xiaolong Ye]

Introduce --base=auto to record the base commit info automatically, the
base_commit will be the merge base of tip commit of the upstream branch
and revision-range specified in cmdline.

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Wu Fengguang <fengguang.wu@intel.com>
Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 Documentation/git-format-patch.txt |  6 ++++++
 builtin/log.c                      | 30 ++++++++++++++++++++++++++---
 t/t4014-format-patch.sh            | 39 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 72 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 1d790f1..bdeecd5 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -575,6 +575,12 @@ You can also use `git format-patch --base=P -3 C` to generate patches
 for A, B and C, and the identifiers for P, X, Y, Z are appended at the
 end of the first message.
 
+If set `--base=auto` in cmdline, it will track base commit automatically,
+the base commit will be the merge base of tip commit of the remote-tracking
+branch and revision-range specified in cmdline.
+For a local branch, you need to track a remote branch by `git branch
+--set-upstream-to` before using this option.
+
 EXAMPLES
 --------
 
diff --git a/builtin/log.c b/builtin/log.c
index ee332ab..db27135 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1205,9 +1205,33 @@ static struct commit *get_base_commit(const char *base_commit,
 	struct commit **rev;
 	int i = 0, rev_nr = 0;
 
-	base = lookup_commit_reference_by_name(base_commit);
-	if (!base)
-		die(_("Unknown commit %s"), base_commit);
+	if (!strcmp(base_commit, "auto")) {
+		struct branch *curr_branch = branch_get(NULL);
+		const char *upstream = branch_get_upstream(curr_branch, NULL);
+		if (upstream) {
+			struct commit_list *base_list;
+			struct commit *commit;
+			unsigned char sha1[20];
+
+			if (get_sha1(upstream, sha1))
+				die(_("Failed to resolve '%s' as a valid ref."), upstream);
+			commit = lookup_commit_or_die(sha1, "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."));
+			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."));
+		}
+	} else {
+		base = lookup_commit_reference_by_name(base_commit);
+		if (!base)
+			die(_("Unknown commit %s"), base_commit);
+	}
 
 	ALLOC_ARRAY(rev, total);
 	for (i = 0; i < total; i++)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 5dcf24f..8102158 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1507,4 +1507,43 @@ test_expect_success 'format-patch --base errors out when base commit is not ance
 	test_cmp expected actual
 '
 
+test_expect_success 'format-patch --base=auto' '
+	git checkout -b upstream master &&
+	git checkout -b local upstream &&
+	git branch --set-upstream-to=upstream &&
+	test_commit N1 &&
+	test_commit N2 &&
+	git format-patch --stdout --base=auto -2 >patch &&
+	grep "^base-commit:" patch >actual &&
+	echo "base-commit: $(git rev-parse upstream)" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'format-patch errors out when history involves criss-cross' '
+	# setup criss-cross history
+	#
+	#   B---M1---D
+	#  / \ /
+	# A   X
+	#  \ / \
+	#   C---M2---E
+	#
+	git checkout master &&
+	test_commit A &&
+	git checkout -b xb master &&
+	test_commit B &&
+	git checkout -b xc master &&
+	test_commit C &&
+	git checkout -b xbc xb -- &&
+	git merge xc &&
+	git checkout -b xcb xc -- &&
+	git branch --set-upstream-to=xbc &&
+	git merge xb &&
+	git checkout xbc &&
+	test_commit D &&
+	git checkout xcb &&
+	test_commit E &&
+	test_must_fail 	git format-patch --base=auto -1
+'
+
 test_done
-- 
2.8.1.343.gda643e5

[PATCH v6 4/4] format-patch: introduce format.useAutoBase configuration

[Xiaolong Ye]

This allows to record the base commit automatically, it is equivalent
to set --base=auto in cmdline.

The format.useAutoBase has lower priority than command line option,
so if user set format.useAutoBase and pass the command line option in
the meantime, base_commit will be the one passed to command line
option.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 Documentation/config.txt |  5 +++++
 builtin/log.c            | 17 +++++++++++------
 t/t4014-format-patch.sh  | 19 +++++++++++++++++++
 3 files changed, 35 insertions(+), 6 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 42d2b50..1fe2a85 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1259,6 +1259,11 @@ format.outputDirectory::
 	Set a custom directory to store the resulting files instead of the
 	current working directory.
 
+format.useAutoBase::
+	A boolean value which lets you enable the `--base=auto` option of
+	format-patch by default.
+
+
 filter.<driver>.clean::
 	The command which is used to convert the content of a worktree
 	file to a blob upon checkin.  See linkgit:gitattributes[5] for
diff --git a/builtin/log.c b/builtin/log.c
index db27135..099f4f7 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -702,6 +702,7 @@ static void add_header(const char *value)
 #define THREAD_DEEP 2
 static int thread;
 static int do_signoff;
+static int base_auto;
 static const char *signature = git_version_string;
 static const char *signature_file;
 static int config_cover_letter;
@@ -786,6 +787,10 @@ 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);
+		return 0;
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1205,7 +1210,11 @@ static struct commit *get_base_commit(const char *base_commit,
 	struct commit **rev;
 	int i = 0, rev_nr = 0;
 
-	if (!strcmp(base_commit, "auto")) {
+	if (base_commit && strcmp(base_commit, "auto")) {
+		base = lookup_commit_reference_by_name(base_commit);
+		if (!base)
+			die(_("Unknown commit %s"), base_commit);
+	} else if ((base_commit && !strcmp(base_commit, "auto")) || base_auto) {
 		struct branch *curr_branch = branch_get(NULL);
 		const char *upstream = branch_get_upstream(curr_branch, NULL);
 		if (upstream) {
@@ -1227,10 +1236,6 @@ static struct commit *get_base_commit(const char *base_commit,
 			      "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 {
-		base = lookup_commit_reference_by_name(base_commit);
-		if (!base)
-			die(_("Unknown commit %s"), base_commit);
 	}
 
 	ALLOC_ARRAY(rev, total);
@@ -1669,7 +1674,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 
 	memset(&bases, 0, sizeof(bases));
-	if (base_commit) {
+	if (base_commit || base_auto) {
 		struct commit *base = get_base_commit(base_commit, list, nr);
 		reset_revision_walk();
 		prepare_bases(&bases, base, list, nr);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 8102158..8049cad 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1546,4 +1546,23 @@ 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 format.useAutoBaseoption' '
+	test_when_finished "git config --unset format.useAutoBase" &&
+	git checkout local &&
+	git config format.useAutoBase true &&
+	git format-patch --stdout -1 >patch &&
+	grep "^base-commit:" patch >actual &&
+	echo "base-commit: $(git rev-parse upstream)" >expected &&
+	test_cmp expected actual
+'
+
+test_expect_success 'format-patch --base overrides format.useAutoBase' '
+	test_when_finished "git config --unset format.useAutoBase" &&
+	git config format.useAutoBase true &&
+	git format-patch --stdout --base=HEAD~1 -1 >patch &&
+	grep "^base-commit:" patch >actual &&
+	echo "base-commit: $(git rev-parse HEAD~1)" >expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.8.1.343.gda643e5

Re: [PATCH v6 4/4] format-patch: introduce format.useAutoBase configuration

[Stefan Beller]

> +format.useAutoBase::
> +       A boolean value which lets you enable the `--base=auto` option of
> +       format-patch by default.
> +
> +

In case you resend, please use just one empty line here?
(No need to resend because of this alone)

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 12:51 AM, Xiaolong Ye <xiaolong.ye@intel.com> wrote:
> Thanks for Junio's reviews and suggestions.
>
> This version contains the following changes since v5:
>
>  - Fix a decl-after-statement in patch 3/4.
>
>  - Improve testcases to cover more scenarios and make them more portable and
>    readable.
>
> Thanks,
> Xiaolong
>

Thanks for this feature!

I am playing around with this series, and here comes a feature request:
I have a local branch with no upstream set. My usual workflow is like this

    git checkout origin/master
    # toy around, do stuff
    git checkout -b new-shiny-feature
    git format-patch origin-master..

Now I have set the format.useautobase option and then the `git format-patch`
fails with

    fatal: Failed to get upstream, if you want to record base commit
automatically,
    please use git branch --set-upstream-to to track a remote branch.
    Or you could specify base commit by --base=<base-commit-id> manually.

but as I indicated I want patches from origin/master onwards,
Could we make use of that information? To record the base in my workflow
currently I need to do:

    git format-patch origin/master.. --base=origin/master

which seems redundant to me.
(I may be holding it wrong though? Should I try to set upstream
branches for my local branches? This seems weird to me as I cannot
push/change the upstream branches directly, as Junio owns the branches)

Thanks,
Stefan

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>     git checkout origin/master
>     # toy around, do stuff
>     git checkout -b new-shiny-feature
>     git format-patch origin-master..
>
> Now I have set the format.useautobase option and then the `git format-patch`
> fails with
>
>     fatal: Failed to get upstream, if you want to record base commit
> automatically,
>     please use git branch --set-upstream-to to track a remote branch.
>     Or you could specify base commit by --base=<base-commit-id> manually.
>
> but as I indicated I want patches from origin/master onwards,
> Could we make use of that information?

As you indicated where other than in this e-mail?  

I think the way for you to indicate that desire expected by this
series is to use "git branch" to set upstream of new-shiny-feature
branch to origin/master.  Shouldn't that work, or is that too much
work?

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>>     git checkout origin/master
>>     # toy around, do stuff
>>     git checkout -b new-shiny-feature
>>     git format-patch origin-master..
>>
>> Now I have set the format.useautobase option and then the `git format-patch`
>> fails with
>>
>>     fatal: Failed to get upstream, if you want to record base commit
>> automatically,
>>     please use git branch --set-upstream-to to track a remote branch.
>>     Or you could specify base commit by --base=<base-commit-id> manually.
>>
>> but as I indicated I want patches from origin/master onwards,
>> Could we make use of that information?
>
> As you indicated where other than in this e-mail?
>
> I think the way for you to indicate that desire expected by this
> series is to use "git branch" to set upstream of new-shiny-feature
> branch to origin/master.  Shouldn't that work, or is that too much
> work?

I can totally do that for longer series which require some back and forth.

But one-offs, such as typo fixes or other small things[1], for which I do
even have a local branch (i.e. checkout origin/master && fix &&
commit && send-email) this is another step that potentially bothers me.
Maybe I'll get used to it.

>From a UI perspective it seems logical to also check if the base
can be obtained from the patch range specifier. the message of
patch 2 focuses on the advantages for the maintainer and 3rd party
people. So I was just testing it as an individual contributor to ensure
it can be used easily. (People only use this once the benefits outweigh
the disadvantages. And as we do not have any advantage of it in Git,
the negatives need to be kept low?)

Thanks,
Stefan

[1] http://thread.gmane.org/gmane.comp.version-control.git/292634

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> I can totally do that for longer series which require some back and forth.
>
> But one-offs, such as typo fixes or other small things[1], for which I do
> even have a local branch (i.e. checkout origin/master && fix &&
> commit && send-email) this is another step that potentially bothers me.

So from where are you proposing Git to grab that information if you
do not tell it?  "If the HEAD is detached, assume that the base is
where it was detached from" or something?

> From a UI perspective it seems logical to also check if the base
> can be obtained from the patch range specifier.

If you are doing "format-patch master..my-branch", what do you
propose to set your base to?  master@{u}, perhaps?

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 11:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> I can totally do that for longer series which require some back and forth.
>>
>> But one-offs, such as typo fixes or other small things[1], for which I do
>> even have a local branch (i.e. checkout origin/master && fix &&
>> commit && send-email) this is another step that potentially bothers me.
>
> So from where are you proposing Git to grab that information if you
> do not tell it?  "If the HEAD is detached, assume that the base is
> where it was detached from" or something?

That would also work for me. In my first mail I was proposing to take
the information from the format-patch argument, such that a one off fix
would be:

    (1) git checkout origin/master
    (2) EDIT
    (3) git commit -a -m "fix"
    (4) git format-patch origin/master..  # <- This is the information.

However you read it as taking the information from the first line,
which is also fine with me, as then the (4) can become

    (4a) git format-patch HEAD^

Another thought:

  Most workflows do not have different remotes per branch, e.g.
  when `master` maps to  `origin/master` as its upstream it is likely that
  `topic-foo` maps to its equivalent at `origin/..` as well.

  Branches come and go in a topic based workflow, so configuring them
  for each new branch is cumbersome, so let's have a default `remote` for
  repository.

If we have a default remote per repository, the base finding algorithm
in format-patch could check if the base(s) of the patch series is a head
in one of the default remote branches, i.e. check all origin/* branches for
a match?


>
>> From a UI perspective it seems logical to also check if the base
>> can be obtained from the patch range specifier.
>
> If you are doing "format-patch master..my-branch", what do you
> propose to set your base to?  master@{u}, perhaps?

Yes. (I usually use that command with |s|master|origin/master|, so the
argument is the upstream already. A local master branch does not exist for me.)

>

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> So from where are you proposing Git to grab that information if you
>> do not tell it?  "If the HEAD is detached, assume that the base is
>> where it was detached from" or something?
>
> That would also work for me. In my first mail I was proposing to take
> the information from the format-patch argument, such that a one off fix
> would be:
>
>     (1) git checkout origin/master
>     (2) EDIT
>     (3) git commit -a -m "fix"
>     (4) git format-patch origin/master..  # <- This is the information.
>
> However you read it as taking the information from the first line,
> which is also fine with me, as then the (4) can become
>
>     (4a) git format-patch HEAD^

Either would work, but reading from (4) feels a lot less black magic
to me.

>> If you are doing "format-patch master..my-branch", what do you
>> propose to set your base to?  master@{u}, perhaps?
>
> Yes. (I usually use that command with |s|master|origin/master|, so the
> argument is the upstream already. A local master branch does not exist for me.)

Let's hear from folks at Intel ;-) Both of the above sounds like
sensible enhancements to me.

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Ye Xiaolong]

On Tue, Apr 26, 2016 at 11:58:39AM -0700, Junio C Hamano wrote:
>Stefan Beller <sbeller@google.com> writes:
>
>>> So from where are you proposing Git to grab that information if you
>>> do not tell it?  "If the HEAD is detached, assume that the base is
>>> where it was detached from" or something?
>>
>> That would also work for me. In my first mail I was proposing to take
>> the information from the format-patch argument, such that a one off fix
>> would be:
>>
>>     (1) git checkout origin/master
>>     (2) EDIT
>>     (3) git commit -a -m "fix"
>>     (4) git format-patch origin/master..  # <- This is the information.
>>
>> However you read it as taking the information from the first line,
>> which is also fine with me, as then the (4) can become
>>
>>     (4a) git format-patch HEAD^

Thanks for the suggestion, I think this feature would definitely save
effort for end users and help to improve the usage rate of this "--base"
option.

If I understand it correctly, we should try to parse the "revision range"
specified in the fomat-patch cmdline if format.useAutoBase is set and we
couldn't find the remote-tracking branch, if its pattern matches something
like "<branchname>..<rev2>", we will try to get base commit from it:

  - If it is a remote branch, just set its head as base
  - If it isn't, try to set branchname@{u} as base

Please correct me if I'm wrong.

>
>Either would work, but reading from (4) feels a lot less black magic
>to me.
>
>>> If you are doing "format-patch master..my-branch", what do you
>>> propose to set your base to?  master@{u}, perhaps?
>>
>> Yes. (I usually use that command with |s|master|origin/master|, so the
>> argument is the upstream already. A local master branch does not exist for me.)
>
>Let's hear from folks at Intel ;-) Both of the above sounds like
>sensible enhancements to me.

Shall I squash these enhancements in this series, or I need to make
another patch for them?

Thanks,
Xiaolong

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Junio C Hamano]

Ye Xiaolong <xiaolong.ye@intel.com> writes:

> On Tue, Apr 26, 2016 at 11:58:39AM -0700, Junio C Hamano wrote:
>
>>Let's hear from folks at Intel ;-) Both of the above sounds like
>>sensible enhancements to me.
>
> Shall I squash these enhancements in this series, or I need to make
> another patch for them?

The update being discussed is not a "oops, the series without this
update is embarrassingly flawed" fix, but rather "the series is good
enough to be used as-is, but here is to potentially make it even
better", I'd prefer to have it as a separate, follow-up patch that
applies on top of what has been queued.  That way, if this turns out
to be not so good idea, removing it alone while preserving what is
already there will be easier, and also others who will be studying
the code to further enhance the heuristics of base selection in the
future will benefit.

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Wed, Apr 27, 2016 at 8:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Ye Xiaolong <xiaolong.ye@intel.com> writes:
>
>> On Tue, Apr 26, 2016 at 11:58:39AM -0700, Junio C Hamano wrote:
>>
>>>Let's hear from folks at Intel ;-) Both of the above sounds like
>>>sensible enhancements to me.
>>
>> Shall I squash these enhancements in this series, or I need to make
>> another patch for them?
>
> The update being discussed is not a "oops, the series without this
> update is embarrassingly flawed" fix, but rather "the series is good
> enough to be used as-is, but here is to potentially make it even
> better", I'd prefer to have it as a separate, follow-up patch that
> applies on top of what has been queued.  That way, if this turns out
> to be not so good idea, removing it alone while preserving what is
> already there will be easier, and also others who will be studying
> the code to further enhance the heuristics of base selection in the
> future will benefit.

I agree. I was just suggesting going the extra mile to make it easier
for the user. :)

Re: [PATCH v6 0/4] Add --base option to git-format-patch to record base tree info

[Stefan Beller]

On Tue, Apr 26, 2016 at 11:20 AM, Stefan Beller <sbeller@google.com> wrote:
> On Tue, Apr 26, 2016 at 11:05 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I think the way for you to indicate that desire expected by this
>> series is to use "git branch" to set upstream of new-shiny-feature
>> branch to origin/master.  Shouldn't that work, or is that too much
>> work?
>
> I can totally do that for longer series which require some back and forth.
>

So the submodule groups series is an example with some back and forth,
so I'll try to take that workflow with setting an upstream there for now.
As the groups stuff is based on origin/sb/submodule-init I set that as the
remote upstream branch. Upon checking that out I get:

    Switched to branch 'submodule-groups'
    Your branch is ahead of 'origin/sb/submodule-init' by 15 commits.
      (use "git push" to publish your local commits)

The first 2 lines are correct, the third however is not correct. (I cannot push
to your repository, but only email patches)

So I wonder if
 * I configured the wrong upstream branch
 * the upstream branch concept is extended to more/other use cases by the
   format.useAutoBase option. (In an email based workflow you would use the
   a remote branch to a remote, which is not owned by yourself, so the push
   advice is invalid from now on and we patch that message)
 * using an explicit upstream branch is the wrong approach here and the
   base should be implicit, i.e. Take the base sha1 and see if there is
   (one/any) remote branch matching that sha1.
   If there is, use the sha1 just fine.

Thanks,
Stefan