Hi Christian,

On Thu, 7 Sep 2023, Christian Couder wrote:

> On Sun, Sep 3, 2023 at 5:47 PM Johannes Schindelin
> <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Sat, 29 Apr 2023, Elijah Newren wrote:
> >
> > > On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee <derrickstolee@github.com> 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