{"thread":{"id":"58923","subject":"[PATCH 0/1] cat-file: quote-format name in error when using -z","startedAt":"2022-12-09T15:08:31Z","lastAt":"2023-06-02T13:29:15Z","messageCount":32,"participants":["Toon Claes","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"468830","messageId":"20221209150048.2400648-1-toon@iotcl.com","threadId":"58923","inReplyTo":null,"subject":"[PATCH 0/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2022-12-09T15:00:47Z","receivedAt":"2022-12-09T15:08:31Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Hi,\n\nWhile we at GitLab have been implementing the use of option `-z` for\n`git-cat-file` with `--batch` we discovered an extreme edge-case when passing\npaths with newlines to the command. In case the object is not found the error\nmessage include the path in the output and thus the error message might get\nspread over multiple lines.\n\nThe provided patch sanitizes the output, making is easier machine-parse it.\n\nToon\n\nToon Claes (1):\n cat-file: quote-format name in error when using -z\n\n builtin/cat-file.c | 10 ++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 18 insertions(+)\n\n\nbase-commit: 2e71cbbddd64695d43383c25c7a054ac4ff86882\n--\n2.39.0.rc0.57\n"},{"id":"468831","messageId":"20221209150048.2400648-2-toon@iotcl.com","threadId":"58923","inReplyTo":"20221209150048.2400648-1-toon@iotcl.com","subject":"[PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2022-12-09T15:00:48Z","receivedAt":"2022-12-09T15:10:35Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Since it's supported to have NUL-delimited input, introduced in\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), it's possible to pass paths that contain newlines. This\nworks great when the object is found, but when it's not, the input path\nis returned in the error message. Because this can contain newlines, the\nerror message might get spread over multiple lines, making it harder to\nmachine-parse this error message.\n\nWith this change, the input is quote-formatted in the error message, if\nneeded. This ensures the error message is always on a single line and\nmakes parsing the error more straightforward.\n\nSigned-off-by: Toon Claes \n---\n builtin/cat-file.c | 10 ++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b3be58b1fb..67dd81238d 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -14,6 +14,7 @@\n #include \"tree-walk.h\"\n #include \"oid-array.h\"\n #include \"packfile.h\"\n+#include \"quote.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n@@ -475,6 +476,13 @@ static void batch_one_object(const char *obj_name,\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t flags, &data->oid, &ctx);\n \tif (result != FOUND) {\n+\t\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\tif (opt->nul_terminated) {\n+\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\tobj_name = quoted.buf;\n+\t\t}\n+\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n \t\t\tprintf(\"%s missing\\n\", obj_name);\n@@ -499,6 +507,8 @@ static void batch_one_object(const char *obj_name,\n \t\t\t result);\n \t\t\tbreak;\n \t\t}\n+\n+\t\tstrbuf_release("ed);\n \t\tfflush(stdout);\n \t\treturn;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 23b8942edb..232bfd1723 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success FUNNYNAMES '--batch-check, -z with newline in non-existent named object' '\n+\tprintf \"HEAD:newline${LF}embedded\" >in &&\n+\tgit cat-file --batch-check -z actual &&\n+\n+\tprintf \"\\\"HEAD:newline\\\\\\\\nembedded\\\" missing\\n\" >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--\n2.39.0.rc0.57\n"},{"id":"468837","messageId":"93d61412-3786-b2b3-3fe8-4574336b08fb@dunelm.org.uk","threadId":"58923","inReplyTo":"20221209150048.2400648-2-toon@iotcl.com","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-09T19:33:37Z","receivedAt":"2022-12-09T19:33:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Toon\n\nOn 09/12/2022 15:00, Toon Claes wrote:\n> Since it's supported to have NUL-delimited input, introduced in\n> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n> 2022-07-22), it's possible to pass paths that contain newlines. This\n> works great when the object is found, but when it's not, the input path\n> is returned in the error message. Because this can contain newlines, the\n> error message might get spread over multiple lines, making it harder to\n> machine-parse this error message.\n> \n> With this change, the input is quote-formatted in the error message, if\n> needed. This ensures the error message is always on a single line and\n> makes parsing the error more straightforward.\n\nThanks for working on this. I'd previously suggested NUL terminating the \noutput of \"git cat-file -z\" to avoid this problem [1] but quoting the \nobject name is a better solution. The implementation looks good, thanks \nfor adding a test - are you sure we need the FUNNYNAMES guard on the \ntest even though it isn't creating files with funny names?\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://lore.kernel.org/git/66b71194-ad0e-18d0-e43b-71e5c47ba111@gmail.com/\n\n> Signed-off-by: Toon Claes \n> ---\n> builtin/cat-file.c | 10 ++++++++++\n> t/t1006-cat-file.sh | 8 ++++++++\n> 2 files changed, 18 insertions(+)\n> \n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index b3be58b1fb..67dd81238d 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -14,6 +14,7 @@\n> #include \"tree-walk.h\"\n> #include \"oid-array.h\"\n> #include \"packfile.h\"\n> +#include \"quote.h\"\n> #include \"object-store.h\"\n> #include \"promisor-remote.h\"\n> #include \"mailmap.h\"\n> @@ -475,6 +476,13 @@ static void batch_one_object(const char *obj_name,\n> \tresult = get_oid_with_context(the_repository, obj_name,\n> \t\t\t\t flags, &data->oid, &ctx);\n> \tif (result != FOUND) {\n> +\t\tstruct strbuf quoted = STRBUF_INIT;\n> +\n> +\t\tif (opt->nul_terminated) {\n> +\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n> +\t\t\tobj_name = quoted.buf;\n> +\t\t}\n> +\n> \t\tswitch (result) {\n> \t\tcase MISSING_OBJECT:\n> \t\t\tprintf(\"%s missing\\n\", obj_name);\n> @@ -499,6 +507,8 @@ static void batch_one_object(const char *obj_name,\n> \t\t\t result);\n> \t\t\tbreak;\n> \t\t}\n> +\n> +\t\tstrbuf_release("ed);\n> \t\tfflush(stdout);\n> \t\treturn;\n> \t}\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 23b8942edb..232bfd1723 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success FUNNYNAMES '--batch-check, -z with newline in non-existent named object' '\n> +\tprintf \"HEAD:newline${LF}embedded\" >in &&\n> +\tgit cat-file --batch-check -z actual &&\n> +\n> +\tprintf \"\\\"HEAD:newline\\\\\\\\nembedded\\\" missing\\n\" >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> --\n> 2.39.0.rc0.57\n"},{"id":"468852","messageId":"xmqq5yekyvrh.fsf@gitster.g","threadId":"58923","inReplyTo":"93d61412-3786-b2b3-3fe8-4574336b08fb@dunelm.org.uk","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-09T23:58:42Z","receivedAt":"2022-12-09T23:58:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Hi Toon\n>\n> On 09/12/2022 15:00, Toon Claes wrote:\n>> Since it's supported to have NUL-delimited input, introduced in\n>> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n>> 2022-07-22), it's possible to pass paths that contain newlines. This\n>> works great when the object is found, but when it's not, the input path\n>> is returned in the error message. Because this can contain newlines, the\n>> error message might get spread over multiple lines, making it harder to\n>> machine-parse this error message.\n>> With this change, the input is quote-formatted in the error message,\n>> if\n>> needed. This ensures the error message is always on a single line and\n>> makes parsing the error more straightforward.\n>\n> Thanks for working on this. I'd previously suggested NUL terminating\n> the output of \"git cat-file -z\" to avoid this problem [1] but quoting\n> the object name is a better solution.\n\nHmph. My knee-jerk reaction was that it is utterly disgusting if we\nquote when we do NUL-terminated. Is your \"quoting is OK over NUL\nterminating\" because \"-z\" applies only to the input? If so, then I\nwould agree that is OK, but shouldn't the quoting apply regardless\nof how the input is formulated? Why do we call the cquote helper\nonly under \"-z\"?\n"},{"id":"468868","messageId":"ddae1940-f58f-e410-7606-d0bcf8d06cd3@dunelm.org.uk","threadId":"58923","inReplyTo":"xmqq5yekyvrh.fsf@gitster.g","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-11T16:30:54Z","receivedAt":"2022-12-11T16:31:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 09/12/2022 23:58, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> Hi Toon\n>>\n>> On 09/12/2022 15:00, Toon Claes wrote:\n>>> Since it's supported to have NUL-delimited input, introduced in\n>>> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n>>> 2022-07-22), it's possible to pass paths that contain newlines. This\n>>> works great when the object is found, but when it's not, the input path\n>>> is returned in the error message. Because this can contain newlines, the\n>>> error message might get spread over multiple lines, making it harder to\n>>> machine-parse this error message.\n>>> With this change, the input is quote-formatted in the error message,\n>>> if\n>>> needed. This ensures the error message is always on a single line and\n>>> makes parsing the error more straightforward.\n>>\n>> Thanks for working on this. I'd previously suggested NUL terminating\n>> the output of \"git cat-file -z\" to avoid this problem [1] but quoting\n>> the object name is a better solution.\n> \n> Hmph. My knee-jerk reaction was that it is utterly disgusting if we\n> quote when we do NUL-terminated. Is your \"quoting is OK over NUL\n> terminating\" because \"-z\" applies only to the input?\n\nYes, if the object exists then delimiting the output with newlines is \nfine, it is only if the object is missing and its name contains a \nnewline that there is a problem. It also makes adopting \"-z\" in existing \nscripts easier as there is no change required when parsing the output.\n\nAs \"-z\" was added in 2.38 there is also a pragmatic reason to prefer \nquoting over NUL terminated output as it allows us to fix this issue \nwithout changing the output delimiter of an existing option.\n\n> If so, then I\n> would agree that is OK, but shouldn't the quoting apply regardless\n> of how the input is formulated? Why do we call the cquote helper\n> only under \"-z\"?\n\nWithout \"-z\" you cannot pass object names that contain newlines so not \nquoting the output does not cause a problem. We could start quoting the \nobject name without \"-z\" but we'd be changing the output without a huge \nbenefit.\n\nBest Wishes\n\nPhillip\n"},{"id":"468871","messageId":"xmqq8rjdv5um.fsf@gitster.g","threadId":"58923","inReplyTo":"ddae1940-f58f-e410-7606-d0bcf8d06cd3@dunelm.org.uk","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-12T00:11:13Z","receivedAt":"2022-12-12T00:11:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> Without \"-z\" you cannot pass object names that contain newlines so not\n> quoting the output does not cause a problem. We could start quoting\n> the object name without \"-z\" but we'd be changing the output without a\n> huge benefit.\n\nThat's fair. The next question is from a devil's advocate:\nis switching to the full cquote the best thing to do?\n\nIf we were using the full cquote from the very beginning, of course\nit is, simply because that is what is used in all other places in\nGit. Using the full cquote does mean a LF byte will be protected\n(i.e. instead of shown literally in the middle of other letters\naround LF, \"other\\nletters around LF\" would be shown), but pathnames\nwith backslashes and double quotes in them that have been shown\nwithout problems would be shown differently and will break existing\nparsers, which are written lazily with the assumption that they are\nperfectly happy to be \"simple\" thans to not having to deal with LF\n(because in their environment a path with LF in it do not matter).\n\nA bit safer thing to do is to replace LF (and not any other bytes\nthat would be quoted with full cquote) in the path given in these\nmessages with something else (like NUL to truncate the output\nthere). As these answers are given in order, the object names are\nnot absolutely needed to identify and match up the input and the\noutput, and properly written parsers would be prepared to see a\nresponse with an object name that it did not request and handle it\nsanely, such a change may not break such a parser for a path with\nany byte that are modified with full cquote.\n\nThe above is with a devil's adovocate hat on, and I do not care too\nmuch, as I do not think butchering backslash with full cquote would\nnot hurt even existing Windows users (if \"HEAD:t\\README.txt\" named\nthe same blob as \"HEAD:t/README.txt\" on a platform, doubling the\nbackslashes in the output would have made quite a lot of damage, but\nI do not think we allow backslashes to name tree paths).\n\nBy the way, there is another use of obj_name in batch_object_write()\nthat can show whatever byte in it literally to the output.\n\nThanks.\n"},{"id":"468885","messageId":"87cz8ohklt.fsf@to1.studio","threadId":"58923","inReplyTo":"xmqq8rjdv5um.fsf@gitster.g","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@to1.studio","sentAt":"2022-12-12T11:34:46Z","receivedAt":"2022-12-12T12:28:37Z","isPatch":true,"sender":{"key":"toon@to1.studio","avatar":null},"body":"\nJunio C Hamano writes:\n\n> Phillip Wood writes:\n>\n>> Without \"-z\" you cannot pass object names that contain newlines so not\n>> quoting the output does not cause a problem. We could start quoting\n>> the object name without \"-z\" but we'd be changing the output without a\n>> huge benefit.\n>\n> That's fair. The next question is from a devil's advocate:\n> is switching to the full cquote the best thing to do?\n>\n> If we were using the full cquote from the very beginning, of course\n> it is, simply because that is what is used in all other places in\n> Git. Using the full cquote does mean a LF byte will be protected\n> (i.e. instead of shown literally in the middle of other letters\n> around LF, \"other\\nletters around LF\" would be shown), but pathnames\n> with backslashes and double quotes in them that have been shown\n> without problems would be shown differently and will break existing\n> parsers, which are written lazily with the assumption that they are\n> perfectly happy to be \"simple\" thans to not having to deal with LF\n> (because in their environment a path with LF in it do not matter).\n>\n> A bit safer thing to do is to replace LF (and not any other bytes\n> that would be quoted with full cquote) in the path given in these\n> messages with something else (like NUL to truncate the output\n> there).\n\nSo object name \"HEAD:other\\nletters around LF\" would give the error\nmessage \"other missing\"? That error message would also occur when the\nuser does not provide -z. I think that might be confusing.\n\n> As these answers are given in order, the object names are\n> not absolutely needed to identify and match up the input and the\n> output, and properly written parsers would be prepared to see a\n> response with an object name that it did not request and handle it\n> sanely, such a change may not break such a parser for a path with\n> any byte that are modified with full cquote.\n\nYes, the answers are returned in order, so personally I don't care too\nmuch about the returned object name format. I even would be fine with a\ngeneric error message that omits the input name, for example \"object\nmissing\". As long as it's clear that the requested object is not found.\n\nFor your information, there is an extreme edge case a user could fake an\nobject, and that's what we want to avoid as well. For example the\ncommand (line break included):\n\nprintf \"aabbccddeeff00112233445566778899aabbccdd blob 26\nthis object is not\" | git cat-file --batch -z\n\nWould print:\n\naabbccddeeff00112233445566778899aabbccdd blob 26\nthis object is not missing\n\nThis is perfectly valid git-cat-file output. Luckily I don't see any way\nhow this can be abused. Generally I think it's a good idea to not return\nthe input as-is in any situation. We could only replace newlines, but\ncquoting already sanitizes the input, so why not use that?\n\n> The above is with a devil's adovocate hat on, and I do not care too\n> much, as I do not think butchering backslash with full cquote would\n> not hurt even existing Windows users (if \"HEAD:t\\README.txt\" named\n> the same blob as \"HEAD:t/README.txt\" on a platform, doubling the\n> backslashes in the output would have made quite a lot of damage, but\n> I do not think we allow backslashes to name tree paths).\n\n> By the way, there is another use of obj_name in batch_object_write()\n> that can show whatever byte in it literally to the output.\n\nAh thanks! I will include in the next version, when we reach a consensus\non when or what to cquote.\n\n--\nToon\n"},{"id":"468952","messageId":"xmqq5yegs28j.fsf@gitster.g","threadId":"58923","inReplyTo":"87cz8ohklt.fsf@to1.studio","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-12T22:09:48Z","receivedAt":"2022-12-12T22:09:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> Ah thanks! I will include in the next version, when we reach a consensus\n> on when or what to cquote.\n\nI think we already have consensus on when---\"-z\" may benefit,\notherwise we should not quote. I am not sure what you mean by \"what\nto cquote\"---are there things other than input object names that may\nbe ambiguous and require quoting?\n"},{"id":"468991","messageId":"1ea2be75-f67c-fa99-12ed-139825ec5c29@dunelm.org.uk","threadId":"58923","inReplyTo":"xmqq8rjdv5um.fsf@gitster.g","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-13T15:06:58Z","receivedAt":"2022-12-13T15:07:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/12/2022 00:11, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> Without \"-z\" you cannot pass object names that contain newlines so not\n>> quoting the output does not cause a problem. We could start quoting\n>> the object name without \"-z\" but we'd be changing the output without a\n>> huge benefit.\n> \n> That's fair. The next question is from a devil's advocate:\n> is switching to the full cquote the best thing to do?\n> \n> If we were using the full cquote from the very beginning, of course\n> it is, simply because that is what is used in all other places in\n> Git. Using the full cquote does mean a LF byte will be protected\n> (i.e. instead of shown literally in the middle of other letters\n> around LF, \"other\\nletters around LF\" would be shown), but pathnames\n> with backslashes and double quotes in them that have been shown\n> without problems would be shown differently and will break existing\n> parsers, which are written lazily with the assumption that they are\n> perfectly happy to be \"simple\" thans to not having to deal with LF\n> (because in their environment a path with LF in it do not matter).\n> \n> A bit safer thing to do is to replace LF (and not any other bytes\n> that would be quoted with full cquote) in the path given in these\n> messages with something else (like NUL to truncate the output\n> there). As these answers are given in order, the object names are\n> not absolutely needed to identify and match up the input and the\n> output, and properly written parsers would be prepared to see a\n> response with an object name that it did not request and handle it\n> sanely, such a change may not break such a parser for a path with\n> any byte that are modified with full cquote.\n> \n> The above is with a devil's adovocate hat on, and I do not care too\n> much, as I do not think butchering backslash with full cquote would\n> not hurt even existing Windows users (if \"HEAD:t\\README.txt\" named\n> the same blob as \"HEAD:t/README.txt\" on a platform, doubling the\n> backslashes in the output would have made quite a lot of damage, but\n> I do not think we allow backslashes to name tree paths).\n\nBy default quoting also affects names that contain non-ascii characters. \nIf we're worried about that we could only quote names that contain LF \nbut as you point out the answers are given in order so I don't think \nthere should be a problem with calling cquote unconditionally. (As this \noption has only existed for one release I suspect there aren't that many \nusers yet as well)\n\nBest Wishes\n\nPhillip\n\n> By the way, there is another use of obj_name in batch_object_write()\n> that can show whatever byte in it literally to the output.\n> \n> Thanks.\n"},{"id":"469005","messageId":"xmqqo7s6wfpl.fsf@gitster.g","threadId":"58923","inReplyTo":"1ea2be75-f67c-fa99-12ed-139825ec5c29@dunelm.org.uk","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-12-14T08:29:42Z","receivedAt":"2022-12-14T08:30:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> By default quoting also affects names that contain non-ascii\n> characters. If we're worried about that we could only quote names that\n> contain LF but as you point out the answers are given in order so I\n> don't think there should be a problem with calling cquote\n> unconditionally. (As this option has only existed for one release I\n> suspect there aren't that many users yet as well)\n\nGood point.\n"},{"id":"469350","messageId":"87a63i7h8h.fsf@to1.studio","threadId":"58923","inReplyTo":"93d61412-3786-b2b3-3fe8-4574336b08fb@dunelm.org.uk","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@to1.studio","sentAt":"2022-12-20T05:31:40Z","receivedAt":"2022-12-20T05:54:15Z","isPatch":true,"sender":{"key":"toon@to1.studio","avatar":null},"body":"\nPhillip Wood writes:\n\n> Thanks for working on this. I'd previously suggested NUL terminating the output\n> of \"git cat-file -z\" to avoid this problem [1]\n>\n> [1]\n> https://lore.kernel.org/git/66b71194-ad0e-18d0-e43b-71e5c47ba111@gmail.com/\n\nWhat happened to this proposal? I don't see any replies to that. That's\na bit sad, because it would have been nice to have it this behavior from\nthe start.\n\n> but quoting the object name is a better solution.\n\nI would not say it's a better solution, but it's a less invasive\nsolution that /minimizes/ breaking changes. Ideally I'd like to have NUL\nterminated output for \"git cat-file -z\". In a success situation I\nassume this would return:\n\n SP SP NUL\n NUL\n\nIn a failure situation something like:\n\n SP missing NUL\n\nSo when you pass -z you can keep reading until the first NUL and then\nyou'll know if you should read for contents as well.\n\nWould you consider change behavior to this now?\n\n--\nToon\n"},{"id":"469360","messageId":"aab41ec5-7854-b62c-f11c-ba850cbac95b@dunelm.org.uk","threadId":"58923","inReplyTo":"87a63i7h8h.fsf@to1.studio","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-12-20T10:18:51Z","receivedAt":"2022-12-20T10:18:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Toon\n\nOn 20/12/2022 05:31, Toon Claes wrote:\n> \n> Phillip Wood writes:\n> \n>> Thanks for working on this. I'd previously suggested NUL terminating the output\n>> of \"git cat-file -z\" to avoid this problem [1]\n>>\n>> [1]\n>> https://lore.kernel.org/git/66b71194-ad0e-18d0-e43b-71e5c47ba111@gmail.com/\n> \n> What happened to this proposal? I don't see any replies to that. That's\n> a bit sad, because it would have been nice to have it this behavior from\n> the start.\n\nYes it would have been nice, unfortunately it feel through the cracks \ndue to a combination of unfortunate timing and lack of time. I think the \npatch was probably in next by the time I realized the problem and wrote \nthat email. Taylor was on holiday at the time and then I went away \naround the time he got back. I know it was on his todo list but I think \nhe was busy catching up from being away getting ready for git merge. I \nhad other things I was working on and unfortunately didn't get round to \nfollowing it up.\n\n>> but quoting the object name is a better solution.\n> \n> I would not say it's a better solution, but it's a less invasive\n> solution that /minimizes/ breaking changes. Ideally I'd like to have NUL\n> terminated output for \"git cat-file -z\". In a success situation I\n> assume this would return:\n> \n> SP SP NUL\n> NUL\n> \n> In a failure situation something like:\n> \n> SP missing NUL\n> \n> So when you pass -z you can keep reading until the first NUL and then\n> you'll know if you should read for contents as well.\n> \n> Would you consider change behavior to this now?\n\nHmm I'm not sure (and luckily it isn't up to me to take the final \ndecision!). I really don't know how many people are using \"-z\" I suspect \nit is not many and so the fallout wouldn't be too bad but we'd still be \ninconveniencing some users. I theory we could so \"sorry we made a \nmistake when implementing this feature and have decided to change it\" \nbut we have a solution in your patch which hopefully does not break any \nusers (I say hopefully because think if one is careful and keep track of \nwhich responses you've read it is possible to detect missing objects now \neven if their name contains a new line but it will be messy and probably \nslightly inefficient but anyone doing that will notice the change in \noutput).\n\nOverall I'd say it is tempting but risky and inconvenient to any \nexisting users of \"-z\" (assuming someone else is actually using it). \nQuoting the object name is definitively the safer option at this point.\n\nBest Wishes\n\nPhillip\n"},{"id":"469432","messageId":"873599q61v.fsf@to1.studio","threadId":"58923","inReplyTo":"aab41ec5-7854-b62c-f11c-ba850cbac95b@dunelm.org.uk","subject":"Re: [PATCH 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@to1.studio","sentAt":"2022-12-21T12:42:02Z","receivedAt":"2022-12-21T12:45:25Z","isPatch":true,"sender":{"key":"toon@to1.studio","avatar":null},"body":"\nPhillip Wood writes:\n\n> Yes it would have been nice, unfortunately it feel through the cracks due to a\n> combination of unfortunate timing and lack of time. I think the patch was\n> probably in next by the time I realized the problem and wrote that email. Taylor\n> was on holiday at the time and then I went away around the time he got back. I\n> know it was on his todo list but I think he was busy catching up from being away\n> getting ready for git merge. I had other things I was working on and\n> unfortunately didn't get round to following it up.\n\nOh sorry, I didn't want to put blame on anyone. Things happen. I just\nwasn't sure I missed anything.\n\n> Overall I'd say it is tempting but risky and inconvenient to any existing users\n> of \"-z\" (assuming someone else is actually using it). Quoting the object name is\n> definitively the safer option at this point.\n\nI assume Taylor would be one of the consumers of output of \"-z\", so\nthanks for putting him in Cc to get his take on this.\n\n--\nToon\n"},{"id":"469804","messageId":"20230105062447.2943709-1-toon@iotcl.com","threadId":"58923","inReplyTo":"20221209150048.2400648-1-toon@iotcl.com","subject":"[PATCH v2 0/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-01-05T06:24:46Z","receivedAt":"2023-01-05T06:25:15Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"This is the second revision of the patch to quote-format the filename in the\nerror message of git-cat-file(1), when using option `-z`.\n\nChanges since v1:\n* Added the same fix to batch_object_write().\n* Removed FUNNYNAMES from the added test.\n\nToon Claes (1):\n cat-file: quote-format name in error when using -z\n\n builtin/cat-file.c | 19 +++++++++++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 27 insertions(+)\n\nRange-diff against v1:\n1: c13602e28e ! 1: 4f39eb0719 cat-file: quote-format name in error when using -z\n @@ builtin/cat-file.c\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n +@@ builtin/cat-file.c: 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\tstruct strbuf quoted = STRBUF_INIT;\n ++\n ++\t\t\tif (opt->nul_terminated &&\n ++\t\t\t obj_name) {\n ++\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n ++\t\t\t\tobj_name = quoted.buf;\n ++\t\t\t}\n ++\n + \t\t\tprintf(\"%s missing\\n\",\n + \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n ++\t\t\tstrbuf_release("ed);\n + \t\t\tfflush(stdout);\n + \t\t\treturn;\n + \t\t}\n @@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t flags, &data->oid, &ctx);\n @@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -z with newl\n \ttest_cmp expect actual\n '\n\n -+test_expect_success FUNNYNAMES '--batch-check, -z with newline in non-existent named object' '\n ++test_expect_success '--batch-check, -z with newline in non-existent named object' '\n +\tprintf \"HEAD:newline${LF}embedded\" >in &&\n +\tgit cat-file --batch-check -z actual &&\n +\n--\n2.38.1\n"},{"id":"469805","messageId":"20230105062447.2943709-2-toon@iotcl.com","threadId":"58923","inReplyTo":"20230105062447.2943709-1-toon@iotcl.com","subject":"[PATCH v2 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-01-05T06:24:47Z","receivedAt":"2023-01-05T06:26:21Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Since it's supported to have NUL-delimited input, introduced in\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), it's possible to pass paths that contain newlines. This\nworks great when the object is found, but when it's not, the input path\nis returned in the error message. Because this can contain newlines, the\nerror message might get spread over multiple lines, making it harder to\nmachine-parse this error message.\n\nWith this change, the input is quote-formatted in the error message, if\nneeded. This ensures the error message is always on a single line and\nmakes parsing the error more straightforward.\n\nSigned-off-by: Toon Claes \n---\n builtin/cat-file.c | 19 +++++++++++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex b3be58b1fb..6d7d5e2188 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -14,6 +14,7 @@\n #include \"tree-walk.h\"\n #include \"oid-array.h\"\n #include \"packfile.h\"\n+#include \"quote.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n@@ -439,8 +440,17 @@ 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\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\t\tif (opt->nul_terminated &&\n+\t\t\t obj_name) {\n+\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\t\tobj_name = quoted.buf;\n+\t\t\t}\n+\n \t\t\tprintf(\"%s missing\\n\",\n \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n+\t\t\tstrbuf_release("ed);\n \t\t\tfflush(stdout);\n \t\t\treturn;\n \t\t}\n@@ -475,6 +485,13 @@ static void batch_one_object(const char *obj_name,\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t flags, &data->oid, &ctx);\n \tif (result != FOUND) {\n+\t\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\tif (opt->nul_terminated) {\n+\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\tobj_name = quoted.buf;\n+\t\t}\n+\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n \t\t\tprintf(\"%s missing\\n\", obj_name);\n@@ -499,6 +516,8 @@ static void batch_one_object(const char *obj_name,\n \t\t\t result);\n \t\t\tbreak;\n \t\t}\n+\n+\t\tstrbuf_release("ed);\n \t\tfflush(stdout);\n \t\treturn;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 23b8942edb..11344a35bf 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success '--batch-check, -z with newline in non-existent named object' '\n+\tprintf \"HEAD:newline${LF}embedded\" >in &&\n+\tgit cat-file --batch-check -z actual &&\n+\n+\tprintf \"\\\"HEAD:newline\\\\\\\\nembedded\\\" missing\\n\" >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--\n2.38.1\n"},{"id":"470453","messageId":"20230116190749.4141516-1-toon@iotcl.com","threadId":"58923","inReplyTo":"20230105062447.2943709-1-toon@iotcl.com","subject":"[PATCH v3 0/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-01-16T19:07:48Z","receivedAt":"2023-01-16T19:08:04Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Hi there,\n\nThis third revision of this patch fixes broken test case that was added in\nt1006.\n\nToon Claes (1):\n cat-file: quote-format name in error when using -z\n\n builtin/cat-file.c | 19 +++++++++++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 27 insertions(+)\n\nRange-diff against v2:\n1: 4f39eb0719 ! 1: a56932572c cat-file: quote-format name in error when using -z\n @@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -z with newl\n '\n\n +test_expect_success '--batch-check, -z with newline in non-existent named object' '\n -+\tprintf \"HEAD:newline${LF}embedded\" >in &&\n ++\tprintf \"HEAD:newline${LF}missing\" >in &&\n +\tgit cat-file --batch-check -z actual &&\n +\n -+\tprintf \"\\\"HEAD:newline\\\\\\\\nembedded\\\" missing\\n\" >expect &&\n ++\tprintf \"\\\"HEAD:newline\\\\\\\\nmissing\\\" missing\\n\" >expect &&\n +\ttest_cmp expect actual\n +'\n +\n--\n2.39.0.rc0.57.g2e71cbbddd.dirty\n"},{"id":"470454","messageId":"20230116190749.4141516-2-toon@iotcl.com","threadId":"58923","inReplyTo":"20230116190749.4141516-1-toon@iotcl.com","subject":"[PATCH v3 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-01-16T19:07:49Z","receivedAt":"2023-01-16T19:08:06Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Since it's supported to have NUL-delimited input, introduced in\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), it's possible to pass paths that contain newlines. This\nworks great when the object is found, but when it's not, the input path\nis returned in the error message. Because this can contain newlines, the\nerror message might get spread over multiple lines, making it harder to\nmachine-parse this error message.\n\nWith this change, the input is quote-formatted in the error message, if\nneeded. This ensures the error message is always on a single line and\nmakes parsing the error more straightforward.\n\nSigned-off-by: Toon Claes \n---\n builtin/cat-file.c | 19 +++++++++++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 27 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex cc17635e76..b678f69773 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -14,6 +14,7 @@\n #include \"tree-walk.h\"\n #include \"oid-array.h\"\n #include \"packfile.h\"\n+#include \"quote.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n@@ -455,8 +456,17 @@ 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\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\t\tif (opt->nul_terminated &&\n+\t\t\t obj_name) {\n+\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\t\tobj_name = quoted.buf;\n+\t\t\t}\n+\n \t\t\tprintf(\"%s missing\\n\",\n \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n+\t\t\tstrbuf_release("ed);\n \t\t\tfflush(stdout);\n \t\t\treturn;\n \t\t}\n@@ -503,6 +513,13 @@ static void batch_one_object(const char *obj_name,\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t flags, &data->oid, &ctx);\n \tif (result != FOUND) {\n+\t\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\tif (opt->nul_terminated) {\n+\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\tobj_name = quoted.buf;\n+\t\t}\n+\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n \t\t\tprintf(\"%s missing\\n\", obj_name);\n@@ -527,6 +544,8 @@ static void batch_one_object(const char *obj_name,\n \t\t\t result);\n \t\t\tbreak;\n \t\t}\n+\n+\t\tstrbuf_release("ed);\n \t\tfflush(stdout);\n \t\treturn;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 23b8942edb..e8ce20e739 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n \ttest_cmp expect actual\n '\n\n+test_expect_success '--batch-check, -z with newline in non-existent named object' '\n+\tprintf \"HEAD:newline${LF}missing\" >in &&\n+\tgit cat-file --batch-check -z actual &&\n+\n+\tprintf \"\\\"HEAD:newline\\\\\\\\nmissing\\\" missing\\n\" >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--\n2.39.0.rc0.57.g2e71cbbddd.dirty\n"},{"id":"470499","messageId":"2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk","threadId":"58923","inReplyTo":"20230116190749.4141516-2-toon@iotcl.com","subject":"Re: [PATCH v3 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-17T15:24:09Z","receivedAt":"2023-01-17T15:25:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Toon\n\nOn 16/01/2023 19:07, Toon Claes wrote:\n> Since it's supported to have NUL-delimited input, introduced in\n> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n> 2022-07-22), it's possible to pass paths that contain newlines. This\n> works great when the object is found, but when it's not, the input path\n> is returned in the error message. Because this can contain newlines, the\n> error message might get spread over multiple lines, making it harder to\n> machine-parse this error message.\n> \n> With this change, the input is quote-formatted in the error message, if\n> needed. This ensures the error message is always on a single line and\n> makes parsing the error more straightforward.\n\nThis looks good but it would be nice to have test coverage for \nbatch_one_object() as well as batch_object_write()\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Toon Claes \n> ---\n> builtin/cat-file.c | 19 +++++++++++++++++++\n> t/t1006-cat-file.sh | 8 ++++++++\n> 2 files changed, 27 insertions(+)\n> \n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index cc17635e76..b678f69773 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -14,6 +14,7 @@\n> #include \"tree-walk.h\"\n> #include \"oid-array.h\"\n> #include \"packfile.h\"\n> +#include \"quote.h\"\n> #include \"object-store.h\"\n> #include \"promisor-remote.h\"\n> #include \"mailmap.h\"\n> @@ -455,8 +456,17 @@ 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\tstruct strbuf quoted = STRBUF_INIT;\n> +\n> +\t\t\tif (opt->nul_terminated &&\n> +\t\t\t obj_name) {\n> +\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n> +\t\t\t\tobj_name = quoted.buf;\n> +\t\t\t}\n> +\n> \t\t\tprintf(\"%s missing\\n\",\n> \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n> +\t\t\tstrbuf_release("ed);\n> \t\t\tfflush(stdout);\n> \t\t\treturn;\n> \t\t}\n> @@ -503,6 +513,13 @@ static void batch_one_object(const char *obj_name,\n> \tresult = get_oid_with_context(the_repository, obj_name,\n> \t\t\t\t flags, &data->oid, &ctx);\n> \tif (result != FOUND) {\n> +\t\tstruct strbuf quoted = STRBUF_INIT;\n> +\n> +\t\tif (opt->nul_terminated) {\n> +\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n> +\t\t\tobj_name = quoted.buf;\n> +\t\t}\n> +\n> \t\tswitch (result) {\n> \t\tcase MISSING_OBJECT:\n> \t\t\tprintf(\"%s missing\\n\", obj_name);\n> @@ -527,6 +544,8 @@ static void batch_one_object(const char *obj_name,\n> \t\t\t result);\n> \t\t\tbreak;\n> \t\t}\n> +\n> +\t\tstrbuf_release("ed);\n> \t\tfflush(stdout);\n> \t\treturn;\n> \t}\n> diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\n> index 23b8942edb..e8ce20e739 100755\n> --- a/t/t1006-cat-file.sh\n> +++ b/t/t1006-cat-file.sh\n> @@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success '--batch-check, -z with newline in non-existent named object' '\n> +\tprintf \"HEAD:newline${LF}missing\" >in &&\n> +\tgit cat-file --batch-check -z actual &&\n> +\n> +\tprintf \"\\\"HEAD:newline\\\\\\\\nmissing\\\" missing\\n\" >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> --\n> 2.39.0.rc0.57.g2e71cbbddd.dirty\n"},{"id":"472969","messageId":"20230303191708.77894-3-toon@iotcl.com","threadId":"58923","inReplyTo":"20230303191708.77894-1-toon@iotcl.com","subject":"[PATCH v4 2/2] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-03-03T19:17:08Z","receivedAt":"2023-03-03T19:22:47Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Since it's supported to have NUL-delimited input, introduced in\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), it's possible to pass paths that contain newlines. This\nworks great when the object is found, but when it's not, the input path\nis returned in the error message. Because this can contain newlines, the\nerror message might get spread over multiple lines, making it harder to\nmachine-parse this error message.\n\nWith this change, the input is quote-formatted in the error message, if\nneeded. This ensures the error message is always on a single line and\nmakes parsing the error more straightforward.\n\nSigned-off-by: Toon Claes \n---\n builtin/cat-file.c | 10 ++++++++++\n t/t1006-cat-file.sh | 8 ++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0c47190f17..e0c7bfaa56 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -14,6 +14,7 @@\n #include \"tree-walk.h\"\n #include \"oid-array.h\"\n #include \"packfile.h\"\n+#include \"quote.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n@@ -433,6 +434,14 @@ static void batch_print_error(const char *obj_name,\n \t\t\t enum get_oid_result result,\n \t\t\t struct batch_options* opt)\n {\n+\t\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\tif (opt->nul_terminated &&\n+\t\t obj_name) {\n+\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\tobj_name = quoted.buf;\n+\t\t}\n+\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n \t\t\tprintf(\"%s missing\\n\", obj_name);\n@@ -458,6 +467,7 @@ static void batch_print_error(const char *obj_name,\n \t\t\tbreak;\n \t\t}\n \t\tfflush(stdout);\n+\t\tstrbuf_release("ed);\n }\n \n /*\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 2d875b17d8..6ac3881305 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -447,6 +447,14 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--batch-check, -z with newline in non-existent named object' '\n+\tprintf \"HEAD:newline${LF}missing\" >in &&\n+\tgit cat-file --batch-check -z actual &&\n+\n+\tprintf \"\\\"HEAD:newline\\\\\\\\nmissing\\\" missing\\n\" >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-- \n2.39.2\n\n"},{"id":"472970","messageId":"20230303191708.77894-2-toon@iotcl.com","threadId":"58923","inReplyTo":"20230303191708.77894-1-toon@iotcl.com","subject":"[PATCH v4 1/2] cat-file: extract printing batch error message into function","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-03-03T19:17:07Z","receivedAt":"2023-03-03T19:22:51Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"There are two callsites that were formatting an error message in batch\nmode if an object could not be found. We're about to make changes to\nthat and to avoid doing that twice, we extract this into a separate\nfunction.\n\nSigned-off-by: Toon Claes \n---\n builtin/cat-file.c | 61 +++++++++++++++++++++++++---------------------\n 1 file changed, 33 insertions(+), 28 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex cc17635e76..0c47190f17 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -429,6 +429,37 @@ static void print_default_format(struct strbuf *scratch, struct expand_data *dat\n \t\t (uintmax_t)data->size);\n }\n \n+static void batch_print_error(const char *obj_name,\n+\t\t\t enum get_oid_result result,\n+\t\t\t struct batch_options* opt)\n+{\n+\t\tswitch (result) {\n+\t\tcase MISSING_OBJECT:\n+\t\t\tprintf(\"%s missing\\n\", obj_name);\n+\t\t\tbreak;\n+\t\tcase SHORT_NAME_AMBIGUOUS:\n+\t\t\tprintf(\"%s ambiguous\\n\", obj_name);\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\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\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\tbreak;\n+\t\tdefault:\n+\t\t\tBUG(\"unknown get_sha1_with_context result %d\\n\",\n+\t\t\t result);\n+\t\t\tbreak;\n+\t\t}\n+\t\tfflush(stdout);\n+}\n+\n /*\n * If \"pack\" is non-NULL, then \"offset\" is the byte offset within the pack from\n * which the object may be accessed (though note that we may also rely on\n@@ -455,9 +486,7 @@ 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\tfflush(stdout);\n+\t\t\tbatch_print_error(obj_name, MISSING_OBJECT, opt);\n \t\t\treturn;\n \t\t}\n \n@@ -503,31 +532,7 @@ static void batch_one_object(const char *obj_name,\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t flags, &data->oid, &ctx);\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\tbreak;\n-\t\tcase SHORT_NAME_AMBIGUOUS:\n-\t\t\tprintf(\"%s ambiguous\\n\", obj_name);\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\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\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\tbreak;\n-\t\tdefault:\n-\t\t\tBUG(\"unknown get_sha1_with_context result %d\\n\",\n-\t\t\t result);\n-\t\t\tbreak;\n-\t\t}\n-\t\tfflush(stdout);\n+\t\tbatch_print_error(obj_name, result, opt);\n \t\treturn;\n \t}\n \n-- \n2.39.2\n\n"},{"id":"472971","messageId":"20230303191708.77894-1-toon@iotcl.com","threadId":"58923","inReplyTo":"20230116190749.4141516-1-toon@iotcl.com","subject":"[PATCH v4 0/2] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-03-03T19:17:06Z","receivedAt":"2023-03-03T19:25:39Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Hi,\n\nThis is the fourth revision of my patch to fix the error message for\ngit-cat-file(1) when using --batch and -z.\n\nI was asked to provide test coverage for both code paths that produce this kind\nof error message. Instead I've decided to extract the error formatting into a\nseparate function. For two reasons:\n\n * It deduplicates code.\n\n * The code path that was not tested is hit by --batch-all-objects. I was not\n able to set up a test that hits that code. I'm happy to write that test, if\n anyone can give me a pointer how to produce a \"missing\" error with that\n option.\n\n\nToon\n\n\n\nToon Claes (2):\n cat-file: extract printing batch error message into function\n cat-file: quote-format name in error when using -z\n\n builtin/cat-file.c | 71 +++++++++++++++++++++++++++------------------\n t/t1006-cat-file.sh | 8 +++++\n 2 files changed, 51 insertions(+), 28 deletions(-)\n\nRange-diff against v3:\n-: ---------- > 1: b2f4ce88f2 cat-file: extract printing batch error message into function\n1: a56932572c ! 2: 9e31796dc1 cat-file: quote-format name in error when using -z\n @@ builtin/cat-file.c\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n #include \"mailmap.h\"\n -@@ builtin/cat-file.c: 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\tstruct strbuf quoted = STRBUF_INIT;\n -+\n -+\t\t\tif (opt->nul_terminated &&\n -+\t\t\t obj_name) {\n -+\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n -+\t\t\t\tobj_name = quoted.buf;\n -+\t\t\t}\n -+\n - \t\t\tprintf(\"%s missing\\n\",\n - \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n -+\t\t\tstrbuf_release("ed);\n - \t\t\tfflush(stdout);\n - \t\t\treturn;\n - \t\t}\n -@@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,\n - \tresult = get_oid_with_context(the_repository, obj_name,\n - \t\t\t\t flags, &data->oid, &ctx);\n - \tif (result != FOUND) {\n +@@ builtin/cat-file.c: static void batch_print_error(const char *obj_name,\n + \t\t\t enum get_oid_result result,\n + \t\t\t struct batch_options* opt)\n + {\n +\t\tstruct strbuf quoted = STRBUF_INIT;\n +\n -+\t\tif (opt->nul_terminated) {\n ++\t\tif (opt->nul_terminated &&\n ++\t\t obj_name) {\n +\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n +\t\t\tobj_name = quoted.buf;\n +\t\t}\n @@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n \t\t\tprintf(\"%s missing\\n\", obj_name);\n -@@ builtin/cat-file.c: static void batch_one_object(const char *obj_name,\n - \t\t\t result);\n +@@ builtin/cat-file.c: static void batch_print_error(const char *obj_name,\n \t\t\tbreak;\n \t\t}\n -+\n -+\t\tstrbuf_release("ed);\n \t\tfflush(stdout);\n - \t\treturn;\n - \t}\n ++\t\tstrbuf_release("ed);\n + }\n +\n + /*\n\n ## t/t1006-cat-file.sh ##\n @@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n--\n2.39.2\n"},{"id":"472972","messageId":"xmqqv8jhegnx.fsf@gitster.g","threadId":"58923","inReplyTo":"20230303191708.77894-1-toon@iotcl.com","subject":"Re: [PATCH v4 0/2] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T20:14:26Z","receivedAt":"2023-03-03T20:14:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> I was asked to provide test coverage for both code paths that produce this kind\n> of error message. Instead I've decided to ...\n\nIs code refactoring a substitute for making sure we have adequate\ntest coverage? If not, \"Instead I've decided to\" is a strange thing\nto say at this point in the cover letter.\n\nRelative to the previous round, you cleaned up the code by\nintroducing a helper function to show error messages, which gave us\nreduced duplication of the code. This is a very good thing and is\nworth mentioning.\n\nYou however didn't get around to add adequate test coverage to the\nresulting code, which was one thing that the previous round was\nfound lacking, and you can use help to improve these patches in this\narea, to help them get merged. Some of the error cases happen only\nin a corrupt repository that lacks objects that are expected to be\nthere (e.g. a tree points at a blob in a non-lazy clone and blob is\nmissing), so such a test may have to deliberately corrupt the test\nrepository by removing .git/object/??/* files. \n\nHopefully we'll hear from those who are willing to help. Thanks for\nworking on this topic.\n\n\n\n\n\n"},{"id":"472973","messageId":"xmqqcz5peg3v.fsf@gitster.g","threadId":"58923","inReplyTo":"20230303191708.77894-2-toon@iotcl.com","subject":"Re: [PATCH v4 1/2] cat-file: extract printing batch error message into function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T20:26:28Z","receivedAt":"2023-03-03T20:26:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> There are two callsites that were formatting an error message in batch\n> mode if an object could not be found. We're about to make changes to\n> that and to avoid doing that twice, we extract this into a separate\n> function.\n>\n> Signed-off-by: Toon Claes \n> ...\n> +static void batch_print_error(const char *obj_name,\n> +\t\t\t enum get_oid_result result,\n> +\t\t\t struct batch_options* opt)\n> +{\n> +\t\tswitch (result) {\n\nThe body of this function is indented way too deep. You should lose\none HT at the beginning from all its lines.\n\n> +\t\tcase MISSING_OBJECT:\n> +\t\t\tprintf(\"%s missing\\n\", obj_name);\n> +\t\t\tbreak;\n\nThis one expects that obj_name is always usable as a string.\n\n> @@ -455,9 +486,7 @@ 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\tfflush(stdout);\n\nThis caller used to be prepared for the case where obj_name is NULL\nand used the hexadecimal object name stored in data->oid in such a\ncase, but now ...\n\n> +\t\t\tbatch_print_error(obj_name, MISSING_OBJECT, opt);\n\n... the updated caller assumes that obj_name is always safe to feed\nto printf(\"%s\") above.\n\nMaybe obj_name is always not-NULL when the control reaches this\npoint in which case the new code would be safe, but if that is the\ncase, the proposed log message should explain how that is true to\njustify this change.\n\nAs batch_object_cb() makes a call to batch_object_write() with\nobj_name set to NULL, I do not think this change is defensible,\nthough.\n"},{"id":"472980","messageId":"xmqqilfhctrr.fsf@gitster.g","threadId":"58923","inReplyTo":"xmqqcz5peg3v.fsf@gitster.g","subject":"Re: [PATCH v4 1/2] cat-file: extract printing batch error message into function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-03T23:14:16Z","receivedAt":"2023-03-03T23:14:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> As batch_object_cb() makes a call to batch_object_write() with\n> obj_name set to NULL, I do not think this change is defensible,\n> though.\n\nThis indeed seems to break t5313 when queued on top of 'master'; it\ntries to run \"git cat-file --batch-all-objects --batch-check\" and\nhits the exact codepath where a missing object is sent to the error\ncodepath without obj_name set to anything.\n\nI guess we now have an existing test that you can mimic that\nexhibits \"missing\" error? I do not know offhand if this test\nalready qualifies as the test coverage Phillip wanted to make sure\nexists.\n\n..... >8 .......... >8 .......... >8 .......... >8 .......... >8 .....\n\nexpecting success of 5313.3 'pack/index object count mismatch': \n\tdo_pack $object &&\n\tmunge $pack 8 \"\\377\\0\\0\\0\" &&\n\tclear_base &&\n\n\t# We enumerate the objects from the completely-fine\n\t# .idx, but notice later that the .pack is bogus\n\t# and fail to show any data.\n\techo \"$object missing\" >expect &&\n\tgit cat-file --batch-all-objects --batch-check >actual &&\n\ttest_cmp expect actual &&\n\n\t# ...and here fail to load the object (without segfaulting),\n\t# but fallback to a good copy if available.\n\ttest_must_fail git cat-file blob $object &&\n\trestore_base &&\n\tgit cat-file blob $object >actual &&\n\ttest_cmp file actual &&\n\n\t# ...and make sure that index-pack --verify, which has its\n\t# own reading routines, does not segfault.\n\ttest_must_fail git index-pack --verify $pack\n\n4+0 records in\n4+0 records out\n4 bytes copied, 0.000119221 s, 33.6 kB/s\nerror: packfile .git/objects/pack/pack-67be769e2843d598c78218852612520795998892.pack claims to have 4278190080 objects while index indicates 1 objects\nerror: packfile .git/objects/pack/pack-67be769e2843d598c78218852612520795998892.pack claims to have 4278190080 objects while index indicates 1 objects\n--- expect\t2023-03-03 23:11:37.504011940 +0000\n+++ actual\t2023-03-03 23:11:37.508012250 +0000\n@@ -1 +1 @@\n-fff0a2476aa5c8e60a3ef21cfc66e0cc670920be missing\n+(null) missing\nnot ok 3 - pack/index object count mismatch\n"},{"id":"476982","messageId":"20230510190116.795641-2-toon@iotcl.com","threadId":"58923","inReplyTo":"20230510190116.795641-1-toon@iotcl.com","subject":"[PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-05-10T19:01:16Z","receivedAt":"2023-05-10T19:09:02Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"Since it's supported to have NUL-delimited input, introduced in\ndb9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n2022-07-22), it's possible to pass paths that contain newlines. This\nworks great when the object is found, but when it's not, the input path\nis returned in the error message. Because this can contain newlines, the\nerror message might get spread over multiple lines, making it harder to\nmachine-parse this error message.\n\nWith this change, the input is quote-formatted in the error message, if\nneeded. This ensures the error message is always on a single line and\nmakes parsing the error more straightforward.\n\nSigned-off-by: Toon Claes \n---\n builtin/cat-file.c | 19 +++++++++++++++++++\n t/t1006-cat-file.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex 0bafc14e6c..0ff31c4593 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -25,6 +25,7 @@\n #include \"object-store.h\"\n #include \"replace-object.h\"\n #include \"promisor-remote.h\"\n+#include \"quote.h\"\n #include \"mailmap.h\"\n #include \"write-or-die.h\"\n \n@@ -470,8 +471,17 @@ 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\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\t\tif (opt->nul_terminated &&\n+\t\t\t obj_name) {\n+\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\t\tobj_name = quoted.buf;\n+\t\t\t}\n+\n \t\t\tprintf(\"%s missing\\n\",\n \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n+\t\t\tstrbuf_release("ed);\n \t\t\tfflush(stdout);\n \t\t\treturn;\n \t\t}\n@@ -518,6 +528,13 @@ static void batch_one_object(const char *obj_name,\n \tresult = get_oid_with_context(the_repository, obj_name,\n \t\t\t\t flags, &data->oid, &ctx);\n \tif (result != FOUND) {\n+\t\tstruct strbuf quoted = STRBUF_INIT;\n+\n+\t\tif (opt->nul_terminated) {\n+\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n+\t\t\tobj_name = quoted.buf;\n+\t\t}\n+\n \t\tswitch (result) {\n \t\tcase MISSING_OBJECT:\n \t\t\tprintf(\"%s missing\\n\", obj_name);\n@@ -542,6 +559,8 @@ static void batch_one_object(const char *obj_name,\n \t\t\t result);\n \t\t\tbreak;\n \t\t}\n+\n+\t\tstrbuf_release("ed);\n \t\tfflush(stdout);\n \t\treturn;\n \t}\ndiff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh\nindex 8eac74b59c..03c1bfb86b 100755\n--- a/t/t1006-cat-file.sh\n+++ b/t/t1006-cat-file.sh\n@@ -447,6 +447,33 @@ test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success '--batch-check, -z with newline in non-existent named object' '\n+\tprintf \"HEAD:newline${LF}missing\" >in &&\n+\tgit cat-file --batch-check -z actual &&\n+\n+\tprintf \"\\\"HEAD:newline\\\\\\\\nmissing\\\" missing\\n\" >expect &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success FUNNYNAMES '--batch-check, -z with missing object having newline in name' '\n+\tgit init missing-object-newline &&\n+\t(\n+\t\tcd missing-object-newline &&\n+\t\tfile=\"newline${LF}embedded\" &&\n+\t\techo_without_newline \"hello\" > $file &&\n+\t\tgit add \"$file\" &&\n+\t\tgit commit -m \"file with newline embedded\" &&\n+\t\ttest_tick &&\n+\n+\t\tsha1=$(git rev-parse HEAD:\"$file\") &&\n+\t\trm .git/objects/$(test_oid_to_path $sha1) &&\n+\t\tprintf \"HEAD:$file\" >in &&\n+\t\tgit cat-file --batch-check -z actual &&\n+\t\tprintf \"\\\"HEAD:newline\\\\\\\\nembedded\\\" missing\\n\" >expect &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n batch_command_multiple_info=\"info $hello_sha1\n info $tree_sha1\n info $commit_sha1\n-- \n2.40.1\n\n"},{"id":"476983","messageId":"20230510190116.795641-1-toon@iotcl.com","threadId":"58923","inReplyTo":"xmqqilfhctrr.fsf@gitster.g","subject":"[PATCH v5 0/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-05-10T19:01:15Z","receivedAt":"2023-05-10T19:10:11Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"This is the fifth revision of this patch. The code changes are identical to v3,\nbut there was one comment[1] about adding test coverage for\nbatch_object_write(). In v4 I tried another approach, but that didn't work well.\nSo for this version I'm just adding the test to version 3 as requested. See\nbelow the range-diff against v3, ignoring v4 ever happened.\n\nKind regards,\nToon\n\n[1]: https://public-inbox.org/git/2a2a46f0-a9bc-06a6-72e1-28800518777c@dunelm.org.uk/\n\nToon Claes (1):\n cat-file: quote-format name in error when using -z\n\n builtin/cat-file.c | 19 +++++++++++++++++++\n t/t1006-cat-file.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+)\n\nRange-diff against v3:\n1: 16f7a123d8 ! 1: 14bccfa9fd cat-file: quote-format name in error when using -z\n @@ t/t1006-cat-file.sh: test_expect_success FUNNYNAMES '--batch-check, -z with newl\n +\tprintf \"\\\"HEAD:newline\\\\\\\\nmissing\\\" missing\\n\" >expect &&\n +\ttest_cmp expect actual\n +'\n ++\n ++test_expect_success FUNNYNAMES '--batch-check, -z with missing object having newline in name' '\n ++\tgit init missing-object-newline &&\n ++\t(\n ++\t\tcd missing-object-newline &&\n ++\t\tfile=\"newline${LF}embedded\" &&\n ++\t\techo_without_newline \"hello\" > $file &&\n ++\t\tgit add \"$file\" &&\n ++\t\tgit commit -m \"file with newline embedded\" &&\n ++\t\ttest_tick &&\n ++\n ++\t\tsha1=$(git rev-parse HEAD:\"$file\") &&\n ++\t\trm .git/objects/$(test_oid_to_path $sha1) &&\n ++\t\tprintf \"HEAD:$file\" >in &&\n ++\t\tgit cat-file --batch-check -z actual &&\n ++\t\tprintf \"\\\"HEAD:newline\\\\\\\\nembedded\\\" missing\\n\" >expect &&\n ++\t\ttest_cmp expect actual\n ++\t)\n ++'\n +\n batch_command_multiple_info=\"info $hello_sha1\n info $tree_sha1\n--\n2.40.1\n"},{"id":"476988","messageId":"xmqqpm782be6.fsf@gitster.g","threadId":"58923","inReplyTo":"20230510190116.795641-2-toon@iotcl.com","subject":"Re: [PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-10T20:13:05Z","receivedAt":"2023-05-10T20:13:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> Since it's supported to have NUL-delimited input, introduced in\n> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n> 2022-07-22), it's possible to pass paths that contain newlines.\n\nIt has been a while since I saw this patch the last time, and it did\nnot immediately click to me how \"pass paths\" relates to passing\nobject names, which is what \"--batch-check\" takes. Perhaps\n\n \"cat-file --batch-check\" may be fed object names of blobs and\n trees as \":\", and with its \"-z\" option, it is\n possible to feed or with newlines in it.\n\nor something?\n\n> This\n> works great when the object is found, but when it's not, the input path\n> is returned in the error message. Because this can contain newlines, the\n> error message might get spread over multiple lines, making it harder to\n> machine-parse this error message.\n\nGood description. I may suggest\n\n \"the input path is returned\" -> \"the input is shown verbatim\"\n\nbecause is not the only thing that can contain LF. E.g.\n\n $ git show -s 'HEAD^{/introduced in\n > db9d67}'\n\ncan find the commit resulting from this patch, so\n\n $ printf \"%s\\0\" 'HEAD^{/introduced in\n > db9d67}:builtin/cat-file.cc' |\n > git cat-file --batch-check -z \n\nwould be an input record with newline in it, that has no newline in\nthe path.\n\n> With this change, the input is quote-formatted in the error message, if\n> needed. This ensures the error message is always on a single line and\n> makes parsing the error more straightforward.\n\nDrop \"With this change, ...\" and give a command to the codebase to\nc-quote the object name in the output, e.g.\n\n C-quote the object name from the input in the error message as\n needed, to ensure that the error message is on a single line and\n ...\n\nThe other side of the coin, however, is that an existing project\nthat is sane enough not to have a path with LF in it, but is not\nsane enough to avoid a path with double-quote in it, would now stop\nbeing able to parse the error message for a missing path.\n\nIt is a regression, and we may argue that it is not a large enough\nregression to block progress given by this patch, but if it is not\ncommon enough to have funny characters in the paths then we wouldn't\nbe seeing this patch in the first place. So I would prefer to see\nthat we at least admit that we are deliberately making this change\nwith a known regression. Perhaps add something like\n\n Note that if a project already parses the error message\n correctly because it does not have paths with newlines, this is\n a breaking change if it has paths with characters that need\n c-quoting (like double quotes and backslashes) that are not\n newlines. We consider that this is a small enough price to pay\n to allow newlines in paths because ...\n\nand fill the \"because ...\" part is sensible. I am not filling the\n\"because\" part, simply because I do not offhand see any good excuse\nto rob Peter to pay Paul in this case.\n\n> @@ -470,8 +471,17 @@ 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\tstruct strbuf quoted = STRBUF_INIT;\n> +\n> +\t\t\tif (opt->nul_terminated &&\n> +\t\t\t obj_name) {\n> +\t\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n> +\t\t\t\tobj_name = quoted.buf;\n> +\t\t\t}\n> +\n> \t\t\tprintf(\"%s missing\\n\",\n> \t\t\t obj_name ? obj_name : oid_to_hex(&data->oid));\n> +\t\t\tstrbuf_release("ed);\n> \t\t\tfflush(stdout);\n> \t\t\treturn;\n> \t\t}\n\nInteresting.\n\nWhen running \"--batch-all-objects\", we do not have obj_name, and we\ndo not have anything to \"quote\", but the fallback label is the full\nhexadecimal object name, and we do not have to worry about line\nbreaks.\n\nOK.\n\n> @@ -518,6 +528,13 @@ static void batch_one_object(const char *obj_name,\n> \tresult = get_oid_with_context(the_repository, obj_name,\n> \t\t\t\t flags, &data->oid, &ctx);\n> \tif (result != FOUND) {\n> +\t\tstruct strbuf quoted = STRBUF_INIT;\n> +\n> +\t\tif (opt->nul_terminated) {\n> +\t\t\tquote_c_style(obj_name, "ed, NULL, 0);\n> +\t\t\tobj_name = quoted.buf;\n> +\t\t}\n> +\n> \t\tswitch (result) {\n> \t\tcase MISSING_OBJECT:\n> \t\t\tprintf(\"%s missing\\n\", obj_name);\n> @@ -542,6 +559,8 @@ static void batch_one_object(const char *obj_name,\n> \t\t\t result);\n> \t\t\tbreak;\n> \t\t}\n> +\n> +\t\tstrbuf_release("ed);\n> \t\tfflush(stdout);\n> \t\treturn;\n> \t}\n\nThis is the side that runs without \"--batch-all-objects\" and always\nhas non-null obj_name, so it looks a bit different from the other\nhunk and is more straight-forward. Looking good.\n\nThanks.\n"},{"id":"477175","messageId":"87h6sh6f81.fsf@iotcl.com","threadId":"58923","inReplyTo":"xmqqpm782be6.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Toon Claes","fromEmail":"toon@iotcl.com","sentAt":"2023-05-12T08:54:20Z","receivedAt":"2023-05-12T10:24:26Z","isPatch":true,"sender":{"key":"toon@iotcl.com","avatar":"https://avatars.githubusercontent.com/u/121621?v=4"},"body":"\nJunio C Hamano writes:\n\n> Toon Claes writes:\n>\n>> Since it's supported to have NUL-delimited input, introduced in\n>> db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`,\n>> 2022-07-22), it's possible to pass paths that contain newlines.\n>\n> It has been a while since I saw this patch the last time, and it did\n> not immediately click to me how \"pass paths\" relates to passing\n> object names, which is what \"--batch-check\" takes. Perhaps\n>\n> \"cat-file --batch-check\" may be fed object names of blobs and\n> trees as \":\", and with its \"-z\" option, it is\n> possible to feed or with newlines in it.\n>\n> or something?\n\nGood suggestion.\n\n>> This\n>> works great when the object is found, but when it's not, the input path\n>> is returned in the error message. Because this can contain newlines, the\n>> error message might get spread over multiple lines, making it harder to\n>> machine-parse this error message.\n>\n> Good description. I may suggest\n>\n> \"the input path is returned\" -> \"the input is shown verbatim\"\n>\n> because is not the only thing that can contain LF. E.g.\n>\n> $ git show -s 'HEAD^{/introduced in\n> > db9d67}'\n>\n> can find the commit resulting from this patch, so\n>\n> $ printf \"%s\\0\" 'HEAD^{/introduced in\n> > db9d67}:builtin/cat-file.cc' |\n> > git cat-file --batch-check -z\n\nThanks for that thorough explanation, makes a lot of sense.\n\n>\n> would be an input record with newline in it, that has no newline in\n> the path.\n>\n>> With this change, the input is quote-formatted in the error message, if\n>> needed. This ensures the error message is always on a single line and\n>> makes parsing the error more straightforward.\n>\n> Drop \"With this change, ...\" and give a command to the codebase to\n> c-quote the object name in the output, e.g.\n>\n> C-quote the object name from the input in the error message as\n> needed, to ensure that the error message is on a single line and\n> ...\n\nSure, I'll update the commit message accordingly.\n\n> The other side of the coin, however, is that an existing project\n> that is sane enough not to have a path with LF in it, but is not\n> sane enough to avoid a path with double-quote in it, would now stop\n> being able to parse the error message for a missing path.\n\nAh interesting, this is not a case I did consider before.\n\n> It is a regression, and we may argue that it is not a large enough\n> regression to block progress given by this patch, but if it is not\n> common enough to have funny characters in the paths then we wouldn't\n> be seeing this patch in the first place. So I would prefer to see\n> that we at least admit that we are deliberately making this change\n> with a known regression. Perhaps add something like\n>\n> Note that if a project already parses the error message\n> correctly because it does not have paths with newlines, this is\n> a breaking change if it has paths with characters that need\n> c-quoting (like double quotes and backslashes) that are not\n> newlines. We consider that this is a small enough price to pay\n> to allow newlines in paths because ...\n>\n> and fill the \"because ...\" part is sensible. I am not filling the\n> \"because\" part, simply because I do not offhand see any good excuse\n> to rob Peter to pay Paul in this case.\n\nI intended to finalize this patch sooner, but other priorities popped\nup. The -z flag was added in v2.38 and it would have been nice to have\nthe patch in v2.40, this would reduce the number of Peters affected. Now\nwe're a couple months and yet another release further between\nintroduction of the flag and making the regression, so I feel your\nsentiment.\n\nI previous conversations[1] we've been talking about making the output\nNUL-terminated as well. We agreed on the cquote fix because the -z flag\nwas still fresh, but maybe at this time we need to revisit this.\n\nIdeally the output should be NUL-terminated if -z is used. This was also\nsuggested[2] when the flag was introduced. Obviously we cannot change\nthis now, because it would break behavior for *everyone* using -z, not\nonly when funny names are used. So if we want to go this route, we\nshould only do so with another flag (e.g. `--null-output`) or a config\noption.\n\nBut I was looking at the git-config(1) documentation:\n\n> core.quotePath::\n> \tCommands that output paths (e.g. 'ls-files', 'diff'), will\n> \tquote \"unusual\" characters in the pathname by enclosing the\n> \tpathname in double-quotes and escaping those characters with\n> \tbackslashes in the same way C escapes control characters (e.g.\n> \t`\\t` for TAB, `\\n` for LF, `\\\\` for backslash) or bytes with\n> \tvalues larger than 0x80 (e.g. octal `\\302\\265` for \"micro\" in\n> \tUTF-8). If this variable is set to false, bytes higher than\n> \t0x80 are not considered \"unusual\" any more. Double-quotes,\n> \tbackslash and control characters are always escaped regardless\n> \tof the setting of this variable. A simple space character is\n> \tnot considered \"unusual\". Many commands can output pathnames\n> \tcompletely verbatim using the `-z` option. The default value\n> \tis true.\n\nIf you read this, the changes of this patch fully contradict this. Also\ndocumentation on other commands (e.g. git-check-ignore(1)) using `-z`\nwill mention the verbatim output. So at the moment I'm leaning toward\nanother solution. I'm looping in Taylor as he added the flag initially,\nso he might have some insights how they are using it and how they expect\nit to work.\n\n-- Toon\n\n[1]: https://lore.kernel.org/git/xmqq5yekyvrh.fsf@gitster.g/\n[2]: https://lore.kernel.org/git/66b71194-ad0e-18d0-e43b-71e5c47ba111@gmail.com/\n"},{"id":"477186","messageId":"xmqqy1ltqygb.fsf@gitster.g","threadId":"58923","inReplyTo":"87h6sh6f81.fsf@iotcl.com","subject":"Re: [PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-12T16:57:56Z","receivedAt":"2023-05-12T16:58:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Toon Claes writes:\n\n> Ideally the output should be NUL-terminated if -z is used. This was also\n> suggested[2] when the flag was introduced. Obviously we cannot change\n> this now, because it would break behavior for *everyone* using -z, not\n> only when funny names are used. So if we want to go this route, we\n> should only do so with another flag (e.g. `--null-output`) or a config\n> option.\n\nYes, `--null-output` came also to my mind. As this new mode of\noutput is for consumption by programs, letting them read\nNUL-terminated records is a viable, if cumbersome, possibility.\n\n> But I was looking at the git-config(1) documentation:\n>\n>> core.quotePath::\n>> \tCommands that output paths (e.g. 'ls-files', 'diff'), will\n>> \tquote \"unusual\" characters in the pathname by enclosing the\n>> \tpathname in double-quotes and escaping those characters with\n>> \tbackslashes in the same way C escapes control characters (e.g.\n>> \t`\\t` for TAB, `\\n` for LF, `\\\\` for backslash) or bytes with\n>> \tvalues larger than 0x80 (e.g. octal `\\302\\265` for \"micro\" in\n>> \tUTF-8). If this variable is set to false, bytes higher than\n>> \t0x80 are not considered \"unusual\" any more. Double-quotes,\n>> \tbackslash and control characters are always escaped regardless\n>> \tof the setting of this variable. A simple space character is\n>> \tnot considered \"unusual\". Many commands can output pathnames\n>> \tcompletely verbatim using the `-z` option. The default value\n>> \tis true.\n>\n> If you read this, the changes of this patch fully contradict this.\n\nHmph, I do not quite see where the contradiction is. If you mean\n\"Many commands can output\" part, I do not think it applies here.\nFirst, your \"cat-file\" does not have to be a part of \"many\". More\nimportantly, the mention of `-z` there is about the option accepted\nby the diff family of commants, e.g. \"git diff --name-only -z\nHEAD^\", that is an output record separator. Your \"-z\" is about the\ninput record separator, and if you are not changing \"-z\" to suddenly\nmean both input and output separator to break existing scripts that\nexpect \"-z\" only applies to input, the above \"completely verbatim\"\ndoes not apply to you.\n\n> Also\n> documentation on other commands (e.g. git-check-ignore(1)) using `-z`\n> will mention the verbatim output.\n\nAgain, it is about the output.\n\nStepping back a bit, how big a problem is this in real life? It\ncertainly is possible to create a pathname with funny byte values in\nit, and in some environments,letters like single-quote that are\nconsidered cumbersome to handle by those who are used to CLI\nprograms may be commonplace. But a path with newline? Or any\ncontrol character for that matter? And this is not even the primary\noutput from the program but is an error message for consumption by\nhumans, no?\n\nI am wondering if it is simpler to just declare that the paths\noutput in error messages have certain bytes, probably all control\ncharacters other than HT, replaced with a dot, and tell the users\nnot to rely on the pathnames being intact if they contain funny\nbytes in them. That way, with the definition of \"work\" being \"you\ncan read the path out of error messages that talk about it\", paths\nwith bytes that c-quote mechanism butchers, like double quotes and\nbackslashes, that have worked before will not be broken, and paths\nwith LF or CRLF in them that have never worked would not work, but\nat least does not break the input stream of whoever is reading the\nerror messages line by line.\n\nI dunno.\n\n\n"},{"id":"477278","messageId":"ec139b78-1d36-f894-e39f-f29877a67b18@gmail.com","threadId":"58923","inReplyTo":"xmqqy1ltqygb.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-05-15T08:47:38Z","receivedAt":"2023-05-15T08:47:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 12/05/2023 17:57, Junio C Hamano wrote:\n> Toon Claes writes:\n> Stepping back a bit, how big a problem is this in real life? It\n> certainly is possible to create a pathname with funny byte values in\n> it, and in some environments,letters like single-quote that are\n> considered cumbersome to handle by those who are used to CLI\n> programs may be commonplace. But a path with newline? Or any\n> control character for that matter? And this is not even the primary\n> output from the program but is an error message for consumption by\n> humans, no?\n> \n> I am wondering if it is simpler to just declare that the paths\n> output in error messages have certain bytes, probably all control\n> characters other than HT, replaced with a dot, and tell the users\n> not to rely on the pathnames being intact if they contain funny\n> bytes in them.\n\nWe could only c-quote the name when it contains a control character \nother that HT. That way names containing double quotes and backslashes \nare unchanged but it will still be possible to parse the path from the \nerror message. If we're going to munge the name we might as well use our \nstandard quoting rather than some ad-hoc scheme.\n\nBest Wishes\n\nPhillip\n\n That way, with the definition of \"work\" being \"you\n> can read the path out of error messages that talk about it\", paths\n> with bytes that c-quote mechanism butchers, like double quotes and\n> backslashes, that have worked before will not be broken, and paths\n> with LF or CRLF in them that have never worked would not work, but\n> at least does not break the input stream of whoever is reading the\n> error messages line by line.\n> \n> I dunno.\n> \n> \n"},{"id":"477289","messageId":"xmqqmt25a4uk.fsf@gitster.g","threadId":"58923","inReplyTo":"ec139b78-1d36-f894-e39f-f29877a67b18@gmail.com","subject":"Re: [PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-05-15T17:20:51Z","receivedAt":"2023-05-15T17:23:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 12/05/2023 17:57, Junio C Hamano wrote:\n>> Toon Claes writes:\n>> Stepping back a bit, how big a problem is this in real life? It\n>> certainly is possible to create a pathname with funny byte values in\n>> it, and in some environments,letters like single-quote that are\n>> considered cumbersome to handle by those who are used to CLI\n>> programs may be commonplace. But a path with newline? Or any\n>> control character for that matter? And this is not even the primary\n>> output from the program but is an error message for consumption by\n>> humans, no?\n>> I am wondering if it is simpler to just declare that the paths\n>> output in error messages have certain bytes, probably all control\n>> characters other than HT, replaced with a dot, and tell the users\n>> not to rely on the pathnames being intact if they contain funny\n>> bytes in them.\n>\n> We could only c-quote the name when it contains a control character\n> other that HT. That way names containing double quotes and backslashes\n> are unchanged but it will still be possible to parse the path from the\n> error message. If we're going to munge the name we might as well use\n> our standard quoting rather than some ad-hoc scheme.\n\nIn the above suggestion, I gave up and no longer aim to do\n\"quoting\". A more appropriate word for the approach is \"redacting\".\nThe message essentially is: If you use truly problematic bytes in\nyour path, they are redacted (so do not use them if it hurts).\n\nThis is because I am not sure how \"names containing dq and bs are\nunchanged\" can be done without ambiguity. If I see a message that\ncomes out of this:\n\n\tprintf(\"%s missing\\n\", obj_name);\n\nand it looks like\n\n\t\"a\\nb\" missing\n\nhow do I tell if it is complaining about the object the user named\nwith a three-byte string (i.e. lowercase-A, newline, lowercase-B),\nor a six-byte string (i.e. dq, lowercase-A, bs, lowercase-N,\nlowercase-B, dq)?\n\nIf we were forbidding '\"' to appear in a refname, then we could take\nadvantage of the fact that the name of an object inside a tree at a\nfunny path would not start with '\"', to disambiguate. For the\nthree- and six-byte string cases above, the formatting function will\ngive these messages (referred to as \"sample output\" below):\n\n\t\"master:a\\nb\" missing\n\tmaster:\"a\\nb\" missing\n\nbecause of your \"we do not exactly do our standard c-quote; we\nexempt dq and bs from the bytes to be quoted\" rule.\n\nBut it still feels a bit misleading. This codepath may have the\nwhole objectname as a single string so that c-quoting the entire\n\" \" inside a single c-quoted string that\nbegins with a dq is easy, but not all codepaths are lucky and some\nmay have to show and separately, concatenated with\n at the outermost output layer, which means that the second\none from the sample output may still mean the path with three-byte\nname in the tree of 'master' commit.\n\nAnd worse yet, because\n\n\tgit branch '\"master'\n\nis possible (even though nobody sane would do that), so \"treat the\nstring as c-quoted only if the object name as a whole begins with a\ndq\", this disambiguation idea would not work. The first one from\nthe sample output could be the blob at the path with a five-byte\nstring name (i.e. lowercase-A, bs, lowercase-N, lowercase-B, dq)\nin the tree of the commit at the tip of branch with seven-byte\nstring name (i.e. dq followed by 'master').\n\nSo, I dunno.\n"},{"id":"477956","messageId":"5b83dba0-e900-ebae-2ad8-f036a40a15c5@gmail.com","threadId":"58923","inReplyTo":"xmqqmt25a4uk.fsf@gitster.g","subject":"Re: [PATCH v5 1/1] cat-file: quote-format name in error when using -z","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-06-02T13:29:08Z","receivedAt":"2023-06-02T13:29:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Junio\n\nSorry for the slow reply, I had intended to reply before but got \ndistracted and forgot about it.\n\nOn 15/05/2023 18:20, Junio C Hamano wrote:\n> Phillip Wood writes:\n> \n>> On 12/05/2023 17:57, Junio C Hamano wrote:\n>>> Toon Claes writes:\n>>> Stepping back a bit, how big a problem is this in real life? It\n>>> certainly is possible to create a pathname with funny byte values in\n>>> it, and in some environments,letters like single-quote that are\n>>> considered cumbersome to handle by those who are used to CLI\n>>> programs may be commonplace. But a path with newline? Or any\n>>> control character for that matter? And this is not even the primary\n>>> output from the program but is an error message for consumption by\n>>> humans, no?\n>>> I am wondering if it is simpler to just declare that the paths\n>>> output in error messages have certain bytes, probably all control\n>>> characters other than HT, replaced with a dot, and tell the users\n>>> not to rely on the pathnames being intact if they contain funny\n>>> bytes in them.\n>>\n>> We could only c-quote the name when it contains a control character\n>> other that HT. That way names containing double quotes and backslashes\n>> are unchanged but it will still be possible to parse the path from the\n>> error message. If we're going to munge the name we might as well use\n>> our standard quoting rather than some ad-hoc scheme.\n> \n> In the above suggestion, I gave up and no longer aim to do\n> \"quoting\". A more appropriate word for the approach is \"redacting\".\n> The message essentially is: If you use truly problematic bytes in\n> your path, they are redacted (so do not use them if it hurts).\n> \n> This is because I am not sure how \"names containing dq and bs are\n> unchanged\" can be done without ambiguity.\n\nD'oh, I should have thought of that. You're right it ends up being \nambiguous. Anyway Patrick has just posted a patch to add NUL terminated \noutput which looks like a cleaner approach.\n\nBest Wishes\n\nPhillip\n\n> If I see a message that\n> comes out of this:\n> \n> \tprintf(\"%s missing\\n\", obj_name);\n> \n> and it looks like\n> \n> \t\"a\\nb\" missing\n> \n> how do I tell if it is complaining about the object the user named\n> with a three-byte string (i.e. lowercase-A, newline, lowercase-B),\n> or a six-byte string (i.e. dq, lowercase-A, bs, lowercase-N,\n> lowercase-B, dq)?\n> \n> If we were forbidding '\"' to appear in a refname, then we could take\n> advantage of the fact that the name of an object inside a tree at a\n> funny path would not start with '\"', to disambiguate. For the\n> three- and six-byte string cases above, the formatting function will\n> give these messages (referred to as \"sample output\" below):\n> \n> \t\"master:a\\nb\" missing\n> \tmaster:\"a\\nb\" missing\n> \n> because of your \"we do not exactly do our standard c-quote; we\n> exempt dq and bs from the bytes to be quoted\" rule.\n> \n> But it still feels a bit misleading. This codepath may have the\n> whole objectname as a single string so that c-quoting the entire\n> \" \" inside a single c-quoted string that\n> begins with a dq is easy, but not all codepaths are lucky and some\n> may have to show and separately, concatenated with\n> at the outermost output layer, which means that the second\n> one from the sample output may still mean the path with three-byte\n> name in the tree of 'master' commit.\n> \n> And worse yet, because\n> \n> \tgit branch '\"master'\n> \n> is possible (even though nobody sane would do that), so \"treat the\n> string as c-quoted only if the object name as a whole begins with a\n> dq\", this disambiguation idea would not work. The first one from\n> the sample output could be the blob at the path with a five-byte\n> string name (i.e. lowercase-A, bs, lowercase-N, lowercase-B, dq)\n> in the tree of the commit at the tip of branch with seven-byte\n> string name (i.e. dq followed by 'master').\n> \n> So, I dunno.\n\n"}]}