Re: [PATCH 11/14] replay: use standard revision ranges

Hi Christian,

On Thu, 7 Sep 2023, Christian Couder wrote:

On Sun, Sep 3, 2023 at 5:47 PM Johannes Schindelin
[off-list ref] wrote:

quoted

On Sat, 29 Apr 2023, Elijah Newren wrote:

quoted

On Mon, Apr 24, 2023 at 8:23 AM Derrick Stolee [off-list ref] wrote:

quoted

quoted

quoted

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_).

quoted

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