Re: [PATCH 11/14] format-patch: extend --range-diff to accept revision range

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 25, 2018, 20:53 UTC
Message-ID: <xmqqva93t4u7.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180722095717.17912-12-sunshine@sunshineco.com>

Eric Sunshine <sunshine@sunshineco.com> writes:

Show 15 quoted lines

> When submitting a revised a patch series, the --range-diff option embeds
> a range-diff in the cover letter showing changes since the previous
> version of the patch series. The argument to --range-diff is a simple
> revision naming the tip of the previous series, which works fine if the
> previous and current versions of the patch series share a common base.
>
> However, it fails if the revision ranges of the old and new versions of
> the series are disjoint. To address this shortcoming, extend
> --range-diff to also accept an explicit revision range for the previous
> series. For example:
>
>     git format-patch --cover-letter --range-diff=v1~3..v1 -3 v2
>
> Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>
> ---

Makes sense. Even though a single "common rev" would serve as a feature to discourage rebasing done "just to catch up" without a good reason, it is a good idea to give an escape hatch like this to support a case where rebasing is the right thing to do.

Show 10 quoted lines

>  static void infer_range_diff_ranges(struct strbuf *r1,
>  				    struct strbuf *r2,
>  				    const char *prev,
> +				    struct commit *origin,
>  				    struct commit *head)
>  {
>  	const char *head_oid = oid_to_hex(&head->object.oid);
>  
> -	strbuf_addf(r1, "%s..%s", head_oid, prev);
> -	strbuf_addf(r2, "%s..%s", prev, head_oid);

I thought "git range-diff" also took the three-dot notation as a short-hand but there is no point using that in this application, which wants the RHS and the LHS range in separate output variables.

Show 9 quoted lines

> +	if (!strstr(prev, "..")) {
> +		strbuf_addf(r1, "%s..%s", head_oid, prev);
> +		strbuf_addf(r2, "%s..%s", prev, head_oid);
> +	} else if (!origin) {
> +		die(_("failed to infer range-diff ranges"));
> +	} else {
> +		strbuf_addstr(r1, prev);
> +		strbuf_addf(r2, "%s..%s",
> +			    oid_to_hex(&origin->object.oid), head_oid);

Interesting.

I actually would feel better to see the second range for the normal case to be computed exactly the same way, i.e.

	int prev_is_range = strstr(prev, "..");

	if (prev_is_range)
		strbuf_addstr(r1, prev);
	else
		strbuf_addf(r1, "%s..%s", head_oid, prev);

	if (origin)
		strbuf_addf(r2, "%s..%s", oid_to_hex(&origin->object.oid, head_oid);
	else if (prev_is_range)
		die("cannot figure out the origin of new series");
	else {
		warning("falling back to use '%s' as the origin of new series",	prev);
		strbuf_addf(r2, "%s..%s", prev, head_oid);
	}

because origin..HEAD is always the set of the "new" series, no matter what "prev" the user chooses to compare that series against, when there is a single "origin".