{"thread":{"id":"55881","subject":"[PATCH 0/4] pack-objects: use revision.c's --stdin parsing","startedAt":"2021-06-08T12:16:48Z","lastAt":"2021-07-26T12:47:08Z","messageCount":35,"participants":["Ævar Arnfjörð Bjarmason","Junio C Hamano","Emily Shaffer","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"426759","messageId":"cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com","threadId":"55881","inReplyTo":null,"subject":"[PATCH 0/4] pack-objects: use revision.c's --stdin parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T12:16:26Z","receivedAt":"2021-06-08T12:16:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The revision.c handles the --stdin flag given to \"git rev-list\", and\nvarious other commands use it to take rev-list-like arguments.\n\nThe pack-objects.c builtin made use of the revevisio.c API, and did\nmostly the same handling of stdin, with a couple of minor differences.\n\nThis series extends the revision.c API to be flexible enough to handle\nthe pack-objects.c case, and moves pack-objects.c over to that new\nAPI. The missing piece was to allow the definition of a\n\"handle_stdin_line\" callback. This new callback allows for either\npartially or entirely consuming the stdin before handing it off (or\nnot) to the revision.c logic.\n\nI'm planning on submitting another series on top of this that'll make\nfurther use of this new API, but wanted to send this part as a\nstand-alone series first.\n\nÆvar Arnfjörð Bjarmason (4):\n upload-pack: run is_repository_shallow() before setup_revisions()\n revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n pack-objects.c: do stdin parsing via revision.c's API\n pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n\n builtin/am.c | 4 +--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/pack-objects.c | 64 ++++++++++++++++++++----------------------\n builtin/rev-list.c | 2 +-\n revision.c | 35 ++++++++++++++++++++---\n revision.h | 60 +++++++++++++++++++++++++++++++++++----\n sequencer.c | 4 +--\n 8 files changed, 123 insertions(+), 50 deletions(-)\n\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426760","messageId":"patch-2.4-002e0f1cf7-20210608T121008Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com","subject":"[PATCH 2/4] revision.h: unify \"disable_stdin\" and \"read_from_stdin\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T12:16:28Z","receivedAt":"2021-06-08T12:16:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03) we\nadded the \"disable_stdin\" flag, and then much later in\na12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we\ngained a \"read_from_stdin\" flag.\n\nThe interaction between these is more subtle than they might appear at\nfirst sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\nof \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\noption.\n\nThe \"read\" is intended to understood as \"I read it\", not \"you should\nread it\". Let's avoid this confusion by using \"consume\" and \"consumed\"\ninstead, i.e. a word whose present and past tense isn't the same.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 4 ++--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/rev-list.c | 2 +-\n revision.c | 4 ++--\n revision.h | 23 ++++++++++++++++++++---\n sequencer.c | 4 ++--\n 7 files changed, 29 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b2d886c81..3a6c8455b4 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n \trev_info.abbrev = 0;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.show_root_diff = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.no_commit_id = 1;\n@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)\n \tfp = xfopen(am_path(state, \"patch\"), \"w\");\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.no_commit_id = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.diffopt.use_color = 0;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9a..c9f66c58c4 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\targv[argc - 1] = \"--\";\n \t}\n \n-\trevs.disable_stdin = 1;\n+\trevs.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tsetup_revisions(argc, argv, &revs, NULL);\n \tif (!revs.pending.nr && is_bare_repository()) {\n \t\tstruct commit *head_commit;\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex f33d30d57b..fc548ebe16 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"index file corrupt\"));\n \topt->abbrev = 0;\n \topt->diff = 1;\n-\topt->disable_stdin = 1;\n+\topt->stdin_handling = REV_INFO_STDIN_IGNORE;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.tweak = diff_tree_tweak_rev;\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 7677b1af5a..88bd9ef954 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr) &&\n-\t !revs.rev_input_given && !revs.read_from_stdin) ||\n+\t !revs.rev_input_given && !revs.consumed_stdin_per_option) ||\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \ndiff --git a/revision.c b/revision.c\nindex 8140561b6c..69b3812093 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n-\t\t\t\tif (revs->disable_stdin) {\n+\t\t\t\tif (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n-\t\t\t\tif (revs->read_from_stdin++)\n+\t\t\t\tif (revs->consumed_stdin_per_option++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n \t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 93aa012f51..03231f089f 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -86,6 +86,11 @@ struct rev_cmdline_info {\n struct oidset;\n struct topo_walk_info;\n \n+enum rev_info_stdin {\n+\tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n+\tREV_INFO_STDIN_IGNORE,\n+};\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -114,9 +119,22 @@ struct rev_info {\n \tint rev_input_given;\n \n \t/*\n-\t * Whether we read from stdin due to the --stdin option.\n+\t * How should we handle seeing --stdin?\n+\t *\n+\t * Defaults to reading if we see it with\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION.\n+\t *\n+\t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n+\t * --stdin option.\n+\t */\n+\tenum rev_info_stdin stdin_handling;\n+\n+\t/*\n+\t * Did we read from stdin due to stdin_handling ==\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n+\t * option?\n \t */\n-\tint read_from_stdin;\n+\tint consumed_stdin_per_option;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n@@ -216,7 +234,6 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1;\n-\tunsigned int\tdisable_stdin:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n \t\t\ttrack_first_time:1,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38..4e73bd79d6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,\n \tlog_tree_opt.abbrev = 0;\n \tlog_tree_opt.diff = 1;\n \tlog_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\tlog_tree_opt.disable_stdin = 1;\n+\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tlog_tree_opt.no_commit_id = 1;\n \tlog_tree_opt.diffopt.file = fopen(buf.buf, \"w\");\n \tlog_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;\n@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,\n \t\t\tlog_tree_opt.diff = 1;\n \t\t\tlog_tree_opt.diffopt.output_format =\n \t\t\t\tDIFF_FORMAT_DIFFSTAT;\n-\t\t\tlog_tree_opt.disable_stdin = 1;\n+\t\t\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \n \t\t\tif (read_oneliner(&buf, rebase_path_orig_head(), 0) &&\n \t\t\t !get_oid(buf.buf, &orig) &&\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426761","messageId":"patch-3.4-1a9eb26587-20210608T121008Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com","subject":"[PATCH 3/4] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T12:16:29Z","receivedAt":"2021-06-08T12:16:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the fgets(..., stdin) parsing in pack-objects.c to use a\nnow-extended version of the rev_info stdin parsing API.\n\nThe fgets() loop being refactored away here was first added in Linus's\nc323ac7d9c5 (git-pack-objects: create a packed object representation.,\n2005-06-25).\n\nLater on rev-list started doing similar parsing in 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05),\nand that code was promoted to a more general API in 1fc561d169a (Move\nread_revisions_from_stdin from builtin-rev-list.c to revision.c,\n2008-07-05).\n\nSince then the API in revision.c has received improvements that have\nbeen missed here. E.g. the arbitrary limit of 1000 bytes was removed\nin 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),\nand it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:\nread --stdin with strbuf_getline(), 2015-10-28).\n\nLet's do the same here, as in 6e8d46f9d4b we can remove the \"if (len\n&& line[len - 1] == '\\n')\" check, it's now redundant to using\nstrbuf_getline(), and we get to skip the whole\n\"warn_on_object_refname_ambiguity\" dance. The\nread_revisions_from_stdin() function in revision.c we're now using\ndoes it for us.\n\nFor now there isn't all that much point in this whole exercises. We\njust end up calling setup_revisions() to loop over stdin for us, but\nthe callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we\nstill need to call handle_revision_arg() ourselves because we'd like\nto call it with different flags.\n\nThis very light use of the API will be further refined in a subsequent\ncommit, for now we're just doing the bare minimum to move this\nexisting code over to the new callback pattern without any functional\nchanges, and making it as friendly to \"git show -w\" and \"the\n--color-moved-ws=allow-indentation-change\" mode as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 66 +++++++++++++++++++++---------------------\n revision.c | 27 +++++++++++++++++\n revision.h | 31 ++++++++++++++++++++\n 3 files changed, 91 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1fbaa34f91..cbb01f2b2d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3745,15 +3745,43 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n+static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n+\tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n+{\n+\tint *flags = stdin_line_priv;\n+\tchar *line = line_sb->buf;\n+\tsize_t len = line_sb->len;\n+\n+\tif (!len)\n+\t\treturn REV_INFO_STDIN_LINE_BREAK;\n+\tif (*line == '-') {\n+\t\tif (!strcmp(line, \"--not\")) {\n+\t\t\t*flags ^= UNINTERESTING;\n+\t\t\twrite_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\tstruct object_id oid;\n+\t\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\t\tregister_shallow(the_repository, &oid);\n+\t\t\tuse_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tdie(_(\"not a rev '%s'\"), line);\n+\t}\n+\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(_(\"bad revision '%s'\"), line);\n+\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tchar line[1000];\n \tint flags = 0;\n-\tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3761,39 +3789,11 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n+\trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n+\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len - 1] == '\\n')\n-\t\t\tline[--len] = 0;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (*line == '-') {\n-\t\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t\tflags ^= UNINTERESTING;\n-\t\t\t\twrite_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\t\tuse_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tdie(_(\"not a rev '%s'\"), line);\n-\t\t}\n-\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\t}\n-\n-\twarn_on_object_refname_ambiguity = save_warning;\n-\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \ndiff --git a/revision.c b/revision.c\nindex 69b3812093..878eb51032 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2119,6 +2119,25 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\tint len = sb.len;\n \t\tif (!len)\n \t\t\tbreak;\n+\n+\t\tif (revs->handle_stdin_line) {\n+\t\t\tint do_break = 0;\n+\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n+\t\t\t\trevs, &sb, revs->stdin_line_priv);\n+\t\n+\t\t\tswitch (ret) {\n+\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_BREAK:\n+\t\t\t\tdo_break = 1;\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (do_break)\n+\t\t\t\tbreak;\n+\t\t}\n+\n \t\tif (sb.buf[0] == '-') {\n \t\t\tif (len == 2 && sb.buf[1] == '-') {\n \t\t\t\tseen_dashdash = 1;\n@@ -2787,6 +2806,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t}\n \t}\n \n+\t/*\n+\t * We've got always_read_from_stdin but no --stdin (or\n+\t * \"consumed_stdin_per_option\" would be set).\n+\t */\n+\tif (revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ &&\n+\t !revs->consumed_stdin_per_option)\n+\t\tread_revisions_from_stdin(revs, &prune_data);\n+\n \tif (prune_data.nr) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\ndiff --git a/revision.h b/revision.h\nindex 03231f089f..9d977cd3cc 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -89,8 +89,18 @@ struct topo_walk_info;\n enum rev_info_stdin {\n \tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n \tREV_INFO_STDIN_IGNORE,\n+\tREV_INFO_STDIN_ALWAYS_READ,\n };\n \n+enum rev_info_stdin_line {\n+\tREV_INFO_STDIN_LINE_PROCESS,\n+\tREV_INFO_STDIN_LINE_BREAK,\n+\tREV_INFO_STDIN_LINE_CONTINUE,\n+};\n+\n+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(\n+\tstruct rev_info *revs, struct strbuf *line, void *stdin_line_priv);\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -126,6 +136,9 @@ struct rev_info {\n \t *\n \t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n \t * --stdin option.\n+\t *\n+\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n+\t * on stdin to be read, even if no --stdin option is provided.\n \t */\n \tenum rev_info_stdin stdin_handling;\n \n@@ -136,6 +149,24 @@ struct rev_info {\n \t */\n \tint consumed_stdin_per_option;\n \n+\t/*\n+\t * When reading from stdin (see \"stdin_handling\" above) define\n+\t * a handle_stdin_line function to consume the lines.\n+\t *\n+\t * - Return 0 to continue revision.c's normal processing of the\n+\t * line (after possibly munging the provided strbuf).\n+\t *\n+\t * - Return 1 to indicate that the line is fully processed,\n+ * moving onto the next line (if any)\n+\t *\n+\t * - Return 2 to process no further lines.\n+\t *\n+\t * Use the \"stdin_line_priv\" to optionally pass your own data\n+\t * around.\n+\t */\n+\trev_info_stdin_line_func handle_stdin_line;\n+\tvoid *stdin_line_priv;\n+\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426762","messageId":"patch-1.4-c56a302e09-20210608T121008Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com","subject":"[PATCH 1/4] upload-pack: run is_repository_shallow() before setup_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T12:16:27Z","receivedAt":"2021-06-08T12:17:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the is_repository_shallow() added in b790e0f67cd (upload-pack:\nsend shallow info over stdin to pack-objects, 2014-03-11) to above\nsetup_revisions().\n\nRunning is_repository_shallow() before setup_revisions() doesn't\nmatter now, but in subsequent commits we'll make the code that\nfollowed setup_revisions() happen inside a callback in that\nfunction. This isolated change documents that re-arranging this part\nof the code is OK in isolation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de00adbb9e..1fbaa34f91 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n+\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426763","messageId":"patch-4.4-15a3a5d047-20210608T121008Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com","subject":"[PATCH 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-08T12:16:30Z","receivedAt":"2021-06-08T12:17:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue the work started in the preceding commit of porting\npack-objects.c over to the new handle_stdin_line callback pattern. The\ncommon case for the users of this API is to do some of their own\nparsing or munging, and then have handle_revision_arg() handle the\nrest.\n\nThe existing users of the --stdin parsing always wanted a flag of \"0\"\nto be passed to handle_revision_arg(), but pack-objects.c wants to set\ncustom flags. Let's support this common case by having a\n\"revarg_flags\" member in the \"rev_info\" struct.\n\nThis allows us to return REV_INFO_STDIN_LINE_PROCESS in the new\nget_object_list_handle_stdin_line() instead of\nREV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now\npass down the right flag for us.\n\nThis also means that we can make the handle_revision_arg() function\nstatic. Now that the only external user of the API has been migrated\nover to the callback mechanism nothing external to revision.c needs to\ncall handle_revision_arg() anymore.\n\nThat handle_revision_arg() function was made public in a combination\nof 5d6f0935e6d (revision.c: allow injecting revision parameters after\nsetup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run\nrev-list equivalent internally., 2006-09-04).\n\nThis change leaves the briefly-used in preceding commits\n\"void *stdin_line_priv\" without any in-tree user, as\nbuiltin/pack-objects.c could be ported over to our new \"revarg_flags\"\ncommon case.\n\nI'm leaving that \"void *stdin_line_priv\" in place anyway for two\nreasons:\n\n 1. It's a common pattern to allow such a \"void *\" to be used for\n callback data, so we might as well follow that pattern here in\n anticipation of a future in-tree user.\n\n 2. I have patches for such an in-tree user already in a series\n that'll be submitted after this one.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 9 ++-------\n revision.c | 4 ++--\n revision.h | 6 ++++--\n 3 files changed, 8 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cbb01f2b2d..0ab2d10853 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3748,7 +3748,6 @@ static void mark_bitmap_preferred_tips(void)\n static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n {\n-\tint *flags = stdin_line_priv;\n \tchar *line = line_sb->buf;\n \tsize_t len = line_sb->len;\n \n@@ -3756,7 +3755,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \t\treturn REV_INFO_STDIN_LINE_BREAK;\n \tif (*line == '-') {\n \t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t*flags ^= UNINTERESTING;\n+\t\t\trevs->revarg_flags ^= UNINTERESTING;\n \t\t\twrite_bitmap_index = 0;\n \t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n \t\t}\n@@ -3770,9 +3769,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \t\t}\n \t\tdie(_(\"not a rev '%s'\"), line);\n \t}\n-\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\treturn REV_INFO_STDIN_LINE_PROCESS;\n }\n \n static void get_object_list(int ac, const char **av)\n@@ -3781,7 +3778,6 @@ static void get_object_list(int ac, const char **av)\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tint flags = 0;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3791,7 +3787,6 @@ static void get_object_list(int ac, const char **av)\n \n \trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n \trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n-\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\ndiff --git a/revision.c b/revision.c\nindex 878eb51032..8054a9d037 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \treturn 0;\n }\n \n-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tint ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);\n \tif (!ret)\n@@ -2145,7 +2145,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\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\tif (handle_revision_arg(sb.buf, revs, revs->revarg_flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\ndiff --git a/revision.h b/revision.h\nindex 9d977cd3cc..c55147e7f2 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -156,6 +156,9 @@ struct rev_info {\n \t * - Return 0 to continue revision.c's normal processing of the\n \t * line (after possibly munging the provided strbuf).\n \t *\n+\t * Change \"revarg_flags\" to affect the subsequent handling\n+\t * in handle_revision_arg()\n+\t *\n \t * - Return 1 to indicate that the line is fully processed,\n * moving onto the next line (if any)\n \t *\n@@ -165,6 +168,7 @@ struct rev_info {\n \t * around.\n \t */\n \trev_info_stdin_line_func handle_stdin_line;\n+\tint revarg_flags;\n \tvoid *stdin_line_priv;\n \n \t/* topo-sort */\n@@ -422,8 +426,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[]);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n-int handle_revision_arg(const char *arg, struct rev_info *revs,\n-\t\t\tint flags, unsigned revarg_opt);\n \n /**\n * Reset the flags used by the revision walking api. You can use this to do\n-- \n2.32.0.rc3.434.gd8aed1f08a7\n\n"},{"id":"426864","messageId":"xmqqk0n3ihla.fsf@gitster.g","threadId":"55881","inReplyTo":"patch-3.4-1a9eb26587-20210608T121008Z-avarab@gmail.com","subject":"Re: [PATCH 3/4] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-09T08:10:25Z","receivedAt":"2021-06-09T08:10:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> @@ -3761,39 +3789,11 @@ static void get_object_list(int ac, const char **av)\n> \t/* make sure shallows are read */\n> \tis_repository_shallow(the_repository);\n> \n> +\trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n> +\trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n> +\trevs.stdin_line_priv = &flags;\n> \tsetup_revisions(ac, av, &revs, &s_r_opt);\n> \n> -\tsave_warning = warn_on_object_refname_ambiguity;\n> -\twarn_on_object_refname_ambiguity = 0;\n> -\n> -\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n> -\t\tint len = strlen(line);\n> -\t\tif (len && line[len - 1] == '\\n')\n> -\t\t\tline[--len] = 0;\n> -\t\tif (!len)\n> -\t\t\tbreak;\n> -\t\tif (*line == '-') {\n> -\t\t\tif (!strcmp(line, \"--not\")) {\n> -\t\t\t\tflags ^= UNINTERESTING;\n> -\t\t\t\twrite_bitmap_index = 0;\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\tif (starts_with(line, \"--shallow \")) {\n> -\t\t\t\tstruct object_id oid;\n> -\t\t\t\tif (get_oid_hex(line + 10, &oid))\n> -\t\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n> -\t\t\t\tregister_shallow(the_repository, &oid);\n> -\t\t\t\tuse_bitmap_index = 0;\n> -\t\t\t\tcontinue;\n> -\t\t\t}\n> -\t\t\tdie(_(\"not a rev '%s'\"), line);\n> -\t\t}\n> -\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n> -\t\t\tdie(_(\"bad revision '%s'\"), line);\n> -\t}\n> -\n> -\twarn_on_object_refname_ambiguity = save_warning;\n> -\n\nOK, so this shows the primary value of the entire series---we\nmanaged to lose handwritten loop around a logic that gets migrated\ninto a callback function that has been used to read revisions from\nthe standard input elsewhere in the system.\n\nAll four patches are cleanly explained and nicely done.\n\nThanks.\n"},{"id":"427733","messageId":"cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com","subject":"[PATCH v2 0/4] pack-objects: use revision.c's --stdin parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T10:57:33Z","receivedAt":"2021-06-17T10:57:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A re-roll of\nhttps://lore.kernel.org/git/cover-0.4-0000000000-20210608T121008Z-avarab@gmail.com/\n\nFixes whitespace issues in v1, and uses the named enums I added\ninstead of their implicit values in the documentation. I also\nconflated BREAK and CONTINUE in that same explanation.\n\nÆvar Arnfjörð Bjarmason (4):\n upload-pack: run is_repository_shallow() before setup_revisions()\n revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n pack-objects.c: do stdin parsing via revision.c's API\n pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n\n builtin/am.c | 4 +--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/pack-objects.c | 64 +++++++++++++++++++---------------------\n builtin/rev-list.c | 2 +-\n revision.c | 35 +++++++++++++++++++---\n revision.h | 66 ++++++++++++++++++++++++++++++++++++++----\n sequencer.c | 4 +--\n 8 files changed, 129 insertions(+), 50 deletions(-)\n\nRange-diff against v1:\n1: c56a302e09 = 1: 6a8b20a7cf upload-pack: run is_repository_shallow() before setup_revisions()\n2: 002e0f1cf7 = 2: d88b2c0410 revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n3: 1a9eb26587 ! 3: d433d7b24a pack-objects.c: do stdin parsing via revision.c's API\n @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,\n +\t\t\tint do_break = 0;\n +\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n +\t\t\t\trevs, &sb, revs->stdin_line_priv);\n -+\t\n ++\n +\t\t\tswitch (ret) {\n +\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n +\t\t\t\tbreak;\n @@ revision.h: struct rev_info {\n +\t * When reading from stdin (see \"stdin_handling\" above) define\n +\t * a handle_stdin_line function to consume the lines.\n +\t *\n -+\t * - Return 0 to continue revision.c's normal processing of the\n -+\t * line (after possibly munging the provided strbuf).\n ++\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n ++\t * revision.c's normal processing of the line (after\n ++\t * possibly munging the provided strbuf).\n +\t *\n -+\t * - Return 1 to indicate that the line is fully processed,\n -+ * moving onto the next line (if any)\n ++\t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n ++\t * lines, or anything further from the current line (just\n ++\t * like REV_INFO_STDIN_LINE_CONTINUE).\n +\t *\n -+\t * - Return 2 to process no further lines.\n ++\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n ++\t * line is fully processed, moving onto the next line (if\n ++\t * any)\n +\t *\n +\t * Use the \"stdin_line_priv\" to optionally pass your own data\n +\t * around.\n4: 15a3a5d047 ! 4: e59a06c314 pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,\n \n ## revision.h ##\n @@ revision.h: struct rev_info {\n - \t * - Return 0 to continue revision.c's normal processing of the\n - \t * line (after possibly munging the provided strbuf).\n + \t * revision.c's normal processing of the line (after\n + \t * possibly munging the provided strbuf).\n \t *\n +\t * Change \"revarg_flags\" to affect the subsequent handling\n +\t * in handle_revision_arg()\n +\t *\n - \t * - Return 1 to indicate that the line is fully processed,\n - * moving onto the next line (if any)\n + \t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n + \t * lines, or anything further from the current line (just\n + \t * like REV_INFO_STDIN_LINE_CONTINUE).\n + \t *\n + \t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n ++\t *\n ++\t * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the\n + \t * line is fully processed, moving onto the next line (if\n + \t * any)\n \t *\n @@ revision.h: struct rev_info {\n \t * around.\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427734","messageId":"patch-1.4-6a8b20a7cf-20210617T105537Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com","subject":"[PATCH v2 1/4] upload-pack: run is_repository_shallow() before setup_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T10:57:34Z","receivedAt":"2021-06-17T10:57:50Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the is_repository_shallow() added in b790e0f67cd (upload-pack:\nsend shallow info over stdin to pack-objects, 2014-03-11) to above\nsetup_revisions().\n\nRunning is_repository_shallow() before setup_revisions() doesn't\nmatter now, but in subsequent commits we'll make the code that\nfollowed setup_revisions() happen inside a callback in that\nfunction. This isolated change documents that re-arranging this part\nof the code is OK in isolation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de00adbb9e..1fbaa34f91 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n+\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427735","messageId":"patch-2.4-d88b2c0410-20210617T105537Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com","subject":"[PATCH v2 2/4] revision.h: unify \"disable_stdin\" and \"read_from_stdin\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T10:57:35Z","receivedAt":"2021-06-17T10:57:51Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03) we\nadded the \"disable_stdin\" flag, and then much later in\na12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we\ngained a \"read_from_stdin\" flag.\n\nThe interaction between these is more subtle than they might appear at\nfirst sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\nof \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\noption.\n\nThe \"read\" is intended to understood as \"I read it\", not \"you should\nread it\". Let's avoid this confusion by using \"consume\" and \"consumed\"\ninstead, i.e. a word whose present and past tense isn't the same.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 4 ++--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/rev-list.c | 2 +-\n revision.c | 4 ++--\n revision.h | 23 ++++++++++++++++++++---\n sequencer.c | 4 ++--\n 7 files changed, 29 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b2d886c81..3a6c8455b4 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n \trev_info.abbrev = 0;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.show_root_diff = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.no_commit_id = 1;\n@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)\n \tfp = xfopen(am_path(state, \"patch\"), \"w\");\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.no_commit_id = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.diffopt.use_color = 0;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9a..c9f66c58c4 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\targv[argc - 1] = \"--\";\n \t}\n \n-\trevs.disable_stdin = 1;\n+\trevs.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tsetup_revisions(argc, argv, &revs, NULL);\n \tif (!revs.pending.nr && is_bare_repository()) {\n \t\tstruct commit *head_commit;\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex f33d30d57b..fc548ebe16 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"index file corrupt\"));\n \topt->abbrev = 0;\n \topt->diff = 1;\n-\topt->disable_stdin = 1;\n+\topt->stdin_handling = REV_INFO_STDIN_IGNORE;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.tweak = diff_tree_tweak_rev;\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 7677b1af5a..88bd9ef954 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr) &&\n-\t !revs.rev_input_given && !revs.read_from_stdin) ||\n+\t !revs.rev_input_given && !revs.consumed_stdin_per_option) ||\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \ndiff --git a/revision.c b/revision.c\nindex 8140561b6c..69b3812093 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n-\t\t\t\tif (revs->disable_stdin) {\n+\t\t\t\tif (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n \t\t\t\t}\n-\t\t\t\tif (revs->read_from_stdin++)\n+\t\t\t\tif (revs->consumed_stdin_per_option++)\n \t\t\t\t\tdie(\"--stdin given twice?\");\n \t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n \t\t\t\tcontinue;\ndiff --git a/revision.h b/revision.h\nindex 17698cb51a..475c8ed785 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -86,6 +86,11 @@ struct rev_cmdline_info {\n struct oidset;\n struct topo_walk_info;\n \n+enum rev_info_stdin {\n+\tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n+\tREV_INFO_STDIN_IGNORE,\n+};\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -114,9 +119,22 @@ struct rev_info {\n \tint rev_input_given;\n \n \t/*\n-\t * Whether we read from stdin due to the --stdin option.\n+\t * How should we handle seeing --stdin?\n+\t *\n+\t * Defaults to reading if we see it with\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION.\n+\t *\n+\t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n+\t * --stdin option.\n+\t */\n+\tenum rev_info_stdin stdin_handling;\n+\n+\t/*\n+\t * Did we read from stdin due to stdin_handling ==\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n+\t * option?\n \t */\n-\tint read_from_stdin;\n+\tint consumed_stdin_per_option;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n@@ -216,7 +234,6 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1;\n-\tunsigned int\tdisable_stdin:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n \t\t\ttrack_first_time:1,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38..4e73bd79d6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,\n \tlog_tree_opt.abbrev = 0;\n \tlog_tree_opt.diff = 1;\n \tlog_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\tlog_tree_opt.disable_stdin = 1;\n+\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tlog_tree_opt.no_commit_id = 1;\n \tlog_tree_opt.diffopt.file = fopen(buf.buf, \"w\");\n \tlog_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;\n@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,\n \t\t\tlog_tree_opt.diff = 1;\n \t\t\tlog_tree_opt.diffopt.output_format =\n \t\t\t\tDIFF_FORMAT_DIFFSTAT;\n-\t\t\tlog_tree_opt.disable_stdin = 1;\n+\t\t\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \n \t\t\tif (read_oneliner(&buf, rebase_path_orig_head(), 0) &&\n \t\t\t !get_oid(buf.buf, &orig) &&\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427736","messageId":"patch-3.4-d433d7b24a-20210617T105537Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com","subject":"[PATCH v2 3/4] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T10:57:36Z","receivedAt":"2021-06-17T10:57:55Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the fgets(..., stdin) parsing in pack-objects.c to use a\nnow-extended version of the rev_info stdin parsing API.\n\nThe fgets() loop being refactored away here was first added in Linus's\nc323ac7d9c5 (git-pack-objects: create a packed object representation.,\n2005-06-25).\n\nLater on rev-list started doing similar parsing in 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05),\nand that code was promoted to a more general API in 1fc561d169a (Move\nread_revisions_from_stdin from builtin-rev-list.c to revision.c,\n2008-07-05).\n\nSince then the API in revision.c has received improvements that have\nbeen missed here. E.g. the arbitrary limit of 1000 bytes was removed\nin 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),\nand it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:\nread --stdin with strbuf_getline(), 2015-10-28).\n\nLet's do the same here, as in 6e8d46f9d4b we can remove the \"if (len\n&& line[len - 1] == '\\n')\" check, it's now redundant to using\nstrbuf_getline(), and we get to skip the whole\n\"warn_on_object_refname_ambiguity\" dance. The\nread_revisions_from_stdin() function in revision.c we're now using\ndoes it for us.\n\nFor now there isn't all that much point in this whole exercises. We\njust end up calling setup_revisions() to loop over stdin for us, but\nthe callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we\nstill need to call handle_revision_arg() ourselves because we'd like\nto call it with different flags.\n\nThis very light use of the API will be further refined in a subsequent\ncommit, for now we're just doing the bare minimum to move this\nexisting code over to the new callback pattern without any functional\nchanges, and making it as friendly to \"git show -w\" and \"the\n--color-moved-ws=allow-indentation-change\" mode as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 66 +++++++++++++++++++++---------------------\n revision.c | 27 +++++++++++++++++\n revision.h | 35 ++++++++++++++++++++++\n 3 files changed, 95 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1fbaa34f91..cbb01f2b2d 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3745,15 +3745,43 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n+static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n+\tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n+{\n+\tint *flags = stdin_line_priv;\n+\tchar *line = line_sb->buf;\n+\tsize_t len = line_sb->len;\n+\n+\tif (!len)\n+\t\treturn REV_INFO_STDIN_LINE_BREAK;\n+\tif (*line == '-') {\n+\t\tif (!strcmp(line, \"--not\")) {\n+\t\t\t*flags ^= UNINTERESTING;\n+\t\t\twrite_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\tstruct object_id oid;\n+\t\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\t\tregister_shallow(the_repository, &oid);\n+\t\t\tuse_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tdie(_(\"not a rev '%s'\"), line);\n+\t}\n+\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(_(\"bad revision '%s'\"), line);\n+\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tchar line[1000];\n \tint flags = 0;\n-\tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3761,39 +3789,11 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n+\trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n+\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len - 1] == '\\n')\n-\t\t\tline[--len] = 0;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (*line == '-') {\n-\t\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t\tflags ^= UNINTERESTING;\n-\t\t\t\twrite_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\t\tuse_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tdie(_(\"not a rev '%s'\"), line);\n-\t\t}\n-\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\t}\n-\n-\twarn_on_object_refname_ambiguity = save_warning;\n-\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \ndiff --git a/revision.c b/revision.c\nindex 69b3812093..2ca2c38447 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2119,6 +2119,25 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\tint len = sb.len;\n \t\tif (!len)\n \t\t\tbreak;\n+\n+\t\tif (revs->handle_stdin_line) {\n+\t\t\tint do_break = 0;\n+\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n+\t\t\t\trevs, &sb, revs->stdin_line_priv);\n+\n+\t\t\tswitch (ret) {\n+\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_BREAK:\n+\t\t\t\tdo_break = 1;\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (do_break)\n+\t\t\t\tbreak;\n+\t\t}\n+\n \t\tif (sb.buf[0] == '-') {\n \t\t\tif (len == 2 && sb.buf[1] == '-') {\n \t\t\t\tseen_dashdash = 1;\n@@ -2787,6 +2806,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t}\n \t}\n \n+\t/*\n+\t * We've got always_read_from_stdin but no --stdin (or\n+\t * \"consumed_stdin_per_option\" would be set).\n+\t */\n+\tif (revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ &&\n+\t !revs->consumed_stdin_per_option)\n+\t\tread_revisions_from_stdin(revs, &prune_data);\n+\n \tif (prune_data.nr) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\ndiff --git a/revision.h b/revision.h\nindex 475c8ed785..5df1523a6c 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -89,8 +89,18 @@ struct topo_walk_info;\n enum rev_info_stdin {\n \tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n \tREV_INFO_STDIN_IGNORE,\n+\tREV_INFO_STDIN_ALWAYS_READ,\n };\n \n+enum rev_info_stdin_line {\n+\tREV_INFO_STDIN_LINE_PROCESS,\n+\tREV_INFO_STDIN_LINE_BREAK,\n+\tREV_INFO_STDIN_LINE_CONTINUE,\n+};\n+\n+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(\n+\tstruct rev_info *revs, struct strbuf *line, void *stdin_line_priv);\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -126,6 +136,9 @@ struct rev_info {\n \t *\n \t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n \t * --stdin option.\n+\t *\n+\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n+\t * on stdin to be read, even if no --stdin option is provided.\n \t */\n \tenum rev_info_stdin stdin_handling;\n \n@@ -136,6 +149,28 @@ struct rev_info {\n \t */\n \tint consumed_stdin_per_option;\n \n+\t/*\n+\t * When reading from stdin (see \"stdin_handling\" above) define\n+\t * a handle_stdin_line function to consume the lines.\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n+\t * revision.c's normal processing of the line (after\n+\t * possibly munging the provided strbuf).\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n+\t * lines, or anything further from the current line (just\n+\t * like REV_INFO_STDIN_LINE_CONTINUE).\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n+\t * line is fully processed, moving onto the next line (if\n+\t * any)\n+\t *\n+\t * Use the \"stdin_line_priv\" to optionally pass your own data\n+\t * around.\n+\t */\n+\trev_info_stdin_line_func handle_stdin_line;\n+\tvoid *stdin_line_priv;\n+\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427737","messageId":"patch-4.4-e59a06c314-20210617T105537Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com","subject":"[PATCH v2 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-17T10:57:37Z","receivedAt":"2021-06-17T10:57:56Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue the work started in the preceding commit of porting\npack-objects.c over to the new handle_stdin_line callback pattern. The\ncommon case for the users of this API is to do some of their own\nparsing or munging, and then have handle_revision_arg() handle the\nrest.\n\nThe existing users of the --stdin parsing always wanted a flag of \"0\"\nto be passed to handle_revision_arg(), but pack-objects.c wants to set\ncustom flags. Let's support this common case by having a\n\"revarg_flags\" member in the \"rev_info\" struct.\n\nThis allows us to return REV_INFO_STDIN_LINE_PROCESS in the new\nget_object_list_handle_stdin_line() instead of\nREV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now\npass down the right flag for us.\n\nThis also means that we can make the handle_revision_arg() function\nstatic. Now that the only external user of the API has been migrated\nover to the callback mechanism nothing external to revision.c needs to\ncall handle_revision_arg() anymore.\n\nThat handle_revision_arg() function was made public in a combination\nof 5d6f0935e6d (revision.c: allow injecting revision parameters after\nsetup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run\nrev-list equivalent internally., 2006-09-04).\n\nThis change leaves the briefly-used in preceding commits\n\"void *stdin_line_priv\" without any in-tree user, as\nbuiltin/pack-objects.c could be ported over to our new \"revarg_flags\"\ncommon case.\n\nI'm leaving that \"void *stdin_line_priv\" in place anyway for two\nreasons:\n\n 1. It's a common pattern to allow such a \"void *\" to be used for\n callback data, so we might as well follow that pattern here in\n anticipation of a future in-tree user.\n\n 2. I have patches for such an in-tree user already in a series\n that'll be submitted after this one.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 9 ++-------\n revision.c | 4 ++--\n revision.h | 8 ++++++--\n 3 files changed, 10 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex cbb01f2b2d..0ab2d10853 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3748,7 +3748,6 @@ static void mark_bitmap_preferred_tips(void)\n static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n {\n-\tint *flags = stdin_line_priv;\n \tchar *line = line_sb->buf;\n \tsize_t len = line_sb->len;\n \n@@ -3756,7 +3755,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \t\treturn REV_INFO_STDIN_LINE_BREAK;\n \tif (*line == '-') {\n \t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t*flags ^= UNINTERESTING;\n+\t\t\trevs->revarg_flags ^= UNINTERESTING;\n \t\t\twrite_bitmap_index = 0;\n \t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n \t\t}\n@@ -3770,9 +3769,7 @@ static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \t\t}\n \t\tdie(_(\"not a rev '%s'\"), line);\n \t}\n-\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\treturn REV_INFO_STDIN_LINE_PROCESS;\n }\n \n static void get_object_list(int ac, const char **av)\n@@ -3781,7 +3778,6 @@ static void get_object_list(int ac, const char **av)\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tint flags = 0;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3791,7 +3787,6 @@ static void get_object_list(int ac, const char **av)\n \n \trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n \trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n-\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\ndiff --git a/revision.c b/revision.c\nindex 2ca2c38447..ad94a646f7 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \treturn 0;\n }\n \n-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tint ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);\n \tif (!ret)\n@@ -2145,7 +2145,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\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\tif (handle_revision_arg(sb.buf, revs, revs->revarg_flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\ndiff --git a/revision.h b/revision.h\nindex 5df1523a6c..c473567d82 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -157,11 +157,16 @@ struct rev_info {\n \t * revision.c's normal processing of the line (after\n \t * possibly munging the provided strbuf).\n \t *\n+\t * Change \"revarg_flags\" to affect the subsequent handling\n+\t * in handle_revision_arg()\n+\t *\n \t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n \t * lines, or anything further from the current line (just\n \t * like REV_INFO_STDIN_LINE_CONTINUE).\n \t *\n \t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the\n \t * line is fully processed, moving onto the next line (if\n \t * any)\n \t *\n@@ -169,6 +174,7 @@ struct rev_info {\n \t * around.\n \t */\n \trev_info_stdin_line_func handle_stdin_line;\n+\tint revarg_flags;\n \tvoid *stdin_line_priv;\n \n \t/* topo-sort */\n@@ -426,8 +432,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[]);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n-int handle_revision_arg(const char *arg, struct rev_info *revs,\n-\t\t\tint flags, unsigned revarg_opt);\n \n /**\n * Reset the flags used by the revision walking api. You can use this to do\n-- \n2.32.0.571.gdba276db2c\n\n"},{"id":"427797","messageId":"YMveZXZRM5vHYRx5@google.com","threadId":"55881","inReplyTo":"patch-2.4-d88b2c0410-20210617T105537Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/4] revision.h: unify \"disable_stdin\" and \"read_from_stdin\"","fromName":"Emily Shaffer","fromEmail":"emilyshaffer@google.com","sentAt":"2021-06-17T23:44:37Z","receivedAt":"2021-06-17T23:44:47Z","isPatch":true,"sender":{"key":"nasamuffin@google.com","avatar":"https://avatars.githubusercontent.com/u/1606826?v=4"},"body":"On Thu, Jun 17, 2021 at 12:57:35PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> \n> In 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03) we\n> added the \"disable_stdin\" flag, and then much later in\n> a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we\n> gained a \"read_from_stdin\" flag.\n> \n> The interaction between these is more subtle than they might appear at\n> first sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\n> of \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\n> option.\n> \n> The \"read\" is intended to understood as \"I read it\", not \"you should\n> read it\". Let's avoid this confusion by using \"consume\" and \"consumed\"\n> instead, i.e. a word whose present and past tense isn't the same.\n\nUnfortunately, I still find your disambiguation text to be ambiguous...\n\n> diff --git a/revision.c b/revision.c\n> index 8140561b6c..69b3812093 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -2741,11 +2741,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n> \t\t\t}\n> \n> \t\t\tif (!strcmp(arg, \"--stdin\")) {\n> -\t\t\t\tif (revs->disable_stdin) {\n> +\t\t\t\tif (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {\n> \t\t\t\t\targv[left++] = arg;\n> \t\t\t\t\tcontinue;\n> \t\t\t\t}\n> -\t\t\t\tif (revs->read_from_stdin++)\n> +\t\t\t\tif (revs->consumed_stdin_per_option++)\n> \t\t\t\t\tdie(\"--stdin given twice?\");\n> \t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n> \t\t\t\tcontinue;\n\nEeh. I know this is not logic introduced by your patch, BUT your patch\ndoes change the semantics of the replacement to ->disable_stdin from a\nbool to an enum. There is an implicit assumption here - \"if\nstdin_handling isn't REV_INFO_STDIN_IGNORE, then it must be\nREV_INFO_CONSUME_ON_OPTION.\" That's true during this patch, but becomes\nfalse in the next patch in this series, and it didn't look to me like\nthis section of code was changed accordingly.\n[snip]\n> +\t/*\n> +\t * Did we read from stdin due to stdin_handling ==\n> +\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n> +\t * option?\n> \t */\n> -\tint read_from_stdin;\n> +\tint consumed_stdin_per_option;\n\nAnd indeed, the comment here confirms the implicit assumption made in\nthe code. Based on this comment, I'd expect the implementation to\nexplicitly check that stdin_handling == CONSUME_ON_OPTION.\n\n - Emily\n"},{"id":"427844","messageId":"20210618175443.773403-1-jonathantanmy@google.com","threadId":"55881","inReplyTo":"patch-2.4-d88b2c0410-20210617T105537Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/4] revision.h: unify \"disable_stdin\" and \"read_from_stdin\"","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-18T17:54:43Z","receivedAt":"2021-06-18T17:54:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"About the subject:\n\n> revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n\nHmm...there is no unification, I think? Both of these remain distinct -\nit's just that both were renamed and one was changed into an enum.\n\n> @@ -114,9 +119,22 @@ struct rev_info {\n> \tint rev_input_given;\n> \n> \t/*\n> -\t * Whether we read from stdin due to the --stdin option.\n> +\t * How should we handle seeing --stdin?\n> +\t *\n> +\t * Defaults to reading if we see it with\n> +\t * REV_INFO_STDIN_CONSUME_ON_OPTION.\n> +\t *\n> +\t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n> +\t * --stdin option.\n> +\t */\n> +\tenum rev_info_stdin stdin_handling;\n\nThis was changed to an enum, because (looking at the next patches) we\nwant to add an entry to it. Maybe mention it here - at the very least,\nthis will help reviewers check that the addition of extra entries to the\nenum in future commits will not negatively affect functionality\nintroduced in this commit.\n\n> +\t/*\n> +\t * Did we read from stdin due to stdin_handling ==\n> +\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n> +\t * option?\n> \t */\n> -\tint read_from_stdin;\n> +\tint consumed_stdin_per_option;\n\nThe usage of \"per\" here seems correct to me, but it's not the first\nmeaning that springs to mind. Could this just be called\n\"consumed_stdin\"?\n"},{"id":"427892","messageId":"20210618185728.779629-1-jonathantanmy@google.com","threadId":"55881","inReplyTo":"patch-3.4-d433d7b24a-20210617T105537Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/4] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-06-18T18:57:27Z","receivedAt":"2021-06-18T18:57:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Change the fgets(..., stdin) parsing in pack-objects.c to use a\n> now-extended version of the rev_info stdin parsing API.\n\nThis leads me to think that the stdin parsing API is already present,\nbut it turns out that it still needs to be extended. Maybe rewrite this\nas \"extend the rev_info stdin parsing API to support , and change the <...> in pack-objects.c to use it\".\n\n> @@ -136,6 +149,28 @@ struct rev_info {\n> \t */\n> \tint consumed_stdin_per_option;\n> \n> +\t/*\n> +\t * When reading from stdin (see \"stdin_handling\" above) define\n> +\t * a handle_stdin_line function to consume the lines.\n> +\t *\n> +\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n> +\t * revision.c's normal processing of the line (after\n> +\t * possibly munging the provided strbuf).\n> +\t *\n> +\t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n> +\t * lines, or anything further from the current line (just\n> +\t * like REV_INFO_STDIN_LINE_CONTINUE).\n> +\t *\n> +\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n> +\t * line is fully processed, moving onto the next line (if\n> +\t * any)\n> +\t *\n> +\t * Use the \"stdin_line_priv\" to optionally pass your own data\n> +\t * around.\n> +\t */\n> +\trev_info_stdin_line_func handle_stdin_line;\n> +\tvoid *stdin_line_priv;\n\nI assume that all 3 of these are needed now to minimize the diff (which\nis fine), but maybe comment on which of these are intended to stay and\nwhich are only temporary. In particular, (once things are stabilized)\nwhen would a caller need BREAK?\n\nAlso mention what happens if handle_stdin_line is not given (I presume\nthat it would just be like a no-op function that returns PROCESS every\ntime).\n\nLooking at the bigger picture, though, and looking at the next patch,\nthis looks like a confusing API especially with the presence of \"--not\".\n(In the next patch, you had to introduce a flags field, and the callback\nhas to remember to always handle \"--not\" and to toggle the flag.)\nPerhaps the \"--not\" problem can be solved by making it revision.c's\nconcern and having the callback take a boolean parameter indicating\nwhether \"--not\" is active or not.\n"},{"id":"428041","messageId":"cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-0000000000-20210617T105537Z-avarab@gmail.com","subject":"[PATCH v3 0/4] pack-objects: use revision.c's --stdin parsing","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:10:12Z","receivedAt":"2021-06-21T15:10:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This v3 fixes issues noted by Emily Shaffer & Jonathan Tan on v2,\nthanks both!\n\nThe REV_INFO_STDIN_LINE_BREAK is gone, it was just a result of some\nWIP hacking, but I was too close to the code to notice. The \"!len\"\ncheck in pack-objects.c was also redundant by moving to the revision.c\nAPI. I have a parallel just-submitted series[1] that tests for that,\nthis version passes tests when combined with those more exhaustive\ntests.\n\nThere were various other nits & suggestions that I either addressed by\nchanging the relevant code / documentation as suggested, or (e.g. in\nthe case of the \"--not\" parsing) explained in commit message(s) why it\nmade sense to leave those special-cases in place.\n\n1. https://lore.kernel.org/git/cover-0.2-00000000000-20210621T145819Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (4):\n upload-pack: run is_repository_shallow() before setup_revisions()\n revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"\n pack-objects.c: do stdin parsing via revision.c's API\n pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n\n builtin/am.c | 4 +--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------\n builtin/rev-list.c | 2 +-\n revision.c | 38 +++++++++++++++++++++-----\n revision.h | 59 ++++++++++++++++++++++++++++++++++++----\n sequencer.c | 4 +--\n 8 files changed, 119 insertions(+), 53 deletions(-)\n\nRange-diff against v2:\n1: 6a8b20a7cf3 = 1: 3840ac28e8d upload-pack: run is_repository_shallow() before setup_revisions()\n2: d88b2c04102 ! 2: 6f69644b808 revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - revision.h: unify \"disable_stdin\" and \"read_from_stdin\"\n + revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"\n \n - In 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03) we\n - added the \"disable_stdin\" flag, and then much later in\n - a12cbe23ef7 (rev-list: make empty --stdin not an error, 2018-08-22) we\n - gained a \"read_from_stdin\" flag.\n + Change the two \"disable_stdin\" and \"read_from_stdin\" flags to an enum,\n + in preparation for a subsequent commit adding more flags.\n \n The interaction between these is more subtle than they might appear at\n first sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\n @@ Commit message\n read it\". Let's avoid this confusion by using \"consume\" and \"consumed\"\n instead, i.e. a word whose present and past tense isn't the same.\n \n + See 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03)\n + where we added the \"disable_stdin\" flag, and a12cbe23ef7 (rev-list:\n + make empty --stdin not an error, 2018-08-22) for the addition of the\n + \"read_from_stdin\" flag.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## builtin/am.c ##\n @@ builtin/rev-list.c: int cmd_rev_list(int argc, const char **argv, const char *pr\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr) &&\n -\t !revs.rev_input_given && !revs.read_from_stdin) ||\n -+\t !revs.rev_input_given && !revs.consumed_stdin_per_option) ||\n ++\t !revs.rev_input_given && !revs.consumed_stdin) ||\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \n @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n -\t\t\t\tif (revs->disable_stdin) {\n -+\t\t\t\tif (revs->stdin_handling == REV_INFO_STDIN_IGNORE) {\n ++\t\t\t\tswitch (revs->stdin_handling) {\n ++\t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n ++\t\t\t\tcase REV_INFO_STDIN_CONSUME_ON_OPTION:\n ++\t\t\t\t\tif (revs->consumed_stdin++)\n ++\t\t\t\t\t\tdie(\"--stdin given twice?\");\n ++\t\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n ++\t\t\t\t\tcontinue;\n \t\t\t\t}\n -\t\t\t\tif (revs->read_from_stdin++)\n -+\t\t\t\tif (revs->consumed_stdin_per_option++)\n - \t\t\t\t\tdie(\"--stdin given twice?\");\n - \t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n - \t\t\t\tcontinue;\n +-\t\t\t\t\tdie(\"--stdin given twice?\");\n +-\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n +-\t\t\t\tcontinue;\n + \t\t\t}\n + \n + \t\t\tif (!strcmp(arg, \"--end-of-options\")) {\n \n ## revision.h ##\n @@ revision.h: struct rev_cmdline_info {\n @@ revision.h: struct rev_info {\n -\t * Whether we read from stdin due to the --stdin option.\n +\t * How should we handle seeing --stdin?\n +\t *\n -+\t * Defaults to reading if we see it with\n -+\t * REV_INFO_STDIN_CONSUME_ON_OPTION.\n ++\t * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll\n ++\t * attempt to read it if we see the --stdin option.\n +\t *\n -+\t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n -+\t * --stdin option.\n ++\t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n ++\t * option.\n +\t */\n +\tenum rev_info_stdin stdin_handling;\n +\n @@ revision.h: struct rev_info {\n +\t * option?\n \t */\n -\tint read_from_stdin;\n -+\tint consumed_stdin_per_option;\n ++\tint consumed_stdin;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n3: d433d7b24a3 ! 3: 943b1b4c12a pack-objects.c: do stdin parsing via revision.c's API\n @@ Metadata\n ## Commit message ##\n pack-objects.c: do stdin parsing via revision.c's API\n \n - Change the fgets(..., stdin) parsing in pack-objects.c to use a\n - now-extended version of the rev_info stdin parsing API.\n + Extend the rev_info stdin parsing API to support hooking into its\n + read_revisions_from_stdin() function, and change the custom stdin\n + parsing in pack-objects.c to use it.\n \n - The fgets() loop being refactored away here was first added in Linus's\n - c323ac7d9c5 (git-pack-objects: create a packed object representation.,\n - 2005-06-25).\n + The pack-objects.c code being refactored away here was first added in\n + Linus's c323ac7d9c5 (git-pack-objects: create a packed object\n + representation., 2005-06-25).\n \n Later on rev-list started doing similar parsing in 42cabc341c4 (Teach\n - rev-list an option to read revs from the standard input., 2006-09-05),\n - and that code was promoted to a more general API in 1fc561d169a (Move\n + rev-list an option to read revs from the standard input., 2006-09-05).\n + That code was promoted to a more general API in 1fc561d169a (Move\n read_revisions_from_stdin from builtin-rev-list.c to revision.c,\n 2008-07-05).\n \n @@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)\n +{\n +\tint *flags = stdin_line_priv;\n +\tchar *line = line_sb->buf;\n -+\tsize_t len = line_sb->len;\n +\n -+\tif (!len)\n -+\t\treturn REV_INFO_STDIN_LINE_BREAK;\n +\tif (*line == '-') {\n +\t\tif (!strcmp(line, \"--not\")) {\n +\t\t\t*flags ^= UNINTERESTING;\n @@ revision.c: static void read_revisions_from_stdin(struct rev_info *revs,\n \t\t\tbreak;\n +\n +\t\tif (revs->handle_stdin_line) {\n -+\t\t\tint do_break = 0;\n +\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n +\t\t\t\trevs, &sb, revs->stdin_line_priv);\n +\n +\t\t\tswitch (ret) {\n +\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n +\t\t\t\tbreak;\n -+\t\t\tcase REV_INFO_STDIN_LINE_BREAK:\n -+\t\t\t\tdo_break = 1;\n -+\t\t\t\tbreak;\n +\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n +\t\t\t\tcontinue;\n +\t\t\t}\n -+\t\t\tif (do_break)\n -+\t\t\t\tbreak;\n +\t\t}\n +\n \t\tif (sb.buf[0] == '-') {\n \t\t\tif (len == 2 && sb.buf[1] == '-') {\n \t\t\t\tseen_dashdash = 1;\n @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n + \n + \t\t\tif (!strcmp(arg, \"--stdin\")) {\n + \t\t\t\tswitch (revs->stdin_handling) {\n ++\t\t\t\tcase REV_INFO_STDIN_ALWAYS_READ:\n + \t\t\t\tcase REV_INFO_STDIN_IGNORE:\n + \t\t\t\t\targv[left++] = arg;\n + \t\t\t\t\tcontinue;\n +@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t}\n \t}\n \n +\t/*\n -+\t * We've got always_read_from_stdin but no --stdin (or\n -+\t * \"consumed_stdin_per_option\" would be set).\n ++\t * We're asked to ALWAYS_READ from stdin, but no --stdin\n ++\t * option (or \"consumed_stdin\" would be set).\n +\t */\n -+\tif (revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ &&\n -+\t !revs->consumed_stdin_per_option)\n ++\tif (!revs->consumed_stdin &&\n ++\t revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)\n +\t\tread_revisions_from_stdin(revs, &prune_data);\n +\n \tif (prune_data.nr) {\n @@ revision.h: struct topo_walk_info;\n \n +enum rev_info_stdin_line {\n +\tREV_INFO_STDIN_LINE_PROCESS,\n -+\tREV_INFO_STDIN_LINE_BREAK,\n +\tREV_INFO_STDIN_LINE_CONTINUE,\n +};\n +\n @@ revision.h: struct topo_walk_info;\n \tstruct commit_list *commits;\n @@ revision.h: struct rev_info {\n \t *\n - \t * Can be set to REV_INFO_STDIN_IGNORE to ignore any provided\n - \t * --stdin option.\n + \t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n + \t * option.\n +\t *\n +\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n +\t * on stdin to be read, even if no --stdin option is provided.\n @@ revision.h: struct rev_info {\n \n @@ revision.h: struct rev_info {\n \t */\n - \tint consumed_stdin_per_option;\n + \tint consumed_stdin;\n \n +\t/*\n +\t * When reading from stdin (see \"stdin_handling\" above) define\n @@ revision.h: struct rev_info {\n +\t * revision.c's normal processing of the line (after\n +\t * possibly munging the provided strbuf).\n +\t *\n -+\t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n -+\t * lines, or anything further from the current line (just\n -+\t * like REV_INFO_STDIN_LINE_CONTINUE).\n -+\t *\n +\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n +\t * line is fully processed, moving onto the next line (if\n +\t * any)\n4: e59a06c3148 ! 4: 34750ab81cf pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n @@ Commit message\n REV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now\n pass down the right flag for us.\n \n - This also means that we can make the handle_revision_arg() function\n - static. Now that the only external user of the API has been migrated\n - over to the callback mechanism nothing external to revision.c needs to\n - call handle_revision_arg() anymore.\n + I considered making the \"--not\" parsing be another flag handled by the\n + revision.c API itself, but since there's only one caller who wants\n + this, and the \"write_bitmap_index = 0\" case is more specific than\n + marking things UNINTERESTING I think it's better to handle it with a\n + more general mechanism.\n +\n + These changes means that we can make the handle_revision_arg()\n + function static. Now that the only external user of the API has been\n + migrated over to the callback mechanism nothing external to revision.c\n + needs to call handle_revision_arg() anymore.\n \n That handle_revision_arg() function was made public in a combination\n of 5d6f0935e6d (revision.c: allow injecting revision parameters after\n @@ builtin/pack-objects.c: static void mark_bitmap_preferred_tips(void)\n {\n -\tint *flags = stdin_line_priv;\n \tchar *line = line_sb->buf;\n - \tsize_t len = line_sb->len;\n \n -@@ builtin/pack-objects.c: static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n - \t\treturn REV_INFO_STDIN_LINE_BREAK;\n - \tif (*line == '-') {\n - \t\tif (!strcmp(line, \"--not\")) {\n +-\tif (*line == '-') {\n +-\t\tif (!strcmp(line, \"--not\")) {\n -\t\t\t*flags ^= UNINTERESTING;\n -+\t\t\trevs->revarg_flags ^= UNINTERESTING;\n - \t\t\twrite_bitmap_index = 0;\n - \t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n - \t\t}\n -@@ builtin/pack-objects.c: static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n - \t\t}\n +-\t\t\twrite_bitmap_index = 0;\n +-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n +-\t\t}\n +-\t\tif (starts_with(line, \"--shallow \")) {\n +-\t\t\tstruct object_id oid;\n +-\t\t\tif (get_oid_hex(line + 10, &oid))\n +-\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n +-\t\t\tregister_shallow(the_repository, &oid);\n +-\t\t\tuse_bitmap_index = 0;\n +-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n +-\t\t}\n ++\tif (*line != '-')\n ++\t\treturn REV_INFO_STDIN_LINE_PROCESS;\n ++\n ++\tif (!strcmp(line, \"--not\")) {\n ++\t\trevs->revarg_flags ^= UNINTERESTING;\n ++\t\twrite_bitmap_index = 0;\n ++\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n ++\t} else if (starts_with(line, \"--shallow \")) {\n ++\t\tstruct object_id oid;\n ++\t\tif (get_oid_hex(line + 10, &oid))\n ++\t\t\tdie(\"not an object name '%s'\", line + 10);\n ++\t\tregister_shallow(the_repository, &oid);\n ++\t\tuse_bitmap_index = 0;\n ++\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n ++\t} else {\n \t\tdie(_(\"not a rev '%s'\"), line);\n \t}\n -\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n -\t\t\tdie(_(\"bad revision '%s'\"), line);\n -\treturn REV_INFO_STDIN_LINE_CONTINUE;\n -+\treturn REV_INFO_STDIN_LINE_PROCESS;\n }\n \n static void get_object_list(int ac, const char **av)\n @@ revision.h: struct rev_info {\n +\t * Change \"revarg_flags\" to affect the subsequent handling\n +\t * in handle_revision_arg()\n +\t *\n - \t * - Return REV_INFO_STDIN_LINE_BREAK to process no further\n - \t * lines, or anything further from the current line (just\n - \t * like REV_INFO_STDIN_LINE_CONTINUE).\n - \t *\n \t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n -+\t *\n -+\t * - Return REV_INFO_STDIN_LINE_BREAK to indicate that the\n \t * line is fully processed, moving onto the next line (if\n \t * any)\n - \t *\n @@ revision.h: struct rev_info {\n \t * around.\n \t */\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428042","messageId":"patch-1.4-3840ac28e8d-20210621T150651Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com","subject":"[PATCH v3 1/4] upload-pack: run is_repository_shallow() before setup_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:10:13Z","receivedAt":"2021-06-21T15:10:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the is_repository_shallow() added in b790e0f67cd (upload-pack:\nsend shallow info over stdin to pack-objects, 2014-03-11) to above\nsetup_revisions().\n\nRunning is_repository_shallow() before setup_revisions() doesn't\nmatter now, but in subsequent commits we'll make the code that\nfollowed setup_revisions() happen inside a callback in that\nfunction. This isolated change documents that re-arranging this part\nof the code is OK in isolation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 65579e09fe0..a4e2ae059c0 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3767,11 +3767,12 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n+\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428043","messageId":"patch-3.4-943b1b4c12a-20210621T150651Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com","subject":"[PATCH v3 3/4] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:10:15Z","receivedAt":"2021-06-21T15:10:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the rev_info stdin parsing API to support hooking into its\nread_revisions_from_stdin() function, and change the custom stdin\nparsing in pack-objects.c to use it.\n\nThe pack-objects.c code being refactored away here was first added in\nLinus's c323ac7d9c5 (git-pack-objects: create a packed object\nrepresentation., 2005-06-25).\n\nLater on rev-list started doing similar parsing in 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05).\nThat code was promoted to a more general API in 1fc561d169a (Move\nread_revisions_from_stdin from builtin-rev-list.c to revision.c,\n2008-07-05).\n\nSince then the API in revision.c has received improvements that have\nbeen missed here. E.g. the arbitrary limit of 1000 bytes was removed\nin 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),\nand it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:\nread --stdin with strbuf_getline(), 2015-10-28).\n\nLet's do the same here, as in 6e8d46f9d4b we can remove the \"if (len\n&& line[len - 1] == '\\n')\" check, it's now redundant to using\nstrbuf_getline(), and we get to skip the whole\n\"warn_on_object_refname_ambiguity\" dance. The\nread_revisions_from_stdin() function in revision.c we're now using\ndoes it for us.\n\nFor now there isn't all that much point in this whole exercises. We\njust end up calling setup_revisions() to loop over stdin for us, but\nthe callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we\nstill need to call handle_revision_arg() ourselves because we'd like\nto call it with different flags.\n\nThis very light use of the API will be further refined in a subsequent\ncommit, for now we're just doing the bare minimum to move this\nexisting code over to the new callback pattern without any functional\nchanges, and making it as friendly to \"git show -w\" and \"the\n--color-moved-ws=allow-indentation-change\" mode as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------\n revision.c | 22 +++++++++++++++\n revision.h | 30 ++++++++++++++++++++\n 3 files changed, 82 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a4e2ae059c0..f8cae6c305a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3755,15 +3755,40 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n+static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n+\tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n+{\n+\tint *flags = stdin_line_priv;\n+\tchar *line = line_sb->buf;\n+\n+\tif (*line == '-') {\n+\t\tif (!strcmp(line, \"--not\")) {\n+\t\t\t*flags ^= UNINTERESTING;\n+\t\t\twrite_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\tstruct object_id oid;\n+\t\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\t\tregister_shallow(the_repository, &oid);\n+\t\t\tuse_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tdie(_(\"not a rev '%s'\"), line);\n+\t}\n+\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(_(\"bad revision '%s'\"), line);\n+\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tchar line[1000];\n \tint flags = 0;\n-\tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3771,39 +3796,11 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n+\trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n+\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len - 1] == '\\n')\n-\t\t\tline[--len] = 0;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (*line == '-') {\n-\t\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t\tflags ^= UNINTERESTING;\n-\t\t\t\twrite_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\t\tuse_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tdie(_(\"not a rev '%s'\"), line);\n-\t\t}\n-\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\t}\n-\n-\twarn_on_object_refname_ambiguity = save_warning;\n-\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \ndiff --git a/revision.c b/revision.c\nindex a65f9b89e99..f55b3a1caf4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2119,6 +2119,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\tint len = sb.len;\n \t\tif (!len)\n \t\t\tbreak;\n+\n+\t\tif (revs->handle_stdin_line) {\n+\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n+\t\t\t\trevs, &sb, revs->stdin_line_priv);\n+\n+\t\t\tswitch (ret) {\n+\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (sb.buf[0] == '-') {\n \t\t\tif (len == 2 && sb.buf[1] == '-') {\n \t\t\t\tseen_dashdash = 1;\n@@ -2742,6 +2755,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tswitch (revs->stdin_handling) {\n+\t\t\t\tcase REV_INFO_STDIN_ALWAYS_READ:\n \t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n@@ -2789,6 +2803,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t}\n \t}\n \n+\t/*\n+\t * We're asked to ALWAYS_READ from stdin, but no --stdin\n+\t * option (or \"consumed_stdin\" would be set).\n+\t */\n+\tif (!revs->consumed_stdin &&\n+\t revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)\n+\t\tread_revisions_from_stdin(revs, &prune_data);\n+\n \tif (prune_data.nr) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\ndiff --git a/revision.h b/revision.h\nindex 79a3421cd1f..ff7994bb13d 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -89,8 +89,17 @@ struct topo_walk_info;\n enum rev_info_stdin {\n \tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n \tREV_INFO_STDIN_IGNORE,\n+\tREV_INFO_STDIN_ALWAYS_READ,\n };\n \n+enum rev_info_stdin_line {\n+\tREV_INFO_STDIN_LINE_PROCESS,\n+\tREV_INFO_STDIN_LINE_CONTINUE,\n+};\n+\n+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(\n+\tstruct rev_info *revs, struct strbuf *line, void *stdin_line_priv);\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -126,6 +135,9 @@ struct rev_info {\n \t *\n \t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n \t * option.\n+\t *\n+\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n+\t * on stdin to be read, even if no --stdin option is provided.\n \t */\n \tenum rev_info_stdin stdin_handling;\n \n@@ -136,6 +148,24 @@ struct rev_info {\n \t */\n \tint consumed_stdin;\n \n+\t/*\n+\t * When reading from stdin (see \"stdin_handling\" above) define\n+\t * a handle_stdin_line function to consume the lines.\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n+\t * revision.c's normal processing of the line (after\n+\t * possibly munging the provided strbuf).\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n+\t * line is fully processed, moving onto the next line (if\n+\t * any)\n+\t *\n+\t * Use the \"stdin_line_priv\" to optionally pass your own data\n+\t * around.\n+\t */\n+\trev_info_stdin_line_func handle_stdin_line;\n+\tvoid *stdin_line_priv;\n+\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428044","messageId":"patch-2.4-6f69644b808-20210621T150651Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com","subject":"[PATCH v3 2/4] revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:10:14Z","receivedAt":"2021-06-21T15:10:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the two \"disable_stdin\" and \"read_from_stdin\" flags to an enum,\nin preparation for a subsequent commit adding more flags.\n\nThe interaction between these is more subtle than they might appear at\nfirst sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\nof \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\noption.\n\nThe \"read\" is intended to understood as \"I read it\", not \"you should\nread it\". Let's avoid this confusion by using \"consume\" and \"consumed\"\ninstead, i.e. a word whose present and past tense isn't the same.\n\nSee 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03)\nwhere we added the \"disable_stdin\" flag, and a12cbe23ef7 (rev-list:\nmake empty --stdin not an error, 2018-08-22) for the addition of the\n\"read_from_stdin\" flag.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 4 ++--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/rev-list.c | 2 +-\n revision.c | 12 +++++++-----\n revision.h | 23 ++++++++++++++++++++---\n sequencer.c | 4 ++--\n 7 files changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b2d886c81b..3a6c8455b47 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n \trev_info.abbrev = 0;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.show_root_diff = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.no_commit_id = 1;\n@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)\n \tfp = xfopen(am_path(state, \"patch\"), \"w\");\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.no_commit_id = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.diffopt.use_color = 0;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..c9f66c58c46 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\targv[argc - 1] = \"--\";\n \t}\n \n-\trevs.disable_stdin = 1;\n+\trevs.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tsetup_revisions(argc, argv, &revs, NULL);\n \tif (!revs.pending.nr && is_bare_repository()) {\n \t\tstruct commit *head_commit;\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex f33d30d57bf..fc548ebe166 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"index file corrupt\"));\n \topt->abbrev = 0;\n \topt->diff = 1;\n-\topt->disable_stdin = 1;\n+\topt->stdin_handling = REV_INFO_STDIN_IGNORE;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.tweak = diff_tree_tweak_rev;\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 7677b1af5a4..524632ba328 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr) &&\n-\t !revs.rev_input_given && !revs.read_from_stdin) ||\n+\t !revs.rev_input_given && !revs.consumed_stdin) ||\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \ndiff --git a/revision.c b/revision.c\nindex 8140561b6c7..a65f9b89e99 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2741,14 +2741,16 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n-\t\t\t\tif (revs->disable_stdin) {\n+\t\t\t\tswitch (revs->stdin_handling) {\n+\t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n+\t\t\t\tcase REV_INFO_STDIN_CONSUME_ON_OPTION:\n+\t\t\t\t\tif (revs->consumed_stdin++)\n+\t\t\t\t\t\tdie(\"--stdin given twice?\");\n+\t\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\t\tcontinue;\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\tcontinue;\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--end-of-options\")) {\ndiff --git a/revision.h b/revision.h\nindex 17698cb51ac..79a3421cd1f 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -86,6 +86,11 @@ struct rev_cmdline_info {\n struct oidset;\n struct topo_walk_info;\n \n+enum rev_info_stdin {\n+\tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n+\tREV_INFO_STDIN_IGNORE,\n+};\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -114,9 +119,22 @@ struct rev_info {\n \tint rev_input_given;\n \n \t/*\n-\t * Whether we read from stdin due to the --stdin option.\n+\t * How should we handle seeing --stdin?\n+\t *\n+\t * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll\n+\t * attempt to read it if we see the --stdin option.\n+\t *\n+\t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n+\t * option.\n+\t */\n+\tenum rev_info_stdin stdin_handling;\n+\n+\t/*\n+\t * Did we read from stdin due to stdin_handling ==\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n+\t * option?\n \t */\n-\tint read_from_stdin;\n+\tint consumed_stdin;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n@@ -216,7 +234,6 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1;\n-\tunsigned int\tdisable_stdin:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n \t\t\ttrack_first_time:1,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..4e73bd79d69 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,\n \tlog_tree_opt.abbrev = 0;\n \tlog_tree_opt.diff = 1;\n \tlog_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\tlog_tree_opt.disable_stdin = 1;\n+\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tlog_tree_opt.no_commit_id = 1;\n \tlog_tree_opt.diffopt.file = fopen(buf.buf, \"w\");\n \tlog_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;\n@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,\n \t\t\tlog_tree_opt.diff = 1;\n \t\t\tlog_tree_opt.diffopt.output_format =\n \t\t\t\tDIFF_FORMAT_DIFFSTAT;\n-\t\t\tlog_tree_opt.disable_stdin = 1;\n+\t\t\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \n \t\t\tif (read_oneliner(&buf, rebase_path_orig_head(), 0) &&\n \t\t\t !get_oid(buf.buf, &orig) &&\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"428045","messageId":"patch-4.4-34750ab81cf-20210621T150651Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com","subject":"[PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-06-21T15:10:16Z","receivedAt":"2021-06-21T15:10:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue the work started in the preceding commit of porting\npack-objects.c over to the new handle_stdin_line callback pattern. The\ncommon case for the users of this API is to do some of their own\nparsing or munging, and then have handle_revision_arg() handle the\nrest.\n\nThe existing users of the --stdin parsing always wanted a flag of \"0\"\nto be passed to handle_revision_arg(), but pack-objects.c wants to set\ncustom flags. Let's support this common case by having a\n\"revarg_flags\" member in the \"rev_info\" struct.\n\nThis allows us to return REV_INFO_STDIN_LINE_PROCESS in the new\nget_object_list_handle_stdin_line() instead of\nREV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now\npass down the right flag for us.\n\nI considered making the \"--not\" parsing be another flag handled by the\nrevision.c API itself, but since there's only one caller who wants\nthis, and the \"write_bitmap_index = 0\" case is more specific than\nmarking things UNINTERESTING I think it's better to handle it with a\nmore general mechanism.\n\nThese changes means that we can make the handle_revision_arg()\nfunction static. Now that the only external user of the API has been\nmigrated over to the callback mechanism nothing external to revision.c\nneeds to call handle_revision_arg() anymore.\n\nThat handle_revision_arg() function was made public in a combination\nof 5d6f0935e6d (revision.c: allow injecting revision parameters after\nsetup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run\nrev-list equivalent internally., 2006-09-04).\n\nThis change leaves the briefly-used in preceding commits\n\"void *stdin_line_priv\" without any in-tree user, as\nbuiltin/pack-objects.c could be ported over to our new \"revarg_flags\"\ncommon case.\n\nI'm leaving that \"void *stdin_line_priv\" in place anyway for two\nreasons:\n\n 1. It's a common pattern to allow such a \"void *\" to be used for\n callback data, so we might as well follow that pattern here in\n anticipation of a future in-tree user.\n\n 2. I have patches for such an in-tree user already in a series\n that'll be submitted after this one.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 35 +++++++++++++++--------------------\n revision.c | 4 ++--\n revision.h | 6 ++++--\n 3 files changed, 21 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f8cae6c305a..50f703e1795 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3758,28 +3758,25 @@ static void mark_bitmap_preferred_tips(void)\n static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n {\n-\tint *flags = stdin_line_priv;\n \tchar *line = line_sb->buf;\n \n-\tif (*line == '-') {\n-\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t*flags ^= UNINTERESTING;\n-\t\t\twrite_bitmap_index = 0;\n-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n-\t\t}\n-\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\tstruct object_id oid;\n-\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\tuse_bitmap_index = 0;\n-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n-\t\t}\n+\tif (*line != '-')\n+\t\treturn REV_INFO_STDIN_LINE_PROCESS;\n+\n+\tif (!strcmp(line, \"--not\")) {\n+\t\trevs->revarg_flags ^= UNINTERESTING;\n+\t\twrite_bitmap_index = 0;\n+\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t} else if (starts_with(line, \"--shallow \")) {\n+\t\tstruct object_id oid;\n+\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\tregister_shallow(the_repository, &oid);\n+\t\tuse_bitmap_index = 0;\n+\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t} else {\n \t\tdie(_(\"not a rev '%s'\"), line);\n \t}\n-\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\treturn REV_INFO_STDIN_LINE_CONTINUE;\n }\n \n static void get_object_list(int ac, const char **av)\n@@ -3788,7 +3785,6 @@ static void get_object_list(int ac, const char **av)\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tint flags = 0;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3798,7 +3794,6 @@ static void get_object_list(int ac, const char **av)\n \n \trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n \trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n-\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\ndiff --git a/revision.c b/revision.c\nindex f55b3a1caf4..17121f40c0b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \treturn 0;\n }\n \n-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tint ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);\n \tif (!ret)\n@@ -2139,7 +2139,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\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\tif (handle_revision_arg(sb.buf, revs, revs->revarg_flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\ndiff --git a/revision.h b/revision.h\nindex ff7994bb13d..2b96c05ced8 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -156,6 +156,9 @@ struct rev_info {\n \t * revision.c's normal processing of the line (after\n \t * possibly munging the provided strbuf).\n \t *\n+\t * Change \"revarg_flags\" to affect the subsequent handling\n+\t * in handle_revision_arg()\n+\t *\n \t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n \t * line is fully processed, moving onto the next line (if\n \t * any)\n@@ -164,6 +167,7 @@ struct rev_info {\n \t * around.\n \t */\n \trev_info_stdin_line_func handle_stdin_line;\n+\tint revarg_flags;\n \tvoid *stdin_line_priv;\n \n \t/* topo-sort */\n@@ -421,8 +425,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[]);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n-int handle_revision_arg(const char *arg, struct rev_info *revs,\n-\t\t\tint flags, unsigned revarg_opt);\n \n /**\n * Reset the flags used by the revision walking api. You can use this to do\n-- \n2.32.0.599.g3967b4fa4ac\n\n"},{"id":"429509","messageId":"xmqqwnq0a24b.fsf@gitster.g","threadId":"55881","inReplyTo":"patch-2.4-6f69644b808-20210621T150651Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/4] revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-08T22:10:12Z","receivedAt":"2021-07-08T22:10:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the two \"disable_stdin\" and \"read_from_stdin\" flags to an enum,\n> in preparation for a subsequent commit adding more flags.\n>\n> The interaction between these is more subtle than they might appear at\n> first sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\n> of \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\n> option.\n>\n> The \"read\" is intended to understood as \"I read it\", not \"you should\n\n\"I've read it already\" would have been easier to understand.\n\n> \t\t\tif (!strcmp(arg, \"--stdin\")) {\n> -\t\t\t\tif (revs->disable_stdin) {\n> +\t\t\t\tswitch (revs->stdin_handling) {\n> +\t\t\t\tcase REV_INFO_STDIN_IGNORE:\n> \t\t\t\t\targv[left++] = arg;\n> \t\t\t\t\tcontinue;\n> +\t\t\t\tcase REV_INFO_STDIN_CONSUME_ON_OPTION:\n> +\t\t\t\t\tif (revs->consumed_stdin++)\n> +\t\t\t\t\t\tdie(\"--stdin given twice?\");\n> +\t\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n> +\t\t\t\t\tcontinue;\n> \t\t\t\t}\n\nMore importantly, consumed_stdin will start at zero and if it is\nfound to be non-zero here, it is an immediate error, hence ...\n\n> +\t/*\n> +\t * Did we read from stdin due to stdin_handling ==\n> +\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n> +\t * option?\n> \t */\n> -\tint read_from_stdin;\n> +\tint consumed_stdin;\n\n... it does not have to be a full integer.\nJust a single-bit would do.\n\nBut none of the above is grave enough to require an update.\n"},{"id":"429510","messageId":"xmqqmtqwa1l4.fsf@gitster.g","threadId":"55881","inReplyTo":"patch-4.4-34750ab81cf-20210621T150651Z-avarab@gmail.com","subject":"Re: [PATCH v3 4/4] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-08T22:21:43Z","receivedAt":"2021-07-08T22:21:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> @@ -3758,28 +3758,25 @@ static void mark_bitmap_preferred_tips(void)\n> static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n> \tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n> {\n> -\tint *flags = stdin_line_priv;\n> \tchar *line = line_sb->buf;\n> \n> -\tif (*line == '-') {\n> -\t\tif (!strcmp(line, \"--not\")) {\n> -\t\t\t*flags ^= UNINTERESTING;\n> -\t\t\twrite_bitmap_index = 0;\n> -\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n> -\t\t}\n> -\t\tif (starts_with(line, \"--shallow \")) {\n> -\t\t\tstruct object_id oid;\n> -\t\t\tif (get_oid_hex(line + 10, &oid))\n> -\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n> -\t\t\tregister_shallow(the_repository, &oid);\n> -\t\t\tuse_bitmap_index = 0;\n> -\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n> -\t\t}\n> +\tif (*line != '-')\n> +\t\treturn REV_INFO_STDIN_LINE_PROCESS;\n> +\n> +\tif (!strcmp(line, \"--not\")) {\n> +\t\trevs->revarg_flags ^= UNINTERESTING;\n> +\t\twrite_bitmap_index = 0;\n> +\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n> +\t} else if (starts_with(line, \"--shallow \")) {\n> +\t\tstruct object_id oid;\n> +\t\tif (get_oid_hex(line + 10, &oid))\n> +\t\t\tdie(\"not an object name '%s'\", line + 10);\n> +\t\tregister_shallow(the_repository, &oid);\n> +\t\tuse_bitmap_index = 0;\n> +\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n> +\t} else {\n> \t\tdie(_(\"not a rev '%s'\"), line);\n> \t}\n> -\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n> -\t\t\tdie(_(\"bad revision '%s'\"), line);\n> -\treturn REV_INFO_STDIN_LINE_CONTINUE;\n> }\n\nNow, this does make things slightly more modular (specifically, it\nno longer is necessary for handle_revision_arg() to be visible\noutside revision.c).\n\nHaving said that, it is not immediately obvious if the amount of\ncode churn by 3/4 and 4/4 is justified. This still looks more like\n\"we can add a new API, so we added it\" than \"we needed a new API for\nsuch and such reasons---now we can do this more easily then before\".\n\nIt's not wrong per-se, but I am not all that impressed. Sorry.\n"},{"id":"429511","messageId":"xmqqfswoa1ku.fsf@gitster.g","threadId":"55881","inReplyTo":"patch-3.4-943b1b4c12a-20210621T150651Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/4] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-08T22:21:53Z","receivedAt":"2021-07-08T22:21:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Extend the rev_info stdin parsing API to support hooking into its\n> read_revisions_from_stdin() function, and change the custom stdin\n> parsing in pack-objects.c to use it.\n\nThis step, while it might not be introducing new bugs, does not look\nanything more than just adding yet another new API for the sake of\nadding one (especially, the number of lines added vs removed does\nnot look so impressive).\n\nPerhaps we'll see why it makes sense in the next step?\n\n> builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------\n> revision.c | 22 +++++++++++++++\n> revision.h | 30 ++++++++++++++++++++\n> 3 files changed, 82 insertions(+), 33 deletions(-)\n"},{"id":"429533","messageId":"patch-1.5-8b2872227c5-20210709T105851Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","subject":"[PATCH v4 1/5] upload-pack: run is_repository_shallow() before setup_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T11:06:12Z","receivedAt":"2021-07-09T11:06:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the is_repository_shallow() added in b790e0f67cd (upload-pack:\nsend shallow info over stdin to pack-objects, 2014-03-11) to above\nsetup_revisions().\n\nRunning is_repository_shallow() before setup_revisions() doesn't\nmatter now, but in subsequent commits we'll make the code that\nfollowed setup_revisions() happen inside a callback in that\nfunction. This isolated change documents that re-arranging this part\nof the code is OK in isolation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex df49f656b96..3639d0379ed 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3774,11 +3774,12 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n+\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429534","messageId":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com","subject":"[PATCH v4 0/5] revision.[ch]: add --stdin parsing API, use in pack-objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T11:06:11Z","receivedAt":"2021-07-09T11:06:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A re-roll of v3[1], this v4 fixes specific issues noted there, but\nmore importantly I've tried to address Junio's comments to the effect\nof \"what's the point of this churny thing?\"\" in [2].\n\nAs the new 3/5 explains what I really wanted this for is a new feature\nfor \"git bundle create\" that I have working locally, which allows it\nto do:\n\n $ printf \"e83c5163316f89bfbde7\\trefs/heads/first-git-dot-git-commit\\n\" |\n git bundle create initial.bundle --stdin\n\nAs well as things like:\n\n $ git for-each-ref 'refs/remotes/origin' |\n sed 's!\\trefs/remotes/origin/!\\trefs/heads/!' |\n git bundle create origin.bundle --stdin\n\nAs elaborated on in 3/5 for that it's much better to have an API like\nthis, but since those changes also needed what I've got in\nab/bundle-updates and ab/bundle-doc (well, not that one really, but\nthe merge conflict would be painful otherwise) I split this series off\nso I wouldn't end up with another 10-20 patch monster.\n\nJunio: I hope that'll convence you to accept this, I can also submit\nthat \"monster\", but it'll be harder to read & review sinc so much of\nit (e.g. this series, and doc refactoring) will be needed just to get\nto the point of implementing that feature.\n\n1. https://lore.kernel.org/git/cover-0.4-00000000000-20210621T150651Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/xmqqfswoa1ku.fsf@gitster.g/\n\nÆvar Arnfjörð Bjarmason (5):\n upload-pack: run is_repository_shallow() before setup_revisions()\n revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"\n revision.[ch]: add a \"handle_stdin_line\" API\n pack-objects.c: do stdin parsing via revision.c's API\n pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n\n builtin/am.c | 4 +--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------\n builtin/rev-list.c | 2 +-\n revision.c | 39 ++++++++++++++++++++++-----\n revision.h | 59 ++++++++++++++++++++++++++++++++++++----\n sequencer.c | 4 +--\n 8 files changed, 120 insertions(+), 53 deletions(-)\n\nRange-diff against v3:\n1: dd2efd1cfb9 < -: ----------- pack-objects tests: cover blindspots in stdin handling\n2: a9702132385 < -: ----------- pack-objects: fix segfault in --stdin-packs option\n3: 3840ac28e8d = 1: 8b2872227c5 upload-pack: run is_repository_shallow() before setup_revisions()\n4: 6f69644b808 ! 2: 630feb06650 revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"\n @@ Commit message\n of \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\n option.\n \n - The \"read\" is intended to understood as \"I read it\", not \"you should\n - read it\". Let's avoid this confusion by using \"consume\" and \"consumed\"\n - instead, i.e. a word whose present and past tense isn't the same.\n + The \"read\" is intended to understood as \"I've read it already\", not\n + \"you should read it\". Let's avoid this confusion by using \"consume\"\n + and \"consumed\" instead, i.e. a word whose present and past tense isn't\n + the same.\n \n See 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03)\n where we added the \"disable_stdin\" flag, and a12cbe23ef7 (rev-list:\n @@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *re\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n +\t\t\t\tcase REV_INFO_STDIN_CONSUME_ON_OPTION:\n -+\t\t\t\t\tif (revs->consumed_stdin++)\n ++\t\t\t\t\tif (revs->consumed_stdin)\n +\t\t\t\t\t\tdie(\"--stdin given twice?\");\n +\t\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n ++\t\t\t\t\trevs->consumed_stdin = 1;\n +\t\t\t\t\tcontinue;\n \t\t\t\t}\n -\t\t\t\tif (revs->read_from_stdin++)\n @@ revision.h: struct rev_cmdline_info {\n struct topo_walk_info;\n \n +enum rev_info_stdin {\n -+\tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n ++\tREV_INFO_STDIN_CONSUME_ON_OPTION,\n +\tREV_INFO_STDIN_IGNORE,\n +};\n +\n @@ revision.h: struct rev_info {\n +\t * option?\n \t */\n -\tint read_from_stdin;\n -+\tint consumed_stdin;\n ++\tint consumed_stdin:1;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n-: ----------- > 3: 60ef49df3b8 revision.[ch]: add a \"handle_stdin_line\" API\n5: 943b1b4c12a ! 4: ebffeb5891f pack-objects.c: do stdin parsing via revision.c's API\n @@ Metadata\n ## Commit message ##\n pack-objects.c: do stdin parsing via revision.c's API\n \n - Extend the rev_info stdin parsing API to support hooking into its\n - read_revisions_from_stdin() function, and change the custom stdin\n - parsing in pack-objects.c to use it.\n + Use the new \"handle_stdin_line\" API in revision.c to parse stdin in\n + pack-objects.c, instead of using custom pack-objects.c-specific code\n + to do so.\n +\n + This means that we can remove the \"if (len && line[len - 1] == '\\n')\"\n + check, it's now redundant to using strbuf_getline(), and we get to\n + skip the whole \"warn_on_object_refname_ambiguity\" dance. The\n + read_revisions_from_stdin() function in revision.c we're now using\n + does it for us.\n \n The pack-objects.c code being refactored away here was first added in\n Linus's c323ac7d9c5 (git-pack-objects: create a packed object\n @@ Commit message\n and it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:\n read --stdin with strbuf_getline(), 2015-10-28).\n \n - Let's do the same here, as in 6e8d46f9d4b we can remove the \"if (len\n - && line[len - 1] == '\\n')\" check, it's now redundant to using\n - strbuf_getline(), and we get to skip the whole\n - \"warn_on_object_refname_ambiguity\" dance. The\n - read_revisions_from_stdin() function in revision.c we're now using\n - does it for us.\n -\n - For now there isn't all that much point in this whole exercises. We\n - just end up calling setup_revisions() to loop over stdin for us, but\n - the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS, we\n + For now we've just made setup_revisions() loop over stdin for us, but\n + the callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS. We\n still need to call handle_revision_arg() ourselves because we'd like\n to call it with different flags.\n \n @@ builtin/pack-objects.c: static void get_object_list(int ac, const char **av)\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \n -\n - ## revision.c ##\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\t\tbreak;\n -+\n -+\t\tif (revs->handle_stdin_line) {\n -+\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n -+\t\t\t\trevs, &sb, revs->stdin_line_priv);\n -+\n -+\t\t\tswitch (ret) {\n -+\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n -+\t\t\t\tbreak;\n -+\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n -+\t\t\t\tcontinue;\n -+\t\t\t}\n -+\t\t}\n -+\n - \t\tif (sb.buf[0] == '-') {\n - \t\t\tif (len == 2 && sb.buf[1] == '-') {\n - \t\t\t\tseen_dashdash = 1;\n -@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n - \n - \t\t\tif (!strcmp(arg, \"--stdin\")) {\n - \t\t\t\tswitch (revs->stdin_handling) {\n -+\t\t\t\tcase REV_INFO_STDIN_ALWAYS_READ:\n - \t\t\t\tcase REV_INFO_STDIN_IGNORE:\n - \t\t\t\t\targv[left++] = arg;\n - \t\t\t\t\tcontinue;\n -@@ revision.c: int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n - \t\t}\n - \t}\n - \n -+\t/*\n -+\t * We're asked to ALWAYS_READ from stdin, but no --stdin\n -+\t * option (or \"consumed_stdin\" would be set).\n -+\t */\n -+\tif (!revs->consumed_stdin &&\n -+\t revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)\n -+\t\tread_revisions_from_stdin(revs, &prune_data);\n -+\n - \tif (prune_data.nr) {\n - \t\t/*\n - \t\t * If we need to introduce the magic \"a lone ':' means no\n -\n - ## revision.h ##\n -@@ revision.h: struct topo_walk_info;\n - enum rev_info_stdin {\n - \tREV_INFO_STDIN_CONSUME_ON_OPTION = 0,\n - \tREV_INFO_STDIN_IGNORE,\n -+\tREV_INFO_STDIN_ALWAYS_READ,\n - };\n - \n -+enum rev_info_stdin_line {\n -+\tREV_INFO_STDIN_LINE_PROCESS,\n -+\tREV_INFO_STDIN_LINE_CONTINUE,\n -+};\n -+\n -+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(\n -+\tstruct rev_info *revs, struct strbuf *line, void *stdin_line_priv);\n -+\n - struct rev_info {\n - \t/* Starting list */\n - \tstruct commit_list *commits;\n -@@ revision.h: struct rev_info {\n - \t *\n - \t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n - \t * option.\n -+\t *\n -+\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n -+\t * on stdin to be read, even if no --stdin option is provided.\n - \t */\n - \tenum rev_info_stdin stdin_handling;\n - \n -@@ revision.h: struct rev_info {\n - \t */\n - \tint consumed_stdin;\n - \n -+\t/*\n -+\t * When reading from stdin (see \"stdin_handling\" above) define\n -+\t * a handle_stdin_line function to consume the lines.\n -+\t *\n -+\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n -+\t * revision.c's normal processing of the line (after\n -+\t * possibly munging the provided strbuf).\n -+\t *\n -+\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n -+\t * line is fully processed, moving onto the next line (if\n -+\t * any)\n -+\t *\n -+\t * Use the \"stdin_line_priv\" to optionally pass your own data\n -+\t * around.\n -+\t */\n -+\trev_info_stdin_line_func handle_stdin_line;\n -+\tvoid *stdin_line_priv;\n -+\n - \t/* topo-sort */\n - \tenum rev_sort_order sort_order;\n - \n6: 34750ab81cf ! 5: 0c049aee67c pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n @@ Commit message\n anticipation of a future in-tree user.\n \n 2. I have patches for such an in-tree user already in a series\n - that'll be submitted after this one.\n + that'll be submitted after this one. See the reference to \"git\n + bundle create --stdin\" in the commit that added the \"handle_stdin_line\"\n + API.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429535","messageId":"patch-2.5-630feb06650-20210709T105851Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","subject":"[PATCH v4 2/5] revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T11:06:13Z","receivedAt":"2021-07-09T11:06:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the two \"disable_stdin\" and \"read_from_stdin\" flags to an enum,\nin preparation for a subsequent commit adding more flags.\n\nThe interaction between these is more subtle than they might appear at\nfirst sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\nof \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\noption.\n\nThe \"read\" is intended to understood as \"I've read it already\", not\n\"you should read it\". Let's avoid this confusion by using \"consume\"\nand \"consumed\" instead, i.e. a word whose present and past tense isn't\nthe same.\n\nSee 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03)\nwhere we added the \"disable_stdin\" flag, and a12cbe23ef7 (rev-list:\nmake empty --stdin not an error, 2018-08-22) for the addition of the\n\"read_from_stdin\" flag.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 4 ++--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/rev-list.c | 2 +-\n revision.c | 13 ++++++++-----\n revision.h | 23 ++++++++++++++++++++---\n sequencer.c | 4 ++--\n 7 files changed, 35 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b2d886c81b..3a6c8455b47 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n \trev_info.abbrev = 0;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.show_root_diff = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.no_commit_id = 1;\n@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)\n \tfp = xfopen(am_path(state, \"patch\"), \"w\");\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.no_commit_id = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.diffopt.use_color = 0;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..c9f66c58c46 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\targv[argc - 1] = \"--\";\n \t}\n \n-\trevs.disable_stdin = 1;\n+\trevs.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tsetup_revisions(argc, argv, &revs, NULL);\n \tif (!revs.pending.nr && is_bare_repository()) {\n \t\tstruct commit *head_commit;\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex f33d30d57bf..fc548ebe166 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"index file corrupt\"));\n \topt->abbrev = 0;\n \topt->diff = 1;\n-\topt->disable_stdin = 1;\n+\topt->stdin_handling = REV_INFO_STDIN_IGNORE;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.tweak = diff_tree_tweak_rev;\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 7677b1af5a4..524632ba328 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -651,7 +651,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr) &&\n-\t !revs.rev_input_given && !revs.read_from_stdin) ||\n+\t !revs.rev_input_given && !revs.consumed_stdin) ||\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \ndiff --git a/revision.c b/revision.c\nindex cddd0542a65..f30b9d40af4 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2742,14 +2742,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n-\t\t\t\tif (revs->disable_stdin) {\n+\t\t\t\tswitch (revs->stdin_handling) {\n+\t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n+\t\t\t\tcase REV_INFO_STDIN_CONSUME_ON_OPTION:\n+\t\t\t\t\tif (revs->consumed_stdin)\n+\t\t\t\t\t\tdie(\"--stdin given twice?\");\n+\t\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\t\trevs->consumed_stdin = 1;\n+\t\t\t\t\tcontinue;\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\tcontinue;\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--end-of-options\")) {\ndiff --git a/revision.h b/revision.h\nindex 5c5510d4220..a942b1250ad 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -86,6 +86,11 @@ struct rev_cmdline_info {\n struct oidset;\n struct topo_walk_info;\n \n+enum rev_info_stdin {\n+\tREV_INFO_STDIN_CONSUME_ON_OPTION,\n+\tREV_INFO_STDIN_IGNORE,\n+};\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -114,9 +119,22 @@ struct rev_info {\n \tint rev_input_given;\n \n \t/*\n-\t * Whether we read from stdin due to the --stdin option.\n+\t * How should we handle seeing --stdin?\n+\t *\n+\t * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll\n+\t * attempt to read it if we see the --stdin option.\n+\t *\n+\t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n+\t * option.\n+\t */\n+\tenum rev_info_stdin stdin_handling;\n+\n+\t/*\n+\t * Did we read from stdin due to stdin_handling ==\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n+\t * option?\n \t */\n-\tint read_from_stdin;\n+\tint consumed_stdin:1;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n@@ -216,7 +234,6 @@ struct rev_info {\n \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1;\n-\tunsigned int\tdisable_stdin:1;\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n \t\t\ttrack_first_time:1,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..4e73bd79d69 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,\n \tlog_tree_opt.abbrev = 0;\n \tlog_tree_opt.diff = 1;\n \tlog_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\tlog_tree_opt.disable_stdin = 1;\n+\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tlog_tree_opt.no_commit_id = 1;\n \tlog_tree_opt.diffopt.file = fopen(buf.buf, \"w\");\n \tlog_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;\n@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,\n \t\t\tlog_tree_opt.diff = 1;\n \t\t\tlog_tree_opt.diffopt.output_format =\n \t\t\t\tDIFF_FORMAT_DIFFSTAT;\n-\t\t\tlog_tree_opt.disable_stdin = 1;\n+\t\t\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \n \t\t\tif (read_oneliner(&buf, rebase_path_orig_head(), 0) &&\n \t\t\t !get_oid(buf.buf, &orig) &&\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429536","messageId":"patch-4.5-ebffeb5891f-20210709T105851Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","subject":"[PATCH v4 4/5] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T11:06:15Z","receivedAt":"2021-07-09T11:06:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the new \"handle_stdin_line\" API in revision.c to parse stdin in\npack-objects.c, instead of using custom pack-objects.c-specific code\nto do so.\n\nThis means that we can remove the \"if (len && line[len - 1] == '\\n')\"\ncheck, it's now redundant to using strbuf_getline(), and we get to\nskip the whole \"warn_on_object_refname_ambiguity\" dance. The\nread_revisions_from_stdin() function in revision.c we're now using\ndoes it for us.\n\nThe pack-objects.c code being refactored away here was first added in\nLinus's c323ac7d9c5 (git-pack-objects: create a packed object\nrepresentation., 2005-06-25).\n\nLater on rev-list started doing similar parsing in 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05).\nThat code was promoted to a more general API in 1fc561d169a (Move\nread_revisions_from_stdin from builtin-rev-list.c to revision.c,\n2008-07-05).\n\nSince then the API in revision.c has received improvements that have\nbeen missed here. E.g. the arbitrary limit of 1000 bytes was removed\nin 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),\nand it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:\nread --stdin with strbuf_getline(), 2015-10-28).\n\nFor now we've just made setup_revisions() loop over stdin for us, but\nthe callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS. We\nstill need to call handle_revision_arg() ourselves because we'd like\nto call it with different flags.\n\nThis very light use of the API will be further refined in a subsequent\ncommit, for now we're just doing the bare minimum to move this\nexisting code over to the new callback pattern without any functional\nchanges, and making it as friendly to \"git show -w\" and \"the\n--color-moved-ws=allow-indentation-change\" mode as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------\n 1 file changed, 30 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 3639d0379ed..595adc89c12 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3762,15 +3762,40 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n+static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n+\tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n+{\n+\tint *flags = stdin_line_priv;\n+\tchar *line = line_sb->buf;\n+\n+\tif (*line == '-') {\n+\t\tif (!strcmp(line, \"--not\")) {\n+\t\t\t*flags ^= UNINTERESTING;\n+\t\t\twrite_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\tstruct object_id oid;\n+\t\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\t\tregister_shallow(the_repository, &oid);\n+\t\t\tuse_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tdie(_(\"not a rev '%s'\"), line);\n+\t}\n+\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(_(\"bad revision '%s'\"), line);\n+\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tchar line[1000];\n \tint flags = 0;\n-\tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3778,39 +3803,11 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n+\trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n+\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len - 1] == '\\n')\n-\t\t\tline[--len] = 0;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (*line == '-') {\n-\t\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t\tflags ^= UNINTERESTING;\n-\t\t\t\twrite_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\t\tuse_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tdie(_(\"not a rev '%s'\"), line);\n-\t\t}\n-\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\t}\n-\n-\twarn_on_object_refname_ambiguity = save_warning;\n-\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429537","messageId":"patch-5.5-0c049aee67c-20210709T105851Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","subject":"[PATCH v4 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T11:06:16Z","receivedAt":"2021-07-09T11:06:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue the work started in the preceding commit of porting\npack-objects.c over to the new handle_stdin_line callback pattern. The\ncommon case for the users of this API is to do some of their own\nparsing or munging, and then have handle_revision_arg() handle the\nrest.\n\nThe existing users of the --stdin parsing always wanted a flag of \"0\"\nto be passed to handle_revision_arg(), but pack-objects.c wants to set\ncustom flags. Let's support this common case by having a\n\"revarg_flags\" member in the \"rev_info\" struct.\n\nThis allows us to return REV_INFO_STDIN_LINE_PROCESS in the new\nget_object_list_handle_stdin_line() instead of\nREV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now\npass down the right flag for us.\n\nI considered making the \"--not\" parsing be another flag handled by the\nrevision.c API itself, but since there's only one caller who wants\nthis, and the \"write_bitmap_index = 0\" case is more specific than\nmarking things UNINTERESTING I think it's better to handle it with a\nmore general mechanism.\n\nThese changes means that we can make the handle_revision_arg()\nfunction static. Now that the only external user of the API has been\nmigrated over to the callback mechanism nothing external to revision.c\nneeds to call handle_revision_arg() anymore.\n\nThat handle_revision_arg() function was made public in a combination\nof 5d6f0935e6d (revision.c: allow injecting revision parameters after\nsetup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run\nrev-list equivalent internally., 2006-09-04).\n\nThis change leaves the briefly-used in preceding commits\n\"void *stdin_line_priv\" without any in-tree user, as\nbuiltin/pack-objects.c could be ported over to our new \"revarg_flags\"\ncommon case.\n\nI'm leaving that \"void *stdin_line_priv\" in place anyway for two\nreasons:\n\n 1. It's a common pattern to allow such a \"void *\" to be used for\n callback data, so we might as well follow that pattern here in\n anticipation of a future in-tree user.\n\n 2. I have patches for such an in-tree user already in a series\n that'll be submitted after this one. See the reference to \"git\n bundle create --stdin\" in the commit that added the \"handle_stdin_line\"\n API.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 35 +++++++++++++++--------------------\n revision.c | 4 ++--\n revision.h | 6 ++++--\n 3 files changed, 21 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 595adc89c12..9f1d7efeb61 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3765,28 +3765,25 @@ static void mark_bitmap_preferred_tips(void)\n static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n {\n-\tint *flags = stdin_line_priv;\n \tchar *line = line_sb->buf;\n \n-\tif (*line == '-') {\n-\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t*flags ^= UNINTERESTING;\n-\t\t\twrite_bitmap_index = 0;\n-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n-\t\t}\n-\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\tstruct object_id oid;\n-\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\tuse_bitmap_index = 0;\n-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n-\t\t}\n+\tif (*line != '-')\n+\t\treturn REV_INFO_STDIN_LINE_PROCESS;\n+\n+\tif (!strcmp(line, \"--not\")) {\n+\t\trevs->revarg_flags ^= UNINTERESTING;\n+\t\twrite_bitmap_index = 0;\n+\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t} else if (starts_with(line, \"--shallow \")) {\n+\t\tstruct object_id oid;\n+\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\tregister_shallow(the_repository, &oid);\n+\t\tuse_bitmap_index = 0;\n+\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t} else {\n \t\tdie(_(\"not a rev '%s'\"), line);\n \t}\n-\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\treturn REV_INFO_STDIN_LINE_CONTINUE;\n }\n \n static void get_object_list(int ac, const char **av)\n@@ -3795,7 +3792,6 @@ static void get_object_list(int ac, const char **av)\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tint flags = 0;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3805,7 +3801,6 @@ static void get_object_list(int ac, const char **av)\n \n \trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n \trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n-\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\ndiff --git a/revision.c b/revision.c\nindex 557b7f113a9..f910b7882ae 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2090,7 +2090,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \treturn 0;\n }\n \n-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tint ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);\n \tif (!ret)\n@@ -2140,7 +2140,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\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\tif (handle_revision_arg(sb.buf, revs, revs->revarg_flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\ndiff --git a/revision.h b/revision.h\nindex 52241c84c5b..3da56333960 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -156,6 +156,9 @@ struct rev_info {\n \t * revision.c's normal processing of the line (after\n \t * possibly munging the provided strbuf).\n \t *\n+\t * Change \"revarg_flags\" to affect the subsequent handling\n+\t * in handle_revision_arg()\n+\t *\n \t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n \t * line is fully processed, moving onto the next line (if\n \t * any)\n@@ -164,6 +167,7 @@ struct rev_info {\n \t * around.\n \t */\n \trev_info_stdin_line_func handle_stdin_line;\n+\tint revarg_flags;\n \tvoid *stdin_line_priv;\n \n \t/* topo-sort */\n@@ -422,8 +426,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[]);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n-int handle_revision_arg(const char *arg, struct rev_info *revs,\n-\t\t\tint flags, unsigned revarg_opt);\n \n /**\n * Reset the flags used by the revision walking api. You can use this to do\n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429538","messageId":"patch-3.5-60ef49df3b8-20210709T105851Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","subject":"[PATCH v4 3/5] revision.[ch]: add a \"handle_stdin_line\" API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-09T11:06:14Z","receivedAt":"2021-07-09T11:06:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the rev_info stdin parsing API to support hooking into its\nread_revisions_from_stdin() function, in the next commit we'll change\nthe the custom stdin parsing in pack-objects.c to use it..\n\nFor that use-case adding API is barely justified. We'll be able to\nmake the handle_revision_arg() static in exchange for it, and we'll\navoid the duplicate dance around setting \"save_warning\" and\n\"warn_on_object_refname_ambiguity\", but we could just continue to do\nthat ourselves in builtin/pack-objects.c\n\nThe real reason to add this is for a change not part of this\nseries. We'll soon teach \"git bundle create\" to accept\nrevision/refname pairs on stdin, and thus do away with the limitation\nof it being impossible to create bundles with ref tips that don't\ncorrespond to the state of the current repository. I.e. this will\nwork:\n\n $ printf \"e83c5163316f89bfbde7\\trefs/heads/first-git-dot-git-commit\\n\" |\n git bundle create initial.bundle --stdin\n\nAs well as things like:\n\n $ git for-each-ref 'refs/remotes/origin' |\n sed 's!\\trefs/remotes/origin/!\\trefs/heads/!' |\n git bundle create origin.bundle --stdin\n\nIn order to do that we'll need to modify the lines we consume on stdin\nrevision.c (which bundle.c uses already), and be assured that that\nstripping extra bundle-specific data from them is the only change in\nbehavior.\n\nThat change will be much more complex if bundle.c needs to start doing\nits own stdin parsing again outside of revision.c, it was recently\nconverted to use revision.c's parsing in 5bb0fd2cab5 (bundle:\narguments can be read from stdin, 2021-01-11)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n revision.c | 22 ++++++++++++++++++++++\n revision.h | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex f30b9d40af4..557b7f113a9 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2120,6 +2120,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\tint len = sb.len;\n \t\tif (!len)\n \t\t\tbreak;\n+\n+\t\tif (revs->handle_stdin_line) {\n+\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n+\t\t\t\trevs, &sb, revs->stdin_line_priv);\n+\n+\t\t\tswitch (ret) {\n+\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (sb.buf[0] == '-') {\n \t\t\tif (len == 2 && sb.buf[1] == '-') {\n \t\t\t\tseen_dashdash = 1;\n@@ -2743,6 +2756,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tswitch (revs->stdin_handling) {\n+\t\t\t\tcase REV_INFO_STDIN_ALWAYS_READ:\n \t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n@@ -2791,6 +2805,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t}\n \t}\n \n+\t/*\n+\t * We're asked to ALWAYS_READ from stdin, but no --stdin\n+\t * option (or \"consumed_stdin\" would be set).\n+\t */\n+\tif (!revs->consumed_stdin &&\n+\t revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)\n+\t\tread_revisions_from_stdin(revs, &prune_data);\n+\n \tif (prune_data.nr) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\ndiff --git a/revision.h b/revision.h\nindex a942b1250ad..52241c84c5b 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -89,8 +89,17 @@ struct topo_walk_info;\n enum rev_info_stdin {\n \tREV_INFO_STDIN_CONSUME_ON_OPTION,\n \tREV_INFO_STDIN_IGNORE,\n+\tREV_INFO_STDIN_ALWAYS_READ,\n };\n \n+enum rev_info_stdin_line {\n+\tREV_INFO_STDIN_LINE_PROCESS,\n+\tREV_INFO_STDIN_LINE_CONTINUE,\n+};\n+\n+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(\n+\tstruct rev_info *revs, struct strbuf *line, void *stdin_line_priv);\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -126,6 +135,9 @@ struct rev_info {\n \t *\n \t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n \t * option.\n+\t *\n+\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n+\t * on stdin to be read, even if no --stdin option is provided.\n \t */\n \tenum rev_info_stdin stdin_handling;\n \n@@ -136,6 +148,24 @@ struct rev_info {\n \t */\n \tint consumed_stdin:1;\n \n+\t/*\n+\t * When reading from stdin (see \"stdin_handling\" above) define\n+\t * a handle_stdin_line function to consume the lines.\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n+\t * revision.c's normal processing of the line (after\n+\t * possibly munging the provided strbuf).\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n+\t * line is fully processed, moving onto the next line (if\n+\t * any)\n+\t *\n+\t * Use the \"stdin_line_priv\" to optionally pass your own data\n+\t * around.\n+\t */\n+\trev_info_stdin_line_func handle_stdin_line;\n+\tvoid *stdin_line_priv;\n+\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-- \n2.32.0.636.g43e71d69cff\n\n"},{"id":"429581","messageId":"xmqqv95j5jjp.fsf@gitster.g","threadId":"55881","inReplyTo":"patch-2.5-630feb06650-20210709T105851Z-avarab@gmail.com","subject":"Re: [PATCH v4 2/5] revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-09T20:17:14Z","receivedAt":"2021-07-09T20:17:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +\t/*\n> +\t * Did we read from stdin due to stdin_handling ==\n> +\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n> +\t * option?\n> \t */\n> -\tint read_from_stdin;\n> +\tint consumed_stdin:1;\n\nWill make this into \"unsigned int\" while queuing, as 1-bit signed\nbitfield would not make much sense.\n\n"},{"id":"431178","messageId":"patch-1.5-b0c7ec31ca9-20210726T124425Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210726T124425Z-avarab@gmail.com","subject":"[PATCH v5 1/5] upload-pack: run is_repository_shallow() before setup_revisions()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-26T12:46:06Z","receivedAt":"2021-07-26T12:46:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Move the is_repository_shallow() added in b790e0f67cd (upload-pack:\nsend shallow info over stdin to pack-objects, 2014-03-11) to above\nsetup_revisions().\n\nRunning is_repository_shallow() before setup_revisions() doesn't\nmatter now, but in subsequent commits we'll make the code that\nfollowed setup_revisions() happen inside a callback in that\nfunction. This isolated change documents that re-arranging this part\nof the code is OK in isolation.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex de00adbb9e0..1fbaa34f91b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3757,11 +3757,12 @@ static void get_object_list(int ac, const char **av)\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n-\tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\tsetup_revisions(ac, av, &revs, &s_r_opt);\n+\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \n-- \n2.32.0.956.g6b0c84ceda8\n\n"},{"id":"431179","messageId":"cover-0.5-00000000000-20210726T124425Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com","subject":"[PATCH v5 0/5] add --stdin parsing API, use in pack-objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-26T12:46:05Z","receivedAt":"2021-07-26T12:46:52Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"A trivial re-roll of v4[1] on top of\ngitster/bc/rev-list-without-commit-line, which it had a conflict\nwith. Also includes the s/int/unsigned int/ fixup that Junio applied\non the v4 he picked up.\n\nhttps://lore.kernel.org/git/cover-0.5-00000000000-20210709T105850Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (5):\n upload-pack: run is_repository_shallow() before setup_revisions()\n revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"\n revision.[ch]: add a \"handle_stdin_line\" API\n pack-objects.c: do stdin parsing via revision.c's API\n pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n\n builtin/am.c | 4 +--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/pack-objects.c | 61 +++++++++++++++++++-----------------------\n builtin/rev-list.c | 2 +-\n revision.c | 39 ++++++++++++++++++++++-----\n revision.h | 60 +++++++++++++++++++++++++++++++++++++----\n sequencer.c | 4 +--\n 8 files changed, 121 insertions(+), 53 deletions(-)\n\nRange-diff against v4:\n1: f2cc9695306 = 1: b0c7ec31ca9 upload-pack: run is_repository_shallow() before setup_revisions()\n2: 50c3b81820b ! 2: 4e5e6620245 revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"\n @@ revision.h: struct rev_info {\n +\t * option?\n \t */\n -\tint read_from_stdin;\n -+\tint consumed_stdin:1;\n ++\tunsigned int consumed_stdin:1;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n @@ revision.h: struct rev_info {\n - \t\t\tdate_mode_explicit:1,\n \t\t\tpreserve_subject:1,\n - \t\t\tencode_email_headers:1;\n + \t\t\tencode_email_headers:1,\n + \t\t\tinclude_header:1;\n -\tunsigned int\tdisable_stdin:1;\n ++\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n \t\t\ttrack_first_time:1,\n3: 0339af8c39a ! 3: e3d24bd6e8a revision.[ch]: add a \"handle_stdin_line\" API\n @@ revision.h: struct rev_info {\n \n @@ revision.h: struct rev_info {\n \t */\n - \tint consumed_stdin:1;\n + \tunsigned int consumed_stdin:1;\n \n +\t/*\n +\t * When reading from stdin (see \"stdin_handling\" above) define\n4: d8080b7fd9c = 4: 4787490a90f pack-objects.c: do stdin parsing via revision.c's API\n5: 32a172aa80a = 5: a8b0976649a pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS\n-- \n2.32.0.956.g6b0c84ceda8\n\n"},{"id":"431180","messageId":"patch-5.5-a8b0976649a-20210726T124425Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210726T124425Z-avarab@gmail.com","subject":"[PATCH v5 5/5] pack-objects.c: make use of REV_INFO_STDIN_LINE_PROCESS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-26T12:46:10Z","receivedAt":"2021-07-26T12:46:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Continue the work started in the preceding commit of porting\npack-objects.c over to the new handle_stdin_line callback pattern. The\ncommon case for the users of this API is to do some of their own\nparsing or munging, and then have handle_revision_arg() handle the\nrest.\n\nThe existing users of the --stdin parsing always wanted a flag of \"0\"\nto be passed to handle_revision_arg(), but pack-objects.c wants to set\ncustom flags. Let's support this common case by having a\n\"revarg_flags\" member in the \"rev_info\" struct.\n\nThis allows us to return REV_INFO_STDIN_LINE_PROCESS in the new\nget_object_list_handle_stdin_line() instead of\nREV_INFO_STDIN_LINE_CONTINUE, as read_revisions_from_stdin() will now\npass down the right flag for us.\n\nI considered making the \"--not\" parsing be another flag handled by the\nrevision.c API itself, but since there's only one caller who wants\nthis, and the \"write_bitmap_index = 0\" case is more specific than\nmarking things UNINTERESTING I think it's better to handle it with a\nmore general mechanism.\n\nThese changes means that we can make the handle_revision_arg()\nfunction static. Now that the only external user of the API has been\nmigrated over to the callback mechanism nothing external to revision.c\nneeds to call handle_revision_arg() anymore.\n\nThat handle_revision_arg() function was made public in a combination\nof 5d6f0935e6d (revision.c: allow injecting revision parameters after\nsetup_revisions()., 2006-09-05) and b5d97e6b0a0 (pack-objects: run\nrev-list equivalent internally., 2006-09-04).\n\nThis change leaves the briefly-used in preceding commits\n\"void *stdin_line_priv\" without any in-tree user, as\nbuiltin/pack-objects.c could be ported over to our new \"revarg_flags\"\ncommon case.\n\nI'm leaving that \"void *stdin_line_priv\" in place anyway for two\nreasons:\n\n 1. It's a common pattern to allow such a \"void *\" to be used for\n callback data, so we might as well follow that pattern here in\n anticipation of a future in-tree user.\n\n 2. I have patches for such an in-tree user already in a series\n that'll be submitted after this one. See the reference to \"git\n bundle create --stdin\" in the commit that added the \"handle_stdin_line\"\n API.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 35 +++++++++++++++--------------------\n revision.c | 4 ++--\n revision.h | 6 ++++--\n 3 files changed, 21 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 35d5247f85a..06a085a9a2a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3748,28 +3748,25 @@ static void mark_bitmap_preferred_tips(void)\n static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n \tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n {\n-\tint *flags = stdin_line_priv;\n \tchar *line = line_sb->buf;\n \n-\tif (*line == '-') {\n-\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t*flags ^= UNINTERESTING;\n-\t\t\twrite_bitmap_index = 0;\n-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n-\t\t}\n-\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\tstruct object_id oid;\n-\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\tuse_bitmap_index = 0;\n-\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n-\t\t}\n+\tif (*line != '-')\n+\t\treturn REV_INFO_STDIN_LINE_PROCESS;\n+\n+\tif (!strcmp(line, \"--not\")) {\n+\t\trevs->revarg_flags ^= UNINTERESTING;\n+\t\twrite_bitmap_index = 0;\n+\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t} else if (starts_with(line, \"--shallow \")) {\n+\t\tstruct object_id oid;\n+\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\tregister_shallow(the_repository, &oid);\n+\t\tuse_bitmap_index = 0;\n+\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t} else {\n \t\tdie(_(\"not a rev '%s'\"), line);\n \t}\n-\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\treturn REV_INFO_STDIN_LINE_CONTINUE;\n }\n \n static void get_object_list(int ac, const char **av)\n@@ -3778,7 +3775,6 @@ static void get_object_list(int ac, const char **av)\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tint flags = 0;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3788,7 +3784,6 @@ static void get_object_list(int ac, const char **av)\n \n \trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n \trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n-\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\ndiff --git a/revision.c b/revision.c\nindex 3f6ab834aff..4164a4fcd11 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2089,7 +2089,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl\n \treturn 0;\n }\n \n-int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n+static int handle_revision_arg(const char *arg, struct rev_info *revs, int flags, unsigned revarg_opt)\n {\n \tint ret = handle_revision_arg_1(arg, revs, flags, revarg_opt);\n \tif (!ret)\n@@ -2139,7 +2139,7 @@ static void read_revisions_from_stdin(struct rev_info *revs,\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\tif (handle_revision_arg(sb.buf, revs, revs->revarg_flags,\n \t\t\t\t\tREVARG_CANNOT_BE_FILENAME))\n \t\t\tdie(\"bad revision '%s'\", sb.buf);\n \t}\ndiff --git a/revision.h b/revision.h\nindex 644b7c8a217..fd7fdbe42e8 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -156,6 +156,9 @@ struct rev_info {\n \t * revision.c's normal processing of the line (after\n \t * possibly munging the provided strbuf).\n \t *\n+\t * Change \"revarg_flags\" to affect the subsequent handling\n+\t * in handle_revision_arg()\n+\t *\n \t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n \t * line is fully processed, moving onto the next line (if\n \t * any)\n@@ -164,6 +167,7 @@ struct rev_info {\n \t * around.\n \t */\n \trev_info_stdin_line_func handle_stdin_line;\n+\tint revarg_flags;\n \tvoid *stdin_line_priv;\n \n \t/* topo-sort */\n@@ -423,8 +427,6 @@ void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,\n \t\t\tconst char * const usagestr[]);\n #define REVARG_CANNOT_BE_FILENAME 01\n #define REVARG_COMMITTISH 02\n-int handle_revision_arg(const char *arg, struct rev_info *revs,\n-\t\t\tint flags, unsigned revarg_opt);\n \n /**\n * Reset the flags used by the revision walking api. You can use this to do\n-- \n2.32.0.956.g6b0c84ceda8\n\n"},{"id":"431181","messageId":"patch-2.5-4e5e6620245-20210726T124425Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210726T124425Z-avarab@gmail.com","subject":"[PATCH v5 2/5] revision.h: refactor \"disable_stdin\" and \"read_from_stdin\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-26T12:46:07Z","receivedAt":"2021-07-26T12:46:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the two \"disable_stdin\" and \"read_from_stdin\" flags to an enum,\nin preparation for a subsequent commit adding more flags.\n\nThe interaction between these is more subtle than they might appear at\nfirst sight, as noted in a12cbe23ef7. \"read_stdin\" is not the inverse\nof \"disable_stdin\", rather we read stdin if we see the \"--stdin\"\noption.\n\nThe \"read\" is intended to understood as \"I've read it already\", not\n\"you should read it\". Let's avoid this confusion by using \"consume\"\nand \"consumed\" instead, i.e. a word whose present and past tense isn't\nthe same.\n\nSee 8b3dce56508 (Teach --stdin option to \"log\" family, 2009-11-03)\nwhere we added the \"disable_stdin\" flag, and a12cbe23ef7 (rev-list:\nmake empty --stdin not an error, 2018-08-22) for the addition of the\n\"read_from_stdin\" flag.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/am.c | 4 ++--\n builtin/blame.c | 2 +-\n builtin/diff-tree.c | 2 +-\n builtin/rev-list.c | 2 +-\n revision.c | 13 ++++++++-----\n revision.h | 24 +++++++++++++++++++++---\n sequencer.c | 4 ++--\n 7 files changed, 36 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 0b2d886c81b..3a6c8455b47 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1355,7 +1355,7 @@ static void write_commit_patch(const struct am_state *state, struct commit *comm\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n \trev_info.abbrev = 0;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.show_root_diff = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.no_commit_id = 1;\n@@ -1390,7 +1390,7 @@ static void write_index_patch(const struct am_state *state)\n \tfp = xfopen(am_path(state, \"patch\"), \"w\");\n \trepo_init_revisions(the_repository, &rev_info, NULL);\n \trev_info.diff = 1;\n-\trev_info.disable_stdin = 1;\n+\trev_info.stdin_handling = REV_INFO_STDIN_IGNORE;\n \trev_info.no_commit_id = 1;\n \trev_info.diffopt.output_format = DIFF_FORMAT_PATCH;\n \trev_info.diffopt.use_color = 0;\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..c9f66c58c46 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -1061,7 +1061,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\targv[argc - 1] = \"--\";\n \t}\n \n-\trevs.disable_stdin = 1;\n+\trevs.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tsetup_revisions(argc, argv, &revs, NULL);\n \tif (!revs.pending.nr && is_bare_repository()) {\n \t\tstruct commit *head_commit;\ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex f33d30d57bf..fc548ebe166 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,7 +122,7 @@ int cmd_diff_tree(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"index file corrupt\"));\n \topt->abbrev = 0;\n \topt->diff = 1;\n-\topt->disable_stdin = 1;\n+\topt->stdin_handling = REV_INFO_STDIN_IGNORE;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.tweak = diff_tree_tweak_rev;\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 36cb909ebaa..f1778833401 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -666,7 +666,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \tif ((!revs.commits && reflog_walk_empty(revs.reflog_info) &&\n \t (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&\n \t !revs.pending.nr) &&\n-\t !revs.rev_input_given && !revs.read_from_stdin) ||\n+\t !revs.rev_input_given && !revs.consumed_stdin) ||\n \t revs.diff)\n \t\tusage(rev_list_usage);\n \ndiff --git a/revision.c b/revision.c\nindex 8140561b6c7..50909339a59 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2741,14 +2741,17 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n-\t\t\t\tif (revs->disable_stdin) {\n+\t\t\t\tswitch (revs->stdin_handling) {\n+\t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n+\t\t\t\tcase REV_INFO_STDIN_CONSUME_ON_OPTION:\n+\t\t\t\t\tif (revs->consumed_stdin)\n+\t\t\t\t\t\tdie(\"--stdin given twice?\");\n+\t\t\t\t\tread_revisions_from_stdin(revs, &prune_data);\n+\t\t\t\t\trevs->consumed_stdin = 1;\n+\t\t\t\t\tcontinue;\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\tcontinue;\n \t\t\t}\n \n \t\t\tif (!strcmp(arg, \"--end-of-options\")) {\ndiff --git a/revision.h b/revision.h\nindex 7464434f600..99458cc0647 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -86,6 +86,11 @@ struct rev_cmdline_info {\n struct oidset;\n struct topo_walk_info;\n \n+enum rev_info_stdin {\n+\tREV_INFO_STDIN_CONSUME_ON_OPTION,\n+\tREV_INFO_STDIN_IGNORE,\n+};\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -114,9 +119,22 @@ struct rev_info {\n \tint rev_input_given;\n \n \t/*\n-\t * Whether we read from stdin due to the --stdin option.\n+\t * How should we handle seeing --stdin?\n+\t *\n+\t * Defaults to REV_INFO_STDIN_CONSUME_ON_OPTION where we'll\n+\t * attempt to read it if we see the --stdin option.\n+\t *\n+\t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n+\t * option.\n+\t */\n+\tenum rev_info_stdin stdin_handling;\n+\n+\t/*\n+\t * Did we read from stdin due to stdin_handling ==\n+\t * REV_INFO_STDIN_CONSUME_ON_OPTION and seeing the --stdin\n+\t * option?\n \t */\n-\tint read_from_stdin;\n+\tunsigned int consumed_stdin:1;\n \n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n@@ -217,7 +235,7 @@ struct rev_info {\n \t\t\tpreserve_subject:1,\n \t\t\tencode_email_headers:1,\n \t\t\tinclude_header:1;\n-\tunsigned int\tdisable_stdin:1;\n+\n \t/* --show-linear-break */\n \tunsigned int\ttrack_linear:1,\n \t\t\ttrack_first_time:1,\ndiff --git a/sequencer.c b/sequencer.c\nindex 0bec01cf38e..4e73bd79d69 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3377,7 +3377,7 @@ static int make_patch(struct repository *r,\n \tlog_tree_opt.abbrev = 0;\n \tlog_tree_opt.diff = 1;\n \tlog_tree_opt.diffopt.output_format = DIFF_FORMAT_PATCH;\n-\tlog_tree_opt.disable_stdin = 1;\n+\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \tlog_tree_opt.no_commit_id = 1;\n \tlog_tree_opt.diffopt.file = fopen(buf.buf, \"w\");\n \tlog_tree_opt.diffopt.use_color = GIT_COLOR_NEVER;\n@@ -4513,7 +4513,7 @@ static int pick_commits(struct repository *r,\n \t\t\tlog_tree_opt.diff = 1;\n \t\t\tlog_tree_opt.diffopt.output_format =\n \t\t\t\tDIFF_FORMAT_DIFFSTAT;\n-\t\t\tlog_tree_opt.disable_stdin = 1;\n+\t\t\tlog_tree_opt.stdin_handling = REV_INFO_STDIN_IGNORE;\n \n \t\t\tif (read_oneliner(&buf, rebase_path_orig_head(), 0) &&\n \t\t\t !get_oid(buf.buf, &orig) &&\n-- \n2.32.0.956.g6b0c84ceda8\n\n"},{"id":"431182","messageId":"patch-4.5-4787490a90f-20210726T124425Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210726T124425Z-avarab@gmail.com","subject":"[PATCH v5 4/5] pack-objects.c: do stdin parsing via revision.c's API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-26T12:46:09Z","receivedAt":"2021-07-26T12:46:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Use the new \"handle_stdin_line\" API in revision.c to parse stdin in\npack-objects.c, instead of using custom pack-objects.c-specific code\nto do so.\n\nThis means that we can remove the \"if (len && line[len - 1] == '\\n')\"\ncheck, it's now redundant to using strbuf_getline(), and we get to\nskip the whole \"warn_on_object_refname_ambiguity\" dance. The\nread_revisions_from_stdin() function in revision.c we're now using\ndoes it for us.\n\nThe pack-objects.c code being refactored away here was first added in\nLinus's c323ac7d9c5 (git-pack-objects: create a packed object\nrepresentation., 2005-06-25).\n\nLater on rev-list started doing similar parsing in 42cabc341c4 (Teach\nrev-list an option to read revs from the standard input., 2006-09-05).\nThat code was promoted to a more general API in 1fc561d169a (Move\nread_revisions_from_stdin from builtin-rev-list.c to revision.c,\n2008-07-05).\n\nSince then the API in revision.c has received improvements that have\nbeen missed here. E.g. the arbitrary limit of 1000 bytes was removed\nin 63d564b3002 (read_revision_from_stdin(): use strbuf, 2009-11-20),\nand it moved to a more simpler strbuf API in 6e8d46f9d4b (revision:\nread --stdin with strbuf_getline(), 2015-10-28).\n\nFor now we've just made setup_revisions() loop over stdin for us, but\nthe callback we define makes no use of REV_INFO_STDIN_LINE_PROCESS. We\nstill need to call handle_revision_arg() ourselves because we'd like\nto call it with different flags.\n\nThis very light use of the API will be further refined in a subsequent\ncommit, for now we're just doing the bare minimum to move this\nexisting code over to the new callback pattern without any functional\nchanges, and making it as friendly to \"git show -w\" and \"the\n--color-moved-ws=allow-indentation-change\" mode as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/pack-objects.c | 63 ++++++++++++++++++++----------------------\n 1 file changed, 30 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex 1fbaa34f91b..35d5247f85a 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -3745,15 +3745,40 @@ static void mark_bitmap_preferred_tips(void)\n \t}\n }\n \n+static enum rev_info_stdin_line get_object_list_handle_stdin_line(\n+\tstruct rev_info *revs, struct strbuf *line_sb, void *stdin_line_priv)\n+{\n+\tint *flags = stdin_line_priv;\n+\tchar *line = line_sb->buf;\n+\n+\tif (*line == '-') {\n+\t\tif (!strcmp(line, \"--not\")) {\n+\t\t\t*flags ^= UNINTERESTING;\n+\t\t\twrite_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tif (starts_with(line, \"--shallow \")) {\n+\t\t\tstruct object_id oid;\n+\t\t\tif (get_oid_hex(line + 10, &oid))\n+\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n+\t\t\tregister_shallow(the_repository, &oid);\n+\t\t\tuse_bitmap_index = 0;\n+\t\t\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+\t\t}\n+\t\tdie(_(\"not a rev '%s'\"), line);\n+\t}\n+\tif (handle_revision_arg(line, revs, *flags, REVARG_CANNOT_BE_FILENAME))\n+\t\t\tdie(_(\"bad revision '%s'\"), line);\n+\treturn REV_INFO_STDIN_LINE_CONTINUE;\n+}\n+\n static void get_object_list(int ac, const char **av)\n {\n \tstruct rev_info revs;\n \tstruct setup_revision_opt s_r_opt = {\n \t\t.allow_exclude_promisor_objects = 1,\n \t};\n-\tchar line[1000];\n \tint flags = 0;\n-\tint save_warning;\n \n \trepo_init_revisions(the_repository, &revs, NULL);\n \tsave_commit_buffer = 0;\n@@ -3761,39 +3786,11 @@ static void get_object_list(int ac, const char **av)\n \t/* make sure shallows are read */\n \tis_repository_shallow(the_repository);\n \n+\trevs.stdin_handling = REV_INFO_STDIN_ALWAYS_READ;\n+\trevs.handle_stdin_line = get_object_list_handle_stdin_line;\n+\trevs.stdin_line_priv = &flags;\n \tsetup_revisions(ac, av, &revs, &s_r_opt);\n \n-\tsave_warning = warn_on_object_refname_ambiguity;\n-\twarn_on_object_refname_ambiguity = 0;\n-\n-\twhile (fgets(line, sizeof(line), stdin) != NULL) {\n-\t\tint len = strlen(line);\n-\t\tif (len && line[len - 1] == '\\n')\n-\t\t\tline[--len] = 0;\n-\t\tif (!len)\n-\t\t\tbreak;\n-\t\tif (*line == '-') {\n-\t\t\tif (!strcmp(line, \"--not\")) {\n-\t\t\t\tflags ^= UNINTERESTING;\n-\t\t\t\twrite_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tif (starts_with(line, \"--shallow \")) {\n-\t\t\t\tstruct object_id oid;\n-\t\t\t\tif (get_oid_hex(line + 10, &oid))\n-\t\t\t\t\tdie(\"not an object name '%s'\", line + 10);\n-\t\t\t\tregister_shallow(the_repository, &oid);\n-\t\t\t\tuse_bitmap_index = 0;\n-\t\t\t\tcontinue;\n-\t\t\t}\n-\t\t\tdie(_(\"not a rev '%s'\"), line);\n-\t\t}\n-\t\tif (handle_revision_arg(line, &revs, flags, REVARG_CANNOT_BE_FILENAME))\n-\t\t\tdie(_(\"bad revision '%s'\"), line);\n-\t}\n-\n-\twarn_on_object_refname_ambiguity = save_warning;\n-\n \tif (use_bitmap_index && !get_object_list_from_bitmap(&revs))\n \t\treturn;\n \n-- \n2.32.0.956.g6b0c84ceda8\n\n"},{"id":"431183","messageId":"patch-3.5-e3d24bd6e8a-20210726T124425Z-avarab@gmail.com","threadId":"55881","inReplyTo":"cover-0.5-00000000000-20210726T124425Z-avarab@gmail.com","subject":"[PATCH v5 3/5] revision.[ch]: add a \"handle_stdin_line\" API","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-26T12:46:08Z","receivedAt":"2021-07-26T12:47:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Extend the rev_info stdin parsing API to support hooking into its\nread_revisions_from_stdin() function, in the next commit we'll change\nthe the custom stdin parsing in pack-objects.c to use it..\n\nFor that use-case adding API is barely justified. We'll be able to\nmake the handle_revision_arg() static in exchange for it, and we'll\navoid the duplicate dance around setting \"save_warning\" and\n\"warn_on_object_refname_ambiguity\", but we could just continue to do\nthat ourselves in builtin/pack-objects.c\n\nThe real reason to add this is for a change not part of this\nseries. We'll soon teach \"git bundle create\" to accept\nrevision/refname pairs on stdin, and thus do away with the limitation\nof it being impossible to create bundles with ref tips that don't\ncorrespond to the state of the current repository. I.e. this will\nwork:\n\n $ printf \"e83c5163316f89bfbde7\\trefs/heads/first-git-dot-git-commit\\n\" |\n git bundle create initial.bundle --stdin\n\nAs well as things like:\n\n $ git for-each-ref 'refs/remotes/origin' |\n sed 's!\\trefs/remotes/origin/!\\trefs/heads/!' |\n git bundle create origin.bundle --stdin\n\nIn order to do that we'll need to modify the lines we consume on stdin\nrevision.c (which bundle.c uses already), and be assured that that\nstripping extra bundle-specific data from them is the only change in\nbehavior.\n\nThat change will be much more complex if bundle.c needs to start doing\nits own stdin parsing again outside of revision.c, it was recently\nconverted to use revision.c's parsing in 5bb0fd2cab5 (bundle:\narguments can be read from stdin, 2021-01-11)\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n revision.c | 22 ++++++++++++++++++++++\n revision.h | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 52 insertions(+)\n\ndiff --git a/revision.c b/revision.c\nindex 50909339a59..3f6ab834aff 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2119,6 +2119,19 @@ static void read_revisions_from_stdin(struct rev_info *revs,\n \t\tint len = sb.len;\n \t\tif (!len)\n \t\t\tbreak;\n+\n+\t\tif (revs->handle_stdin_line) {\n+\t\t\tenum rev_info_stdin_line ret = revs->handle_stdin_line(\n+\t\t\t\trevs, &sb, revs->stdin_line_priv);\n+\n+\t\t\tswitch (ret) {\n+\t\t\tcase REV_INFO_STDIN_LINE_PROCESS:\n+\t\t\t\tbreak;\n+\t\t\tcase REV_INFO_STDIN_LINE_CONTINUE:\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t}\n+\n \t\tif (sb.buf[0] == '-') {\n \t\t\tif (len == 2 && sb.buf[1] == '-') {\n \t\t\t\tseen_dashdash = 1;\n@@ -2742,6 +2755,7 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \n \t\t\tif (!strcmp(arg, \"--stdin\")) {\n \t\t\t\tswitch (revs->stdin_handling) {\n+\t\t\t\tcase REV_INFO_STDIN_ALWAYS_READ:\n \t\t\t\tcase REV_INFO_STDIN_IGNORE:\n \t\t\t\t\targv[left++] = arg;\n \t\t\t\t\tcontinue;\n@@ -2790,6 +2804,14 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s\n \t\t}\n \t}\n \n+\t/*\n+\t * We're asked to ALWAYS_READ from stdin, but no --stdin\n+\t * option (or \"consumed_stdin\" would be set).\n+\t */\n+\tif (!revs->consumed_stdin &&\n+\t revs->stdin_handling == REV_INFO_STDIN_ALWAYS_READ)\n+\t\tread_revisions_from_stdin(revs, &prune_data);\n+\n \tif (prune_data.nr) {\n \t\t/*\n \t\t * If we need to introduce the magic \"a lone ':' means no\ndiff --git a/revision.h b/revision.h\nindex 99458cc0647..644b7c8a217 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -89,8 +89,17 @@ struct topo_walk_info;\n enum rev_info_stdin {\n \tREV_INFO_STDIN_CONSUME_ON_OPTION,\n \tREV_INFO_STDIN_IGNORE,\n+\tREV_INFO_STDIN_ALWAYS_READ,\n };\n \n+enum rev_info_stdin_line {\n+\tREV_INFO_STDIN_LINE_PROCESS,\n+\tREV_INFO_STDIN_LINE_CONTINUE,\n+};\n+\n+typedef enum rev_info_stdin_line (*rev_info_stdin_line_func)(\n+\tstruct rev_info *revs, struct strbuf *line, void *stdin_line_priv);\n+\n struct rev_info {\n \t/* Starting list */\n \tstruct commit_list *commits;\n@@ -126,6 +135,9 @@ struct rev_info {\n \t *\n \t * Can be set to REV_INFO_STDIN_IGNORE to ignore the --stdin\n \t * option.\n+\t *\n+\t * Set it to REV_INFO_STDIN_ALWAYS_READ if there's always data\n+\t * on stdin to be read, even if no --stdin option is provided.\n \t */\n \tenum rev_info_stdin stdin_handling;\n \n@@ -136,6 +148,24 @@ struct rev_info {\n \t */\n \tunsigned int consumed_stdin:1;\n \n+\t/*\n+\t * When reading from stdin (see \"stdin_handling\" above) define\n+\t * a handle_stdin_line function to consume the lines.\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_PROCESS to continue\n+\t * revision.c's normal processing of the line (after\n+\t * possibly munging the provided strbuf).\n+\t *\n+\t * - Return REV_INFO_STDIN_LINE_CONTINUE to indicate that the\n+\t * line is fully processed, moving onto the next line (if\n+\t * any)\n+\t *\n+\t * Use the \"stdin_line_priv\" to optionally pass your own data\n+\t * around.\n+\t */\n+\trev_info_stdin_line_func handle_stdin_line;\n+\tvoid *stdin_line_priv;\n+\n \t/* topo-sort */\n \tenum rev_sort_order sort_order;\n \n-- \n2.32.0.956.g6b0c84ceda8\n\n"}]}