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

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

[Xiaolong Ye]

This is a re-roll of 

   http://article.gmane.org/gmane.comp.version-control.git/286873

It is almost a rewrite of v1, Changes include:

 - Make commit_patch_id() a publib helper function, so it can be available to other modules.

 - The new option "--base" will not only record the base commit id(a public well-known commit),
   but also the prerequisite patches's patch ids, then the 0day test robot could get these info,
   thus to checkout the well-known base tree-ish, apply the prerequisite patches and then the
   patches being send can be applied to correct tree to be evaluated. (0day catches every patch
   series to LKML and maintains a patch-id => commit-id database for in-flight patches.)

 - For developers' convenience, we propose a new format.base configuration to track the base commit
   automatically, if current branch the developer work on has its remote-tracking branch,
   the base commit would be the tip commit of the remote branch, if current branch is not tracked
   with any remote branch(eg. checkout by a local branch or a commit), thus its upstream could not
   be obtained, it will just record the parent commit-id of the patch series, as well as patch-id for
   reference.

   so here is the text UI:

   1) for the cases that exact base commit could be obtained(eithor from manual input --base=<base-commit-id>
	   or from upstream if base=auto is set) 

	 ** base-commit-info **
	 base-commit: ab5d01a29eb7380ceab070f0807c2939849c44bc
	 prerequisite-patch-id: 61400f965fdc0c2fbe8ad9cb5316c3efe6e05c14
	 prerequisite-patch-id: 063a398ef398647d8d839e6f9090d38ea9e0551c

   2) for cases that exact base commit is failed to obtained
	
	 ** parent-commit-info **
	 parent-commit: ab5d01a29eb7380ceab070f0807c2939849c44bc
	 parent-patch-id: caf2dae24db5b4c49a6c4134dd6d9908e898424b

Thanks for fengguang and junio's suggestions and prototype of implementation.

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

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

-- 
2.8.0.rc4.4.ga41a987

** base-commit-info **
base-commit: 808ecd4cca75acac5e4868f15d3e647fc73698d3

[PATCH v2 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.0.rc4.4.ga41a987

[PATCH v2 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).

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 Documentation/git-format-patch.txt | 15 ++++++
 builtin/log.c                      | 98 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 6821441..a5f145e 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -265,6 +265,21 @@ 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 would say "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).
+
 --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..d3804b3 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1185,6 +1185,88 @@ 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 *prerequisite_head)
+{
+	struct commit *base = NULL, *commit;
+	struct rev_info revs;
+	struct diff_options diffopt;
+	struct object_id *patch_id;
+	unsigned char sha1[20];
+	int pos = 0;
+
+	if (!prerequisite_head)
+		return;
+	base = lookup_commit_reference_by_name(base_commit);
+	if (!base)
+		die(_("Unknown commit %s"), base_commit);
+	oidcpy(&bases->base_commit, &base->object.oid);
+
+	if (base == prerequisite_head)
+		return;
+
+	if (!in_merge_bases(base, prerequisite_head))
+		die(_("base commit should be the ancestor of revs you specified"));
+
+	init_revisions(&revs, NULL);
+	revs.max_parents = 1;
+
+	base->object.flags |= UNINTERESTING;
+	add_pending_object(&revs, &base->object, "base");
+	prerequisite_head->object.flags |= 0;
+	add_pending_object(&revs, &prerequisite_head->object, "prerequisite-head");
+
+	diff_setup(&diffopt);
+	DIFF_OPT_SET(&diffopt, RECURSIVE);
+	diff_setup_done(&diffopt);
+
+	if (prepare_revision_walk(&revs))
+		die(_("revision walk setup failed"));
+	/*
+	 * Traverse the commits list between base and prerequisite head,
+	 * get the patch ids and stuff them in bases structure.
+	 */
+	while ((commit = get_revision(&revs)) != NULL) {
+		if (commit_patch_id(commit, &diffopt, sha1))
+			return;
+		ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
+		patch_id = bases->patch_id + pos;
+		hashcpy(patch_id->hash, sha1);
+		pos++;
+		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;
+
+	printf("** base-commit-info **\n");
+
+	/* 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 +1291,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 +1356,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 +1594,15 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		signature = strbuf_detach(&buf, NULL);
 	}
 
+	if (base_commit) {
+		struct commit *prerequisite_head = NULL;
+		if (list[nr - 1]->parents)
+			prerequisite_head = list[nr - 1]->parents->item;
+		memset(&bases, 0, sizeof(bases));
+		reset_revision_walk();
+		prepare_bases(&bases, base_commit, prerequisite_head);
+	}
+
 	if (in_reply_to || thread || cover_letter)
 		rev.ref_message_ids = xcalloc(1, sizeof(struct string_list));
 	if (in_reply_to) {
@@ -1520,6 +1616,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 +1682,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.rc4.4.ga41a987

[PATCH v2 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 tip commit of the upstream branch. If upstream branch cannot be determined,
it will just record the parent's commit id and patch id.

Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
---
 builtin/log.c | 70 +++++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 51 insertions(+), 19 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index d3804b3..e8a3964 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1187,6 +1187,8 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 
 struct base_tree_info {
 	struct object_id base_commit;
+	struct object_id parent_commit;
+	struct object_id parent_patch_id;
 	int nr_patch_id, alloc_patch_id;
 	struct object_id *patch_id;
 };
@@ -1199,15 +1201,38 @@ 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;
+	const char *upstream;
 	unsigned char sha1[20];
 	int pos = 0;
 
 	if (!prerequisite_head)
 		return;
-	base = lookup_commit_reference_by_name(base_commit);
-	if (!base)
-		die(_("Unknown commit %s"), base_commit);
-	oidcpy(&bases->base_commit, &base->object.oid);
+
+	diff_setup(&diffopt);
+	DIFF_OPT_SET(&diffopt, RECURSIVE);
+	diff_setup_done(&diffopt);
+
+	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);
+			base = lookup_commit_or_die(sha1, "upstream base");
+			oidcpy(&bases->base_commit, &base->object.oid);
+		} else {
+			commit_patch_id(prerequisite_head, &diffopt, sha1);
+			oidcpy(&bases->parent_commit, &prerequisite_head->object.oid);
+			hashcpy(bases->parent_patch_id.hash, sha1);
+			return;
+		}
+	} else {
+		base = lookup_commit_reference_by_name(base_commit);
+		if (!base)
+			die(_("Unknown commit %s"), base_commit);
+		oidcpy(&bases->base_commit, &base->object.oid);
+	}
 
 	if (base == prerequisite_head)
 		return;
@@ -1223,10 +1248,6 @@ static void prepare_bases(struct base_tree_info *bases,
 	prerequisite_head->object.flags |= 0;
 	add_pending_object(&revs, &prerequisite_head->object, "prerequisite-head");
 
-	diff_setup(&diffopt);
-	DIFF_OPT_SET(&diffopt, RECURSIVE);
-	diff_setup_done(&diffopt);
-
 	if (prepare_revision_walk(&revs))
 		die(_("revision walk setup failed"));
 	/*
@@ -1249,22 +1270,33 @@ 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))
+	if (is_null_oid(&bases->base_commit) && is_null_oid(&bases->parent_commit))
 		return;
 
-	printf("** base-commit-info **\n");
+	if (!is_null_oid(&bases->base_commit)) {
+		printf("** base-commit-info **\n");
+
+		/* Show the base commit */
+		printf("base-commit: %s\n", oid_to_hex(&bases->base_commit));
 
-	/* 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]));
 
-	/* 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);
+	} else if (!is_null_oid(&bases->parent_commit)) {
+		printf("** parent-commit-info **\n");
 
-	free(bases->patch_id);
-	bases->nr_patch_id = 0;
-	bases->alloc_patch_id = 0;
-	oidclr(&bases->base_commit);
+		/* Show the parent commit */
+		printf("parent-commit: %s\n", oid_to_hex(&bases->parent_commit));
+		/* Show the parent patch id */
+		printf("parent-patch-id: %s\n", oid_to_hex(&bases->parent_patch_id));
+
+		oidclr(&bases->parent_commit);
+	}
 }
 
 int cmd_format_patch(int argc, const char **argv, const char *prefix)
-- 
2.8.0.rc4.4.ga41a987

[PATCH v2 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 |  5 +++++
 builtin/log.c                      | 21 ++++++++++++++-------
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index a5f145e..7c7a61a 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -279,6 +279,11 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
 	"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).
+	If 'format.base=auto' is set in configuration file, it is equivalent
+	to set '--base=auto' in cmdline, it will track the base commit
+	automatically, the base commit will be the tip commit of upstream
+	branch, if upstream branch cannot be determined, it will just record
+	the parent's commit id and patch id.
 
 --root::
 	Treat the revision argument as a <revision range>, even if it
diff --git a/builtin/log.c b/builtin/log.c
index e8a3964..fc468ce 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);
 }
@@ -1213,7 +1220,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) {
@@ -1227,11 +1239,6 @@ static void prepare_bases(struct base_tree_info *bases,
 			hashcpy(bases->parent_patch_id.hash, sha1);
 			return;
 		}
-	} else {
-		base = lookup_commit_reference_by_name(base_commit);
-		if (!base)
-			die(_("Unknown commit %s"), base_commit);
-		oidcpy(&bases->base_commit, &base->object.oid);
 	}
 
 	if (base == prerequisite_head)
@@ -1626,7 +1633,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) {
 		struct commit *prerequisite_head = NULL;
 		if (list[nr - 1]->parents)
 			prerequisite_head = list[nr - 1]->parents->item;
-- 
2.8.0.rc4.4.ga41a987

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

[Junio C Hamano]

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

Reviewing the patch out of order, caller first and then callee.

> +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;
> +
> +	printf("** base-commit-info **\n");

I am not sure if you want to have this line (an empty line might not
hurt), as the "base-commit: ..." that comes next is a clear enough
indication of what these lines are.

> +	if (base_commit) {
> +		struct commit *prerequisite_head = NULL;
> +		if (list[nr - 1]->parents)
> +			prerequisite_head = list[nr - 1]->parents->item;
> +		memset(&bases, 0, sizeof(bases));
> +		reset_revision_walk();
> +		prepare_bases(&bases, base_commit, prerequisite_head);
> +	}
> +

list[] holds the commits in reverse topological order, so the first
parent of the last element in the list[] would hopefully give you
the latest change your series depends on, and that is why you are
working on list[nr - 1] here.

I however think that is flawed.

What if your series A, B and C are on this topology?

    ---P---X---A---M---C
        \         /
         Y---Z---B

"git format-patch --base=P -3 C" would show A, B and C.  It may show
B, A and C, as A and B are independent (you would be flattening the
history into the shape you have in your documentation part of the
patch in order to adjust for their interactions before running
format-patch if that were not the case).  Depending on which one
happens to be chosen as prerequisite_head, you would miss either X
or Y & Z, wouldn't you?

A simpler and safer (but arguably less useful) approach may be to
use the logic and limitation of your patch as-is but add code to
check that the history is linear and refuse to do the "base" thing.
But just in case you want to handle a more general case like the
above, read on.

> +static void prepare_bases(struct base_tree_info *bases,
> +			  const char *base_commit,
> +			  struct commit *prerequisite_head)
> +{
> +	struct commit *base = NULL, *commit;
> +	struct rev_info revs;
> +	struct diff_options diffopt;
> +	struct object_id *patch_id;
> +	unsigned char sha1[20];
> +	int pos = 0;
> +
> +	if (!prerequisite_head)
> +		return;
> +	base = lookup_commit_reference_by_name(base_commit);
> +	if (!base)
> +		die(_("Unknown commit %s"), base_commit);
> +	oidcpy(&bases->base_commit, &base->object.oid);
> +
> +	if (base == prerequisite_head)
> +		return;
> +
> +	if (!in_merge_bases(base, prerequisite_head))
> +		die(_("base commit should be the ancestor of revs you specified"));
> +
> +	init_revisions(&revs, NULL);
> +	revs.max_parents = 1;
> +
> +	base->object.flags |= UNINTERESTING;
> +	add_pending_object(&revs, &base->object, "base");
> +	prerequisite_head->object.flags |= 0;
> +	add_pending_object(&revs, &prerequisite_head->object, "prerequisite-head");
> +
> +	diff_setup(&diffopt);
> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
> +	diff_setup_done(&diffopt);
> +
> +	if (prepare_revision_walk(&revs))
> +		die(_("revision walk setup failed"));
> +	/*
> +	 * Traverse the commits list between base and prerequisite head,
> +	 * get the patch ids and stuff them in bases structure.
> +	 */
> +	while ((commit = get_revision(&revs)) != NULL) {
> +		if (commit_patch_id(commit, &diffopt, sha1))
> +			return;
> +		ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
> +		patch_id = bases->patch_id + pos;
> +		hashcpy(patch_id->hash, sha1);
> +		pos++;
> +		bases->nr_patch_id++;

Micronit.  Aren't pos and nr_patch_id always the same?

> +	}
> +}

I think the right thing to do is probably to start another revision
walk (which you do) but setting the starting points of the traversal
to all elements in the list[] (which you don't--you use either A^ or
B^).  And skip the ones in the list[] before grabbing its patch-id
in the loop.  That way, you do not have to worry about the topology
of the history tripping you up at all.

So I'd suggest to redo this function perhaps like so, if you do want
to handle the more general case:

static void prepare_bases(struct base_tree_info *bases,
			  const char *base_commit,
			  struct commit **list,
                          int total)
{
	... boilerplate ...

	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;
	add_pending_commit(&revs, base, UNINTERESTING);
	for (i = 0; i < total; i++)
        	add_pending_commit(&revs, list[i], 0);

	if (prepare_revision_walk(&revs))
		die(_("revision walk setup failed"));

	while ((commit = get_revision(&revs)) != NULL) {
        	if (COMMIT_IS_IN_LIST(commit))
                	continue;
		if (commit_patch_id(commit, &diffopt, sha1))
                	die("cannot get patch id");
		... do your ptach_id thing ...
	}
}

And COMMIT_IS_IN_LIST() can probably be implemented by using commit->util
field, e.g. change the part that sets up the traversal

	for (i = 0; i < total; i++) {
        	add_pending_commit(&revs, list[i], 0);
		list[i]->util = (void *)1;
	}

to mark those in list[] as such, and the test would be

		if (commit->util)
                	continue; /* on list[] */

or something like that.

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

[Junio C Hamano]

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

> +
> +	diff_setup(&diffopt);
> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
> +	diff_setup_done(&diffopt);

It is annoying that you moved "diff" stuff here (if it can be
initialized once at the beginning and then reused over and over,
it should have been done here from the beginning at PATCH 2/4).

> +	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);
> +			base = lookup_commit_or_die(sha1, "upstream base");
> +			oidcpy(&bases->base_commit, &base->object.oid);
> +		} else {
> +			commit_patch_id(prerequisite_head, &diffopt, sha1);
> +			oidcpy(&bases->parent_commit, &prerequisite_head->object.oid);
> +			hashcpy(bases->parent_patch_id.hash, sha1);
> +			return;

What happens if you did this sequence?

	$ git fetch origin
        $ git checkout -b fork origin/master
        $ git fetch origin
        $ git format-patch --base=auto origin..

You grab the updated origin/master as base and use it here, no?
At that point the topology would look like:

          1---2---3 updated upstream
         /
	0---X---Y---Z---A---B---C
        ^
        old upstream

so you are basing your worn on "0" (old upstream) but setting base
to "3"

Wouldn't that trigger "base must be an ancestor of Z" check you had
in [PATCH 2/4]?

I also do not see the point of showing "parent id" which as far as I
can see is just a random commit object name and show different
output that is not even described what it is.  It would be better to

 * find the upstream (i.e. 3 in the picture) and then with our range
   (i.e. A B and C) compute the merge base (i.e. you would find 0)
   and use it as base;

 * if there is no upstream, error out and tell the user that there
   is no upstream.  The user is intelligent enough and knows what
   commit the base should be.

I suspect, but I didn't think things through.

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

[Ye Xiaolong]

On Wed, Mar 23, 2016 at 11:08:20AM -0700, Junio C Hamano wrote:
>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>
>Reviewing the patch out of order, caller first and then callee.
>
>> +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;
>> +
>> +	printf("** base-commit-info **\n");
>
>I am not sure if you want to have this line (an empty line might not
>hurt), as the "base-commit: ..." that comes next is a clear enough
>indication of what these lines are.

Ok, I will remove this extra line.
>
>> +	if (base_commit) {
>> +		struct commit *prerequisite_head = NULL;
>> +		if (list[nr - 1]->parents)
>> +			prerequisite_head = list[nr - 1]->parents->item;
>> +		memset(&bases, 0, sizeof(bases));
>> +		reset_revision_walk();
>> +		prepare_bases(&bases, base_commit, prerequisite_head);
>> +	}
>> +
>
>list[] holds the commits in reverse topological order, so the first
>parent of the last element in the list[] would hopefully give you
>the latest change your series depends on, and that is why you are
>working on list[nr - 1] here.

Yes, I just considered linear topology before.
>
>I however think that is flawed.
>
>What if your series A, B and C are on this topology?
>
>    ---P---X---A---M---C
>        \         /
>         Y---Z---B
>
>"git format-patch --base=P -3 C" would show A, B and C.  It may show
>B, A and C, as A and B are independent (you would be flattening the
>history into the shape you have in your documentation part of the
>patch in order to adjust for their interactions before running
>format-patch if that were not the case).  Depending on which one
>happens to be chosen as prerequisite_head, you would miss either X
>or Y & Z, wouldn't you?
>
>A simpler and safer (but arguably less useful) approach may be to
>use the logic and limitation of your patch as-is but add code to
>check that the history is linear and refuse to do the "base" thing.
>But just in case you want to handle a more general case like the
>above, read on.
>
>> +static void prepare_bases(struct base_tree_info *bases,
>> +			  const char *base_commit,
>> +			  struct commit *prerequisite_head)
>> +{
>> +	struct commit *base = NULL, *commit;
>> +	struct rev_info revs;
>> +	struct diff_options diffopt;
>> +	struct object_id *patch_id;
>> +	unsigned char sha1[20];
>> +	int pos = 0;
>> +
>> +	if (!prerequisite_head)
>> +		return;
>> +	base = lookup_commit_reference_by_name(base_commit);
>> +	if (!base)
>> +		die(_("Unknown commit %s"), base_commit);
>> +	oidcpy(&bases->base_commit, &base->object.oid);
>> +
>> +	if (base == prerequisite_head)
>> +		return;
>> +
>> +	if (!in_merge_bases(base, prerequisite_head))
>> +		die(_("base commit should be the ancestor of revs you specified"));
>> +
>> +	init_revisions(&revs, NULL);
>> +	revs.max_parents = 1;
>> +
>> +	base->object.flags |= UNINTERESTING;
>> +	add_pending_object(&revs, &base->object, "base");
>> +	prerequisite_head->object.flags |= 0;
>> +	add_pending_object(&revs, &prerequisite_head->object, "prerequisite-head");
>> +
>> +	diff_setup(&diffopt);
>> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
>> +	diff_setup_done(&diffopt);
>> +
>> +	if (prepare_revision_walk(&revs))
>> +		die(_("revision walk setup failed"));
>> +	/*
>> +	 * Traverse the commits list between base and prerequisite head,
>> +	 * get the patch ids and stuff them in bases structure.
>> +	 */
>> +	while ((commit = get_revision(&revs)) != NULL) {
>> +		if (commit_patch_id(commit, &diffopt, sha1))
>> +			return;
>> +		ALLOC_GROW(bases->patch_id, bases->nr_patch_id + 1, bases->alloc_patch_id);
>> +		patch_id = bases->patch_id + pos;
>> +		hashcpy(patch_id->hash, sha1);
>> +		pos++;
>> +		bases->nr_patch_id++;
>
>Micronit.  Aren't pos and nr_patch_id always the same?
Sorry, will only use nr_patch_id.
>
>> +	}
>> +}
>
>I think the right thing to do is probably to start another revision
>walk (which you do) but setting the starting points of the traversal
>to all elements in the list[] (which you don't--you use either A^ or
>B^).  And skip the ones in the list[] before grabbing its patch-id
>in the loop.  That way, you do not have to worry about the topology
>of the history tripping you up at all.
>
>So I'd suggest to redo this function perhaps like so, if you do want
>to handle the more general case:

Thanks for the elaborated suggestions. I will redo the prepare_bases
function accordingly to handle more general case.
>
>static void prepare_bases(struct base_tree_info *bases,
>			  const char *base_commit,
>			  struct commit **list,
>                          int total)
>{
>	... boilerplate ...
>
>	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;
>	add_pending_commit(&revs, base, UNINTERESTING);
>	for (i = 0; i < total; i++)
>        	add_pending_commit(&revs, list[i], 0);
>
>	if (prepare_revision_walk(&revs))
>		die(_("revision walk setup failed"));
>
>	while ((commit = get_revision(&revs)) != NULL) {
>        	if (COMMIT_IS_IN_LIST(commit))
>                	continue;
>		if (commit_patch_id(commit, &diffopt, sha1))
>                	die("cannot get patch id");
>		... do your ptach_id thing ...
>	}
>}
>
>And COMMIT_IS_IN_LIST() can probably be implemented by using commit->util
>field, e.g. change the part that sets up the traversal
>
>	for (i = 0; i < total; i++) {
>        	add_pending_commit(&revs, list[i], 0);
>		list[i]->util = (void *)1;
>	}
>
>to mark those in list[] as such, and the test would be
>
>		if (commit->util)
>                	continue; /* on list[] */
>
>or something like that.

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

[Ye Xiaolong]

On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:
>Xiaolong Ye <xiaolong.ye@intel.com> writes:
>
>> +
>> +	diff_setup(&diffopt);
>> +	DIFF_OPT_SET(&diffopt, RECURSIVE);
>> +	diff_setup_done(&diffopt);
>
>It is annoying that you moved "diff" stuff here (if it can be
>initialized once at the beginning and then reused over and over,
>it should have been done here from the beginning at PATCH 2/4).

My bad, I will put "diff" stuff at the beginning at PATCH 2/4.
>
>> +	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);
>> +			base = lookup_commit_or_die(sha1, "upstream base");
>> +			oidcpy(&bases->base_commit, &base->object.oid);
>> +		} else {
>> +			commit_patch_id(prerequisite_head, &diffopt, sha1);
>> +			oidcpy(&bases->parent_commit, &prerequisite_head->object.oid);
>> +			hashcpy(bases->parent_patch_id.hash, sha1);
>> +			return;
>
>What happens if you did this sequence?
>
>	$ git fetch origin
>        $ git checkout -b fork origin/master
>        $ git fetch origin
>        $ git format-patch --base=auto origin..
>
>You grab the updated origin/master as base and use it here, no?
>At that point the topology would look like:
>
>          1---2---3 updated upstream
>         /
>	0---X---Y---Z---A---B---C
>        ^
>        old upstream
>
>so you are basing your worn on "0" (old upstream) but setting base
>to "3"
>
>Wouldn't that trigger "base must be an ancestor of Z" check you had
>in [PATCH 2/4]?

Yes, this is flawed, I will follow your below suggestion to compute
the merge base as the base commit through upstream and specified range.

>
>I also do not see the point of showing "parent id" which as far as I
>can see is just a random commit object name and show different
>output that is not even described what it is.  It would be better to

Here is our consideration:
There is high chance that branch_get_upstream will retrun NULL(thus we
are not able to get exact base commit), since developers may checkout
branch from a local branch or a commit and haven't set "--set-upstream-to"
to track a remote branch, in this case, we want to provide likely useful
info(here is parent commit id and patch id), based on it, 0day robot still
have good chance to find the suitable base.
Otherwise, I'm afraid this annotation system won't work effectively in long run.

Thanks,
Xiaolong.
>
> * find the upstream (i.e. 3 in the picture) and then with our range
>   (i.e. A B and C) compute the merge base (i.e. you would find 0)
>   and use it as base;
>
> * if there is no upstream, error out and tell the user that there
>   is no upstream.  The user is intelligent enough and knows what
>   commit the base should be.
>
>I suspect, but I didn't think things through.
>
>
>--
>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 v2 3/4] format-patch: introduce --base=auto option

[Junio C Hamano]

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

> On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:
>>I also do not see the point of showing "parent id" which as far as I
>>can see is just a random commit object name and show different
>>output that is not even described what it is.  It would be better to
>
> Here is our consideration:
> There is high chance that branch_get_upstream will retrun NULL(thus we
> are not able to get exact base commit), since developers may checkout
> branch from a local branch or a commit and haven't set "--set-upstream-to"
> to track a remote branch, in this case, we want to provide likely useful
> info(here is parent commit id and patch id)

I do not agree with that reasoning at all, primarily because your
"likely useful" is not justfied.

Could you explain what makes you think that it is "likely" that the
commit that matches "parent commit id" is available to the recipient
of the patch?

Whatever the reason is, if it _is_ likely, then I do not see a point
in spending cycle to do get-upstream and merge-base, or giving an
option to the user to explicitly specify the base.  Given that this
series does these things, I'd have to conclude your "likely useful"
is "might possibly turn out to be useful in some cases if you are
lucky but is no way reliable" at best.

Rather than throwing an unreliable information in the output and
blindly proceeding, I'd think you would want to error out and tell
the user to explicitly give the base without producing the patch
output.  That way, you will not get patches with unreliable
information.

Suggesting to use set-upstream-to when you give that error message
may also be helpful.

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

[Ye Xiaolong]

On Thu, Mar 24, 2016 at 10:01:58AM -0700, Junio C Hamano wrote:
>Ye Xiaolong <xiaolong.ye@intel.com> writes:
>
>> On Wed, Mar 23, 2016 at 11:25:41AM -0700, Junio C Hamano wrote:
>>>I also do not see the point of showing "parent id" which as far as I
>>>can see is just a random commit object name and show different
>>>output that is not even described what it is.  It would be better to
>>
>> Here is our consideration:
>> There is high chance that branch_get_upstream will retrun NULL(thus we
>> are not able to get exact base commit), since developers may checkout
>> branch from a local branch or a commit and haven't set "--set-upstream-to"
>> to track a remote branch, in this case, we want to provide likely useful
>> info(here is parent commit id and patch id)
>
>I do not agree with that reasoning at all, primarily because your
>"likely useful" is not justfied.
>
>Could you explain what makes you think that it is "likely" that the
>commit that matches "parent commit id" is available to the recipient
>of the patch?
>

Hi, Junio

Still want to discuss with you about the proposal that showing the "parent
commit-id as well as patch-id" when exact base commit is failed to get through
cmdline or upstream", from 0day robot's view, there would be 2 kinds of base tree
info appended at the bottom of patch message.

For the info starts with "base-commit: <xxx> ...", robot would know it
is reliable base commit, it would just checkout it and apply the prerequisite
patches and patchset for the work.

For the info starts with "parent-commit: <xxx>; parent-patch-id: <xxx>",
there are 3 situations 0day robot would need to handle:

1) parent commit is well-known public commit(e.g. one in Linus's tree),
   in this case, robot will just checkout the parent commit and apply the
   patchset accordingly.

2) parent commit is well-known in-flight patch, since 0day maintains the
   database of in-flight patches indexed by their patch-ids and
   commit-ids(of the git tree mentioned below), it also exports a git tree
   which holds all in-flight patches, where each patchset map to a new branch:

   https://github.com/0day-ci/linux/branches

   0day will find that patch in database by parent patch id, then do the
   checkout and apply work.

3) parent commit/patch-id is unknown, maybe because it's a
   - private patch;
   - public patch that's slightly modified locally;
   - public patch that's not covered by the 0day database
   all should be rare cases.

In practice, most developers may not feed exact commit id by --base=<xxx>
regularly when using git format-patch, this "showing parent commit" solution
could save developers' energy and reach a high coverage in the meantime.

Thanks,
Xiaolong.

>Whatever the reason is, if it _is_ likely, then I do not see a point
>in spending cycle to do get-upstream and merge-base, or giving an
>option to the user to explicitly specify the base.  Given that this
>series does these things, I'd have to conclude your "likely useful"
>is "might possibly turn out to be useful in some cases if you are
>lucky but is no way reliable" at best.
>
>Rather than throwing an unreliable information in the output and
>blindly proceeding, I'd think you would want to error out and tell
>the user to explicitly give the base without producing the patch
>output.  That way, you will not get patches with unreliable
>information.
>
>Suggesting to use set-upstream-to when you give that error message
>may also be helpful.

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

[Junio C Hamano]

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

> For the info starts with "base-commit: <xxx> ...", robot would know it
> is reliable base commit, it would just checkout it and apply the prerequisite
> patches and patchset for the work.
>
> For the info starts with "parent-commit: <xxx>; parent-patch-id: <xxx>",
> there are 3 situations 0day robot would need to handle:

It all depends on how you are computing this "parent-commit" thing.
When I reviewed the patch that implemented the fallback, my
impression was that it wasn't computing anything relevant, but just
was picking one of the random commit nearby in the history.

Assuming that it is not the case, and it is computing something
sensible, then I still do not see a point in marking these guessed
ones differently as "parent-commit" (not "base-commit").  After all,
a "base-commit" that was explicitly specified by the user can be
something that is inappropriate (e.g. the user may have thought the
base was a well-known one when his work was built on top of an early
draft of the patch that was later tweaked), so recipients of the
patch series need to be prepared to deal with a bogus base anyway.

> 2) parent commit is well-known in-flight patch, since 0day maintains the
>    database of in-flight patches indexed by their patch-ids and
>    commit-ids(of the git tree mentioned below), it also exports a git tree
>    which holds all in-flight patches, where each patchset map to a new branch:
>
>    https://github.com/0day-ci/linux/branches
>
>    0day will find that patch in database by parent patch id, then do the
>    checkout and apply work.

If a user starts from some solid base, applies a well-known
in-flight series, and builds his series on it and sends his series
out, and the base is identified by the patch-id of the last patch of
the in-flight series, then from that "parent patch-id" the robot can
find a commit that is the result of applying the same in-flight
series on top of some other solid base, as there is no coordination
between the user and 0day-ci system.  I would imagine that the
difference between the bases do matter--otherwise you wouldn't be
building this elaborate "base-commit" system that rejects the notion
that it is not sufficient for patches to textually apply cleanly and
insists that they have to be applied to exactly one known state.

So, I am not sure why you think "patch-id" is a good way to identify
the commit to be based on.

> In practice, most developers may not feed exact commit id by --base=<xxx>
> regularly when using git format-patch, this "showing parent commit" solution
> could save developers' energy and reach a high coverage in the meantime.

So, while I understand the desire and wish, I am not convinced "as a
fallback, use parent patch-id and guess where to apply" is a good
apprach to make the wish come true.