{"thread":{"id":"56851","subject":"[PATCH 0/2] cat-file: force flush of stdout on empty string","startedAt":"2021-11-05T21:56:45Z","lastAt":"2021-11-08T20:11:36Z","messageCount":8,"participants":["John Cai via GitGitGadget","Junio C Hamano","Ævar Arnfjörð Bjarmason","John Cai"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"440554","messageId":"pull.1124.git.git.1636149400.gitgitgadget@gmail.com","threadId":"56851","inReplyTo":null,"subject":"[PATCH 0/2] cat-file: force flush of stdout on empty string","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-05T21:56:38Z","receivedAt":"2021-11-05T21:56:45Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"When in --buffer mode, it is very useful for the caller to have control over\nwhen the buffer is flushed. Currently there is no convenient way to signal\nfor the buffer to be flushed. One workaround is to provide any nonexisting\ncommit to git-cat-file's stdin, in which case the buffer will be flushed and\na \"$FOO missing\" message will be displayed. However, this is not an ideal\nworkaround.\n\nInstead, this commit teaches git-cat-file to look for an empty string in\nstdin, which will trigger a flush of stdout.\n\nJohn Cai (2):\n  cat-file: force flush of stdout on empty string\n  docs: update behavior of git-cat-file --buffer\n\n Documentation/git-cat-file.txt |  3 ++-\n builtin/cat-file.c             | 11 ++++++++++-\n 2 files changed, 12 insertions(+), 2 deletions(-)\n\n\nbase-commit: 6d82a21a3b699caf378cb0f89b6b0e803fc58480\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1124%2Fjohn-cai%2Fjc%2Fflush-buffer-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1124/john-cai/jc/flush-buffer-v1\nPull-Request: https://github.com/git/git/pull/1124\n-- \ngitgitgadget\n"},{"id":"440555","messageId":"2d687baeed82e7b90d383bad8e209f50e0ce8c87.1636149400.git.gitgitgadget@gmail.com","threadId":"56851","inReplyTo":"pull.1124.git.git.1636149400.gitgitgadget@gmail.com","subject":"[PATCH 1/2] cat-file: force flush of stdout on empty string","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-05T21:56:39Z","receivedAt":"2021-11-05T21:56:45Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nWhen in --buffer mode, it is very useful for the caller to have control\nover when the buffer is flushed. Currently there is no convenient way to\nsignal for the buffer to be flushed. One workaround is to provide any\nnonexisting commit to git-cat-file's stdin, in which case the buffer\nwill be flushed and a \"$FOO missing\" message will be displayed. However,\nthis is not an ideal workaround.\n\nInstead, this commit teaches git-cat-file to look for an empty string in\nstdin, which will trigger a flush of stdout.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n builtin/cat-file.c | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 86fc03242b8..4d17f30f24e 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name,\n \tint flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;\n \tenum get_oid_result result;\n \n+\tif (opt->buffer_output && obj_name[0] == '\\0') {\n+\t\tfflush(stdout);\n+\t\treturn;\n+\t}\n+\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t      flags, &data->oid, &ctx);\n \tif (result != FOUND) {\n@@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt)\n \t\t\tdata.rest = p;\n \t\t}\n \n-\t\tbatch_one_object(input.buf, &output, opt, &data);\n+\t\t /*\n+\t\t  * When in buffer mode and input.buf is an empty string,\n+\t\t  * flush to stdout.\n+\t\t  */\n+\t\t batch_one_object(input.buf, &output, opt, &data);\n \t}\n \n \tstrbuf_release(&input);\n-- \ngitgitgadget\n\n"},{"id":"440556","messageId":"18d6c2819ade2e514a408e4173c6a5048e3f2cd1.1636149400.git.gitgitgadget@gmail.com","threadId":"56851","inReplyTo":"pull.1124.git.git.1636149400.gitgitgadget@gmail.com","subject":"[PATCH 2/2] docs: update behavior of git-cat-file --buffer","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-11-05T21:56:40Z","receivedAt":"2021-11-05T21:56:47Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai <johncai86@gmail.com>\n\nWhen an empty string is entered into stdin, git-cat-file --buffer will\nflush stdout immediately. This commit updates the man page accordingly.\n\nSigned-off-by: John Cai <johncai86@gmail.com>\n---\n Documentation/git-cat-file.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 27b27e2b300..c98e8dc3669 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -104,7 +104,8 @@ OPTIONS\n \tthat a process can interactively read and write from\n \t`cat-file`. With this option, the output uses normal stdio\n \tbuffering; this is much more efficient when invoking\n-\t`--batch-check` on a large number of objects.\n+\t`--batch-check` on a large number of objects. An empty string will\n+\tforce a flush of stdout.\n \n --unordered::\n \tWhen `--batch-all-objects` is in use, visit objects in an\n-- \ngitgitgadget\n"},{"id":"440563","messageId":"xmqqsfwaumlc.fsf@gitster.g","threadId":"56851","inReplyTo":"2d687baeed82e7b90d383bad8e209f50e0ce8c87.1636149400.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] cat-file: force flush of stdout on empty string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-06T00:58:07Z","receivedAt":"2021-11-06T00:58:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> @@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name,\n>  \tint flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;\n>  \tenum get_oid_result result;\n>  \n> +\tif (opt->buffer_output && obj_name[0] == '\\0') {\n> +\t\tfflush(stdout);\n> +\t\treturn;\n> +\t}\n> +\n\nThis might work in practice, but it a bad design taste to add this\nchange here.  The function is designed to take an object name\nstring, and it even prepares a flag variable needed to make a call\nto turn that object name into object data.  We do not need to\ncontaminate the interface with \"usually this takes an object name,\nbut there are these other special cases ...\".  The higher in the\ncallchain we place special cases, the better the lower level\nfunctions become, as that allows them to concentrate on doing one\nsingle thing well.\n\n>  \tresult = get_oid_with_context(the_repository, obj_name,\n>  \t\t\t\t      flags, &data->oid, &ctx);\n>  \tif (result != FOUND) {\n> @@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt)\n>  \t\t\tdata.rest = p;\n>  \t\t}\n>  \n> -\t\tbatch_one_object(input.buf, &output, opt, &data);\n> +\t\t /*\n> +\t\t  * When in buffer mode and input.buf is an empty string,\n> +\t\t  * flush to stdout.\n> +\t\t  */\n\nChecking \"do we have the flush instruction (in which case we'd do\nthe flush here), or do we have textual name of an object (in which\ncase we'd call batch_one_object())?\" here would be far cleaner and\nresults in an easier-to-explain code.  With a cleanly written code\nto do so, it probably does not even need a new comment here.\n\nThis brings up another issue.  Is \"flushing\" the *ONLY* special\nthing we would ever do in this codepath in the future?  I doubt so.\nSquatting on an \"empty string\" is a selfish design that hurts those\nwho will come after you in the future, as they need to find other\nways to ask for a \"special thing\".\n\nIf we are inventing a special syntax that allows us to spell\ncommands that are distinguishable from a validly-spelled object name\nto cause something special (like \"flushing the output stream\"),\nperhaps we want to use a bit more extensible and explicit syntax and\nuse it from day one?\n\nFor example, if no string that begins with three dots can ever be a\nvalid way to spell an object name, perhaps \"...flush\" might be a\nbetter \"please do this special thing\" syntax than an empty string.\nIt is easily extensible (the next special thing can follow suit to\nsay \"...$verb\" to tell the machinery to $verb the input).  When we\ncompare between an empty string and \"...flush\", the latter clearly\nis more descriptive, too.\n\nNote that I offhand do not know if \"a valid string that name an\nobject would never begin with three-dot\" is true.  Please check\nif that is true if you choose to use it, or you can find and use\nanother convention that allows us to clearly distinguish the\n\"special\" instruction and object names.\n\nThanks.\n\n> +\t\t batch_one_object(input.buf, &output, opt, &data);\n>  \t}\n>  \n>  \tstrbuf_release(&input);\n"},{"id":"440564","messageId":"211106.86k0hmgc8q.gmgdl@evledraar.gmail.com","threadId":"56851","inReplyTo":"xmqqsfwaumlc.fsf@gitster.g","subject":"Re: [PATCH 1/2] cat-file: force flush of stdout on empty string","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-06T04:01:10Z","receivedAt":"2021-11-06T04:05:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Nov 05 2021, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>\n>> @@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name,\n>>  \tint flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;\n>>  \tenum get_oid_result result;\n>>  \n>> +\tif (opt->buffer_output && obj_name[0] == '\\0') {\n>> +\t\tfflush(stdout);\n>> +\t\treturn;\n>> +\t}\n>> +\n>\n> This might work in practice, but it a bad design taste to add this\n> change here.  The function is designed to take an object name\n> string, and it even prepares a flag variable needed to make a call\n> to turn that object name into object data.  We do not need to\n> contaminate the interface with \"usually this takes an object name,\n> but there are these other special cases ...\".  The higher in the\n> callchain we place special cases, the better the lower level\n> functions become, as that allows them to concentrate on doing one\n> single thing well.\n>\n>>  \tresult = get_oid_with_context(the_repository, obj_name,\n>>  \t\t\t\t      flags, &data->oid, &ctx);\n>>  \tif (result != FOUND) {\n>> @@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt)\n>>  \t\t\tdata.rest = p;\n>>  \t\t}\n>>  \n>> -\t\tbatch_one_object(input.buf, &output, opt, &data);\n>> +\t\t /*\n>> +\t\t  * When in buffer mode and input.buf is an empty string,\n>> +\t\t  * flush to stdout.\n>> +\t\t  */\n>\n> Checking \"do we have the flush instruction (in which case we'd do\n> the flush here), or do we have textual name of an object (in which\n> case we'd call batch_one_object())?\" here would be far cleaner and\n> results in an easier-to-explain code.  With a cleanly written code\n> to do so, it probably does not even need a new comment here.\n>\n> This brings up another issue.  Is \"flushing\" the *ONLY* special\n> thing we would ever do in this codepath in the future?  I doubt so.\n> Squatting on an \"empty string\" is a selfish design that hurts those\n> who will come after you in the future, as they need to find other\n> ways to ask for a \"special thing\".\n>\n> If we are inventing a special syntax that allows us to spell\n> commands that are distinguishable from a validly-spelled object name\n> to cause something special (like \"flushing the output stream\"),\n> perhaps we want to use a bit more extensible and explicit syntax and\n> use it from day one?\n>\n> For example, if no string that begins with three dots can ever be a\n> valid way to spell an object name, perhaps \"...flush\" might be a\n> better \"please do this special thing\" syntax than an empty string.\n> It is easily extensible (the next special thing can follow suit to\n> say \"...$verb\" to tell the machinery to $verb the input).  When we\n> compare between an empty string and \"...flush\", the latter clearly\n> is more descriptive, too.\n>\n> Note that I offhand do not know if \"a valid string that name an\n> object would never begin with three-dot\" is true.  Please check\n> if that is true if you choose to use it, or you can find and use\n> another convention that allows us to clearly distinguish the\n> \"special\" instruction and object names.\n\nI had much the same thought, this is a useful feature, but let's not\nsquat on the one bit of open syntax we have.\n\nJohn: I think a better direction here is to add a mode to cat-file to\nemulate what \"git update-ref --stdin\" supports. Here's a demo of that\n(also quoted below):\nhttps://github.com/git/git/commit/7794f6cfdbdca0dd6bab0dea16193ebf018b86a9\n\nThat's on top of some general UI improvements to cat-file I've got\nlocally:\nhttps://github.com/git/git/compare/master...avar:avar/cat-file-usage-and-options-handling\n\nThat WIP patch on top follows below, of course it's a *lot* more initial\nscaffolding, but I think once we get past that initial step it's a much\nbetter path forward. As noted the code is also almost entirely\ncopy/pasted from update-ref.c, and perhaps some of the shared parts\ncould be moved to some library both could use.\n\nI couldn't think of a better name than --stdin-cmd, suggestions most\nwelcome.\n\nFrom 7794f6cfdbdca0dd6bab0dea16193ebf018b86a9 Mon Sep 17 00:00:00 2001\nMessage-Id: <patch-1.1-7794f6cfdbd-20211106T040307Z-avarab@gmail.com>\nFrom: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n <avarab@gmail.com>\nDate: Sat, 6 Nov 2021 04:54:04 +0100\nSubject: [PATCH] WIP cat-file: add a --stdin-cmd mode\nMIME-Version: 1.0\nContent-Type: text/plain; charset=UTF-8\nContent-Transfer-Encoding: 8bit\n\nThis WIP patch is mostly stealing code from builtin/update-ref.c and\nimplementing the same sort of prefixed command-mode that it\nsupports. I.e. in addition to --batch now supporting:\n\n    <object> LF\n\nIt'll support with --stdin-cmd, with and without -z, respectively:\n\n    object <object> NL\n    object <object> NUL\n\nThe plus being that we can now implement additional commands. Let's\nstart that by scratching the itch John Cai wanted to address in [1]\nand implement a (with and without -z):\n\n    fflush NL\n    fflush NUL\n\nThat command simply calls fflush(stdout), which could be done as an\nemergent effect before by feeding the input a \"NL\".\n\nI think this will be useful for other things, e.g. I've observed in\nthe past that a not-trivial part of \"cat-file --batch\" time is spent\non parsing its <object> argument and seeing if it's a revision, ref\netc.\n\nSo we could e.g. add a command that only accepts a full-length 40\ncharacter SHA-1, or switch the --format output mid-request etc.\n\n1. https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n builtin/cat-file.c | 116 ++++++++++++++++++++++++++++++++++++++++++++-\n 1 file changed, 115 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b76f2a00046..afdb976c6e7 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -26,7 +26,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@@ -507,6 +510,106 @@ 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 *next, const char *end,\n+\t\t\t     struct strbuf *output,\n+\t\t\t     struct expand_data *data)\n+{\n+\tsize_t len = end - next - 1;\n+\tchar *p = (char *)next;\n+\tchar old = p[len];\n+\n+\tp[len] = '\\0';\n+\tbatch_one_object(next, output, opt, data);\n+\tp[len] = old;\n+}\n+\n+static void parse_cmd_fflush(struct batch_options *opt,\n+\t\t\t     const char *next, const char *end,\n+\t\t\t     struct strbuf *output,\n+\t\t\t     struct expand_data *data)\n+{\n+\tif (*next != line_termination)\n+\t\tdie(\"fflush: extra input: %s\", next);\n+\tfflush(stdout);\n+}\n+\n+static const struct parse_cmd {\n+\tconst char *prefix;\n+\tvoid (*fn)(struct batch_options *, const char *, const char *, struct strbuf *, struct expand_data *);\n+\tunsigned args;\n+\tenum batch_state state;\n+} command[] = {\n+\t{ \"object\", parse_cmd_object, 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\tsize_t i, j;\n+\t\tconst struct parse_cmd *cmd = NULL;\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(command); i++) {\n+\t\t\tconst char *prefix = command[i].prefix;\n+\t\t\tchar c;\n+\n+\t\t\tif (!starts_with(input.buf, prefix))\n+\t\t\t\tcontinue;\n+\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 = command[i].args ? ' ' : line_termination;\n+\t\t\tif (input.buf[strlen(prefix)] != c)\n+\t\t\t\tcontinue;\n+\n+\t\t\tcmd = &command[i];\n+\t\t\tbreak;\n+\t\t}\n+\t\tif (!cmd)\n+\t\t\tdie(\"unknown command: %s\", input.buf);\n+\n+\t\t/*\n+\t\t * Read additional arguments if NUL-terminated. Do not raise an\n+\t\t * error in case there is an early EOF to let the command\n+\t\t * handle missing arguments with a proper error message.\n+\t\t */\n+\t\tfor (j = 1; line_termination == '\\0' && j < cmd->args; j++)\n+\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n+\t\t\t\tbreak;\n+\n+\t\tswitch (state) {\n+\t\tcase BATCH_STATE_OPEN:\n+\t\t\t/* TODO: command state management */\n+\t\t\tbreak;\n+\t\t}\n+\n+\t\tcmd->fn(opt, input.buf + strlen(cmd->prefix) + !!cmd->args,\n+\t\t\tinput.buf + input.len, output, data);\n+\t}\n+\n+\tstrbuf_release(&input);\n+}\n+\n static int batch_objects(struct batch_options *opt)\n {\n \tstruct strbuf input = STRBUF_INIT;\n@@ -514,6 +617,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@@ -589,7 +693,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@@ -607,6 +712,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@@ -684,6 +792,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\t\tbatch_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\tOPT_BOOL(0, \"stdin-cmd\", &batch.stdin_cmd,\n+\t\t\t N_(\"with --batch[-check]: enters stdin 'command mode\")),\n+\t\tOPT_BOOL('z', NULL, &batch.end_null, N_(\"with --stdin-cmd, use NUL termination\")),\n+\n \t\t/* Batch-specific options */\n \t\tOPT_GROUP(N_(\"Change or optimize batch output\")),\n \t\tOPT_BOOL(0, \"buffer\", &batch.buffer_output, N_(\"buffer --batch output\")),\n@@ -737,6 +849,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-- \n2.34.0.rc1.741.gab7bfd97031\n\n"},{"id":"440661","messageId":"20211108034254.ycdhvkdng63abput@Johns-MacBook-Pro-3.local","threadId":"56851","inReplyTo":"211106.86k0hmgc8q.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] cat-file: force flush of stdout on empty string","fromName":"John Cai","fromEmail":"jcai@gitlab.com","sentAt":"2021-11-08T03:42:54Z","receivedAt":"2021-11-08T03:42:59Z","isPatch":true,"sender":{"key":"jcai@gitlab.com","avatar":"https://gravatar.com/avatar/4ba958d432b21b53dd76009b44d31b70bd387a31d2957cee1b257ced4bf812f8?d=mp&s=160"},"body":"O Sat, Nov 06, 2021 at 05:01:10AM +0100, Ævar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Nov 05 2021, Junio C Hamano wrote:\n> \n> > \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> >\n> >> @@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name,\n> >>  \tint flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;\n> >>  \tenum get_oid_result result;\n> >>  \n> >> +\tif (opt->buffer_output && obj_name[0] == '\\0') {\n> >> +\t\tfflush(stdout);\n> >> +\t\treturn;\n> >> +\t}\n> >> +\n> >\n> > This might work in practice, but it a bad design taste to add this\n> > change here.  The function is designed to take an object name\n> > string, and it even prepares a flag variable needed to make a call\n> > to turn that object name into object data.  We do not need to\n> > contaminate the interface with \"usually this takes an object name,\n> > but there are these other special cases ...\".  The higher in the\n> > callchain we place special cases, the better the lower level\n> > functions become, as that allows them to concentrate on doing one\n> > single thing well.\n> >\n> >>  \tresult = get_oid_with_context(the_repository, obj_name,\n> >>  \t\t\t\t      flags, &data->oid, &ctx);\n> >>  \tif (result != FOUND) {\n> >> @@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt)\n> >>  \t\t\tdata.rest = p;\n> >>  \t\t}\n> >>  \n> >> -\t\tbatch_one_object(input.buf, &output, opt, &data);\n> >> +\t\t /*\n> >> +\t\t  * When in buffer mode and input.buf is an empty string,\n> >> +\t\t  * flush to stdout.\n> >> +\t\t  */\n> >\n> > Checking \"do we have the flush instruction (in which case we'd do\n> > the flush here), or do we have textual name of an object (in which\n> > case we'd call batch_one_object())?\" here would be far cleaner and\n> > results in an easier-to-explain code.  With a cleanly written code\n> > to do so, it probably does not even need a new comment here.\n> >\n> > This brings up another issue.  Is \"flushing\" the *ONLY* special\n> > thing we would ever do in this codepath in the future?  I doubt so.\n> > Squatting on an \"empty string\" is a selfish design that hurts those\n> > who will come after you in the future, as they need to find other\n> > ways to ask for a \"special thing\".\n> >\n> > If we are inventing a special syntax that allows us to spell\n> > commands that are distinguishable from a validly-spelled object name\n> > to cause something special (like \"flushing the output stream\"),\n> > perhaps we want to use a bit more extensible and explicit syntax and\n> > use it from day one?\n> >\n> > For example, if no string that begins with three dots can ever be a\n> > valid way to spell an object name, perhaps \"...flush\" might be a\n> > better \"please do this special thing\" syntax than an empty string.\n> > It is easily extensible (the next special thing can follow suit to\n> > say \"...$verb\" to tell the machinery to $verb the input).  When we\n> > compare between an empty string and \"...flush\", the latter clearly\n> > is more descriptive, too.\n> >\n> > Note that I offhand do not know if \"a valid string that name an\n> > object would never begin with three-dot\" is true.  Please check\n> > if that is true if you choose to use it, or you can find and use\n> > another convention that allows us to clearly distinguish the\n> > \"special\" instruction and object names.\n> \n> I had much the same thought, this is a useful feature, but let's not\n> squat on the one bit of open syntax we have.\n> \n> John: I think a better direction here is to add a mode to cat-file to\n> emulate what \"git update-ref --stdin\" supports. Here's a demo of that\n> (also quoted below):\n> https://github.com/git/git/commit/7794f6cfdbdca0dd6bab0dea16193ebf018b86a9\n> \n> That's on top of some general UI improvements to cat-file I've got\n> locally:\n> https://github.com/git/git/compare/master...avar:avar/cat-file-usage-and-options-handling\n> \n> That WIP patch on top follows below, of course it's a *lot* more initial\n> scaffolding, but I think once we get past that initial step it's a much\n> better path forward. As noted the code is also almost entirely\n> copy/pasted from update-ref.c, and perhaps some of the shared parts\n> could be moved to some library both could use.\n> \n> I couldn't think of a better name than --stdin-cmd, suggestions most\n> welcome.\n> \n> From 7794f6cfdbdca0dd6bab0dea16193ebf018b86a9 Mon Sep 17 00:00:00 2001\n> Message-Id: <patch-1.1-7794f6cfdbd-20211106T040307Z-avarab@gmail.com>\n> From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n>  <avarab@gmail.com>\n> Date: Sat, 6 Nov 2021 04:54:04 +0100\n> Subject: [PATCH] WIP cat-file: add a --stdin-cmd mode\n> MIME-Version: 1.0\n> Content-Type: text/plain; charset=UTF-8\n> Content-Transfer-Encoding: 8bit\n> \n> This WIP patch is mostly stealing code from builtin/update-ref.c and\n> implementing the same sort of prefixed command-mode that it\n> supports. I.e. in addition to --batch now supporting:\n> \n>     <object> LF\n> \n> It'll support with --stdin-cmd, with and without -z, respectively:\n> \n>     object <object> NL\n>     object <object> NUL\n> \n> The plus being that we can now implement additional commands. Let's\n> start that by scratching the itch John Cai wanted to address in [1]\n> and implement a (with and without -z):\n> \n>     fflush NL\n>     fflush NUL\n> \n> That command simply calls fflush(stdout), which could be done as an\n> emergent effect before by feeding the input a \"NL\".\n> \n> I think this will be useful for other things, e.g. I've observed in\n> the past that a not-trivial part of \"cat-file --batch\" time is spent\n> on parsing its <object> argument and seeing if it's a revision, ref\n> etc.\n> \n> So we could e.g. add a command that only accepts a full-length 40\n> character SHA-1, or switch the --format output mid-request etc.\n> \n> 1. https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n> ---\n>  builtin/cat-file.c | 116 ++++++++++++++++++++++++++++++++++++++++++++-\n>  1 file changed, 115 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index b76f2a00046..afdb976c6e7 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -26,7 +26,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> @@ -507,6 +510,106 @@ 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 *next, const char *end,\n> +\t\t\t     struct strbuf *output,\n> +\t\t\t     struct expand_data *data)\n> +{\n> +\tsize_t len = end - next - 1;\n> +\tchar *p = (char *)next;\n> +\tchar old = p[len];\n> +\n> +\tp[len] = '\\0';\n> +\tbatch_one_object(next, output, opt, data);\n> +\tp[len] = old;\n> +}\n> +\n> +static void parse_cmd_fflush(struct batch_options *opt,\n> +\t\t\t     const char *next, const char *end,\n> +\t\t\t     struct strbuf *output,\n> +\t\t\t     struct expand_data *data)\n> +{\n> +\tif (*next != line_termination)\n> +\t\tdie(\"fflush: extra input: %s\", next);\n> +\tfflush(stdout);\n> +}\n> +\n> +static const struct parse_cmd {\n> +\tconst char *prefix;\n> +\tvoid (*fn)(struct batch_options *, const char *, const char *, struct strbuf *, struct expand_data *);\n> +\tunsigned args;\n> +\tenum batch_state state;\n> +} command[] = {\n> +\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n> +\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n> +};\nI think overall this approach is cleaner and makes sense. My only\nquestion is, are there more commands in the future that will need some\nspecial command syntax? Just wondering whether YAGNI applies here.\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\tsize_t i, j;\n> +\t\tconst struct parse_cmd *cmd = NULL;\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(command); i++) {\n> +\t\t\tconst char *prefix = command[i].prefix;\n> +\t\t\tchar c;\n> +\n> +\t\t\tif (!starts_with(input.buf, prefix))\n> +\t\t\t\tcontinue;\n> +\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 = command[i].args ? ' ' : line_termination;\n> +\t\t\tif (input.buf[strlen(prefix)] != c)\n> +\t\t\t\tcontinue;\n> +\n> +\t\t\tcmd = &command[i];\n> +\t\t\tbreak;\n> +\t\t}\n> +\t\tif (!cmd)\n> +\t\t\tdie(\"unknown command: %s\", input.buf);\n> +\n> +\t\t/*\n> +\t\t * Read additional arguments if NUL-terminated. Do not raise an\n> +\t\t * error in case there is an early EOF to let the command\n> +\t\t * handle missing arguments with a proper error message.\n> +\t\t */\n> +\t\tfor (j = 1; line_termination == '\\0' && j < cmd->args; j++)\n> +\t\t\tif (strbuf_appendwholeline(&input, stdin, line_termination))\n> +\t\t\t\tbreak;\n> +\n> +\t\tswitch (state) {\n> +\t\tcase BATCH_STATE_OPEN:\n> +\t\t\t/* TODO: command state management */\n> +\t\t\tbreak;\n> +\t\t}\n> +\n> +\t\tcmd->fn(opt, input.buf + strlen(cmd->prefix) + !!cmd->args,\n> +\t\t\tinput.buf + input.len, output, data);\n> +\t}\n> +\n> +\tstrbuf_release(&input);\n> +}\n> +\n>  static int batch_objects(struct batch_options *opt)\n>  {\n>  \tstruct strbuf input = STRBUF_INIT;\n> @@ -514,6 +617,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> @@ -589,7 +693,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> @@ -607,6 +712,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> @@ -684,6 +792,10 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>  \t\t\tbatch_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\tOPT_BOOL(0, \"stdin-cmd\", &batch.stdin_cmd,\n> +\t\t\t N_(\"with --batch[-check]: enters stdin 'command mode\")),\n> +\t\tOPT_BOOL('z', NULL, &batch.end_null, N_(\"with --stdin-cmd, use NUL termination\")),\n> +\n>  \t\t/* Batch-specific options */\n>  \t\tOPT_GROUP(N_(\"Change or optimize batch output\")),\n>  \t\tOPT_BOOL(0, \"buffer\", &batch.buffer_output, N_(\"buffer --batch output\")),\n> @@ -737,6 +849,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> -- \n> 2.34.0.rc1.741.gab7bfd97031\n> \n"},{"id":"440666","messageId":"211108.86h7cmfw33.gmgdl@evledraar.gmail.com","threadId":"56851","inReplyTo":"20211108034254.ycdhvkdng63abput@Johns-MacBook-Pro-3.local","subject":"Re: [PATCH 1/2] cat-file: force flush of stdout on empty string","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-11-08T15:15:38Z","receivedAt":"2021-11-08T16:31:20Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 07 2021, John Cai wrote:\n\n> O Sat, Nov 06, 2021 at 05:01:10AM +0100, Ævar Arnfjörð Bjarmason wrote:\n>> \n>> On Fri, Nov 05 2021, Junio C Hamano wrote:\n>> \n>> > \"John Cai via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n>> >\n>> >> @@ -405,6 +405,11 @@ static void batch_one_object(const char *obj_name,\n>> >>  \tint flags = opt->follow_symlinks ? GET_OID_FOLLOW_SYMLINKS : 0;\n>> >>  \tenum get_oid_result result;\n>> >>  \n>> >> +\tif (opt->buffer_output && obj_name[0] == '\\0') {\n>> >> +\t\tfflush(stdout);\n>> >> +\t\treturn;\n>> >> +\t}\n>> >> +\n>> >\n>> > This might work in practice, but it a bad design taste to add this\n>> > change here.  The function is designed to take an object name\n>> > string, and it even prepares a flag variable needed to make a call\n>> > to turn that object name into object data.  We do not need to\n>> > contaminate the interface with \"usually this takes an object name,\n>> > but there are these other special cases ...\".  The higher in the\n>> > callchain we place special cases, the better the lower level\n>> > functions become, as that allows them to concentrate on doing one\n>> > single thing well.\n>> >\n>> >>  \tresult = get_oid_with_context(the_repository, obj_name,\n>> >>  \t\t\t\t      flags, &data->oid, &ctx);\n>> >>  \tif (result != FOUND) {\n>> >> @@ -609,7 +614,11 @@ static int batch_objects(struct batch_options *opt)\n>> >>  \t\t\tdata.rest = p;\n>> >>  \t\t}\n>> >>  \n>> >> -\t\tbatch_one_object(input.buf, &output, opt, &data);\n>> >> +\t\t /*\n>> >> +\t\t  * When in buffer mode and input.buf is an empty string,\n>> >> +\t\t  * flush to stdout.\n>> >> +\t\t  */\n>> >\n>> > Checking \"do we have the flush instruction (in which case we'd do\n>> > the flush here), or do we have textual name of an object (in which\n>> > case we'd call batch_one_object())?\" here would be far cleaner and\n>> > results in an easier-to-explain code.  With a cleanly written code\n>> > to do so, it probably does not even need a new comment here.\n>> >\n>> > This brings up another issue.  Is \"flushing\" the *ONLY* special\n>> > thing we would ever do in this codepath in the future?  I doubt so.\n>> > Squatting on an \"empty string\" is a selfish design that hurts those\n>> > who will come after you in the future, as they need to find other\n>> > ways to ask for a \"special thing\".\n>> >\n>> > If we are inventing a special syntax that allows us to spell\n>> > commands that are distinguishable from a validly-spelled object name\n>> > to cause something special (like \"flushing the output stream\"),\n>> > perhaps we want to use a bit more extensible and explicit syntax and\n>> > use it from day one?\n>> >\n>> > For example, if no string that begins with three dots can ever be a\n>> > valid way to spell an object name, perhaps \"...flush\" might be a\n>> > better \"please do this special thing\" syntax than an empty string.\n>> > It is easily extensible (the next special thing can follow suit to\n>> > say \"...$verb\" to tell the machinery to $verb the input).  When we\n>> > compare between an empty string and \"...flush\", the latter clearly\n>> > is more descriptive, too.\n>> >\n>> > Note that I offhand do not know if \"a valid string that name an\n>> > object would never begin with three-dot\" is true.  Please check\n>> > if that is true if you choose to use it, or you can find and use\n>> > another convention that allows us to clearly distinguish the\n>> > \"special\" instruction and object names.\n>> \n>> I had much the same thought, this is a useful feature, but let's not\n>> squat on the one bit of open syntax we have.\n>> \n>> John: I think a better direction here is to add a mode to cat-file to\n>> emulate what \"git update-ref --stdin\" supports. Here's a demo of that\n>> (also quoted below):\n>> https://github.com/git/git/commit/7794f6cfdbdca0dd6bab0dea16193ebf018b86a9\n>> \n>> That's on top of some general UI improvements to cat-file I've got\n>> locally:\n>> https://github.com/git/git/compare/master...avar:avar/cat-file-usage-and-options-handling\n>> \n>> That WIP patch on top follows below, of course it's a *lot* more initial\n>> scaffolding, but I think once we get past that initial step it's a much\n>> better path forward. As noted the code is also almost entirely\n>> copy/pasted from update-ref.c, and perhaps some of the shared parts\n>> could be moved to some library both could use.\n>> \n>> I couldn't think of a better name than --stdin-cmd, suggestions most\n>> welcome.\n>> \n>> From 7794f6cfdbdca0dd6bab0dea16193ebf018b86a9 Mon Sep 17 00:00:00 2001\n>> Message-Id: <patch-1.1-7794f6cfdbd-20211106T040307Z-avarab@gmail.com>\n>> From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?=\n>>  <avarab@gmail.com>\n>> Date: Sat, 6 Nov 2021 04:54:04 +0100\n>> Subject: [PATCH] WIP cat-file: add a --stdin-cmd mode\n>> MIME-Version: 1.0\n>> Content-Type: text/plain; charset=UTF-8\n>> Content-Transfer-Encoding: 8bit\n>> \n>> This WIP patch is mostly stealing code from builtin/update-ref.c and\n>> implementing the same sort of prefixed command-mode that it\n>> supports. I.e. in addition to --batch now supporting:\n>> \n>>     <object> LF\n>> \n>> It'll support with --stdin-cmd, with and without -z, respectively:\n>> \n>>     object <object> NL\n>>     object <object> NUL\n>> \n>> The plus being that we can now implement additional commands. Let's\n>> start that by scratching the itch John Cai wanted to address in [1]\n>> and implement a (with and without -z):\n>> \n>>     fflush NL\n>>     fflush NUL\n>> \n>> That command simply calls fflush(stdout), which could be done as an\n>> emergent effect before by feeding the input a \"NL\".\n>> \n>> I think this will be useful for other things, e.g. I've observed in\n>> the past that a not-trivial part of \"cat-file --batch\" time is spent\n>> on parsing its <object> argument and seeing if it's a revision, ref\n>> etc.\n>> \n>> So we could e.g. add a command that only accepts a full-length 40\n>> character SHA-1, or switch the --format output mid-request etc.\n>> \n>> 1. https://lore.kernel.org/git/pull.1124.git.git.1636149400.gitgitgadget@gmail.com/\n>> \n>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n>> ---\n>>  builtin/cat-file.c | 116 ++++++++++++++++++++++++++++++++++++++++++++-\n>>  1 file changed, 115 insertions(+), 1 deletion(-)\n>> \n>> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n>> index b76f2a00046..afdb976c6e7 100644\n>> --- a/builtin/cat-file.c\n>> +++ b/builtin/cat-file.c\n>> @@ -26,7 +26,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>> @@ -507,6 +510,106 @@ 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 *next, const char *end,\n>> +\t\t\t     struct strbuf *output,\n>> +\t\t\t     struct expand_data *data)\n>> +{\n>> +\tsize_t len = end - next - 1;\n>> +\tchar *p = (char *)next;\n>> +\tchar old = p[len];\n>> +\n>> +\tp[len] = '\\0';\n>> +\tbatch_one_object(next, output, opt, data);\n>> +\tp[len] = old;\n>> +}\n>> +\n>> +static void parse_cmd_fflush(struct batch_options *opt,\n>> +\t\t\t     const char *next, const char *end,\n>> +\t\t\t     struct strbuf *output,\n>> +\t\t\t     struct expand_data *data)\n>> +{\n>> +\tif (*next != line_termination)\n>> +\t\tdie(\"fflush: extra input: %s\", next);\n>> +\tfflush(stdout);\n>> +}\n>> +\n>> +static const struct parse_cmd {\n>> +\tconst char *prefix;\n>> +\tvoid (*fn)(struct batch_options *, const char *, const char *, struct strbuf *, struct expand_data *);\n>> +\tunsigned args;\n>> +\tenum batch_state state;\n>> +} command[] = {\n>> +\t{ \"object\", parse_cmd_object, 1, BATCH_STATE_OPEN },\n>> +\t{ \"fflush\", parse_cmd_fflush, 0, BATCH_STATE_OPEN },\n>> +};\n> I think overall this approach is cleaner and makes sense. My only\n> question is, are there more commands in the future that will need some\n> special command syntax? Just wondering whether YAGNI applies here.\n\nAn obvious addition is to at least add the ability to set the various\noptions on the fly, i.e. now you need to use --batch-check, and then\nkill it and restart if you'd like the content with --batch, ditto for\n--textconv.\n\nE.g. the gitaly backend for gitlab.com keeps two cat-filfe processes\naround just to flip-flop between those two, sometimes you want the\ncontent, sometimes you're just checking if the object exists.\n\nI'd also like to add something to expose the likes of -e and -t\ndirectly, i.e. even with --batch-check you often want to just check\nexistence, but get the size too, you could supply a format, but like the\nabove you sometimes want the size or whatever, and killing/starting a\nnew process just for that is a hassle...\n"},{"id":"440680","messageId":"xmqqk0hitnkc.fsf@gitster.g","threadId":"56851","inReplyTo":"211108.86h7cmfw33.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH 1/2] cat-file: force flush of stdout on empty string","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-11-08T20:11:31Z","receivedAt":"2021-11-08T20:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> I think overall this approach is cleaner and makes sense. My only\n>> question is, are there more commands in the future that will need some\n>> special command syntax? Just wondering whether YAGNI applies here.\n>\n> An obvious addition is to at least add the ability to set the various\n> options on the fly, i.e. now you need to use --batch-check, and then\n> kill it and restart if you'd like the content with --batch, ditto for\n> --textconv.\n>\n> E.g. the gitaly backend for gitlab.com keeps two cat-filfe processes\n> around just to flip-flop between those two, sometimes you want the\n> content, sometimes you're just checking if the object exists.\n>\n> I'd also like to add something to expose the likes of -e and -t\n> directly, i.e. even with --batch-check you often want to just check\n> existence, but get the size too, you could supply a format, but like the\n> above you sometimes want the size or whatever, and killing/starting a\n> new process just for that is a hassle...\n\nYeah, with \"plug\" and \"unplug\" instruction you do not have to keep\nissuing \"flush\" when you want to go interactive, and other things\nbecome easy to do, so even though it would make it a bit more\nverbose to require \"object \" prefix for the kind of lines that were\nhistorically the only ones accepted by the command, I think it is a\ngood direction to go in.\n\nThanks.\n"}]}