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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Junio,

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

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

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

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