Hi,
On Tue, Oct 10, 2023 at 5:39 AM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
> * Patch 12/15 (replay: disallow revision specific options and
> pathspecs) in version 4 has been removed, so there are now only 14
> patches instead of 15 in the series. This follows a suggestion by
> Dscho, and goes in the direction Elijah initially wanted before
> Derrick Stolee argued for disallowing revision specific options and
> pathspecs.
That's too strongly worded; and may be misleading. My primary goal in
that discussion was that setup_revisions() should not be a disallowed
API for future consumers such as git-replay. My secondary thought, at
the time, was that although I agreed that setup_revisions() was a
problematic API, I didn't think fixing it should be a prerequisite for
new features to make use of it.
However, your paragraph here could easily be read that I think the
setup_revisions() API is fine. I don't. I actually think fixing the
setup_revisions() API and preventing not only git-replay but many
other commands from accepting non-sensical flags, as suggested by
Stolee, is a very good idea. I even brought up the example
$ git stash show --graph --relative-date --min-parents=3
--simplify-merges --cherry --show-pulls --unpacked -v -t -8
--format=oneline --abbrev=12 --pretty=fuller --show-notes
--encode-email-headers --always --branches --indexed-objects stash@{0}
in the thread[1] along with the comment "guess which flags are ignored
and which aren't". That's garbage. This digging plus various things
Stolee said actually convinced me that perhaps prioritizing fixing the
setup_revisions() API over adding new consumers does make sense.
But, I don't feel nearly as strong about it as Stolee on
prioritization, so I'm not going to object too strongly with this
patch being tossed for now. But I do want to note that I actually
like Stolee's suggestion that we should fix that API and tighten what
many commands accept.
[1] https://lore.kernel.org/git/f5dd91a7-ba11-917a-39e2-2737829558cb@xxxxxxxxxx/
> * In patch 2/14 (replay: introduce new builtin) the command is now in
> the "plumbingmanipulators" category instead of the "mainporcelain"
> category. This is more in line with the goal of introducing it as a
> plumbing command for now. Thanks to a suggestion from Dscho.
I do want to eventually make it a porcelain, but I think it's pretty
far from that in its current state, so this is a good change.
> * In patch 6/14 (replay: change rev walking options) the commit
> message has been improved, including its subject, to better match
> and explain what the patch does.
This (and multiple other changes I elided) are good; thanks for
pushing forward on this.
> Also instead of forcing reverse
> order we use the reverse order by default but allow it to be changed
> using `--reverse`. Thanks to Dscho.
I can see why this might sometimes be useful for exclusively linear
history, but it seems to open a can of worms and possibly unfixable
corner cases for non-linear history. I'd rather not do this, or at
least pull it out of this series and let us discuss it in some follow
up series. There are some other alternatives that might handle such
usecases better.
> 6: ec51351889 ! 6: 37d545d5d6 replay: don't simplify history
...
> + We want the command to work from older commits to newer ones by default,
> + but we are Ok with letting users reverse that, using --reverse, if that's
> + what they really want.
As noted above, _I_ am not ok with this yet. Given the patch
prominently bears my name, the "we" here at a minimum is wrong. I
would rather leave this change out for now and discuss it for a
follow-on series.
> @@ 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
really minor point: "works on" or "works in" or "works with" ?
