[PATCH 0/3] revision: handle pseudo-opts in `--stdin` mode

[PATCH 0/3] revision: handle pseudo-opts in `--stdin` mode

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

Hi,

both git-rev-list(1) and git-log(1) provide a `--stdin` mode that will
cause these commands to read additional commits from standard input. The
primary usecase will typically be scripts that potentially process a
long list of commits which might otherwise bust command line length
limits.

One notable omission though is that this mode does not allow us to pass
pseudo-options like `--all`, `--branch`, `--glob=` or `--not`. And while
it is possible to mix command line arguments and `--stdin` in order to
mix both pseudo-options and a potentially large set of commits, the end
result is arguably more complex for the user.

This patch series thus implements support for handling pseudo-options in
`--stdin` mode for both git-rev-list(1) and git-log(1).

Patrick

Patrick Steinhardt (3):
  revision: reorder `read_revisions_from_stdin()`
  revision: small readability improvement for reading from stdin
  revision: handle pseudo-opts in `--stdin` mode

 Documentation/rev-list-options.txt |  9 ++--
 revision.c                         | 76 +++++++++++++++++-------------
 t/t6017-rev-list-stdin.sh          | 32 +++++++++++++
 3 files changed, 79 insertions(+), 38 deletions(-)

-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 1/3] revision: reorder `read_revisions_from_stdin()`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

Reorder `read_revisions_from_stdin()` so that we can start using
`handle_revision_pseudo_opt()` without a forward declaration in a
subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c | 66 +++++++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/revision.c b/revision.c
index b33cc1d106..cc22ccd76e 100644
--- a/revision.c
+++ b/revision.c
@@ -2195,39 +2195,6 @@ static void read_pathspec_from_stdin(struct strbuf *sb,
 		strvec_push(prune, sb->buf);
 }
 
-static void read_revisions_from_stdin(struct rev_info *revs,
-				      struct strvec *prune)
-{
-	struct strbuf sb;
-	int seen_dashdash = 0;
-	int save_warning;
-
-	save_warning = warn_on_object_refname_ambiguity;
-	warn_on_object_refname_ambiguity = 0;
-
-	strbuf_init(&sb, 1000);
-	while (strbuf_getline(&sb, stdin) != EOF) {
-		int len = sb.len;
-		if (!len)
-			break;
-		if (sb.buf[0] == '-') {
-			if (len == 2 && sb.buf[1] == '-') {
-				seen_dashdash = 1;
-				break;
-			}
-			die("options not supported in --stdin mode");
-		}
-		if (handle_revision_arg(sb.buf, revs, 0,
-					REVARG_CANNOT_BE_FILENAME))
-			die("bad revision '%s'", sb.buf);
-	}
-	if (seen_dashdash)
-		read_pathspec_from_stdin(&sb, prune);
-
-	strbuf_release(&sb);
-	warn_on_object_refname_ambiguity = save_warning;
-}
-
 static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
 {
 	append_grep_pattern(&revs->grep_filter, ptn, "command line", 0, what);
@@ -2816,6 +2783,39 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 	return 1;
 }
 
+static void read_revisions_from_stdin(struct rev_info *revs,
+				      struct strvec *prune)
+{
+	struct strbuf sb;
+	int seen_dashdash = 0;
+	int save_warning;
+
+	save_warning = warn_on_object_refname_ambiguity;
+	warn_on_object_refname_ambiguity = 0;
+
+	strbuf_init(&sb, 1000);
+	while (strbuf_getline(&sb, stdin) != EOF) {
+		int len = sb.len;
+		if (!len)
+			break;
+		if (sb.buf[0] == '-') {
+			if (len == 2 && sb.buf[1] == '-') {
+				seen_dashdash = 1;
+				break;
+			}
+			die("options not supported in --stdin mode");
+		}
+		if (handle_revision_arg(sb.buf, revs, 0,
+					REVARG_CANNOT_BE_FILENAME))
+			die("bad revision '%s'", sb.buf);
+	}
+	if (seen_dashdash)
+		read_pathspec_from_stdin(&sb, prune);
+
+	strbuf_release(&sb);
+	warn_on_object_refname_ambiguity = save_warning;
+}
+
 static void NORETURN diagnose_missing_default(const char *def)
 {
 	int flags;
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] revision: reorder `read_revisions_from_stdin()`

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Reorder `read_revisions_from_stdin()` so that we can start using
> `handle_revision_pseudo_opt()` without a forward declaration in a
> subsequent commit.

Makes sense.

[PATCH 2/3] revision: small readability improvement for reading from stdin

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1028 bytes --]

The code that reads lines from standard input manually compares whether
the read line matches "--", which is a bit awkward to read. Refactor it
to instead use strcmp(3P) for a small code style improvement.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index cc22ccd76e..dcb7951b4e 100644
--- a/revision.c
+++ b/revision.c
@@ -2795,16 +2795,18 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 
 	strbuf_init(&sb, 1000);
 	while (strbuf_getline(&sb, stdin) != EOF) {
-		int len = sb.len;
-		if (!len)
+		if (!sb.len)
 			break;
+
 		if (sb.buf[0] == '-') {
-			if (len == 2 && sb.buf[1] == '-') {
+			if (!strcmp(sb.buf, "--")) {
 				seen_dashdash = 1;
 				break;
 			}
+
 			die("options not supported in --stdin mode");
 		}
+
 		if (handle_revision_arg(sb.buf, revs, 0,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] revision: small readability improvement for reading from stdin

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The code that reads lines from standard input manually compares whether
> the read line matches "--", which is a bit awkward to read. Refactor it
> to instead use strcmp(3P) for a small code style improvement.

It is unclear if it is an "improvement".  We've already checked the
first byte to be "-" and we only need to check the second one (and
the length of the string) to see if we have a double-dash, instead
of checking the first byte again.

I am not sure if these excessive blank lines are helping, either.

The only reason I would be inclined to support the main change in
this patch (but not additional blank lines) is because I will be
suggesting to lift the logic to detect double-dash from the "line
begins with dash" block in my review of the next step.  Once that is
done, double-dash detection cannot rely on the fact that we have
already checked the first byte.

I do agree with the change to remove the temporary variable "len",
if we were to remove the "len == 2" comparison, as it makes "len"
less useful.

Thanks.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  revision.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/revision.c b/revision.c
> index cc22ccd76e..dcb7951b4e 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2795,16 +2795,18 @@ static void read_revisions_from_stdin(struct rev_info *revs,
>  
>  	strbuf_init(&sb, 1000);
>  	while (strbuf_getline(&sb, stdin) != EOF) {
> -		int len = sb.len;
> -		if (!len)
> +		if (!sb.len)
>  			break;
> +
>  		if (sb.buf[0] == '-') {
> -			if (len == 2 && sb.buf[1] == '-') {
> +			if (!strcmp(sb.buf, "--")) {
>  				seen_dashdash = 1;
>  				break;
>  			}
> +
>  			die("options not supported in --stdin mode");
>  		}
> +
>  		if (handle_revision_arg(sb.buf, revs, 0,
>  					REVARG_CANNOT_BE_FILENAME))
>  			die("bad revision '%s'", sb.buf);

[PATCH 3/3] revision: handle pseudo-opts in `--stdin` mode

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 4632 bytes --]

While both git-rev-list(1) and git-log(1) support `--stdin`, it only
accepts commits and files. Most notably, it is impossible to pass any of
the pseudo-opts like `--all`, `--glob=` or others via stdin.

This makes it hard to use this function in certain scripted scenarios,
like when one wants to support queries against specific revisions, but
also against reference patterns. While this is theoretically possible by
using arguments, this may run into issues once we hit platform limits
with sufficiently large queries. And because `--stdin` cannot handle
pseudo-opts, the only alternative would be to use a mixture of arguments
and standard input, which is cumbersome.

Implement support for handling pseudo-opts in both commands to support
this usecase better. One notable restriction here is that `--stdin` only
supports "stuck" arguments in the form of `--glob=foo`. This is because
"unstuck" arguments would also require us to read the next line, which
would add quite some complexity to the code. This restriction should be
fine for scripted usage though.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/rev-list-options.txt |  9 +++++----
 revision.c                         | 12 ++++++++---
 t/t6017-rev-list-stdin.sh          | 32 ++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 3000888a90..e6468bf0eb 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -236,10 +236,11 @@ ifndef::git-rev-list[]
 endif::git-rev-list[]
 
 --stdin::
-	In addition to the '<commit>' listed on the command
-	line, read them from the standard input. If a `--` separator is
-	seen, stop reading commits and start reading paths to limit the
-	result.
+	In addition to getting arguments from the command line, read
+	them for 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.
 
 ifdef::git-rev-list[]
 --quiet::
diff --git a/revision.c b/revision.c
index dcb7951b4e..b90794cca4 100644
--- a/revision.c
+++ b/revision.c
@@ -2784,7 +2784,8 @@ 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;
@@ -2799,12 +2800,17 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			break;
 
 		if (sb.buf[0] == '-') {
+			const char *argv[2] = { sb.buf, NULL };
+
 			if (!strcmp(sb.buf, "--")) {
 				seen_dashdash = 1;
 				break;
 			}
 
-			die("options not supported in --stdin mode");
+			if (handle_revision_pseudo_opt(revs, argv, flags))
+				continue;
+
+			die(_("option '%s' not supported in --stdin mode"), sb.buf);
 		}
 
 		if (handle_revision_arg(sb.buf, revs, 0,
@@ -2890,7 +2896,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);
+				read_revisions_from_stdin(revs, &prune_data, &flags);
 				continue;
 			}
 
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 05162512a0..11cb27c3ed 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -60,6 +60,11 @@ check side-1 ^side-7 -- file-2
 check side-3 ^side-4 -- file-3
 check side-3 ^side-2
 check side-3 ^side-2 -- file-1
+check --all
+check --all --not --branches
+check --glob=refs/heads
+check --glob=refs/heads --
+check --glob=refs/heads -- file-1
 
 test_expect_success 'not only --stdin' '
 	cat >expect <<-EOF &&
@@ -78,4 +83,31 @@ test_expect_success 'not only --stdin' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pseudo-opt with missing value' '
+	cat >input <<-EOF &&
+	--glob
+	refs/heads
+	EOF
+
+	cat >expect <<-EOF &&
+	fatal: Option ${SQ}--glob${SQ} requires a value
+	EOF
+
+	test_must_fail git rev-list --stdin <input 2>error &&
+	test_cmp expect error
+'
+
+test_expect_success 'unknown options' '
+	cat >input <<-EOF &&
+	--unknown=value
+	EOF
+
+	cat >expect <<-EOF &&
+	fatal: option ${SQ}--unknown=value${SQ} not supported in --stdin mode
+	EOF
+
+	test_must_fail git rev-list --stdin <input 2>error &&
+	test_cmp expect error
+'
+
 test_done
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] revision: handle pseudo-opts in `--stdin` mode

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>  		if (sb.buf[0] == '-') {
> +			const char *argv[2] = { sb.buf, NULL };
> +
>  			if (!strcmp(sb.buf, "--")) {
>  				seen_dashdash = 1;
>  				break;
>  			}
>  
> -			die("options not supported in --stdin mode");
> +			if (handle_revision_pseudo_opt(revs, argv, flags))
> +				continue;
> +
> +			die(_("option '%s' not supported in --stdin mode"), sb.buf);
>  		}

The reason why we had the double-dash checking inside this block is
because we rejected any dashed options other than double-dash, but
with this step, that is no longer true, so it may make more sense to
move the "seen_dashdash" code outside the block.  Also unlike the
command line options that allows "--end-of-options" marker to allow
tweaking how a failing handle_revision_arg() call is handled, this
codepath does not seem to have any matching provision.  In the
original code, which did not accept any options, it was perfectly
understandable that it was unaware of "--end-of-options", but now we
do allow dashed options, shouldn't we be supporting it here, too?

Thanks.

Re: [PATCH 3/3] revision: handle pseudo-opts in `--stdin` mode

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1705 bytes --]

On Wed, Jun 14, 2023 at 08:56:00AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >  		if (sb.buf[0] == '-') {
> > +			const char *argv[2] = { sb.buf, NULL };
> > +
> >  			if (!strcmp(sb.buf, "--")) {
> >  				seen_dashdash = 1;
> >  				break;
> >  			}
> >  
> > -			die("options not supported in --stdin mode");
> > +			if (handle_revision_pseudo_opt(revs, argv, flags))
> > +				continue;
> > +
> > +			die(_("option '%s' not supported in --stdin mode"), sb.buf);
> >  		}
> 
> The reason why we had the double-dash checking inside this block is
> because we rejected any dashed options other than double-dash, but
> with this step, that is no longer true, so it may make more sense to
> move the "seen_dashdash" code outside the block.  Also unlike the
> command line options that allows "--end-of-options" marker to allow
> tweaking how a failing handle_revision_arg() call is handled, this
> codepath does not seem to have any matching provision.  In the
> original code, which did not accept any options, it was perfectly
> understandable that it was unaware of "--end-of-options", but now we
> do allow dashed options, shouldn't we be supporting it here, too?
> 
> Thanks.

Good point, we should. Most importantly, the code would currently refuse
to enumerate references with a leading dash, and there would be no way
to work around this issue.

E.g. this works:

$ git rev-list --end-of-options -branch

Whereas neither of these do in the current version:

$ printf "%s\n" -branch | git rev-list
$ printf "%s\n" --end-of-options -branch | git rev-list

Will send a v2 with `--end-of-options` handling, thanks!

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 0/3] revision: handle pseudo-opts in `--stdin` mode

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 6470 bytes --]

Hi,

this is the second version of my patch series to support handling of
pseudo-opts like `--all` or `--glob=` in both git-rev-list(1)'s and
git-log(1)'s `--stdin` mode.

Changes compared to v1:

    - Patch 2/3: Changed the small refactoring to hoist out the check
      for "--" out of the `if (sb.buf[0] == '-')` block completely to
      make for an easier read.

    - Patch 3/3: Implemented support for `--end-of-options` so that it
      becomes possible to ask for references that have a leading dash.

    - Patch 3/3: Fixed an issue where invalid pseudo-opts (e.g.
      `--no-walk=garbage`) would not be recognized.

This is based on Junio's feedback, thank you!

Patrick

Patrick Steinhardt (3):
  revision: reorder `read_revisions_from_stdin()`
  revision: small readability improvement for reading from stdin
  revision: handle pseudo-opts in `--stdin` mode

 Documentation/rev-list-options.txt |  9 ++--
 revision.c                         | 82 +++++++++++++++++-------------
 t/t6017-rev-list-stdin.sh          | 51 ++++++++++++++++++-
 3 files changed, 103 insertions(+), 39 deletions(-)

Range-diff against v1:
1:  6cd4f79482 = 1:  6cd4f79482 revision: reorder `read_revisions_from_stdin()`
2:  38c0415ee9 ! 2:  5c1a9a0d08 revision: small readability improvement for reading from stdin
    @@ Commit message
         revision: small readability improvement for reading from stdin
     
         The code that reads lines from standard input manually compares whether
    -    the read line matches "--", which is a bit awkward to read. Refactor it
    -    to instead use strcmp(3P) for a small code style improvement.
    +    the read line matches "--", which is a bit awkward to read. Furthermore,
    +    we're about to extend the code to also support reading pseudo-options
    +    via standard input, requiring more elaborate handling of lines with a
    +    leading dash.
    +
    +    Refactor the code by hoisting out the check for "--" outside of the
    +    block that checks for a leading dash.
     
         Signed-off-by: Patrick Steinhardt <ps@pks.im>
     
    @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
     -		int len = sb.len;
     -		if (!len)
     +		if (!sb.len)
    - 			break;
    ++			break;
     +
    - 		if (sb.buf[0] == '-') {
    ++		if (!strcmp(sb.buf, "--")) {
    ++			seen_dashdash = 1;
    + 			break;
    +-		if (sb.buf[0] == '-') {
     -			if (len == 2 && sb.buf[1] == '-') {
    -+			if (!strcmp(sb.buf, "--")) {
    - 				seen_dashdash = 1;
    - 				break;
    - 			}
    -+
    - 			die("options not supported in --stdin mode");
    +-				seen_dashdash = 1;
    +-				break;
    +-			}
    +-			die("options not supported in --stdin mode");
      		}
    ++
    ++		if (sb.buf[0] == '-')
    ++			die("options not supported in --stdin mode");
     +
      		if (handle_revision_arg(sb.buf, revs, 0,
      					REVARG_CANNOT_BE_FILENAME))
3:  4575917127 ! 3:  0cf189b378 revision: handle pseudo-opts in `--stdin` mode
    @@ revision.c: static int handle_revision_pseudo_opt(struct rev_info *revs,
      {
      	struct strbuf sb;
      	int seen_dashdash = 0;
    ++	int seen_end_of_options = 0;
    + 	int save_warning;
    + 
    + 	save_warning = warn_on_object_refname_ambiguity;
     @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,
      			break;
    + 		}
      
    - 		if (sb.buf[0] == '-') {
    -+			const char *argv[2] = { sb.buf, NULL };
    -+
    - 			if (!strcmp(sb.buf, "--")) {
    - 				seen_dashdash = 1;
    - 				break;
    - 			}
    - 
    +-		if (sb.buf[0] == '-')
     -			die("options not supported in --stdin mode");
    -+			if (handle_revision_pseudo_opt(revs, argv, flags))
    ++		if (!seen_end_of_options && sb.buf[0] == '-') {
    ++			const char *argv[] = { sb.buf, NULL };
    ++
    ++			if (!strcmp(sb.buf, "--end-of-options")) {
    ++				seen_end_of_options = 1;
     +				continue;
    ++			}
     +
    -+			die(_("option '%s' not supported in --stdin mode"), sb.buf);
    - 		}
    ++			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,
    + 					REVARG_CANNOT_BE_FILENAME))
     @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
      				}
      				if (revs->read_from_stdin++)
    @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re
      
     
      ## t/t6017-rev-list-stdin.sh ##
    +@@ t/t6017-rev-list-stdin.sh: test_expect_success setup '
    + 			git add file-$i &&
    + 			test_tick &&
    + 			git commit -m side-$i || exit
    +-		done
    ++		done &&
    ++
    ++		git update-ref refs/heads/-dashed-branch HEAD
    + 	)
    + '
    + 
     @@ t/t6017-rev-list-stdin.sh: check side-1 ^side-7 -- file-2
      check side-3 ^side-4 -- file-3
      check side-3 ^side-2
    @@ t/t6017-rev-list-stdin.sh: check side-1 ^side-7 -- file-2
     +check --glob=refs/heads
     +check --glob=refs/heads --
     +check --glob=refs/heads -- file-1
    ++check --end-of-options -dashed-branch
      
      test_expect_success 'not only --stdin' '
      	cat >expect <<-EOF &&
    @@ t/t6017-rev-list-stdin.sh: test_expect_success 'not only --stdin' '
     +	test_cmp expect error
     +'
     +
    -+test_expect_success 'unknown options' '
    ++test_expect_success 'pseudo-opt with invalid value' '
    ++	cat >input <<-EOF &&
    ++	--no-walk=garbage
    ++	EOF
    ++
    ++	cat >expect <<-EOF &&
    ++	error: invalid argument to --no-walk
    ++	fatal: invalid option ${SQ}--no-walk=garbage${SQ} in --stdin mode
    ++	EOF
    ++
    ++	test_must_fail git rev-list --stdin <input 2>error &&
    ++	test_cmp expect error
    ++'
    ++
    ++test_expect_success 'unknown option without --end-of-options' '
     +	cat >input <<-EOF &&
    -+	--unknown=value
    ++	-dashed-branch
     +	EOF
     +
     +	cat >expect <<-EOF &&
    -+	fatal: option ${SQ}--unknown=value${SQ} not supported in --stdin mode
    ++	fatal: invalid option ${SQ}-dashed-branch${SQ} in --stdin mode
     +	EOF
     +
     +	test_must_fail git rev-list --stdin <input 2>error &&
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 1/3] revision: reorder `read_revisions_from_stdin()`

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2605 bytes --]

Reorder `read_revisions_from_stdin()` so that we can start using
`handle_revision_pseudo_opt()` without a forward declaration in a
subsequent commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c | 66 +++++++++++++++++++++++++++---------------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/revision.c b/revision.c
index b33cc1d106..cc22ccd76e 100644
--- a/revision.c
+++ b/revision.c
@@ -2195,39 +2195,6 @@ static void read_pathspec_from_stdin(struct strbuf *sb,
 		strvec_push(prune, sb->buf);
 }
 
-static void read_revisions_from_stdin(struct rev_info *revs,
-				      struct strvec *prune)
-{
-	struct strbuf sb;
-	int seen_dashdash = 0;
-	int save_warning;
-
-	save_warning = warn_on_object_refname_ambiguity;
-	warn_on_object_refname_ambiguity = 0;
-
-	strbuf_init(&sb, 1000);
-	while (strbuf_getline(&sb, stdin) != EOF) {
-		int len = sb.len;
-		if (!len)
-			break;
-		if (sb.buf[0] == '-') {
-			if (len == 2 && sb.buf[1] == '-') {
-				seen_dashdash = 1;
-				break;
-			}
-			die("options not supported in --stdin mode");
-		}
-		if (handle_revision_arg(sb.buf, revs, 0,
-					REVARG_CANNOT_BE_FILENAME))
-			die("bad revision '%s'", sb.buf);
-	}
-	if (seen_dashdash)
-		read_pathspec_from_stdin(&sb, prune);
-
-	strbuf_release(&sb);
-	warn_on_object_refname_ambiguity = save_warning;
-}
-
 static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)
 {
 	append_grep_pattern(&revs->grep_filter, ptn, "command line", 0, what);
@@ -2816,6 +2783,39 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,
 	return 1;
 }
 
+static void read_revisions_from_stdin(struct rev_info *revs,
+				      struct strvec *prune)
+{
+	struct strbuf sb;
+	int seen_dashdash = 0;
+	int save_warning;
+
+	save_warning = warn_on_object_refname_ambiguity;
+	warn_on_object_refname_ambiguity = 0;
+
+	strbuf_init(&sb, 1000);
+	while (strbuf_getline(&sb, stdin) != EOF) {
+		int len = sb.len;
+		if (!len)
+			break;
+		if (sb.buf[0] == '-') {
+			if (len == 2 && sb.buf[1] == '-') {
+				seen_dashdash = 1;
+				break;
+			}
+			die("options not supported in --stdin mode");
+		}
+		if (handle_revision_arg(sb.buf, revs, 0,
+					REVARG_CANNOT_BE_FILENAME))
+			die("bad revision '%s'", sb.buf);
+	}
+	if (seen_dashdash)
+		read_pathspec_from_stdin(&sb, prune);
+
+	strbuf_release(&sb);
+	warn_on_object_refname_ambiguity = save_warning;
+}
+
 static void NORETURN diagnose_missing_default(const char *def)
 {
 	int flags;
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/3] revision: small readability improvement for reading from stdin

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1355 bytes --]

The code that reads lines from standard input manually compares whether
the read line matches "--", which is a bit awkward to read. Furthermore,
we're about to extend the code to also support reading pseudo-options
via standard input, requiring more elaborate handling of lines with a
leading dash.

Refactor the code by hoisting out the check for "--" outside of the
block that checks for a leading dash.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 revision.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/revision.c b/revision.c
index cc22ccd76e..3a39f41bb8 100644
--- a/revision.c
+++ b/revision.c
@@ -2795,16 +2795,17 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 
 	strbuf_init(&sb, 1000);
 	while (strbuf_getline(&sb, stdin) != EOF) {
-		int len = sb.len;
-		if (!len)
+		if (!sb.len)
+			break;
+
+		if (!strcmp(sb.buf, "--")) {
+			seen_dashdash = 1;
 			break;
-		if (sb.buf[0] == '-') {
-			if (len == 2 && sb.buf[1] == '-') {
-				seen_dashdash = 1;
-				break;
-			}
-			die("options not supported in --stdin mode");
 		}
+
+		if (sb.buf[0] == '-')
+			die("options not supported in --stdin mode");
+
 		if (handle_revision_arg(sb.buf, revs, 0,
 					REVARG_CANNOT_BE_FILENAME))
 			die("bad revision '%s'", sb.buf);
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 3/3] revision: handle pseudo-opts in `--stdin` mode

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 5460 bytes --]

While both git-rev-list(1) and git-log(1) support `--stdin`, it only
accepts commits and files. Most notably, it is impossible to pass any of
the pseudo-opts like `--all`, `--glob=` or others via stdin.

This makes it hard to use this function in certain scripted scenarios,
like when one wants to support queries against specific revisions, but
also against reference patterns. While this is theoretically possible by
using arguments, this may run into issues once we hit platform limits
with sufficiently large queries. And because `--stdin` cannot handle
pseudo-opts, the only alternative would be to use a mixture of arguments
and standard input, which is cumbersome.

Implement support for handling pseudo-opts in both commands to support
this usecase better. One notable restriction here is that `--stdin` only
supports "stuck" arguments in the form of `--glob=foo`. This is because
"unstuck" arguments would also require us to read the next line, which
would add quite some complexity to the code. This restriction should be
fine for scripted usage though.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 Documentation/rev-list-options.txt |  9 +++---
 revision.c                         | 21 +++++++++---
 t/t6017-rev-list-stdin.sh          | 51 +++++++++++++++++++++++++++++-
 3 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt
index 3000888a90..e6468bf0eb 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -236,10 +236,11 @@ ifndef::git-rev-list[]
 endif::git-rev-list[]
 
 --stdin::
-	In addition to the '<commit>' listed on the command
-	line, read them from the standard input. If a `--` separator is
-	seen, stop reading commits and start reading paths to limit the
-	result.
+	In addition to getting arguments from the command line, read
+	them for 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.
 
 ifdef::git-rev-list[]
 --quiet::
diff --git a/revision.c b/revision.c
index 3a39f41bb8..a0b147913f 100644
--- a/revision.c
+++ b/revision.c
@@ -2784,10 +2784,12 @@ 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;
 
 	save_warning = warn_on_object_refname_ambiguity;
@@ -2803,8 +2805,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,
 			break;
 		}
 
-		if (sb.buf[0] == '-')
-			die("options not supported in --stdin mode");
+		if (!seen_end_of_options && sb.buf[0] == '-') {
+			const char *argv[] = { sb.buf, NULL };
+
+			if (!strcmp(sb.buf, "--end-of-options")) {
+				seen_end_of_options = 1;
+				continue;
+			}
+
+			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,
 					REVARG_CANNOT_BE_FILENAME))
@@ -2889,7 +2902,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);
+				read_revisions_from_stdin(revs, &prune_data, &flags);
 				continue;
 			}
 
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 05162512a0..a57f1ae2ba 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -48,7 +48,9 @@ test_expect_success setup '
 			git add file-$i &&
 			test_tick &&
 			git commit -m side-$i || exit
-		done
+		done &&
+
+		git update-ref refs/heads/-dashed-branch HEAD
 	)
 '
 
@@ -60,6 +62,12 @@ check side-1 ^side-7 -- file-2
 check side-3 ^side-4 -- file-3
 check side-3 ^side-2
 check side-3 ^side-2 -- file-1
+check --all
+check --all --not --branches
+check --glob=refs/heads
+check --glob=refs/heads --
+check --glob=refs/heads -- file-1
+check --end-of-options -dashed-branch
 
 test_expect_success 'not only --stdin' '
 	cat >expect <<-EOF &&
@@ -78,4 +86,45 @@ test_expect_success 'not only --stdin' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pseudo-opt with missing value' '
+	cat >input <<-EOF &&
+	--glob
+	refs/heads
+	EOF
+
+	cat >expect <<-EOF &&
+	fatal: Option ${SQ}--glob${SQ} requires a value
+	EOF
+
+	test_must_fail git rev-list --stdin <input 2>error &&
+	test_cmp expect error
+'
+
+test_expect_success 'pseudo-opt with invalid value' '
+	cat >input <<-EOF &&
+	--no-walk=garbage
+	EOF
+
+	cat >expect <<-EOF &&
+	error: invalid argument to --no-walk
+	fatal: invalid option ${SQ}--no-walk=garbage${SQ} in --stdin mode
+	EOF
+
+	test_must_fail git rev-list --stdin <input 2>error &&
+	test_cmp expect error
+'
+
+test_expect_success 'unknown option without --end-of-options' '
+	cat >input <<-EOF &&
+	-dashed-branch
+	EOF
+
+	cat >expect <<-EOF &&
+	fatal: invalid option ${SQ}-dashed-branch${SQ} in --stdin mode
+	EOF
+
+	test_must_fail git rev-list --stdin <input 2>error &&
+	test_cmp expect error
+'
+
 test_done
-- 
2.41.0


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]