* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
2023-09-21 10:05 [PATCH] revision: make pseudo-opt flags read via stdin behave consistently Patrick Steinhardt
@ 2023-09-21 18:45 ` Taylor Blau
2023-09-25 12:48 ` Patrick Steinhardt
2023-09-21 19:04 ` Junio C Hamano
2023-09-25 13:02 ` [PATCH v2] " Patrick Steinhardt
2 siblings, 1 reply; 7+ messages in thread
From: Taylor Blau @ 2023-09-21 18:45 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
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,
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
2023-09-21 18:45 ` Taylor Blau
@ 2023-09-25 12:48 ` Patrick Steinhardt
2023-09-25 22:47 ` Taylor Blau
0 siblings, 1 reply; 7+ messages in thread
From: Patrick Steinhardt @ 2023-09-25 12:48 UTC (permalink / raw)
To: Taylor Blau; +Cc: git, Christian Couder
[-- Attachment #1: Type: text/plain, Size: 6936 bytes --]
On Thu, Sep 21, 2023 at 02:45:13PM -0400, Taylor Blau wrote:
> 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 ;-).
I agree with all of what you say. But yes, we both come to the same
conclusion: it's not great behaviour, but we can't really do anything
about it because it's been like this since basically forever.
I'm a bit confused though, because further down your mail you seem to
disagree with what you write here by proposing to change the behaviour
regardless.
[snip]
> > 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).
It does not change the behaviour, it only documents the current state
such that it's at least spelled out somewhere.
> > --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.
That was indeed my first approach. But this change would break behaviour
that was introduced with 42cabc341c4 (Teach rev-list an option to read
revs from the standard input., 2006-09-05) almost 17 years ago. If we
change it now then this is very likely to cause problems somewhere.
To clarify:
- Since 2006 we had `--stdin`. Revisions read via `--stdin` were not
influenced by `--not`.
- With Git v2.41 I introduced the ability to read pseudo-opts via
`--stdin`. These _are_ influenced by `--not`, which is
inconsistent with the way we handle normal revs.
I want to rectify the newly introduced pseudo-opts-via-stdin to behave
the same as the first point now. Like this, we make the behaviour more
consistent with what we did for the last 17 years.
Is there anything in my commit message that can be clarified such that
it becomes less confusing?
Patrick
> --- 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,
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
2023-09-25 12:48 ` Patrick Steinhardt
@ 2023-09-25 22:47 ` Taylor Blau
0 siblings, 0 replies; 7+ messages in thread
From: Taylor Blau @ 2023-09-25 22:47 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
On Mon, Sep 25, 2023 at 02:48:35PM +0200, Patrick Steinhardt wrote:
> > > 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).
>
> It does not change the behaviour, it only documents the current state
> such that it's at least spelled out somewhere.
Sorry, I must have been confused when writing this :-<. Looking at it
again, I agree that the current behavior is that "--not --stdin" treats
any tips given over stdin as INTERESTING, so this documentation is
consistent with that.
> > 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.
>
> That was indeed my first approach. But this change would break behaviour
> that was introduced with 42cabc341c4 (Teach rev-list an option to read
> revs from the standard input., 2006-09-05) almost 17 years ago. If we
> change it now then this is very likely to cause problems somewhere.
Per above, you're absolutely right. I think that the patch that you
proposed here LGTM.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
2023-09-21 10:05 [PATCH] revision: make pseudo-opt flags read via stdin behave consistently Patrick Steinhardt
2023-09-21 18:45 ` Taylor Blau
@ 2023-09-21 19:04 ` Junio C Hamano
2023-09-25 12:51 ` Patrick Steinhardt
2023-09-25 13:02 ` [PATCH v2] " Patrick Steinhardt
2 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2023-09-21 19:04 UTC (permalink / raw)
To: Patrick Steinhardt; +Cc: git, Christian Couder
Patrick Steinhardt <ps@pks.im> 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@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.
> @@ -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.
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently
2023-09-21 19:04 ` Junio C Hamano
@ 2023-09-25 12:51 ` Patrick Steinhardt
0 siblings, 0 replies; 7+ messages in thread
From: Patrick Steinhardt @ 2023-09-25 12:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Christian Couder
[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]
On Thu, Sep 21, 2023 at 12:04:11PM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
[snip]
> > 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.
Doesn't hurt to make this more explicit as well, yes. I'll send a v2
that adds this bit.
Thanks!
Patrick
> > @@ -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.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] revision: make pseudo-opt flags read via stdin behave consistently
2023-09-21 10:05 [PATCH] revision: make pseudo-opt flags read via stdin behave consistently Patrick Steinhardt
2023-09-21 18:45 ` Taylor Blau
2023-09-21 19:04 ` Junio C Hamano
@ 2023-09-25 13:02 ` Patrick Steinhardt
2 siblings, 0 replies; 7+ messages in thread
From: Patrick Steinhardt @ 2023-09-25 13:02 UTC (permalink / raw)
To: git; +Cc: Christian Couder, Junio C Hamano, Taylor Blau
[-- Attachment #1: Type: text/plain, Size: 7615 bytes --]
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).
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.
- 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.
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 retroactively change it and make the behaviour sane.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Christian Couder <christian.couder@gmail.com>
---
Range-diff against v1:
1: b93d4c8c23 ! 1: 6221acd279 revision: make pseudo-opt flags read via stdin behave consistently
@@ Commit message
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.
+ right thing to retroactively change it and make the behaviour sane.
Signed-off-by: Patrick Steinhardt <ps@pks.im>
Reported-by: Christian Couder <christian.couder@gmail.com>
@@ Documentation/rev-list-options.txt: endif::git-log[]
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.
++ through stdin will not be affected by it. Conversely, when passed
++ via standard input, the revisions passed on the command line will
++ not be affected by it.
--all::
Pretend as if all the refs in `refs/`, along with `HEAD`, are
Documentation/rev-list-options.txt | 8 +++++++-
revision.c | 10 +++++-----
t/t6017-rev-list-stdin.sh | 21 +++++++++++++++++++++
3 files changed, 33 insertions(+), 6 deletions(-)
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b2..66d71d1b95 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -151,6 +151,10 @@ 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. Conversely, when passed
+ via standard input, the revisions passed on the command line will
+ not be affected by it.
--all::
Pretend as if all the refs in `refs/`, along with `HEAD`, are
@@ -240,7 +244,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;
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
@@ -2817,13 +2817,13 @@ static void read_revisions_from_stdin(struct rev_info *revs,
continue;
}
- if (handle_revision_pseudo_opt(revs, argv, flags) > 0)
+ if (handle_revision_pseudo_opt(revs, argv, &flags) > 0)
continue;
die(_("invalid option '%s' in --stdin mode"), sb.buf);
}
- if (handle_revision_arg(sb.buf, revs, 0,
+ if (handle_revision_arg(sb.buf, revs, flags,
REVARG_CANNOT_BE_FILENAME))
die("bad revision '%s'", sb.buf);
}
@@ -2906,7 +2906,7 @@ 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, &flags);
+ read_revisions_from_stdin(revs, &prune_data);
continue;
}
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index a57f1ae2ba..4821b90e74 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -68,6 +68,7 @@ check --glob=refs/heads
check --glob=refs/heads --
check --glob=refs/heads -- file-1
check --end-of-options -dashed-branch
+check --all --not refs/heads/main
test_expect_success 'not only --stdin' '
cat >expect <<-EOF &&
@@ -127,4 +128,24 @@ test_expect_success 'unknown option without --end-of-options' '
test_cmp expect error
'
+test_expect_success '--not on command line does not influence revisions read via --stdin' '
+ cat >input <<-EOF &&
+ refs/heads/main
+ EOF
+ git rev-list refs/heads/main >expect &&
+
+ git rev-list refs/heads/main --not --stdin <input >actual &&
+ test_cmp expect actual
+'
+
+test_expect_success '--not via stdin does not influence revisions from command line' '
+ cat >input <<-EOF &&
+ --not
+ EOF
+ git rev-list refs/heads/main >expect &&
+
+ git rev-list refs/heads/main --stdin refs/heads/main <input >actual &&
+ test_cmp expect actual
+'
+
test_done
--
2.42.0
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 7+ messages in thread