From: Derrick Stolee <derrickstolee@github.com>
Date: Fri, 14 Apr 2023 14:23:08 GMT
Subject: Re: [PATCH 11/14] replay: use standard revision ranges
Message-ID: <89c78da5-388a-e52b-b20b-e376ac90de14@github.com>
In-Reply-To: <8daf2603-2818-9c9d-7a06-6af2872a045a@github.com>

On 4/14/2023 10:09 AM, Derrick Stolee wrote:
> On 4/7/2023 3:24 AM, Christian Couder wrote:
>> From: Elijah Newren <newren@gmail.com>
>>
>> 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!
> 
> 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 realizing after hitting "send" that this change is
intentional (based on your test updates). I don't agree with
it, though.

Sending arbitrary command-line arguments to setup_revisions()
creates an opportunity for behavior you are not expecting.

For instance, can users pass multiple ranges? Can users
supply --first-parent? What happens if they add an --author
filter?

(I was able to get a segfault by rebasing this series with
--author=stolee because the commit list became empty. Something
to watch for.)
 
> 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>.

For that reason, I think we should be using explicit argument
parsing in the builtin and only transform arguments we
understand into the setup_revisions() (by building a strvec).

Thanks,
-Stolee