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

[Junio C Hamano <gitster@pobox.com>]

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.