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.

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

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,