revision: handle pseudo-opts in `--stdin` mode

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.

@@ 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