Re: [PATCH 11/14] replay: use standard revision ranges

From: Elijah Newren <newren@gmail.com>
Date: Apr 15, 2023, 19:07 UTC
Message-ID: <CABPp-BGfG3VeY1gOugzig8PLan1AS66BMWnyFSOsLOy-zqLdXw@mail.gmail.com>
In-Reply-To: <89c78da5-388a-e52b-b20b-e376ac90de14@github.com>

On Fri, Apr 14, 2023 at 7:23 AM Derrick Stolee <derrickstolee@github.com> wrote:

Show 30 quoted lines

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

Note that fast-export has always accepted the full range of setup_revisions() flags, even though many options make no sense. Perhaps we could tighten up the parsing for fast-export, but limiting fast-export to three commits in order to avoid other nonsensical combinations would destroy its utility.

You may think I've gone on a tangent, but I think it's an important comparison point here. I think limiting replay to three commits does the same: destroys its intended utility. replay is a patch-based analogue to fast-export, and we need a wide range of the revision ranges for it to work.

(There are other interesting comparisons here too. For example, if someone specifies commits rather than branches to fast-export, then things break down: new commits may be written but nothing takes advantage of them so they merely become garbage to be gc'ed later. Likely not what the user wanted. The exact same issue existed for replay. However, in replay's case I tried to add some sanity checking for it -- perhaps that sanity checking would be useful to copy to fast-export?)

> For instance, can users pass multiple ranges?

There's no such thing as multiple ranges for most commands (see f302c1e4aa0 ("revisions(7): clarify that most commands take a single revision range", 2021-05-18))

However, users should absolutely be allowed to specify something like

  $ git replay --onto new-base master..my-topic some-base..other-topic

which is one revision range, and I'd expect replay to take commits in the history of either my-topic or other-topic, which are not in the history of either master or some-base, and replay them. It'd be equivalent, in most cases, to the separate commands:

   $ git replay --onto new-base ^master ^some-base my-topic
   $ git replay --onto new-base ^master ^some-base other-topic

The first of the two commands above, for example, would replay the commits in my-topic back to nearer of (master, some-base) and do the replaying on top of new-base. The second command is similar, but with my-topic replaced by other-topic.

Notice, though, that I said most cases. There's an important case where the single command is different from the two commands: when my-topic and other-topic share common commits (which is not also shared by master and some-base), we want the replayed common history to remain common. If you do two separate commands, the replayed common history is no longer common (those commits will instead be duplicated with separate committer dates). So, supporting the single command form is an intrinsic design consideration for git-replay.

> Can users supply --first-parent?

Absolutely, and it might be a useful replacement for or supplement to --[no-]rebase-cousins.

Also, we definitely want to allow --ancestry-path and --ancestry-path=<commit>, the latter of which was specifically written for use in git-replay.

In fact, --first-parent sometimes might be equivalent to a particular specialization of --ancestry-path=<commit>, so yeah, I definitely want to allow it.

> What happens if they add an --author filter?

git fast-export accepts full revision ranges, including ones that make no sense like this. We could add patches to exclude such options, to both replay and fast-export. Or just document that they make no sense.

To answer the question as asked, though, let me first provide a little background: for a commit A with parent B, git-replay checks whether the parent of the current commit (i.e. for A that'd be B) has been replayed (as B'). If so, it will make the parent of A' be B'. If there is no B' (e.g. because B was part of "upstream" and thus wasn't replayed), it makes the parent of A' be whatever is passed to --onto. This allows git-replay to not only rebase/replay a linear set of patches, but a partially or even fully divergent set of branches.

Now, with that background, if someone passes --author, there will be a lot of parent commits that aren't replayed, resulting in each new "first" commit being rebased on top of --onto, and only the final one(s) being referenced by the updated ref commands. In other words, it'll leave lots of orphaned commits behind and only keep the "newest" contiguous ones belonging to the author. In simpler terms, passing --author to git-replay would "make a really big mess of things" from the user perspective, but an implementationally well-defined mess. This, again, is similar to passing --author to fast-export -- that tool has very different primitives involved causing it to make a completely different big mess of things (and perhaps even bigger mess of things), but the mess is at least implementationally well-defined to those that understand the data structures and implementation. (And yes, I thought of this exact case while developing the tool, for what it's worth.)

Does that mean I want to allow people to pass --author to fast-export or replay? I'd be perfectly happy with patches that prohibit it in both tools. But I'm also not worried about people off-roading and trying it and getting non-sense. A documentation patch stating that using options that "disconnect" history make no sense with either replay or fast-export might be the easiest (and most future-proof) solution.

> [unstated: and what about other similar options?]

I'd really rather not have an allowlist of which revision options should be allowed for use by git-replay. A denylist might be okay, if also implemented for fast-export, but I'm more of the opinion that we just document that both commands only make sense with "contiguous" history.

Most importantly, though, at a minimum I do absolutely want to allow several negative revisions to be specified, several positive revisions to be specified, and special flags like --ancestry-path or --first-parent to be specified. There may well be additional flags, both current and future, that should be allowed too.

In short, I don't want another limited rebase; I want a more generic tool.

Hope that helps, Elijah