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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 12, 2016, 19:08 UTC
Message-ID: <xmqq7fg2r6fi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1460342873-28900-3-git-send-email-xiaolong.ye@intel.com>

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 it at the end of the_first_

It probably was a good idea to add stress during the discussion to compare various possibilities, but there no longer is a need to italicise "first" like this, I think.

> message(either the cover letter or the first patch in the series).

Please have space before "(" (also found elsewhere in this message) to make this readable.

Show 13 quoted lines

>
> The base tree info consists of the "base commit", which is a well-known
> commit that is part of the stable part of the project history everybody
> else works off of, and zero or more "prerequisite patches", which are
> well-known patches in flight that is not yet part of the "base commit"
> that need to be applied on top of "base commit" in topological order
> before the patches can be applied.
>
> The "base commit" is shown as "base-commit: " followed by the 40-hex of
> the commit object name.  A "prerequisite patch" is shown as
> "prerequisite-patch-id: " followed by the 40-hex "patch id", which is a
> sum of SHA-1 of the file diffs associated with a patch, with whitespace
> and line numbers ignored, it's reasonably stable and unique.

Let's be more helpful to end users. They do not need to know the exact formula, especially when there is a command to generate or check the id for themselves:

    "patch id", which can be obtained by passing the patch through the
    "git patch-id --stable" command

or something?

> For example, we have history where base is Z, with three prerequisites
> X-Y-Z, before the patch series A-B-C, i.e.

base is Z???

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

perhaps?

Show 7 quoted lines

>
> 	P---X---Y---Z---A---B---C
>
> We could say "git format-patch --base=P -3 C"(or variants thereof, e.g.
> with "--cover-letter" of using "Z..C" instead of "-3 C" to specify the
> range), then we could get base tree information block showing at the
> end of _first_ message as below:

Again, if "first" is _so_ important to stress, it probably is worth saying that by "first" you mean either patch 1/n or patch 0/n when the cover letter exists.

Also "could" may have made sense while we were having discussion on possible design of the hypothetical feature, but with the patch applied, the feature becomes a reality, so you can and should stop living in the hypothetical world and do s/could/can/ the above.

	With "git format-patch --base=P -3 C" (or variants...), the
	base tree information block is shown at the end of the first
	message the command outputs (either the first patch, or the
	cover letter), like this:

perhaps?

I assume that the patch to the documentation has the same text I commented on the above, so I won't repeat my comments to them.

Show 40 quoted lines

> 	base-commit: P
> 	prerequisite-patch-id: X
> 	prerequisite-patch-id: Y
> 	prerequisite-patch-id: Z
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Wu Fengguang <fengguang.wu@intel.com>
> Signed-off-by: Xiaolong Ye <xiaolong.ye@intel.com>
> ---
>  Documentation/git-format-patch.txt | 56 +++++++++++++++++++++++
>  builtin/log.c                      | 92 ++++++++++++++++++++++++++++++++++++++
>  t/t4014-format-patch.sh            | 15 +++++++
>  3 files changed, 163 insertions(+)
> ...
> +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;
> +	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;
> +	revs.topo_order = 1;
> +	for (i = 0; i < total; i++) {
> +		if (!in_merge_bases(base, list[i]) || base == list[i])
> +			die(_("base commit should be the ancestor of revision list"));

This check looks overly expensive, but I do not think of a more efficient way to do this, given that "All the commits from our series must reach the specified base" is what you seem to want.

My understanding is that if base=P is given and you are doing "format-patch Z..C" in this picture:

    Q---P---Z---B---*---C
     \             /
      .-----------A

your list would become A, B and C, and you want to detect that P is not an ancestor of A. merge_bases_many() computes a wrong thing for this use case, and you'd need to go one-by-one.

Unless there is some clever trick to take advantage of the previous traversal you made in order to find out A, B and C are the commits that are part of your series somehow.

Anybody with clever ideas?