Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 21, 2023, 19:04 UTC
Message-ID: <xmqqmsxf5owk.fsf@gitster.g>
In-Reply-To: <b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 14 quoted lines

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

Show 18 quoted lines

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> Reported-by: Christian Couder <christian.couder@gmail.com>
> ---
>  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.

Show 8 quoted lines

> @@ -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.