| Message ID | 457591712799719f23fa47e59d5a3c1cd497e802.1686744685.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | revision: handle pseudo-opts in `--stdin` mode | expand |
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.
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
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
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(-)