Patrick Steinhardt <ps@xxxxxx> writes:
> Instead, we change the behaviour of how pseudo-opts read via standard
> input influence the flags such that the effect is fully localized. With
> this change, when reading `--not` via standard input, it will:
>
> - _Not_ influence subsequent revisions or pseudo-options passed on
> the command line, which is a change in behaviour.
>
> - Influence pseudo-options passed via standard input.
>
> - Influence normal revisions passed via standard input, which is a
> change in behaviour.
>
> Thus, all flags read via standard input are fully self-contained to that
> standard input, only.
I have to wonder what the most natural expectation by end-users be,
when "cmd --opt1 --stdin --opt3 arg2" is run and its stdin is fed
"--opt2 arg1". One interpretation may be to act as if "--stdin" on
the command line is replaced with what was read, but taken literally
that would make "cmd --opt1 --opt2 arg1 --opt3 arg2" that does not
make sense (i.e. options must come before arguments). We could
declare "--stdin is replaced by options read from there, and
non-options read from the standard input are handled separately",
but then it could be argued "cmd --opt1 --opt2 --opt3 arg2 arg1"
and "cmd --opt1 --opt2 --opt3 arg1 arg2" are equally plausible.
So in a sense, "what is read from --stdin is self contained" may be
the easiest to explain position to take.
> While this is a breaking change as well, the behaviour has only been
> recently introduced with Git v2.42.0. Furthermore, the current behaviour
> can be regarded as a simple bug. With that in mind it feels like the
> right thing to do retroactively change it and make the behaviour sane.
While I also appreciate your cautious approach to consider the risk
that this "fix" may have negative consequence, I tend to agree that
the behaviour is simply buggy and deserves to be fixed on the
'maint' track.
> Signed-off-by: Patrick Steinhardt <ps@xxxxxx>
> Reported-by: Christian Couder <christian.couder@xxxxxxxxx>
> ---
> Documentation/rev-list-options.txt | 6 +++++-
> revision.c | 10 +++++-----
> t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
> 3 files changed, 31 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
> index a4a0cb93b2..9bf13bac53 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -151,6 +151,8 @@ endif::git-log[]
> --not::
> Reverses the meaning of the '{caret}' prefix (or lack thereof)
> for all following revision specifiers, up to the next `--not`.
> + When used on the command line before --stdin, the revisions passed
> + through stdin will not be affected by it.
Do we also need to say "when read from --stdin, the revisions passed
on the command line are not affected" as well? I know you have it
where you explian "--stdin" in the next hunk, but since you are
adding one-half of the interaction, it may be less confusing to also
mention the other half at the same time.
> @@ -240,7 +242,9 @@ endif::git-rev-list[]
> them from standard input as well. This accepts commits and
> pseudo-options like `--all` and `--glob=`. When a `--` separator
> is seen, the following input is treated as paths and used to
> - limit the result.
> + limit the result. Flags like `--not` which are read via standard input
> + are only respected for arguments passed in the same way and will not
> + influence any subsequent command line arguments.
Other than that, looking good, and the changes to the code look all
sensible.
Thanks.
