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@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).
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:
signature.asc
Description: PGP signature
