{"thread":{"id":"57333","subject":"[RFC v3] cat-file: add a --stdin-cmd mode","startedAt":"2022-01-28T18:33:53Z","lastAt":"2022-02-02T17:15:12Z","messageCount":12,"participants":["John Cai","Bagas Sanjaya","Junio C Hamano","Christian Couder","Ævar Arnfjörð Bjarmason","Phillip Wood","Taylor Blau"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"447230","messageId":"20220128183319.43496-1-johncai86@gmail.com","threadId":"57333","inReplyTo":null,"subject":"[RFC v3] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-28T18:33:19Z","receivedAt":"2022-01-28T18:33:53Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This RFC patch proposes a new flag --batch-command that works with\ngit-cat-file --batch. Similar to git-update-ref --stdin, it will accept\ncommands and arguments from stdin.\n\nThe start of this idea was discussed in [1], where the original\nmotivation was to be able to control when the buffer was flushed to\nstdout in --buffer mode.\n\nHowever, this can actually be much more useful in situations when\ngit-cat-file --batch is being used as a long lived backend query\nprocess. At GitLab, we use a pair of cat-file processes. One for\niterating over object metadata with --batch-check, and the other to grab\nobject contents with --batch. However, if we had --batch-command, we could\nget rid of the second --batch-check process, and just have one process\nwhere we can flip between getting object info, and getting object contents.\nThis can lead to huge savings since on a given server there could be hundreds to\nthousands of git cat-file processes at a time.\n\ngit cat-file --batch-command\n\n$ <command> [arg1] [arg2] NL\n\nThis patch adds three commands: object, info, fflush\n\n$ object <sha1> NL\n$ info <sha1> NL\n$ fflush NL\n\nThese three would be immediately useful in GitLab's context, but one can\nimagine this mode to be further extended for other things.\n\nFuture improvements:\n- a non-trivial part of \"cat-file --batch\" time is spent\non parsing its argument and seeing if it's a revision, ref etc. So we\ncould add a command that only accepts a full-length 40\ncharacter SHA-1.\n\nThis would be the first step in adding such an interface to\ngit-cat-file.\n\n[1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n\nHelped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n\nTaylor, I'd be interested in your thoughts on this proposal since you helped\nreview the previous patch that turned into this RFC. Thanks!\n\nChanges from v2:\n\n- refactored tests to be within run_tests()\n- added a test to test --buffer behavior with fflush\n- cleaned up cat-file.c: clarified var names, removed unnecessary code\n  based on suggestions from Phillip Wood\n- removed strvec changes\n\nChanges from v1:\n\n- changed option name to batch-command.\n- changed command function interface to receive the whole line after the command\n  name to put the onus of parsing arguments to each individual command function.\n- pass in whole line to batch_one_object in both parse_cmd_object and\n  parse_cmd_info to support spaces in the object reference.\n- removed addition of -z to include in a separate patch series\n- added documentation.\n---\n Documentation/git-cat-file.txt |  15 +++++\n builtin/cat-file.c             | 114 +++++++++++++++++++++++++++++++--\n t/t1006-cat-file.sh            |  83 ++++++++++++++++++++++++\n 3 files changed, 205 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex bef76f4dd0..254e546c79 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -96,6 +96,21 @@ OPTIONS\n \tneed to specify the path, separated by whitespace.  See the\n \tsection `BATCH OUTPUT` below for details.\n \n+--batch-command::\n+\tEnter a command mode that reads from stdin. May not be combined with any\n+\tother options or arguments except `--textconv` or `--filters`, in which\n+\tcase the input lines also need to specify the path, separated by\n+\twhitespace.  See the section `BATCH OUTPUT` below for details.\n+\n+object <object>::\n+\tPrint object contents for object reference <object>\n+\n+info <object>::\n+\tPrint object info for object reference <object>\n+\n+flush::\n+\tFlush to stdout immediately when used with --buffer\n+\n --batch-all-objects::\n \tInstead of reading a list of objects on stdin, perform the\n \trequested batch operation on all objects in the repository and\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 7b3f42950e..cc9e47943b 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -24,9 +24,11 @@ struct batch_options {\n \tint buffer_output;\n \tint all_objects;\n \tint unordered;\n-\tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n+\tint mode; /* may be 'w' or 'c' for --filters or --textconv */\n \tconst char *format;\n+\tint command;\n };\n+static char line_termination = '\\n';\n \n static const char *force_path;\n \n@@ -302,19 +304,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \tif (data->type == OBJ_BLOB) {\n \t\tif (opt->buffer_output)\n \t\t\tfflush(stdout);\n-\t\tif (opt->cmdmode) {\n+\t\tif (opt->mode) {\n \t\t\tchar *contents;\n \t\t\tunsigned long size;\n \n \t\t\tif (!data->rest)\n \t\t\t\tdie(\"missing path for '%s'\", oid_to_hex(oid));\n \n-\t\t\tif (opt->cmdmode == 'w') {\n+\t\t\tif (opt->mode == 'w') {\n \t\t\t\tif (filter_object(data->rest, 0100644, oid,\n \t\t\t\t\t\t  &contents, &size))\n \t\t\t\t\tdie(\"could not convert '%s' %s\",\n \t\t\t\t\t    oid_to_hex(oid), data->rest);\n-\t\t\t} else if (opt->cmdmode == 'c') {\n+\t\t\t} else if (opt->mode == 'c') {\n \t\t\t\tenum object_type type;\n \t\t\t\tif (!textconv_object(the_repository,\n \t\t\t\t\t\t     data->rest, 0100644, oid,\n@@ -326,7 +328,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t\t\t\t\tdie(\"could not convert '%s' %s\",\n \t\t\t\t\t    oid_to_hex(oid), data->rest);\n \t\t\t} else\n-\t\t\t\tBUG(\"invalid cmdmode: %c\", opt->cmdmode);\n+\t\t\t\tBUG(\"invalid mode: %c\", opt->mode);\n \t\t\tbatch_write(opt, contents, size);\n \t\t\tfree(contents);\n \t\t} else {\n@@ -508,6 +510,95 @@ static int batch_unordered_packed(const struct object_id *oid,\n \t\t\t\t      data);\n }\n \n+static void parse_cmd_object(struct batch_options *opt,\n+\t\t\t     const char *line,\n+\t\t\t     struct strbuf *output,\n+\t\t\t     struct expand_data *data)\n+{\n+\topt->print_contents = 1;\n+\tbatch_one_object(line, output, opt, data);\n+}\n+\n+static void parse_cmd_info(struct batch_options *opt,\n+\t\t\t   const char *line,\n+\t\t\t   struct strbuf *output,\n+\t\t\t   struct expand_data *data)\n+{\n+\topt->print_contents = 0;\n+\tbatch_one_object(line, output, opt, data);\n+}\n+\n+static void parse_cmd_fflush(struct batch_options *opt,\n+\t\t\t     const char *line,\n+\t\t\t     struct strbuf *output,\n+\t\t\t     struct expand_data *data)\n+{\n+\tfflush(stdout);\n+}\n+\n+typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,\n+\t\t\t       struct strbuf *, struct expand_data *);\n+\n+static const struct parse_cmd {\n+\tconst char *prefix;\n+\tparse_cmd_fn_t fn;\n+\tunsigned takes_args;\n+} commands[] = {\n+\t{ \"object\", parse_cmd_object, 1},\n+\t{ \"info\", parse_cmd_info, 1},\n+\t{ \"fflush\", parse_cmd_fflush, 0},\n+};\n+\n+static void batch_objects_command(struct batch_options *opt,\n+\t\t\t\t    struct strbuf *output,\n+\t\t\t\t    struct expand_data *data)\n+{\n+\tstruct strbuf input = STRBUF_INIT;\n+\n+\t/* Read each line dispatch its command */\n+\twhile (!strbuf_getwholeline(&input, stdin, line_termination)) {\n+\t\tint i;\n+\t\tconst struct parse_cmd *cmd = NULL;\n+\t\tconst char *p, *cmd_end;\n+\n+\t\tif (*input.buf == line_termination)\n+\t\t\tdie(\"empty command in input\");\n+\t\telse if (isspace(*input.buf))\n+\t\t\tdie(\"whitespace before command: %s\", input.buf);\n+\n+\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n+\t\t\tconst char *prefix = commands[i].prefix;\n+\t\t\tchar c;\n+\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_end))\n+\t\t\t\tcontinue;\n+\t\t\t/*\n+\t\t\t * If the command has arguments, verify that it's\n+\t\t\t * followed by a space. Otherwise, it shall be followed\n+\t\t\t * by a line terminator.\n+\t\t\t */\n+\t\t\tc = commands[i].takes_args ? ' ' : line_termination;\n+\t\t\tif (input.buf[strlen(prefix)] != c)\n+\t\t\t\tdie(\"arguments invalid for command: %s\", commands[i].prefix);\n+\n+\t\t\tcmd = &commands[i];\n+\t\t\tif (cmd->takes_args) {\n+\t\t\t\tp = cmd_end + 1;\n+\t\t\t\t// strip newline before handing it to the\n+\t\t\t\t// handling function\n+\t\t\t\tinput.buf[strcspn(input.buf, \"\\n\")] = '\\0';\n+\t\t\t}\n+\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tif (!cmd)\n+\t\t\tdie(\"unknown command: %s\", input.buf);\n+\n+\t\tcmd->fn(opt, p, output, data);\n+\t}\n+\tstrbuf_release(&input);\n+}\n+\n static int batch_objects(struct batch_options *opt)\n {\n \tstruct strbuf input = STRBUF_INIT;\n@@ -515,6 +606,7 @@ static int batch_objects(struct batch_options *opt)\n \tstruct expand_data data;\n \tint save_warning;\n \tint retval = 0;\n+\tconst int command = opt->command;\n \n \tif (!opt->format)\n \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n@@ -529,7 +621,7 @@ static int batch_objects(struct batch_options *opt)\n \tstrbuf_expand(&output, opt->format, expand_format, &data);\n \tdata.mark_query = 0;\n \tstrbuf_release(&output);\n-\tif (opt->cmdmode)\n+\tif (opt->mode)\n \t\tdata.split_on_whitespace = 1;\n \n \t/*\n@@ -590,6 +682,9 @@ static int batch_objects(struct batch_options *opt)\n \tsave_warning = warn_on_object_refname_ambiguity;\n \twarn_on_object_refname_ambiguity = 0;\n \n+\tif (command)\n+\t\tbatch_objects_command(opt, &output, &data);\n+\n \twhile (strbuf_getline(&input, stdin) != EOF) {\n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\n@@ -636,6 +731,7 @@ static int batch_option_callback(const struct option *opt,\n \n \tbo->enabled = 1;\n \tbo->print_contents = !strcmp(opt->long_name, \"batch\");\n+\tbo->command = !strcmp(opt->long_name, \"batch-command\");\n \tbo->format = arg;\n \n \treturn 0;\n@@ -683,6 +779,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\t\tN_(\"like --batch, but don't emit <contents>\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\tbatch_option_callback),\n+\t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"\"),\n+\t\t\t N_(\"enters batch mode that accepts commands\"),\n+\t\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n+\t\t\t batch_option_callback),\n \t\tOPT_CMDMODE(0, \"batch-all-objects\", &opt,\n \t\t\t    N_(\"with --batch[-check]: ignores stdin, batches all known objects\"), 'b'),\n \t\t/* Batch-specific options */\n@@ -742,7 +842,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t/* Return early if we're in batch mode? */\n \tif (batch.enabled) {\n \t\tif (opt_cw)\n-\t\t\tbatch.cmdmode = opt;\n+\t\t\tbatch.mode = opt;\n \t\telse if (opt && opt != 'b')\n \t\t\tusage_msg_optf(_(\"'-%c' is incompatible with batch mode\"),\n \t\t\t\t       usage, options, opt);\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 145eee11df..92f0b14a95 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -112,6 +112,46 @@ maybe_remove_timestamp () {\n     fi\n }\n \n+run_buffer_test () {\n+\ttype=$1\n+\tsha1=$2\n+\tsize=$3\n+\tflush=$4\n+\n+\tmkfifo V.input\n+\texec 8<>V.input\n+\trm V.input\n+\n+\tmkfifo V.output\n+\texec 9<>V.output\n+\trm V.output\n+\t\n+\t(\n+\t\tgit cat-file --buffer --batch-command <&8 >&9 &\n+\t\techo $! >&9 &&\n+\t\twait $!\n+\t\techo >&9 EARLY_EXIT\n+\t) &\n+\tsh_pid=$!\n+\tread fi_pid <&9\n+\ttest_when_finished \"\n+\t\texec 8>&-; exec 9>&-;\n+\t\tkill $sh_pid && wait $sh_pid\n+\t\tkill $fi_pid && wait $fi_pid\n+\t\ttrue\"\n+\texpect=$(echo \"$sha1 $type $size\")\n+\techo \"info $sha1\" >&8\n+\tif test \"$flush\" = \"true\"\n+\tthen\n+\t\techo \"fflush\" >&8\n+\telse\n+\t\texpect=\"EARLY_EXIT\"\n+\t\tkill $fi_pid && wait $fi_pid\n+\tfi\n+\tread actual <&9\n+\ttest \"$actual\" = \"$expect\"\n+}\n+\n run_tests () {\n     type=$1\n     sha1=$2\n@@ -177,6 +217,18 @@ $content\"\n \ttest_cmp expect actual\n     '\n \n+    test -z \"$content\" ||\n+    test_expect_success \"--batch-command output of $type content is correct\" '\n+\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n+\tmaybe_remove_timestamp \"$(echo object $sha1 | git cat-file --batch-command)\" $no_ts >actual &&\n+\ttest_cmp expect actual\n+    '\n+\n+    test_expect_success \"batch-command output of $type info is correct\" '\n+\techo \"$sha1 $type $size\" >expect &&\n+\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n+\ttest_cmp expect actual\n+'\n     test_expect_success \"custom --batch-check format\" '\n \techo \"$type $sha1\" >expect &&\n \techo $sha1 | git cat-file --batch-check=\"%(objecttype) %(objectname)\" >actual &&\n@@ -232,12 +284,29 @@ test_expect_success '--batch-check without %(rest) considers whole line' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--batch-command --buffer with flush is correct for blob' '\n+\trun_buffer_test 'blob' $hello_sha1 $hello_size true\n+'\n+\n+test_expect_success '--batch-command --buffer without flush is correct for blob' '\n+\trun_buffer_test 'blob' $hello_sha1 $hello_size false\n+'\n+\n tree_sha1=$(git write-tree)\n+\n tree_size=$(($(test_oid rawsz) + 13))\n tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n \n run_tests 'tree' $tree_sha1 $tree_size \"\" \"$tree_pretty_content\"\n \n+test_expect_success '--batch-command --buffer with flush is correct for tree' '\n+\trun_buffer_test 'tree' $tree_sha1 $tree_size true\n+'\n+\n+test_expect_success '--batch-command --buffer without flush is correct for tree' '\n+\trun_buffer_test 'tree' $tree_sha1 $tree_size false\n+'\n+\n commit_message=\"Initial commit\"\n commit_sha1=$(echo_without_newline \"$commit_message\" | git commit-tree $tree_sha1)\n commit_size=$(($(test_oid hexsz) + 137))\n@@ -263,6 +332,14 @@ tag_size=$(strlen \"$tag_content\")\n \n run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n \n+test_expect_success '--batch-command --buffer with flush is correct for tag' '\n+\trun_buffer_test 'tag' $tag_sha1 $tag_size true\n+'\n+\n+test_expect_success '--batch-command --buffer without flush is correct for tag' '\n+\trun_buffer_test 'tag' $tag_sha1 $tag_size false\n+'\n+\n test_expect_success \\\n     \"Reach a blob from a tag pointing to it\" \\\n     \"test '$hello_content' = \\\"\\$(git cat-file blob $tag_sha1)\\\"\"\n@@ -964,4 +1041,10 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'batch-command unknown command' '\n+\techo unknown_command >cmd &&\n+\ttest_expect_code 128 git cat-file --batch-command < cmd 2>err &&\n+\tgrep -E \"^fatal:.*unknown command.*\" err\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"447328","messageId":"56f851f3-946b-0524-1643-25799ef46b55@gmail.com","threadId":"57333","inReplyTo":"20220128183319.43496-1-johncai86@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Bagas Sanjaya","fromEmail":"bagasdotme@gmail.com","sentAt":"2022-01-31T11:44:20Z","receivedAt":"2022-01-31T11:58:44Z","isPatch":false,"sender":{"key":"bagasdotme@gmail.com","avatar":"https://avatars.githubusercontent.com/u/40219486?v=4"},"body":"On 29/01/22 01.33, John Cai wrote:\n> Future improvements:\n> - a non-trivial part of \"cat-file --batch\" time is spent\n> on parsing its argument and seeing if it's a revision, ref etc. So we\n> could add a command that only accepts a full-length 40\n> character SHA-1.\n\nI think the full hash is actually revision name.\n\n-- \nAn old man doll... just what I always wanted! - Clara\n"},{"id":"447355","messageId":"xmqqpmo7ix0l.fsf@gitster.g","threadId":"57333","inReplyTo":"56f851f3-946b-0524-1643-25799ef46b55@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-01-31T18:24:26Z","receivedAt":"2022-01-31T18:24:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bagas Sanjaya <bagasdotme@gmail.com> writes:\n\n> On 29/01/22 01.33, John Cai wrote:\n>> Future improvements:\n>> - a non-trivial part of \"cat-file --batch\" time is spent\n>> on parsing its argument and seeing if it's a revision, ref etc. So we\n>> could add a command that only accepts a full-length 40\n>> character SHA-1.\n>\n> I think the full hash is actually revision name.\n\nThere is no entry for \"revision name\" in Documentation/glossary-content.txt\n;-)\n\nBut to John, if you have a loop that feedseach line to get_oid(), \n\n\twhile (getline(buf)) {\n\t\tstruct object_id oid;\n\t\tif (get_oid(buf, &oid))\n\t\t\twarn and continue;\n\t\tuse oid;\n\t}\n\nis it much slower than a mode that can ONLY handle a full object\nname input, i.e.\n\n\twhile (getline(buf)) {\n\t\tstruct object_id oid;\n\t\tif (get_oid_hex(buf, &oid))\n\t\t\twarn and continue;\n\t\tuse oid;\n\t}\n\nwhen your input is restricted to full object names?\n\nget_oid() == repo_get_oid()\n-> get_oid_with_context()\n   -> get_oid_with_context_1()\n      -> get_oid_1()\n\t -> peel_onion()\n\t -> get_oid_basic()\n\t    -> get_oid_hex()\n\t    -> repo_dwim_ref()\n\nit seems that warn_ambiguous_refs and warn_on_object_refname_ambiguity\nwe would waste time on refname discovery but I see cat-file already\nhas some provision to disable this check.  So when we do not need to\ncall repo_dwim_ref(), do we still spend measurable cycles in this\ncallchain?\n\n\n\n\n\n"},{"id":"447400","messageId":"CAP8UFD3sHvA3Gx9+d=VjQ11sEqWF47AEeo-m4bGsVO3OUJ4dLw@mail.gmail.com","threadId":"57333","inReplyTo":"20220128183319.43496-1-johncai86@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2022-02-01T09:39:30Z","receivedAt":"2022-02-01T09:39:44Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Jan 31, 2022 at 9:34 PM John Cai <johncai86@gmail.com> wrote:\n>\n> This RFC patch proposes a new flag --batch-command that works with\n> git-cat-file --batch.\n\nThe subject is \"Re: [RFC v3] cat-file: add a --stdin-cmd mode\" and now\nyou are talking about '--batch-command' instead of '--stdin-cmd'.\n\n\"that works with git-cat-file --batch\" is not very clear. Maybe you\ncould find a wording that explains better how --batch-command is\ndifferent from --batch.\n\nAlso I think at this point this should probably not be an RFC patch\nanymore but a regular one.\n\n> Similar to git-update-ref --stdin, it will accept\n> commands and arguments from stdin.\n>\n> The start of this idea was discussed in [1], where the original\n> motivation was to be able to control when the buffer was flushed to\n> stdout in --buffer mode.\n\nThat would be nice in a cover letter but I am not sure a commit\nmessage is the right place for this.\n\n> However, this can actually be much more useful in situations when\n> git-cat-file --batch is being used as a long lived backend query\n> process. At GitLab, we use a pair of cat-file processes. One for\n> iterating over object metadata with --batch-check, and the other to grab\n> object contents with --batch. However, if we had --batch-command, we could\n> get rid of the second --batch-check process,\n\nMaybe s/second// would make it clear that there are no two\n--batch-command processes.\n\n> and just have one process\n> where we can flip between getting object info, and getting object contents.\n> This can lead to huge savings since on a given server there could be hundreds to\n> thousands of git cat-file processes at a time.\n\nIt's not clear if all the git cat-file processes you are talking about\nare mostly --batch-check processes or --batch processes, or a roughly\nequal amount of both. My guess is the latter and that --batch-command\nwould mean that there would be around two times fewer cat-file\nprocesses.\n\n> git cat-file --batch-command\n>\n> $ <command> [arg1] [arg2] NL\n\nIt's a bit unclear what the 2 above lines mean. Maybe you could add a\nsmall explanation like for example \"The new flag can be used like\nthis:\" and \"It receives commands from stdin in the format:\"\n\nAlso not sure why there is a '$' char in front of '<command> [arg1]\n[arg2] NL' but not in front of 'git cat-file --batch-command'. It\ndoesn't look like in the 'git update-ref --stdin' doc that '$' are\nused in front of the commands that can be passed through stdin.\n\n> This patch adds three commands: object, info, fflush\n\nMaybe s/three commands/the following first three commands/\n\n> $ object <sha1> NL\n> $ info <sha1> NL\n> $ fflush NL\n\nIdem about '$'.\n\n> These three would be immediately useful in GitLab's context, but one can\n> imagine this mode to be further extended for other things.\n\nNot very clear which \"mode\" you are talking about. You have been\ntalking about a mode only in the subject so far. Maybe you should talk\na bit about that above when '<command> [arg1] [arg2] NL' is\nintroduced.\n\nAlso you don't talk about the output format. --batch and --batch-check\naccept [=<format>], but it looks like --batch-command doesn't.\n\n> Future improvements:\n> - a non-trivial part of \"cat-file --batch\" time is spent\n> on parsing its argument and seeing if it's a revision, ref etc. So we\n> could add a command that only accepts a full-length 40\n> character SHA-1.\n\nIn a cover letter that would be ok, but I am not sure that a commit\nmessage is the best place for this kind of details about future work.\n\n> This would be the first step in adding such an interface to\n> git-cat-file.\n>\n> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n>\n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: John Cai <johncai86@gmail.com>\n> ---\n>\n> Taylor, I'd be interested in your thoughts on this proposal since you helped\n> review the previous patch that turned into this RFC. Thanks!\n>\n> Changes from v2:\n>\n> - refactored tests to be within run_tests()\n> - added a test to test --buffer behavior with fflush\n> - cleaned up cat-file.c: clarified var names, removed unnecessary code\n>   based on suggestions from Phillip Wood\n> - removed strvec changes\n>\n> Changes from v1:\n>\n> - changed option name to batch-command.\n> - changed command function interface to receive the whole line after the command\n>   name to put the onus of parsing arguments to each individual command function.\n> - pass in whole line to batch_one_object in both parse_cmd_object and\n>   parse_cmd_info to support spaces in the object reference.\n> - removed addition of -z to include in a separate patch series\n> - added documentation.\n> ---\n>  Documentation/git-cat-file.txt |  15 +++++\n>  builtin/cat-file.c             | 114 +++++++++++++++++++++++++++++++--\n>  t/t1006-cat-file.sh            |  83 ++++++++++++++++++++++++\n>  3 files changed, 205 insertions(+), 7 deletions(-)\n>\n> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n> index bef76f4dd0..254e546c79 100644\n> --- a/Documentation/git-cat-file.txt\n> +++ b/Documentation/git-cat-file.txt\n> @@ -96,6 +96,21 @@ OPTIONS\n>         need to specify the path, separated by whitespace.  See the\n>         section `BATCH OUTPUT` below for details.\n>\n> +--batch-command::\n> +       Enter a command mode that reads from stdin.\n\nMaybe s/a command mode that reads from stdin/a mode that reads\ncommands from stdin/\n\nAlso I would expect something about the output, like perhaps \"...and\nouputs the command results to stdout\".\n\n> May not be combined with any\n> +       other options or arguments except `--textconv` or `--filters`, in which\n> +       case the input lines also need to specify the path, separated by\n> +       whitespace.  See the section `BATCH OUTPUT` below for details.\n\nThe BATCH OUTPUT section says that a format can be passed but that\ndoesn't seem to be the case with --batch-command. So you might need to\nmake some changes to that section too or add a bit more details about\nthe output here.\n\n> +object <object>::\n> +       Print object contents for object reference <object>\n> +\n> +info <object>::\n> +       Print object info for object reference <object>\n> +\n> +flush::\n> +       Flush to stdout immediately when used with --buffer\n> +\n>  --batch-all-objects::\n>         Instead of reading a list of objects on stdin, perform the\n>         requested batch operation on all objects in the repository and\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 7b3f42950e..cc9e47943b 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -24,9 +24,11 @@ struct batch_options {\n>         int buffer_output;\n>         int all_objects;\n>         int unordered;\n> -       int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n> +       int mode; /* may be 'w' or 'c' for --filters or --textconv */\n>         const char *format;\n> +       int command;\n>  };\n\nMaybe add a blank line here.\n\n> +static char line_termination = '\\n';\n>\n>  static const char *force_path;\n>\n> @@ -302,19 +304,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n>         if (data->type == OBJ_BLOB) {\n>                 if (opt->buffer_output)\n>                         fflush(stdout);\n> -               if (opt->cmdmode) {\n> +               if (opt->mode) {\n\nThe mechanical s/cmdmode/mode/g change could have been made in a\npreparatory patch to make this patch a bit smaller and easier to\ndigest.\n\n> +static void batch_objects_command(struct batch_options *opt,\n> +                                   struct strbuf *output,\n> +                                   struct expand_data *data)\n> +{\n> +       struct strbuf input = STRBUF_INIT;\n> +\n> +       /* Read each line dispatch its command */\n> +       while (!strbuf_getwholeline(&input, stdin, line_termination)) {\n> +               int i;\n> +               const struct parse_cmd *cmd = NULL;\n> +               const char *p, *cmd_end;\n> +\n> +               if (*input.buf == line_termination)\n> +                       die(\"empty command in input\");\n> +               else if (isspace(*input.buf))\n> +                       die(\"whitespace before command: %s\", input.buf);\n> +\n> +               for (i = 0; i < ARRAY_SIZE(commands); i++) {\n> +                       const char *prefix = commands[i].prefix;\n> +                       char c;\n> +                       if (!skip_prefix(input.buf, prefix, &cmd_end))\n> +                               continue;\n> +                       /*\n> +                        * If the command has arguments, verify that it's\n> +                        * followed by a space. Otherwise, it shall be followed\n> +                        * by a line terminator.\n> +                        */\n> +                       c = commands[i].takes_args ? ' ' : line_termination;\n> +                       if (input.buf[strlen(prefix)] != c)\n> +                               die(\"arguments invalid for command: %s\", commands[i].prefix);\n> +\n> +                       cmd = &commands[i];\n> +                       if (cmd->takes_args) {\n> +                               p = cmd_end + 1;\n> +                               // strip newline before handing it to the\n> +                               // handling function\n\nSo above the /* */ comments delimiters are used but here // is used. I\nam not sure we support // these days, but if we do, I think it would\nbe better to avoid mixing comment styles in the same function.\n\n> +                               input.buf[strcspn(input.buf, \"\\n\")] = '\\0';\n> +                       }\n> +\n> +                       break;\n> +               }\n> +\n> +               if (!cmd)\n> +                       die(\"unknown command: %s\", input.buf);\n> +\n> +               cmd->fn(opt, p, output, data);\n> +       }\n> +       strbuf_release(&input);\n> +}\n\n> @@ -590,6 +682,9 @@ static int batch_objects(struct batch_options *opt)\n>         save_warning = warn_on_object_refname_ambiguity;\n>         warn_on_object_refname_ambiguity = 0;\n>\n> +       if (command)\n> +               batch_objects_command(opt, &output, &data);\n> +\n>         while (strbuf_getline(&input, stdin) != EOF) {\n\nI think batch_objects_command() will consume everything from stdin, so\nit doesn't make sense to try to read again from stdin after it. Maybe\nthe whole while (...) { ... } clause should be inside an else clause\nor something.\n\n>                 if (data.split_on_whitespace) {\n>                         /*\n> @@ -636,6 +731,7 @@ static int batch_option_callback(const struct option *opt,\n>\n>         bo->enabled = 1;\n>         bo->print_contents = !strcmp(opt->long_name, \"batch\");\n> +       bo->command = !strcmp(opt->long_name, \"batch-command\");\n>         bo->format = arg;\n>\n>         return 0;\n"},{"id":"447402","messageId":"220201.86r18map8t.gmgdl@evledraar.gmail.com","threadId":"57333","inReplyTo":"xmqqpmo7ix0l.fsf@gitster.g","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-01T09:48:38Z","receivedAt":"2022-02-01T09:51:51Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jan 31 2022, Junio C Hamano wrote:\n\n> Bagas Sanjaya <bagasdotme@gmail.com> writes:\n>\n>> On 29/01/22 01.33, John Cai wrote:\n>>> Future improvements:\n>>> - a non-trivial part of \"cat-file --batch\" time is spent\n>>> on parsing its argument and seeing if it's a revision, ref etc. So we\n>>> could add a command that only accepts a full-length 40\n>>> character SHA-1.\n>>\n>> I think the full hash is actually revision name.\n>\n> There is no entry for \"revision name\" in Documentation/glossary-content.txt\n> ;-)\n>\n> But to John, if you have a loop that feedseach line to get_oid(), \n>\n> \twhile (getline(buf)) {\n> \t\tstruct object_id oid;\n> \t\tif (get_oid(buf, &oid))\n> \t\t\twarn and continue;\n> \t\tuse oid;\n> \t}\n>\n> is it much slower than a mode that can ONLY handle a full object\n> name input, i.e.\n>\n> \twhile (getline(buf)) {\n> \t\tstruct object_id oid;\n> \t\tif (get_oid_hex(buf, &oid))\n> \t\t\twarn and continue;\n> \t\tuse oid;\n> \t}\n>\n> when your input is restricted to full object names?\n>\n> get_oid() == repo_get_oid()\n> -> get_oid_with_context()\n>    -> get_oid_with_context_1()\n>       -> get_oid_1()\n> \t -> peel_onion()\n> \t -> get_oid_basic()\n> \t    -> get_oid_hex()\n> \t    -> repo_dwim_ref()\n>\n> it seems that warn_ambiguous_refs and warn_on_object_refname_ambiguity\n> we would waste time on refname discovery but I see cat-file already\n> has some provision to disable this check.  So when we do not need to\n> call repo_dwim_ref(), do we still spend measurable cycles in this\n> callchain?\n\nFor what it's worth I think this claim that we spend a non-trivial\namount of time on the difference between these two comes from me\noriginally. I'd had a chat with John about various things to try out in\nsuch a \"cat-file --batch\" mode, and this was one of those things.\n\nI tried instrumenting the relevant code in builtin/cat-file.c the other\n(but forgot to reply to this thread), and whatever I'd found there at\nthe time (this was weeks/months ago) I couldn't reproduce.\n\nSo there's probably nothing worthwhile to check out here, i.e. the\ntrivial cost of get_oid_with_context() is probably nothing to worry\nabout.\n"},{"id":"447403","messageId":"e75ba9ea-fdda-6e9f-4dd6-24190117d93b@gmail.com","threadId":"57333","inReplyTo":"20220128183319.43496-1-johncai86@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-02-01T10:43:30Z","receivedAt":"2022-02-01T10:43:35Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nOn 28/01/2022 18:33, John Cai wrote:\n> This RFC patch proposes a new flag --batch-command that works with\n> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept\n> commands and arguments from stdin.\n> \n> The start of this idea was discussed in [1], where the original\n> motivation was to be able to control when the buffer was flushed to\n> stdout in --buffer mode.\n> \n> However, this can actually be much more useful in situations when\n> git-cat-file --batch is being used as a long lived backend query\n> process. At GitLab, we use a pair of cat-file processes. One for\n> iterating over object metadata with --batch-check, and the other to grab\n> object contents with --batch. However, if we had --batch-command, we could\n> get rid of the second --batch-check process, and just have one process\n> where we can flip between getting object info, and getting object contents.\n> This can lead to huge savings since on a given server there could be hundreds to\n> thousands of git cat-file processes at a time.\n> \n> git cat-file --batch-command\n> \n> $ <command> [arg1] [arg2] NL\n> \n> This patch adds three commands: object, info, fflush\n> \n> $ object <sha1> NL\n> $ info <sha1> NL\n> $ fflush NL\n> \n> These three would be immediately useful in GitLab's context, but one can\n> imagine this mode to be further extended for other things.\n\nI think this is moving in the right direction, I've left some comments \nbelow.\n\n> Future improvements:\n> - a non-trivial part of \"cat-file --batch\" time is spent\n> on parsing its argument and seeing if it's a revision, ref etc. So we\n> could add a command that only accepts a full-length 40\n> character SHA-1.\n> \n> This would be the first step in adding such an interface to\n> git-cat-file.\n> \n> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n> \n> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> Signed-off-by: John Cai <johncai86@gmail.com>\n> ---\n> \n> Taylor, I'd be interested in your thoughts on this proposal since you helped\n> review the previous patch that turned into this RFC. Thanks!\n> \n> Changes from v2:\n> \n> - refactored tests to be within run_tests()\n> - added a test to test --buffer behavior with fflush\n> - cleaned up cat-file.c: clarified var names, removed unnecessary code\n>    based on suggestions from Phillip Wood\n> - removed strvec changes\n> \n> Changes from v1:\n> \n> - changed option name to batch-command.\n> - changed command function interface to receive the whole line after the command\n>    name to put the onus of parsing arguments to each individual command function.\n> - pass in whole line to batch_one_object in both parse_cmd_object and\n>    parse_cmd_info to support spaces in the object reference.\n> - removed addition of -z to include in a separate patch series\n> - added documentation.\n> ---\n>   Documentation/git-cat-file.txt |  15 +++++\n>   builtin/cat-file.c             | 114 +++++++++++++++++++++++++++++++--\n>   t/t1006-cat-file.sh            |  83 ++++++++++++++++++++++++\n>   3 files changed, 205 insertions(+), 7 deletions(-)\n> \n> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n> index bef76f4dd0..254e546c79 100644\n> --- a/Documentation/git-cat-file.txt\n> +++ b/Documentation/git-cat-file.txt\n> @@ -96,6 +96,21 @@ OPTIONS\n>   \tneed to specify the path, separated by whitespace.  See the\n>   \tsection `BATCH OUTPUT` below for details.\n>   \n> +--batch-command::\n> +\tEnter a command mode that reads from stdin. May not be combined with any\n> +\tother options or arguments except `--textconv` or `--filters`, in which\n> +\tcase the input lines also need to specify the path, separated by\n> +\twhitespace.  See the section `BATCH OUTPUT` below for details.\n> +\n> +object <object>::\n> +\tPrint object contents for object reference <object>\n> +\n> +info <object>::\n> +\tPrint object info for object reference <object>\n> +\n> +flush::\n\nI think this is a better name than fflush as calling fflush is just an \nimplementation detail. I've been thinking about this and have some \nconcerns with the current implementation as it allows the caller to \nforce a flush but the output may be flushed at other times and so it \ndoes not allow for deadlock free reading and writing to cat-file from a \nsingle process. If instead we buffered the input lines and only \nprocessed then when we received a flush command it would be easy to read \nand write to cat-file from a single process without deadlocks. This \nwould mean that callers would have to add flush commands to get any \noutput before they closed the pipe writing to cat-file.\n\n> +\tFlush to stdout immediately when used with --buffer\n> +\n>   --batch-all-objects::\n>   \tInstead of reading a list of objects on stdin, perform the\n>   \trequested batch operation on all objects in the repository and\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 7b3f42950e..cc9e47943b 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -24,9 +24,11 @@ struct batch_options {\n>   \tint buffer_output;\n>   \tint all_objects;\n>   \tint unordered;\n> -\tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n> +\tint mode; /* may be 'w' or 'c' for --filters or --textconv */\n\nIf you really need to rename  this variable then doing that in a \nseparate preparatory patch would make this easier to review as it \nseparates out two separate sets of changes.\n\n>   \tconst char *format;\n> +\tint command;\n\nCan't we just call this batch_command and leave cmdmode alone? I don't \nknow maybe cmdmode would be confused with having something to do with \nbatch_command then.\n\n>   };\n> +static char line_termination = '\\n';\n\nThis seems unnecessary as it is the only permitted line ending\n\n>   static const char *force_path;\n>   \n> @@ -302,19 +304,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n>   \tif (data->type == OBJ_BLOB) {\n>   \t\tif (opt->buffer_output)\n>   \t\t\tfflush(stdout);\n> -\t\tif (opt->cmdmode) {\n> +\t\tif (opt->mode) {\n>   \t\t\tchar *contents;\n>   \t\t\tunsigned long size;\n>   \n>   \t\t\tif (!data->rest)\n>   \t\t\t\tdie(\"missing path for '%s'\", oid_to_hex(oid));\n>   \n> -\t\t\tif (opt->cmdmode == 'w') {\n> +\t\t\tif (opt->mode == 'w') {\n>   \t\t\t\tif (filter_object(data->rest, 0100644, oid,\n>   \t\t\t\t\t\t  &contents, &size))\n>   \t\t\t\t\tdie(\"could not convert '%s' %s\",\n>   \t\t\t\t\t    oid_to_hex(oid), data->rest);\n> -\t\t\t} else if (opt->cmdmode == 'c') {\n> +\t\t\t} else if (opt->mode == 'c') {\n>   \t\t\t\tenum object_type type;\n>   \t\t\t\tif (!textconv_object(the_repository,\n>   \t\t\t\t\t\t     data->rest, 0100644, oid,\n> @@ -326,7 +328,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n>   \t\t\t\t\tdie(\"could not convert '%s' %s\",\n>   \t\t\t\t\t    oid_to_hex(oid), data->rest);\n>   \t\t\t} else\n> -\t\t\t\tBUG(\"invalid cmdmode: %c\", opt->cmdmode);\n> +\t\t\t\tBUG(\"invalid mode: %c\", opt->mode);\n>   \t\t\tbatch_write(opt, contents, size);\n>   \t\t\tfree(contents);\n>   \t\t} else {\n> @@ -508,6 +510,95 @@ static int batch_unordered_packed(const struct object_id *oid,\n>   \t\t\t\t      data);\n>   }\n>   \n> +static void parse_cmd_object(struct batch_options *opt,\n> +\t\t\t     const char *line,\n> +\t\t\t     struct strbuf *output,\n> +\t\t\t     struct expand_data *data)\n> +{\n> +\topt->print_contents = 1;\n> +\tbatch_one_object(line, output, opt, data);\n> +}\n> +\n> +static void parse_cmd_info(struct batch_options *opt,\n> +\t\t\t   const char *line,\n> +\t\t\t   struct strbuf *output,\n> +\t\t\t   struct expand_data *data)\n> +{\n> +\topt->print_contents = 0;\n> +\tbatch_one_object(line, output, opt, data);\n> +}\n> +\n> +static void parse_cmd_fflush(struct batch_options *opt,\n> +\t\t\t     const char *line,\n> +\t\t\t     struct strbuf *output,\n> +\t\t\t     struct expand_data *data)\n> +{\n> +\tfflush(stdout);\n> +}\n> +\n> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,\n> +\t\t\t       struct strbuf *, struct expand_data *);\n> +\n> +static const struct parse_cmd {\n> +\tconst char *prefix;\n> +\tparse_cmd_fn_t fn;\n> +\tunsigned takes_args;\n> +} commands[] = {\n> +\t{ \"object\", parse_cmd_object, 1},\n> +\t{ \"info\", parse_cmd_info, 1},\n> +\t{ \"fflush\", parse_cmd_fflush, 0},\n> +};\n> +\n> +static void batch_objects_command(struct batch_options *opt,\n> +\t\t\t\t    struct strbuf *output,\n> +\t\t\t\t    struct expand_data *data)\n> +{\n> +\tstruct strbuf input = STRBUF_INIT;\n> +\n> +\t/* Read each line dispatch its command */\n> +\twhile (!strbuf_getwholeline(&input, stdin, line_termination)) {\n> +\t\tint i;\n> +\t\tconst struct parse_cmd *cmd = NULL;\n> +\t\tconst char *p, *cmd_end;\n> +\n> +\t\tif (*input.buf == line_termination)\n> +\t\t\tdie(\"empty command in input\");\n> +\t\telse if (isspace(*input.buf))\n> +\t\t\tdie(\"whitespace before command: %s\", input.buf);\n> +\n> +\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n> +\t\t\tconst char *prefix = commands[i].prefix;\n> +\t\t\tchar c;\n> +\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_end))\n> +\t\t\t\tcontinue;\n> +\t\t\t/*\n> +\t\t\t * If the command has arguments, verify that it's\n> +\t\t\t * followed by a space. Otherwise, it shall be followed\n> +\t\t\t * by a line terminator.\n> +\t\t\t */\n> +\t\t\tc = commands[i].takes_args ? ' ' : line_termination;\n> +\t\t\tif (input.buf[strlen(prefix)] != c)\n\nAs I pointed out in the last round you can use cmd_end here rather than \ncalling strlen. I've just noticed that this will fail for a command that \ndoes not take arguments when the last input line does not end with '\\n'. \nI think\n\tif (*cmd_end && *cmd_end != c)\nwould be safer\n\n> +\t\t\t\tdie(\"arguments invalid for command: %s\", commands[i].prefix);\n> +\n> +\t\t\tcmd = &commands[i];\n> +\t\t\tif (cmd->takes_args) {\n> +\t\t\t\tp = cmd_end + 1;\n> +\t\t\t\t// strip newline before handing it to the\n> +\t\t\t\t// handling function\n> +\t\t\t\tinput.buf[strcspn(input.buf, \"\\n\")] = '\\0';\n\nWhen I suggested replacing strstr() last time I failed to notice that we \n  know that if there is a newline character it is at input.buf[len - 1] \nso we don't need to scan the whole string to find it.\n\n\tif (input.buf[input.len - 1] == '\\n')\n\t\tinput.buf[--buf.len] = '\\0';\n\n(feel free to change it to avoid the prefix operator)\n\n> +\t\t\t}\n> +\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tif (!cmd)\n> +\t\t\tdie(\"unknown command: %s\", input.buf);\n> +\n> +\t\tcmd->fn(opt, p, output, data);\n> +\t}\n> +\tstrbuf_release(&input);\n> +}\n> +\n>   static int batch_objects(struct batch_options *opt)\n>   {\n>   \tstruct strbuf input = STRBUF_INIT;\n> @@ -515,6 +606,7 @@ static int batch_objects(struct batch_options *opt)\n>   \tstruct expand_data data;\n>   \tint save_warning;\n>   \tint retval = 0;\n> +\tconst int command = opt->command;\n>   \n>   \tif (!opt->format)\n>   \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n> @@ -529,7 +621,7 @@ static int batch_objects(struct batch_options *opt)\n>   \tstrbuf_expand(&output, opt->format, expand_format, &data);\n>   \tdata.mark_query = 0;\n>   \tstrbuf_release(&output);\n> -\tif (opt->cmdmode)\n> +\tif (opt->mode)\n>   \t\tdata.split_on_whitespace = 1;\n>   \n>   \t/*\n> @@ -590,6 +682,9 @@ static int batch_objects(struct batch_options *opt)\n>   \tsave_warning = warn_on_object_refname_ambiguity;\n>   \twarn_on_object_refname_ambiguity = 0;\n>   \n> +\tif (command)\n> +\t\tbatch_objects_command(opt, &output, &data);\n> +\n\nMoving this is good as we no longer need to touch the line below\n\n>   \twhile (strbuf_getline(&input, stdin) != EOF) {\n>   \t\tif (data.split_on_whitespace) {\n>   \t\t\t/*\n> @@ -636,6 +731,7 @@ static int batch_option_callback(const struct option *opt,\n>   \n>   \tbo->enabled = 1;\n>   \tbo->print_contents = !strcmp(opt->long_name, \"batch\");\n> +\tbo->command = !strcmp(opt->long_name, \"batch-command\");\n>   \tbo->format = arg;\n>   \n>   \treturn 0;\n> @@ -683,6 +779,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>   \t\t\tN_(\"like --batch, but don't emit <contents>\"),\n>   \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n>   \t\t\tbatch_option_callback),\n> +\t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"\"),\n> +\t\t\t N_(\"enters batch mode that accepts commands\"),\n> +\t\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n> +\t\t\t batch_option_callback),\n>   \t\tOPT_CMDMODE(0, \"batch-all-objects\", &opt,\n>   \t\t\t    N_(\"with --batch[-check]: ignores stdin, batches all known objects\"), 'b'),\n>   \t\t/* Batch-specific options */\n> @@ -742,7 +842,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>   \t/* Return early if we're in batch mode? */\n>   \tif (batch.enabled) {\n>   \t\tif (opt_cw)\n> -\t\t\tbatch.cmdmode = opt;\n> +\t\t\tbatch.mode = opt;\n>   \t\telse if (opt && opt != 'b')\n>   \t\t\tusage_msg_optf(_(\"'-%c' is incompatible with batch mode\"),\n>   \t\t\t\t       usage, options, opt);\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 145eee11df..92f0b14a95 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -112,6 +112,46 @@ maybe_remove_timestamp () {\n>       fi\n>   }\n\nIt's really good that you've found a way to test fflush\n\n> +run_buffer_test () {\n> +\ttype=$1\n> +\tsha1=$2\n> +\tsize=$3\n> +\tflush=$4\n> +\n> +\tmkfifo V.input\n> +\texec 8<>V.input\n> +\trm V.input\n> +\n> +\tmkfifo V.output\n> +\texec 9<>V.output\n> +\trm V.output\n> +\t\n> +\t(\n> +\t\tgit cat-file --buffer --batch-command <&8 >&9 &\n> +\t\techo $! >&9 &&\n> +\t\twait $!\n> +\t\techo >&9 EARLY_EXIT\n> +\t) &\n> +\tsh_pid=$!\n> +\tread fi_pid <&9 > +\ttest_when_finished \"\n> +\t\texec 8>&-; exec 9>&-;\n> +\t\tkill $sh_pid && wait $sh_pid\n> +\t\tkill $fi_pid && wait $fi_pid\n> +\t\ttrue\"\n> +\texpect=$(echo \"$sha1 $type $size\")\n> +\techo \"info $sha1\" >&8\n> +\tif test \"$flush\" = \"true\"\n> +\tthen\n> +\t\techo \"fflush\" >&8\n\nIf I change \"fflush\" to \"bad-command\" then this test still passes. This \nis because die() flushes stdout and the read below will get the output \nit was expecting and not 'EARLY_EXIT'. I have a suggested change below \nto get around that also checks the exit code of \"git cat-file\" (This \ntest effectively puts a git command upstream of a pipe which we try to \navoid in order to pick up any crashes)\n\n> +\telse\n> +\t\texpect=\"EARLY_EXIT\"\n> +\t\tkill $fi_pid && wait $fi_pid\n> +\tfi\n> +\tread actual <&9\n> +\ttest \"$actual\" = \"$expect\"\n> +}\n> +\n>   run_tests () {\n>       type=$1\n>       sha1=$2\n> @@ -177,6 +217,18 @@ $content\"\n>   \ttest_cmp expect actual\n>       '\n\nHere is my suggestion based on your test - I ended up splitting the \nfflush and no-fflush cases\n\nrun_buffer_test_flush () {\n\ttype=$1\n\tsha1=$2\n\tsize=$3\n\n\tmkfifo input &&\n\ttest_when_finished 'rm input'\n\tmkfifo output &&\n\texec 9<>output &&\n\ttest_when_finished 'rm output; exec 9<&-'\n\t(\n\t    \t# TODO - Ideally we'd pipe the output of cat-file\n\t    \t# through \"sed s'/$/\\\\/'\" to make sure that that read\n\t    \t# would consume all the available\n\t    \t# output. Unfortunately we cannot do this as we cannot\n\t    \t# control when sed flushes its output. We could write\n\t    \t# a test helper in C that appended a '\\' to the end of\n\t\t# each line and flushes its output after every line.\n\t\tgit cat-file --buffer --batch-command <input 2>err &\n\t\techo $! &&\n\t\twait $!\n\t\techo $?\n\t) >&9 &\n\tsh_pid=$! &&\n\tread cat_file_pid <&9 &&\n\ttest_when_finished \"kill $cat_file_pid\n\t\t\t    kill $sh_pid; wait $sh_pid; :\" &&\n\t(\n\t\ttest_write_lines \"info $sha1\" fflush \"info $sha1\" &&\n\t\t# TODO - consume all available input, not just one\n\t\t# line (see above).\n\t\tread actual <&9 &&\n\t\techo \"$actual\" >actual &&\n\t\techo \"$sha1 $type $size\" >expect &&\n\t\ttest_cmp expect actual\n\t) >input &&\n\t# check output is flushed on exit\n\tread actual <&9 &&\n\techo \"$actual\" >actual &&\n\ttest_cmp expect actual &&\n\ttest_must_be_empty err &&\n\tread status <&9 &&\n\ttest \"$status\" -eq 0\n}\n\nrun_buffer_test_no_flush () {\n\ttype=$1\n\tsha1=$2\n\tsize=$3\n\n\tmkfifo input &&\n\ttest_when_finished 'rm input'\n\tmkfifo pid &&\n\texec 9<>pid &&\n\ttest_when_finished 'rm pid; exec 9<&-'\n\t(\n\t\tgit cat-file --buffer --batch-command <input >output &\n\t\techo $! &&\n\t\twait $!\n\t\techo $?\n\t) >&9 &\n\tsh_pid=$! &&\n\tread cat_file_pid <&9 &&\n\ttest_when_finished \"kill $cat_file_pid\n\t\t\t    kill $sh_pid; wait $sh_pid; :\" &&\n\t(\n\t\ttest_write_lines \"info $sha1\" \"info $sha1\" &&\n\t\tkill $cat_file_pid &&\n\t\tread status <&9 &&\n\t\ttest \"$status\" -ne 0 &&\n\t\ttest_must_be_empty output\n\t) >input\n}\n\nBest Wishes\n\nPhillip\n\n> +    test -z \"$content\" ||\n> +    test_expect_success \"--batch-command output of $type content is correct\" '\n> +\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n> +\tmaybe_remove_timestamp \"$(echo object $sha1 | git cat-file --batch-command)\" $no_ts >actual &&\n> +\ttest_cmp expect actual\n> +    '\n> +\n> +    test_expect_success \"batch-command output of $type info is correct\" '\n> +\techo \"$sha1 $type $size\" >expect &&\n> +\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n> +\ttest_cmp expect actual\n> +'\n>       test_expect_success \"custom --batch-check format\" '\n>   \techo \"$type $sha1\" >expect &&\n>   \techo $sha1 | git cat-file --batch-check=\"%(objecttype) %(objectname)\" >actual &&\n> @@ -232,12 +284,29 @@ test_expect_success '--batch-check without %(rest) considers whole line' '\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success '--batch-command --buffer with flush is correct for blob' '\n> +\trun_buffer_test 'blob' $hello_sha1 $hello_size true\n> +'\n> +\n> +test_expect_success '--batch-command --buffer without flush is correct for blob' '\n> +\trun_buffer_test 'blob' $hello_sha1 $hello_size false\n> +'\n> +\n>   tree_sha1=$(git write-tree)\n> +\n>   tree_size=$(($(test_oid rawsz) + 13))\n>   tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n>   \n>   run_tests 'tree' $tree_sha1 $tree_size \"\" \"$tree_pretty_content\"\n>   \n> +test_expect_success '--batch-command --buffer with flush is correct for tree' '\n> +\trun_buffer_test 'tree' $tree_sha1 $tree_size true\n> +'\n> +\n> +test_expect_success '--batch-command --buffer without flush is correct for tree' '\n> +\trun_buffer_test 'tree' $tree_sha1 $tree_size false\n> +'\n> +\n>   commit_message=\"Initial commit\"\n>   commit_sha1=$(echo_without_newline \"$commit_message\" | git commit-tree $tree_sha1)\n>   commit_size=$(($(test_oid hexsz) + 137))\n> @@ -263,6 +332,14 @@ tag_size=$(strlen \"$tag_content\")\n>   \n>   run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n>   \n> +test_expect_success '--batch-command --buffer with flush is correct for tag' '\n> +\trun_buffer_test 'tag' $tag_sha1 $tag_size true\n> +'\n> +\n> +test_expect_success '--batch-command --buffer without flush is correct for tag' '\n> +\trun_buffer_test 'tag' $tag_sha1 $tag_size false\n> +'\n> +\n>   test_expect_success \\\n>       \"Reach a blob from a tag pointing to it\" \\\n>       \"test '$hello_content' = \\\"\\$(git cat-file blob $tag_sha1)\\\"\"\n> @@ -964,4 +1041,10 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success 'batch-command unknown command' '\n> +\techo unknown_command >cmd &&\n> +\ttest_expect_code 128 git cat-file --batch-command < cmd 2>err &&\n> +\tgrep -E \"^fatal:.*unknown command.*\" err\n> +'\n> +\n>   test_done\n\n"},{"id":"447436","messageId":"YflzZsb/txsopusP@nand.local","threadId":"57333","inReplyTo":"CAP8UFD3sHvA3Gx9+d=VjQ11sEqWF47AEeo-m4bGsVO3OUJ4dLw@mail.gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-01T17:52:38Z","receivedAt":"2022-02-01T17:52:42Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 01, 2022 at 10:39:30AM +0100, Christian Couder wrote:\n> Also I think at this point this should probably not be an RFC patch\n> anymore but a regular one.\n\nI think that this is due to my first \"review\" in [1], where I tried to\nget an understanding of what John's concrete usage plans were, since I\ncouldn't figure out what they were on my own.\n\nI'm not sure that I've seen a response along the lines of \"we need to\ncontrol when the output stream is flushed in order to do ...\" yet, but I\nwould be interested to see one before moving too much further ahead of\nwhere we already are.\n\n(Apologies if such a response was written, and I missed it).\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/YehomwNiIs0l83W7@nand.local/\n"},{"id":"447448","messageId":"674CD40F-7060-4892-97CD-1940AEF7C3D7@gmail.com","threadId":"57333","inReplyTo":"YflzZsb/txsopusP@nand.local","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-01T19:27:54Z","receivedAt":"2022-02-01T19:27:58Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Taylor,\n\nOn 1 Feb 2022, at 12:52, Taylor Blau wrote:\n\n> On Tue, Feb 01, 2022 at 10:39:30AM +0100, Christian Couder wrote:\n>> Also I think at this point this should probably not be an RFC patch\n>> anymore but a regular one.\n>\n> I think that this is due to my first \"review\" in [1], where I tried to\n> get an understanding of what John's concrete usage plans were, since I\n> couldn't figure out what they were on my own.\n>\n> I'm not sure that I've seen a response along the lines of \"we need to\n> control when the output stream is flushed in order to do ...\" yet, but I\n> would be interested to see one before moving too much further ahead of\n> where we already are.\n\nThis would be useful when there is another process A interacting with a long\nrunning git cat-file process B that is retrieving object information from the odb\ninteractively but also wants to use --buffer mode.\n\nIn this scenario, if A is asked to retrieve a large list of object metadata, it wants to\nuse --buffer mode to be more efficient but it would need a way to ensure that all of the\ncontents have been flushed to the output. If we want to keep B running to save startup time\n(since otherwise we might need to spawn many git cat-file processes), then having\na flush command where we can guarantee a flush would be very handy.\n\n>\n> (Apologies if such a response was written, and I missed it).\n\nNope, don't think I explained the need for a flush command very clearly.\n\nthanks\n>\n> Thanks,\n> Taylor\n>\n> [1]: https://lore.kernel.org/git/YehomwNiIs0l83W7@nand.local/\n"},{"id":"447455","messageId":"YfmUiXJ//ZC5NZU+@nand.local","threadId":"57333","inReplyTo":"674CD40F-7060-4892-97CD-1940AEF7C3D7@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-01T20:14:01Z","receivedAt":"2022-02-01T20:14:10Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 01, 2022 at 02:27:54PM -0500, John Cai wrote:\n> On 1 Feb 2022, at 12:52, Taylor Blau wrote:\n> > I'm not sure that I've seen a response along the lines of \"we need to\n> > control when the output stream is flushed in order to do ...\" yet, but I\n> > would be interested to see one before moving too much further ahead of\n> > where we already are.\n>\n> This would be useful when there is another process A interacting with\n> a long running git cat-file process B that is retrieving object\n> information from the odb interactively but also wants to use --buffer\n> mode.\n\nLet me try and repeat my understanding of what you said to make sure\nthat I fully grok the use-case you have in mind.\n\nYou have a repository and want to have a long-running `git cat-file`\nprocess that can serve multiple requests. Because the processes which\ninteract with your long-running `cat-file` may ask for many objects, you\ndon't want to flush the output buffer after each object, and so would\nideally like to use `--buffer`.\n\nBut that doesn't quite work, since the `cat-file` process may not have\ndecided to flush its output buffer even when process A is about to go\naway.\n\nI wonder about the viability of accomplishing this via a signal handler,\ni.e., that `cat-file` would call fflush(2) whenever it receives e.g.,\nSIGUSR1. A couple of possible downsides:\n\n  - SIGUSR1 doesn't exist on Windows AFAIK.\n\n  - There are definitely going to be synchrony issues to contend with.\n    What happens if we receive our signal while writing to the output\n    stream? I think you would just need to mark a variable that\n    indicates we should flush after finishing serving the current\n    request, but I haven't thought too hard about it.\n\nSo maybe a signal isn't the way to go. But I don't think `--stdin-cmd`\nis the simplest approach either. At the very least, I don't totally\nunderstand your plan after implementing a flush command. You mention\nthat it would be nice to implement other commands, but I'm not totally\nconvinced by your examples[1].\n\nI wonder if we could strike a middle ground, which might look like `git\ncat-file --batch --buffer`, and just feeding it something which we know\nfor certain isn't an object identifier. In other words, what if we\ndid something as simple as:\n\n--- >8 ---\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d94050e6c1..bae162fc18 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -595,6 +595,11 @@ static int batch_objects(struct batch_options *opt)\n \twarn_on_object_refname_ambiguity = 0;\n\n \twhile (strbuf_getline(&input, stdin) != EOF) {\n+\t\tif (!strcmp(\"<flush>\", input.buf)) {\n+\t\t\tfflush(stdout);\n+\t\t\tcontinue;\n+\t\t}\n+\n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\n \t\t\t * Split at first whitespace, tying off the beginning\n\n--- 8< ---\n\nOn the other hand, something even hackier than the above is that we\nflush stdout whenever we get a request to print an object which could\nnot be found. So if you feed a single \"\\n\" to your `cat-file` process,\nyou'll get \" missing\" on its output, and the buffer will immediately be\nflushed.\n\nI'm not sure that I'd recommend relying on that behavior exactly, but if\nyou're looking for a short-term solution, it might work ;).\n\nThanks,\nTaylor\n\n[1]: One that comes to mind is changing the output format mid-stream.\n     But how often does it really make sense to change the output\n     format? I can understand wanting to flush at the end asking\n     cat-file for a bunch of objects, but I don't see how you would want\n     to change the output format often enough that shaving off Git's\n     negligible startup cost is worthwhile (or couldn't be accomplished\n     by just spawning another cat-file process and using that).\n"},{"id":"447484","messageId":"YfniNw/PoBeei+33@nand.local","threadId":"57333","inReplyTo":"3FE1D509-8AD0-4F0E-9298-DFD3552A98EF@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-02-02T01:45:27Z","receivedAt":"2022-02-02T01:45:31Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Feb 01, 2022 at 08:34:06PM -0500, John Cai wrote:\n> > So maybe a signal isn't the way to go. But I don't think `--stdin-cmd`\n> > is the simplest approach either. At the very least, I don't totally\n> > understand your plan after implementing a flush command. You mention\n> > that it would be nice to implement other commands, but I'm not totally\n> > convinced by your examples[1].\n>\n> I agree that if flush were the only use case for a new flag, it might not\n> be worth it. But, the flush command is only one of the use cases that a\n> --stdin-cmd (now changed to --batch-command per Phillip's suggestion)\n>\n> There are two other commands in this RFC\n>\n> object <rev>\n> info <rev>\n>\n> These are described in [1] that are also a motivation for a command\n> interface.\n> The description in [1] explains why a command interface would be necessary.\n\nThis seems like a more realistic and well-motivated proposal, IMHO. I am\na little curious that having the ability to switch between asking for an\nobject's contents and its metadata would lead to saving thousands of\nprocesses per server.\n\n(Apropos of this series, I am curious: how long does GitLab keep a pair\nof cat-file programs running for each repository?)\n\nIn any case, I'll take a closer look over the aforementioned version and\nlet you know what my thoughts are, probably tomorrow.\n\n> 1. https://lore.kernel.org/git/20220128183319.43496-1-johncai86@gmail.com/\n\nThanks,\nTaylor\n"},{"id":"447539","messageId":"A17C623A-D6F7-4300-AE72-64DDE49A5DAA@gmail.com","threadId":"57333","inReplyTo":"CAP8UFD3sHvA3Gx9+d=VjQ11sEqWF47AEeo-m4bGsVO3OUJ4dLw@mail.gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-02T13:11:22Z","receivedAt":"2022-02-02T13:11:27Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Christian\n\nOn 1 Feb 2022, at 4:39, Christian Couder wrote:\n\n> On Mon, Jan 31, 2022 at 9:34 PM John Cai <johncai86@gmail.com> wrote:\n>>\n>> This RFC patch proposes a new flag --batch-command that works with\n>> git-cat-file --batch.\n>\n> The subject is \"Re: [RFC v3] cat-file: add a --stdin-cmd mode\" and now\n> you are talking about '--batch-command' instead of '--stdin-cmd'.\n>\n> \"that works with git-cat-file --batch\" is not very clear. Maybe you\n> could find a wording that explains better how --batch-command is\n> different from --batch.\n>\n> Also I think at this point this should probably not be an RFC patch\n> anymore but a regular one.\n>\n>> Similar to git-update-ref --stdin, it will accept\n>> commands and arguments from stdin.\n>>\n>> The start of this idea was discussed in [1], where the original\n>> motivation was to be able to control when the buffer was flushed to\n>> stdout in --buffer mode.\n>\n> That would be nice in a cover letter but I am not sure a commit\n> message is the right place for this.\n>\n>> However, this can actually be much more useful in situations when\n>> git-cat-file --batch is being used as a long lived backend query\n>> process. At GitLab, we use a pair of cat-file processes. One for\n>> iterating over object metadata with --batch-check, and the other to grab\n>> object contents with --batch. However, if we had --batch-command, we could\n>> get rid of the second --batch-check process,\n>\n> Maybe s/second// would make it clear that there are no two\n> --batch-command processes.\n>\n>> and just have one process\n>> where we can flip between getting object info, and getting object contents.\n>> This can lead to huge savings since on a given server there could be hundreds to\n>> thousands of git cat-file processes at a time.\n>\n> It's not clear if all the git cat-file processes you are talking about\n> are mostly --batch-check processes or --batch processes, or a roughly\n> equal amount of both. My guess is the latter and that --batch-command\n> would mean that there would be around two times fewer cat-file\n> processes.\n>\n>> git cat-file --batch-command\n>>\n>> $ <command> [arg1] [arg2] NL\n>\n> It's a bit unclear what the 2 above lines mean. Maybe you could add a\n> small explanation like for example \"The new flag can be used like\n> this:\" and \"It receives commands from stdin in the format:\"\n>\n> Also not sure why there is a '$' char in front of '<command> [arg1]\n> [arg2] NL' but not in front of 'git cat-file --batch-command'. It\n> doesn't look like in the 'git update-ref --stdin' doc that '$' are\n> used in front of the commands that can be passed through stdin.\n>\n>> This patch adds three commands: object, info, fflush\n>\n> Maybe s/three commands/the following first three commands/\n>\n>> $ object <sha1> NL\n>> $ info <sha1> NL\n>> $ fflush NL\n>\n> Idem about '$'.\n>\n>> These three would be immediately useful in GitLab's context, but one can\n>> imagine this mode to be further extended for other things.\n>\n> Not very clear which \"mode\" you are talking about. You have been\n> talking about a mode only in the subject so far. Maybe you should talk\n> a bit about that above when '<command> [arg1] [arg2] NL' is\n> introduced.\n>\n> Also you don't talk about the output format. --batch and --batch-check\n> accept [=<format>], but it looks like --batch-command doesn't.\n>\n>> Future improvements:\n>> - a non-trivial part of \"cat-file --batch\" time is spent\n>> on parsing its argument and seeing if it's a revision, ref etc. So we\n>> could add a command that only accepts a full-length 40\n>> character SHA-1.\n>\n> In a cover letter that would be ok, but I am not sure that a commit\n> message is the best place for this kind of details about future work.\n>\n>> This would be the first step in adding such an interface to\n>> git-cat-file.\n>>\n>> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n>>\n>> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> Signed-off-by: John Cai <johncai86@gmail.com>\n>> ---\n>>\n>> Taylor, I'd be interested in your thoughts on this proposal since you helped\n>> review the previous patch that turned into this RFC. Thanks!\n>>\n>> Changes from v2:\n>>\n>> - refactored tests to be within run_tests()\n>> - added a test to test --buffer behavior with fflush\n>> - cleaned up cat-file.c: clarified var names, removed unnecessary code\n>>   based on suggestions from Phillip Wood\n>> - removed strvec changes\n>>\n>> Changes from v1:\n>>\n>> - changed option name to batch-command.\n>> - changed command function interface to receive the whole line after the command\n>>   name to put the onus of parsing arguments to each individual command function.\n>> - pass in whole line to batch_one_object in both parse_cmd_object and\n>>   parse_cmd_info to support spaces in the object reference.\n>> - removed addition of -z to include in a separate patch series\n>> - added documentation.\n>> ---\n>>  Documentation/git-cat-file.txt |  15 +++++\n>>  builtin/cat-file.c             | 114 +++++++++++++++++++++++++++++++--\n>>  t/t1006-cat-file.sh            |  83 ++++++++++++++++++++++++\n>>  3 files changed, 205 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n>> index bef76f4dd0..254e546c79 100644\n>> --- a/Documentation/git-cat-file.txt\n>> +++ b/Documentation/git-cat-file.txt\n>> @@ -96,6 +96,21 @@ OPTIONS\n>>         need to specify the path, separated by whitespace.  See the\n>>         section `BATCH OUTPUT` below for details.\n>>\n>> +--batch-command::\n>> +       Enter a command mode that reads from stdin.\n>\n> Maybe s/a command mode that reads from stdin/a mode that reads\n> commands from stdin/\n>\n> Also I would expect something about the output, like perhaps \"...and\n> ouputs the command results to stdout\".\n>\n>> May not be combined with any\n>> +       other options or arguments except `--textconv` or `--filters`, in which\n>> +       case the input lines also need to specify the path, separated by\n>> +       whitespace.  See the section `BATCH OUTPUT` below for details.\n>\n> The BATCH OUTPUT section says that a format can be passed but that\n> doesn't seem to be the case with --batch-command. So you might need to\n> make some changes to that section too or add a bit more details about\n> the output here.\n\nThinking about this more, I wonder if it'd be worth it to allow the --batch-command=<format>\nfor backwards compatibility reasons for users who switch over to using --batch-command\nfrom --batch and --batch-check.\n\nThe only thing that makes me hesitant is that the <format> would only be relevant to\nthe \"info\" and \"object\" commands instead of being relevant to all commands in --batch-command\nmode.\n\n>\n>> +object <object>::\n>> +       Print object contents for object reference <object>\n>> +\n>> +info <object>::\n>> +       Print object info for object reference <object>\n>> +\n>> +flush::\n>> +       Flush to stdout immediately when used with --buffer\n>> +\n>>  --batch-all-objects::\n>>         Instead of reading a list of objects on stdin, perform the\n>>         requested batch operation on all objects in the repository and\n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index 7b3f42950e..cc9e47943b 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -24,9 +24,11 @@ struct batch_options {\n>>         int buffer_output;\n>>         int all_objects;\n>>         int unordered;\n>> -       int cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n>> +       int mode; /* may be 'w' or 'c' for --filters or --textconv */\n>>         const char *format;\n>> +       int command;\n>>  };\n>\n> Maybe add a blank line here.\n>\n>> +static char line_termination = '\\n';\n>>\n>>  static const char *force_path;\n>>\n>> @@ -302,19 +304,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n>>         if (data->type == OBJ_BLOB) {\n>>                 if (opt->buffer_output)\n>>                         fflush(stdout);\n>> -               if (opt->cmdmode) {\n>> +               if (opt->mode) {\n>\n> The mechanical s/cmdmode/mode/g change could have been made in a\n> preparatory patch to make this patch a bit smaller and easier to\n> digest.\n>\n>> +static void batch_objects_command(struct batch_options *opt,\n>> +                                   struct strbuf *output,\n>> +                                   struct expand_data *data)\n>> +{\n>> +       struct strbuf input = STRBUF_INIT;\n>> +\n>> +       /* Read each line dispatch its command */\n>> +       while (!strbuf_getwholeline(&input, stdin, line_termination)) {\n>> +               int i;\n>> +               const struct parse_cmd *cmd = NULL;\n>> +               const char *p, *cmd_end;\n>> +\n>> +               if (*input.buf == line_termination)\n>> +                       die(\"empty command in input\");\n>> +               else if (isspace(*input.buf))\n>> +                       die(\"whitespace before command: %s\", input.buf);\n>> +\n>> +               for (i = 0; i < ARRAY_SIZE(commands); i++) {\n>> +                       const char *prefix = commands[i].prefix;\n>> +                       char c;\n>> +                       if (!skip_prefix(input.buf, prefix, &cmd_end))\n>> +                               continue;\n>> +                       /*\n>> +                        * If the command has arguments, verify that it's\n>> +                        * followed by a space. Otherwise, it shall be followed\n>> +                        * by a line terminator.\n>> +                        */\n>> +                       c = commands[i].takes_args ? ' ' : line_termination;\n>> +                       if (input.buf[strlen(prefix)] != c)\n>> +                               die(\"arguments invalid for command: %s\", commands[i].prefix);\n>> +\n>> +                       cmd = &commands[i];\n>> +                       if (cmd->takes_args) {\n>> +                               p = cmd_end + 1;\n>> +                               // strip newline before handing it to the\n>> +                               // handling function\n>\n> So above the /* */ comments delimiters are used but here // is used. I\n> am not sure we support // these days, but if we do, I think it would\n> be better to avoid mixing comment styles in the same function.\n>\n>> +                               input.buf[strcspn(input.buf, \"\\n\")] = '\\0';\n>> +                       }\n>> +\n>> +                       break;\n>> +               }\n>> +\n>> +               if (!cmd)\n>> +                       die(\"unknown command: %s\", input.buf);\n>> +\n>> +               cmd->fn(opt, p, output, data);\n>> +       }\n>> +       strbuf_release(&input);\n>> +}\n>\n>> @@ -590,6 +682,9 @@ static int batch_objects(struct batch_options *opt)\n>>         save_warning = warn_on_object_refname_ambiguity;\n>>         warn_on_object_refname_ambiguity = 0;\n>>\n>> +       if (command)\n>> +               batch_objects_command(opt, &output, &data);\n>> +\n>>         while (strbuf_getline(&input, stdin) != EOF) {\n>\n> I think batch_objects_command() will consume everything from stdin, so\n> it doesn't make sense to try to read again from stdin after it. Maybe\n> the whole while (...) { ... } clause should be inside an else clause\n> or something.\n>\n>>                 if (data.split_on_whitespace) {\n>>                         /*\n>> @@ -636,6 +731,7 @@ static int batch_option_callback(const struct option *opt,\n>>\n>>         bo->enabled = 1;\n>>         bo->print_contents = !strcmp(opt->long_name, \"batch\");\n>> +       bo->command = !strcmp(opt->long_name, \"batch-command\");\n>>         bo->format = arg;\n>>\n>>         return 0;\n"},{"id":"447543","messageId":"20D11066-BFC5-4888-B15B-66FE4EC1A050@gmail.com","threadId":"57333","inReplyTo":"e75ba9ea-fdda-6e9f-4dd6-24190117d93b@gmail.com","subject":"Re: [RFC v3] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-02-02T15:05:24Z","receivedAt":"2022-02-02T17:15:12Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Phillip\n\nOn 1 Feb 2022, at 5:43, Phillip Wood wrote:\n\n> Hi John\n>\n> On 28/01/2022 18:33, John Cai wrote:\n>> This RFC patch proposes a new flag --batch-command that works with\n>> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept\n>> commands and arguments from stdin.\n>>\n>> The start of this idea was discussed in [1], where the original\n>> motivation was to be able to control when the buffer was flushed to\n>> stdout in --buffer mode.\n>>\n>> However, this can actually be much more useful in situations when\n>> git-cat-file --batch is being used as a long lived backend query\n>> process. At GitLab, we use a pair of cat-file processes. One for\n>> iterating over object metadata with --batch-check, and the other to grab\n>> object contents with --batch. However, if we had --batch-command, we could\n>> get rid of the second --batch-check process, and just have one process\n>> where we can flip between getting object info, and getting object contents.\n>> This can lead to huge savings since on a given server there could be hundreds to\n>> thousands of git cat-file processes at a time.\n>>\n>> git cat-file --batch-command\n>>\n>> $ <command> [arg1] [arg2] NL\n>>\n>> This patch adds three commands: object, info, fflush\n>>\n>> $ object <sha1> NL\n>> $ info <sha1> NL\n>> $ fflush NL\n>>\n>> These three would be immediately useful in GitLab's context, but one can\n>> imagine this mode to be further extended for other things.\n>\n> I think this is moving in the right direction, I've left some comments below.\n>\n>> Future improvements:\n>> - a non-trivial part of \"cat-file --batch\" time is spent\n>> on parsing its argument and seeing if it's a revision, ref etc. So we\n>> could add a command that only accepts a full-length 40\n>> character SHA-1.\n>>\n>> This would be the first step in adding such an interface to\n>> git-cat-file.\n>>\n>> [1] https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n>>\n>> Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> Signed-off-by: John Cai <johncai86@gmail.com>\n>> ---\n>>\n>> Taylor, I'd be interested in your thoughts on this proposal since you helped\n>> review the previous patch that turned into this RFC. Thanks!\n>>\n>> Changes from v2:\n>>\n>> - refactored tests to be within run_tests()\n>> - added a test to test --buffer behavior with fflush\n>> - cleaned up cat-file.c: clarified var names, removed unnecessary code\n>>    based on suggestions from Phillip Wood\n>> - removed strvec changes\n>>\n>> Changes from v1:\n>>\n>> - changed option name to batch-command.\n>> - changed command function interface to receive the whole line after the command\n>>    name to put the onus of parsing arguments to each individual command function.\n>> - pass in whole line to batch_one_object in both parse_cmd_object and\n>>    parse_cmd_info to support spaces in the object reference.\n>> - removed addition of -z to include in a separate patch series\n>> - added documentation.\n>> ---\n>>   Documentation/git-cat-file.txt |  15 +++++\n>>   builtin/cat-file.c             | 114 +++++++++++++++++++++++++++++++--\n>>   t/t1006-cat-file.sh            |  83 ++++++++++++++++++++++++\n>>   3 files changed, 205 insertions(+), 7 deletions(-)\n>>\n>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n>> index bef76f4dd0..254e546c79 100644\n>> --- a/Documentation/git-cat-file.txt\n>> +++ b/Documentation/git-cat-file.txt\n>> @@ -96,6 +96,21 @@ OPTIONS\n>>   \tneed to specify the path, separated by whitespace.  See the\n>>   \tsection `BATCH OUTPUT` below for details.\n>>  +--batch-command::\n>> +\tEnter a command mode that reads from stdin. May not be combined with any\n>> +\tother options or arguments except `--textconv` or `--filters`, in which\n>> +\tcase the input lines also need to specify the path, separated by\n>> +\twhitespace.  See the section `BATCH OUTPUT` below for details.\n>> +\n>> +object <object>::\n>> +\tPrint object contents for object reference <object>\n>> +\n>> +info <object>::\n>> +\tPrint object info for object reference <object>\n>> +\n>> +flush::\n>\n> I think this is a better name than fflush as calling fflush is just an implementation detail. I've been thinking about this and have some concerns with the current implementation as it allows the caller to force a flush but the output may be flushed at other times and so it does not allow for deadlock free reading and writing to cat-file from a single process. If instead we buffered the input lines and only processed then when we received a flush command it would be easy to read and write to cat-file from a single process without deadlocks. This would mean that callers would have to add flush commands to get any output before they closed the pipe writing to cat-file.\n\nTo address this concern, I wonder if instead of a flush command what we could do is to have --batch-command support \"read transactions\" similar to update-ref. eg:\n\ngit cat-file --batch-command\n\nbegin info\n<sha1>\n<sha1>\n<sha1>\n<sha1>\ncommit\n\nWe could queue up these references, and then get them all once \"commit\" is entered and call fflush().\n\n>\n>> +\tFlush to stdout immediately when used with --buffer\n>> +\n>>   --batch-all-objects::\n>>   \tInstead of reading a list of objects on stdin, perform the\n>>   \trequested batch operation on all objects in the repository and\n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index 7b3f42950e..cc9e47943b 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -24,9 +24,11 @@ struct batch_options {\n>>   \tint buffer_output;\n>>   \tint all_objects;\n>>   \tint unordered;\n>> -\tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n>> +\tint mode; /* may be 'w' or 'c' for --filters or --textconv */\n>\n> If you really need to rename  this variable then doing that in a separate preparatory patch would make this easier to review as it separates out two separate sets of changes.\n>\n>>   \tconst char *format;\n>> +\tint command;\n>\n> Can't we just call this batch_command and leave cmdmode alone? I don't know maybe cmdmode would be confused with having something to do with batch_command then.\n>\n>>   };\n>> +static char line_termination = '\\n';\n>\n> This seems unnecessary as it is the only permitted line ending\n>\n>>   static const char *force_path;\n>>  @@ -302,19 +304,19 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n>>   \tif (data->type == OBJ_BLOB) {\n>>   \t\tif (opt->buffer_output)\n>>   \t\t\tfflush(stdout);\n>> -\t\tif (opt->cmdmode) {\n>> +\t\tif (opt->mode) {\n>>   \t\t\tchar *contents;\n>>   \t\t\tunsigned long size;\n>>    \t\t\tif (!data->rest)\n>>   \t\t\t\tdie(\"missing path for '%s'\", oid_to_hex(oid));\n>>  -\t\t\tif (opt->cmdmode == 'w') {\n>> +\t\t\tif (opt->mode == 'w') {\n>>   \t\t\t\tif (filter_object(data->rest, 0100644, oid,\n>>   \t\t\t\t\t\t  &contents, &size))\n>>   \t\t\t\t\tdie(\"could not convert '%s' %s\",\n>>   \t\t\t\t\t    oid_to_hex(oid), data->rest);\n>> -\t\t\t} else if (opt->cmdmode == 'c') {\n>> +\t\t\t} else if (opt->mode == 'c') {\n>>   \t\t\t\tenum object_type type;\n>>   \t\t\t\tif (!textconv_object(the_repository,\n>>   \t\t\t\t\t\t     data->rest, 0100644, oid,\n>> @@ -326,7 +328,7 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n>>   \t\t\t\t\tdie(\"could not convert '%s' %s\",\n>>   \t\t\t\t\t    oid_to_hex(oid), data->rest);\n>>   \t\t\t} else\n>> -\t\t\t\tBUG(\"invalid cmdmode: %c\", opt->cmdmode);\n>> +\t\t\t\tBUG(\"invalid mode: %c\", opt->mode);\n>>   \t\t\tbatch_write(opt, contents, size);\n>>   \t\t\tfree(contents);\n>>   \t\t} else {\n>> @@ -508,6 +510,95 @@ static int batch_unordered_packed(const struct object_id *oid,\n>>   \t\t\t\t      data);\n>>   }\n>>  +static void parse_cmd_object(struct batch_options *opt,\n>> +\t\t\t     const char *line,\n>> +\t\t\t     struct strbuf *output,\n>> +\t\t\t     struct expand_data *data)\n>> +{\n>> +\topt->print_contents = 1;\n>> +\tbatch_one_object(line, output, opt, data);\n>> +}\n>> +\n>> +static void parse_cmd_info(struct batch_options *opt,\n>> +\t\t\t   const char *line,\n>> +\t\t\t   struct strbuf *output,\n>> +\t\t\t   struct expand_data *data)\n>> +{\n>> +\topt->print_contents = 0;\n>> +\tbatch_one_object(line, output, opt, data);\n>> +}\n>> +\n>> +static void parse_cmd_fflush(struct batch_options *opt,\n>> +\t\t\t     const char *line,\n>> +\t\t\t     struct strbuf *output,\n>> +\t\t\t     struct expand_data *data)\n>> +{\n>> +\tfflush(stdout);\n>> +}\n>> +\n>> +typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,\n>> +\t\t\t       struct strbuf *, struct expand_data *);\n>> +\n>> +static const struct parse_cmd {\n>> +\tconst char *prefix;\n>> +\tparse_cmd_fn_t fn;\n>> +\tunsigned takes_args;\n>> +} commands[] = {\n>> +\t{ \"object\", parse_cmd_object, 1},\n>> +\t{ \"info\", parse_cmd_info, 1},\n>> +\t{ \"fflush\", parse_cmd_fflush, 0},\n>> +};\n>> +\n>> +static void batch_objects_command(struct batch_options *opt,\n>> +\t\t\t\t    struct strbuf *output,\n>> +\t\t\t\t    struct expand_data *data)\n>> +{\n>> +\tstruct strbuf input = STRBUF_INIT;\n>> +\n>> +\t/* Read each line dispatch its command */\n>> +\twhile (!strbuf_getwholeline(&input, stdin, line_termination)) {\n>> +\t\tint i;\n>> +\t\tconst struct parse_cmd *cmd = NULL;\n>> +\t\tconst char *p, *cmd_end;\n>> +\n>> +\t\tif (*input.buf == line_termination)\n>> +\t\t\tdie(\"empty command in input\");\n>> +\t\telse if (isspace(*input.buf))\n>> +\t\t\tdie(\"whitespace before command: %s\", input.buf);\n>> +\n>> +\t\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n>> +\t\t\tconst char *prefix = commands[i].prefix;\n>> +\t\t\tchar c;\n>> +\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_end))\n>> +\t\t\t\tcontinue;\n>> +\t\t\t/*\n>> +\t\t\t * If the command has arguments, verify that it's\n>> +\t\t\t * followed by a space. Otherwise, it shall be followed\n>> +\t\t\t * by a line terminator.\n>> +\t\t\t */\n>> +\t\t\tc = commands[i].takes_args ? ' ' : line_termination;\n>> +\t\t\tif (input.buf[strlen(prefix)] != c)\n>\n> As I pointed out in the last round you can use cmd_end here rather than calling strlen. I've just noticed that this will fail for a command that does not take arguments when the last input line does not end with '\\n'. I think\n>     if (*cmd_end && *cmd_end != c)\n> would be safer\n>\n>> +\t\t\t\tdie(\"arguments invalid for command: %s\", commands[i].prefix);\n>> +\n>> +\t\t\tcmd = &commands[i];\n>> +\t\t\tif (cmd->takes_args) {\n>> +\t\t\t\tp = cmd_end + 1;\n>> +\t\t\t\t// strip newline before handing it to the\n>> +\t\t\t\t// handling function\n>> +\t\t\t\tinput.buf[strcspn(input.buf, \"\\n\")] = '\\0';\n>\n> When I suggested replacing strstr() last time I failed to notice that we  know that if there is a newline character it is at input.buf[len - 1] so we don't need to scan the whole string to find it.\n>\n>     if (input.buf[input.len - 1] == '\\n')\n>     \tinput.buf[--buf.len] = '\\0';\n>\n> (feel free to change it to avoid the prefix operator)\n>\n>> +\t\t\t}\n>> +\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\n>> +\t\tif (!cmd)\n>> +\t\t\tdie(\"unknown command: %s\", input.buf);\n>> +\n>> +\t\tcmd->fn(opt, p, output, data);\n>> +\t}\n>> +\tstrbuf_release(&input);\n>> +}\n>> +\n>>   static int batch_objects(struct batch_options *opt)\n>>   {\n>>   \tstruct strbuf input = STRBUF_INIT;\n>> @@ -515,6 +606,7 @@ static int batch_objects(struct batch_options *opt)\n>>   \tstruct expand_data data;\n>>   \tint save_warning;\n>>   \tint retval = 0;\n>> +\tconst int command = opt->command;\n>>    \tif (!opt->format)\n>>   \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n>> @@ -529,7 +621,7 @@ static int batch_objects(struct batch_options *opt)\n>>   \tstrbuf_expand(&output, opt->format, expand_format, &data);\n>>   \tdata.mark_query = 0;\n>>   \tstrbuf_release(&output);\n>> -\tif (opt->cmdmode)\n>> +\tif (opt->mode)\n>>   \t\tdata.split_on_whitespace = 1;\n>>    \t/*\n>> @@ -590,6 +682,9 @@ static int batch_objects(struct batch_options *opt)\n>>   \tsave_warning = warn_on_object_refname_ambiguity;\n>>   \twarn_on_object_refname_ambiguity = 0;\n>>  +\tif (command)\n>> +\t\tbatch_objects_command(opt, &output, &data);\n>> +\n>\n> Moving this is good as we no longer need to touch the line below\n>\n>>   \twhile (strbuf_getline(&input, stdin) != EOF) {\n>>   \t\tif (data.split_on_whitespace) {\n>>   \t\t\t/*\n>> @@ -636,6 +731,7 @@ static int batch_option_callback(const struct option *opt,\n>>    \tbo->enabled = 1;\n>>   \tbo->print_contents = !strcmp(opt->long_name, \"batch\");\n>> +\tbo->command = !strcmp(opt->long_name, \"batch-command\");\n>>   \tbo->format = arg;\n>>    \treturn 0;\n>> @@ -683,6 +779,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>>   \t\t\tN_(\"like --batch, but don't emit <contents>\"),\n>>   \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n>>   \t\t\tbatch_option_callback),\n>> +\t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"\"),\n>> +\t\t\t N_(\"enters batch mode that accepts commands\"),\n>> +\t\t\t PARSE_OPT_NOARG | PARSE_OPT_NONEG,\n>> +\t\t\t batch_option_callback),\n>>   \t\tOPT_CMDMODE(0, \"batch-all-objects\", &opt,\n>>   \t\t\t    N_(\"with --batch[-check]: ignores stdin, batches all known objects\"), 'b'),\n>>   \t\t/* Batch-specific options */\n>> @@ -742,7 +842,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>>   \t/* Return early if we're in batch mode? */\n>>   \tif (batch.enabled) {\n>>   \t\tif (opt_cw)\n>> -\t\t\tbatch.cmdmode = opt;\n>> +\t\t\tbatch.mode = opt;\n>>   \t\telse if (opt && opt != 'b')\n>>   \t\t\tusage_msg_optf(_(\"'-%c' is incompatible with batch mode\"),\n>>   \t\t\t\t       usage, options, opt);\n>> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n>> index 145eee11df..92f0b14a95 100755\n>> --- a/t/t1006-cat-file.sh\n>> +++ b/t/t1006-cat-file.sh\n>> @@ -112,6 +112,46 @@ maybe_remove_timestamp () {\n>>       fi\n>>   }\n>\n> It's really good that you've found a way to test fflush\n>\n>> +run_buffer_test () {\n>> +\ttype=$1\n>> +\tsha1=$2\n>> +\tsize=$3\n>> +\tflush=$4\n>> +\n>> +\tmkfifo V.input\n>> +\texec 8<>V.input\n>> +\trm V.input\n>> +\n>> +\tmkfifo V.output\n>> +\texec 9<>V.output\n>> +\trm V.output\n>> +\t\n>> +\t(\n>> +\t\tgit cat-file --buffer --batch-command <&8 >&9 &\n>> +\t\techo $! >&9 &&\n>> +\t\twait $!\n>> +\t\techo >&9 EARLY_EXIT\n>> +\t) &\n>> +\tsh_pid=$!\n>> +\tread fi_pid <&9 > +\ttest_when_finished \"\n>> +\t\texec 8>&-; exec 9>&-;\n>> +\t\tkill $sh_pid && wait $sh_pid\n>> +\t\tkill $fi_pid && wait $fi_pid\n>> +\t\ttrue\"\n>> +\texpect=$(echo \"$sha1 $type $size\")\n>> +\techo \"info $sha1\" >&8\n>> +\tif test \"$flush\" = \"true\"\n>> +\tthen\n>> +\t\techo \"fflush\" >&8\n>\n> If I change \"fflush\" to \"bad-command\" then this test still passes. This is because die() flushes stdout and the read below will get the output it was expecting and not 'EARLY_EXIT'. I have a suggested change below to get around that also checks the exit code of \"git cat-file\" (This test effectively puts a git command upstream of a pipe which we try to avoid in order to pick up any crashes)\n>\n>> +\telse\n>> +\t\texpect=\"EARLY_EXIT\"\n>> +\t\tkill $fi_pid && wait $fi_pid\n>> +\tfi\n>> +\tread actual <&9\n>> +\ttest \"$actual\" = \"$expect\"\n>> +}\n>> +\n>>   run_tests () {\n>>       type=$1\n>>       sha1=$2\n>> @@ -177,6 +217,18 @@ $content\"\n>>   \ttest_cmp expect actual\n>>       '\n>\n> Here is my suggestion based on your test - I ended up splitting the fflush and no-fflush cases\n>\n> run_buffer_test_flush () {\n>     type=$1\n>     sha1=$2\n>     size=$3\n>\n>     mkfifo input &&\n>     test_when_finished 'rm input'\n>     mkfifo output &&\n>     exec 9<>output &&\n>     test_when_finished 'rm output; exec 9<&-'\n>     (\n>         \t# TODO - Ideally we'd pipe the output of cat-file\n>         \t# through \"sed s'/$/\\\\/'\" to make sure that that read\n>         \t# would consume all the available\n>         \t# output. Unfortunately we cannot do this as we cannot\n>         \t# control when sed flushes its output. We could write\n>         \t# a test helper in C that appended a '\\' to the end of\n>     \t# each line and flushes its output after every line.\n>     \tgit cat-file --buffer --batch-command <input 2>err &\n>     \techo $! &&\n>     \twait $!\n>     \techo $?\n>     ) >&9 &\n>     sh_pid=$! &&\n>     read cat_file_pid <&9 &&\n>     test_when_finished \"kill $cat_file_pid\n>     \t\t    kill $sh_pid; wait $sh_pid; :\" &&\n>     (\n>     \ttest_write_lines \"info $sha1\" fflush \"info $sha1\" &&\n>     \t# TODO - consume all available input, not just one\n>     \t# line (see above).\n>     \tread actual <&9 &&\n>     \techo \"$actual\" >actual &&\n>     \techo \"$sha1 $type $size\" >expect &&\n>     \ttest_cmp expect actual\n>     ) >input &&\n>     # check output is flushed on exit\n>     read actual <&9 &&\n>     echo \"$actual\" >actual &&\n>     test_cmp expect actual &&\n>     test_must_be_empty err &&\n>     read status <&9 &&\n>     test \"$status\" -eq 0\n> }\n>\n> run_buffer_test_no_flush () {\n>     type=$1\n>     sha1=$2\n>     size=$3\n>\n>     mkfifo input &&\n>     test_when_finished 'rm input'\n>     mkfifo pid &&\n>     exec 9<>pid &&\n>     test_when_finished 'rm pid; exec 9<&-'\n>     (\n>     \tgit cat-file --buffer --batch-command <input >output &\n>     \techo $! &&\n>     \twait $!\n>     \techo $?\n>     ) >&9 &\n>     sh_pid=$! &&\n>     read cat_file_pid <&9 &&\n>     test_when_finished \"kill $cat_file_pid\n>     \t\t    kill $sh_pid; wait $sh_pid; :\" &&\n>     (\n>     \ttest_write_lines \"info $sha1\" \"info $sha1\" &&\n>     \tkill $cat_file_pid &&\n>     \tread status <&9 &&\n>     \ttest \"$status\" -ne 0 &&\n>     \ttest_must_be_empty output\n>     ) >input\n> }\n>\n> Best Wishes\n>\n> Phillip\n>\n>> +    test -z \"$content\" ||\n>> +    test_expect_success \"--batch-command output of $type content is correct\" '\n>> +\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n>> +\tmaybe_remove_timestamp \"$(echo object $sha1 | git cat-file --batch-command)\" $no_ts >actual &&\n>> +\ttest_cmp expect actual\n>> +    '\n>> +\n>> +    test_expect_success \"batch-command output of $type info is correct\" '\n>> +\techo \"$sha1 $type $size\" >expect &&\n>> +\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n>> +\ttest_cmp expect actual\n>> +'\n>>       test_expect_success \"custom --batch-check format\" '\n>>   \techo \"$type $sha1\" >expect &&\n>>   \techo $sha1 | git cat-file --batch-check=\"%(objecttype) %(objectname)\" >actual &&\n>> @@ -232,12 +284,29 @@ test_expect_success '--batch-check without %(rest) considers whole line' '\n>>   \ttest_cmp expect actual\n>>   '\n>>  +test_expect_success '--batch-command --buffer with flush is correct for blob' '\n>> +\trun_buffer_test 'blob' $hello_sha1 $hello_size true\n>> +'\n>> +\n>> +test_expect_success '--batch-command --buffer without flush is correct for blob' '\n>> +\trun_buffer_test 'blob' $hello_sha1 $hello_size false\n>> +'\n>> +\n>>   tree_sha1=$(git write-tree)\n>> +\n>>   tree_size=$(($(test_oid rawsz) + 13))\n>>   tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n>>    run_tests 'tree' $tree_sha1 $tree_size \"\" \"$tree_pretty_content\"\n>>  +test_expect_success '--batch-command --buffer with flush is correct for tree' '\n>> +\trun_buffer_test 'tree' $tree_sha1 $tree_size true\n>> +'\n>> +\n>> +test_expect_success '--batch-command --buffer without flush is correct for tree' '\n>> +\trun_buffer_test 'tree' $tree_sha1 $tree_size false\n>> +'\n>> +\n>>   commit_message=\"Initial commit\"\n>>   commit_sha1=$(echo_without_newline \"$commit_message\" | git commit-tree $tree_sha1)\n>>   commit_size=$(($(test_oid hexsz) + 137))\n>> @@ -263,6 +332,14 @@ tag_size=$(strlen \"$tag_content\")\n>>    run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n>>  +test_expect_success '--batch-command --buffer with flush is correct for tag' '\n>> +\trun_buffer_test 'tag' $tag_sha1 $tag_size true\n>> +'\n>> +\n>> +test_expect_success '--batch-command --buffer without flush is correct for tag' '\n>> +\trun_buffer_test 'tag' $tag_sha1 $tag_size false\n>> +'\n>> +\n>>   test_expect_success \\\n>>       \"Reach a blob from a tag pointing to it\" \\\n>>       \"test '$hello_content' = \\\"\\$(git cat-file blob $tag_sha1)\\\"\"\n>> @@ -964,4 +1041,10 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'\n>>   \ttest_cmp expect actual\n>>   '\n>>  +test_expect_success 'batch-command unknown command' '\n>> +\techo unknown_command >cmd &&\n>> +\ttest_expect_code 128 git cat-file --batch-command < cmd 2>err &&\n>> +\tgrep -E \"^fatal:.*unknown command.*\" err\n>> +'\n>> +\n>>   test_done\n"}]}