On Sun, Oct 29, 2023 at 7:02 AM Elijah Newren <newren@xxxxxxxxx> wrote: > On Thu, Oct 26, 2023 at 6:44 AM Johannes Schindelin > <Johannes.Schindelin@xxxxxx> wrote: > > On Tue, 10 Oct 2023, Christian Couder wrote: > > > [...] > > > + /* 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. > > I agree that warnings or error messages would be better. Ok, a warning() is emitted now in case an command-line option will be overridden. > But if we're talking about something short of that, I'd actually argue > the opposite of what you do here. I intentionally moved these > assignments after setup_revisions(), and in my mind, the purpose in > doing so was to satisfy the Principle of Least Astonishment. My > experience with git-fast-export, where some settings are made before > calling setup_revisions() and then can be overridden, and then do > completely hideous things, was much worse to me than just admitting > the flags are bad given the various assumptions the tool makes. I > have some patches sitting around to fix fast-export that I never got > around to upstreaming, but when it came time to implement git-replay, > I made sure to fix what I viewed as the bigger problem. I hope you will be able to upstream such changes. > [...] > > > @@ 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@xxxxxx/, > > 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`. Ok, I have made changes in the v6 I just sent, so that there is EXPERIMENTAL both in the NAME and SYNOPSIS. > > 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. Nit: Hyrum's Law says: "With a sufficient number of users of an API, it does not matter what you promise in the contract: all observable behaviors of your system will be depended on by somebody." The doc is part of the contract, which according to this law doesn't matter. So I don't see why you use this law to suggest a doc change. > > 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. > > I fully support this. Absolutely, 100%. Ok. Note that as I changed the SYNOPSIS, I also had to change the usage string, so that it matches the SYNOPSIS, otherwise a test would fail. So there is "EXPERIMENTAL" in the usage string too.
