On 4/14/2023 10:09 AM, Derrick Stolee 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! > > 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
