Hi Dscho, On Sun, Sep 3, 2023 at 5:47 PM Johannes Schindelin <Johannes.Schindelin@xxxxxx> wrote: > > Hi Elijah & Stolee, > > 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. 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. 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. > 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, so it might want to be part of a separate patch series once the existing one has graduated. 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.
