revision: make pseudo-opt flags read via stdin behave consistently

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index a4a0cb93b241b8d5d9c9bc9b200a277a0e4f7992..66d71d1b957683102b1fe748f3327f20b2c90187 100644 (file)
--- 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 2f4c53ea207b22a68097708b327884ae970c1978..a1f573ca060aa9363f12b5dd9ddebfd19ea252af 100644 (file)
--- 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 a57f1ae2baa4a72e88183fcaa3b55802a80599ef..4821b90e7479ad8f4878ab4432ce0e9d2ce47de5 100755 (executable)
--- 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