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

Message

Patrick Steinhardt June 15, 2023, 2:39 p.m. UTC

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 <[e-mail]>
     
    @@ 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 &&