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

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Aug 26, 2018, 18:36 UTC
Message-ID: <nycvar.QRO.7.76.6.1808241821470.73@tvgsbejvaqbjf.bet>
In-Reply-To: <xmqq1sb8hdvd.fsf@gitster-ct.c.googlers.com>

Hi Junio,

On Wed, 8 Aug 2018, Junio C Hamano wrote:

Show 14 quoted lines

> Pratik Karki <predatoramigo@gmail.com> writes:
> 
> > 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?

I suggested to drop the distinction, in favor of simpler code. Not for the time being, but for good.

I reworded the commit message thusly:

    builtin rebase: support `git rebase --onto A...B`

    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 it would be nice to retain this distinction, dropping it makes it
    possible to simply use the `get_oid_mb()` function. Besides, it happens
    rarely in real-world scenarios.

    Therefore, in the interest of keeping the code less complex, let's just
    use that function, and live with an error message that does not
    distinguish between those two error conditions.

Show 16 quoted lines

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

I did run into this recently, but not with an `--onto` option. I forgot the details (I meant to write it down, and forgot that, too).

Sorry for musing, back on the topic. Yes, it shares the same, and *that* makes it okay. Remember: this patch series is not about improving `git rebase` at all. It is about converting from shell script to builtin.

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

I will make a note so that I hopefully won't forget.

Thanks, Dscho

Show 5 quoted lines

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