Hi Christian, On Thu, 7 Sep 2023, Christian Couder wrote: > On Sun, Sep 3, 2023 at 5:47 PM Johannes Schindelin > <Johannes.Schindelin@xxxxxx> wrote: > > > > On Sat, 29 Apr 2023, Elijah Newren wrote: > > > > > On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: > > > > > Basically, I'm not super thrilled about exposing options that are > > > > unlikely to be valuable to users and instead are more likely to cause > > > > confusion due to changes that won't successfully apply. > > > > > > Oh, I got thrown by the "right now" portion of your comment; I > > > couldn't see how time or future changes would affect anything to make > > > it less (or more) confusing for users. > > > > > > Quick clarification, though: while you correctly point out the type of > > > confusion the user would experience without my overriding, my > > > overriding of rev.reverse (after setup_revisions() returns, not before > > > it is called) precludes that experience. The override means none of > > > the above happens, and they would instead just wonder why their option > > > is being ignored. > > > > FWIW here is my view on the matter: `git replay`, at least in its current > > incarnation, is a really low-level tool. As such, I actually do not want > > to worry much about protecting users from nonsensical invocations. > > > > In that light, I would like to see that code rejecting all revision > > options except `--diff-algorithm` be dropped. Should we ever decide to add > > a non-low-level mode to `git replay`, we can easily add some user-friendly > > sanity check of the options then, and only for that non-low-level code. > > For now, I feel that it's just complicating things, and `git replay` is in > > the experimental phase anyway. > > I would be Ok with removing the patch (called "replay: disallow > revision specific options and pathspecs") > that rejects all revision options and pathspecs if there is a > consensus for that. Well, since you talk about "consensus" and I already made my strong preference known, how about you add yours? > It might not simplify things too much if there is still an exception for > `--diff-algorithm` though. Also it's not clear if you are Ok with > allowing pathspecs or not. I want to remove all the rev-list-option-disallowing code. Including the pathspec one. > The idea with disallowing all of them was to later add back those that > make sense along with tests and maybe docs to explain them in the > context of this command. It was not to disallow them permanently. So I > would think the best path forward would be a patch series on top of this > one that would revert the patch disallowing these options and maybe > pathspecs, and instead allow most of them and document and test things a > bit. I understand that that was the reasoning. What I would like to suggest is that we should treat `git replay` as a low-level (or: "plumbing" in Git Speak) command. That would allow us to leave the option for stricter command-line parameter validation to an _additional_ option, to be added later (or never), and to stop worrying about this tool being used in ways that make no sense (or make no sense _to us_, _now_). > > And further, I would even like to see that `--reverse` override go, and > > turn it into `revs.reverse = !revs.reverse` instead. (And yes, I can > > easily think of instances where I would have wanted to reverse a series of > > patches...). > > I think this might deserve docs and tests too, I agree. > so it might want to be part of a separate patch series once the existing > one has graduated. Here, I disagree. This is a bug, from my perspective, and needs to be fixed before graduating. > At this point I don't think it's worth delaying this patch series for > relatively small issues like this. There are many different ways this > new command can be polished and improved. The important thing is that > it looks like we all agree that the new command makes sense and should > have roughly the basic set of features that Elijah originally > implemented, so let's go with this, and then we can improve and > iterate on top of this. If it were for a small issue such as a typo, sure. But it is a bug that `--no-reverse` is hard-coded (in a confusing manner so because the `reverse` attribute is set), and the `--no-reverse`/`--reverse` options are ignored, silently so. Ciao, Johannes
