{"thread":{"id":"59861","subject":"[PATCH 1/3] revision: reorder `read_revisions_from_stdin()`","startedAt":"2023-06-14T12:18:44Z","lastAt":"2023-06-15T14:42:51Z","messageCount":12,"participants":["Patrick Steinhardt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"478364","messageId":"6cd4f79482276eae333bab8cebd114fa913659e6.1686744685.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686744685.git.ps@pks.im","subject":"[PATCH 1/3] revision: reorder `read_revisions_from_stdin()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-14T12:18:32Z","receivedAt":"2023-06-14T12:18:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Reorder `read_revisions_from_stdin()` so that we can start using\n`handle_revision_pseudo_opt()` without a forward declaration in a\nsubsequent commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n revision.c | 66 +++++++++++++++++++++++++++---------------------------\n 1 file changed, 33 insertions(+), 33 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b33cc1d106..cc22ccd76e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2195,39 +2195,6 @@ static void read_pathspec_from_stdin(struct strbuf *sb,\n \t\tstrvec_push(prune, sb->buf);\n }\n \n-static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t struct strvec *prune)\n-{\n-\tstruct strbuf sb;\n-\tint seen_dashdash = 0;\n-\tint save_warning;\n-\n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\tstrbuf_init(&sb, 1000);\n-\twhile (strbuf_getline(&sb, stdin) != EOF) {\n-\t\tint len = sb.len;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (sb.buf[0] == '-') {\n-\t\t\tif (len == 2 && sb.buf[1] == '-') {\n-\t\t\t\tseen_dashdash = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tdie(\"options not supported in --stdin mode\");\n-\t\t}\n-\t\tif (handle_revision_arg(sb.buf, revs, 0,\n-\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(\"bad revision '%s'\", sb.buf);\n-\t}\n-\tif (seen_dashdash)\n-\t\tread_pathspec_from_stdin(&sb, prune);\n-\n-\tstrbuf_release(&sb);\n-\twarn_on_object_refname_ambiguity = save_warning;\n-}\n-\n static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)\n {\n \tappend_grep_pattern(&revs->grep_filter, ptn, \"command line\", 0, what);\n@@ -2816,6 +2783,39 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \treturn 1;\n }\n \n+static void read_revisions_from_stdin(struct rev_info *revs,\n+\t\t\t\t struct strvec *prune)\n+{\n+\tstruct strbuf sb;\n+\tint seen_dashdash = 0;\n+\tint save_warning;\n+\n+\tsave_warning = warn_on_object_refname_ambiguity;\n+\twarn_on_object_refname_ambiguity = 0;\n+\n+\tstrbuf_init(&sb, 1000);\n+\twhile (strbuf_getline(&sb, stdin) != EOF) {\n+\t\tint len = sb.len;\n+\t\tif (!len)\n+\t\t\tbreak;\n+\t\tif (sb.buf[0] == '-') {\n+\t\t\tif (len == 2 && sb.buf[1] == '-') {\n+\t\t\t\tseen_dashdash = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t}\n+\t\tif (handle_revision_arg(sb.buf, revs, 0,\n+\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(\"bad revision '%s'\", sb.buf);\n+\t}\n+\tif (seen_dashdash)\n+\t\tread_pathspec_from_stdin(&sb, prune);\n+\n+\tstrbuf_release(&sb);\n+\twarn_on_object_refname_ambiguity = save_warning;\n+}\n+\n static void NORETURN diagnose_missing_default(const char *def)\n {\n \tint flags;\n-- \n2.41.0\n\n"},{"id":"478365","messageId":"cover.1686744685.git.ps@pks.im","threadId":"59861","inReplyTo":null,"subject":"[PATCH 0/3] revision: handle pseudo-opts in `--stdin` mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-14T12:18:28Z","receivedAt":"2023-06-14T12:18:46Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nboth git-rev-list(1) and git-log(1) provide a `--stdin` mode that will\ncause these commands to read additional commits from standard input. The\nprimary usecase will typically be scripts that potentially process a\nlong list of commits which might otherwise bust command line length\nlimits.\n\nOne notable omission though is that this mode does not allow us to pass\npseudo-options like `--all`, `--branch`, `--glob=` or `--not`. And while\nit is possible to mix command line arguments and `--stdin` in order to\nmix both pseudo-options and a potentially large set of commits, the end\nresult is arguably more complex for the user.\n\nThis patch series thus implements support for handling pseudo-options in\n`--stdin` mode for both git-rev-list(1) and git-log(1).\n\nPatrick\n\nPatrick Steinhardt (3):\n revision: reorder `read_revisions_from_stdin()`\n revision: small readability improvement for reading from stdin\n revision: handle pseudo-opts in `--stdin` mode\n\n Documentation/rev-list-options.txt | 9 ++--\n revision.c | 76 +++++++++++++++++-------------\n t/t6017-rev-list-stdin.sh | 32 +++++++++++++\n 3 files changed, 79 insertions(+), 38 deletions(-)\n\n-- \n2.41.0\n\n"},{"id":"478366","messageId":"38c0415ee9074276e9832bbbafdee8833cd7ddb9.1686744685.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686744685.git.ps@pks.im","subject":"[PATCH 2/3] revision: small readability improvement for reading from stdin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-14T12:18:37Z","receivedAt":"2023-06-14T12:18:58Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The code that reads lines from standard input manually compares whether\nthe read line matches \"--\", which is a bit awkward to read. Refactor it\nto instead use strcmp(3P) for a small code style improvement.\n\nSigned-off-by: Patrick Steinhardt \n---\n revision.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex cc22ccd76e..dcb7951b4e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2795,16 +2795,18 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \n \tstrbuf_init(&sb, 1000);\n \twhile (strbuf_getline(&sb, stdin) != EOF) {\n-\t\tint len = sb.len;\n-\t\tif (!len)\n+\t\tif (!sb.len)\n \t\t\tbreak;\n+\n \t\tif (sb.buf[0] == '-') {\n-\t\t\tif (len == 2 && sb.buf[1] == '-') {\n+\t\t\tif (!strcmp(sb.buf, \"--\")) {\n \t\t\t\tseen_dashdash = 1;\n \t\t\t\tbreak;\n \t\t\t}\n+\n \t\t\tdie(\"options not supported in --stdin mode\");\n \t\t}\n+\n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n-- \n2.41.0\n\n"},{"id":"478367","messageId":"457591712799719f23fa47e59d5a3c1cd497e802.1686744685.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686744685.git.ps@pks.im","subject":"[PATCH 3/3] revision: handle pseudo-opts in `--stdin` mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-14T12:18:42Z","receivedAt":"2023-06-14T12:19:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While both git-rev-list(1) and git-log(1) support `--stdin`, it only\naccepts commits and files. Most notably, it is impossible to pass any of\nthe pseudo-opts like `--all`, `--glob=` or others via stdin.\n\nThis makes it hard to use this function in certain scripted scenarios,\nlike when one wants to support queries against specific revisions, but\nalso against reference patterns. While this is theoretically possible by\nusing arguments, this may run into issues once we hit platform limits\nwith sufficiently large queries. And because `--stdin` cannot handle\npseudo-opts, the only alternative would be to use a mixture of arguments\nand standard input, which is cumbersome.\n\nImplement support for handling pseudo-opts in both commands to support\nthis usecase better. One notable restriction here is that `--stdin` only\nsupports \"stuck\" arguments in the form of `--glob=foo`. This is because\n\"unstuck\" arguments would also require us to read the next line, which\nwould add quite some complexity to the code. This restriction should be\nfine for scripted usage though.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/rev-list-options.txt | 9 +++++----\n revision.c | 12 ++++++++---\n t/t6017-rev-list-stdin.sh | 32 ++++++++++++++++++++++++++++++\n 3 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 3000888a90..e6468bf0eb 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -236,10 +236,11 @@ ifndef::git-rev-list[]\n endif::git-rev-list[]\n \n --stdin::\n-\tIn addition to the '' listed on the command\n-\tline, read them from the standard input. If a `--` separator is\n-\tseen, stop reading commits and start reading paths to limit the\n-\tresult.\n+\tIn addition to getting arguments from the command line, read\n+\tthem for standard input as well. This accepts commits and\n+\tpseudo-options like `--all` and `--glob=`. When a `--` separator\n+\tis seen, the following input is treated as paths and used to\n+\tlimit the result.\n \n ifdef::git-rev-list[]\n --quiet::\ndiff --git a/revision.c b/revision.c\nindex dcb7951b4e..b90794cca4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2784,7 +2784,8 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n }\n \n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t struct strvec *prune)\n+\t\t\t\t struct strvec *prune,\n+\t\t\t\t int *flags)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n@@ -2799,12 +2800,17 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\tbreak;\n \n \t\tif (sb.buf[0] == '-') {\n+\t\t\tconst char *argv[2] = { sb.buf, NULL };\n+\n \t\t\tif (!strcmp(sb.buf, \"--\")) {\n \t\t\t\tseen_dashdash = 1;\n \t\t\t\tbreak;\n \t\t\t}\n \n-\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags))\n+\t\t\t\tcontinue;\n+\n+\t\t\tdie(_(\"option '%s' not supported in --stdin mode\"), sb.buf);\n \t\t}\n \n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n@@ -2890,7 +2896,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\tread_revisions_from_stdin(revs, &prune_data, &flags);\n \t\t\t\tcontinue;\n \t\t\t}\n \ndiff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh\nindex 05162512a0..11cb27c3ed 100755\n--- a/t/t6017-rev-list-stdin.sh\n+++ b/t/t6017-rev-list-stdin.sh\n@@ -60,6 +60,11 @@ check side-1 ^side-7 -- file-2\n check side-3 ^side-4 -- file-3\n check side-3 ^side-2\n check side-3 ^side-2 -- file-1\n+check --all\n+check --all --not --branches\n+check --glob=refs/heads\n+check --glob=refs/heads --\n+check --glob=refs/heads -- file-1\n \n test_expect_success 'not only --stdin' '\n \tcat >expect <<-EOF &&\n@@ -78,4 +83,31 @@ test_expect_success 'not only --stdin' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pseudo-opt with missing value' '\n+\tcat >input <<-EOF &&\n+\t--glob\n+\trefs/heads\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tfatal: Option ${SQ}--glob${SQ} requires a value\n+\tEOF\n+\n+\ttest_must_fail git rev-list --stdin error &&\n+\ttest_cmp expect error\n+'\n+\n+test_expect_success 'unknown options' '\n+\tcat >input <<-EOF &&\n+\t--unknown=value\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tfatal: option ${SQ}--unknown=value${SQ} not supported in --stdin mode\n+\tEOF\n+\n+\ttest_must_fail git rev-list --stdin error &&\n+\ttest_cmp expect error\n+'\n+\n test_done\n-- \n2.41.0\n\n"},{"id":"478374","messageId":"xmqq352uyr5b.fsf@gitster.g","threadId":"59861","inReplyTo":"457591712799719f23fa47e59d5a3c1cd497e802.1686744685.git.ps@pks.im","subject":"Re: [PATCH 3/3] revision: handle pseudo-opts in `--stdin` mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T15:56:00Z","receivedAt":"2023-06-14T15:56:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> \t\tif (sb.buf[0] == '-') {\n> +\t\t\tconst char *argv[2] = { sb.buf, NULL };\n> +\n> \t\t\tif (!strcmp(sb.buf, \"--\")) {\n> \t\t\t\tseen_dashdash = 1;\n> \t\t\t\tbreak;\n> \t\t\t}\n> \n> -\t\t\tdie(\"options not supported in --stdin mode\");\n> +\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags))\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tdie(_(\"option '%s' not supported in --stdin mode\"), sb.buf);\n> \t\t}\n\nThe reason why we had the double-dash checking inside this block is\nbecause we rejected any dashed options other than double-dash, but\nwith this step, that is no longer true, so it may make more sense to\nmove the \"seen_dashdash\" code outside the block. Also unlike the\ncommand line options that allows \"--end-of-options\" marker to allow\ntweaking how a failing handle_revision_arg() call is handled, this\ncodepath does not seem to have any matching provision. In the\noriginal code, which did not accept any options, it was perfectly\nunderstandable that it was unaware of \"--end-of-options\", but now we\ndo allow dashed options, shouldn't we be supporting it here, too?\n\nThanks.\n"},{"id":"478376","messageId":"xmqqr0qexcc5.fsf@gitster.g","threadId":"59861","inReplyTo":"6cd4f79482276eae333bab8cebd114fa913659e6.1686744685.git.ps@pks.im","subject":"Re: [PATCH 1/3] revision: reorder `read_revisions_from_stdin()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T16:01:14Z","receivedAt":"2023-06-14T16:01:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Reorder `read_revisions_from_stdin()` so that we can start using\n> `handle_revision_pseudo_opt()` without a forward declaration in a\n> subsequent commit.\n\nMakes sense.\n"},{"id":"478377","messageId":"xmqqh6raxc8b.fsf@gitster.g","threadId":"59861","inReplyTo":"38c0415ee9074276e9832bbbafdee8833cd7ddb9.1686744685.git.ps@pks.im","subject":"Re: [PATCH 2/3] revision: small readability improvement for reading from stdin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-14T16:03:32Z","receivedAt":"2023-06-14T16:03:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> The code that reads lines from standard input manually compares whether\n> the read line matches \"--\", which is a bit awkward to read. Refactor it\n> to instead use strcmp(3P) for a small code style improvement.\n\nIt is unclear if it is an \"improvement\". We've already checked the\nfirst byte to be \"-\" and we only need to check the second one (and\nthe length of the string) to see if we have a double-dash, instead\nof checking the first byte again.\n\nI am not sure if these excessive blank lines are helping, either.\n\nThe only reason I would be inclined to support the main change in\nthis patch (but not additional blank lines) is because I will be\nsuggesting to lift the logic to detect double-dash from the \"line\nbegins with dash\" block in my review of the next step. Once that is\ndone, double-dash detection cannot rely on the fact that we have\nalready checked the first byte.\n\nI do agree with the change to remove the temporary variable \"len\",\nif we were to remove the \"len == 2\" comparison, as it makes \"len\"\nless useful.\n\nThanks.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> revision.c | 8 +++++---\n> 1 file changed, 5 insertions(+), 3 deletions(-)\n>\n> diff --git a/revision.c b/revision.c\n> index cc22ccd76e..dcb7951b4e 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2795,16 +2795,18 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n> \n> \tstrbuf_init(&sb, 1000);\n> \twhile (strbuf_getline(&sb, stdin) != EOF) {\n> -\t\tint len = sb.len;\n> -\t\tif (!len)\n> +\t\tif (!sb.len)\n> \t\t\tbreak;\n> +\n> \t\tif (sb.buf[0] == '-') {\n> -\t\t\tif (len == 2 && sb.buf[1] == '-') {\n> +\t\t\tif (!strcmp(sb.buf, \"--\")) {\n> \t\t\t\tseen_dashdash = 1;\n> \t\t\t\tbreak;\n> \t\t\t}\n> +\n> \t\t\tdie(\"options not supported in --stdin mode\");\n> \t\t}\n> +\n> \t\tif (handle_revision_arg(sb.buf, revs, 0,\n> \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n> \t\t\tdie(\"bad revision '%s'\", sb.buf);\n"},{"id":"478435","messageId":"2ot3z23uzikdrgsl7wdqwzpxe4xihkpekqn2rg5y3vfq5yklv6@njx2gf3wwcab","threadId":"59861","inReplyTo":"xmqq352uyr5b.fsf@gitster.g","subject":"Re: [PATCH 3/3] revision: handle pseudo-opts in `--stdin` mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-15T14:25:18Z","receivedAt":"2023-06-15T14:25:28Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Wed, Jun 14, 2023 at 08:56:00AM -0700, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > \t\tif (sb.buf[0] == '-') {\n> > +\t\t\tconst char *argv[2] = { sb.buf, NULL };\n> > +\n> > \t\t\tif (!strcmp(sb.buf, \"--\")) {\n> > \t\t\t\tseen_dashdash = 1;\n> > \t\t\t\tbreak;\n> > \t\t\t}\n> > \n> > -\t\t\tdie(\"options not supported in --stdin mode\");\n> > +\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags))\n> > +\t\t\t\tcontinue;\n> > +\n> > +\t\t\tdie(_(\"option '%s' not supported in --stdin mode\"), sb.buf);\n> > \t\t}\n> \n> The reason why we had the double-dash checking inside this block is\n> because we rejected any dashed options other than double-dash, but\n> with this step, that is no longer true, so it may make more sense to\n> move the \"seen_dashdash\" code outside the block. Also unlike the\n> command line options that allows \"--end-of-options\" marker to allow\n> tweaking how a failing handle_revision_arg() call is handled, this\n> codepath does not seem to have any matching provision. In the\n> original code, which did not accept any options, it was perfectly\n> understandable that it was unaware of \"--end-of-options\", but now we\n> do allow dashed options, shouldn't we be supporting it here, too?\n> \n> Thanks.\n\nGood point, we should. Most importantly, the code would currently refuse\nto enumerate references with a leading dash, and there would be no way\nto work around this issue.\n\nE.g. this works:\n\n$ git rev-list --end-of-options -branch\n\nWhereas neither of these do in the current version:\n\n$ printf \"%s\\n\" -branch | git rev-list\n$ printf \"%s\\n\" --end-of-options -branch | git rev-list\n\nWill send a v2 with `--end-of-options` handling, thanks!\n\nPatrick\n"},{"id":"478436","messageId":"6cd4f79482276eae333bab8cebd114fa913659e6.1686839572.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686839572.git.ps@pks.im","subject":"[PATCH v2 1/3] revision: reorder `read_revisions_from_stdin()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-15T14:39:51Z","receivedAt":"2023-06-15T14:40:12Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Reorder `read_revisions_from_stdin()` so that we can start using\n`handle_revision_pseudo_opt()` without a forward declaration in a\nsubsequent commit.\n\nSigned-off-by: Patrick Steinhardt \n---\n revision.c | 66 +++++++++++++++++++++++++++---------------------------\n 1 file changed, 33 insertions(+), 33 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex b33cc1d106..cc22ccd76e 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2195,39 +2195,6 @@ static void read_pathspec_from_stdin(struct strbuf *sb,\n \t\tstrvec_push(prune, sb->buf);\n }\n \n-static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t struct strvec *prune)\n-{\n-\tstruct strbuf sb;\n-\tint seen_dashdash = 0;\n-\tint save_warning;\n-\n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\tstrbuf_init(&sb, 1000);\n-\twhile (strbuf_getline(&sb, stdin) != EOF) {\n-\t\tint len = sb.len;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (sb.buf[0] == '-') {\n-\t\t\tif (len == 2 && sb.buf[1] == '-') {\n-\t\t\t\tseen_dashdash = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tdie(\"options not supported in --stdin mode\");\n-\t\t}\n-\t\tif (handle_revision_arg(sb.buf, revs, 0,\n-\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(\"bad revision '%s'\", sb.buf);\n-\t}\n-\tif (seen_dashdash)\n-\t\tread_pathspec_from_stdin(&sb, prune);\n-\n-\tstrbuf_release(&sb);\n-\twarn_on_object_refname_ambiguity = save_warning;\n-}\n-\n static void add_grep(struct rev_info *revs, const char *ptn, enum grep_pat_token what)\n {\n \tappend_grep_pattern(&revs->grep_filter, ptn, \"command line\", 0, what);\n@@ -2816,6 +2783,39 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n \treturn 1;\n }\n \n+static void read_revisions_from_stdin(struct rev_info *revs,\n+\t\t\t\t struct strvec *prune)\n+{\n+\tstruct strbuf sb;\n+\tint seen_dashdash = 0;\n+\tint save_warning;\n+\n+\tsave_warning = warn_on_object_refname_ambiguity;\n+\twarn_on_object_refname_ambiguity = 0;\n+\n+\tstrbuf_init(&sb, 1000);\n+\twhile (strbuf_getline(&sb, stdin) != EOF) {\n+\t\tint len = sb.len;\n+\t\tif (!len)\n+\t\t\tbreak;\n+\t\tif (sb.buf[0] == '-') {\n+\t\t\tif (len == 2 && sb.buf[1] == '-') {\n+\t\t\t\tseen_dashdash = 1;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\t}\n+\t\tif (handle_revision_arg(sb.buf, revs, 0,\n+\t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(\"bad revision '%s'\", sb.buf);\n+\t}\n+\tif (seen_dashdash)\n+\t\tread_pathspec_from_stdin(&sb, prune);\n+\n+\tstrbuf_release(&sb);\n+\twarn_on_object_refname_ambiguity = save_warning;\n+}\n+\n static void NORETURN diagnose_missing_default(const char *def)\n {\n \tint flags;\n-- \n2.41.0\n\n"},{"id":"478437","messageId":"0cf189b3787ec7a8999da202eae902ec7e912d39.1686839572.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686839572.git.ps@pks.im","subject":"[PATCH v2 3/3] revision: handle pseudo-opts in `--stdin` mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-15T14:39:59Z","receivedAt":"2023-06-15T14:40:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"While both git-rev-list(1) and git-log(1) support `--stdin`, it only\naccepts commits and files. Most notably, it is impossible to pass any of\nthe pseudo-opts like `--all`, `--glob=` or others via stdin.\n\nThis makes it hard to use this function in certain scripted scenarios,\nlike when one wants to support queries against specific revisions, but\nalso against reference patterns. While this is theoretically possible by\nusing arguments, this may run into issues once we hit platform limits\nwith sufficiently large queries. And because `--stdin` cannot handle\npseudo-opts, the only alternative would be to use a mixture of arguments\nand standard input, which is cumbersome.\n\nImplement support for handling pseudo-opts in both commands to support\nthis usecase better. One notable restriction here is that `--stdin` only\nsupports \"stuck\" arguments in the form of `--glob=foo`. This is because\n\"unstuck\" arguments would also require us to read the next line, which\nwould add quite some complexity to the code. This restriction should be\nfine for scripted usage though.\n\nSigned-off-by: Patrick Steinhardt \n---\n Documentation/rev-list-options.txt | 9 +++---\n revision.c | 21 +++++++++---\n t/t6017-rev-list-stdin.sh | 51 +++++++++++++++++++++++++++++-\n 3 files changed, 72 insertions(+), 9 deletions(-)\n\ndiff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt\nindex 3000888a90..e6468bf0eb 100644\n--- a/Documentation/rev-list-options.txt\n+++ b/Documentation/rev-list-options.txt\n@@ -236,10 +236,11 @@ ifndef::git-rev-list[]\n endif::git-rev-list[]\n \n --stdin::\n-\tIn addition to the '' listed on the command\n-\tline, read them from the standard input. If a `--` separator is\n-\tseen, stop reading commits and start reading paths to limit the\n-\tresult.\n+\tIn addition to getting arguments from the command line, read\n+\tthem for standard input as well. This accepts commits and\n+\tpseudo-options like `--all` and `--glob=`. When a `--` separator\n+\tis seen, the following input is treated as paths and used to\n+\tlimit the result.\n \n ifdef::git-rev-list[]\n --quiet::\ndiff --git a/revision.c b/revision.c\nindex 3a39f41bb8..a0b147913f 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2784,10 +2784,12 @@ static int handle_revision_pseudo_opt(struct rev_info *revs,\n }\n \n static void read_revisions_from_stdin(struct rev_info *revs,\n-\t\t\t\t struct strvec *prune)\n+\t\t\t\t struct strvec *prune,\n+\t\t\t\t int *flags)\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n+\tint seen_end_of_options = 0;\n \tint save_warning;\n \n \tsave_warning = warn_on_object_refname_ambiguity;\n@@ -2803,8 +2805,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\tbreak;\n \t\t}\n \n-\t\tif (sb.buf[0] == '-')\n-\t\t\tdie(\"options not supported in --stdin mode\");\n+\t\tif (!seen_end_of_options && sb.buf[0] == '-') {\n+\t\t\tconst char *argv[] = { sb.buf, NULL };\n+\n+\t\t\tif (!strcmp(sb.buf, \"--end-of-options\")) {\n+\t\t\t\tseen_end_of_options = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags) > 0)\n+\t\t\t\tcontinue;\n+\n+\t\t\tdie(_(\"invalid option '%s' in --stdin mode\"), sb.buf);\n+\t\t}\n \n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n@@ -2889,7 +2902,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n-\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\tread_revisions_from_stdin(revs, &prune_data, &flags);\n \t\t\t\tcontinue;\n \t\t\t}\n \ndiff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh\nindex 05162512a0..a57f1ae2ba 100755\n--- a/t/t6017-rev-list-stdin.sh\n+++ b/t/t6017-rev-list-stdin.sh\n@@ -48,7 +48,9 @@ test_expect_success setup '\n \t\t\tgit add file-$i &&\n \t\t\ttest_tick &&\n \t\t\tgit commit -m side-$i || exit\n-\t\tdone\n+\t\tdone &&\n+\n+\t\tgit update-ref refs/heads/-dashed-branch HEAD\n \t)\n '\n \n@@ -60,6 +62,12 @@ check side-1 ^side-7 -- file-2\n check side-3 ^side-4 -- file-3\n check side-3 ^side-2\n check side-3 ^side-2 -- file-1\n+check --all\n+check --all --not --branches\n+check --glob=refs/heads\n+check --glob=refs/heads --\n+check --glob=refs/heads -- file-1\n+check --end-of-options -dashed-branch\n \n test_expect_success 'not only --stdin' '\n \tcat >expect <<-EOF &&\n@@ -78,4 +86,45 @@ test_expect_success 'not only --stdin' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'pseudo-opt with missing value' '\n+\tcat >input <<-EOF &&\n+\t--glob\n+\trefs/heads\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tfatal: Option ${SQ}--glob${SQ} requires a value\n+\tEOF\n+\n+\ttest_must_fail git rev-list --stdin error &&\n+\ttest_cmp expect error\n+'\n+\n+test_expect_success 'pseudo-opt with invalid value' '\n+\tcat >input <<-EOF &&\n+\t--no-walk=garbage\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\terror: invalid argument to --no-walk\n+\tfatal: invalid option ${SQ}--no-walk=garbage${SQ} in --stdin mode\n+\tEOF\n+\n+\ttest_must_fail git rev-list --stdin error &&\n+\ttest_cmp expect error\n+'\n+\n+test_expect_success 'unknown option without --end-of-options' '\n+\tcat >input <<-EOF &&\n+\t-dashed-branch\n+\tEOF\n+\n+\tcat >expect <<-EOF &&\n+\tfatal: invalid option ${SQ}-dashed-branch${SQ} in --stdin mode\n+\tEOF\n+\n+\ttest_must_fail git rev-list --stdin error &&\n+\ttest_cmp expect error\n+'\n+\n test_done\n-- \n2.41.0\n\n"},{"id":"478438","messageId":"5c1a9a0d08b25ce94ba1498cd88051abd062369c.1686839572.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686839572.git.ps@pks.im","subject":"[PATCH v2 2/3] revision: small readability improvement for reading from stdin","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-15T14:39:55Z","receivedAt":"2023-06-15T14:40:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The code that reads lines from standard input manually compares whether\nthe read line matches \"--\", which is a bit awkward to read. Furthermore,\nwe're about to extend the code to also support reading pseudo-options\nvia standard input, requiring more elaborate handling of lines with a\nleading dash.\n\nRefactor the code by hoisting out the check for \"--\" outside of the\nblock that checks for a leading dash.\n\nSigned-off-by: Patrick Steinhardt \n---\n revision.c | 17 +++++++++--------\n 1 file changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex cc22ccd76e..3a39f41bb8 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2795,16 +2795,17 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \n \tstrbuf_init(&sb, 1000);\n \twhile (strbuf_getline(&sb, stdin) != EOF) {\n-\t\tint len = sb.len;\n-\t\tif (!len)\n+\t\tif (!sb.len)\n+\t\t\tbreak;\n+\n+\t\tif (!strcmp(sb.buf, \"--\")) {\n+\t\t\tseen_dashdash = 1;\n \t\t\tbreak;\n-\t\tif (sb.buf[0] == '-') {\n-\t\t\tif (len == 2 && sb.buf[1] == '-') {\n-\t\t\t\tseen_dashdash = 1;\n-\t\t\t\tbreak;\n-\t\t\t}\n-\t\t\tdie(\"options not supported in --stdin mode\");\n \t\t}\n+\n+\t\tif (sb.buf[0] == '-')\n+\t\t\tdie(\"options not supported in --stdin mode\");\n+\n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n-- \n2.41.0\n\n"},{"id":"478439","messageId":"cover.1686839572.git.ps@pks.im","threadId":"59861","inReplyTo":"cover.1686744685.git.ps@pks.im","subject":"[PATCH v2 0/3] revision: handle pseudo-opts in `--stdin` mode","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-15T14:39:46Z","receivedAt":"2023-06-15T14:42:51Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series to support handling of\npseudo-opts like `--all` or `--glob=` in both git-rev-list(1)'s and\ngit-log(1)'s `--stdin` mode.\n\nChanges compared to v1:\n\n - Patch 2/3: Changed the small refactoring to hoist out the check\n for \"--\" out of the `if (sb.buf[0] == '-')` block completely to\n make for an easier read.\n\n - Patch 3/3: Implemented support for `--end-of-options` so that it\n becomes possible to ask for references that have a leading dash.\n\n - Patch 3/3: Fixed an issue where invalid pseudo-opts (e.g.\n `--no-walk=garbage`) would not be recognized.\n\nThis is based on Junio's feedback, thank you!\n\nPatrick\n\nPatrick Steinhardt (3):\n revision: reorder `read_revisions_from_stdin()`\n revision: small readability improvement for reading from stdin\n revision: handle pseudo-opts in `--stdin` mode\n\n Documentation/rev-list-options.txt | 9 ++--\n revision.c | 82 +++++++++++++++++-------------\n t/t6017-rev-list-stdin.sh | 51 ++++++++++++++++++-\n 3 files changed, 103 insertions(+), 39 deletions(-)\n\nRange-diff against v1:\n1: 6cd4f79482 = 1: 6cd4f79482 revision: reorder `read_revisions_from_stdin()`\n2: 38c0415ee9 ! 2: 5c1a9a0d08 revision: small readability improvement for reading from stdin\n @@ Commit message\n revision: small readability improvement for reading from stdin\n \n The code that reads lines from standard input manually compares whether\n - the read line matches \"--\", which is a bit awkward to read. Refactor it\n - to instead use strcmp(3P) for a small code style improvement.\n + the read line matches \"--\", which is a bit awkward to read. Furthermore,\n + we're about to extend the code to also support reading pseudo-options\n + via standard input, requiring more elaborate handling of lines with a\n + leading dash.\n +\n + Refactor the code by hoisting out the check for \"--\" outside of the\n + block that checks for a leading dash.\n \n Signed-off-by: Patrick Steinhardt \n \n @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,\n -\t\tint len = sb.len;\n -\t\tif (!len)\n +\t\tif (!sb.len)\n - \t\t\tbreak;\n ++\t\t\tbreak;\n +\n - \t\tif (sb.buf[0] == '-') {\n ++\t\tif (!strcmp(sb.buf, \"--\")) {\n ++\t\t\tseen_dashdash = 1;\n + \t\t\tbreak;\n +-\t\tif (sb.buf[0] == '-') {\n -\t\t\tif (len == 2 && sb.buf[1] == '-') {\n -+\t\t\tif (!strcmp(sb.buf, \"--\")) {\n - \t\t\t\tseen_dashdash = 1;\n - \t\t\t\tbreak;\n - \t\t\t}\n -+\n - \t\t\tdie(\"options not supported in --stdin mode\");\n +-\t\t\t\tseen_dashdash = 1;\n +-\t\t\t\tbreak;\n +-\t\t\t}\n +-\t\t\tdie(\"options not supported in --stdin mode\");\n \t\t}\n ++\n ++\t\tif (sb.buf[0] == '-')\n ++\t\t\tdie(\"options not supported in --stdin mode\");\n +\n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n3: 4575917127 ! 3: 0cf189b378 revision: handle pseudo-opts in `--stdin` mode\n @@ revision.c: static int handle_revision_pseudo_opt(struct rev_info *revs,\n {\n \tstruct strbuf sb;\n \tint seen_dashdash = 0;\n ++\tint seen_end_of_options = 0;\n + \tint save_warning;\n + \n + \tsave_warning = warn_on_object_refname_ambiguity;\n @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\tbreak;\n + \t\t}\n \n - \t\tif (sb.buf[0] == '-') {\n -+\t\t\tconst char *argv[2] = { sb.buf, NULL };\n -+\n - \t\t\tif (!strcmp(sb.buf, \"--\")) {\n - \t\t\t\tseen_dashdash = 1;\n - \t\t\t\tbreak;\n - \t\t\t}\n - \n +-\t\tif (sb.buf[0] == '-')\n -\t\t\tdie(\"options not supported in --stdin mode\");\n -+\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags))\n ++\t\tif (!seen_end_of_options && sb.buf[0] == '-') {\n ++\t\t\tconst char *argv[] = { sb.buf, NULL };\n ++\n ++\t\t\tif (!strcmp(sb.buf, \"--end-of-options\")) {\n ++\t\t\t\tseen_end_of_options = 1;\n +\t\t\t\tcontinue;\n ++\t\t\t}\n +\n -+\t\t\tdie(_(\"option '%s' not supported in --stdin mode\"), sb.buf);\n - \t\t}\n ++\t\t\tif (handle_revision_pseudo_opt(revs, argv, flags) > 0)\n ++\t\t\t\tcontinue;\n ++\n ++\t\t\tdie(_(\"invalid option '%s' in --stdin mode\"), sb.buf);\n ++\t\t}\n \n \t\tif (handle_revision_arg(sb.buf, revs, 0,\n + \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t\t}\n \t\t\t\tif (revs->read_from_stdin++)\n @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re\n \n \n ## t/t6017-rev-list-stdin.sh ##\n +@@ t/t6017-rev-list-stdin.sh: test_expect_success setup '\n + \t\t\tgit add file-$i &&\n + \t\t\ttest_tick &&\n + \t\t\tgit commit -m side-$i || exit\n +-\t\tdone\n ++\t\tdone &&\n ++\n ++\t\tgit update-ref refs/heads/-dashed-branch HEAD\n + \t)\n + '\n + \n @@ t/t6017-rev-list-stdin.sh: check side-1 ^side-7 -- file-2\n check side-3 ^side-4 -- file-3\n check side-3 ^side-2\n @@ t/t6017-rev-list-stdin.sh: check side-1 ^side-7 -- file-2\n +check --glob=refs/heads\n +check --glob=refs/heads --\n +check --glob=refs/heads -- file-1\n ++check --end-of-options -dashed-branch\n \n test_expect_success 'not only --stdin' '\n \tcat >expect <<-EOF &&\n @@ t/t6017-rev-list-stdin.sh: test_expect_success 'not only --stdin' '\n +\ttest_cmp expect error\n +'\n +\n -+test_expect_success 'unknown options' '\n ++test_expect_success 'pseudo-opt with invalid value' '\n ++\tcat >input <<-EOF &&\n ++\t--no-walk=garbage\n ++\tEOF\n ++\n ++\tcat >expect <<-EOF &&\n ++\terror: invalid argument to --no-walk\n ++\tfatal: invalid option ${SQ}--no-walk=garbage${SQ} in --stdin mode\n ++\tEOF\n ++\n ++\ttest_must_fail git rev-list --stdin error &&\n ++\ttest_cmp expect error\n ++'\n ++\n ++test_expect_success 'unknown option without --end-of-options' '\n +\tcat >input <<-EOF &&\n -+\t--unknown=value\n ++\t-dashed-branch\n +\tEOF\n +\n +\tcat >expect <<-EOF &&\n -+\tfatal: option ${SQ}--unknown=value${SQ} not supported in --stdin mode\n ++\tfatal: invalid option ${SQ}-dashed-branch${SQ} in --stdin mode\n +\tEOF\n +\n +\ttest_must_fail git rev-list --stdin error &&\n-- \n2.41.0\n\n"}]}