{"thread":{"id":"57310","subject":"[RFC v2] cat-file: add a --stdin-cmd mode","startedAt":"2022-01-25T22:51:03Z","lastAt":"2022-01-28T06:07:16Z","messageCount":6,"participants":["John Cai","Phillip Wood","Eric Wong"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"446892","messageId":"20220125225008.45944-1-johncai86@gmail.com","threadId":"57310","inReplyTo":null,"subject":"[RFC v2] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-25T22:50:08Z","receivedAt":"2022-01-25T22:51:03Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"This RFC patch proposes a new flag --stdin-cmd 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 --stdin-cmd, we could\nget rid of the second --batch-check process, and just have one progress\nwhere we can flip between getting object info, and getting object contents.\nThis can lead to huge savings.\n\ngit cat-file --batch --stdin-cmd\n\n$ <command> [arg1] [arg2] NL\n\nWe can also add a -z mode to allow for NUL-terminated lines\n\n$ <command> [arg1] [arg2] NUL\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\nFor instance, 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---\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 strvec.c                       |  23 +++++++\n strvec.h                       |   8 +++\n t/t1006-cat-file.sh            |  32 +++++++++\n 5 files changed, 191 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex bef76f4dd0..8aefa45e4c 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..30794284d5 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -16,6 +16,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n+#include \"strvec.h\"\n \n struct batch_options {\n \tint enabled;\n@@ -26,7 +27,10 @@ struct batch_options {\n \tint unordered;\n \tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n \tconst char *format;\n+\tint stdin_cmd;\n+\tint end_null;\n };\n+static char line_termination = '\\n';\n \n static const char *force_path;\n \n@@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,\n \t\t\t\t      data);\n }\n \n+enum batch_state {\n+\t/* Non-transactional state open for commands. */\n+\tBATCH_STATE_OPEN,\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 args;\n+\tenum batch_state state;\n+} commands[] = {\n+\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n+\t{ \"info\", parse_cmd_info, 1, BATCH_STATE_OPEN },\n+\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n+};\n+\n+static void batch_objects_stdin_cmd(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+\tenum batch_state state = BATCH_STATE_OPEN;\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;\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\tconst char *cmd_name;\n+\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_name))\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].args ? ' ' : line_termination;\n+\t\t\tif (input.buf[strlen(prefix)] != c)\n+\t\t\t\tcontinue;\n+\n+\t\t\tcmd = &commands[i];\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!cmd)\n+\t\t\tdie(\"unknown command: %s\", input.buf);\n+\n+\t\tp = input.buf + strlen(cmd->prefix) + 1;\n+\t\tconst char *pos = strstr(p, &line_termination);\n+\n+\t\tswitch (state) {\n+\t\tcase BATCH_STATE_OPEN:\n+\t\t\tbreak;\n+\t\t}\n+\t\tcmd->fn(opt, xstrndup(p, pos-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 +615,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 stdin_cmd = opt->stdin_cmd;\n \n \tif (!opt->format)\n \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n@@ -590,7 +691,8 @@ 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-\twhile (strbuf_getline(&input, stdin) != EOF) {\n+\twhile (!stdin_cmd &&\n+\t       strbuf_getline(&input, stdin) != EOF) {\n \t\tif (data.split_on_whitespace) {\n \t\t\t/*\n \t\t\t * Split at first whitespace, tying off the beginning\n@@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)\n \t\tbatch_one_object(input.buf, &output, opt, &data);\n \t}\n \n+\tif (stdin_cmd)\n+\t\tbatch_objects_stdin_cmd(opt, &output, &data);\n+\n \tstrbuf_release(&input);\n \tstrbuf_release(&output);\n \twarn_on_object_refname_ambiguity = save_warning;\n@@ -636,6 +741,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->stdin_cmd = !strcmp(opt->long_name, \"batch-command\");\n \tbo->format = arg;\n \n \treturn 0;\n@@ -683,6 +789,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@@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t/* Batch defaults */\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n+\tif (batch.end_null)\n+\t\tline_termination = '\\0';\n \n \t/* Return early if we're in batch mode? */\n \tif (batch.enabled) {\ndiff --git a/strvec.c b/strvec.c\nindex 61a76ce6cb..7dca04bf7a 100644\n--- a/strvec.c\n+++ b/strvec.c\n@@ -85,6 +85,29 @@ void strvec_split(struct strvec *array, const char *to_split)\n \t}\n }\n \n+size_t strvec_split_delim(struct strvec *array, const char *string,\n+\t\t\t  int delim, int maxsplit)\n+{\n+\tsize_t count = 0;\n+\tconst char *p = string, *end;\n+\n+\tfor (;;) {\n+\t\tcount++;\n+\t\tif (maxsplit >= 0 && count > maxsplit) {\n+\t\t\tstrvec_push(array, p);\n+\t\t\treturn count;\n+\t\t}\n+\t\tend = strchr(p, delim);\n+\t\tif (end) {\n+\t\t\tstrvec_push_nodup(array, xmemdupz(p, end - p));\n+\t\t\tp = end + 1;\n+\t\t} else {\n+\t\t\tstrvec_push(array, p);\n+\t\t\treturn count;\n+\t\t}\n+\t}\n+}\n+\n void strvec_clear(struct strvec *array)\n {\n \tif (array->v != empty_strvec) {\ndiff --git a/strvec.h b/strvec.h\nindex 9f55c8766b..c474918b91 100644\n--- a/strvec.h\n+++ b/strvec.h\n@@ -73,6 +73,14 @@ void strvec_pop(struct strvec *);\n /* Splits by whitespace; does not handle quoted arguments! */\n void strvec_split(struct strvec *, const char *);\n \n+/**\n+ * strvec_split_delim() is a split function that behaves more like its\n+ * string_list_split() cousin than the whitespace-splitting\n+ * strvec_split().\n+ */\n+size_t strvec_split_delim(struct strvec *array, const char *string,\n+\t\t\t  int delim, int maxsplit);\n+\n /**\n  * Free all memory associated with the array and return it to the\n  * initial, empty state.\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 145eee11df..935ab1cd34 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -964,4 +964,36 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'\n \ttest_cmp expect actual\n '\n \n+F='%s\\0'\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_expect_success 'setup object data' '\n+\tcontent=\"Object Data\" &&\n+\tsize=$(strlen \"$content\") &&\n+\tsha1=$(echo_without_newline \"$content\" | git hash-object -w --stdin)\n+'\n+\n+test_expect_success 'batch-command calling object works' '\n+\techo \"object $sha1\" | git cat-file --batch-command >actual &&\n+\techo \"$sha1 blob $size\" >expect &&\n+\techo `git cat-file -p \"$sha1\"` >>expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'batch-command calling info works' '\n+\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n+\techo \"$sha1 blob $size\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'batch-command fflush works' '\n+\tprintf \"fflush\\n\" > cmd &&\n+\ttest_expect_code 0 git cat-file --batch-command < cmd 2>err\n+'\n+\n test_done\n-- \n2.34.1\n\n"},{"id":"446893","messageId":"96DB3B31-F76C-475F-8556-65FAE78CD25F@gmail.com","threadId":"57310","inReplyTo":"20220125225008.45944-1-johncai86@gmail.com","subject":"Re: [RFC v2] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-25T23:00:47Z","receivedAt":"2022-01-25T23:00:51Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\n\nOn 25 Jan 2022, at 17:50, John Cai wrote:\n\n> This RFC patch proposes a new flag --stdin-cmd that works with\n> git-cat-file --batch. Similar to git-update-ref --stdin, it will accept\n> commands and arguments from stdin.\n\nSorry I forgot to change this summary. s/--stdin-cmd/--batch-command\n\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 --stdin-cmd, we could\n> get rid of the second --batch-check process, and just have one progress\n> where we can flip between getting object info, and getting object contents.\n> This can lead to huge savings.\n>\n> git cat-file --batch --stdin-cmd\n>\n> $ <command> [arg1] [arg2] NL\n>\n> We can also add a -z mode to allow for NUL-terminated lines\n>\n> $ <command> [arg1] [arg2] NUL\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> For instance, 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> 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>  strvec.c                       |  23 +++++++\n>  strvec.h                       |   8 +++\n>  t/t1006-cat-file.sh            |  32 +++++++++\n>  5 files changed, 191 insertions(+), 1 deletion(-)\n>\n> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n> index bef76f4dd0..8aefa45e4c 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\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 7b3f42950e..30794284d5 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -16,6 +16,7 @@\n>  #include \"packfile.h\"\n>  #include \"object-store.h\"\n>  #include \"promisor-remote.h\"\n> +#include \"strvec.h\"\n>\n>  struct batch_options {\n>  \tint enabled;\n> @@ -26,7 +27,10 @@ struct batch_options {\n>  \tint unordered;\n>  \tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n>  \tconst char *format;\n> +\tint stdin_cmd;\n> +\tint end_null;\n>  };\n> +static char line_termination = '\\n';\n>\n>  static const char *force_path;\n>\n> @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,\n>  \t\t\t\t      data);\n>  }\n>\n> +enum batch_state {\n> +\t/* Non-transactional state open for commands. */\n> +\tBATCH_STATE_OPEN,\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 args;\n> +\tenum batch_state state;\n> +} commands[] = {\n> +\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n> +\t{ \"info\", parse_cmd_info, 1, BATCH_STATE_OPEN },\n> +\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n> +};\n> +\n> +static void batch_objects_stdin_cmd(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> +\tenum batch_state state = BATCH_STATE_OPEN;\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;\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\tconst char *cmd_name;\n> +\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_name))\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].args ? ' ' : line_termination;\n> +\t\t\tif (input.buf[strlen(prefix)] != c)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tcmd = &commands[i];\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (!cmd)\n> +\t\t\tdie(\"unknown command: %s\", input.buf);\n> +\n> +\t\tp = input.buf + strlen(cmd->prefix) + 1;\n> +\t\tconst char *pos = strstr(p, &line_termination);\n> +\n> +\t\tswitch (state) {\n> +\t\tcase BATCH_STATE_OPEN:\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcmd->fn(opt, xstrndup(p, pos-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 +615,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 stdin_cmd = opt->stdin_cmd;\n>\n>  \tif (!opt->format)\n>  \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n> @@ -590,7 +691,8 @@ 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> -\twhile (strbuf_getline(&input, stdin) != EOF) {\n> +\twhile (!stdin_cmd &&\n> +\t       strbuf_getline(&input, stdin) != EOF) {\n>  \t\tif (data.split_on_whitespace) {\n>  \t\t\t/*\n>  \t\t\t * Split at first whitespace, tying off the beginning\n> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)\n>  \t\tbatch_one_object(input.buf, &output, opt, &data);\n>  \t}\n>\n> +\tif (stdin_cmd)\n> +\t\tbatch_objects_stdin_cmd(opt, &output, &data);\n> +\n>  \tstrbuf_release(&input);\n>  \tstrbuf_release(&output);\n>  \twarn_on_object_refname_ambiguity = save_warning;\n> @@ -636,6 +741,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->stdin_cmd = !strcmp(opt->long_name, \"batch-command\");\n>  \tbo->format = arg;\n>\n>  \treturn 0;\n> @@ -683,6 +789,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> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>  \t/* Batch defaults */\n>  \tif (batch.buffer_output < 0)\n>  \t\tbatch.buffer_output = batch.all_objects;\n> +\tif (batch.end_null)\n> +\t\tline_termination = '\\0';\n>\n>  \t/* Return early if we're in batch mode? */\n>  \tif (batch.enabled) {\n> diff --git a/strvec.c b/strvec.c\n> index 61a76ce6cb..7dca04bf7a 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n> @@ -85,6 +85,29 @@ void strvec_split(struct strvec *array, const char *to_split)\n>  \t}\n>  }\n>\n> +size_t strvec_split_delim(struct strvec *array, const char *string,\n> +\t\t\t  int delim, int maxsplit)\n> +{\n> +\tsize_t count = 0;\n> +\tconst char *p = string, *end;\n> +\n> +\tfor (;;) {\n> +\t\tcount++;\n> +\t\tif (maxsplit >= 0 && count > maxsplit) {\n> +\t\t\tstrvec_push(array, p);\n> +\t\t\treturn count;\n> +\t\t}\n> +\t\tend = strchr(p, delim);\n> +\t\tif (end) {\n> +\t\t\tstrvec_push_nodup(array, xmemdupz(p, end - p));\n> +\t\t\tp = end + 1;\n> +\t\t} else {\n> +\t\t\tstrvec_push(array, p);\n> +\t\t\treturn count;\n> +\t\t}\n> +\t}\n> +}\n> +\n>  void strvec_clear(struct strvec *array)\n>  {\n>  \tif (array->v != empty_strvec) {\n> diff --git a/strvec.h b/strvec.h\n> index 9f55c8766b..c474918b91 100644\n> --- a/strvec.h\n> +++ b/strvec.h\n> @@ -73,6 +73,14 @@ void strvec_pop(struct strvec *);\n>  /* Splits by whitespace; does not handle quoted arguments! */\n>  void strvec_split(struct strvec *, const char *);\n>\n> +/**\n> + * strvec_split_delim() is a split function that behaves more like its\n> + * string_list_split() cousin than the whitespace-splitting\n> + * strvec_split().\n> + */\n> +size_t strvec_split_delim(struct strvec *array, const char *string,\n> +\t\t\t  int delim, int maxsplit);\n> +\n>  /**\n>   * Free all memory associated with the array and return it to the\n>   * initial, empty state.\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 145eee11df..935ab1cd34 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -964,4 +964,36 @@ test_expect_success 'cat-file --batch-all-objects --batch-check ignores replace'\n>  \ttest_cmp expect actual\n>  '\n>\n> +F='%s\\0'\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_expect_success 'setup object data' '\n> +\tcontent=\"Object Data\" &&\n> +\tsize=$(strlen \"$content\") &&\n> +\tsha1=$(echo_without_newline \"$content\" | git hash-object -w --stdin)\n> +'\n> +\n> +test_expect_success 'batch-command calling object works' '\n> +\techo \"object $sha1\" | git cat-file --batch-command >actual &&\n> +\techo \"$sha1 blob $size\" >expect &&\n> +\techo `git cat-file -p \"$sha1\"` >>expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'batch-command calling info works' '\n> +\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n> +\techo \"$sha1 blob $size\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'batch-command fflush works' '\n> +\tprintf \"fflush\\n\" > cmd &&\n> +\ttest_expect_code 0 git cat-file --batch-command < cmd 2>err\n> +'\n> +\n>  test_done\n> -- \n> 2.34.1\n"},{"id":"447063","messageId":"1540ee6b-d7ef-428e-d2f8-74bc4847c34f@gmail.com","threadId":"57310","inReplyTo":"20220125225008.45944-1-johncai86@gmail.com","subject":"Re: [RFC v2] cat-file: add a --stdin-cmd mode","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-01-27T11:25:34Z","receivedAt":"2022-01-27T11:25:40Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nOn 25/01/2022 22:50, John Cai wrote:\n> This RFC patch proposes a new flag --stdin-cmd 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 --stdin-cmd, we could\n> get rid of the second --batch-check process, and just have one progress\n> where we can flip between getting object info, and getting object contents.\n> This can lead to huge savings.\n> \n> git cat-file --batch --stdin-cmd\n> \n> $ <command> [arg1] [arg2] NL\n> \n> We can also add a -z mode to allow for NUL-terminated lines\n> \n> $ <command> [arg1] [arg2] NUL\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> For instance, 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> 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\nI've left some comments below, they're mostly small details, I like the \nnew option name and the changes you've made to the command parsing.\n\n> ---\n>   Documentation/git-cat-file.txt |  15 +++++\n>   builtin/cat-file.c             | 114 ++++++++++++++++++++++++++++++++-\n>   strvec.c                       |  23 +++++++\n>   strvec.h                       |   8 +++\n>   t/t1006-cat-file.sh            |  32 +++++++++\n>   5 files changed, 191 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n> index bef76f4dd0..8aefa45e4c 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\nis this missing a '-'?\n\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\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 7b3f42950e..30794284d5 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -16,6 +16,7 @@\n>   #include \"packfile.h\"\n>   #include \"object-store.h\"\n>   #include \"promisor-remote.h\"\n> +#include \"strvec.h\"\n>   \n>   struct batch_options {\n>   \tint enabled;\n> @@ -26,7 +27,10 @@ struct batch_options {\n>   \tint unordered;\n>   \tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n>   \tconst char *format;\n> +\tint stdin_cmd;\n\nNow that the option has been renamed it would be nice to rename the \ncorresponding variable to match\n\n> +\tint end_null;\n\nIf you're not adding '-z' here then you don't need this or the addition \nbelow.\n\n>   };\n> +static char line_termination = '\\n';\n>   \n>   static const char *force_path;\n>   \n> @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,\n>   \t\t\t\t      data);\n>   }\n>   \n> +enum batch_state {\n> +\t/* Non-transactional state open for commands. */\n> +\tBATCH_STATE_OPEN,\n> +};\n\nI forgot to ask what the idea behind the batch state is last time - \nwhat's it for?\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 args;\n\nThis is now a flag so maybe 'takes_args' would better describe its purpose.\n\n> +\tenum batch_state state;\n> +} commands[] = {\n> +\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n> +\t{ \"info\", parse_cmd_info, 1, BATCH_STATE_OPEN },\n> +\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n> +};\n> +\n> +static void batch_objects_stdin_cmd(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> +\tenum batch_state state = BATCH_STATE_OPEN;\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;\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\tconst char *cmd_name;\n\nskip_prefix() sets this to the end of the name so maybe 'cmd_end' would \nbe clearer?\n\n> +\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_name))\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].args ? ' ' : line_termination;\n> +\t\t\tif (input.buf[strlen(prefix)] != c)\n\nNow that you're using skip_prefix() you can write this as\n\t\t\tif (*cmd_end != c)\n\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tcmd = &commands[i];\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (!cmd)\n> +\t\t\tdie(\"unknown command: %s\", input.buf);\n> +\n> +\t\tp = input.buf + strlen(cmd->prefix) + 1;\n\nThis can be simplified to\n\t\tp = cmd_end + 1;\n\n> +\t\tconst char *pos = strstr(p, &line_termination);\n\nThis isn't needed without '-z'. If it were required then using \nstrchrnul() would prevent a NULL pointer dereference when the last input \nline does not end with a terminator. I think we typically call a pointer \nto the end of the line 'eol' or 'end'. Also variables should be declared \nat the top of the function.\n\n> +\t\tswitch (state) {\n> +\t\tcase BATCH_STATE_OPEN:\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tcmd->fn(opt, xstrndup(p, pos-p), output, data);\n\nIs there a reason this is passing a copy of the string?\n\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 +615,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 stdin_cmd = opt->stdin_cmd;\n>   \n>   \tif (!opt->format)\n>   \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n> @@ -590,7 +691,8 @@ 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> -\twhile (strbuf_getline(&input, stdin) != EOF) {\n> +\twhile (!stdin_cmd &&\n\nIf you moved the 'if (stdin_cmd)' block above this block we could loose \nthis change. I'm not sure if that is possible without looking at the \nwhole function though.\n\n> +\t       strbuf_getline(&input, stdin) != EOF) {\n>   \t\tif (data.split_on_whitespace) {\n>   \t\t\t/*\n>   \t\t\t * Split at first whitespace, tying off the beginning\n> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)\n>   \t\tbatch_one_object(input.buf, &output, opt, &data);\n>   \t}\n>   \n> +\tif (stdin_cmd)\n> +\t\tbatch_objects_stdin_cmd(opt, &output, &data);\n> +\n>   \tstrbuf_release(&input);\n>   \tstrbuf_release(&output);\n>   \twarn_on_object_refname_ambiguity = save_warning;\n> @@ -636,6 +741,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->stdin_cmd = !strcmp(opt->long_name, \"batch-command\");\n>   \tbo->format = arg;\n>   \n>   \treturn 0;\n> @@ -683,6 +789,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> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>   \t/* Batch defaults */\n>   \tif (batch.buffer_output < 0)\n>   \t\tbatch.buffer_output = batch.all_objects;\n> +\tif (batch.end_null)\n> +\t\tline_termination = '\\0';\n>   \n>   \t/* Return early if we're in batch mode? */\n>   \tif (batch.enabled) {\n> diff --git a/strvec.c b/strvec.c\n> index 61a76ce6cb..7dca04bf7a 100644\n> --- a/strvec.c\n> +++ b/strvec.c\n>[...]\n\nWe don't need any strvec changes now that we don't split the input lines \nto --bactch-command\n\n> +F='%s\\0'\n\nThis isn't used now\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_expect_success 'setup object data' '\n> +\tcontent=\"Object Data\" &&\n> +\tsize=$(strlen \"$content\") &&\n> +\tsha1=$(echo_without_newline \"$content\" | git hash-object -w --stdin)\n> +'\n> +\n> +test_expect_success 'batch-command calling object works' '\n> +\techo \"object $sha1\" | git cat-file --batch-command >actual &&\n> +\techo \"$sha1 blob $size\" >expect &&\n> +\techo `git cat-file -p \"$sha1\"` >>expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +test_expect_success 'batch-command calling info works' '\n> +\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n> +\techo \"$sha1 blob $size\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n\nI had a quick look at this test file and there is a loop at the top that \nruns some --batch tests on various inputs, I wonder if these two tests \ncould go in there.\n\n> +test_expect_success 'batch-command fflush works' '\n> +\tprintf \"fflush\\n\" > cmd &&\n> +\ttest_expect_code 0 git cat-file --batch-command < cmd 2>err\n> +'\n\nIt'd be nice to check this actually flushes the output.\n\nBest Wishes\n\nPhillip\n\n>   test_done\n\n"},{"id":"447129","messageId":"1C2DA310-452B-4846-9183-0D3CDB05620B@gmail.com","threadId":"57310","inReplyTo":"1540ee6b-d7ef-428e-d2f8-74bc4847c34f@gmail.com","subject":"Re: [RFC v2] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-27T21:04:19Z","receivedAt":"2022-01-27T21:04:23Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hey Phillip,\n\nFirst of all thank you for the thorough review—it really helps someone who’s learning how to contribute!\n\nOn 27 Jan 2022, at 6:25, Phillip Wood wrote:\n\n> Hi John\n>\n> On 25/01/2022 22:50, John Cai wrote:\n>> This RFC patch proposes a new flag --stdin-cmd 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 --stdin-cmd, we could\n>> get rid of the second --batch-check process, and just have one progress\n>> where we can flip between getting object info, and getting object contents.\n>> This can lead to huge savings.\n>>\n>> git cat-file --batch --stdin-cmd\n>>\n>> $ <command> [arg1] [arg2] NL\n>>\n>> We can also add a -z mode to allow for NUL-terminated lines\n>>\n>> $ <command> [arg1] [arg2] NUL\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>> For instance, 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>> 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> I've left some comments below, they're mostly small details, I like the new option name and the changes you've made to the command parsing.\n>\n>> ---\n>>   Documentation/git-cat-file.txt |  15 +++++\n>>   builtin/cat-file.c             | 114 ++++++++++++++++++++++++++++++++-\n>>   strvec.c                       |  23 +++++++\n>>   strvec.h                       |   8 +++\n>>   t/t1006-cat-file.sh            |  32 +++++++++\n>>   5 files changed, 191 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n>> index bef76f4dd0..8aefa45e4c 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>\n> is this missing a '-'?\n>\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\n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index 7b3f42950e..30794284d5 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -16,6 +16,7 @@\n>>   #include \"packfile.h\"\n>>   #include \"object-store.h\"\n>>   #include \"promisor-remote.h\"\n>> +#include \"strvec.h\"\n>>    struct batch_options {\n>>   \tint enabled;\n>> @@ -26,7 +27,10 @@ struct batch_options {\n>>   \tint unordered;\n>>   \tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n>>   \tconst char *format;\n>> +\tint stdin_cmd;\n>\n> Now that the option has been renamed it would be nice to rename the corresponding variable to match\n\nI was trying to find a good name. There is already a cmdmode variable. I’m thinking stdin_cmd is not such a\nbad name since we are receiving commands from stdin. suggestions welcome!\n\n>\n>> +\tint end_null;\n>\n> If you're not adding '-z' here then you don't need this or the addition below.\n>\n>>   };\n>> +static char line_termination = '\\n';\n>>    static const char *force_path;\n>>  @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,\n>>   \t\t\t\t      data);\n>>   }\n>>  +enum batch_state {\n>> +\t/* Non-transactional state open for commands. */\n>> +\tBATCH_STATE_OPEN,\n>> +};\n>\n> I forgot to ask what the idea behind the batch state is last time - what's it for?\n\nThis is to support transactional semantics for commands we want to support in the future, but\nsince this is already a biggish change, we can leave this out of this series.\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 args;\n>\n> This is now a flag so maybe 'takes_args' would better describe its purpose.\n>\n>> +\tenum batch_state state;\n>> +} commands[] = {\n>> +\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n>> +\t{ \"info\", parse_cmd_info, 1, BATCH_STATE_OPEN },\n>> +\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n>> +};\n>> +\n>> +static void batch_objects_stdin_cmd(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>> +\tenum batch_state state = BATCH_STATE_OPEN;\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;\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\tconst char *cmd_name;\n>\n> skip_prefix() sets this to the end of the name so maybe 'cmd_end' would be clearer?\n>\n>> +\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_name))\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].args ? ' ' : line_termination;\n>> +\t\t\tif (input.buf[strlen(prefix)] != c)\n>\n> Now that you're using skip_prefix() you can write this as\n>     \t\tif (*cmd_end != c)\n>\n>> +\t\t\t\tcontinue;\n>> +\n>> +\t\t\tcmd = &commands[i];\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tif (!cmd)\n>> +\t\t\tdie(\"unknown command: %s\", input.buf);\n>> +\n>> +\t\tp = input.buf + strlen(cmd->prefix) + 1;\n>\n> This can be simplified to\n>     \tp = cmd_end + 1;\n>\n>> +\t\tconst char *pos = strstr(p, &line_termination);\n>\n> This isn't needed without '-z'. If it were required then using strchrnul() would prevent a NULL pointer dereference when the last input line does not end with a terminator. I think we typically call a pointer to the end of the line 'eol' or 'end'. Also variables should be declared at the top of the function.\n>\n>> +\t\tswitch (state) {\n>> +\t\tcase BATCH_STATE_OPEN:\n>> +\t\t\tbreak;\n>> +\t\t}\n>> +\t\tcmd->fn(opt, xstrndup(p, pos-p), output, data);\n>\n> Is there a reason this is passing a copy of the string?\n>\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 +615,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 stdin_cmd = opt->stdin_cmd;\n>>    \tif (!opt->format)\n>>   \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n>> @@ -590,7 +691,8 @@ static int batch_objects(struct batch_options *opt)\n>>   \tsave_warning = warn_on_object_refname_ambiguity;\n>>   \twarn_on_object_refname_ambiguity = 0;\n>>  -\twhile (strbuf_getline(&input, stdin) != EOF) {\n>> +\twhile (!stdin_cmd &&\n>\n> If you moved the 'if (stdin_cmd)' block above this block we could loose this change. I'm not sure if that is possible without looking at the whole function though.\n>\n>> +\t       strbuf_getline(&input, stdin) != EOF) {\n>>   \t\tif (data.split_on_whitespace) {\n>>   \t\t\t/*\n>>   \t\t\t * Split at first whitespace, tying off the beginning\n>> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)\n>>   \t\tbatch_one_object(input.buf, &output, opt, &data);\n>>   \t}\n>>  +\tif (stdin_cmd)\n>> +\t\tbatch_objects_stdin_cmd(opt, &output, &data);\n>> +\n>>   \tstrbuf_release(&input);\n>>   \tstrbuf_release(&output);\n>>   \twarn_on_object_refname_ambiguity = save_warning;\n>> @@ -636,6 +741,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->stdin_cmd = !strcmp(opt->long_name, \"batch-command\");\n>>   \tbo->format = arg;\n>>    \treturn 0;\n>> @@ -683,6 +789,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>> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>>   \t/* Batch defaults */\n>>   \tif (batch.buffer_output < 0)\n>>   \t\tbatch.buffer_output = batch.all_objects;\n>> +\tif (batch.end_null)\n>> +\t\tline_termination = '\\0';\n>>    \t/* Return early if we're in batch mode? */\n>>   \tif (batch.enabled) {\n>> diff --git a/strvec.c b/strvec.c\n>> index 61a76ce6cb..7dca04bf7a 100644\n>> --- a/strvec.c\n>> +++ b/strvec.c\n>> [...]\n>\n> We don't need any strvec changes now that we don't split the input lines to --bactch-command\n>\n>> +F='%s\\0'\n>\n> This isn't used now\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_expect_success 'setup object data' '\n>> +\tcontent=\"Object Data\" &&\n>> +\tsize=$(strlen \"$content\") &&\n>> +\tsha1=$(echo_without_newline \"$content\" | git hash-object -w --stdin)\n>> +'\n>> +\n>> +test_expect_success 'batch-command calling object works' '\n>> +\techo \"object $sha1\" | git cat-file --batch-command >actual &&\n>> +\techo \"$sha1 blob $size\" >expect &&\n>> +\techo `git cat-file -p \"$sha1\"` >>expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'batch-command calling info works' '\n>> +\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n>> +\techo \"$sha1 blob $size\" >expect &&\n>> +\ttest_cmp expect actual\n>> +'\n>\n> I had a quick look at this test file and there is a loop at the top that runs some --batch tests on various inputs, I wonder if these two tests could go in there.\n>\n>> +test_expect_success 'batch-command fflush works' '\n>> +\tprintf \"fflush\\n\" > cmd &&\n>> +\ttest_expect_code 0 git cat-file --batch-command < cmd 2>err\n>> +'\n>\n> It'd be nice to check this actually flushes the output.\n\ncould you give me some ideas on how to do this?\n\n>\n> Best Wishes\n>\n> Phillip\n>\n>>   test_done\n"},{"id":"447171","messageId":"1D711BE5-9B84-43C7-A8A4-E047430D7A01@gmail.com","threadId":"57310","inReplyTo":"1C2DA310-452B-4846-9183-0D3CDB05620B@gmail.com","subject":"Re: [RFC v2] cat-file: add a --stdin-cmd mode","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-01-28T04:16:22Z","receivedAt":"2022-01-28T04:16:26Z","isPatch":false,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Resending this as my last message accidentally included some html and got rejected by the listserv\n\n\nOn 27 Jan 2022, at 16:04, John Cai wrote:\n\n> Hey Phillip,\n>\n> First of all thank you for the thorough review—it really helps someone who’s learning how to contribute!\n>\n> On 27 Jan 2022, at 6:25, Phillip Wood wrote:\n>\n>> Hi John\n>>\n>> On 25/01/2022 22:50, John Cai wrote:\n>>> This RFC patch proposes a new flag --stdin-cmd 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 --stdin-cmd, we could\n>>> get rid of the second --batch-check process, and just have one progress\n>>> where we can flip between getting object info, and getting object contents.\n>>> This can lead to huge savings.\n>>>\n>>> git cat-file --batch --stdin-cmd\n>>>\n>>> $ <command> [arg1] [arg2] NL\n>>>\n>>> We can also add a -z mode to allow for NUL-terminated lines\n>>>\n>>> $ <command> [arg1] [arg2] NUL\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>>> For instance, 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>>> 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>> I've left some comments below, they're mostly small details, I like the new option name and the changes you've made to the command parsing.\n>>\n>>> ---\n>>>   Documentation/git-cat-file.txt |  15 +++++\n>>>   builtin/cat-file.c             | 114 ++++++++++++++++++++++++++++++++-\n>>>   strvec.c                       |  23 +++++++\n>>>   strvec.h                       |   8 +++\n>>>   t/t1006-cat-file.sh            |  32 +++++++++\n>>>   5 files changed, 191 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n>>> index bef76f4dd0..8aefa45e4c 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>>\n>> is this missing a '-'?\n>>\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\n>>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>>> index 7b3f42950e..30794284d5 100644\n>>> --- a/builtin/cat-file.c\n>>> +++ b/builtin/cat-file.c\n>>> @@ -16,6 +16,7 @@\n>>>   #include \"packfile.h\"\n>>>   #include \"object-store.h\"\n>>>   #include \"promisor-remote.h\"\n>>> +#include \"strvec.h\"\n>>>    struct batch_options {\n>>>   \tint enabled;\n>>> @@ -26,7 +27,10 @@ struct batch_options {\n>>>   \tint unordered;\n>>>   \tint cmdmode; /* may be 'w' or 'c' for --filters or --textconv */\n>>>   \tconst char *format;\n>>> +\tint stdin_cmd;\n>>\n>> Now that the option has been renamed it would be nice to rename the corresponding variable to match\n>\n> I was trying to find a good name. There is already a cmdmode variable. I’m thinking stdin_cmd is not such a\n> bad name since we are receiving commands from stdin. suggestions welcome!\n>\n>>\n>>> +\tint end_null;\n>>\n>> If you're not adding '-z' here then you don't need this or the addition below.\n>>\n>>>   };\n>>> +static char line_termination = '\\n';\n>>>    static const char *force_path;\n>>>  @@ -508,6 +512,102 @@ static int batch_unordered_packed(const struct object_id *oid,\n>>>   \t\t\t\t      data);\n>>>   }\n>>>  +enum batch_state {\n>>> +\t/* Non-transactional state open for commands. */\n>>> +\tBATCH_STATE_OPEN,\n>>> +};\n>>\n>> I forgot to ask what the idea behind the batch state is last time - what's it for?\n>\n> This is to support transactional semantics for commands we want to support in the future, but\n> since this is already a biggish change, we can leave this out of this series.\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 args;\n>>\n>> This is now a flag so maybe 'takes_args' would better describe its purpose.\n>>\n>>> +\tenum batch_state state;\n>>> +} commands[] = {\n>>> +\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n>>> +\t{ \"info\", parse_cmd_info, 1, BATCH_STATE_OPEN },\n>>> +\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n>>> +};\n>>> +\n>>> +static void batch_objects_stdin_cmd(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>>> +\tenum batch_state state = BATCH_STATE_OPEN;\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;\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\tconst char *cmd_name;\n>>\n>> skip_prefix() sets this to the end of the name so maybe 'cmd_end' would be clearer?\n>>\n>>> +\t\t\tif (!skip_prefix(input.buf, prefix, &cmd_name))\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].args ? ' ' : line_termination;\n>>> +\t\t\tif (input.buf[strlen(prefix)] != c)\n>>\n>> Now that you're using skip_prefix() you can write this as\n>>     \t\tif (*cmd_end != c)\n>>\n>>> +\t\t\t\tcontinue;\n>>> +\n>>> +\t\t\tcmd = &commands[i];\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t\tif (!cmd)\n>>> +\t\t\tdie(\"unknown command: %s\", input.buf);\n>>> +\n>>> +\t\tp = input.buf + strlen(cmd->prefix) + 1;\n>>\n>> This can be simplified to\n>>     \tp = cmd_end + 1;\n>>\n>>> +\t\tconst char *pos = strstr(p, &line_termination);\n>>\n>> This isn't needed without '-z'. If it were required then using strchrnul() would prevent a NULL pointer dereference when the last input line does not end with a terminator. I think we typically call a pointer to the end of the line 'eol' or 'end'. Also variables should be declared at the top of the function.\n>>\n>>> +\t\tswitch (state) {\n>>> +\t\tcase BATCH_STATE_OPEN:\n>>> +\t\t\tbreak;\n>>> +\t\t}\n>>> +\t\tcmd->fn(opt, xstrndup(p, pos-p), output, data);\n>>\n>> Is there a reason this is passing a copy of the string?\n>>\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 +615,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 stdin_cmd = opt->stdin_cmd;\n>>>    \tif (!opt->format)\n>>>   \t\topt->format = \"%(objectname) %(objecttype) %(objectsize)\";\n>>> @@ -590,7 +691,8 @@ static int batch_objects(struct batch_options *opt)\n>>>   \tsave_warning = warn_on_object_refname_ambiguity;\n>>>   \twarn_on_object_refname_ambiguity = 0;\n>>>  -\twhile (strbuf_getline(&input, stdin) != EOF) {\n>>> +\twhile (!stdin_cmd &&\n>>\n>> If you moved the 'if (stdin_cmd)' block above this block we could loose this change. I'm not sure if that is possible without looking at the whole function though.\n>>\n>>> +\t       strbuf_getline(&input, stdin) != EOF) {\n>>>   \t\tif (data.split_on_whitespace) {\n>>>   \t\t\t/*\n>>>   \t\t\t * Split at first whitespace, tying off the beginning\n>>> @@ -608,6 +710,9 @@ static int batch_objects(struct batch_options *opt)\n>>>   \t\tbatch_one_object(input.buf, &output, opt, &data);\n>>>   \t}\n>>>  +\tif (stdin_cmd)\n>>> +\t\tbatch_objects_stdin_cmd(opt, &output, &data);\n>>> +\n>>>   \tstrbuf_release(&input);\n>>>   \tstrbuf_release(&output);\n>>>   \twarn_on_object_refname_ambiguity = save_warning;\n>>> @@ -636,6 +741,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->stdin_cmd = !strcmp(opt->long_name, \"batch-command\");\n>>>   \tbo->format = arg;\n>>>    \treturn 0;\n>>> @@ -683,6 +789,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>>> @@ -738,6 +848,8 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>>>   \t/* Batch defaults */\n>>>   \tif (batch.buffer_output < 0)\n>>>   \t\tbatch.buffer_output = batch.all_objects;\n>>> +\tif (batch.end_null)\n>>> +\t\tline_termination = '\\0';\n>>>    \t/* Return early if we're in batch mode? */\n>>>   \tif (batch.enabled) {\n>>> diff --git a/strvec.c b/strvec.c\n>>> index 61a76ce6cb..7dca04bf7a 100644\n>>> --- a/strvec.c\n>>> +++ b/strvec.c\n>>> [...]\n>>\n>> We don't need any strvec changes now that we don't split the input lines to --bactch-command\n>>\n>>> +F='%s\\0'\n>>\n>> This isn't used now\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_expect_success 'setup object data' '\n>>> +\tcontent=\"Object Data\" &&\n>>> +\tsize=$(strlen \"$content\") &&\n>>> +\tsha1=$(echo_without_newline \"$content\" | git hash-object -w --stdin)\n>>> +'\n>>> +\n>>> +test_expect_success 'batch-command calling object works' '\n>>> +\techo \"object $sha1\" | git cat-file --batch-command >actual &&\n>>> +\techo \"$sha1 blob $size\" >expect &&\n>>> +\techo `git cat-file -p \"$sha1\"` >>expect &&\n>>> +\ttest_cmp expect actual\n>>> +'\n>>> +\n>>> +test_expect_success 'batch-command calling info works' '\n>>> +\techo \"info $sha1\" | git cat-file --batch-command >actual &&\n>>> +\techo \"$sha1 blob $size\" >expect &&\n>>> +\ttest_cmp expect actual\n>>> +'\n>>\n>> I had a quick look at this test file and there is a loop at the top that runs some --batch tests on various inputs, I wonder if these two tests could go in there.\n>>\n>>> +test_expect_success 'batch-command fflush works' '\n>>> +\tprintf \"fflush\\n\" > cmd &&\n>>> +\ttest_expect_code 0 git cat-file --batch-command < cmd 2>err\n>>> +'\n>>\n>> It'd be nice to check this actually flushes the output.\n>\n> could you give me some ideas on how to do this?\n\nOk, after filling around I think it can be something like this (copied over from an example in t9300-fast-import.sh\n\nrun_buffer_test () {\n\ttype=$1\n\tsha1=$2\n\tsize=$3\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) &\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\techo \"fflush\" >&8\n\tread actual <&9\n\ttest \"$actual\" = \"$expect\"\n}\n\nNot sure if there's a simpler way since we need to simulate writing to and reading from the process.\n\nThanks\n\n>\n>>\n>> Best Wishes\n>>\n>> Phillip\n>>\n>>>   test_done\n"},{"id":"447172","messageId":"20220128060714.M764085@dcvr","threadId":"57310","inReplyTo":"20220125225008.45944-1-johncai86@gmail.com","subject":"Re: [RFC v2] cat-file: add a --stdin-cmd mode","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2022-01-28T06:07:14Z","receivedAt":"2022-01-28T06:07:16Z","isPatch":false,"sender":{"key":"e@80x24.org","avatar":null},"body":"John Cai <johncai86@gmail.com> wrote:\n> This RFC patch proposes a new flag --stdin-cmd 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 --stdin-cmd, we could\n> get rid of the second --batch-check process, and just have one progress\n> where we can flip between getting object info, and getting object contents.\n> This can lead to huge savings.\n\nCool.  I wish I had this back when I worked on a project which uses git.\nIn case I ever continue, having 2-3 features in git itself would be useful:\n\n1) ability to add new alternates / object directories on-the-fly\n2) ability to deal with unlinked packs\n\nAnd maybe:\n\n3) ability to remove alternates\n\nI've implemented 1 and 2 via libgit2; but not yet 3 (not really\nimportant, atm).\n\nThanks.\n"}]}