{"thread":{"id":"60252","subject":"[PATCH] revision: make pseudo-opt flags read via stdin behave consistently","startedAt":"2023-09-21T17:11:00Z","lastAt":"2023-09-25T22:47:23Z","messageCount":7,"participants":["Patrick Steinhardt","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"482093","messageId":"b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im","threadId":"60252","inReplyTo":null,"subject":"[PATCH] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-21T10:05:57Z","receivedAt":"2023-09-21T17:11:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When reading revisions from stdin via git-rev-list(1)'s `--stdin` option\nthen these revisions never honor flags like `--not` which have been\npassed on the command line. Thus, an invocation like e.g. `git rev-list\n--all --not --stdin` will not treat all revisions read from stdin as\nuninteresting. While this behaviour may be surprising to a user, it's\nbeen this way ever since it has been introduced via 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05).\n\nWith that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`\nmode, 2023-06-15) we have introduced a new mode to read pseudo opts from\nstandard input where this behaviour is a lot more confusing. If you pass\n`--not` via stdin, it will:\n\n    - Influence subsequent revisions or pseudo-options passed on the\n      command line.\n\n    - Influence pseudo-options passed via standard input.\n\n    - _Not_ influence normal revisions passed via standard input.\n\nThis behaviour is extremely inconsistent and bound to cause confusion.\n\nWhile it would be nice to retroactively change the behaviour for how\n`--not` and `--stdin` behave together, chances are quite high that this\nwould break existing scripts that expect the current behaviour that has\nbeen around for many years by now. This is thus not really a viable\noption to explore to fix the inconsistency.\n\nInstead, we change the behaviour of how pseudo-opts read via standard\ninput influence the flags such that the effect is fully localized. With\nthis change, when reading `--not` via standard input, it will:\n\n    - _Not_ influence subsequent revisions or pseudo-options passed on\n      the command line, which is a change in behaviour.\n\n    - Influence pseudo-options passed via standard input.\n\n    - Influence normal revisions passed via standard input, which is a\n      change in behaviour.\n\nThus, all flags read via standard input are fully self-contained to that\nstandard input, only.\n\nWhile this is a breaking change as well, the behaviour has only been\nrecently introduced with Git v2.42.0. Furthermore, the current behaviour\ncan be regarded as a simple bug. With that in mind it feels like the\nright thing to do retroactively change it and make the behaviour sane.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\nReported-by: Christian Couder <christian.couder@gmail.com>\n---\n Documentation/rev-list-options.txt |  6 +++++-\n revision.c                         | 10 +++++-----\n t/t6017-rev-list-stdin.sh          | 21 +++++++++++++++++++++\n 3 files changed, 31 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a4a0cb93b2..9bf13bac53 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -151,6 +151,8 @@ endif::git-log[]\n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n \tfor all following revision specifiers, up to the next `--not`.\n+\tWhen used on the command line before --stdin, the revisions passed\n+\tthrough stdin will not be affected by it.\n \n --all::\n \tPretend as if all the refs in `refs/`, along with `HEAD`, are\n@@ -240,7 +242,9 @@ endif::git-rev-list[]\n \tthem from standard input as well. This accepts commits and\n \tpseudo-options like `--all` and `--glob=`. When a `--` separator\n \tis seen, the following input is treated as paths and used to\n-\tlimit the result.\n+\tlimit the result. Flags like `--not` which are read via standard input\n+\tare only respected for arguments passed in the same way and will not\n+\tinfluence any subsequent command line arguments.\n \n ifdef::git-rev-list[]\n --quiet::\ndiff --git a/revision.c b/revision.c\nindex 2f4c53ea20..a1f573ca06 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n }\n \n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t      struct strvec *prune,\n-\t\t\t\t      int *flags)\n+\t\t\t\t      struct strvec *prune)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n \tint seen_end_of_options = 0;\n \tint save_warning;\n+\tint flags = 0;\n \n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n@@ -2817,13 +2817,13 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags) > 0)\n+\t\t\tif (handle_revision_pseudo_opt(revs, argv, &flags) > 0)\n \t\t\t\tcontinue;\n \n \t\t\tdie(_(\"invalid option '%s' in --stdin mode\"), sb.buf);\n \t\t}\n \n-\t\tif (handle_revision_arg(sb.buf, revs, 0,\n+\t\tif (handle_revision_arg(sb.buf, revs, flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\n@@ -2906,7 +2906,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\t\tread_revisions_from_stdin(revs, &prune_data, &flags);\n+\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n \t\t\t\tcontinue;\n \t\t\t}\n \ndiff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh\nindex a57f1ae2ba..4821b90e74 100755\n--- a/t/t6017-rev-list-stdin.sh\n+++ b/t/t6017-rev-list-stdin.sh\n@@ -68,6 +68,7 @@ check --glob=refs/heads\n check --glob=refs/heads --\n check --glob=refs/heads -- file-1\n check --end-of-options -dashed-branch\n+check --all --not refs/heads/main\n \n test_expect_success 'not only --stdin' '\n \tcat >expect <<-EOF &&\n@@ -127,4 +128,24 @@ test_expect_success 'unknown option without --end-of-options' '\n \ttest_cmp expect error\n '\n \n+test_expect_success '--not on command line does not influence revisions read via --stdin' '\n+\tcat >input <<-EOF &&\n+\trefs/heads/main\n+\tEOF\n+\tgit rev-list refs/heads/main >expect &&\n+\n+\tgit rev-list refs/heads/main --not --stdin <input >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--not via stdin does not influence revisions from command line' '\n+\tcat >input <<-EOF &&\n+\t--not\n+\tEOF\n+\tgit rev-list refs/heads/main >expect &&\n+\n+\tgit rev-list refs/heads/main --stdin refs/heads/main <input >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.42.0\n\n"},{"id":"482098","messageId":"ZQyPOScCKhHeZNrr@nand.local","threadId":"60252","inReplyTo":"b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im","subject":"Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-21T18:45:13Z","receivedAt":"2023-09-21T18:55:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Sep 21, 2023 at 12:05:57PM +0200, Patrick Steinhardt wrote:\n> When reading revisions from stdin via git-rev-list(1)'s `--stdin` option\n> then these revisions never honor flags like `--not` which have been\n> passed on the command line. Thus, an invocation like e.g. `git rev-list\n> --all --not --stdin` will not treat all revisions read from stdin as\n> uninteresting. While this behaviour may be surprising to a user, it's\n> been this way ever since it has been introduced via 42cabc341c4 (Teach\n> rev-list an option to read revs from the standard input., 2006-09-05).\n\nI'm not sure I agree that `--all --not --stdin` marking the tips given\non stdin as uninteresting would be surprising to users. It feels like\n`--stdin` introduces its own \"scope\" that `--not` should apply to.\n\nI might be biased or looking at this differently than how other users\nmight, but `--all --not --stdin` reads like \"traverse everything except\nwhat I give you over stdin\", and deviating from that behavior feels more\nsurprising than not.\n\nEither way, since this comes from as far back as 42cabc341c4, I think\nthat we're stuck with this behavior one way or the other ;-).\n\n> With that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`\n> mode, 2023-06-15) we have introduced a new mode to read pseudo opts from\n> standard input where this behaviour is a lot more confusing. If you pass\n> `--not` via stdin, it will:\n>\n>     - Influence subsequent revisions or pseudo-options passed on the\n>       command line.\n\nI agree here that this behavior is legitimately surprising and should\nprobably be considered a bug.\n\n>     - Influence pseudo-options passed via standard input.\n>\n>     - _Not_ influence normal revisions passed via standard input.\n>\n> This behaviour is extremely inconsistent and bound to cause confusion.\n>\n> While it would be nice to retroactively change the behaviour for how\n> `--not` and `--stdin` behave together, chances are quite high that this\n> would break existing scripts that expect the current behaviour that has\n> been around for many years by now. This is thus not really a viable\n> option to explore to fix the inconsistency.\n>\n> Instead, we change the behaviour of how pseudo-opts read via standard\n> input influence the flags such that the effect is fully localized. With\n> this change, when reading `--not` via standard input, it will:\n>\n>     - _Not_ influence subsequent revisions or pseudo-options passed on\n>       the command line, which is a change in behaviour.\n>\n>     - Influence pseudo-options passed via standard input.\n>\n>     - Influence normal revisions passed via standard input, which is a\n>       change in behaviour.\n\nThese all same very sane to me. Let's read on...\n\n> Thus, all flags read via standard input are fully self-contained to that\n> standard input, only.\n>\n> While this is a breaking change as well, the behaviour has only been\n> recently introduced with Git v2.42.0. Furthermore, the current behaviour\n> can be regarded as a simple bug. With that in mind it feels like the\n> right thing to do retroactively change it and make the behaviour sane.\n\nI agree. I am not so sympathetic to scripts that rely on this behavior,\nwhich feels like it is obviously broken.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> Reported-by: Christian Couder <christian.couder@gmail.com>\n> ---\n>  Documentation/rev-list-options.txt |  6 +++++-\n>  revision.c                         | 10 +++++-----\n>  t/t6017-rev-list-stdin.sh          | 21 +++++++++++++++++++++\n>  3 files changed, 31 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index a4a0cb93b2..9bf13bac53 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -151,6 +151,8 @@ endif::git-log[]\n>  --not::\n>  \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n>  \tfor all following revision specifiers, up to the next `--not`.\n> +\tWhen used on the command line before --stdin, the revisions passed\n> +\tthrough stdin will not be affected by it.\n\nHmmph. This seems to change the behavior introduced by 42cabc341c4. Am I\nreading this correctly that tips on stdin with '--not --stdin' would not\nbe marked as UNINTERESTING?\n\n(Reading this back, there are a lot of double-negatives in what I just\nwrote making it confusing for me at least. What I'm getting at here is\nthat I think `--not --stdin` should mark tips given via stdin as\nUNINTERESTING).\n\n>  --all::\n>  \tPretend as if all the refs in `refs/`, along with `HEAD`, are\n> @@ -240,7 +242,9 @@ endif::git-rev-list[]\n>  \tthem from standard input as well. This accepts commits and\n>  \tpseudo-options like `--all` and `--glob=`. When a `--` separator\n>  \tis seen, the following input is treated as paths and used to\n> -\tlimit the result.\n> +\tlimit the result. Flags like `--not` which are read via standard input\n> +\tare only respected for arguments passed in the same way and will not\n> +\tinfluence any subsequent command line arguments.\n>\n>  ifdef::git-rev-list[]\n>  --quiet::\n> diff --git a/revision.c b/revision.c\n> index 2f4c53ea20..a1f573ca06 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n>  }\n>\n>  static void read_revisions_from_stdin(struct rev_info *revs,\n> -\t\t\t\t      struct strvec *prune,\n> -\t\t\t\t      int *flags)\n> +\t\t\t\t      struct strvec *prune)\n>  {\n>  \tstruct strbuf sb;\n>  \tint seen_dashdash = 0;\n>  \tint seen_end_of_options = 0;\n>  \tint save_warning;\n> +\tint flags = 0;\n\nOK, I think this confirms my assumption that the `--not` in `--not\n--stdin` does not propagate across to the input given over stdin. I am\nnot sure that we are safely able to change that behavior.\n\nI wonder if we could instead do something like:\n\n  - inherit the current set of psuedo-opts when processing input over\n    `--stdin`\n  - allow pseudo-opts within the context of `--stdin` arbitrarily\n  - prevent those psuedo-opts applied while processing `--stdin` from\n    leaking over to subsequent command-line arguments.\n\nHere's one approach for doing that, where we make a copy of the current\nset of flags when calling `read_revisions_from_stdin()` instead of\npassing a pointer to the global state.\n\n--- 8< ---\ndiff --git a/revision.c b/revision.c\nindex a1f573ca06..d8dad35d52 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n }\n\n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t      struct strvec *prune)\n+\t\t\t\t      struct strvec *prune,\n+\t\t\t\t      int flags)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n \tint seen_end_of_options = 0;\n \tint save_warning;\n-\tint flags = 0;\n\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n@@ -2906,7 +2906,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\tread_revisions_from_stdin(revs, &prune_data,\n+\t\t\t\t\t\t\t  flags);\n \t\t\t\tcontinue;\n \t\t\t}\n--- >8 ---\n\nThanks,\n"},{"id":"482102","messageId":"xmqqmsxf5owk.fsf@gitster.g","threadId":"60252","inReplyTo":"b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im","subject":"Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-09-21T19:04:11Z","receivedAt":"2023-09-21T19:56:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Instead, we change the behaviour of how pseudo-opts read via standard\n> input influence the flags such that the effect is fully localized. With\n> this change, when reading `--not` via standard input, it will:\n>\n>     - _Not_ influence subsequent revisions or pseudo-options passed on\n>       the command line, which is a change in behaviour.\n>\n>     - Influence pseudo-options passed via standard input.\n>\n>     - Influence normal revisions passed via standard input, which is a\n>       change in behaviour.\n>\n> Thus, all flags read via standard input are fully self-contained to that\n> standard input, only.\n\nI have to wonder what the most natural expectation by end-users be,\nwhen \"cmd --opt1 --stdin --opt3 arg2\" is run and its stdin is fed\n\"--opt2 arg1\".  One interpretation may be to act as if \"--stdin\" on\nthe command line is replaced with what was read, but taken literally\nthat would make \"cmd --opt1 --opt2 arg1 --opt3 arg2\" that does not\nmake sense (i.e. options must come before arguments).  We could\ndeclare \"--stdin is replaced by options read from there, and\nnon-options read from the standard input are handled separately\",\nbut then it could be argued \"cmd --opt1 --opt2 --opt3 arg2 arg1\"\nand \"cmd --opt1 --opt2 --opt3 arg1 arg2\" are equally plausible.\n\nSo in a sense, \"what is read from --stdin is self contained\" may be\nthe easiest to explain position to take.\n\n> While this is a breaking change as well, the behaviour has only been\n> recently introduced with Git v2.42.0. Furthermore, the current behaviour\n> can be regarded as a simple bug. With that in mind it feels like the\n> right thing to do retroactively change it and make the behaviour sane.\n\nWhile I also appreciate your cautious approach to consider the risk\nthat this \"fix\" may have negative consequence, I tend to agree that\nthe behaviour is simply buggy and deserves to be fixed on the\n'maint' track.\n\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> Reported-by: Christian Couder <christian.couder@gmail.com>\n> ---\n>  Documentation/rev-list-options.txt |  6 +++++-\n>  revision.c                         | 10 +++++-----\n>  t/t6017-rev-list-stdin.sh          | 21 +++++++++++++++++++++\n>  3 files changed, 31 insertions(+), 6 deletions(-)\n>\n> diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> index a4a0cb93b2..9bf13bac53 100644\n> --- a/Documentation/rev-list-options.txt\n> +++ b/Documentation/rev-list-options.txt\n> @@ -151,6 +151,8 @@ endif::git-log[]\n>  --not::\n>  \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n>  \tfor all following revision specifiers, up to the next `--not`.\n> +\tWhen used on the command line before --stdin, the revisions passed\n> +\tthrough stdin will not be affected by it.\n\nDo we also need to say \"when read from --stdin, the revisions passed\non the command line are not affected\" as well?  I know you have it\nwhere you explian \"--stdin\" in the next hunk, but since you are\nadding one-half of the interaction, it may be less confusing to also\nmention the other half at the same time.\n\n> @@ -240,7 +242,9 @@ endif::git-rev-list[]\n>  \tthem from standard input as well. This accepts commits and\n>  \tpseudo-options like `--all` and `--glob=`. When a `--` separator\n>  \tis seen, the following input is treated as paths and used to\n> -\tlimit the result.\n> +\tlimit the result. Flags like `--not` which are read via standard input\n> +\tare only respected for arguments passed in the same way and will not\n> +\tinfluence any subsequent command line arguments.\n\nOther than that, looking good, and the changes to the code look all\nsensible.\n\nThanks.\n"},{"id":"482267","messageId":"ZRGBo1TsgSYrMt01@tanuki","threadId":"60252","inReplyTo":"ZQyPOScCKhHeZNrr@nand.local","subject":"Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-25T12:48:35Z","receivedAt":"2023-09-25T12:48:48Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 21, 2023 at 02:45:13PM -0400, Taylor Blau wrote:\n> On Thu, Sep 21, 2023 at 12:05:57PM +0200, Patrick Steinhardt wrote:\n> > When reading revisions from stdin via git-rev-list(1)'s `--stdin` option\n> > then these revisions never honor flags like `--not` which have been\n> > passed on the command line. Thus, an invocation like e.g. `git rev-list\n> > --all --not --stdin` will not treat all revisions read from stdin as\n> > uninteresting. While this behaviour may be surprising to a user, it's\n> > been this way ever since it has been introduced via 42cabc341c4 (Teach\n> > rev-list an option to read revs from the standard input., 2006-09-05).\n> \n> I'm not sure I agree that `--all --not --stdin` marking the tips given\n> on stdin as uninteresting would be surprising to users. It feels like\n> `--stdin` introduces its own \"scope\" that `--not` should apply to.\n> \n> I might be biased or looking at this differently than how other users\n> might, but `--all --not --stdin` reads like \"traverse everything except\n> what I give you over stdin\", and deviating from that behavior feels more\n> surprising than not.\n> \n> Either way, since this comes from as far back as 42cabc341c4, I think\n> that we're stuck with this behavior one way or the other ;-).\n\nI agree with all of what you say. But yes, we both come to the same\nconclusion: it's not great behaviour, but we can't really do anything\nabout it because it's been like this since basically forever.\n\nI'm a bit confused though, because further down your mail you seem to\ndisagree with what you write here by proposing to change the behaviour\nregardless.\n\n[snip]\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > Reported-by: Christian Couder <christian.couder@gmail.com>\n> > ---\n> >  Documentation/rev-list-options.txt |  6 +++++-\n> >  revision.c                         | 10 +++++-----\n> >  t/t6017-rev-list-stdin.sh          | 21 +++++++++++++++++++++\n> >  3 files changed, 31 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> > index a4a0cb93b2..9bf13bac53 100644\n> > --- a/Documentation/rev-list-options.txt\n> > +++ b/Documentation/rev-list-options.txt\n> > @@ -151,6 +151,8 @@ endif::git-log[]\n> >  --not::\n> >  \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n> >  \tfor all following revision specifiers, up to the next `--not`.\n> > +\tWhen used on the command line before --stdin, the revisions passed\n> > +\tthrough stdin will not be affected by it.\n> \n> Hmmph. This seems to change the behavior introduced by 42cabc341c4. Am I\n> reading this correctly that tips on stdin with '--not --stdin' would not\n> be marked as UNINTERESTING?\n> \n> (Reading this back, there are a lot of double-negatives in what I just\n> wrote making it confusing for me at least. What I'm getting at here is\n> that I think `--not --stdin` should mark tips given via stdin as\n> UNINTERESTING).\n\nIt does not change the behaviour, it only documents the current state\nsuch that it's at least spelled out somewhere.\n\n> >  --all::\n> >  \tPretend as if all the refs in `refs/`, along with `HEAD`, are\n> > @@ -240,7 +242,9 @@ endif::git-rev-list[]\n> >  \tthem from standard input as well. This accepts commits and\n> >  \tpseudo-options like `--all` and `--glob=`. When a `--` separator\n> >  \tis seen, the following input is treated as paths and used to\n> > -\tlimit the result.\n> > +\tlimit the result. Flags like `--not` which are read via standard input\n> > +\tare only respected for arguments passed in the same way and will not\n> > +\tinfluence any subsequent command line arguments.\n> >\n> >  ifdef::git-rev-list[]\n> >  --quiet::\n> > diff --git a/revision.c b/revision.c\n> > index 2f4c53ea20..a1f573ca06 100644\n> > --- a/revision.c\n> > +++ b/revision.c\n> > @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n> >  }\n> >\n> >  static void read_revisions_from_stdin(struct rev_info *revs,\n> > -\t\t\t\t      struct strvec *prune,\n> > -\t\t\t\t      int *flags)\n> > +\t\t\t\t      struct strvec *prune)\n> >  {\n> >  \tstruct strbuf sb;\n> >  \tint seen_dashdash = 0;\n> >  \tint seen_end_of_options = 0;\n> >  \tint save_warning;\n> > +\tint flags = 0;\n> \n> OK, I think this confirms my assumption that the `--not` in `--not\n> --stdin` does not propagate across to the input given over stdin. I am\n> not sure that we are safely able to change that behavior.\n> \n> I wonder if we could instead do something like:\n> \n>   - inherit the current set of psuedo-opts when processing input over\n>     `--stdin`\n>   - allow pseudo-opts within the context of `--stdin` arbitrarily\n>   - prevent those psuedo-opts applied while processing `--stdin` from\n>     leaking over to subsequent command-line arguments.\n> \n> Here's one approach for doing that, where we make a copy of the current\n> set of flags when calling `read_revisions_from_stdin()` instead of\n> passing a pointer to the global state.\n\nThat was indeed my first approach. But this change would break behaviour\nthat was introduced with 42cabc341c4 (Teach rev-list an option to read\nrevs from the standard input., 2006-09-05) almost 17 years ago. If we\nchange it now then this is very likely to cause problems somewhere.\n\nTo clarify:\n\n    - Since 2006 we had `--stdin`. Revisions read via `--stdin` were not\n      influenced by `--not`.\n\n    - With Git v2.41 I introduced the ability to read pseudo-opts via\n      `--stdin`. These _are_ influenced by `--not`, which is\n      inconsistent with the way we handle normal revs.\n\nI want to rectify the newly introduced pseudo-opts-via-stdin to behave\nthe same as the first point now. Like this, we make the behaviour more\nconsistent with what we did for the last 17 years.\n\nIs there anything in my commit message that can be clarified such that\nit becomes less confusing?\n\nPatrick\n\n> --- 8< ---\n> diff --git a/revision.c b/revision.c\n> index a1f573ca06..d8dad35d52 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n>  }\n> \n>  static void read_revisions_from_stdin(struct rev_info *revs,\n> -\t\t\t\t      struct strvec *prune)\n> +\t\t\t\t      struct strvec *prune,\n> +\t\t\t\t      int flags)\n>  {\n>  \tstruct strbuf sb;\n>  \tint seen_dashdash = 0;\n>  \tint seen_end_of_options = 0;\n>  \tint save_warning;\n> -\tint flags = 0;\n> \n>  \tsave_warning = warn_on_object_refname_ambiguity;\n>  \twarn_on_object_refname_ambiguity = 0;\n> @@ -2906,7 +2906,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n>  \t\t\t\t}\n>  \t\t\t\tif (revs->read_from_stdin++)\n>  \t\t\t\t\tdie(\"--stdin given twice?\");\n> -\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n> +\t\t\t\tread_revisions_from_stdin(revs, &prune_data,\n> +\t\t\t\t\t\t\t  flags);\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> --- >8 ---\n> \n> Thanks,\n"},{"id":"482268","messageId":"ZRGCUYxsN_jgkmCO@tanuki","threadId":"60252","inReplyTo":"xmqqmsxf5owk.fsf@gitster.g","subject":"Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-25T12:51:29Z","receivedAt":"2023-09-25T12:51:36Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Sep 21, 2023 at 12:04:11PM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n[snip]\n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > Reported-by: Christian Couder <christian.couder@gmail.com>\n> > ---\n> >  Documentation/rev-list-options.txt |  6 +++++-\n> >  revision.c                         | 10 +++++-----\n> >  t/t6017-rev-list-stdin.sh          | 21 +++++++++++++++++++++\n> >  3 files changed, 31 insertions(+), 6 deletions(-)\n> >\n> > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> > index a4a0cb93b2..9bf13bac53 100644\n> > --- a/Documentation/rev-list-options.txt\n> > +++ b/Documentation/rev-list-options.txt\n> > @@ -151,6 +151,8 @@ endif::git-log[]\n> >  --not::\n> >  \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n> >  \tfor all following revision specifiers, up to the next `--not`.\n> > +\tWhen used on the command line before --stdin, the revisions passed\n> > +\tthrough stdin will not be affected by it.\n> \n> Do we also need to say \"when read from --stdin, the revisions passed\n> on the command line are not affected\" as well?  I know you have it\n> where you explian \"--stdin\" in the next hunk, but since you are\n> adding one-half of the interaction, it may be less confusing to also\n> mention the other half at the same time.\n\nDoesn't hurt to make this more explicit as well, yes. I'll send a v2\nthat adds this bit.\n\nThanks!\n\nPatrick\n\n> > @@ -240,7 +242,9 @@ endif::git-rev-list[]\n> >  \tthem from standard input as well. This accepts commits and\n> >  \tpseudo-options like `--all` and `--glob=`. When a `--` separator\n> >  \tis seen, the following input is treated as paths and used to\n> > -\tlimit the result.\n> > +\tlimit the result. Flags like `--not` which are read via standard input\n> > +\tare only respected for arguments passed in the same way and will not\n> > +\tinfluence any subsequent command line arguments.\n> \n> Other than that, looking good, and the changes to the code look all\n> sensible.\n> \n> Thanks.\n"},{"id":"482269","messageId":"6221acd2796853144f8e84081655fbc79fdc6634.1695646898.git.ps@pks.im","threadId":"60252","inReplyTo":"b93d4c8c23552abab64084b62f27944e7e192c0c.1695290733.git.ps@pks.im","subject":"[PATCH v2] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-09-25T13:02:00Z","receivedAt":"2023-09-25T13:02:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"When reading revisions from stdin via git-rev-list(1)'s `--stdin` option\nthen these revisions never honor flags like `--not` which have been\npassed on the command line. Thus, an invocation like e.g. `git rev-list\n--all --not --stdin` will not treat all revisions read from stdin as\nuninteresting. While this behaviour may be surprising to a user, it's\nbeen this way ever since it has been introduced via 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05).\n\nWith that said, in c40f0b7877 (revision: handle pseudo-opts in `--stdin`\nmode, 2023-06-15) we have introduced a new mode to read pseudo opts from\nstandard input where this behaviour is a lot more confusing. If you pass\n`--not` via stdin, it will:\n\n    - Influence subsequent revisions or pseudo-options passed on the\n      command line.\n\n    - Influence pseudo-options passed via standard input.\n\n    - _Not_ influence normal revisions passed via standard input.\n\nThis behaviour is extremely inconsistent and bound to cause confusion.\n\nWhile it would be nice to retroactively change the behaviour for how\n`--not` and `--stdin` behave together, chances are quite high that this\nwould break existing scripts that expect the current behaviour that has\nbeen around for many years by now. This is thus not really a viable\noption to explore to fix the inconsistency.\n\nInstead, we change the behaviour of how pseudo-opts read via standard\ninput influence the flags such that the effect is fully localized. With\nthis change, when reading `--not` via standard input, it will:\n\n    - _Not_ influence subsequent revisions or pseudo-options passed on\n      the command line, which is a change in behaviour.\n\n    - Influence pseudo-options passed via standard input.\n\n    - Influence normal revisions passed via standard input, which is a\n      change in behaviour.\n\nThus, all flags read via standard input are fully self-contained to that\nstandard input, only.\n\nWhile this is a breaking change as well, the behaviour has only been\nrecently introduced with Git v2.42.0. Furthermore, the current behaviour\ncan be regarded as a simple bug. With that in mind it feels like the\nright thing to retroactively change it and make the behaviour sane.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\nReported-by: Christian Couder <christian.couder@gmail.com>\n---\nRange-diff against v1:\n1:  b93d4c8c23 ! 1:  6221acd279 revision: make pseudo-opt flags read via stdin behave consistently\n    @@ Commit message\n         While this is a breaking change as well, the behaviour has only been\n         recently introduced with Git v2.42.0. Furthermore, the current behaviour\n         can be regarded as a simple bug. With that in mind it feels like the\n    -    right thing to do retroactively change it and make the behaviour sane.\n    +    right thing to retroactively change it and make the behaviour sane.\n     \n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n         Reported-by: Christian Couder <christian.couder@gmail.com>\n    @@ Documentation/rev-list-options.txt: endif::git-log[]\n      \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n      \tfor all following revision specifiers, up to the next `--not`.\n     +\tWhen used on the command line before --stdin, the revisions passed\n    -+\tthrough stdin will not be affected by it.\n    ++\tthrough stdin will not be affected by it. Conversely, when passed\n    ++\tvia standard input, the revisions passed on the command line will\n    ++\tnot be affected by it.\n      \n      --all::\n      \tPretend as if all the refs in `refs/`, along with `HEAD`, are\n\n Documentation/rev-list-options.txt |  8 +++++++-\n revision.c                         | 10 +++++-----\n t/t6017-rev-list-stdin.sh          | 21 +++++++++++++++++++++\n 3 files changed, 33 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex a4a0cb93b2..66d71d1b95 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -151,6 +151,10 @@ endif::git-log[]\n --not::\n \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n \tfor all following revision specifiers, up to the next `--not`.\n+\tWhen used on the command line before --stdin, the revisions passed\n+\tthrough stdin will not be affected by it. Conversely, when passed\n+\tvia standard input, the revisions passed on the command line will\n+\tnot be affected by it.\n \n --all::\n \tPretend as if all the refs in `refs/`, along with `HEAD`, are\n@@ -240,7 +244,9 @@ endif::git-rev-list[]\n \tthem from standard input as well. This accepts commits and\n \tpseudo-options like `--all` and `--glob=`. When a `--` separator\n \tis seen, the following input is treated as paths and used to\n-\tlimit the result.\n+\tlimit the result. Flags like `--not` which are read via standard input\n+\tare only respected for arguments passed in the same way and will not\n+\tinfluence any subsequent command line arguments.\n \n ifdef::git-rev-list[]\n --quiet::\ndiff --git a/revision.c b/revision.c\nindex 2f4c53ea20..a1f573ca06 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2788,13 +2788,13 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n }\n \n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t      struct strvec *prune,\n-\t\t\t\t      int *flags)\n+\t\t\t\t      struct strvec *prune)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n \tint seen_end_of_options = 0;\n \tint save_warning;\n+\tint flags = 0;\n \n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n@@ -2817,13 +2817,13 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\t\tcontinue;\n \t\t\t}\n \n-\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags) > 0)\n+\t\t\tif (handle_revision_pseudo_opt(revs, argv, &flags) > 0)\n \t\t\t\tcontinue;\n \n \t\t\tdie(_(\"invalid option '%s' in --stdin mode\"), sb.buf);\n \t\t}\n \n-\t\tif (handle_revision_arg(sb.buf, revs, 0,\n+\t\tif (handle_revision_arg(sb.buf, revs, flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\n@@ -2906,7 +2906,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\t\tread_revisions_from_stdin(revs, &prune_data, &flags);\n+\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n \t\t\t\tcontinue;\n \t\t\t}\n \ndiff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh\nindex a57f1ae2ba..4821b90e74 100755\n--- a/t/t6017-rev-list-stdin.sh\n+++ b/t/t6017-rev-list-stdin.sh\n@@ -68,6 +68,7 @@ check --glob=refs/heads\n check --glob=refs/heads --\n check --glob=refs/heads -- file-1\n check --end-of-options -dashed-branch\n+check --all --not refs/heads/main\n \n test_expect_success 'not only --stdin' '\n \tcat >expect <<-EOF &&\n@@ -127,4 +128,24 @@ test_expect_success 'unknown option without --end-of-options' '\n \ttest_cmp expect error\n '\n \n+test_expect_success '--not on command line does not influence revisions read via --stdin' '\n+\tcat >input <<-EOF &&\n+\trefs/heads/main\n+\tEOF\n+\tgit rev-list refs/heads/main >expect &&\n+\n+\tgit rev-list refs/heads/main --not --stdin <input >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success '--not via stdin does not influence revisions from command line' '\n+\tcat >input <<-EOF &&\n+\t--not\n+\tEOF\n+\tgit rev-list refs/heads/main >expect &&\n+\n+\tgit rev-list refs/heads/main --stdin refs/heads/main <input >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.42.0\n\n"},{"id":"482311","messageId":"ZRIN9gJsRgG4qj5r@nand.local","threadId":"60252","inReplyTo":"ZRGBo1TsgSYrMt01@tanuki","subject":"Re: [PATCH] revision: make pseudo-opt flags read via stdin behave consistently","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2023-09-25T22:47:18Z","receivedAt":"2023-09-25T22:47:23Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 25, 2023 at 02:48:35PM +0200, Patrick Steinhardt wrote:\n> > > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\n> > > index a4a0cb93b2..9bf13bac53 100644\n> > > --- a/Documentation/rev-list-options.txt\n> > > +++ b/Documentation/rev-list-options.txt\n> > > @@ -151,6 +151,8 @@ endif::git-log[]\n> > >  --not::\n> > >  \tReverses the meaning of the '{caret}' prefix (or lack thereof)\n> > >  \tfor all following revision specifiers, up to the next `--not`.\n> > > +\tWhen used on the command line before --stdin, the revisions passed\n> > > +\tthrough stdin will not be affected by it.\n> >\n> > Hmmph. This seems to change the behavior introduced by 42cabc341c4. Am I\n> > reading this correctly that tips on stdin with '--not --stdin' would not\n> > be marked as UNINTERESTING?\n> >\n> > (Reading this back, there are a lot of double-negatives in what I just\n> > wrote making it confusing for me at least. What I'm getting at here is\n> > that I think `--not --stdin` should mark tips given via stdin as\n> > UNINTERESTING).\n>\n> It does not change the behaviour, it only documents the current state\n> such that it's at least spelled out somewhere.\n\nSorry, I must have been confused when writing this :-<. Looking at it\nagain, I agree that the current behavior is that \"--not --stdin\" treats\nany tips given over stdin as INTERESTING, so this documentation is\nconsistent with that.\n\n> > I wonder if we could instead do something like:\n> >\n> >   - inherit the current set of psuedo-opts when processing input over\n> >     `--stdin`\n> >   - allow pseudo-opts within the context of `--stdin` arbitrarily\n> >   - prevent those psuedo-opts applied while processing `--stdin` from\n> >     leaking over to subsequent command-line arguments.\n> >\n> > Here's one approach for doing that, where we make a copy of the current\n> > set of flags when calling `read_revisions_from_stdin()` instead of\n> > passing a pointer to the global state.\n>\n> That was indeed my first approach. But this change would break behaviour\n> that was introduced with 42cabc341c4 (Teach rev-list an option to read\n> revs from the standard input., 2006-09-05) almost 17 years ago. If we\n> change it now then this is very likely to cause problems somewhere.\n\nPer above, you're absolutely right. I think that the patch that you\nproposed here LGTM.\n\nThanks,\nTaylor\n"}]}