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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 23, 2016, 18:25 UTC
Message-ID: <xmqqbn65caqi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1458723147-7335-4-git-send-email-xiaolong.ye@intel.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).

Show 13 quoted lines

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