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