On Fri, Apr 14, 2023 at 7:09 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote:
>
> On 4/7/2023 3:24 AM, Christian Couder wrote:
> > From: Elijah Newren <newren@xxxxxxxxx>
> >
> > Instead of the fixed "<oldbase> <branch>" arguments, the replay
> > command now accepts "<revision-range>..." arguments in a similar
> > way as many other Git commands. This makes its interface more
> > standard and more flexible.
>
> Unfortunately, while doing this, you have broken the --onto
> logic:
>
> $ git replay --onto HEAD~2 HEAD~1 HEAD
> fatal: replaying down to root commit is not supported yet!
One of the things Christian asked/proposed a while ago was instead of
modifying fast-rebase into git-replay, just build git-replay from the
ground up. I argued for using fast-rebase as a starting point, but I
think you've perhaps given an example for why I may have been wrong to
do so. It has caused confusion, because fast-rebase used a very rigid
syntax and specification (because it was a simple test script designed
to handle exactly one simple setup) that is completely against what we
want here.
In particular, you are probably thinking of
$ git replay --onto HEAD~2 HEAD~1 HEAD
as meaning replay the commits in the range HEAD~1..HEAD (i.e. just
HEAD) with the new base of HEAD~2. That's the inflexible way
fast-rebase worked. We are dispensing with that here, though; your
command means replay the commits in the history of either HEAD~1 or
HEAD (all the way to the root since you had no negative references) on
top of HEAD~2. If you had instead said:
$ git replay --onto HEAD~2 HEAD~1..HEAD
then I think `git replay` handles it just fine. Christian did cover
this in the commit message, but it's perhaps subtle and easily missed.
Anyway, at no point in this series does `git replay` support rebasing
commits back to the root, so the error message is what I'd expect.
The problem was we weren't clear enough about a different syntax being
expected.
> The rev-walk you are supplying by this line...
>
> > + argc = setup_revisions(argc, argv, &revs, NULL);
>
> is taking the remaining arguments and using them as tips to
> walk. We need to be able to recognize that --onto A B C means
> that A is the new base and our walk is B..C.
>
> I'm not sure if there might be a way to use a callback for
> the --onto option and pull out the next three options into
> 'new-base', 'old-base', 'tip' values or something.
>
> Overall, I don't think being flexible in the CLI is of high
> value for this command. Let's be as prescriptive as possible.
>
> Something like:
>
> 'git replay [options] <base> <tip>'
> This mode means to rebase <tip> onto <base>,
> detecting the range of commits to rewrite.
>
> 'git replay [options] <new-base> <old-base> <tip>'
> This mode means to rebase the range <old-base>..<tip>
> onto <new-base>.
>
> We don't even need "--onto" for these positional arguments.
So, from my view, the problem with this alternative design is that it
inflexibly hardcodes a linear range of commits, in a way that likely
precludes future extension. In particular it:
* precludes handling multiple divergent branches, which I think was
a core design requirement
* seems problematic for extending this to handle replaying of merges
(where being able to select what to replay needs more control)
* more generally from the above two, this precludes the opportunity
to specify both multiple positive and negative refs
* precludes things like using `--ancestry-path=<commit>` which was
specifically designed for use with git-replay[1]
* may not work well with --first-parent, which I think should be
allowed (in fact, --first-parent in some instances is a specialization
of --ancestry-path=<commit>)
* seems to presume that rebasing is the only thing we want to do,
possibly precluding supporting cherry-pick-like commands
[1] See https://lore.kernel.org/git/CABPp-BHmj+QCBFDrH77iNfEU41V=UDu7nhBYkAbCsbXhshJzzw@xxxxxxxxxxxxxx/,
look for "Here's my usecase"
And on a minor note, it also copies the positional argument UI design
from rebase that I've always considered to be an annoying UI design
flaw. (Not sure if others agree with me on that, but it's always
bothered me.)
Since you followed up on this, I'll add more detail in response to
your other email.
