Re: [PATCH v5 00/14] Introduce new `git replay` command
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 26, 2023, 13:44 UTC
- Message-ID
- <bd872b81-80a9-5e4e-dcb6-faebc9671848@gmx.de>
- In-Reply-To
- <20231010123847.2777056-1-christian.couder@gmail.com>
Hi Christian,
On Tue, 10 Oct 2023, Christian Couder wrote:
Show 34 quoted lines
> 6: ec51351889 ! 6: 37d545d5d6 replay: don't simplify history > @@ Metadata > Author: Elijah Newren <newren@gmail.com> > > ## Commit message ## > - replay: don't simplify history > + replay: change rev walking options > > Let's set the rev walking options we need after calling > - setup_revisions() instead of before. This makes it clearer which options > - we need. > + setup_revisions() instead of before. This enforces options we always > + want. > + > + We want the command to work from older commits to newer ones by default, > + but we are Ok with letting users reverse that, using --reverse, if that's > + what they really want. > > Also we don't want history simplification, as we want to deal with all > the commits in the affected range. > > + Helped-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Co-authored-by: Christian Couder <chriscool@tuxfamily.org> > Signed-off-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > @@ builtin/replay.c: int cmd_replay(int argc, const char **argv, const char *prefix > } > > + /* requirements/overrides for revs */ > -+ revs.reverse = 1; > ++ revs.reverse = !revs.reverse; > + revs.sort_order = REV_SORT_IN_GRAPH_ORDER; > + revs.topo_order = 1; > + revs.simplify_history = 0;
This still overrides a couple of command-line options, _silently_. I would prefer those three assignments to be moved just before the `setup_revisions()` call.
Letting users override these settings may not make much sense, but it makes even less sense to pretend to let them override the settings and then just ignore them without warning. (See also https://en.wikipedia.org/wiki/Principle_of_least_astonishment.)
Moving these three assignments before the `setup_revisions()` call would neatly remedy that.
Show 37 quoted lines
> 7: cd4ed07d2d = 7: 2943f08926 replay: add an important FIXME comment about gpg signing > 8: e45a55917c = 8: f81962ba41 replay: remove progress and info output > 9: 0587a76cbb = 9: 236747497e replay: remove HEAD related sanity check > 10: d10368e87a = 10: 3374d5be23 replay: make it a minimal server side command > 11: 4e09572c43 ! 11: 197d076a93 replay: use standard revision ranges > @@ Commit message > way as many other Git commands. This makes its interface more > standard and more flexible. > > + This also enables many revision related options accepted and > + eaten by setup_revisions(). If the replay command was a high level > + one or had a high level mode, it would make sense to restrict some > + of the possible options, like those generating non-contiguous > + history, as they could be confusing for most users. > + > Also as the interface of the command is now mostly finalized, > we can add some documentation as well as testcases to make sure > the command will continue to work as designed in the future. > > + We only document the rev-list related options among all the > + revision related options that are now accepted, as the rev-list > + related ones are probably the most useful for now. > + > + Helped-by: Dragan Simic <dsimic@manjaro.org> > + Helped-by: Linus Arver <linusa@google.com> > Co-authored-by: Christian Couder <chriscool@tuxfamily.org> > Signed-off-by: Elijah Newren <newren@gmail.com> > Signed-off-by: Christian Couder <chriscool@tuxfamily.org> > @@ Documentation/git-replay.txt (new) > + > +NAME > +---- > -+git-replay - Replay commits on a different base, without touching working tree > ++git-replay - Replay commits on a new base, works on bare repos too > + > + > +SYNOPSIS
As mentioned in https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/, I would like the `EXPERIMENTAL` label to be shown prominently here. Probably not only the `SYNOPSIS` as I had originally suggested but also in the `NAME`.
Otherwise we may end up with the same situation as with the (from my perspective, failed) `git switch`/`git restore` experiment, where we wanted to explore a better user experience than the overloaded `git checkout` command, only to now be stuck with having to maintain backward-compatibility for `git switch`/`git restore` command-line options that were not meant to be set in stone but to be iterated on, instead. A real-life demonstration of [Hyrum's Law](hyrumslaw.com/), if you like. Or, from a different angle, we re-enacted https://xkcd.com/927/ in a way.
I'd like to suggest to learn from history and avoid this by tacking on a warning label right at the top of the documentation. We may eventually manage to iterate `git replay` to a point where it is totally capable to supersede `git rebase`, by doing everything the latter does, except better, who knows? But we _do_ need the liberty to make sweeping changes to this new builtin if we want to have a prayer of doing that. And I fear that not even mentioning the EXPERIMENTAL nature right at the top of the manual page would just render us into that undesirable corner.
In addition, I am still a bit uneasy with introducing both the manual page and the test script in this commit (see my comments in https://lore.kernel.org/git/03460733-0219-c648-5757-db1958f8042e@gmx.de/). It would be better to uphold our high standard and introduce scaffolds for both files in the first commit, then populate the file contents incrementally in the same the patches that introduce the corresponding options/features/changes.
The rest of the interdiff consists mostly of context changes intermixed with a couple of changes that I like.
Ciao, Johannes