{"thread":{"id":"62546","subject":"[PATCH] fast-import: disallow \".\" and \"..\" path components","startedAt":"2024-11-25T17:58:43Z","lastAt":"2024-11-28T09:51:56Z","messageCount":14,"participants":["Elijah Newren via GitGitGadget","Eric Sunshine","Elijah Newren","Patrick Steinhardt","Kristoffer Haugsbakk","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"508106","messageId":"pull.1831.git.1732557520428.gitgitgadget@gmail.com","threadId":"62546","inReplyTo":null,"subject":"[PATCH] fast-import: disallow \".\" and \"..\" path components","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-25T17:58:40Z","receivedAt":"2024-11-25T17:58:43Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf a user specified e.g.\n M 100644 :1 ../some-file\nthen fast-import previously would happily create a git history where\nthere is a tree in the top-level directory named \"..\", and with a file\ninside that directory named \"some-file\". The top-level \"..\" directory\ncauses problems. While git checkout will die with errors and fsck will\nreport hasDotdot problems, the user is going to have problems trying to\nremove the problematic file. Simply avoid creating this bad history in\nthe first place.\n\nSigned-off-by: Elijah Newren \n---\n fast-import: disallow \".\" and \"..\" path components\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1831%2Fnewren%2Fdisallow-dotdot-fast-import-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1831/newren/disallow-dotdot-fast-import-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1831\n\n builtin/fast-import.c | 3 +++\n t/t9300-fast-import.sh | 20 ++++++++++++++++++++\n 2 files changed, 23 insertions(+)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 76d5c20f141..e0e0ca3657a 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1466,6 +1466,9 @@ static int tree_content_set(\n \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n \te = new_tree_entry();\n \te->name = to_atom(p, n);\n+\tif (!strcmp(e->name->str_dat, \".\") || !strcmp(e->name->str_dat, \"..\")) {\n+\t\tdie(\"path %s contains invalid component\", p);\n+\t}\n \te->versions[0].mode = 0;\n \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n \tt->entries[t->entry_count++] = e;\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 6224f54d4d2..caf3dc003a0 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -522,6 +522,26 @@ test_expect_success 'B: fail on invalid committer (5)' '\n \ttest_must_fail git fast-import input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata < wrote:\n> If a user specified e.g.\n> M 100644 :1 ../some-file\n> then fast-import previously would happily create a git history where\n> there is a tree in the top-level directory named \"..\", and with a file\n> inside that directory named \"some-file\". The top-level \"..\" directory\n> causes problems. While git checkout will die with errors and fsck will\n> report hasDotdot problems, the user is going to have problems trying to\n> remove the problematic file. Simply avoid creating this bad history in\n> the first place.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> @@ -1466,6 +1466,9 @@ static int tree_content_set(\n> e->name = to_atom(p, n);\n> + if (!strcmp(e->name->str_dat, \".\") || !strcmp(e->name->str_dat, \"..\")) {\n> + die(\"path %s contains invalid component\", p);\n> + }\n\nProbably not worth a reroll, but is_dot_or_dotdot() might be usable here.\n\n(And -- style nit -- the braces could be dropped.)\n"},{"id":"508108","messageId":"CABPp-BHCRmzb1LwwhZWwPc6Gs5BP-UMo6qgywcYz9pWppM06Jw@mail.gmail.com","threadId":"62546","inReplyTo":"CAPig+cReWbkomYYNAY_Q+6ezukSt8sZ+Q0MP=45DY4XeS3M==w@mail.gmail.com","subject":"Re: [PATCH] fast-import: disallow \".\" and \"..\" path components","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2024-11-25T18:24:09Z","receivedAt":"2024-11-25T18:24:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Nov 25, 2024 at 10:15 AM Eric Sunshine wrote:\n>\n> On Mon, Nov 25, 2024 at 12:58 PM Elijah Newren via GitGitGadget\n> wrote:\n> > If a user specified e.g.\n> > M 100644 :1 ../some-file\n> > then fast-import previously would happily create a git history where\n> > there is a tree in the top-level directory named \"..\", and with a file\n> > inside that directory named \"some-file\". The top-level \"..\" directory\n> > causes problems. While git checkout will die with errors and fsck will\n> > report hasDotdot problems, the user is going to have problems trying to\n> > remove the problematic file. Simply avoid creating this bad history in\n> > the first place.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> > @@ -1466,6 +1466,9 @@ static int tree_content_set(\n> > e->name = to_atom(p, n);\n> > + if (!strcmp(e->name->str_dat, \".\") || !strcmp(e->name->str_dat, \"..\")) {\n> > + die(\"path %s contains invalid component\", p);\n> > + }\n>\n> Probably not worth a reroll, but is_dot_or_dotdot() might be usable here.\n>\n> (And -- style nit -- the braces could be dropped.)\n\nGood catches, thanks. I think they are worth a reroll; I'll send one in.\n"},{"id":"508109","messageId":"pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com","threadId":"62546","inReplyTo":"pull.1831.git.1732557520428.gitgitgadget@gmail.com","subject":"[PATCH v2] fast-import: disallow \".\" and \"..\" path components","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2024-11-25T19:00:48Z","receivedAt":"2024-11-25T19:00:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf a user specified e.g.\n M 100644 :1 ../some-file\nthen fast-import previously would happily create a git history where\nthere is a tree in the top-level directory named \"..\", and with a file\ninside that directory named \"some-file\". The top-level \"..\" directory\ncauses problems. While git checkout will die with errors and fsck will\nreport hasDotdot problems, the user is going to have problems trying to\nremove the problematic file. Simply avoid creating this bad history in\nthe first place.\n\nSigned-off-by: Elijah Newren \n---\n fast-import: disallow \".\" and \"..\" path components\n \n Changes since v1:\n \n * make use of is_dot_or_dotdot() from dir.h\n * fix style issue\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1831%2Fnewren%2Fdisallow-dotdot-fast-import-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1831/newren/disallow-dotdot-fast-import-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1831\n\nRange-diff vs v1:\n\n 1: 86ea3df0351 ! 1: 447b6794a4a fast-import: disallow \".\" and \"..\" path components\n @@ builtin/fast-import.c: static int tree_content_set(\n \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n \te = new_tree_entry();\n \te->name = to_atom(p, n);\n -+\tif (!strcmp(e->name->str_dat, \".\") || !strcmp(e->name->str_dat, \"..\")) {\n ++\tif (is_dot_or_dotdot(e->name->str_dat))\n +\t\tdie(\"path %s contains invalid component\", p);\n -+\t}\n \te->versions[0].mode = 0;\n \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n \tt->entries[t->entry_count++] = e;\n\n\n builtin/fast-import.c | 2 ++\n t/t9300-fast-import.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 76d5c20f141..995ef76f9d6 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -1466,6 +1466,8 @@ static int tree_content_set(\n \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n \te = new_tree_entry();\n \te->name = to_atom(p, n);\n+\tif (is_dot_or_dotdot(e->name->str_dat))\n+\t\tdie(\"path %s contains invalid component\", p);\n \te->versions[0].mode = 0;\n \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n \tt->entries[t->entry_count++] = e;\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 6224f54d4d2..caf3dc003a0 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -522,6 +522,26 @@ test_expect_success 'B: fail on invalid committer (5)' '\n \ttest_must_fail git fast-import input <<-INPUT_END &&\n+\tblob\n+\tmark :1\n+\tdata < $GIT_COMMITTER_DATE\n+\tdata < From: Elijah Newren \n> \n> If a user specified e.g.\n> M 100644 :1 ../some-file\n> then fast-import previously would happily create a git history where\n> there is a tree in the top-level directory named \"..\", and with a file\n> inside that directory named \"some-file\". The top-level \"..\" directory\n> causes problems. While git checkout will die with errors and fsck will\n> report hasDotdot problems, the user is going to have problems trying to\n> remove the problematic file. Simply avoid creating this bad history in\n> the first place.\n\nMakes sense.\n\nMore generally this made me wonder whether we should maybe extract some\nbits out of \"fsck.c\" so that we don't have to duplicate the checks done\nthere in git-fast-import(1). This would for example include checks for\n\".git\" and its HFS/NTFS variants as well as tree entry length checks for\nnames longer than 4096 characters.\n\nThis of course does not have to be part of your patch, which looks good\nto me.\n\nThanks!\n\nPatrick\n"},{"id":"508215","messageId":"79f6027b-123c-40a6-975b-1c6053b9391c@app.fastmail.com","threadId":"62546","inReplyTo":"pull.1831.v2.git.1732561248717.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: disallow \".\" and \"..\" path components","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-11-27T08:28:03Z","receivedAt":"2024-11-27T08:30:24Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"Hi. I see that this is in `next` now so the following might\nbe irrelevant.\n\nOn Mon, Nov 25, 2024, at 20:00, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> [...]\n> diff --git a/builtin/fast-import.c b/builtin/fast-import.c\n> index 76d5c20f141..995ef76f9d6 100644\n> --- a/builtin/fast-import.c\n> +++ b/builtin/fast-import.c\n> @@ -1466,6 +1466,8 @@ static int tree_content_set(\n> \t\troot->tree = t = grow_tree_content(t, t->entry_count);\n> \te = new_tree_entry();\n> \te->name = to_atom(p, n);\n> +\tif (is_dot_or_dotdot(e->name->str_dat))\n> +\t\tdie(\"path %s contains invalid component\", p);\n\nNit: single-quoting the path seems more common:\n\n $ git grep \"\\\"path '%s'\" ':!po/' | wc -l\n 17\n $ git grep \"\\\"path %s\" ':!po/' | wc -l\n 4\n\n> \te->versions[0].mode = 0;\n> \toidclr(&e->versions[0].oid, the_repository->hash_algo);\n> \tt->entries[t->entry_count++] = e;\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 6224f54d4d2..caf3dc003a0 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -522,6 +522,26 @@ test_expect_success 'B: fail on invalid committer (5)' '\n> \ttest_must_fail git fast-import '\n>\n> +test_expect_success 'B: fail on invalid file path' '\n> +\tcat >input <<-INPUT_END &&\n> +\tblob\n> +\tmark :1\n> +\tdata < +\tFile contents\n> +\tEOF\n> +\n> +\tcommit refs/heads/badpath\n> +\tcommitter Name $GIT_COMMITTER_DATE\n> +\tdata < +\tCommit Message\n> +\tCOMMIT\n> +\tM 100644 :1 ../invalid-path\n\nMaybe the test could be parameterized so that both `..` and `.` can\nbe tested? Like in `test_path_eol_success`.\n\n-- \nKristoffer Haugsbakk\n\n"},{"id":"508225","messageId":"xmqq1pywvmhq.fsf_-_@gitster.g","threadId":"62546","inReplyTo":"79f6027b-123c-40a6-975b-1c6053b9391c@app.fastmail.com","subject":"Re* [PATCH v2] fast-import: disallow \".\" and \"..\" path components","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-27T13:23:45Z","receivedAt":"2024-11-27T13:23:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" writes:\n\n>> +\tif (is_dot_or_dotdot(e->name->str_dat))\n>> +\t\tdie(\"path %s contains invalid component\", p);\n>\n> Nit: single-quoting the path seems more common:\n>\n> $ git grep \"\\\"path '%s'\" ':!po/' | wc -l\n> 17\n> $ git grep \"\\\"path %s\" ':!po/' | wc -l\n> 4\n\nAh, I missed that one. Thanks for catching.\n\nWe probably should write it down.\n\n--- >8 ---\n[PATCH] CodingGuidelines: a handful of error message guidelines\n\nIt is more efficient to have something in the coding guidelines\ndocument to point at, when we want to review and comment on a new\nmessage in the codebase to make sure it \"fits\" in the set of\nexisting messages.\n\nLet's write down established best practice we are aware of.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I am writing what I think is the established practice from\n memory; clarifications, corrections, and additions are all\n welcome.\n\n Documentation/CodingGuidelines | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\nindex 87904791cb..0444391983 100644\n--- c/Documentation/CodingGuidelines\n+++ w/Documentation/CodingGuidelines\n@@ -703,16 +703,22 @@ Program Output\n \n Error Messages\n \n- - Do not end error messages with a full stop.\n+ - Do not end a single-sentence error message with a full stop.\n \n - Do not capitalize the first word, only because it is the first word\n- in the message (\"unable to open %s\", not \"Unable to open %s\"). But\n+ in the message (\"unable to open '%s'\", not \"Unable to open '%s'\"). But\n \"SHA-3 not supported\" is fine, because the reason the first word is\n capitalized is not because it is at the beginning of the sentence,\n but because the word would be spelled in capital letters even when\n it appeared in the middle of the sentence.\n \n- - Say what the error is first (\"cannot open %s\", not \"%s: cannot open\")\n+ - Say what the error is first (\"cannot open '%s'\", not \"%s: cannot open\").\n+\n+ - Enclose the subject of an error inside a pair of single quotes,\n+ e.g. `die(_(\"unable to open '%s'\"), path)`.\n+\n+ - Unless there is a compelling reason not to, error messages should\n+ be marked for `_(\"translation\")`.\n \n \n Externally Visible Names\n"},{"id":"508229","messageId":"20241127142458.GA1712@coredump.intra.peff.net","threadId":"62546","inReplyTo":"Z0Vxb4OVJYeAdgds@pks.im","subject":"Re: [PATCH v2] fast-import: disallow \".\" and \"..\" path components","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-27T14:24:58Z","receivedAt":"2024-11-27T14:25:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 26, 2024 at 07:57:57AM +0100, Patrick Steinhardt wrote:\n\n> On Mon, Nov 25, 2024 at 07:00:48PM +0000, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> > \n> > If a user specified e.g.\n> > M 100644 :1 ../some-file\n> > then fast-import previously would happily create a git history where\n> > there is a tree in the top-level directory named \"..\", and with a file\n> > inside that directory named \"some-file\". The top-level \"..\" directory\n> > causes problems. While git checkout will die with errors and fsck will\n> > report hasDotdot problems, the user is going to have problems trying to\n> > remove the problematic file. Simply avoid creating this bad history in\n> > the first place.\n> \n> Makes sense.\n> \n> More generally this made me wonder whether we should maybe extract some\n> bits out of \"fsck.c\" so that we don't have to duplicate the checks done\n> there in git-fast-import(1). This would for example include checks for\n> \".git\" and its HFS/NTFS variants as well as tree entry length checks for\n> names longer than 4096 characters.\n\nI had the same thought, but I think the right code to be using is\nverify_path(). That's what ultimately is used to let names into the\nindex from trees, from update-index, or from other tools like git-apply.\n\nSo I'd consider that authoritative, and fsck is mostly trying to follow\nthose rules while looking at only a single tree at a time. But\nfast-import should have the whole path as a string, just like the index\ncode does).\n\n-Peff\n"},{"id":"508243","messageId":"CAPig+cR+-QCjoobdaMf_t5AJGxxXqbCkn9UeiPMQFvAt2T6kxQ@mail.gmail.com","threadId":"62546","inReplyTo":"xmqq1pywvmhq.fsf_-_@gitster.g","subject":"Re: Re* [PATCH v2] fast-import: disallow \".\" and \"..\" path components","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-11-27T19:41:53Z","receivedAt":"2024-11-27T19:42:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 27, 2024 at 8:23 AM Junio C Hamano wrote:\n> [PATCH] CodingGuidelines: a handful of error message guidelines\n>\n> It is more efficient to have something in the coding guidelines\n> document to point at, when we want to review and comment on a new\n> message in the codebase to make sure it \"fits\" in the set of\n> existing messages.\n>\n> Let's write down established best practice we are aware of.\n>\n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git c/Documentation/CodingGuidelines w/Documentation/CodingGuidelines\n> @@ -703,16 +703,22 @@ Program Output\n> Error Messages\n>\n> - - Do not end error messages with a full stop.\n> + - Do not end a single-sentence error message with a full stop.\n>\n> - Do not capitalize the first word, only because it is the first word\n> - in the message (\"unable to open %s\", not \"Unable to open %s\"). But\n> + in the message (\"unable to open '%s'\", not \"Unable to open '%s'\"). But\n> \"SHA-3 not supported\" is fine, because the reason the first word is\n> capitalized is not because it is at the beginning of the sentence,\n> but because the word would be spelled in capital letters even when\n> it appeared in the middle of the sentence.\n>\n> - - Say what the error is first (\"cannot open %s\", not \"%s: cannot open\")\n> + - Say what the error is first (\"cannot open '%s'\", not \"%s: cannot open\").\n> +\n> + - Enclose the subject of an error inside a pair of single quotes,\n> + e.g. `die(_(\"unable to open '%s'\"), path)`.\n\nThese changes all seem fine.\n\n> + - Unless there is a compelling reason not to, error messages should\n> + be marked for `_(\"translation\")`.\n\nWe might want to spell this out more fully, such as stating that\nmessages from porcelain commands should be marked for translation, but\nmessages in plumbing should not. Also, perhaps mention explicitly that\nBUG(\"message\") should not be marked for translation since they are\nintended to be read by Git developers, not by end-users.\n"},{"id":"508246","messageId":"xmqqr06ws2bu.fsf@gitster.g","threadId":"62546","inReplyTo":"20241127142458.GA1712@coredump.intra.peff.net","subject":"Re: [PATCH v2] fast-import: disallow \".\" and \"..\" path components","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-27T23:07:33Z","receivedAt":"2024-11-27T23:07:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I had the same thought, but I think the right code to be using is\n> verify_path(). That's what ultimately is used to let names into the\n> index from trees, from update-index, or from other tools like git-apply.\n\nYeah, I agree that is the right helper to use.\n"},{"id":"508256","messageId":"xmqqa5dkqjmr.fsf_-_@gitster.g","threadId":"62546","inReplyTo":"xmqq1pywvmhq.fsf_-_@gitster.g","subject":"[PATCH v2] CodingGuidelines: a handful of error message guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-28T00:36:44Z","receivedAt":"2024-11-28T00:36:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Taking input from comments by Eric (thanks) on the previous round,\nthis iteration adds a bit more about Porcelain/Plumbing and BUG().\n\n diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n index 71e4742fd5..2b8f99f333 100644\n --- a/Documentation/CodingGuidelines\n +++ b/Documentation/CodingGuidelines\n @@ -703,8 +703,15 @@ Error Messages\n - Enclose the subject of an error inside a pair of single quotes,\n e.g. `die(_(\"unable to open '%s'\"), path)`.\n \n - - Unless there is a compelling reason not to, error messages should\n - be marked for `_(\"translation\")`.\n + - Unless there is a compelling reason not to, error messages from the\n + Porcelain command should be marked for `_(\"translation\")`.\n +\n + - Error messages from the plumbing commands are sometimes meant for\n + machine consumption and should not be marked for `_(\"translation\")`\n + to keep them 'grep'-able.\n +\n + - BUG(\"message\") are for communicating the specific error to\n + developers, and not to be translated.\n \n \n Externally Visible Names\n\n--- >8 ---\nIt is more efficient to have something in the coding guidelines\ndocument to point at, when we want to review and comment on a new\nmessage in the codebase to make sure it \"fits\" in the set of\nexisting messages.\n\nLet's write down established best practice we are aware of.\n\nHelped-by: Eric Sunshine \nSigned-off-by: Junio C Hamano \n---\n Documentation/CodingGuidelines | 19 ++++++++++++++++---\n 1 file changed, 16 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\nindex 3263245b03..2b8f99f333 100644\n--- a/Documentation/CodingGuidelines\n+++ b/Documentation/CodingGuidelines\n@@ -689,16 +689,29 @@ Program Output\n \n Error Messages\n \n- - Do not end error messages with a full stop.\n+ - Do not end a single-sentence error message with a full stop.\n \n - Do not capitalize the first word, only because it is the first word\n- in the message (\"unable to open %s\", not \"Unable to open %s\"). But\n+ in the message (\"unable to open '%s'\", not \"Unable to open '%s'\"). But\n \"SHA-3 not supported\" is fine, because the reason the first word is\n capitalized is not because it is at the beginning of the sentence,\n but because the word would be spelled in capital letters even when\n it appeared in the middle of the sentence.\n \n- - Say what the error is first (\"cannot open %s\", not \"%s: cannot open\")\n+ - Say what the error is first (\"cannot open '%s'\", not \"%s: cannot open\").\n+\n+ - Enclose the subject of an error inside a pair of single quotes,\n+ e.g. `die(_(\"unable to open '%s'\"), path)`.\n+\n+ - Unless there is a compelling reason not to, error messages from the\n+ Porcelain command should be marked for `_(\"translation\")`.\n+\n+ - Error messages from the plumbing commands are sometimes meant for\n+ machine consumption and should not be marked for `_(\"translation\")`\n+ to keep them 'grep'-able.\n+\n+ - BUG(\"message\") are for communicating the specific error to\n+ developers, and not to be translated.\n \n \n Externally Visible Names\n\n-- \n2.47.1-499-g8536fed62d\n\n\n"},{"id":"508265","messageId":"CAPig+cSXGreymhni_J8TZ56rp+VZB_Z7ekaRtwFdxE3a2QWS5w@mail.gmail.com","threadId":"62546","inReplyTo":"xmqqa5dkqjmr.fsf_-_@gitster.g","subject":"Re: [PATCH v2] CodingGuidelines: a handful of error message guidelines","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-11-28T07:57:03Z","receivedAt":"2024-11-28T07:57:15Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Nov 27, 2024 at 7:36 PM Junio C Hamano wrote:\n> It is more efficient to have something in the coding guidelines\n> document to point at, when we want to review and comment on a new\n> message in the codebase to make sure it \"fits\" in the set of\n> existing messages.\n>\n> Let's write down established best practice we are aware of.\n>\n> Helped-by: Eric Sunshine \n> Signed-off-by: Junio C Hamano \n> ---\n> diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines\n> @@ -689,16 +689,29 @@ Program Output\n> Error Messages\n>\n> - - Say what the error is first (\"cannot open %s\", not \"%s: cannot open\")\n> + - Say what the error is first (\"cannot open '%s'\", not \"%s: cannot open\").\n> +\n> + - Enclose the subject of an error inside a pair of single quotes,\n> + e.g. `die(_(\"unable to open '%s'\"), path)`.\n> +\n> + - Unless there is a compelling reason not to, error messages from the\n> + Porcelain command should be marked for `_(\"translation\")`.\n\nHere you capitalize \"Porcelain\" but below, \"plumbing\" is all lowercase.\n\n> + - Error messages from the plumbing commands are sometimes meant for\n> + machine consumption and should not be marked for `_(\"translation\")`\n> + to keep them 'grep'-able.\n\nUsing the same example, `_(\"translation\")`, for both the \"should be\"\nand \"should not be\" cases may very well confuse readers. (It certainly\nconfused me.) Perhaps mirroring the example of an item earlier in the\nlist would be clearer:\n\n - Unless there is a compelling reason not to, error messages from\n porcelain commands should be marked for translation, e.g.\n `die(_(\"bad revision\"))`\n\n - Error messages from plumbing commands are sometimes meant for\n machine consumption, thus should not be marked for translation,\n e.g. `die(\"bad revision\")`\n\n> + - BUG(\"message\") are for communicating the specific error to\n> + developers, and not to be translated.\n\nOkay, although could be slightly more explicit:\n\n - BUG(\"message\") is for communicating a specific failure to\n developers, not end-users, thus should not be translated.\n"},{"id":"508267","messageId":"xmqq34jbogg3.fsf@gitster.g","threadId":"62546","inReplyTo":"CAPig+cSXGreymhni_J8TZ56rp+VZB_Z7ekaRtwFdxE3a2QWS5w@mail.gmail.com","subject":"Re: [PATCH v2] CodingGuidelines: a handful of error message guidelines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-28T09:28:28Z","receivedAt":"2024-11-28T09:28:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> + - Unless there is a compelling reason not to, error messages from the\n>> + Porcelain command should be marked for `_(\"translation\")`.\n>\n> Here you capitalize \"Porcelain\" but below, \"plumbing\" is all lowercase.\n\n;-) I think that is how we spell them in our documentation when we\ncontrast them against each other.\n\n>> + - Error messages from the plumbing commands are sometimes meant for\n>> + machine consumption and should not be marked for `_(\"translation\")`\n>> + to keep them 'grep'-able.\n>\n> Using the same example, `_(\"translation\")`, for both the \"should be\"\n> and \"should not be\" cases may very well confuse readers. (It certainly\n> confused me.) Perhaps mirroring the example of an item earlier in the\n> list would be clearer:\n>\n> - Unless there is a compelling reason not to, error messages from\n> porcelain commands should be marked for translation, e.g.\n> `die(_(\"bad revision\"))`\n>\n> - Error messages from plumbing commands are sometimes meant for\n> machine consumption, thus should not be marked for translation,\n> e.g. `die(\"bad revision\")`\n\nThanks, that is much better. Let me steal it verbatim in the\nhopefully final reroll.\n\n>> + - BUG(\"message\") are for communicating the specific error to\n>> + developers, and not to be translated.\n>\n> Okay, although could be slightly more explicit:\n>\n> - BUG(\"message\") is for communicating a specific failure to\n> developers, not end-users, thus should not be translated.\n\nThe way I read your rewrite is that the \"communitation\" mentioned is\nbetween the program and the user who saw the message. I wanted to\nsay that the message is seen first by an end-user, and then is\ncommunicated to developers. And not translating is one way to make\nsure the message is not mangled, and stays grep-able, during the\ngame of telephone.\n\nWould this work better?\n\n - In order to help the user who saw BUG(\"message\") to accurately\n communicate it to developers, do not mark them for translation.\n\nThanks.\n"},{"id":"508268","messageId":"CAPig+cS3KdUKms7UkJQkkMD5WQnD6Khauq4RwttetzrJ34VCdg@mail.gmail.com","threadId":"62546","inReplyTo":"xmqq34jbogg3.fsf@gitster.g","subject":"Re: [PATCH v2] CodingGuidelines: a handful of error message guidelines","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2024-11-28T09:51:44Z","receivedAt":"2024-11-28T09:51:56Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Nov 28, 2024 at 4:28 AM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> >> + Porcelain command should be marked for `_(\"translation\")`.\n> >\n> > Here you capitalize \"Porcelain\" but below, \"plumbing\" is all lowercase.\n>\n> ;-) I think that is how we spell them in our documentation when we\n> contrast them against each other.\n\nI must not have been paying close enough attention.\n\n> >> + - BUG(\"message\") are for communicating the specific error to\n> >> + developers, and not to be translated.\n> >\n> > Okay, although could be slightly more explicit:\n> >\n> > - BUG(\"message\") is for communicating a specific failure to\n> > developers, not end-users, thus should not be translated.\n>\n> The way I read your rewrite is that the \"communitation\" mentioned is\n> between the program and the user who saw the message. I wanted to\n> say that the message is seen first by an end-user, and then is\n> communicated to developers. And not translating is one way to make\n> sure the message is not mangled, and stays grep-able, during the\n> game of telephone.\n>\n> Would this work better?\n>\n> - In order to help the user who saw BUG(\"message\") to accurately\n> communicate it to developers, do not mark them for translation.\n\nLet's not spend too much time fine-tuning this. I found your original\nclearer than this rewrite. It was just the \"and not to be\" bit that\nmade my reading hiccup. Taking your original but substituting in\n\"thus\" may help:\n\n - BUG(\"message\") are for communicating the specific error to\n developers, thus should not be translated.\n"}]}