{"thread":{"id":"59827","subject":"[PATCH 0/5] cat-file: introduce NUL-terminated output format","startedAt":"2023-06-02T13:02:39Z","lastAt":"2023-06-12T20:44:34Z","messageCount":22,"participants":["Patrick Steinhardt","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"477949","messageId":"cover.1685710884.git.ps@pks.im","threadId":"59827","inReplyTo":null,"subject":"[PATCH 0/5] cat-file: introduce NUL-terminated output format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-02T13:02:26Z","receivedAt":"2023-06-02T13:02:39Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nwith Git v2.38.0, we have introduced the `-z` option to git-cat-file(1)\nthat causes it to read NUL-terminated queries from standard input\ninstead of the newline-terminated queries. This fixes issues when the\nquery themselves contains a newline, like it can happen if e.g. we need\nto pass a path-scoped revision with embedded newlines.\n\nUnfortunately, this change is causing a new problem. When the object\ncannot be looked up, then we print an error message that echoes the\ninput:\n\n```\n$ echo does-not-exist | git cat-file --batch\ndoes-not-exist missing\n```\n\nNow if the input itself contains newlines then the output can become\ninherently unparsable:\n\n```\n$ printf \"7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\\n1234567890\\n\\n\\commit000\" |\n    git cat-file --batch -z\n7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\n1234567890\n\ncommit missing\n```\n\nIdeally, `-z` should have also switched the output to be NUL-terminated.\nBut it didn't, and we cannot change this issue now without doing a\nbackwards incompatible change. Instead, this series introduces a new\n`-Z` mode that switches both input and output to be NUL-terminated to\nfix the issue.\n\nNote that Toon has sent a patch series for this issue to address the\nsame issue, see the thread starting at [1]. I've collaborated with him\ninternally at GitLab to arrive at this new patch series which thus\neffectively supersedes the patches he has sent. The approach is also\ndifferent: while his patches start quoting the output, the approach\nchosen by my series only changes the lines to be NUL terminated. This\nshould make it easier to use for scripted purposes compared to having to\nde-quote the input.\n\nI've put all folks that participated in the original thread into Cc.\n\nPatrick\n\n[1]: <20221209150048.2400648-1-toon@iotcl.com>\n\nPatrick Steinhardt (5):\n  t1006: don't strip timestamps from expected results\n  t1006: modernize test style to use `test_cmp`\n  strbuf: provide CRLF-aware helper to read until a specified delimiter\n  cat-file: simplify reading from standard input\n  cat-file: Introduce new option to delimit output with NUL characters\n\n Documentation/git-cat-file.txt |  13 +-\n builtin/cat-file.c             |  85 +++++------\n strbuf.c                       |  11 +-\n strbuf.h                       |  12 ++\n t/t1006-cat-file.sh            | 249 +++++++++++++++++++++------------\n 5 files changed, 232 insertions(+), 138 deletions(-)\n\n-- \n2.40.1\n\n"},{"id":"477950","messageId":"5c8b4a1d70b170e3426cdd537edc0c076115be0a.1685710884.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"[PATCH 1/5] t1006: don't strip timestamps from expected results","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-02T13:02:30Z","receivedAt":"2023-06-02T13:02:41Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1006 we have a bunch of tests that verify the output format of the\ngit-cat-file(1) command. But while part of the output for some tests\nwould include commit timestamps, we don't verify those but instead strip\nthem before comparing expected with actual results. This is done by the\nfunction `maybe_remove_timestamp`, which goes all the way back to the\nancient commit b335d3f121 (Add tests for git cat-file, 2008-04-23).\n\nOur tests had been in a different shape back then. Most importantly we\ndidn't yet have the infrastructure to create objects with deterministic\ntimestamps. Nowadays we do though, and thus there is no reason anymore\nto strip the timestamps.\n\nRefactor the tests to not strip the timestamp anymore.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t1006-cat-file.sh | 68 +++++++++++++++++----------------------------\n 1 file changed, 26 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 8eac74b59c..f139d56fb4 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -109,26 +109,12 @@ strlen () {\n     echo_without_newline \"$1\" | wc -c | sed -e 's/^ *//'\n }\n \n-maybe_remove_timestamp () {\n-\tif test -z \"$2\"; then\n-\t\techo_without_newline \"$1\"\n-\telse\n-\t\techo_without_newline \"$(printf '%s\\n' \"$1\" | remove_timestamp)\"\n-\tfi\n-}\n-\n-remove_timestamp () {\n-\tsed -e 's/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//'\n-}\n-\n-\n run_tests () {\n     type=$1\n     sha1=$2\n     size=$3\n     content=$4\n     pretty_content=$5\n-    no_ts=$6\n \n     batch_output=\"$sha1 $type $size\n $content\"\n@@ -163,21 +149,21 @@ $content\"\n \n     test -z \"$content\" ||\n     test_expect_success \"Content of $type is correct\" '\n-\tmaybe_remove_timestamp \"$content\" $no_ts >expect &&\n-\tmaybe_remove_timestamp \"$(git cat-file $type $sha1)\" $no_ts >actual &&\n+\techo_without_newline \"$content\" >expect &&\n+\tgit cat-file $type $sha1 >actual &&\n \ttest_cmp expect actual\n     '\n \n     test_expect_success \"Pretty content of $type is correct\" '\n-\tmaybe_remove_timestamp \"$pretty_content\" $no_ts >expect &&\n-\tmaybe_remove_timestamp \"$(git cat-file -p $sha1)\" $no_ts >actual &&\n+\techo_without_newline \"$pretty_content\" >expect &&\n+\tgit cat-file -p $sha1 >actual &&\n \ttest_cmp expect actual\n     '\n \n     test -z \"$content\" ||\n     test_expect_success \"--batch output of $type is correct\" '\n-\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n-\tmaybe_remove_timestamp \"$(echo $sha1 | git cat-file --batch)\" $no_ts >actual &&\n+\techo \"$batch_output\" >expect &&\n+\techo $sha1 | git cat-file --batch >actual &&\n \ttest_cmp expect actual\n     '\n \n@@ -191,9 +177,8 @@ $content\"\n     do\n \ttest -z \"$content\" ||\n \t\ttest_expect_success \"--batch-command $opt output of $type content is correct\" '\n-\t\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n-\t\tmaybe_remove_timestamp \"$(test_write_lines \"contents $sha1\" |\n-\t\tgit cat-file --batch-command $opt)\" $no_ts >actual &&\n+\t\techo \"$batch_output\" >expect &&\n+\t\ttest_write_lines \"contents $sha1\" | git cat-file --batch-command $opt >actual &&\n \t\ttest_cmp expect actual\n \t'\n \n@@ -228,10 +213,9 @@ $content\"\n     test_expect_success \"--batch without type ($type)\" '\n \t{\n \t\techo \"$size\" &&\n-\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t\techo \"$content\"\n \t} >expect &&\n-\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual.full &&\n-\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual &&\n \ttest_cmp expect actual\n     '\n \n@@ -239,10 +223,9 @@ $content\"\n     test_expect_success \"--batch without size ($type)\" '\n \t{\n \t\techo \"$type\" &&\n-\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t\techo \"$content\"\n \t} >expect &&\n-\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual.full &&\n-\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual &&\n \ttest_cmp expect actual\n     '\n }\n@@ -284,7 +267,7 @@ test_expect_success '--batch-check without %(rest) considers whole line' '\n \n tree_sha1=$(git write-tree)\n tree_size=$(($(test_oid rawsz) + 13))\n-tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n+tree_pretty_content=\"100644 blob $hello_sha1\thello${LF}\"\n \n run_tests 'tree' $tree_sha1 $tree_size \"\" \"$tree_pretty_content\"\n \n@@ -292,12 +275,12 @@ commit_message=\"Initial commit\"\n commit_sha1=$(echo_without_newline \"$commit_message\" | git commit-tree $tree_sha1)\n commit_size=$(($(test_oid hexsz) + 137))\n commit_content=\"tree $tree_sha1\n-author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 0 +0000\n-committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 0 +0000\n+author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n \n $commit_message\"\n \n-run_tests 'commit' $commit_sha1 $commit_size \"$commit_content\" \"$commit_content\" 1\n+run_tests 'commit' $commit_sha1 $commit_size \"$commit_content\" \"$commit_content\"\n \n tag_header_without_timestamp=\"object $hello_sha1\n type blob\n@@ -311,7 +294,7 @@ $tag_description\"\n tag_sha1=$(echo_without_newline \"$tag_content\" | git hash-object -t tag --stdin -w)\n tag_size=$(strlen \"$tag_content\")\n \n-run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n+run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\"\n \n test_expect_success \\\n     \"Reach a blob from a tag pointing to it\" \\\n@@ -400,13 +383,16 @@ deadbeef missing\n  missing\"\n \n test_expect_success '--batch with multiple sha1s gives correct format' '\n-\ttest \"$(maybe_remove_timestamp \"$batch_output\" 1)\" = \"$(maybe_remove_timestamp \"$(echo_without_newline \"$batch_input\" | git cat-file --batch)\" 1)\"\n+\techo \"$batch_output\" >expect &&\n+\techo_without_newline \"$batch_input\" | git cat-file --batch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--batch, -z with multiple sha1s gives correct format' '\n \techo_without_newline_nul \"$batch_input\" >in &&\n-\ttest \"$(maybe_remove_timestamp \"$batch_output\" 1)\" = \\\n-\t\"$(maybe_remove_timestamp \"$(git cat-file --batch -z <in)\" 1)\"\n+\techo \"$batch_output\" >expect &&\n+\tgit cat-file --batch -z <in >actual &&\n+\ttest_cmp expect actual\n '\n \n batch_check_input=\"$hello_sha1\n@@ -480,7 +466,7 @@ contents deadbeef\n flush\"\n \n test_expect_success '--batch-command with multiple command calls gives correct format' '\n-\tremove_timestamp >expect <<-EOF &&\n+\tcat >expect <<-EOF &&\n \t$hello_sha1 blob $hello_size\n \t$hello_content\n \t$commit_sha1 commit $commit_size\n@@ -491,15 +477,13 @@ test_expect_success '--batch-command with multiple command calls gives correct f\n \tEOF\n \n \techo \"$batch_command_multiple_contents\" >in &&\n-\tgit cat-file --batch-command --buffer <in >actual_raw &&\n+\tgit cat-file --batch-command --buffer <in >actual &&\n \n-\tremove_timestamp <actual_raw >actual &&\n \ttest_cmp expect actual &&\n \n \techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n-\tgit cat-file --batch-command --buffer -z <in >actual_raw &&\n+\tgit cat-file --batch-command --buffer -z <in >actual &&\n \n-\tremove_timestamp <actual_raw >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.40.1\n\n"},{"id":"477951","messageId":"251fc2a387067bb6d575cfa39f088d4474844a59.1685710884.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"[PATCH 2/5] t1006: modernize test style to use `test_cmp`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-02T13:02:34Z","receivedAt":"2023-06-02T13:02:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The tests for git-cat-file(1) are quite old and haven't ever been\nupdated since they were introduced. They thus tend to use old idioms\nthat have since grown outdated. Most importantly, many of the tests use\n`test $A = $B` to compare expected and actual output. This has the\ndownside that it is impossible to tell what exactly is different between\nboth versions in case the test fails.\n\nRefactor the tests to instead use `test_cmp`. While more verbose, it\nboth tends to be more readable and will result in a nice diff in case\nstates don't match.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t1006-cat-file.sh | 70 ++++++++++++++++++++++++++++++---------------\n 1 file changed, 47 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex f139d56fb4..7b985cfded 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -296,9 +296,11 @@ tag_size=$(strlen \"$tag_content\")\n \n run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\"\n \n-test_expect_success \\\n-    \"Reach a blob from a tag pointing to it\" \\\n-    \"test '$hello_content' = \\\"\\$(git cat-file blob $tag_sha1)\\\"\"\n+test_expect_success \"Reach a blob from a tag pointing to it\" '\n+\techo_without_newline \"$hello_content\" >expect &&\n+\tgit cat-file blob $tag_sha1 >actual &&\n+\ttest_cmp expect actual\n+'\n \n for batch in batch batch-check batch-command\n do\n@@ -334,30 +336,47 @@ do\n done\n \n test_expect_success \"--batch-check for a non-existent named object\" '\n-    test \"foobar42 missing\n-foobar84 missing\" = \\\n-    \"$( ( echo foobar42 && echo_without_newline foobar84 ) | git cat-file --batch-check)\"\n+\tcat >expect <<-EOF &&\n+\tfoobar42 missing\n+\tfoobar84 missing\n+\tEOF\n+\n+\tprintf \"foobar42\\nfoobar84\" >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check for a non-existent hash\" '\n-    test \"0000000000000000000000000000000000000042 missing\n-0000000000000000000000000000000000000084 missing\" = \\\n-    \"$( ( echo 0000000000000000000000000000000000000042 &&\n-\t echo_without_newline 0000000000000000000000000000000000000084 ) |\n-       git cat-file --batch-check)\"\n+\tcat >expect <<-EOF &&\n+\t0000000000000000000000000000000000000042 missing\n+\t0000000000000000000000000000000000000084 missing\n+\tEOF\n+\n+\tprintf \"0000000000000000000000000000000000000042\\n0000000000000000000000000000000000000084\" >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch for an existent and a non-existent hash\" '\n-    test \"$tag_sha1 tag $tag_size\n-$tag_content\n-0000000000000000000000000000000000000000 missing\" = \\\n-    \"$( ( echo $tag_sha1 &&\n-\t echo_without_newline 0000000000000000000000000000000000000000 ) |\n-       git cat-file --batch)\"\n+\tcat >expect <<-EOF &&\n+\t$tag_sha1 tag $tag_size\n+\t$tag_content\n+\t0000000000000000000000000000000000000000 missing\n+\tEOF\n+\n+\tprintf \"$tag_sha1\\n0000000000000000000000000000000000000000\" >in &&\n+\tgit cat-file --batch <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check for an empty line\" '\n-    test \" missing\" = \"$(echo | git cat-file --batch-check)\"\n+\tcat >expect <<-EOF &&\n+\t missing\n+\tEOF\n+\n+\techo >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'empty --batch-check notices missing object' '\n@@ -384,7 +403,8 @@ deadbeef missing\n \n test_expect_success '--batch with multiple sha1s gives correct format' '\n \techo \"$batch_output\" >expect &&\n-\techo_without_newline \"$batch_input\" | git cat-file --batch >actual &&\n+\techo_without_newline \"$batch_input\" >in &&\n+\tgit cat-file --batch <in >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -411,13 +431,17 @@ deadbeef missing\n  missing\"\n \n test_expect_success \"--batch-check with multiple sha1s gives correct format\" '\n-    test \"$batch_check_output\" = \\\n-    \"$(echo_without_newline \"$batch_check_input\" | git cat-file --batch-check)\"\n+\techo \"$batch_check_output\" >expect &&\n+\techo_without_newline \"$batch_check_input\" >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check, -z with multiple sha1s gives correct format\" '\n-    echo_without_newline_nul \"$batch_check_input\" >in &&\n-    test \"$batch_check_output\" = \"$(git cat-file --batch-check -z <in)\"\n+\techo \"$batch_check_output\" >expect &&\n+\techo_without_newline_nul \"$batch_check_input\" >in &&\n+\tgit cat-file --batch-check -z <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n-- \n2.40.1\n\n"},{"id":"477952","messageId":"8127eeac97200da9aafccdf16cb7ba06f68b0121.1685710884.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"[PATCH 3/5] strbuf: provide CRLF-aware helper to read until a specified delimiter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-02T13:02:38Z","receivedAt":"2023-06-02T13:02:49Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Many of our commands support reading input that is separated either via\nnewlines or via NUL characters. Furthermore, in order to be a better\ncross platform citizen, these commands typically know to strip the CRLF\nsequence so that we also support reading newline-separated inputs on\ne.g. the Windows platform. This results in the following kind of awkward\npattern:\n\n```\nstruct strbuf input = STRBUF_INIT;\n\nwhile (1) {\n\tint ret;\n\n\tif (nul_terminated)\n\t\tret = strbuf_getline_nul(&input, stdin);\n\telse\n\t\tret = strbuf_getline(&input, stdin);\n\tif (ret)\n\t\tbreak;\n\n\t...\n}\n```\n\nIntroduce a new CRLF-aware helper function that can read up to a user\nspecified delimiter. If the delimiter is `\\n` the function knows to also\nstrip CRLF, otherwise it will only strip the specified delimiter. This\nresults in the following, much more readable code pattern:\n\n```\nstruct strbuf input = STRBUF_INIT;\n\nwhile (strbuf_getdelim_strip_crlf(&input, stdin, delim) != EOF) {\n\t...\n}\n```\n\nThe new function will be used in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n strbuf.c | 11 ++++++++---\n strbuf.h | 12 ++++++++++++\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 08eec8f1d8..31dc48c0ab 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -721,11 +721,11 @@ static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)\n \treturn 0;\n }\n \n-int strbuf_getline(struct strbuf *sb, FILE *fp)\n+int strbuf_getdelim_strip_crlf(struct strbuf *sb, FILE *fp, int term)\n {\n-\tif (strbuf_getwholeline(sb, fp, '\\n'))\n+\tif (strbuf_getwholeline(sb, fp, term))\n \t\treturn EOF;\n-\tif (sb->buf[sb->len - 1] == '\\n') {\n+\tif (term == '\\n' && sb->buf[sb->len - 1] == '\\n') {\n \t\tstrbuf_setlen(sb, sb->len - 1);\n \t\tif (sb->len && sb->buf[sb->len - 1] == '\\r')\n \t\t\tstrbuf_setlen(sb, sb->len - 1);\n@@ -733,6 +733,11 @@ int strbuf_getline(struct strbuf *sb, FILE *fp)\n \treturn 0;\n }\n \n+int strbuf_getline(struct strbuf *sb, FILE *fp)\n+{\n+\treturn strbuf_getdelim_strip_crlf(sb, fp, '\\n');\n+}\n+\n int strbuf_getline_lf(struct strbuf *sb, FILE *fp)\n {\n \treturn strbuf_getdelim(sb, fp, '\\n');\ndiff --git a/strbuf.h b/strbuf.h\nindex 3dfeadb44c..0e69b656bc 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -475,6 +475,18 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n  */\n ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n \n+/**\n+ * Read from a FILE * until the specified terminator is encountered,\n+ * overwriting the existing contents of the strbuf.\n+ *\n+ * Reading stops after the terminator or at EOF.  The terminator is\n+ * removed from the buffer before returning.  If the terminator is LF\n+ * and if it is preceded by a CR, then the whole CRLF is stripped.\n+ * Returns 0 unless there was nothing left before EOF, in which case\n+ * it returns `EOF`.\n+ */\n+int strbuf_getdelim_strip_crlf(struct strbuf *sb, FILE *fp, int term);\n+\n /**\n  * Read a line from a FILE *, overwriting the existing contents of\n  * the strbuf.  The strbuf_getline*() family of functions share\n-- \n2.40.1\n\n"},{"id":"477953","messageId":"e7cba8dc4cebde0841ac04702155890d87e74492.1685710884.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"[PATCH 4/5] cat-file: simplify reading from standard input","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-02T13:02:42Z","receivedAt":"2023-06-02T13:02:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The batch modes of git-cat-file(1) read queries from stantard input that\nare either newline- or NUL-delimited. This code was introduced via\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), which notes that:\n\n\"\"\"\nThe refactoring here is slightly unfortunate, since we turn loops like:\n\n     while (strbuf_getline(&buf, stdin) != EOF)\n\n into:\n\n     while (1) {\n         int ret;\n         if (opt->nul_terminated)\n             ret = strbuf_getline_nul(&input, stdin);\n         else\n             ret = strbuf_getline(&input, stdin);\n\n         if (ret == EOF)\n             break;\n     }\n\"\"\"\n\nThe commit proposed introducing a helper function that is easier to use,\nwhich is just what we have done in the preceding commit. Refactor the\ncode to use this new helper to simplify the loop.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/cat-file.c | 32 +++++++++-----------------------\n 1 file changed, 9 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0bafc14e6c..001dcb24d6 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -42,7 +42,7 @@ struct batch_options {\n \tint all_objects;\n \tint unordered;\n \tint transform_mode; /* may be 'w' or 'c' for --filters or --textconv */\n-\tint nul_terminated;\n+\tchar input_delim;\n \tconst char *format;\n };\n \n@@ -694,20 +694,12 @@ static void batch_objects_command(struct batch_options *opt,\n \tstruct queued_cmd *queued_cmd = NULL;\n \tsize_t alloc = 0, nr = 0;\n \n-\twhile (1) {\n-\t\tint i, ret;\n+\twhile (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {\n+\t\tint i;\n \t\tconst struct parse_cmd *cmd = NULL;\n \t\tconst char *p = NULL, *cmd_end;\n \t\tstruct queued_cmd call = {0};\n \n-\t\tif (opt->nul_terminated)\n-\t\t\tret = strbuf_getline_nul(&input, stdin);\n-\t\telse\n-\t\t\tret = strbuf_getline(&input, stdin);\n-\n-\t\tif (ret)\n-\t\t\tbreak;\n-\n \t\tif (!input.len)\n \t\t\tdie(_(\"empty command in input\"));\n \t\tif (isspace(*input.buf))\n@@ -851,16 +843,7 @@ static int batch_objects(struct batch_options *opt)\n \t\tgoto cleanup;\n \t}\n \n-\twhile (1) {\n-\t\tint ret;\n-\t\tif (opt->nul_terminated)\n-\t\t\tret = strbuf_getline_nul(&input, stdin);\n-\t\telse\n-\t\t\tret = strbuf_getline(&input, stdin);\n-\n-\t\tif (ret == EOF)\n-\t\t\tbreak;\n-\n+\twhile (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != 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@@ -929,6 +912,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tconst char *exp_type = NULL, *obj_name = NULL;\n \tstruct batch_options batch = {0};\n \tint unknown_type = 0;\n+\tint input_nul_terminated = 0;\n \n \tconst char * const usage[] = {\n \t\tN_(\"git cat-file <type> <object>\"),\n@@ -965,7 +949,7 @@ 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_BOOL('z', NULL, &batch.nul_terminated, N_(\"stdin is NUL-terminated\")),\n+\t\tOPT_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n \t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"format\"),\n \t\t\tN_(\"read commands from stdin\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -1024,10 +1008,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \telse if (batch.all_objects)\n \t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n \t\t\t       \"--batch-all-objects\");\n-\telse if (batch.nul_terminated)\n+\telse if (input_nul_terminated)\n \t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n \t\t\t       \"-z\");\n \n+\tbatch.input_delim = input_nul_terminated ? '\\0' : '\\n';\n+\n \t/* Batch defaults */\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n-- \n2.40.1\n\n"},{"id":"477954","messageId":"07a7c34615ec68fa42c725fd34d6144b6b191f03.1685710884.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"[PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-02T13:02:46Z","receivedAt":"2023-06-02T13:03:07Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with\n`-z`, 2022-07-22), we have introduced a new mode to read the input via\nNUL-delimited records instead of newline-delimited records. This allows\nthe user to query for revisions that have newlines in their path\ncomponent. While unusual, such queries are perfectly valid and thus it\nis clear that we should be able to support them properly.\n\nUnfortunately, the commit only changed the input to be NUL-delimited,\nbut didn't change the output at the same time. While this is fine for\nqueries that are processed successfully, it is less so for queries that\naren't. In the case of missing commits for example the result can become\nentirely unparsable:\n\n```\n$ printf \"7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\\n1234567890\\n\\n\\commit000\" |\n    git cat-file --batch -z\n7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\n1234567890\n\ncommit missing\n```\n\nThis is of course a crafted query that is intentionally gaming the\ndeficiency, but more benign queries that contain newlines would have\nsimilar problems.\n\nIdeally, we should have also changed the output to be NUL-delimited when\n`-z` is specified to avoid this problem. As the input is NUL-delimited,\nit is clear that the output in this case cannot ever contain NUL\ncharacters by itself. Furthermore, Git does not allow NUL characters in\nrevisions anyway, further stressing the point that using NUL-delimited\noutput is safe. The only exception is of course the object data itself,\nbut as git-cat-file(1) prints the size of the object data clients should\nread until that specified size has been consumed.\n\nBut even though `-z` has only been introduced a few releases ago in Git\nv2.38.0, changing the output format retroactively to also NUL-delimit\noutput would be a backwards incompatible change. And while one could\nmake the argument that the output is inherently broken already, we need\nto assume that there are existing users out there that use it just fine\ngiven that revisions containing newlines are quite exotic.\n\nInstead, introduce a new option `-Z` that switches to NUL-delimited\ninput and output. The old `-z` option is marked as deprecated with a\nhint that its output may become unparsable.\n\nCo-authored-by: Toon Claes <toon@iotcl.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git-cat-file.txt |  13 +++-\n builtin/cat-file.c             |  55 +++++++++------\n t/t1006-cat-file.sh            | 123 ++++++++++++++++++++++++---------\n 3 files changed, 137 insertions(+), 54 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 411de2e27d..b1f48fdfb1 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n 'git cat-file' (-t | -s) [--allow-unknown-type] <object>\n 'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects]\n \t     [--buffer] [--follow-symlinks] [--unordered]\n-\t     [--textconv | --filters] [-z]\n+\t     [--textconv | --filters] [-z] [-Z]\n 'git cat-file' (--textconv | --filters)\n \t     [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\n \n@@ -246,6 +246,12 @@ respectively print:\n -z::\n \tOnly meaningful with `--batch`, `--batch-check`, or\n \t`--batch-command`; input is NUL-delimited instead of\n+\tnewline-delimited. This option is deprecated in favor of\n+\t`-Z` as the output can otherwise be ambiguous.\n+\n+-Z::\n+\tOnly meaningful with `--batch`, `--batch-check`, or\n+\t`--batch-command`; input and output is NUL-delimited instead of\n \tnewline-delimited.\n \n \n@@ -384,6 +390,11 @@ notdir SP <size> LF\n is printed when, during symlink resolution, a file is used as a\n directory name.\n \n+Alternatively, when `-Z` is passed, the line feeds in any of the above examples\n+are replaced with NUL terminators. This ensures that output will be parsable if\n+the output itself would contain a linefeed and is thus recommended for\n+scripting purposes.\n+\n CAVEATS\n -------\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 001dcb24d6..90ef407d30 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -43,6 +43,7 @@ struct batch_options {\n \tint unordered;\n \tint transform_mode; /* may be 'w' or 'c' for --filters or --textconv */\n \tchar input_delim;\n+\tchar output_delim;\n \tconst char *format;\n };\n \n@@ -437,11 +438,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t}\n }\n \n-static void print_default_format(struct strbuf *scratch, struct expand_data *data)\n+static void print_default_format(struct strbuf *scratch, struct expand_data *data,\n+\t\t\t\t struct batch_options *opt)\n {\n-\tstrbuf_addf(scratch, \"%s %s %\"PRIuMAX\"\\n\", oid_to_hex(&data->oid),\n+\tstrbuf_addf(scratch, \"%s %s %\"PRIuMAX\"%c\", oid_to_hex(&data->oid),\n \t\t    type_name(data->type),\n-\t\t    (uintmax_t)data->size);\n+\t\t    (uintmax_t)data->size, opt->output_delim);\n }\n \n /*\n@@ -470,8 +472,8 @@ static void batch_object_write(const char *obj_name,\n \t\t\t\t\t\t       &data->oid, &data->info,\n \t\t\t\t\t\t       OBJECT_INFO_LOOKUP_REPLACE);\n \t\tif (ret < 0) {\n-\t\t\tprintf(\"%s missing\\n\",\n-\t\t\t       obj_name ? obj_name : oid_to_hex(&data->oid));\n+\t\t\tprintf(\"%s missing%c\",\n+\t\t\t       obj_name ? obj_name : oid_to_hex(&data->oid), opt->output_delim);\n \t\t\tfflush(stdout);\n \t\t\treturn;\n \t\t}\n@@ -492,17 +494,18 @@ static void batch_object_write(const char *obj_name,\n \tstrbuf_reset(scratch);\n \n \tif (!opt->format) {\n-\t\tprint_default_format(scratch, data);\n+\t\tprint_default_format(scratch, data, opt);\n \t} else {\n \t\tstrbuf_expand(scratch, opt->format, expand_format, data);\n-\t\tstrbuf_addch(scratch, '\\n');\n+\t\tstrbuf_addch(scratch, opt->output_delim);\n \t}\n \n \tbatch_write(opt, scratch->buf, scratch->len);\n \n \tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n+\t\tchar buf[] = {opt->output_delim};\n \t\tprint_object_or_die(opt, data);\n-\t\tbatch_write(opt, \"\\n\", 1);\n+\t\tbatch_write(opt, buf, 1);\n \t}\n }\n \n@@ -520,22 +523,25 @@ static void batch_one_object(const char *obj_name,\n \tif (result != FOUND) {\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n-\t\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\t\tprintf(\"%s missing%c\", obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase SHORT_NAME_AMBIGUOUS:\n-\t\t\tprintf(\"%s ambiguous\\n\", obj_name);\n+\t\t\tprintf(\"%s ambiguous%c\", obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase DANGLING_SYMLINK:\n-\t\t\tprintf(\"dangling %\"PRIuMAX\"\\n%s\\n\",\n-\t\t\t       (uintmax_t)strlen(obj_name), obj_name);\n+\t\t\tprintf(\"dangling %\"PRIuMAX\"%c%s%c\",\n+\t\t\t       (uintmax_t)strlen(obj_name),\n+\t\t\t       opt->output_delim, obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase SYMLINK_LOOP:\n-\t\t\tprintf(\"loop %\"PRIuMAX\"\\n%s\\n\",\n-\t\t\t       (uintmax_t)strlen(obj_name), obj_name);\n+\t\t\tprintf(\"loop %\"PRIuMAX\"%c%s%c\",\n+\t\t\t       (uintmax_t)strlen(obj_name),\n+\t\t\t       opt->output_delim, obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase NOT_DIR:\n-\t\t\tprintf(\"notdir %\"PRIuMAX\"\\n%s\\n\",\n-\t\t\t       (uintmax_t)strlen(obj_name), obj_name);\n+\t\t\tprintf(\"notdir %\"PRIuMAX\"%c%s%c\",\n+\t\t\t       (uintmax_t)strlen(obj_name),\n+\t\t\t       opt->output_delim, obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tBUG(\"unknown get_sha1_with_context result %d\\n\",\n@@ -547,9 +553,9 @@ static void batch_one_object(const char *obj_name,\n \t}\n \n \tif (ctx.mode == 0) {\n-\t\tprintf(\"symlink %\"PRIuMAX\"\\n%s\\n\",\n+\t\tprintf(\"symlink %\"PRIuMAX\"%c%s%c\",\n \t\t       (uintmax_t)ctx.symlink_path.len,\n-\t\t       ctx.symlink_path.buf);\n+\t\t       opt->output_delim, ctx.symlink_path.buf, opt->output_delim);\n \t\tfflush(stdout);\n \t\treturn;\n \t}\n@@ -913,6 +919,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tstruct batch_options batch = {0};\n \tint unknown_type = 0;\n \tint input_nul_terminated = 0;\n+\tint nul_terminated = 0;\n \n \tconst char * const usage[] = {\n \t\tN_(\"git cat-file <type> <object>\"),\n@@ -920,7 +927,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tN_(\"git cat-file (-t | -s) [--allow-unknown-type] <object>\"),\n \t\tN_(\"git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\\n\"\n \t\t   \"             [--buffer] [--follow-symlinks] [--unordered]\\n\"\n-\t\t   \"             [--textconv | --filters] [-z]\"),\n+\t\t   \"             [--textconv | --filters] [-z] [-Z]\"),\n \t\tN_(\"git cat-file (--textconv | --filters)\\n\"\n \t\t   \"             [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\"),\n \t\tNULL\n@@ -950,6 +957,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n \t\t\tbatch_option_callback),\n \t\tOPT_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n+\t\tOPT_BOOL('Z', NULL, &nul_terminated, N_(\"stdin and stdout is NUL-terminated\")),\n \t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"format\"),\n \t\t\tN_(\"read commands from stdin\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -1011,8 +1019,15 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \telse if (input_nul_terminated)\n \t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n \t\t\t       \"-z\");\n+\telse if (nul_terminated)\n+\t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n+\t\t\t       \"-Z\");\n \n-\tbatch.input_delim = input_nul_terminated ? '\\0' : '\\n';\n+\tbatch.input_delim = batch.output_delim = '\\n';\n+\tif (input_nul_terminated)\n+\t\tbatch.input_delim = '\\0';\n+\tif (nul_terminated)\n+\t\tbatch.input_delim = batch.output_delim = '\\0';\n \n \t/* Batch defaults */\n \tif (batch.buffer_output < 0)\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 7b985cfded..d73a0be1b9 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -89,7 +89,8 @@ done\n for opt in --buffer \\\n \t--follow-symlinks \\\n \t--batch-all-objects \\\n-\t-z\n+\t-z \\\n+\t-Z\n do\n \ttest_expect_success \"usage: bad option combination: $opt without batch mode\" '\n \t\ttest_incompatible_usage git cat-file $opt &&\n@@ -392,17 +393,18 @@ deadbeef\n \n \"\n \n-batch_output=\"$hello_sha1 blob $hello_size\n-$hello_content\n-$commit_sha1 commit $commit_size\n-$commit_content\n-$tag_sha1 tag $tag_size\n-$tag_content\n-deadbeef missing\n- missing\"\n+printf \"%s\\0\" \\\n+\t\"$hello_sha1 blob $hello_size\" \\\n+\t\"$hello_content\" \\\n+\t\"$commit_sha1 commit $commit_size\" \\\n+\t\"$commit_content\" \\\n+\t\"$tag_sha1 tag $tag_size\" \\\n+\t\"$tag_content\" \\\n+\t\"deadbeef missing\" \\\n+\t\" missing\" >batch_output\n \n test_expect_success '--batch with multiple sha1s gives correct format' '\n-\techo \"$batch_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_output >expect &&\n \techo_without_newline \"$batch_input\" >in &&\n \tgit cat-file --batch <in >actual &&\n \ttest_cmp expect actual\n@@ -410,11 +412,17 @@ test_expect_success '--batch with multiple sha1s gives correct format' '\n \n test_expect_success '--batch, -z with multiple sha1s gives correct format' '\n \techo_without_newline_nul \"$batch_input\" >in &&\n-\techo \"$batch_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_output >expect &&\n \tgit cat-file --batch -z <in >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--batch, -Z with multiple sha1s gives correct format' '\n+\techo_without_newline_nul \"$batch_input\" >in &&\n+\tgit cat-file --batch -Z <in >actual &&\n+\ttest_cmp batch_output actual\n+'\n+\n batch_check_input=\"$hello_sha1\n $tree_sha1\n $commit_sha1\n@@ -423,40 +431,55 @@ deadbeef\n \n \"\n \n-batch_check_output=\"$hello_sha1 blob $hello_size\n-$tree_sha1 tree $tree_size\n-$commit_sha1 commit $commit_size\n-$tag_sha1 tag $tag_size\n-deadbeef missing\n- missing\"\n+printf \"%s\\0\" \\\n+\t\"$hello_sha1 blob $hello_size\" \\\n+\t\"$tree_sha1 tree $tree_size\" \\\n+\t\"$commit_sha1 commit $commit_size\" \\\n+\t\"$tag_sha1 tag $tag_size\" \\\n+\t\"deadbeef missing\" \\\n+\t\" missing\" >batch_check_output\n \n test_expect_success \"--batch-check with multiple sha1s gives correct format\" '\n-\techo \"$batch_check_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_check_output >expect &&\n \techo_without_newline \"$batch_check_input\" >in &&\n \tgit cat-file --batch-check <in >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check, -z with multiple sha1s gives correct format\" '\n-\techo \"$batch_check_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_check_output >expect &&\n \techo_without_newline_nul \"$batch_check_input\" >in &&\n \tgit cat-file --batch-check -z <in >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n+test_expect_success \"--batch-check, -Z with multiple sha1s gives correct format\" '\n+\techo_without_newline_nul \"$batch_check_input\" >in &&\n+\tgit cat-file --batch-check -Z <in >actual &&\n+\ttest_cmp batch_check_output actual\n+'\n+\n+test_expect_success FUNNYNAMES 'setup with newline in input' '\n \ttouch -- \"newline${LF}embedded\" &&\n \tgit add -- \"newline${LF}embedded\" &&\n \tgit commit -m \"file with newline embedded\" &&\n \ttest_tick &&\n \n-\tprintf \"HEAD:newline${LF}embedded\" >in &&\n-\tgit cat-file --batch-check -z <in >actual &&\n+\tprintf \"HEAD:newline${LF}embedded\" >in\n+'\n \n+test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n+\tgit cat-file --batch-check -z <in >actual &&\n \techo \"$(git rev-parse \"HEAD:newline${LF}embedded\") blob 0\" >expect &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n+\tgit cat-file --batch-check -Z <in >actual &&\n+\tprintf \"%s\\0\" \"$(git rev-parse \"HEAD:newline${LF}embedded\") blob 0\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n batch_command_multiple_info=\"info $hello_sha1\n info $tree_sha1\n info $commit_sha1\n@@ -480,7 +503,13 @@ test_expect_success '--batch-command with multiple info calls gives correct form\n \techo \"$batch_command_multiple_info\" | tr \"\\n\" \"\\0\" >in &&\n \tgit cat-file --batch-command --buffer -z <in >actual &&\n \n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\techo \"$batch_command_multiple_info\" | tr \"\\n\" \"\\0\" >in &&\n+\ttr \"\\n\" \"\\0\" <expect >expect_nul &&\n+\tgit cat-file --batch-command --buffer -Z <in >actual &&\n+\n+\ttest_cmp expect_nul actual\n '\n \n batch_command_multiple_contents=\"contents $hello_sha1\n@@ -490,15 +519,15 @@ contents deadbeef\n flush\"\n \n test_expect_success '--batch-command with multiple command calls gives correct format' '\n-\tcat >expect <<-EOF &&\n-\t$hello_sha1 blob $hello_size\n-\t$hello_content\n-\t$commit_sha1 commit $commit_size\n-\t$commit_content\n-\t$tag_sha1 tag $tag_size\n-\t$tag_content\n-\tdeadbeef missing\n-\tEOF\n+\tprintf \"%s\\0\" \\\n+\t\t\"$hello_sha1 blob $hello_size\" \\\n+\t\t\"$hello_content\" \\\n+\t\t\"$commit_sha1 commit $commit_size\" \\\n+\t\t\"$commit_content\" \\\n+\t\t\"$tag_sha1 tag $tag_size\" \\\n+\t\t\"$tag_content\" \\\n+\t\t\"deadbeef missing\" >expect_nul &&\n+\ttr \"\\0\" \"\\n\" <expect_nul >expect &&\n \n \techo \"$batch_command_multiple_contents\" >in &&\n \tgit cat-file --batch-command --buffer <in >actual &&\n@@ -508,7 +537,12 @@ test_expect_success '--batch-command with multiple command calls gives correct f\n \techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n \tgit cat-file --batch-command --buffer -z <in >actual &&\n \n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n+\tgit cat-file --batch-command --buffer -Z <in >actual &&\n+\n+\ttest_cmp expect_nul actual\n '\n \n test_expect_success 'setup blobs which are likely to delta' '\n@@ -848,6 +882,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for brok\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for broken in-repo, same-dir links' '\n+\tprintf \"HEAD:broken-same-dir-link\\0\" >in &&\n+\tprintf \"dangling 25\\0HEAD:broken-same-dir-link\\0\" >expect &&\n+\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git cat-file --batch-check --follow-symlinks works for same-dir links-to-links' '\n \techo HEAD:link-to-link | git cat-file --batch-check --follow-symlinks >actual &&\n \ttest_cmp found actual\n@@ -862,6 +903,15 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for pare\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for parent-dir links' '\n+\techo HEAD:dir/parent-dir-link | git cat-file --batch-check --follow-symlinks >actual &&\n+\ttest_cmp found actual &&\n+\tprintf \"notdir 29\\0HEAD:dir/parent-dir-link/nope\\0\" >expect &&\n+\tprintf \"HEAD:dir/parent-dir-link/nope\\0\" >in &&\n+\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git cat-file --batch-check --follow-symlinks works for .. links' '\n \techo dangling 22 >expect &&\n \techo HEAD:dir/link-dir/nope >>expect &&\n@@ -976,6 +1026,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlink breaks loops' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch-check --follow-symlink -Z breaks loops' '\n+\tprintf \"loop 10\\0HEAD:loop1\\0\" >expect &&\n+\tprintf \"HEAD:loop1\\0\" >in &&\n+\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git cat-file --batch --follow-symlink returns correct sha and mode' '\n \techo HEAD:morx | git cat-file --batch >expect &&\n \techo HEAD:morx | git cat-file --batch --follow-symlinks >actual &&\n-- \n2.40.1\n\n"},{"id":"478003","messageId":"xmqqmt1h7205.fsf@gitster.g","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"Re: [PATCH 0/5] cat-file: introduce NUL-terminated output format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-03T01:44:58Z","receivedAt":"2023-06-03T01:45:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Note that Toon has sent a patch series for this issue to address the\n> same issue, see the thread starting at [1]. I've collaborated with him\n> internally at GitLab to arrive at this new patch series which thus\n> effectively supersedes the patches he has sent. The approach is also\n> different: while his patches start quoting the output, the approach\n> chosen by my series only changes the lines to be NUL terminated. This\n> should make it easier to use for scripted purposes compared to having to\n> de-quote the input.\n\nOK.  Will try to drop the other topic and queue this one instead.\n\nThanks.\n"},{"id":"478047","messageId":"9900512f-b0da-2e47-f1ab-ed51ec2c78ff@gmail.com","threadId":"59827","inReplyTo":"07a7c34615ec68fa42c725fd34d6144b6b191f03.1685710884.git.ps@pks.im","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-05T15:47:14Z","receivedAt":"2023-06-05T15:48:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Patrick\n\nOn 02/06/2023 14:02, Patrick Steinhardt wrote:\n> In db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with\n> `-z`, 2022-07-22), we have introduced a new mode to read the input via\n> NUL-delimited records instead of newline-delimited records. This allows\n> the user to query for revisions that have newlines in their path\n> component. While unusual, such queries are perfectly valid and thus it\n> is clear that we should be able to support them properly.\n> \n> Unfortunately, the commit only changed the input to be NUL-delimited,\n> but didn't change the output at the same time. While this is fine for\n> queries that are processed successfully, it is less so for queries that\n> aren't. In the case of missing commits for example the result can become\n> entirely unparsable:\n> \n> ```\n> $ printf \"7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\\n1234567890\\n\\n\\commit000\" |\n>      git cat-file --batch -z\n> 7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\n> 1234567890\n> \n> commit missing\n> ```\n> \n> This is of course a crafted query that is intentionally gaming the\n> deficiency, but more benign queries that contain newlines would have\n> similar problems.\n> \n> Ideally, we should have also changed the output to be NUL-delimited when\n> `-z` is specified to avoid this problem. As the input is NUL-delimited,\n> it is clear that the output in this case cannot ever contain NUL\n> characters by itself. Furthermore, Git does not allow NUL characters in\n> revisions anyway, further stressing the point that using NUL-delimited\n> output is safe. The only exception is of course the object data itself,\n> but as git-cat-file(1) prints the size of the object data clients should\n> read until that specified size has been consumed.\n> \n> But even though `-z` has only been introduced a few releases ago in Git\n> v2.38.0, changing the output format retroactively to also NUL-delimit\n> output would be a backwards incompatible change. And while one could\n> make the argument that the output is inherently broken already, we need\n> to assume that there are existing users out there that use it just fine\n> given that revisions containing newlines are quite exotic.\n> \n> Instead, introduce a new option `-Z` that switches to NUL-delimited\n> input and output. The old `-z` option is marked as deprecated with a\n> hint that its output may become unparsable.\n\nThe commit message explains the problem well, I agree adding a new \noption is the cleanest solution.\n\n> Co-authored-by: Toon Claes <toon@iotcl.com>\n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>   Documentation/git-cat-file.txt |  13 +++-\n>   builtin/cat-file.c             |  55 +++++++++------\n>   t/t1006-cat-file.sh            | 123 ++++++++++++++++++++++++---------\n>   3 files changed, 137 insertions(+), 54 deletions(-)\n> \n> diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\n> index 411de2e27d..b1f48fdfb1 100644\n> --- a/Documentation/git-cat-file.txt\n> +++ b/Documentation/git-cat-file.txt\n> @@ -14,7 +14,7 @@ SYNOPSIS\n>   'git cat-file' (-t | -s) [--allow-unknown-type] <object>\n>   'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects]\n>   \t     [--buffer] [--follow-symlinks] [--unordered]\n> -\t     [--textconv | --filters] [-z]\n> +\t     [--textconv | --filters] [-z] [-Z]\n>   'git cat-file' (--textconv | --filters)\n>   \t     [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\n>   \n> @@ -246,6 +246,12 @@ respectively print:\n>   -z::\n>   \tOnly meaningful with `--batch`, `--batch-check`, or\n>   \t`--batch-command`; input is NUL-delimited instead of\n> +\tnewline-delimited. This option is deprecated in favor of\n> +\t`-Z` as the output can otherwise be ambiguous.\n> +\n> +-Z::\n> +\tOnly meaningful with `--batch`, `--batch-check`, or\n> +\t`--batch-command`; input and output is NUL-delimited instead of\n>   \tnewline-delimited.\n\nThe documentation changes look good. I wonder if we should put the \ndocumentation for \"-Z\" above \"-z\" so users see the preferred option first.\n\n>   \n> @@ -384,6 +390,11 @@ notdir SP <size> LF\n>   is printed when, during symlink resolution, a file is used as a\n>   directory name.\n>   \n> +Alternatively, when `-Z` is passed, the line feeds in any of the above examples\n> +are replaced with NUL terminators. This ensures that output will be parsable if\n> +the output itself would contain a linefeed and is thus recommended for\n> +scripting purposes.\n> +\n>   CAVEATS\n>   -------\n>   \n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index 001dcb24d6..90ef407d30 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -492,17 +494,18 @@ static void batch_object_write(const char *obj_name,\n>   \tstrbuf_reset(scratch);\n>   \n>   \tif (!opt->format) {\n> -\t\tprint_default_format(scratch, data);\n> +\t\tprint_default_format(scratch, data, opt);\n>   \t} else {\n>   \t\tstrbuf_expand(scratch, opt->format, expand_format, data);\n> -\t\tstrbuf_addch(scratch, '\\n');\n> +\t\tstrbuf_addch(scratch, opt->output_delim);\n>   \t}\n>   \n>   \tbatch_write(opt, scratch->buf, scratch->len);\n>   \n>   \tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n> +\t\tchar buf[] = {opt->output_delim};\n\nI found this a bit confusing, I think it would be clearer just to do\n\n\tbatch_write(opt, &opt->output_delim, 1);\n\n>   \t\tprint_object_or_die(opt, data);\n> -\t\tbatch_write(opt, \"\\n\", 1);\n> +\t\tbatch_write(opt, buf, 1);\n>   \t}\n>   }\n\n> @@ -920,7 +927,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>   \t\tN_(\"git cat-file (-t | -s) [--allow-unknown-type] <object>\"),\n>   \t\tN_(\"git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\\n\"\n>   \t\t   \"             [--buffer] [--follow-symlinks] [--unordered]\\n\"\n> -\t\t   \"             [--textconv | --filters] [-z]\"),\n> +\t\t   \"             [--textconv | --filters] [-z] [-Z]\"),\n\nIf we're recommending that people don't use '-z' then maybe we should \nremove it from the synopsis and add OPT_HIDDEN to it below.\n\n>   \t\tN_(\"git cat-file (--textconv | --filters)\\n\"\n>   \t\t   \"             [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\"),\n>   \t\tNULL\n> @@ -950,6 +957,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n>   \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n>   \t\t\tbatch_option_callback),\n>   \t\tOPT_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n> +\t\tOPT_BOOL('Z', NULL, &nul_terminated, N_(\"stdin and stdout is NUL-terminated\")),\n\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 7b985cfded..d73a0be1b9 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -392,17 +393,18 @@ deadbeef\n>   \n>   \"\n>   \n> -batch_output=\"$hello_sha1 blob $hello_size\n> -$hello_content\n> -$commit_sha1 commit $commit_size\n> -$commit_content\n> -$tag_sha1 tag $tag_size\n> -$tag_content\n> -deadbeef missing\n> - missing\"\n> +printf \"%s\\0\" \\\n> +\t\"$hello_sha1 blob $hello_size\" \\\n> +\t\"$hello_content\" \\\n> +\t\"$commit_sha1 commit $commit_size\" \\\n> +\t\"$commit_content\" \\\n> +\t\"$tag_sha1 tag $tag_size\" \\\n> +\t\"$tag_content\" \\\n> +\t\"deadbeef missing\" \\\n> +\t\" missing\" >batch_output\n\nI think writing the expected output to a file is a good change as we \nalways use it with test_cmp. As \"-z\" is deprecated I think it makes \nsense to model the expected output for \"-Z\" and use tr for the \"-z\" \ntests as you have done here. It looks like we have good coverage of the \nnew option.\n\nThanks for working on this\n\nPhillip\n\n>   test_expect_success '--batch with multiple sha1s gives correct format' '\n> -\techo \"$batch_output\" >expect &&\n> +\ttr \"\\0\" \"\\n\" <batch_output >expect &&\n>   \techo_without_newline \"$batch_input\" >in &&\n>   \tgit cat-file --batch <in >actual &&\n>   \ttest_cmp expect actual\n> @@ -410,11 +412,17 @@ test_expect_success '--batch with multiple sha1s gives correct format' '\n>   \n>   test_expect_success '--batch, -z with multiple sha1s gives correct format' '\n>   \techo_without_newline_nul \"$batch_input\" >in &&\n> -\techo \"$batch_output\" >expect &&\n> +\ttr \"\\0\" \"\\n\" <batch_output >expect &&\n>   \tgit cat-file --batch -z <in >actual &&\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success '--batch, -Z with multiple sha1s gives correct format' '\n> +\techo_without_newline_nul \"$batch_input\" >in &&\n> +\tgit cat-file --batch -Z <in >actual &&\n> +\ttest_cmp batch_output actual\n> +'\n> +\n>   batch_check_input=\"$hello_sha1\n>   $tree_sha1\n>   $commit_sha1\n> @@ -423,40 +431,55 @@ deadbeef\n>   \n>   \"\n>   \n> -batch_check_output=\"$hello_sha1 blob $hello_size\n> -$tree_sha1 tree $tree_size\n> -$commit_sha1 commit $commit_size\n> -$tag_sha1 tag $tag_size\n> -deadbeef missing\n> - missing\"\n> +printf \"%s\\0\" \\\n> +\t\"$hello_sha1 blob $hello_size\" \\\n> +\t\"$tree_sha1 tree $tree_size\" \\\n> +\t\"$commit_sha1 commit $commit_size\" \\\n> +\t\"$tag_sha1 tag $tag_size\" \\\n> +\t\"deadbeef missing\" \\\n> +\t\" missing\" >batch_check_output\n>   \n>   test_expect_success \"--batch-check with multiple sha1s gives correct format\" '\n> -\techo \"$batch_check_output\" >expect &&\n> +\ttr \"\\0\" \"\\n\" <batch_check_output >expect &&\n>   \techo_without_newline \"$batch_check_input\" >in &&\n>   \tgit cat-file --batch-check <in >actual &&\n>   \ttest_cmp expect actual\n>   '\n>   \n>   test_expect_success \"--batch-check, -z with multiple sha1s gives correct format\" '\n> -\techo \"$batch_check_output\" >expect &&\n> +\ttr \"\\0\" \"\\n\" <batch_check_output >expect &&\n>   \techo_without_newline_nul \"$batch_check_input\" >in &&\n>   \tgit cat-file --batch-check -z <in >actual &&\n>   \ttest_cmp expect actual\n>   '\n>   \n> -test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n> +test_expect_success \"--batch-check, -Z with multiple sha1s gives correct format\" '\n> +\techo_without_newline_nul \"$batch_check_input\" >in &&\n> +\tgit cat-file --batch-check -Z <in >actual &&\n> +\ttest_cmp batch_check_output actual\n> +'\n> +\n> +test_expect_success FUNNYNAMES 'setup with newline in input' '\n>   \ttouch -- \"newline${LF}embedded\" &&\n>   \tgit add -- \"newline${LF}embedded\" &&\n>   \tgit commit -m \"file with newline embedded\" &&\n>   \ttest_tick &&\n>   \n> -\tprintf \"HEAD:newline${LF}embedded\" >in &&\n> -\tgit cat-file --batch-check -z <in >actual &&\n> +\tprintf \"HEAD:newline${LF}embedded\" >in\n> +'\n>   \n> +test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n> +\tgit cat-file --batch-check -z <in >actual &&\n>   \techo \"$(git rev-parse \"HEAD:newline${LF}embedded\") blob 0\" >expect &&\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n> +\tgit cat-file --batch-check -Z <in >actual &&\n> +\tprintf \"%s\\0\" \"$(git rev-parse \"HEAD:newline${LF}embedded\") blob 0\" >expect &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   batch_command_multiple_info=\"info $hello_sha1\n>   info $tree_sha1\n>   info $commit_sha1\n> @@ -480,7 +503,13 @@ test_expect_success '--batch-command with multiple info calls gives correct form\n>   \techo \"$batch_command_multiple_info\" | tr \"\\n\" \"\\0\" >in &&\n>   \tgit cat-file --batch-command --buffer -z <in >actual &&\n>   \n> -\ttest_cmp expect actual\n> +\ttest_cmp expect actual &&\n> +\n> +\techo \"$batch_command_multiple_info\" | tr \"\\n\" \"\\0\" >in &&\n> +\ttr \"\\n\" \"\\0\" <expect >expect_nul &&\n> +\tgit cat-file --batch-command --buffer -Z <in >actual &&\n> +\n> +\ttest_cmp expect_nul actual\n>   '\n>   \n>   batch_command_multiple_contents=\"contents $hello_sha1\n> @@ -490,15 +519,15 @@ contents deadbeef\n>   flush\"\n>   \n>   test_expect_success '--batch-command with multiple command calls gives correct format' '\n> -\tcat >expect <<-EOF &&\n> -\t$hello_sha1 blob $hello_size\n> -\t$hello_content\n> -\t$commit_sha1 commit $commit_size\n> -\t$commit_content\n> -\t$tag_sha1 tag $tag_size\n> -\t$tag_content\n> -\tdeadbeef missing\n> -\tEOF\n> +\tprintf \"%s\\0\" \\\n> +\t\t\"$hello_sha1 blob $hello_size\" \\\n> +\t\t\"$hello_content\" \\\n> +\t\t\"$commit_sha1 commit $commit_size\" \\\n> +\t\t\"$commit_content\" \\\n> +\t\t\"$tag_sha1 tag $tag_size\" \\\n> +\t\t\"$tag_content\" \\\n> +\t\t\"deadbeef missing\" >expect_nul &&\n> +\ttr \"\\0\" \"\\n\" <expect_nul >expect &&\n>   \n>   \techo \"$batch_command_multiple_contents\" >in &&\n>   \tgit cat-file --batch-command --buffer <in >actual &&\n> @@ -508,7 +537,12 @@ test_expect_success '--batch-command with multiple command calls gives correct f\n>   \techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n>   \tgit cat-file --batch-command --buffer -z <in >actual &&\n>   \n> -\ttest_cmp expect actual\n> +\ttest_cmp expect actual &&\n> +\n> +\techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n> +\tgit cat-file --batch-command --buffer -Z <in >actual &&\n> +\n> +\ttest_cmp expect_nul actual\n>   '\n>   \n>   test_expect_success 'setup blobs which are likely to delta' '\n> @@ -848,6 +882,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for brok\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for broken in-repo, same-dir links' '\n> +\tprintf \"HEAD:broken-same-dir-link\\0\" >in &&\n> +\tprintf \"dangling 25\\0HEAD:broken-same-dir-link\\0\" >expect &&\n> +\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   test_expect_success 'git cat-file --batch-check --follow-symlinks works for same-dir links-to-links' '\n>   \techo HEAD:link-to-link | git cat-file --batch-check --follow-symlinks >actual &&\n>   \ttest_cmp found actual\n> @@ -862,6 +903,15 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for pare\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for parent-dir links' '\n> +\techo HEAD:dir/parent-dir-link | git cat-file --batch-check --follow-symlinks >actual &&\n> +\ttest_cmp found actual &&\n> +\tprintf \"notdir 29\\0HEAD:dir/parent-dir-link/nope\\0\" >expect &&\n> +\tprintf \"HEAD:dir/parent-dir-link/nope\\0\" >in &&\n> +\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   test_expect_success 'git cat-file --batch-check --follow-symlinks works for .. links' '\n>   \techo dangling 22 >expect &&\n>   \techo HEAD:dir/link-dir/nope >>expect &&\n> @@ -976,6 +1026,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlink breaks loops' '\n>   \ttest_cmp expect actual\n>   '\n>   \n> +test_expect_success 'git cat-file --batch-check --follow-symlink -Z breaks loops' '\n> +\tprintf \"loop 10\\0HEAD:loop1\\0\" >expect &&\n> +\tprintf \"HEAD:loop1\\0\" >in &&\n> +\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>   test_expect_success 'git cat-file --batch --follow-symlink returns correct sha and mode' '\n>   \techo HEAD:morx | git cat-file --batch >expect &&\n>   \techo HEAD:morx | git cat-file --batch --follow-symlinks >actual &&\n"},{"id":"478054","messageId":"xmqq35355utz.fsf@gitster.g","threadId":"59827","inReplyTo":"9900512f-b0da-2e47-f1ab-ed51ec2c78ff@gmail.com","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-05T23:54:16Z","receivedAt":"2023-06-05T23:54:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood <phillip.wood123@gmail.com> writes:\n\n>> Instead, introduce a new option `-Z` that switches to NUL-delimited\n>> input and output. The old `-z` option is marked as deprecated with a\n>> hint that its output may become unparsable.\n>\n> The commit message explains the problem well, I agree adding a new\n> option is the cleanest solution.\n>\n>...\n>>   @@ -246,6 +246,12 @@ respectively print:\n>>   -z::\n>>   \tOnly meaningful with `--batch`, `--batch-check`, or\n>>   \t`--batch-command`; input is NUL-delimited instead of\n>> +\tnewline-delimited. This option is deprecated in favor of\n>> +\t`-Z` as the output can otherwise be ambiguous.\n>> +\n>> +-Z::\n>> +\tOnly meaningful with `--batch`, `--batch-check`, or\n>> +\t`--batch-command`; input and output is NUL-delimited instead of\n>>   \tnewline-delimited.\n>\n> The documentation changes look good. I wonder if we should put the\n> documentation for \"-Z\" above \"-z\" so users see the preferred option\n> first.\n\nHmph, I expected \"-z\" and \"-Z\" to be orthogonal, the former\ncontrolling how input records are delimited, the latter controlling\nhow output records are delimited, as it usually is a good idea to\nkeep things that could be orthogonal to be orthogonal to avoid\nunnecessarily robbing users flexibility.  \"-Z is a new way that is\npreferred over -z\" was something I did not expect, actually.\n\nI am not outright rejecting such a deliberately limiting design, but\nI'll have to think about it a bit.\n"},{"id":"478058","messageId":"xmqqjzwh4c53.fsf@gitster.g","threadId":"59827","inReplyTo":"07a7c34615ec68fa42c725fd34d6144b6b191f03.1685710884.git.ps@pks.im","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-06T01:23:20Z","receivedAt":"2023-06-06T01:23:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Subject: Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters\n\nIf you are going to make \"-Z\" and \"-z\" not orthogonal, then this\nshould say\n\n    cat-file: introduce -Z option to delimit both input and output with ...\n\nor something.\n"},{"id":"478074","messageId":"ZH67eBAtFxo95aBL@ncase","threadId":"59827","inReplyTo":"xmqq35355utz.fsf@gitster.g","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T04:52:08Z","receivedAt":"2023-06-06T04:52:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 06, 2023 at 08:54:16AM +0900, Junio C Hamano wrote:\n> Phillip Wood <phillip.wood123@gmail.com> writes:\n> \n> >> Instead, introduce a new option `-Z` that switches to NUL-delimited\n> >> input and output. The old `-z` option is marked as deprecated with a\n> >> hint that its output may become unparsable.\n> >\n> > The commit message explains the problem well, I agree adding a new\n> > option is the cleanest solution.\n> >\n> >...\n> >>   @@ -246,6 +246,12 @@ respectively print:\n> >>   -z::\n> >>   \tOnly meaningful with `--batch`, `--batch-check`, or\n> >>   \t`--batch-command`; input is NUL-delimited instead of\n> >> +\tnewline-delimited. This option is deprecated in favor of\n> >> +\t`-Z` as the output can otherwise be ambiguous.\n> >> +\n> >> +-Z::\n> >> +\tOnly meaningful with `--batch`, `--batch-check`, or\n> >> +\t`--batch-command`; input and output is NUL-delimited instead of\n> >>   \tnewline-delimited.\n> >\n> > The documentation changes look good. I wonder if we should put the\n> > documentation for \"-Z\" above \"-z\" so users see the preferred option\n> > first.\n> \n> Hmph, I expected \"-z\" and \"-Z\" to be orthogonal, the former\n> controlling how input records are delimited, the latter controlling\n> how output records are delimited, as it usually is a good idea to\n> keep things that could be orthogonal to be orthogonal to avoid\n> unnecessarily robbing users flexibility.  \"-Z is a new way that is\n> preferred over -z\" was something I did not expect, actually.\n> \n> I am not outright rejecting such a deliberately limiting design, but\n> I'll have to think about it a bit.\n\nWell, the way I see it we shouldn't have ever decoupled the input and\noutput format, and I consider it a bug that `-z` did as it makes it\nunusable with arbitrary input. So I'd rather be helping the user of\nthese modes to do the right thing and NUL-delimit both input and output\nthan running into these edge cases later down the road.\n\nThat being said I'd be fine to change this series to mean \"-Z changes\nstdout\" if you insist. In that case we should be pointing out in our\ndocumentation that \"You should never use `-z` without `-Z` when you\nprocess arbitrary input\".\n\nPatrick\n"},{"id":"478075","messageId":"ZH69UBT_H0OtGR6l@ncase","threadId":"59827","inReplyTo":"9900512f-b0da-2e47-f1ab-ed51ec2c78ff@gmail.com","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:00:00Z","receivedAt":"2023-06-06T05:00:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jun 05, 2023 at 04:47:14PM +0100, Phillip Wood wrote:\n> > @@ -384,6 +390,11 @@ notdir SP <size> LF\n> >   is printed when, during symlink resolution, a file is used as a\n> >   directory name.\n> >   \n> > +Alternatively, when `-Z` is passed, the line feeds in any of the above examples\n> > +are replaced with NUL terminators. This ensures that output will be parsable if\n> > +the output itself would contain a linefeed and is thus recommended for\n> > +scripting purposes.\n> > +\n> >   CAVEATS\n> >   -------\n> >   \n> > diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> > index 001dcb24d6..90ef407d30 100644\n> > --- a/builtin/cat-file.c\n> > +++ b/builtin/cat-file.c\n> > @@ -492,17 +494,18 @@ static void batch_object_write(const char *obj_name,\n> >   \tstrbuf_reset(scratch);\n> >   \n> >   \tif (!opt->format) {\n> > -\t\tprint_default_format(scratch, data);\n> > +\t\tprint_default_format(scratch, data, opt);\n> >   \t} else {\n> >   \t\tstrbuf_expand(scratch, opt->format, expand_format, data);\n> > -\t\tstrbuf_addch(scratch, '\\n');\n> > +\t\tstrbuf_addch(scratch, opt->output_delim);\n> >   \t}\n> >   \n> >   \tbatch_write(opt, scratch->buf, scratch->len);\n> >   \n> >   \tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n> > +\t\tchar buf[] = {opt->output_delim};\n> \n> I found this a bit confusing, I think it would be clearer just to do\n> \n> \tbatch_write(opt, &opt->output_delim, 1);\n\nAgreed, that's cleaner.\n\n> >   \t\tprint_object_or_die(opt, data);\n> > -\t\tbatch_write(opt, \"\\n\", 1);\n> > +\t\tbatch_write(opt, buf, 1);\n> >   \t}\n> >   }\n> \n> > @@ -920,7 +927,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n> >   \t\tN_(\"git cat-file (-t | -s) [--allow-unknown-type] <object>\"),\n> >   \t\tN_(\"git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\\n\"\n> >   \t\t   \"             [--buffer] [--follow-symlinks] [--unordered]\\n\"\n> > -\t\t   \"             [--textconv | --filters] [-z]\"),\n> > +\t\t   \"             [--textconv | --filters] [-z] [-Z]\"),\n> \n> If we're recommending that people don't use '-z' then maybe we should \n> remove it from the synopsis and add OPT_HIDDEN to it below.\n\nI might still change this depending on the conclusion Junio and I will\narrive at, but for now I agree that this makes sense.\n\n> >   \t\tN_(\"git cat-file (--textconv | --filters)\\n\"\n> >   \t\t   \"             [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\"),\n> >   \t\tNULL\n> > @@ -950,6 +957,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n> >   \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n> >   \t\t\tbatch_option_callback),\n> >   \t\tOPT_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n> > +\t\tOPT_BOOL('Z', NULL, &nul_terminated, N_(\"stdin and stdout is NUL-terminated\")),\n> \n> > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> > index 7b985cfded..d73a0be1b9 100755\n> > --- a/t/t1006-cat-file.sh\n> > +++ b/t/t1006-cat-file.sh\n> > @@ -392,17 +393,18 @@ deadbeef\n> >   \n> >   \"\n> >   \n> > -batch_output=\"$hello_sha1 blob $hello_size\n> > -$hello_content\n> > -$commit_sha1 commit $commit_size\n> > -$commit_content\n> > -$tag_sha1 tag $tag_size\n> > -$tag_content\n> > -deadbeef missing\n> > - missing\"\n> > +printf \"%s\\0\" \\\n> > +\t\"$hello_sha1 blob $hello_size\" \\\n> > +\t\"$hello_content\" \\\n> > +\t\"$commit_sha1 commit $commit_size\" \\\n> > +\t\"$commit_content\" \\\n> > +\t\"$tag_sha1 tag $tag_size\" \\\n> > +\t\"$tag_content\" \\\n> > +\t\"deadbeef missing\" \\\n> > +\t\" missing\" >batch_output\n> \n> I think writing the expected output to a file is a good change as we \n> always use it with test_cmp. As \"-z\" is deprecated I think it makes \n> sense to model the expected output for \"-Z\" and use tr for the \"-z\" \n> tests as you have done here. It looks like we have good coverage of the \n> new option.\n\nIt's actually also required in order to not have to specify the expected\noutput twice. While we could leave this as-is, translating it to be NUL\nterminated via `tr \\n \\0` doesn't work as the output contains newlines\nin places where we don't want to translate them to NUL delimiters. And\nstoring the NUL-delimited string in a variable doesn't work either as\nshells will truncate the C strings.\n\nThanks for your review!\n\nPatrick\n"},{"id":"478076","messageId":"cover.1686028409.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1685710884.git.ps@pks.im","subject":"[PATCH v2 0/5] catfile: introduce NUL-terminated output format","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:19:24Z","receivedAt":"2023-06-06T05:19:35Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis is the second version of my patch series that introduces a new NUL\nterminated output format to git-cat-file(1) in order to make its output\nunambiguously parsable in the case where the input contains newlines.\n\nChanges compared to v1:\n\n    - Improved the commit subject of v5 to mention that the new option\n      changes both input and output to be NUL delimited.\n\n    - Extended the commit message to make a better case for why `-Z`\n      changes both input and output format to be NUL terminated, instead\n      of having `-z` change the input and `-Z` change the output format.\n\n    - The `-Z` option is now sorted before `-z` in git-cat-file(1).\n\n    - The `-z` option is now deprecated \"even more\", where it is hidden\n      from the synopsis as well as from the `-h` output.\n\n    - A small change to pass the delimiter to `batch_write()` directly\n      instead of storing it in a temporary array first.\n\nThanks for your feedback, Junio and Phillip!\n\nPatrick\n\nPatrick Steinhardt (5):\n  t1006: don't strip timestamps from expected results\n  t1006: modernize test style to use `test_cmp`\n  strbuf: provide CRLF-aware helper to read until a specified delimiter\n  cat-file: simplify reading from standard input\n  cat-file: introduce option to delimit input and output with NUL\n\n Documentation/git-cat-file.txt |  15 +-\n builtin/cat-file.c             |  85 +++++------\n strbuf.c                       |  11 +-\n strbuf.h                       |  12 ++\n t/t1006-cat-file.sh            | 249 +++++++++++++++++++++------------\n 5 files changed, 233 insertions(+), 139 deletions(-)\n\nRange-diff against v1:\n1:  5c8b4a1d70 = 1:  5c8b4a1d70 t1006: don't strip timestamps from expected results\n2:  251fc2a387 = 2:  251fc2a387 t1006: modernize test style to use `test_cmp`\n3:  8127eeac97 = 3:  8127eeac97 strbuf: provide CRLF-aware helper to read until a specified delimiter\n4:  e7cba8dc4c = 4:  e7cba8dc4c cat-file: simplify reading from standard input\n5:  07a7c34615 ! 5:  79ed618c84 cat-file: Introduce new option to delimit output with NUL characters\n    @@ Metadata\n     Author: Patrick Steinhardt <ps@pks.im>\n     \n      ## Commit message ##\n    -    cat-file: Introduce new option to delimit output with NUL characters\n    +    cat-file: introduce option to delimit input and output with NUL\n     \n         In db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with\n         `-z`, 2022-07-22), we have introduced a new mode to read the input via\n    @@ Commit message\n         given that revisions containing newlines are quite exotic.\n     \n         Instead, introduce a new option `-Z` that switches to NUL-delimited\n    -    input and output. The old `-z` option is marked as deprecated with a\n    -    hint that its output may become unparsable.\n    +    input and output. While this new option could arguably only switch the\n    +    output format to be NUL-delimited, the consequence would be that users\n    +    have to always specify both `-z` and `-Z` when the input may contain\n    +    newlines. On the other hand, if the user knows that there never will be\n    +    newlines in the input, they don't have to use either of those options.\n    +    There is thus no usecase that would warrant treating input and output\n    +    format separately, which is why we instead opt to \"do the right thing\"\n    +    and have `-Z` mean to NUL-terminate both formats.\n    +\n    +    The old `-z` option is marked as deprecated with a hint that its output\n    +    may become unparsable. It is thus hidden both from the synopsis as well\n    +    as the command's help output.\n     \n         Co-authored-by: Toon Claes <toon@iotcl.com>\n         Signed-off-by: Patrick Steinhardt <ps@pks.im>\n    @@ Documentation/git-cat-file.txt: SYNOPSIS\n      'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects]\n      \t     [--buffer] [--follow-symlinks] [--unordered]\n     -\t     [--textconv | --filters] [-z]\n    -+\t     [--textconv | --filters] [-z] [-Z]\n    ++\t     [--textconv | --filters] [-Z]\n      'git cat-file' (--textconv | --filters)\n      \t     [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\n      \n     @@ Documentation/git-cat-file.txt: respectively print:\n    - -z::\n    - \tOnly meaningful with `--batch`, `--batch-check`, or\n    - \t`--batch-command`; input is NUL-delimited instead of\n    -+\tnewline-delimited. This option is deprecated in favor of\n    -+\t`-Z` as the output can otherwise be ambiguous.\n    -+\n    + \t/etc/passwd\n    + --\n    + \n     +-Z::\n     +\tOnly meaningful with `--batch`, `--batch-check`, or\n     +\t`--batch-command`; input and output is NUL-delimited instead of\n    - \tnewline-delimited.\n    ++\tnewline-delimited.\n    ++\n    + -z::\n    + \tOnly meaningful with `--batch`, `--batch-check`, or\n    + \t`--batch-command`; input is NUL-delimited instead of\n    +-\tnewline-delimited.\n    ++\tnewline-delimited. This option is deprecated in favor of\n    ++\t`-Z` as the output can otherwise be ambiguous.\n      \n      \n    + OUTPUT\n     @@ Documentation/git-cat-file.txt: notdir SP <size> LF\n      is printed when, during symlink resolution, a file is used as a\n      directory name.\n    @@ builtin/cat-file.c: static void batch_object_write(const char *obj_name,\n      \tbatch_write(opt, scratch->buf, scratch->len);\n      \n      \tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n    -+\t\tchar buf[] = {opt->output_delim};\n      \t\tprint_object_or_die(opt, data);\n     -\t\tbatch_write(opt, \"\\n\", 1);\n    -+\t\tbatch_write(opt, buf, 1);\n    ++\t\tbatch_write(opt, &opt->output_delim, 1);\n      \t}\n      }\n      \n    @@ builtin/cat-file.c: int cmd_cat_file(int argc, const char **argv, const char *pr\n      \t\tN_(\"git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\\n\"\n      \t\t   \"             [--buffer] [--follow-symlinks] [--unordered]\\n\"\n     -\t\t   \"             [--textconv | --filters] [-z]\"),\n    -+\t\t   \"             [--textconv | --filters] [-z] [-Z]\"),\n    ++\t\t   \"             [--textconv | --filters] [-Z]\"),\n      \t\tN_(\"git cat-file (--textconv | --filters)\\n\"\n      \t\t   \"             [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\"),\n      \t\tNULL\n     @@ builtin/cat-file.c: 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_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n    +-\t\tOPT_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n    ++\t\tOPT_BOOL_F('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\"),\n    ++\t\t\tPARSE_OPT_HIDDEN),\n     +\t\tOPT_BOOL('Z', NULL, &nul_terminated, N_(\"stdin and stdout is NUL-terminated\")),\n      \t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"format\"),\n      \t\t\tN_(\"read commands from stdin\"),\n-- \n2.41.0\n\n"},{"id":"478077","messageId":"5c8b4a1d70b170e3426cdd537edc0c076115be0a.1686028409.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1686028409.git.ps@pks.im","subject":"[PATCH v2 1/5] t1006: don't strip timestamps from expected results","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:19:29Z","receivedAt":"2023-06-06T05:19:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In t1006 we have a bunch of tests that verify the output format of the\ngit-cat-file(1) command. But while part of the output for some tests\nwould include commit timestamps, we don't verify those but instead strip\nthem before comparing expected with actual results. This is done by the\nfunction `maybe_remove_timestamp`, which goes all the way back to the\nancient commit b335d3f121 (Add tests for git cat-file, 2008-04-23).\n\nOur tests had been in a different shape back then. Most importantly we\ndidn't yet have the infrastructure to create objects with deterministic\ntimestamps. Nowadays we do though, and thus there is no reason anymore\nto strip the timestamps.\n\nRefactor the tests to not strip the timestamp anymore.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t1006-cat-file.sh | 68 +++++++++++++++++----------------------------\n 1 file changed, 26 insertions(+), 42 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 8eac74b59c..f139d56fb4 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -109,26 +109,12 @@ strlen () {\n     echo_without_newline \"$1\" | wc -c | sed -e 's/^ *//'\n }\n \n-maybe_remove_timestamp () {\n-\tif test -z \"$2\"; then\n-\t\techo_without_newline \"$1\"\n-\telse\n-\t\techo_without_newline \"$(printf '%s\\n' \"$1\" | remove_timestamp)\"\n-\tfi\n-}\n-\n-remove_timestamp () {\n-\tsed -e 's/ [0-9][0-9]* [-+][0-9][0-9][0-9][0-9]$//'\n-}\n-\n-\n run_tests () {\n     type=$1\n     sha1=$2\n     size=$3\n     content=$4\n     pretty_content=$5\n-    no_ts=$6\n \n     batch_output=\"$sha1 $type $size\n $content\"\n@@ -163,21 +149,21 @@ $content\"\n \n     test -z \"$content\" ||\n     test_expect_success \"Content of $type is correct\" '\n-\tmaybe_remove_timestamp \"$content\" $no_ts >expect &&\n-\tmaybe_remove_timestamp \"$(git cat-file $type $sha1)\" $no_ts >actual &&\n+\techo_without_newline \"$content\" >expect &&\n+\tgit cat-file $type $sha1 >actual &&\n \ttest_cmp expect actual\n     '\n \n     test_expect_success \"Pretty content of $type is correct\" '\n-\tmaybe_remove_timestamp \"$pretty_content\" $no_ts >expect &&\n-\tmaybe_remove_timestamp \"$(git cat-file -p $sha1)\" $no_ts >actual &&\n+\techo_without_newline \"$pretty_content\" >expect &&\n+\tgit cat-file -p $sha1 >actual &&\n \ttest_cmp expect actual\n     '\n \n     test -z \"$content\" ||\n     test_expect_success \"--batch output of $type is correct\" '\n-\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n-\tmaybe_remove_timestamp \"$(echo $sha1 | git cat-file --batch)\" $no_ts >actual &&\n+\techo \"$batch_output\" >expect &&\n+\techo $sha1 | git cat-file --batch >actual &&\n \ttest_cmp expect actual\n     '\n \n@@ -191,9 +177,8 @@ $content\"\n     do\n \ttest -z \"$content\" ||\n \t\ttest_expect_success \"--batch-command $opt output of $type content is correct\" '\n-\t\tmaybe_remove_timestamp \"$batch_output\" $no_ts >expect &&\n-\t\tmaybe_remove_timestamp \"$(test_write_lines \"contents $sha1\" |\n-\t\tgit cat-file --batch-command $opt)\" $no_ts >actual &&\n+\t\techo \"$batch_output\" >expect &&\n+\t\ttest_write_lines \"contents $sha1\" | git cat-file --batch-command $opt >actual &&\n \t\ttest_cmp expect actual\n \t'\n \n@@ -228,10 +213,9 @@ $content\"\n     test_expect_success \"--batch without type ($type)\" '\n \t{\n \t\techo \"$size\" &&\n-\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t\techo \"$content\"\n \t} >expect &&\n-\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual.full &&\n-\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\techo $sha1 | git cat-file --batch=\"%(objectsize)\" >actual &&\n \ttest_cmp expect actual\n     '\n \n@@ -239,10 +223,9 @@ $content\"\n     test_expect_success \"--batch without size ($type)\" '\n \t{\n \t\techo \"$type\" &&\n-\t\tmaybe_remove_timestamp \"$content\" $no_ts\n+\t\techo \"$content\"\n \t} >expect &&\n-\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual.full &&\n-\tmaybe_remove_timestamp \"$(cat actual.full)\" $no_ts >actual &&\n+\techo $sha1 | git cat-file --batch=\"%(objecttype)\" >actual &&\n \ttest_cmp expect actual\n     '\n }\n@@ -284,7 +267,7 @@ test_expect_success '--batch-check without %(rest) considers whole line' '\n \n tree_sha1=$(git write-tree)\n tree_size=$(($(test_oid rawsz) + 13))\n-tree_pretty_content=\"100644 blob $hello_sha1\thello\"\n+tree_pretty_content=\"100644 blob $hello_sha1\thello${LF}\"\n \n run_tests 'tree' $tree_sha1 $tree_size \"\" \"$tree_pretty_content\"\n \n@@ -292,12 +275,12 @@ commit_message=\"Initial commit\"\n commit_sha1=$(echo_without_newline \"$commit_message\" | git commit-tree $tree_sha1)\n commit_size=$(($(test_oid hexsz) + 137))\n commit_content=\"tree $tree_sha1\n-author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 0 +0000\n-committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 0 +0000\n+author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> $GIT_AUTHOR_DATE\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n \n $commit_message\"\n \n-run_tests 'commit' $commit_sha1 $commit_size \"$commit_content\" \"$commit_content\" 1\n+run_tests 'commit' $commit_sha1 $commit_size \"$commit_content\" \"$commit_content\"\n \n tag_header_without_timestamp=\"object $hello_sha1\n type blob\n@@ -311,7 +294,7 @@ $tag_description\"\n tag_sha1=$(echo_without_newline \"$tag_content\" | git hash-object -t tag --stdin -w)\n tag_size=$(strlen \"$tag_content\")\n \n-run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\" 1\n+run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\"\n \n test_expect_success \\\n     \"Reach a blob from a tag pointing to it\" \\\n@@ -400,13 +383,16 @@ deadbeef missing\n  missing\"\n \n test_expect_success '--batch with multiple sha1s gives correct format' '\n-\ttest \"$(maybe_remove_timestamp \"$batch_output\" 1)\" = \"$(maybe_remove_timestamp \"$(echo_without_newline \"$batch_input\" | git cat-file --batch)\" 1)\"\n+\techo \"$batch_output\" >expect &&\n+\techo_without_newline \"$batch_input\" | git cat-file --batch >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success '--batch, -z with multiple sha1s gives correct format' '\n \techo_without_newline_nul \"$batch_input\" >in &&\n-\ttest \"$(maybe_remove_timestamp \"$batch_output\" 1)\" = \\\n-\t\"$(maybe_remove_timestamp \"$(git cat-file --batch -z <in)\" 1)\"\n+\techo \"$batch_output\" >expect &&\n+\tgit cat-file --batch -z <in >actual &&\n+\ttest_cmp expect actual\n '\n \n batch_check_input=\"$hello_sha1\n@@ -480,7 +466,7 @@ contents deadbeef\n flush\"\n \n test_expect_success '--batch-command with multiple command calls gives correct format' '\n-\tremove_timestamp >expect <<-EOF &&\n+\tcat >expect <<-EOF &&\n \t$hello_sha1 blob $hello_size\n \t$hello_content\n \t$commit_sha1 commit $commit_size\n@@ -491,15 +477,13 @@ test_expect_success '--batch-command with multiple command calls gives correct f\n \tEOF\n \n \techo \"$batch_command_multiple_contents\" >in &&\n-\tgit cat-file --batch-command --buffer <in >actual_raw &&\n+\tgit cat-file --batch-command --buffer <in >actual &&\n \n-\tremove_timestamp <actual_raw >actual &&\n \ttest_cmp expect actual &&\n \n \techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n-\tgit cat-file --batch-command --buffer -z <in >actual_raw &&\n+\tgit cat-file --batch-command --buffer -z <in >actual &&\n \n-\tremove_timestamp <actual_raw >actual &&\n \ttest_cmp expect actual\n '\n \n-- \n2.41.0\n\n"},{"id":"478078","messageId":"251fc2a387067bb6d575cfa39f088d4474844a59.1686028409.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1686028409.git.ps@pks.im","subject":"[PATCH v2 2/5] t1006: modernize test style to use `test_cmp`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:19:33Z","receivedAt":"2023-06-06T05:19:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The tests for git-cat-file(1) are quite old and haven't ever been\nupdated since they were introduced. They thus tend to use old idioms\nthat have since grown outdated. Most importantly, many of the tests use\n`test $A = $B` to compare expected and actual output. This has the\ndownside that it is impossible to tell what exactly is different between\nboth versions in case the test fails.\n\nRefactor the tests to instead use `test_cmp`. While more verbose, it\nboth tends to be more readable and will result in a nice diff in case\nstates don't match.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n t/t1006-cat-file.sh | 70 ++++++++++++++++++++++++++++++---------------\n 1 file changed, 47 insertions(+), 23 deletions(-)\n\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex f139d56fb4..7b985cfded 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -296,9 +296,11 @@ tag_size=$(strlen \"$tag_content\")\n \n run_tests 'tag' $tag_sha1 $tag_size \"$tag_content\" \"$tag_content\"\n \n-test_expect_success \\\n-    \"Reach a blob from a tag pointing to it\" \\\n-    \"test '$hello_content' = \\\"\\$(git cat-file blob $tag_sha1)\\\"\"\n+test_expect_success \"Reach a blob from a tag pointing to it\" '\n+\techo_without_newline \"$hello_content\" >expect &&\n+\tgit cat-file blob $tag_sha1 >actual &&\n+\ttest_cmp expect actual\n+'\n \n for batch in batch batch-check batch-command\n do\n@@ -334,30 +336,47 @@ do\n done\n \n test_expect_success \"--batch-check for a non-existent named object\" '\n-    test \"foobar42 missing\n-foobar84 missing\" = \\\n-    \"$( ( echo foobar42 && echo_without_newline foobar84 ) | git cat-file --batch-check)\"\n+\tcat >expect <<-EOF &&\n+\tfoobar42 missing\n+\tfoobar84 missing\n+\tEOF\n+\n+\tprintf \"foobar42\\nfoobar84\" >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check for a non-existent hash\" '\n-    test \"0000000000000000000000000000000000000042 missing\n-0000000000000000000000000000000000000084 missing\" = \\\n-    \"$( ( echo 0000000000000000000000000000000000000042 &&\n-\t echo_without_newline 0000000000000000000000000000000000000084 ) |\n-       git cat-file --batch-check)\"\n+\tcat >expect <<-EOF &&\n+\t0000000000000000000000000000000000000042 missing\n+\t0000000000000000000000000000000000000084 missing\n+\tEOF\n+\n+\tprintf \"0000000000000000000000000000000000000042\\n0000000000000000000000000000000000000084\" >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch for an existent and a non-existent hash\" '\n-    test \"$tag_sha1 tag $tag_size\n-$tag_content\n-0000000000000000000000000000000000000000 missing\" = \\\n-    \"$( ( echo $tag_sha1 &&\n-\t echo_without_newline 0000000000000000000000000000000000000000 ) |\n-       git cat-file --batch)\"\n+\tcat >expect <<-EOF &&\n+\t$tag_sha1 tag $tag_size\n+\t$tag_content\n+\t0000000000000000000000000000000000000000 missing\n+\tEOF\n+\n+\tprintf \"$tag_sha1\\n0000000000000000000000000000000000000000\" >in &&\n+\tgit cat-file --batch <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check for an empty line\" '\n-    test \" missing\" = \"$(echo | git cat-file --batch-check)\"\n+\tcat >expect <<-EOF &&\n+\t missing\n+\tEOF\n+\n+\techo >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'empty --batch-check notices missing object' '\n@@ -384,7 +403,8 @@ deadbeef missing\n \n test_expect_success '--batch with multiple sha1s gives correct format' '\n \techo \"$batch_output\" >expect &&\n-\techo_without_newline \"$batch_input\" | git cat-file --batch >actual &&\n+\techo_without_newline \"$batch_input\" >in &&\n+\tgit cat-file --batch <in >actual &&\n \ttest_cmp expect actual\n '\n \n@@ -411,13 +431,17 @@ deadbeef missing\n  missing\"\n \n test_expect_success \"--batch-check with multiple sha1s gives correct format\" '\n-    test \"$batch_check_output\" = \\\n-    \"$(echo_without_newline \"$batch_check_input\" | git cat-file --batch-check)\"\n+\techo \"$batch_check_output\" >expect &&\n+\techo_without_newline \"$batch_check_input\" >in &&\n+\tgit cat-file --batch-check <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check, -z with multiple sha1s gives correct format\" '\n-    echo_without_newline_nul \"$batch_check_input\" >in &&\n-    test \"$batch_check_output\" = \"$(git cat-file --batch-check -z <in)\"\n+\techo \"$batch_check_output\" >expect &&\n+\techo_without_newline_nul \"$batch_check_input\" >in &&\n+\tgit cat-file --batch-check -z <in >actual &&\n+\ttest_cmp expect actual\n '\n \n test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n-- \n2.41.0\n\n"},{"id":"478079","messageId":"8127eeac97200da9aafccdf16cb7ba06f68b0121.1686028409.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1686028409.git.ps@pks.im","subject":"[PATCH v2 3/5] strbuf: provide CRLF-aware helper to read until a specified delimiter","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:19:37Z","receivedAt":"2023-06-06T05:20:10Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Many of our commands support reading input that is separated either via\nnewlines or via NUL characters. Furthermore, in order to be a better\ncross platform citizen, these commands typically know to strip the CRLF\nsequence so that we also support reading newline-separated inputs on\ne.g. the Windows platform. This results in the following kind of awkward\npattern:\n\n```\nstruct strbuf input = STRBUF_INIT;\n\nwhile (1) {\n\tint ret;\n\n\tif (nul_terminated)\n\t\tret = strbuf_getline_nul(&input, stdin);\n\telse\n\t\tret = strbuf_getline(&input, stdin);\n\tif (ret)\n\t\tbreak;\n\n\t...\n}\n```\n\nIntroduce a new CRLF-aware helper function that can read up to a user\nspecified delimiter. If the delimiter is `\\n` the function knows to also\nstrip CRLF, otherwise it will only strip the specified delimiter. This\nresults in the following, much more readable code pattern:\n\n```\nstruct strbuf input = STRBUF_INIT;\n\nwhile (strbuf_getdelim_strip_crlf(&input, stdin, delim) != EOF) {\n\t...\n}\n```\n\nThe new function will be used in a subsequent commit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n strbuf.c | 11 ++++++++---\n strbuf.h | 12 ++++++++++++\n 2 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 08eec8f1d8..31dc48c0ab 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -721,11 +721,11 @@ static int strbuf_getdelim(struct strbuf *sb, FILE *fp, int term)\n \treturn 0;\n }\n \n-int strbuf_getline(struct strbuf *sb, FILE *fp)\n+int strbuf_getdelim_strip_crlf(struct strbuf *sb, FILE *fp, int term)\n {\n-\tif (strbuf_getwholeline(sb, fp, '\\n'))\n+\tif (strbuf_getwholeline(sb, fp, term))\n \t\treturn EOF;\n-\tif (sb->buf[sb->len - 1] == '\\n') {\n+\tif (term == '\\n' && sb->buf[sb->len - 1] == '\\n') {\n \t\tstrbuf_setlen(sb, sb->len - 1);\n \t\tif (sb->len && sb->buf[sb->len - 1] == '\\r')\n \t\t\tstrbuf_setlen(sb, sb->len - 1);\n@@ -733,6 +733,11 @@ int strbuf_getline(struct strbuf *sb, FILE *fp)\n \treturn 0;\n }\n \n+int strbuf_getline(struct strbuf *sb, FILE *fp)\n+{\n+\treturn strbuf_getdelim_strip_crlf(sb, fp, '\\n');\n+}\n+\n int strbuf_getline_lf(struct strbuf *sb, FILE *fp)\n {\n \treturn strbuf_getdelim(sb, fp, '\\n');\ndiff --git a/strbuf.h b/strbuf.h\nindex 3dfeadb44c..0e69b656bc 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -475,6 +475,18 @@ int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n  */\n ssize_t strbuf_write(struct strbuf *sb, FILE *stream);\n \n+/**\n+ * Read from a FILE * until the specified terminator is encountered,\n+ * overwriting the existing contents of the strbuf.\n+ *\n+ * Reading stops after the terminator or at EOF.  The terminator is\n+ * removed from the buffer before returning.  If the terminator is LF\n+ * and if it is preceded by a CR, then the whole CRLF is stripped.\n+ * Returns 0 unless there was nothing left before EOF, in which case\n+ * it returns `EOF`.\n+ */\n+int strbuf_getdelim_strip_crlf(struct strbuf *sb, FILE *fp, int term);\n+\n /**\n  * Read a line from a FILE *, overwriting the existing contents of\n  * the strbuf.  The strbuf_getline*() family of functions share\n-- \n2.41.0\n\n"},{"id":"478080","messageId":"e7cba8dc4cebde0841ac04702155890d87e74492.1686028409.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1686028409.git.ps@pks.im","subject":"[PATCH v2 4/5] cat-file: simplify reading from standard input","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:19:41Z","receivedAt":"2023-06-06T05:20:15Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The batch modes of git-cat-file(1) read queries from stantard input that\nare either newline- or NUL-delimited. This code was introduced via\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), which notes that:\n\n\"\"\"\nThe refactoring here is slightly unfortunate, since we turn loops like:\n\n     while (strbuf_getline(&buf, stdin) != EOF)\n\n into:\n\n     while (1) {\n         int ret;\n         if (opt->nul_terminated)\n             ret = strbuf_getline_nul(&input, stdin);\n         else\n             ret = strbuf_getline(&input, stdin);\n\n         if (ret == EOF)\n             break;\n     }\n\"\"\"\n\nThe commit proposed introducing a helper function that is easier to use,\nwhich is just what we have done in the preceding commit. Refactor the\ncode to use this new helper to simplify the loop.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/cat-file.c | 32 +++++++++-----------------------\n 1 file changed, 9 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0bafc14e6c..001dcb24d6 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -42,7 +42,7 @@ struct batch_options {\n \tint all_objects;\n \tint unordered;\n \tint transform_mode; /* may be 'w' or 'c' for --filters or --textconv */\n-\tint nul_terminated;\n+\tchar input_delim;\n \tconst char *format;\n };\n \n@@ -694,20 +694,12 @@ static void batch_objects_command(struct batch_options *opt,\n \tstruct queued_cmd *queued_cmd = NULL;\n \tsize_t alloc = 0, nr = 0;\n \n-\twhile (1) {\n-\t\tint i, ret;\n+\twhile (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != EOF) {\n+\t\tint i;\n \t\tconst struct parse_cmd *cmd = NULL;\n \t\tconst char *p = NULL, *cmd_end;\n \t\tstruct queued_cmd call = {0};\n \n-\t\tif (opt->nul_terminated)\n-\t\t\tret = strbuf_getline_nul(&input, stdin);\n-\t\telse\n-\t\t\tret = strbuf_getline(&input, stdin);\n-\n-\t\tif (ret)\n-\t\t\tbreak;\n-\n \t\tif (!input.len)\n \t\t\tdie(_(\"empty command in input\"));\n \t\tif (isspace(*input.buf))\n@@ -851,16 +843,7 @@ static int batch_objects(struct batch_options *opt)\n \t\tgoto cleanup;\n \t}\n \n-\twhile (1) {\n-\t\tint ret;\n-\t\tif (opt->nul_terminated)\n-\t\t\tret = strbuf_getline_nul(&input, stdin);\n-\t\telse\n-\t\t\tret = strbuf_getline(&input, stdin);\n-\n-\t\tif (ret == EOF)\n-\t\t\tbreak;\n-\n+\twhile (strbuf_getdelim_strip_crlf(&input, stdin, opt->input_delim) != 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@@ -929,6 +912,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tconst char *exp_type = NULL, *obj_name = NULL;\n \tstruct batch_options batch = {0};\n \tint unknown_type = 0;\n+\tint input_nul_terminated = 0;\n \n \tconst char * const usage[] = {\n \t\tN_(\"git cat-file <type> <object>\"),\n@@ -965,7 +949,7 @@ 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_BOOL('z', NULL, &batch.nul_terminated, N_(\"stdin is NUL-terminated\")),\n+\t\tOPT_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n \t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"format\"),\n \t\t\tN_(\"read commands from stdin\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -1024,10 +1008,12 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \telse if (batch.all_objects)\n \t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n \t\t\t       \"--batch-all-objects\");\n-\telse if (batch.nul_terminated)\n+\telse if (input_nul_terminated)\n \t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n \t\t\t       \"-z\");\n \n+\tbatch.input_delim = input_nul_terminated ? '\\0' : '\\n';\n+\n \t/* Batch defaults */\n \tif (batch.buffer_output < 0)\n \t\tbatch.buffer_output = batch.all_objects;\n-- \n2.41.0\n\n"},{"id":"478081","messageId":"79ed618c845fae3f06b6ff4b3ade58e1fcccb758.1686028409.git.ps@pks.im","threadId":"59827","inReplyTo":"cover.1686028409.git.ps@pks.im","subject":"[PATCH v2 5/5] cat-file: introduce option to delimit input and output with NUL","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:19:45Z","receivedAt":"2023-06-06T05:20:17Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with\n`-z`, 2022-07-22), we have introduced a new mode to read the input via\nNUL-delimited records instead of newline-delimited records. This allows\nthe user to query for revisions that have newlines in their path\ncomponent. While unusual, such queries are perfectly valid and thus it\nis clear that we should be able to support them properly.\n\nUnfortunately, the commit only changed the input to be NUL-delimited,\nbut didn't change the output at the same time. While this is fine for\nqueries that are processed successfully, it is less so for queries that\naren't. In the case of missing commits for example the result can become\nentirely unparsable:\n\n```\n$ printf \"7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\\n1234567890\\n\\n\\commit000\" |\n    git cat-file --batch -z\n7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\n1234567890\n\ncommit missing\n```\n\nThis is of course a crafted query that is intentionally gaming the\ndeficiency, but more benign queries that contain newlines would have\nsimilar problems.\n\nIdeally, we should have also changed the output to be NUL-delimited when\n`-z` is specified to avoid this problem. As the input is NUL-delimited,\nit is clear that the output in this case cannot ever contain NUL\ncharacters by itself. Furthermore, Git does not allow NUL characters in\nrevisions anyway, further stressing the point that using NUL-delimited\noutput is safe. The only exception is of course the object data itself,\nbut as git-cat-file(1) prints the size of the object data clients should\nread until that specified size has been consumed.\n\nBut even though `-z` has only been introduced a few releases ago in Git\nv2.38.0, changing the output format retroactively to also NUL-delimit\noutput would be a backwards incompatible change. And while one could\nmake the argument that the output is inherently broken already, we need\nto assume that there are existing users out there that use it just fine\ngiven that revisions containing newlines are quite exotic.\n\nInstead, introduce a new option `-Z` that switches to NUL-delimited\ninput and output. While this new option could arguably only switch the\noutput format to be NUL-delimited, the consequence would be that users\nhave to always specify both `-z` and `-Z` when the input may contain\nnewlines. On the other hand, if the user knows that there never will be\nnewlines in the input, they don't have to use either of those options.\nThere is thus no usecase that would warrant treating input and output\nformat separately, which is why we instead opt to \"do the right thing\"\nand have `-Z` mean to NUL-terminate both formats.\n\nThe old `-z` option is marked as deprecated with a hint that its output\nmay become unparsable. It is thus hidden both from the synopsis as well\nas the command's help output.\n\nCo-authored-by: Toon Claes <toon@iotcl.com>\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n Documentation/git-cat-file.txt |  15 +++-\n builtin/cat-file.c             |  57 +++++++++------\n t/t1006-cat-file.sh            | 123 ++++++++++++++++++++++++---------\n 3 files changed, 139 insertions(+), 56 deletions(-)\n\ndiff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt\nindex 411de2e27d..0e4936d182 100644\n--- a/Documentation/git-cat-file.txt\n+++ b/Documentation/git-cat-file.txt\n@@ -14,7 +14,7 @@ SYNOPSIS\n 'git cat-file' (-t | -s) [--allow-unknown-type] <object>\n 'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects]\n \t     [--buffer] [--follow-symlinks] [--unordered]\n-\t     [--textconv | --filters] [-z]\n+\t     [--textconv | --filters] [-Z]\n 'git cat-file' (--textconv | --filters)\n \t     [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\n \n@@ -243,10 +243,16 @@ respectively print:\n \t/etc/passwd\n --\n \n+-Z::\n+\tOnly meaningful with `--batch`, `--batch-check`, or\n+\t`--batch-command`; input and output is NUL-delimited instead of\n+\tnewline-delimited.\n+\n -z::\n \tOnly meaningful with `--batch`, `--batch-check`, or\n \t`--batch-command`; input is NUL-delimited instead of\n-\tnewline-delimited.\n+\tnewline-delimited. This option is deprecated in favor of\n+\t`-Z` as the output can otherwise be ambiguous.\n \n \n OUTPUT\n@@ -384,6 +390,11 @@ notdir SP <size> LF\n is printed when, during symlink resolution, a file is used as a\n directory name.\n \n+Alternatively, when `-Z` is passed, the line feeds in any of the above examples\n+are replaced with NUL terminators. This ensures that output will be parsable if\n+the output itself would contain a linefeed and is thus recommended for\n+scripting purposes.\n+\n CAVEATS\n -------\n \ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 001dcb24d6..e5f72229df 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -43,6 +43,7 @@ struct batch_options {\n \tint unordered;\n \tint transform_mode; /* may be 'w' or 'c' for --filters or --textconv */\n \tchar input_delim;\n+\tchar output_delim;\n \tconst char *format;\n };\n \n@@ -437,11 +438,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d\n \t}\n }\n \n-static void print_default_format(struct strbuf *scratch, struct expand_data *data)\n+static void print_default_format(struct strbuf *scratch, struct expand_data *data,\n+\t\t\t\t struct batch_options *opt)\n {\n-\tstrbuf_addf(scratch, \"%s %s %\"PRIuMAX\"\\n\", oid_to_hex(&data->oid),\n+\tstrbuf_addf(scratch, \"%s %s %\"PRIuMAX\"%c\", oid_to_hex(&data->oid),\n \t\t    type_name(data->type),\n-\t\t    (uintmax_t)data->size);\n+\t\t    (uintmax_t)data->size, opt->output_delim);\n }\n \n /*\n@@ -470,8 +472,8 @@ static void batch_object_write(const char *obj_name,\n \t\t\t\t\t\t       &data->oid, &data->info,\n \t\t\t\t\t\t       OBJECT_INFO_LOOKUP_REPLACE);\n \t\tif (ret < 0) {\n-\t\t\tprintf(\"%s missing\\n\",\n-\t\t\t       obj_name ? obj_name : oid_to_hex(&data->oid));\n+\t\t\tprintf(\"%s missing%c\",\n+\t\t\t       obj_name ? obj_name : oid_to_hex(&data->oid), opt->output_delim);\n \t\t\tfflush(stdout);\n \t\t\treturn;\n \t\t}\n@@ -492,17 +494,17 @@ static void batch_object_write(const char *obj_name,\n \tstrbuf_reset(scratch);\n \n \tif (!opt->format) {\n-\t\tprint_default_format(scratch, data);\n+\t\tprint_default_format(scratch, data, opt);\n \t} else {\n \t\tstrbuf_expand(scratch, opt->format, expand_format, data);\n-\t\tstrbuf_addch(scratch, '\\n');\n+\t\tstrbuf_addch(scratch, opt->output_delim);\n \t}\n \n \tbatch_write(opt, scratch->buf, scratch->len);\n \n \tif (opt->batch_mode == BATCH_MODE_CONTENTS) {\n \t\tprint_object_or_die(opt, data);\n-\t\tbatch_write(opt, \"\\n\", 1);\n+\t\tbatch_write(opt, &opt->output_delim, 1);\n \t}\n }\n \n@@ -520,22 +522,25 @@ static void batch_one_object(const char *obj_name,\n \tif (result != FOUND) {\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n-\t\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\t\tprintf(\"%s missing%c\", obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase SHORT_NAME_AMBIGUOUS:\n-\t\t\tprintf(\"%s ambiguous\\n\", obj_name);\n+\t\t\tprintf(\"%s ambiguous%c\", obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase DANGLING_SYMLINK:\n-\t\t\tprintf(\"dangling %\"PRIuMAX\"\\n%s\\n\",\n-\t\t\t       (uintmax_t)strlen(obj_name), obj_name);\n+\t\t\tprintf(\"dangling %\"PRIuMAX\"%c%s%c\",\n+\t\t\t       (uintmax_t)strlen(obj_name),\n+\t\t\t       opt->output_delim, obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase SYMLINK_LOOP:\n-\t\t\tprintf(\"loop %\"PRIuMAX\"\\n%s\\n\",\n-\t\t\t       (uintmax_t)strlen(obj_name), obj_name);\n+\t\t\tprintf(\"loop %\"PRIuMAX\"%c%s%c\",\n+\t\t\t       (uintmax_t)strlen(obj_name),\n+\t\t\t       opt->output_delim, obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tcase NOT_DIR:\n-\t\t\tprintf(\"notdir %\"PRIuMAX\"\\n%s\\n\",\n-\t\t\t       (uintmax_t)strlen(obj_name), obj_name);\n+\t\t\tprintf(\"notdir %\"PRIuMAX\"%c%s%c\",\n+\t\t\t       (uintmax_t)strlen(obj_name),\n+\t\t\t       opt->output_delim, obj_name, opt->output_delim);\n \t\t\tbreak;\n \t\tdefault:\n \t\t\tBUG(\"unknown get_sha1_with_context result %d\\n\",\n@@ -547,9 +552,9 @@ static void batch_one_object(const char *obj_name,\n \t}\n \n \tif (ctx.mode == 0) {\n-\t\tprintf(\"symlink %\"PRIuMAX\"\\n%s\\n\",\n+\t\tprintf(\"symlink %\"PRIuMAX\"%c%s%c\",\n \t\t       (uintmax_t)ctx.symlink_path.len,\n-\t\t       ctx.symlink_path.buf);\n+\t\t       opt->output_delim, ctx.symlink_path.buf, opt->output_delim);\n \t\tfflush(stdout);\n \t\treturn;\n \t}\n@@ -913,6 +918,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \tstruct batch_options batch = {0};\n \tint unknown_type = 0;\n \tint input_nul_terminated = 0;\n+\tint nul_terminated = 0;\n \n \tconst char * const usage[] = {\n \t\tN_(\"git cat-file <type> <object>\"),\n@@ -920,7 +926,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \t\tN_(\"git cat-file (-t | -s) [--allow-unknown-type] <object>\"),\n \t\tN_(\"git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\\n\"\n \t\t   \"             [--buffer] [--follow-symlinks] [--unordered]\\n\"\n-\t\t   \"             [--textconv | --filters] [-z]\"),\n+\t\t   \"             [--textconv | --filters] [-Z]\"),\n \t\tN_(\"git cat-file (--textconv | --filters)\\n\"\n \t\t   \"             [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]\"),\n \t\tNULL\n@@ -949,7 +955,9 @@ 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_BOOL('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\")),\n+\t\tOPT_BOOL_F('z', NULL, &input_nul_terminated, N_(\"stdin is NUL-terminated\"),\n+\t\t\tPARSE_OPT_HIDDEN),\n+\t\tOPT_BOOL('Z', NULL, &nul_terminated, N_(\"stdin and stdout is NUL-terminated\")),\n \t\tOPT_CALLBACK_F(0, \"batch-command\", &batch, N_(\"format\"),\n \t\t\tN_(\"read commands from stdin\"),\n \t\t\tPARSE_OPT_OPTARG | PARSE_OPT_NONEG,\n@@ -1011,8 +1019,15 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix)\n \telse if (input_nul_terminated)\n \t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n \t\t\t       \"-z\");\n+\telse if (nul_terminated)\n+\t\tusage_msg_optf(_(\"'%s' requires a batch mode\"), usage, options,\n+\t\t\t       \"-Z\");\n \n-\tbatch.input_delim = input_nul_terminated ? '\\0' : '\\n';\n+\tbatch.input_delim = batch.output_delim = '\\n';\n+\tif (input_nul_terminated)\n+\t\tbatch.input_delim = '\\0';\n+\tif (nul_terminated)\n+\t\tbatch.input_delim = batch.output_delim = '\\0';\n \n \t/* Batch defaults */\n \tif (batch.buffer_output < 0)\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 7b985cfded..d73a0be1b9 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -89,7 +89,8 @@ done\n for opt in --buffer \\\n \t--follow-symlinks \\\n \t--batch-all-objects \\\n-\t-z\n+\t-z \\\n+\t-Z\n do\n \ttest_expect_success \"usage: bad option combination: $opt without batch mode\" '\n \t\ttest_incompatible_usage git cat-file $opt &&\n@@ -392,17 +393,18 @@ deadbeef\n \n \"\n \n-batch_output=\"$hello_sha1 blob $hello_size\n-$hello_content\n-$commit_sha1 commit $commit_size\n-$commit_content\n-$tag_sha1 tag $tag_size\n-$tag_content\n-deadbeef missing\n- missing\"\n+printf \"%s\\0\" \\\n+\t\"$hello_sha1 blob $hello_size\" \\\n+\t\"$hello_content\" \\\n+\t\"$commit_sha1 commit $commit_size\" \\\n+\t\"$commit_content\" \\\n+\t\"$tag_sha1 tag $tag_size\" \\\n+\t\"$tag_content\" \\\n+\t\"deadbeef missing\" \\\n+\t\" missing\" >batch_output\n \n test_expect_success '--batch with multiple sha1s gives correct format' '\n-\techo \"$batch_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_output >expect &&\n \techo_without_newline \"$batch_input\" >in &&\n \tgit cat-file --batch <in >actual &&\n \ttest_cmp expect actual\n@@ -410,11 +412,17 @@ test_expect_success '--batch with multiple sha1s gives correct format' '\n \n test_expect_success '--batch, -z with multiple sha1s gives correct format' '\n \techo_without_newline_nul \"$batch_input\" >in &&\n-\techo \"$batch_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_output >expect &&\n \tgit cat-file --batch -z <in >actual &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--batch, -Z with multiple sha1s gives correct format' '\n+\techo_without_newline_nul \"$batch_input\" >in &&\n+\tgit cat-file --batch -Z <in >actual &&\n+\ttest_cmp batch_output actual\n+'\n+\n batch_check_input=\"$hello_sha1\n $tree_sha1\n $commit_sha1\n@@ -423,40 +431,55 @@ deadbeef\n \n \"\n \n-batch_check_output=\"$hello_sha1 blob $hello_size\n-$tree_sha1 tree $tree_size\n-$commit_sha1 commit $commit_size\n-$tag_sha1 tag $tag_size\n-deadbeef missing\n- missing\"\n+printf \"%s\\0\" \\\n+\t\"$hello_sha1 blob $hello_size\" \\\n+\t\"$tree_sha1 tree $tree_size\" \\\n+\t\"$commit_sha1 commit $commit_size\" \\\n+\t\"$tag_sha1 tag $tag_size\" \\\n+\t\"deadbeef missing\" \\\n+\t\" missing\" >batch_check_output\n \n test_expect_success \"--batch-check with multiple sha1s gives correct format\" '\n-\techo \"$batch_check_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_check_output >expect &&\n \techo_without_newline \"$batch_check_input\" >in &&\n \tgit cat-file --batch-check <in >actual &&\n \ttest_cmp expect actual\n '\n \n test_expect_success \"--batch-check, -z with multiple sha1s gives correct format\" '\n-\techo \"$batch_check_output\" >expect &&\n+\ttr \"\\0\" \"\\n\" <batch_check_output >expect &&\n \techo_without_newline_nul \"$batch_check_input\" >in &&\n \tgit cat-file --batch-check -z <in >actual &&\n \ttest_cmp expect actual\n '\n \n-test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n+test_expect_success \"--batch-check, -Z with multiple sha1s gives correct format\" '\n+\techo_without_newline_nul \"$batch_check_input\" >in &&\n+\tgit cat-file --batch-check -Z <in >actual &&\n+\ttest_cmp batch_check_output actual\n+'\n+\n+test_expect_success FUNNYNAMES 'setup with newline in input' '\n \ttouch -- \"newline${LF}embedded\" &&\n \tgit add -- \"newline${LF}embedded\" &&\n \tgit commit -m \"file with newline embedded\" &&\n \ttest_tick &&\n \n-\tprintf \"HEAD:newline${LF}embedded\" >in &&\n-\tgit cat-file --batch-check -z <in >actual &&\n+\tprintf \"HEAD:newline${LF}embedded\" >in\n+'\n \n+test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n+\tgit cat-file --batch-check -z <in >actual &&\n \techo \"$(git rev-parse \"HEAD:newline${LF}embedded\") blob 0\" >expect &&\n \ttest_cmp expect actual\n '\n \n+test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' '\n+\tgit cat-file --batch-check -Z <in >actual &&\n+\tprintf \"%s\\0\" \"$(git rev-parse \"HEAD:newline${LF}embedded\") blob 0\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n batch_command_multiple_info=\"info $hello_sha1\n info $tree_sha1\n info $commit_sha1\n@@ -480,7 +503,13 @@ test_expect_success '--batch-command with multiple info calls gives correct form\n \techo \"$batch_command_multiple_info\" | tr \"\\n\" \"\\0\" >in &&\n \tgit cat-file --batch-command --buffer -z <in >actual &&\n \n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\techo \"$batch_command_multiple_info\" | tr \"\\n\" \"\\0\" >in &&\n+\ttr \"\\n\" \"\\0\" <expect >expect_nul &&\n+\tgit cat-file --batch-command --buffer -Z <in >actual &&\n+\n+\ttest_cmp expect_nul actual\n '\n \n batch_command_multiple_contents=\"contents $hello_sha1\n@@ -490,15 +519,15 @@ contents deadbeef\n flush\"\n \n test_expect_success '--batch-command with multiple command calls gives correct format' '\n-\tcat >expect <<-EOF &&\n-\t$hello_sha1 blob $hello_size\n-\t$hello_content\n-\t$commit_sha1 commit $commit_size\n-\t$commit_content\n-\t$tag_sha1 tag $tag_size\n-\t$tag_content\n-\tdeadbeef missing\n-\tEOF\n+\tprintf \"%s\\0\" \\\n+\t\t\"$hello_sha1 blob $hello_size\" \\\n+\t\t\"$hello_content\" \\\n+\t\t\"$commit_sha1 commit $commit_size\" \\\n+\t\t\"$commit_content\" \\\n+\t\t\"$tag_sha1 tag $tag_size\" \\\n+\t\t\"$tag_content\" \\\n+\t\t\"deadbeef missing\" >expect_nul &&\n+\ttr \"\\0\" \"\\n\" <expect_nul >expect &&\n \n \techo \"$batch_command_multiple_contents\" >in &&\n \tgit cat-file --batch-command --buffer <in >actual &&\n@@ -508,7 +537,12 @@ test_expect_success '--batch-command with multiple command calls gives correct f\n \techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n \tgit cat-file --batch-command --buffer -z <in >actual &&\n \n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\techo \"$batch_command_multiple_contents\" | tr \"\\n\" \"\\0\" >in &&\n+\tgit cat-file --batch-command --buffer -Z <in >actual &&\n+\n+\ttest_cmp expect_nul actual\n '\n \n test_expect_success 'setup blobs which are likely to delta' '\n@@ -848,6 +882,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for brok\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for broken in-repo, same-dir links' '\n+\tprintf \"HEAD:broken-same-dir-link\\0\" >in &&\n+\tprintf \"dangling 25\\0HEAD:broken-same-dir-link\\0\" >expect &&\n+\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git cat-file --batch-check --follow-symlinks works for same-dir links-to-links' '\n \techo HEAD:link-to-link | git cat-file --batch-check --follow-symlinks >actual &&\n \ttest_cmp found actual\n@@ -862,6 +903,15 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for pare\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for parent-dir links' '\n+\techo HEAD:dir/parent-dir-link | git cat-file --batch-check --follow-symlinks >actual &&\n+\ttest_cmp found actual &&\n+\tprintf \"notdir 29\\0HEAD:dir/parent-dir-link/nope\\0\" >expect &&\n+\tprintf \"HEAD:dir/parent-dir-link/nope\\0\" >in &&\n+\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git cat-file --batch-check --follow-symlinks works for .. links' '\n \techo dangling 22 >expect &&\n \techo HEAD:dir/link-dir/nope >>expect &&\n@@ -976,6 +1026,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlink breaks loops' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'git cat-file --batch-check --follow-symlink -Z breaks loops' '\n+\tprintf \"loop 10\\0HEAD:loop1\\0\" >expect &&\n+\tprintf \"HEAD:loop1\\0\" >in &&\n+\tgit cat-file --batch-check --follow-symlinks -Z <in >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'git cat-file --batch --follow-symlink returns correct sha and mode' '\n \techo HEAD:morx | git cat-file --batch >expect &&\n \techo HEAD:morx | git cat-file --batch --follow-symlinks >actual &&\n-- \n2.41.0\n\n"},{"id":"478082","messageId":"xmqqsfb5np14.fsf@gitster.g","threadId":"59827","inReplyTo":"ZH67eBAtFxo95aBL@ncase","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-06T05:22:15Z","receivedAt":"2023-06-06T05:22:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> Hmph, I expected \"-z\" and \"-Z\" to be orthogonal, the former\n>> controlling how input records are delimited, the latter controlling\n>> how output records are delimited, as it usually is a good idea to\n>> keep things that could be orthogonal to be orthogonal to avoid\n>> unnecessarily robbing users flexibility.  \"-Z is a new way that is\n>> preferred over -z\" was something I did not expect, actually.\n>> \n>> I am not outright rejecting such a deliberately limiting design, but\n>> I'll have to think about it a bit.\n>\n> Well, the way I see it we shouldn't have ever decoupled the input and\n> output format, and I consider it a bug that `-z` did as it makes it\n> unusable with arbitrary input. So I'd rather be helping the user of\n> these modes to do the right thing and NUL-delimit both input and output\n> than running into these edge cases later down the road.\n\nIf that is the direction we want to go in, then the title of this\nstep must be updated to say \"-Z\" does both output and input.  Also\nwith the same number of bytes it takes to say \"new option\", you can\nsay what that new option is, so do that.\n\n> That being said I'd be fine to change this series to mean \"-Z changes\n> stdout\" if you insist. In that case we should be pointing out in our\n> documentation that \"You should never use `-z` without `-Z` when you\n> process arbitrary input\".\n\nYou are not making sense.  If we were to leave them orthogonal to\nkeep flexibility, it is because there can be cases where using '-Z'\nwithout using '-z' (and vice versa) makes sense; \"you should never\"\nhas no place to live in such a world.\n"},{"id":"478083","messageId":"ZH7Eoj_5oTzmSd4G@ncase","threadId":"59827","inReplyTo":"xmqqsfb5np14.fsf@gitster.g","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2023-06-06T05:31:14Z","receivedAt":"2023-06-06T05:31:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jun 06, 2023 at 02:22:15PM +0900, Junio C Hamano wrote:\n> Patrick Steinhardt <ps@pks.im> writes:\n> \n> >> Hmph, I expected \"-z\" and \"-Z\" to be orthogonal, the former\n> >> controlling how input records are delimited, the latter controlling\n> >> how output records are delimited, as it usually is a good idea to\n> >> keep things that could be orthogonal to be orthogonal to avoid\n> >> unnecessarily robbing users flexibility.  \"-Z is a new way that is\n> >> preferred over -z\" was something I did not expect, actually.\n> >> \n> >> I am not outright rejecting such a deliberately limiting design, but\n> >> I'll have to think about it a bit.\n> >\n> > Well, the way I see it we shouldn't have ever decoupled the input and\n> > output format, and I consider it a bug that `-z` did as it makes it\n> > unusable with arbitrary input. So I'd rather be helping the user of\n> > these modes to do the right thing and NUL-delimit both input and output\n> > than running into these edge cases later down the road.\n> \n> If that is the direction we want to go in, then the title of this\n> step must be updated to say \"-Z\" does both output and input.  Also\n> with the same number of bytes it takes to say \"new option\", you can\n> say what that new option is, so do that.\n\nSorry, my v2 and your mail crossed, so the new title doesn't yet mention\n`-Z`. I agree it would make sense to do so, and if there's a v3 I'll\nchange it accordingly. Otherwise, please feel free to change:\n\n    cat-file: introduce option to delimit input and output with NUL\n\nto\n\n    cat-file: introduce `-Z` to delimit input and output with NUL\n\n> > That being said I'd be fine to change this series to mean \"-Z changes\n> > stdout\" if you insist. In that case we should be pointing out in our\n> > documentation that \"You should never use `-z` without `-Z` when you\n> > process arbitrary input\".\n> \n> You are not making sense.  If we were to leave them orthogonal to\n> keep flexibility, it is because there can be cases where using '-Z'\n> without using '-z' (and vice versa) makes sense; \"you should never\"\n> has no place to live in such a world.\n\nWell, that's exactly what I'm arguing: I don't think it does make sense\nto keep them orthogonal. I cannot think of usecases where you'd want to\nonly change either of those options once you start processing arbitrary\ninput. If you exactly know that your input doesn't contain newlines then\nyou don't need either `-z` nor `-Z`. On the other hand, if it does, then\nyou must be using both.\n\nI only proposed the alternative with the intent of being diplomatic and\nnot fully close the door on your remark in case you felt strongly about\nit.\n\nI've thus sent v2 to double down on my current approach where `-Z`\nchanges both stdin and stdout and tried to make a better case for it in\nthe commit message.\n\nPatrick\n"},{"id":"478293","messageId":"xmqqr0qga41b.fsf@gitster.g","threadId":"59827","inReplyTo":"ZH7Eoj_5oTzmSd4G@ncase","subject":"Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T19:12:16Z","receivedAt":"2023-06-12T19:12:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n>> > That being said I'd be fine to change this series to mean \"-Z changes\n>> > stdout\" if you insist. In that case we should be pointing out in our\n>> > documentation that \"You should never use `-z` without `-Z` when you\n>> > process arbitrary input\".\n>> \n>> You are not making sense.  If we were to leave them orthogonal to\n>> keep flexibility, it is because there can be cases where using '-Z'\n>> without using '-z' (and vice versa) makes sense; \"you should never\"\n>> has no place to live in such a world.\n>\n> Well, that's exactly what I'm arguing: I don't think it does make sense\n> to keep them orthogonal.\n\nI was not commenting on that.\n\nMy \"you are not making sense\" was because you said \"I'd be fine to\nmake -Z to mean 'stdout is NUL delimited'\".  If somebody thinks it\nis a good idea to make '-Z' mean 'stdout is NUL delimited without\naffecting how the input is delimited', it can only be because that\nsomebody thinks that it makes sense to make input and output\northogonal.\n\nIt is perfectly OK if you are not fine to make -Z to affect only the\nstandard output stream.  That stance is at least self consistent.\n\n"},{"id":"478299","messageId":"xmqqilbs9ztk.fsf@gitster.g","threadId":"59827","inReplyTo":"cover.1686028409.git.ps@pks.im","subject":"Re: [PATCH v2 0/5] catfile: introduce NUL-terminated output format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-06-12T20:43:19Z","receivedAt":"2023-06-12T20:44:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> this is the second version of my patch series that introduces a new NUL\n> terminated output format to git-cat-file(1) in order to make its output\n> unambiguously parsable in the case where the input contains newlines.\n\nWill queue.  Thanks.\n"}]}