Re: [PATCH 02/11] builtin rebase: support `git rebase --onto A...B`

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 8, 2018, 19:12 UTC
Message-ID: <xmqq1sb8hdvd.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180808134830.19949-3-predatoramigo@gmail.com>

Pratik Karki <predatoramigo@gmail.com> writes:

Show 7 quoted lines

> This commit implements support for an --onto argument that is actually a
> "symmetric range" i.e. `<rev1>...<rev2>`.
>
> The equivalent shell script version of the code offers two different
> error messages for the cases where there is no merge base vs more than
> one merge base. Though following the similar approach would be nice,
> this would create more complexity than it is of current. Currently, for

Sorry, but it is unclear what you mean by "than it is of current." Do you mean we leave it broken at this step in the series for now for expediency, with the intention to later revisit and fix it, or do you mean something else?

Show 15 quoted lines

> simple convenience, the `get_oid_mb()` function is used whose return
> value does not discern between those two error conditions.
>
> Signed-off-by: Pratik Karki <predatoramigo@gmail.com>
> ...
> @@ -387,7 +389,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	if (!options.onto_name)
>  		options.onto_name = options.upstream_name;
>  	if (strstr(options.onto_name, "...")) {
> -		die("TODO");
> +		if (get_oid_mb(options.onto_name, &merge_base) < 0)
> +			die(_("'%s': need exactly one merge base"),
> +			    options.onto_name);
> +		options.onto = lookup_commit_or_die(&merge_base,
> +						    options.onto_name);

The original is slightly sloppy in that it will misparse

	rebase --onto 'master^{/log ... message}'

and this shares the same, which I think is probably OK. When this actually becomes problematic, the original can easily be salvaged by making it to fall back to the same peel_committish in its else clause; I am not sure if this C rewrite is as easily be fixed the same way, though.

>  	} else {
>  		options.onto = peel_committish(options.onto_name);
>  		if (!options.onto)