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

[Taylor Blau <me@ttaylorr.com>]

On Thu, Sep 21, 2023 at 12:05:57PM +0200, Patrick Steinhardt wrote:
> When reading revisions from stdin via git-rev-list(1)'s `--stdin` option
> then these revisions never honor flags like `--not` which have been
> passed on the command line. Thus, an invocation like e.g. `git rev-list
> --all --not --stdin` will not treat all revisions read from stdin as
> uninteresting. While this behaviour may be surprising to a user, it's
> been this way ever since it has been introduced via 42cabc341c4 (Teach
> rev-list an option to read revs from the standard input., 2006-09-05).

I'm not sure I agree that `--all --not --stdin` marking the tips given
on stdin as uninteresting would be surprising to users. It feels like
`--stdin` introduces its own "scope" that `--not` should apply to.

I might be biased or looking at this differently than how other users
might, but `--all --not --stdin` reads like "traverse everything except
what I give you over stdin", and deviating from that behavior feels more
surprising than not.

Either way, since this comes from as far back as 42cabc341c4, I think
that we're stuck with this behavior one way or the other ;-).

> With that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`
> mode, 2023-06-15) we have introduced a new mode to read pseudo opts from
> standard input where this behaviour is a lot more confusing. If you pass
> `--not` via stdin, it will:
>
>     - Influence subsequent revisions or pseudo-options passed on the
>       command line.

I agree here that this behavior is legitimately surprising and should
probably be considered a bug.

>     - Influence pseudo-options passed via standard input.
>
>     - _Not_ influence normal revisions passed via standard input.
>
> This behaviour is extremely inconsistent and bound to cause confusion.
>
> While it would be nice to retroactively change the behaviour for how
> `--not` and `--stdin` behave together, chances are quite high that this
> would break existing scripts that expect the current behaviour that has
> been around for many years by now. This is thus not really a viable
> option to explore to fix the inconsistency.
>
> 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.

These all same very sane to me. Let's read on...

> Thus, all flags read via standard input are fully self-contained to that
> standard input, only.
>
> 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.

I agree. I am not so sympathetic to scripts that rely on this behavior,
which feels like it is obviously broken.

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

Hmmph. This seems to change the behavior introduced by 42cabc341c4. Am I
reading this correctly that tips on stdin with '--not --stdin' would not
be marked as UNINTERESTING?

(Reading this back, there are a lot of double-negatives in what I just
wrote making it confusing for me at least. What I'm getting at here is
that I think `--not --stdin` should mark tips given via stdin as
UNINTERESTING).

>  --all::
>  	Pretend as if all the refs in `refs/`, along with `HEAD`, are
> @@ -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.
>
>  ifdef::git-rev-list[]
>  --quiet::
> diff --git a/revision.c b/revision.c
> index 2f4c53ea20..a1f573ca06 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
>  }
>
>  static void read_revisions_from_stdin(struct rev_info *revs,
> -				      struct strvec *prune,
> -				      int *flags)
> +				      struct strvec *prune)
>  {
>  	struct strbuf sb;
>  	int seen_dashdash = 0;
>  	int seen_end_of_options = 0;
>  	int save_warning;
> +	int flags = 0;

OK, I think this confirms my assumption that the `--not` in `--not
--stdin` does not propagate across to the input given over stdin. I am
not sure that we are safely able to change that behavior.

I wonder if we could instead do something like:

  - inherit the current set of psuedo-opts when processing input over
    `--stdin`
  - allow pseudo-opts within the context of `--stdin` arbitrarily
  - prevent those psuedo-opts applied while processing `--stdin` from
    leaking over to subsequent command-line arguments.

Here's one approach for doing that, where we make a copy of the current
set of flags when calling `read_revisions_from_stdin()` instead of
passing a pointer to the global state.

--- 8< ---
diff --git a/revision.c b/revision.c
index a1f573ca06..d8dad35d52 100644
--- a/revision.c
+++ b/revision.c
@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 }

 static void read_revisions_from_stdin(struct rev_info *revs,
-				      struct strvec *prune)
+				      struct strvec *prune,
+				      int flags)
 {
 	struct strbuf sb;
 	int seen_dashdash = 0;
 	int seen_end_of_options = 0;
 	int save_warning;
-	int flags = 0;

 	save_warning = warn_on_object_refname_ambiguity;
 	warn_on_object_refname_ambiguity = 0;
@@ -2906,7 +2906,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
 				}
 				if (revs->read_from_stdin++)
 					die("--stdin given twice?");
-				read_revisions_from_stdin(revs, &prune_data);
+				read_revisions_from_stdin(revs, &prune_data,
+							  flags);
 				continue;
 			}
--- >8 ---

Thanks,