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

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

[Xiaolong Ye]

V3 mainly improves the implementation according to Junio's comments,
Changes vs v2 include:

 - Remove the unnecessary output line "** base-commit-info **".
 
 - Improve the traverse logic to handle not only linear topology, but more
   general cases, it will start revision walk by setting the starting points
   of the traversal to all elements in the rev list[], and skip the ones in 
   list[], only grab the patch-ids of prerequisite patches.

 - If --base=auto is set, it will get merge base of upstream and rev range
   we specified and use it as base commit. If there is no upstream, we just
   error out and suggest to use set-upstream-to to track a remote branch
   as upstream.

v1 can be found here: http://article.gmane.org/gmane.comp.version-control.git/286873
v2 can be found here: http://article.gmane.org/gmane.comp.version-control.git/289603

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.base configuration

 Documentation/git-format-patch.txt |  31 ++++++++++
 builtin/log.c                      | 119 +++++++++++++++++++++++++++++++++++++
 patch-ids.c                        |   2 +-
 patch-ids.h                        |   2 +
 4 files changed, 153 insertions(+), 1 deletion(-)

-- 
2.8.0.4.gcb5a9af

base-commit: 90f7b16b3adc78d4bbabbd426fb69aa78c714f71

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

[Xiaolong Ye]

Make commit_patch_id() available to other builtins.

Helped-by: Junio C Hamano <gitster@pobox.com>
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.0.4.gcb5a9af

[PATCH v3 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 this information at the end of the
_first_ message (either the cover letter or the first patch in the series).

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 | 25 +++++++++++
 builtin/log.c                      | 89 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 114 insertions(+)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6821441..067d562 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -265,6 +265,31 @@ 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 whole tree
+	the patch series applies to. For example, the patch submitter
+	has a commit history of this shape:
+
+	---P---X---Y---Z---A---B---C
+
+	where "P" is the well-known public commit (e.g. one in Linus's tree),
+	"X", "Y", "Z" are prerequisite patches in flight, and "A", "B", "C"
+	are the work being sent out, the submitter could say "git format-patch
+	--base=P -3 C" (or variants thereof, e.g. with "--cover" or using
+	"Z..C" instead of "-3 C" to specify the range), and the identifiers
+	for P, X, Y, Z are appended at the end of the _first_ message (either
+	the cover letter or the first patch in the series).
+
+	For non-linear topology, such as
+
+	    ---P---X---A---M---C
+		\         /
+		 Y---Z---B
+
+	the submitter could 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.
+
 --root::
 	Treat the revision argument as a <revision range>, even if it
 	is just a single commit (that would normally be treated as a
diff --git a/builtin/log.c b/builtin/log.c
index 0d738d6..03cbab0 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1185,6 +1185,82 @@ 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 void prepare_bases(struct base_tree_info *bases,
+			  const char *base_commit,
+			  struct commit **list,
+			  int total)
+{
+	struct commit *base = NULL, *commit;
+	struct rev_info revs;
+	struct diff_options diffopt;
+	struct object_id *patch_id;
+	unsigned char sha1[20];
+	int i;
+
+	diff_setup(&diffopt);
+	DIFF_OPT_SET(&diffopt, RECURSIVE);
+	diff_setup_done(&diffopt);
+
+	base = lookup_commit_reference_by_name(base_commit);
+	if (!base)
+		die(_("Unknown commit %s"), base_commit);
+	oidcpy(&bases->base_commit, &base->object.oid);
+
+	init_revisions(&revs, NULL);
+	revs.max_parents = 1;
+	base->object.flags |= UNINTERESTING;
+	add_pending_object(&revs, &base->object, "base");
+	for (i = 0; i < total; i++) {
+		list[i]->object.flags |= 0;
+		add_pending_object(&revs, &list[i]->object, "rev_list");
+		list[i]->util = (void *)1;
+	}
+
+	if (prepare_revision_walk(&revs))
+		die(_("revision walk setup failed"));
+	/*
+	 * Traverse the prerequisite commits list,
+	 * get the patch ids and stuff them in bases structure.
+	 */
+	while ((commit = get_revision(&revs)) != NULL) {
+		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 = 0; i < bases->nr_patch_id; 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;
@@ -1209,6 +1285,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"),
@@ -1271,6 +1350,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")),
@@ -1507,6 +1588,12 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		signature = strbuf_detach(&buf, NULL);
 	}
 
+	if (base_commit) {
+		memset(&bases, 0, sizeof(bases));
+		reset_revision_walk();
+		prepare_bases(&bases, base_commit, list, nr);
+	}
+
 	if (in_reply_to || thread || cover_letter)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
@@ -1520,6 +1607,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--;
 	}
@@ -1585,6 +1673,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);
-- 
2.8.0.4.gcb5a9af

[PATCH v3 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 |  4 ++++
 builtin/log.c                      | 31 +++++++++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 067d562..d8fe651 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -290,6 +290,10 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
 	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.
+
 --root::
 	Treat the revision argument as a <revision range>, even if it
 	is just a single commit (that would normally be treated as a
diff --git a/builtin/log.c b/builtin/log.c
index 03cbab0..c5efe73 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1200,6 +1200,9 @@ static void prepare_bases(struct base_tree_info *bases,
 	struct rev_info revs;
 	struct diff_options diffopt;
 	struct object_id *patch_id;
+	struct branch *curr_branch;
+	struct commit_list *base_list;
+	const char *upstream;
 	unsigned char sha1[20];
 	int i;
 
@@ -1207,10 +1210,30 @@ static void prepare_bases(struct base_tree_info *bases,
 	DIFF_OPT_SET(&diffopt, RECURSIVE);
 	diff_setup_done(&diffopt);
 
-	base = lookup_commit_reference_by_name(base_commit);
-	if (!base)
-		die(_("Unknown commit %s"), base_commit);
-	oidcpy(&bases->base_commit, &base->object.oid);
+	if (!strcmp(base_commit, "auto")) {
+		curr_branch = branch_get(NULL);
+		upstream = branch_get_upstream(curr_branch, NULL);
+		if (upstream) {
+			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);
+			if (!bases)
+				die(_("Could not find merge base."));
+			base = base_list->item;
+			free_commit_list(base_list);
+			oidcpy(&bases->base_commit, &base->object.oid);
+		} 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);
+		oidcpy(&bases->base_commit, &base->object.oid);
+	}
 
 	init_revisions(&revs, NULL);
 	revs.max_parents = 1;
-- 
2.8.0.4.gcb5a9af

[PATCH v3 4/4] format-patch: introduce format.base configuration

[Xiaolong Ye]

We can set format.base=auto to record the base commit info automatically,
it is equivalent to set --base=auto in cmdline.

The format.base has lower priority than command line option, so if user
set format.base=auto 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/git-format-patch.txt |  2 ++
 builtin/log.c                      | 21 ++++++++++++++-------
 2 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index d8fe651..10149ab 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -293,6 +293,8 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
 	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.
+	If 'format.base=auto' is set in configuration file, it is equivalent
+	to set '--base=auto' in cmdline.
 
 --root::
 	Treat the revision argument as a <revision range>, even if it
diff --git a/builtin/log.c b/builtin/log.c
index c5efe73..821a778 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -699,6 +699,7 @@ static int do_signoff;
 static const char *signature = git_version_string;
 static const char *signature_file;
 static int config_cover_letter;
+static int config_base_commit;
 static const char *config_output_directory;
 
 enum {
@@ -780,6 +781,12 @@ 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.base")){
+		if (value && !strcasecmp(value, "auto")) {
+			config_base_commit = 1;
+			return 0;
+		}
+	}
 
 	return git_log_config(var, value, cb);
 }
@@ -1210,7 +1217,12 @@ static void prepare_bases(struct base_tree_info *bases,
 	DIFF_OPT_SET(&diffopt, RECURSIVE);
 	diff_setup_done(&diffopt);
 
-	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);
+		oidcpy(&bases->base_commit, &base->object.oid);
+	} else if ((base_commit && !strcmp(base_commit, "auto")) || config_base_commit) {
 		curr_branch = branch_get(NULL);
 		upstream = branch_get_upstream(curr_branch, NULL);
 		if (upstream) {
@@ -1228,11 +1240,6 @@ static void prepare_bases(struct base_tree_info *bases,
 			      "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);
-		oidcpy(&bases->base_commit, &base->object.oid);
 	}
 
 	init_revisions(&revs, NULL);
@@ -1611,7 +1618,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		signature = strbuf_detach(&buf, NULL);
 	}
 
-	if (base_commit) {
+	if (base_commit || config_base_commit) {
 		memset(&bases, 0, sizeof(bases));
 		reset_revision_walk();
 		prepare_bases(&bases, base_commit, list, nr);
-- 
2.8.0.4.gcb5a9af

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

[Junio C Hamano]

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

> 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 this information at the end of the
> _first_ message (either the cover letter or the first patch in the series).

You'd need a description of what "base tree info" consists of as a
separate paragraph after the above paragraph.  I'd also suggest to

	s/and append this information/and append it/;

Based on my understanding of what you consider "base tree info", it
may look like this, but you know your design better, so I'd expect
you to rewrite it to be more useful, or at least to fill in the
blanks.

	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 WHAT ORDER???
	before the patches can be applied.

	"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 ???DOING WHAT???

> 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 | 25 +++++++++++
>  builtin/log.c                      | 89 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 114 insertions(+)
>
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 6821441..067d562 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -265,6 +265,31 @@ 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 whole tree
> +	the patch series applies to. For example, the patch submitter
> +	has a commit history of this shape:
> +
> +	---P---X---Y---Z---A---B---C
> +
> +	where "P" is the well-known public commit (e.g. one in Linus's tree),
> +	"X", "Y", "Z" are prerequisite patches in flight, and "A", "B", "C"
> +	are the work being sent out, the submitter could say "git format-patch
> +	--base=P -3 C" (or variants thereof, e.g. with "--cover" or using
> +	"Z..C" instead of "-3 C" to specify the range), and the identifiers
> +	for P, X, Y, Z are appended at the end of the _first_ message (either
> +	the cover letter or the first patch in the series).
> +
> +	For non-linear topology, such as
> +
> +	    ---P---X---A---M---C
> +		\         /
> +		 Y---Z---B
> +
> +	the submitter could 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.

The contents of this look OK, but does it format correctly via
AsciiDoc?  I suspect that only the first paragraph up to "of this
shape:" would appear correctly and all the rest would become funny.

Also the definition of "base tree information" you need to have in
the log message should be given somewhere in this documentation, not
necessarily in the documentation of --base=<commit> option.

Because the use of this new option is not an essential part of
workflow of all users of format-patch, it may be a good idea to have
its own separate section, perhaps between the "DISCUSSION" and
"EXAMPLES" sections, titled "BASE TREE IDENTIFICATION", move the
bulk of text above there with the specification of what "base tree
info" consists of there.

And shorten the description of the option to something like:

--base=<commit>::
	Record the base tree information to identify the state the
	patch series applies to.  See the BASE TREE IDENTIFICATION
        section below for details.

or something.

> diff --git a/builtin/log.c b/builtin/log.c
> index 0d738d6..03cbab0 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1185,6 +1185,82 @@ 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 void prepare_bases(struct base_tree_info *bases,
> +			  const char *base_commit,
> +			  struct commit **list,
> +			  int total)
> +{
> +	struct commit *base = NULL, *commit;
> +	struct rev_info revs;
> +	struct diff_options diffopt;
> +	struct object_id *patch_id;
> +	unsigned char sha1[20];
> +	int i;
> +
> +	diff_setup(&diffopt);
> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
> +	diff_setup_done(&diffopt);
> +
> +	base = lookup_commit_reference_by_name(base_commit);
> +	if (!base)
> +		die(_("Unknown commit %s"), base_commit);
> +	oidcpy(&bases->base_commit, &base->object.oid);
> +
> +	init_revisions(&revs, NULL);
> +	revs.max_parents = 1;
> +	base->object.flags |= UNINTERESTING;
> +	add_pending_object(&revs, &base->object, "base");
> +	for (i = 0; i < total; i++) {
> +		list[i]->object.flags |= 0;

What does this statement do, exactly?  Are you clearing some bits
but not others, and if so which ones?

> +		add_pending_object(&revs, &list[i]->object, "rev_list");
> +		list[i]->util = (void *)1;

Are we sure commit objects not on the list have their ->util cleared?
The while() loop below seems to rely on that to correctly filter out
the ones that are on the list.

> +	}
> +
> +	if (prepare_revision_walk(&revs))
> +		die(_("revision walk setup failed"));
> +	/*
> +	 * Traverse the prerequisite commits list,
> +	 * get the patch ids and stuff them in bases structure.
> +	 */
> +	while ((commit = get_revision(&revs)) != NULL) {
> +		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);

The variable patch_id is used only once here.  Perhaps either write

	hashcpy(bases->patch_id[bases->nr_patch_id]->hash, sha1);

to get rid of the variable, or move its declaration inside the
while() loop to limit its scope?

Has this traversal been told, when setting up the &revs structure,
to show commits in specific order (like "topo order")?  Should it
be?

> +		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 = 0; i < bases->nr_patch_id; i++)
> +		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));

This shows the patches in the order discovered by the revision
traversal, which typically is newer to older.  Is that intended?
Is it assumed that the order of the patches does not matter?

> @@ -1209,6 +1285,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)

The remainder of the patch looks very sensible, including the call
to reset_revision_walk().

Thanks.

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

[Junio C Hamano]

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

> 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.

This line is probably a bit too long.

>
> 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 |  4 ++++
>  builtin/log.c                      | 31 +++++++++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 067d562..d8fe651 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -290,6 +290,10 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
>  	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.
> +
>  --root::
>  	Treat the revision argument as a <revision range>, even if it
>  	is just a single commit (that would normally be treated as a
> diff --git a/builtin/log.c b/builtin/log.c
> index 03cbab0..c5efe73 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1200,6 +1200,9 @@ static void prepare_bases(struct base_tree_info *bases,
>  	struct rev_info revs;
>  	struct diff_options diffopt;
>  	struct object_id *patch_id;
> +	struct branch *curr_branch;
> +	struct commit_list *base_list;
> +	const char *upstream;
>  	unsigned char sha1[20];
>  	int i;
>  
> @@ -1207,10 +1210,30 @@ static void prepare_bases(struct base_tree_info *bases,
>  	DIFF_OPT_SET(&diffopt, RECURSIVE);
>  	diff_setup_done(&diffopt);
>  
> -	base = lookup_commit_reference_by_name(base_commit);
> -	if (!base)
> -		die(_("Unknown commit %s"), base_commit);
> -	oidcpy(&bases->base_commit, &base->object.oid);
> +	if (!strcmp(base_commit, "auto")) {
> +		curr_branch = branch_get(NULL);

Can branch_get() return NULL?  Which ...

> +		upstream = branch_get_upstream(curr_branch, NULL);

... would cause branch_get_upstream() to give you an error (which
you ignore)?  I guess that is OK because upstream will safely be set
to NULL in that case.

> +		if (upstream) {
> +			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);
> +			if (!bases)
> +				die(_("Could not find merge base."));
> +			base = base_list->item;
> +			free_commit_list(base_list);

What should happen when there are multiple merge bases?  The code
picks one at random and ignores the remainder, if I am reading this
correctly.

> +			oidcpy(&bases->base_commit, &base->object.oid);
> +		} 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);
> +		oidcpy(&bases->base_commit, &base->object.oid);
> +	}
>  
>  	init_revisions(&revs, NULL);
>  	revs.max_parents = 1;

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

[Junio C Hamano]

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

> V3 mainly improves the implementation according to Junio's comments,
> Changes vs v2 include:
>
>  - Remove the unnecessary output line "** base-commit-info **".
>  
>  - Improve the traverse logic to handle not only linear topology, but more
>    general cases, it will start revision walk by setting the starting points
>    of the traversal to all elements in the rev list[], and skip the ones in 
>    list[], only grab the patch-ids of prerequisite patches.

This looks much more sensible than the previous ones.  I sent a few
comments on remaining issues separately.


Thanks.

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

[Ye Xiaolong]

On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:
>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>
>> 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 this information at the end of the
>> _first_ message (either the cover letter or the first patch in the series).
>
>You'd need a description of what "base tree info" consists of as a
>separate paragraph after the above paragraph.  I'd also suggest to
>
>	s/and append this information/and append it/;
>
>Based on my understanding of what you consider "base tree info", it
>may look like this, but you know your design better, so I'd expect
>you to rewrite it to be more useful, or at least to fill in the
>blanks.
>
>	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 WHAT ORDER???
>	before the patches can be applied.
>
>	"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 ???DOING WHAT???
>
Thanks for the review.

Ok, I'll polish up the description of base tree info and add it to commit log
as you suggested.

>> 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 | 25 +++++++++++
>>  builtin/log.c                      | 89 ++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 114 insertions(+)
>>
>> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> index 6821441..067d562 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -265,6 +265,31 @@ 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 whole tree
>> +	the patch series applies to. For example, the patch submitter
>> +	has a commit history of this shape:
>> +
>> +	---P---X---Y---Z---A---B---C
>> +
>> +	where "P" is the well-known public commit (e.g. one in Linus's tree),
>> +	"X", "Y", "Z" are prerequisite patches in flight, and "A", "B", "C"
>> +	are the work being sent out, the submitter could say "git format-patch
>> +	--base=P -3 C" (or variants thereof, e.g. with "--cover" or using
>> +	"Z..C" instead of "-3 C" to specify the range), and the identifiers
>> +	for P, X, Y, Z are appended at the end of the _first_ message (either
>> +	the cover letter or the first patch in the series).
>> +
>> +	For non-linear topology, such as
>> +
>> +	    ---P---X---A---M---C
>> +		\         /
>> +		 Y---Z---B
>> +
>> +	the submitter could 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.
>
>The contents of this look OK, but does it format correctly via
>AsciiDoc?  I suspect that only the first paragraph up to "of this
>shape:" would appear correctly and all the rest would become funny.

Sorry, just heard of AsciiDoc, I will try to use it to do the right format work.

>
>Also the definition of "base tree information" you need to have in
>the log message should be given somewhere in this documentation, not
>necessarily in the documentation of --base=<commit> option.
>
>Because the use of this new option is not an essential part of
>workflow of all users of format-patch, it may be a good idea to have
>its own separate section, perhaps between the "DISCUSSION" and
>"EXAMPLES" sections, titled "BASE TREE IDENTIFICATION", move the
>bulk of text above there with the specification of what "base tree
>info" consists of there.
>
>And shorten the description of the option to something like:
>
>--base=<commit>::
>	Record the base tree information to identify the state the
>	patch series applies to.  See the BASE TREE IDENTIFICATION
>        section below for details.
>
>or something.

I'll restructure the descriptions in a resend.

>
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 0d738d6..03cbab0 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1185,6 +1185,82 @@ 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 void prepare_bases(struct base_tree_info *bases,
>> +			  const char *base_commit,
>> +			  struct commit **list,
>> +			  int total)
>> +{
>> +	struct commit *base = NULL, *commit;
>> +	struct rev_info revs;
>> +	struct diff_options diffopt;
>> +	struct object_id *patch_id;
>> +	unsigned char sha1[20];
>> +	int i;
>> +
>> +	diff_setup(&diffopt);
>> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
>> +	diff_setup_done(&diffopt);
>> +
>> +	base = lookup_commit_reference_by_name(base_commit);
>> +	if (!base)
>> +		die(_("Unknown commit %s"), base_commit);
>> +	oidcpy(&bases->base_commit, &base->object.oid);
>> +
>> +	init_revisions(&revs, NULL);
>> +	revs.max_parents = 1;
>> +	base->object.flags |= UNINTERESTING;
>> +	add_pending_object(&revs, &base->object, "base");
>> +	for (i = 0; i < total; i++) {
>> +		list[i]->object.flags |= 0;
>
>What does this statement do, exactly?  Are you clearing some bits
>but not others, and if so which ones?

My mistake, it's useless and should be removed.

>
>> +		add_pending_object(&revs, &list[i]->object, "rev_list");
>> +		list[i]->util = (void *)1;
>
>Are we sure commit objects not on the list have their ->util cleared?
>The while() loop below seems to rely on that to correctly filter out
>the ones that are on the list.
>

I'll need to check it.

>> +	}
>> +
>> +	if (prepare_revision_walk(&revs))
>> +		die(_("revision walk setup failed"));
>> +	/*
>> +	 * Traverse the prerequisite commits list,
>> +	 * get the patch ids and stuff them in bases structure.
>> +	 */
>> +	while ((commit = get_revision(&revs)) != NULL) {
>> +		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);
>
>The variable patch_id is used only once here.  Perhaps either write
>
>	hashcpy(bases->patch_id[bases->nr_patch_id]->hash, sha1);
>
>to get rid of the variable, or move its declaration inside the
>while() loop to limit its scope?
>

Sure, I'll move the patch_id declaration inside the loop.

>Has this traversal been told, when setting up the &revs structure,
>to show commits in specific order (like "topo order")?  Should it
>be?

Thanks for the reminder, this traversal need to be in topo order,
I'll set revs.topo_order to 1 explicitly.

>
>> +		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 = 0; i < bases->nr_patch_id; i++)
>> +		printf("prerequisite-patch-id: %s\n", oid_to_hex(&bases->patch_id[i]));
>
>This shows the patches in the order discovered by the revision
>traversal, which typically is newer to older.  Is that intended?
>Is it assumed that the order of the patches does not matter?

The prerequisite patches should show in topological order, thus robot
could parse them one by one and apply the patches in reverse order.

Thanks,
Xiaolong.
>
>> @@ -1209,6 +1285,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>
>The remainder of the patch looks very sensible, including the call
>to reset_revision_walk().
>
>Thanks.

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

[Ye Xiaolong]

On Thu, Mar 31, 2016 at 10:43:48AM -0700, Junio C Hamano wrote:
>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>
>> 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.
>
>This line is probably a bit too long.

How about simplifying it to "the base_commit is the merge base of upstream and
specified revision-range."?

>
>>
>> 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 |  4 ++++
>>  builtin/log.c                      | 31 +++++++++++++++++++++++++++----
>>  2 files changed, 31 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
>> index 067d562..d8fe651 100644
>> --- a/Documentation/git-format-patch.txt
>> +++ b/Documentation/git-format-patch.txt
>> @@ -290,6 +290,10 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
>>  	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.
>> +
>>  --root::
>>  	Treat the revision argument as a <revision range>, even if it
>>  	is just a single commit (that would normally be treated as a
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 03cbab0..c5efe73 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1200,6 +1200,9 @@ static void prepare_bases(struct base_tree_info *bases,
>>  	struct rev_info revs;
>>  	struct diff_options diffopt;
>>  	struct object_id *patch_id;
>> +	struct branch *curr_branch;
>> +	struct commit_list *base_list;
>> +	const char *upstream;
>>  	unsigned char sha1[20];
>>  	int i;
>>  
>> @@ -1207,10 +1210,30 @@ static void prepare_bases(struct base_tree_info *bases,
>>  	DIFF_OPT_SET(&diffopt, RECURSIVE);
>>  	diff_setup_done(&diffopt);
>>  
>> -	base = lookup_commit_reference_by_name(base_commit);
>> -	if (!base)
>> -		die(_("Unknown commit %s"), base_commit);
>> -	oidcpy(&bases->base_commit, &base->object.oid);
>> +	if (!strcmp(base_commit, "auto")) {
>> +		curr_branch = branch_get(NULL);
>
>Can branch_get() return NULL?  Which ...
>
>> +		upstream = branch_get_upstream(curr_branch, NULL);
>
>... would cause branch_get_upstream() to give you an error (which
>you ignore)?  I guess that is OK because upstream will safely be set
>to NULL in that case.
>
Yes, branch_get_upstream(curr_branch, NULL) will safely return NULL if curr_branch
is NULL, so I think there is no need to add error handling for branch_get().

>> +		if (upstream) {
>> +			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);
>> +			if (!bases)
>> +				die(_("Could not find merge base."));
>> +			base = base_list->item;
>> +			free_commit_list(base_list);
>
>What should happen when there are multiple merge bases?  The code
>picks one at random and ignores the remainder, if I am reading this
>correctly.

If there is more than one merge base, commits in base_list should be sorted by date,
if I am understanding it correctly, so base_list->item should be the lastest
merge base commit, it should be enough for us to used as base commit.

Thanks,
Xiaolong.
>
>> +			oidcpy(&bases->base_commit, &base->object.oid);
>> +		} 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);
>> +		oidcpy(&bases->base_commit, &base->object.oid);
>> +	}
>>  
>>  	init_revisions(&revs, NULL);
>>  	revs.max_parents = 1;

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

[Junio C Hamano]

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

> On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:
>
>>The contents of this look OK, but does it format correctly via
>>AsciiDoc?  I suspect that only the first paragraph up to "of this
>>shape:" would appear correctly and all the rest would become funny.
>
> Sorry, just heard of AsciiDoc, I will try to use it to do the right format work.

Please make sure "make -C Documentation" produces sensible output
for *.1 (manpage) and *.html.

>>> +	init_revisions(&revs, NULL);
>>> +	revs.max_parents = 1;
>>> +	base->object.flags |= UNINTERESTING;
>>> +	add_pending_object(&revs, &base->object, "base");
>>> +	for (i = 0; i < total; i++) {
>>> +		list[i]->object.flags |= 0;
>>
>>What does this statement do, exactly?  Are you clearing some bits
>>but not others, and if so which ones?
>
> My mistake, it's useless and should be removed.

It probably make sense to do "&= ~UNINTERESTING" there, though.  You
are adding one UNINTERESTING object (i.e. the base) and adding
objects that are on the list[] as interesting.

>>This shows the patches in the order discovered by the revision
>>traversal, which typically is newer to older.  Is that intended?
>>Is it assumed that the order of the patches does not matter?
>
> The prerequisite patches should show in topological order, thus robot
> could parse them one by one and apply the patches in reverse order.

If you have history where base is B, with three prerequisites 1-2-3,
before the patch series A-B-C, i.e.

	B---1---2---3---A---B---C

if you are showing "base-commit: B" as the first line in the base
tree information block, it would be natural to expect that the
prerequisite patch ids are listed for 1 and then 2 and then finally
3, i.e.

	base-commit: B
        prerequisite-patch-id: 1
        prerequisite-patch-id: 2
        prerequisite-patch-id: 3

no?

Also I know _you_ intend to consume this by robot, but it makes me
wonder if with a minimum update you can make the output also more
useful for bystander humans.  A mailing list participant may

 - see an early round of a series that interests her,
 - try to apply them to her tree,
 - find that the series does not apply, but
 - sees that a block to help identify to what tree the series is
   meant to apply.

With a list of 40-hex alone, she may not be able to figure out the
prerequisites, but if there is some other clue that helps her to
identify the base commit and these patches, she may be able to
construct a tree that is close enough.  Maybe you can help her by
appending the title of the commit and patches at the end of these
lines?

This is not a strong suggestion (yet); I am thinking aloud at this
point, without knowing how much it would help in practice to do so.

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

[Junio C Hamano]

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

> On Thu, Mar 31, 2016 at 10:43:48AM -0700, Junio C Hamano wrote:
>>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>>
>>> 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.
>>
>>This line is probably a bit too long.
>
> How about simplifying it to "the base_commit is the merge base of upstream and
> specified revision-range."?

What I meant was not that profound.  I just wanted you to wrap your
lines a bit shorter so that quoting in the discussion thread like
this would not make the result overlong to fit on a 80-column
terminal ;-)

>>> +			base = base_list->item;
>>> +			free_commit_list(base_list);
>>
>>What should happen when there are multiple merge bases?  The code
>>picks one at random and ignores the remainder, if I am reading this
>>correctly.
>
> If there is more than one merge base, commits in base_list should
> be sorted by date, if I am understanding it correctly, so
> base_list->item should be the lastest merge base commit, it should
> be enough for us to used as base commit.

By definition, when there are multiple merge bases, there is no
latest one among them.

When the history involves criss-cross merges, there can be more than
one 'best' common ancestor for two commits.  For example, with this
topology (note that X is not a commit; it merely denotes crossing of
two lines):

       ---1---o---A
      /    \ /
  ---O      X
      \    / \
       ---2---o---o---B

both '1' and '2' are merge-bases of 'A' and 'B'.  And the timestamps
on one (be it committer or author timestamp) being later than those
of the other do not make it any more suitable than the other one.

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

[Ye Xiaolong]

On Fri, Apr 01, 2016 at 09:00:20AM -0700, Junio C Hamano wrote:
>Ye Xiaolong <xiaolong.ye@intel.com> writes:
>
>> On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:
>>
>>>The contents of this look OK, but does it format correctly via
>>>AsciiDoc?  I suspect that only the first paragraph up to "of this
>>>shape:" would appear correctly and all the rest would become funny.
>>
>> Sorry, just heard of AsciiDoc, I will try to use it to do the right format work.
>
>Please make sure "make -C Documentation" produces sensible output
>for *.1 (manpage) and *.html.
OK.

>
>>>> +	init_revisions(&revs, NULL);
>>>> +	revs.max_parents = 1;
>>>> +	base->object.flags |= UNINTERESTING;
>>>> +	add_pending_object(&revs, &base->object, "base");
>>>> +	for (i = 0; i < total; i++) {
>>>> +		list[i]->object.flags |= 0;
>>>
>>>What does this statement do, exactly?  Are you clearing some bits
>>>but not others, and if so which ones?
>>
>> My mistake, it's useless and should be removed.
>
>It probably make sense to do "&= ~UNINTERESTING" there, though.  You
>are adding one UNINTERESTING object (i.e. the base) and adding
>objects that are on the list[] as interesting.
Yeah, it does make sense, I'll do the change.

>
>>>This shows the patches in the order discovered by the revision
>>>traversal, which typically is newer to older.  Is that intended?
>>>Is it assumed that the order of the patches does not matter?
>>
>> The prerequisite patches should show in topological order, thus robot
>> could parse them one by one and apply the patches in reverse order.
>
>If you have history where base is B, with three prerequisites 1-2-3,
>before the patch series A-B-C, i.e.
>
>	B---1---2---3---A---B---C
>
>if you are showing "base-commit: B" as the first line in the base
>tree information block, it would be natural to expect that the
>prerequisite patch ids are listed for 1 and then 2 and then finally
>3, i.e.
>
>	base-commit: B
>        prerequisite-patch-id: 1
>        prerequisite-patch-id: 2
>        prerequisite-patch-id: 3
>
>no?
I think this sounds more sensible than what I had thought, I'll adjust 
the showing sequence accordingly.

>
>Also I know _you_ intend to consume this by robot, but it makes me
>wonder if with a minimum update you can make the output also more
>useful for bystander humans.  A mailing list participant may
>
> - see an early round of a series that interests her,
> - try to apply them to her tree,
> - find that the series does not apply, but
> - sees that a block to help identify to what tree the series is
>   meant to apply.
>
>With a list of 40-hex alone, she may not be able to figure out the
>prerequisites, but if there is some other clue that helps her to
>identify the base commit and these patches, she may be able to
>construct a tree that is close enough.  Maybe you can help her by
>appending the title of the commit and patches at the end of these
>lines?
>
>This is not a strong suggestion (yet); I am thinking aloud at this
>point, without knowing how much it would help in practice to do so.
Thanks for the suggestions, actually it is what we have planned for next
step to make the output info more friendly for human being, I'll
follow up on it.

Thanks,
Xiaolong.

>--
>To unsubscribe from this list: send the line "unsubscribe git" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Ye Xiaolong]

On Fri, Apr 01, 2016 at 09:06:20AM -0700, Junio C Hamano wrote:
>Ye Xiaolong <xiaolong.ye@intel.com> writes:
>
>> On Thu, Mar 31, 2016 at 10:43:48AM -0700, Junio C Hamano wrote:
>>>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>>>
>>>> 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.
>>>
>>>This line is probably a bit too long.
>>
>> How about simplifying it to "the base_commit is the merge base of upstream and
>> specified revision-range."?
>
>What I meant was not that profound.  I just wanted you to wrap your
>lines a bit shorter so that quoting in the discussion thread like
>this would not make the result overlong to fit on a 80-column
>terminal ;-)

Emm, get your point now, I'll shorten the lines to fit in 80-column.

>
>>>> +			base = base_list->item;
>>>> +			free_commit_list(base_list);
>>>
>>>What should happen when there are multiple merge bases?  The code
>>>picks one at random and ignores the remainder, if I am reading this
>>>correctly.
>>
>> If there is more than one merge base, commits in base_list should
>> be sorted by date, if I am understanding it correctly, so
>> base_list->item should be the lastest merge base commit, it should
>> be enough for us to used as base commit.
>
>By definition, when there are multiple merge bases, there is no
>latest one among them.
>
>When the history involves criss-cross merges, there can be more than
>one 'best' common ancestor for two commits.  For example, with this
>topology (note that X is not a commit; it merely denotes crossing of
>two lines):
>
>       ---1---o---A
>      /    \ /
>  ---O      X
>      \    / \
>       ---2---o---o---B
>
>both '1' and '2' are merge-bases of 'A' and 'B'.  And the timestamps
>on one (be it committer or author timestamp) being later than those
>of the other do not make it any more suitable than the other one.
>

For this criss-cross merges, as neither merge base(like 1) is better
than the other(both 1 and 2 are 'best' merge bases), I think it should
be fine to pick a random one as base commit(Or you prefer to show all of
them?) and I'll add this part of discusstion into documentation.

Thanks,
Xiaolong.

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

[Junio C Hamano]

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

>       ---1---o---A
>      /    \ /
>  ---O      X
>      \    / \
>       ---2---o---o---B
>
> For this criss-cross merges, as neither merge base(like 1) is better
> than the other(both 1 and 2 are 'best' merge bases), I think it should
> be fine to pick a random one as base commit(Or you prefer to show all of
> them?) and I'll add this part of discusstion into documentation.

I think you should error out; it would give you blatantly wrong to
pick either one at random.

Suppose A is where your remote-tracking branch is, and B is where
the user started working on her serie.  We are sending patches built
on top of B (not depicted) with "format-patch --base=A B..".

If you picked '1' as the base, you'll include commits on the ===
stretch as prerequisite patches (think of any non-merge --o-- in the
picture to consist of multiple commits), but you won't be showing
what the merge 'M' between '1' and '2' did to the tree of '1' to
arrive at the resulting tree of 'M'.

       ---1---o---A
      /    \ /
  ---O      X
      \    / \
       ---2---M===o===B...your.patches.here...

If you picked '2' as the base, you'll include commits on the ===
stretch as prerequisite patches, but again you won't be showing what
the merge 'M' between '1' and '2' did to the tree of '2' to arrive
at the resulting tree of 'M'..

       ---1---o---A
      /    \ /
  ---O      X
      \    / \
       ---2===M===o===B...your.patches.here...

So either case, you cannot rebuild the tree of B by going from the
base and piling on patches in such a case, because you won't have
"patch" for merge 'M'.

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

[Ye Xiaolong]

On Thu, Mar 31, 2016 at 10:38:04AM -0700, Junio C Hamano wrote:
>> diff --git a/builtin/log.c b/builtin/log.c
>> index 0d738d6..03cbab0 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c
>> @@ -1185,6 +1185,82 @@ 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 void prepare_bases(struct base_tree_info *bases,
>> +			  const char *base_commit,
>> +			  struct commit **list,
>> +			  int total)
>> +{
>> +	struct commit *base = NULL, *commit;
>> +	struct rev_info revs;
>> +	struct diff_options diffopt;
>> +	struct object_id *patch_id;
>> +	unsigned char sha1[20];
>> +	int i;
>> +
>> +	diff_setup(&diffopt);
>> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
>> +	diff_setup_done(&diffopt);
>> +
>> +	base = lookup_commit_reference_by_name(base_commit);
>> +	if (!base)
>> +		die(_("Unknown commit %s"), base_commit);
>> +	oidcpy(&bases->base_commit, &base->object.oid);
>> +
>> +	init_revisions(&revs, NULL);
>> +	revs.max_parents = 1;
>> +	base->object.flags |= UNINTERESTING;
>> +	add_pending_object(&revs, &base->object, "base");
>> +	for (i = 0; i < total; i++) {
>> +		list[i]->object.flags |= 0;
>
>What does this statement do, exactly?  Are you clearing some bits
>but not others, and if so which ones?
>
>> +		add_pending_object(&revs, &list[i]->object, "rev_list");
>> +		list[i]->util = (void *)1;
>
>Are we sure commit objects not on the list have their ->util cleared?
>The while() loop below seems to rely on that to correctly filter out
>the ones that are on the list.
>
After some investigation and according to my understanding, the commit
object is allocated through alloc_commit_node->alloc_node, 

void *alloc_commit_node(void)
{
	struct commit *c = alloc_node(&commit_state, sizeof(struct commit));
	c->object.type = OBJ_COMMIT;
	c->index = alloc_commit_index();
	return c;
}

static inline void *alloc_node(struct alloc_state *s, size_t node_size)
{
	void *ret;

	if (!s->nr) {
		s->nr = BLOCKING;
		s->p = xmalloc(BLOCKING * node_size);
	}
	s->nr--;
	s->count++;
	ret = s->p;
	s->p = (char *)s->p + node_size;
	memset(ret, 0, node_size);
	return ret;
}

So the commit->util should be cleared after initialization, and it has
not been touched except above "for" loop in our code execution path, I think
it is safe to rely on it to filter out commits that are on the rev list.

Thanks,
Xiaolong.
>> +	}
>> +
>> +	if (prepare_revision_walk(&revs))
>> +		die(_("revision walk setup failed"));
>> +	/*
>> +	 * Traverse the prerequisite commits list,
>> +	 * get the patch ids and stuff them in bases structure.
>> +	 */
>> +	while ((commit = get_revision(&revs)) != NULL) {
>> +		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);
>