{"thread":{"id":"59447","subject":"[PATCH 3/3] advice: translate all actions in error_resolve_conflict()","startedAt":"2023-03-23T16:47:20Z","lastAt":"2023-08-07T20:21:04Z","messageCount":21,"participants":["Oswald Buddenhagen","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"473997","messageId":"20230323162234.995485-3-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":"20230323162234.995485-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 3/3] advice: translate all actions in error_resolve_conflict()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:34Z","receivedAt":"2023-03-23T16:47:20Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"action_name() returns a N_()'d string (for good reasons), so we still\nneed to _() it.\n\nIn practice, this affects 'rebase'.\n\nWhether this is actually useful is debatable ...\n\nSigned-off-by: Oswald Buddenhagen \n---\n advice.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/advice.c b/advice.c\nindex d6232439c3..f75f3df582 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -192,7 +192,7 @@ int error_resolve_conflict(const char *me)\n \t\terror(_(\"Reverting is not possible because you have unmerged files.\"));\n \telse\n \t\terror(_(\"It is not possible to %s because you have unmerged files.\"),\n-\t\t\tme);\n+\t\t\t_(me));\n \n \tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n \t\t/*\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"473999","messageId":"20230323162234.995485-1-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":null,"subject":"[PATCH 1/3] Capitalization and punctuation fixes to some user visible messages","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:32Z","receivedAt":"2023-03-23T16:47:23Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"These messages are used in multi-line context, where not sticking to\nproper grammar makes things hard to read and has questionable looks.\nTherefore, these changes go against the usual rules for error messages.\n\nSigned-off-by: Oswald Buddenhagen \n---\n builtin/pull.c | 2 +-\n sequencer.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 56f679d94a..bb2ddc93ab 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1044,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (!opt_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n-\t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n+\t\t\t\t_(\"Please commit or stash them.\"), 1, 0);\n \n \t\tif (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n \t\t\toidclr(&rebase_fork_point);\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..fda68cd33d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3629,13 +3629,13 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\t\t \"\\n\"),\n \t\t\tcommand_line,\n \t\t\tdirty ? N_(\"and made changes to the index and/or the \"\n-\t\t\t\t\"working tree\\n\") : \"\");\n+\t\t\t\t\"working tree.\\n\") : \"\");\n \t\tif (status == 127)\n \t\t\t/* command not found */\n \t\t\tstatus = 1;\n \t} else if (dirty) {\n \t\twarning(_(\"execution succeeded: %s\\nbut \"\n-\t\t\t \"left changes to the index and/or the working tree\\n\"\n+\t\t\t \"left changes to the index and/or the working tree.\\n\"\n \t\t\t \"Commit or stash your changes, and then run\\n\"\n \t\t\t \"\\n\"\n \t\t\t \" git rebase --continue\\n\"\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"473998","messageId":"20230323162234.995485-2-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":"20230323162234.995485-1-oswald.buddenhagen@gmx.de","subject":"[PATCH 2/3] sequencer: actually translate report in do_exec()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T16:22:33Z","receivedAt":"2023-03-23T16:47:24Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"N_() is meant to be used on strings that are subsequently _()'d, which\nisn't the case here.\n\nSigned-off-by: Oswald Buddenhagen \n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fda68cd33d..21748bbfb0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3628,7 +3628,7 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\t\t \" git rebase --continue\\n\"\n \t\t\t \"\\n\"),\n \t\t\tcommand_line,\n-\t\t\tdirty ? N_(\"and made changes to the index and/or the \"\n+\t\t\tdirty ? _(\"and made changes to the index and/or the \"\n \t\t\t\t\"working tree.\\n\") : \"\");\n \t\tif (status == 127)\n \t\t\t/* command not found */\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"474041","messageId":"xmqqttyb6w3j.fsf@gitster.g","threadId":"59447","inReplyTo":"20230323162234.995485-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 1/3] Capitalization and punctuation fixes to some user visible messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T20:39:28Z","receivedAt":"2023-03-23T20:39:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> \t\t\trequire_clean_work_tree(the_repository,\n> \t\t\t\tN_(\"pull with rebase\"),\n> -\t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n> +\t\t\t\t_(\"Please commit or stash them.\"), 1, 0);\n\nThe require_clean_work_tree() function uses the message this patch\ntouches here like so:\n\n\t...\n\tif (err) {\n\t\tif (hint)\n\t\t\terror(\"%s\", hint);\n\t\tif (!gently)\n\t\t\texit(128);\n\t}\n\nand the message given to error() is shown with \"error: \" prefix.\nOur friendly CodingGuidelines is fairly clear on what to do to a\nsingle sentence error message like this one:\n\n| Error Messages\n| \n| - Do not end error messages 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| \"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\nSo, if you want to touch this message, what you want to do is to\nleave the capitalization alone, but to drop the full stop at the\nend.\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 3be23d7ca2..fda68cd33d 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3629,13 +3629,13 @@ static int do_exec(struct repository *r, const char *command_line)\n> \t\t\t \"\\n\"),\n> \t\t\tcommand_line,\n> \t\t\tdirty ? N_(\"and made changes to the index and/or the \"\n> -\t\t\t\t\"working tree\\n\") : \"\");\n> +\t\t\t\t\"working tree.\\n\") : \"\");\n\nI think this long and multi-sentence warning message should be split\ninto two and the latter half should become an advice message that\nthe user can squelch. Until it happens, which would require us to\nrewrite these messages, I wouldn't bother touching this message if I\nwere you.\n\n> \t} else if (dirty) {\n> \t\twarning(_(\"execution succeeded: %s\\nbut \"\n> -\t\t\t \"left changes to the index and/or the working tree\\n\"\n> +\t\t\t \"left changes to the index and/or the working tree.\\n\"\n> \t\t\t \"Commit or stash your changes, and then run\\n\"\n> \t\t\t \"\\n\"\n> \t\t\t \" git rebase --continue\\n\"\n\nLikewise.\n"},{"id":"474043","messageId":"xmqqpm8z6vxk.fsf@gitster.g","threadId":"59447","inReplyTo":"20230323162234.995485-2-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 2/3] sequencer: actually translate report in do_exec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T20:43:03Z","receivedAt":"2023-03-23T20:43:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> N_() is meant to be used on strings that are subsequently _()'d, which\n> isn't the case here.\n\nCorrect. But the original does not look particularly a good way to\nallow people to translate the messages (namely, it splits a sentence\nin the middle and makes a sentence lego).\n\nI wonder if it should prepare two full sentence messages, one for\ndirty case and the other for non-dirty case? In any case, I think\nthe latter half of this big warning() should be done as an advice\nmessage that the user can squelch, so I wouldn't worry too much\nabout it before that happens.\n\nThanks.\n\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index fda68cd33d..21748bbfb0 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3628,7 +3628,7 @@ static int do_exec(struct repository *r, const char *command_line)\n> \t\t\t \" git rebase --continue\\n\"\n> \t\t\t \"\\n\"),\n> \t\t\tcommand_line,\n> -\t\t\tdirty ? N_(\"and made changes to the index and/or the \"\n> +\t\t\tdirty ? _(\"and made changes to the index and/or the \"\n> \t\t\t\t\"working tree.\\n\") : \"\");\n> \t\tif (status == 127)\n> \t\t\t/* command not found */\n"},{"id":"474048","messageId":"ZBzAVMCOadgE1sa8@ugly","threadId":"59447","inReplyTo":"xmqqttyb6w3j.fsf@gitster.g","subject":"Re: [PATCH 1/3] Capitalization and punctuation fixes to some user visible messages","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T21:10:44Z","receivedAt":"2023-03-23T21:11:31Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 01:39:28PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>\n>> \t\t\trequire_clean_work_tree(the_repository,\n>> \t\t\t\tN_(\"pull with rebase\"),\n>> -\t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n>> +\t\t\t\t_(\"Please commit or stash them.\"), 1, 0);\n>\n>Our friendly CodingGuidelines is fairly clear on what to do to a\n>single sentence error message like this one: [...]\n>\ni know. that's why the commit message makes a point of stating that it's \nmaking an exception.\n\nthe reasoning is basically that it's not *really* an error message, it's \njust abusing the machinery. you obviously noticed that yourself, \nsuggesting to turn these strings into advice messages. but that kinda \nfeels like overkill to me.\n\n"},{"id":"474050","messageId":"ZBzCoZ0MuysnVj9t@ugly","threadId":"59447","inReplyTo":"xmqqpm8z6vxk.fsf@gitster.g","subject":"Re: [PATCH 2/3] sequencer: actually translate report in do_exec()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T21:20:33Z","receivedAt":"2023-03-23T21:20:42Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 01:43:03PM -0700, Junio C Hamano wrote:\n>But the original does not look particularly a good way to\n>allow people to translate the messages (namely, it splits a sentence\n>in the middle and makes a sentence lego).\n>\nit's not that bad, because the \"brick\" is inserted at a location that is \nspecified by another translatable string.\n\n>I wonder if it should prepare two full sentence messages, one for\n>dirty case and the other for non-dirty case?\n>\n... so this seems unnecessary if no translator complained yet.\n\n>In any case, I think\n>the latter half of this big warning() should be done as an advice\n>message that the user can squelch, so I wouldn't worry too much\n>about it before that happens.\n>\nyeah, but unless this is on someone's shortlist, i don't think it makes \nsense to hold up my fix.\n\n"},{"id":"474052","messageId":"xmqqcz4z6tzj.fsf@gitster.g","threadId":"59447","inReplyTo":"ZBzCoZ0MuysnVj9t@ugly","subject":"Re: [PATCH 2/3] sequencer: actually translate report in do_exec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-23T21:25:04Z","receivedAt":"2023-03-23T21:25:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> yeah, but unless this is on someone's shortlist, i don't think it\n> makes sense to hold up my fix.\n\nMy point was that your \"fix\" is not really a \"fix\" that is worth the\ncode churn. Or did I mis-read your patches?\n\nThanks.\n"},{"id":"474055","messageId":"ZBzKauthEiqNS0nc@ugly","threadId":"59447","inReplyTo":"xmqqcz4z6tzj.fsf@gitster.g","subject":"Re: [PATCH 2/3] sequencer: actually translate report in do_exec()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-03-23T21:53:46Z","receivedAt":"2023-03-23T21:53:53Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Thu, Mar 23, 2023 at 02:25:04PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>\n>> yeah, but unless this is on someone's shortlist, i don't think it\n>> makes sense to hold up my fix.\n>\n>My point was that your \"fix\" is not really a \"fix\" that is worth the\n>code churn. Or did I mis-read your patches?\n>\nthey all fix actual (minor) bugs: missing i18n, bad grammar (the missing \nperiods between sentences actaully threw me off, which is how i got \nstarted on the series at all), and arguably bad capitalization (though \nobviously, for my own sake i couldn't care less about *that*).\n"},{"id":"474088","messageId":"xmqqcz4y5huz.fsf@gitster.g","threadId":"59447","inReplyTo":"20230323162234.995485-3-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH 3/3] advice: translate all actions in error_resolve_conflict()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-24T14:44:36Z","receivedAt":"2023-03-24T14:44:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> action_name() returns a N_()'d string (for good reasons), so we still\n> need to _() it.\n>\n> In practice, this affects 'rebase'.\n>\n> Whether this is actually useful is debatable ...\n\nYes, it is debatable. It may be much better to add a new \"else if\"\nthat covers the case we _know_ is not covered with the current code.\n\nHaving 'me' in _() would be consistent with its source marked with\nN_() as you found out, but I agree with you that it is debatable if\nthis patch is moving things in the right direction. It would belong\nto the part that should never be exercised once we give action names\nproper covering by adding missing \"else if\".\n\nWe could even replace the fallback else with a \"else BUG()\" to ensure\nthat the action names all callers pass have corresponding message\nthat can be translated without sentence lego.\n\nThanks.\n\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> advice.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/advice.c b/advice.c\n> index d6232439c3..f75f3df582 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -192,7 +192,7 @@ int error_resolve_conflict(const char *me)\n> \t\terror(_(\"Reverting is not possible because you have unmerged files.\"));\n> \telse\n> \t\terror(_(\"It is not possible to %s because you have unmerged files.\"),\n> -\t\t\tme);\n> +\t\t\t_(me));\n> \n> \tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n> \t\t/*\n"},{"id":"476269","messageId":"20230428125649.1719796-2-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":"20230428125649.1719796-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2 2/3] sequencer: actually translate report in do_exec()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-28T12:56:48Z","receivedAt":"2023-04-28T12:56:54Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"N_() is meant to be used on strings that are subsequently _()'d, which\nisn't the case here.\n\nThe affected construct is a bit questionable from an i18n perspective,\nas it pieces together a sentence from separate strings. However, it\ndoesn't appear to be that bad, as the \"assembly instructions\" are in a\ntranslatable message as well. Lacking specific complaints from\ntranslators, it doesn't seem worth changing this.\n\nSigned-off-by: Oswald Buddenhagen \nCc: Junio C Hamano \n\n---\nv2:\n- mention the word puzzle in the commit message\n---\n sequencer.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3be23d7ca2..0677c9ab09 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3628,7 +3628,7 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\t\t \" git rebase --continue\\n\"\n \t\t\t \"\\n\"),\n \t\t\tcommand_line,\n-\t\t\tdirty ? N_(\"and made changes to the index and/or the \"\n+\t\t\tdirty ? _(\"and made changes to the index and/or the \"\n \t\t\t\t\"working tree\\n\") : \"\");\n \t\tif (status == 127)\n \t\t\t/* command not found */\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"476270","messageId":"20230428125649.1719796-3-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":"20230428125649.1719796-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-28T12:56:49Z","receivedAt":"2023-04-28T12:56:56Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"These are conscious violations of the usual rules for error messages,\nbased on this reasoning:\n- If an error message is directly followed by another sentence, it needs\n to be properly terminated with a period, lest the grammar looks broken\n and becomes hard to read.\n- That second sentence isn't actually an error message any more, so it\n should abide to conventional language rules for good looks and\n legibility. Arguably, these should be converted to advice messages\n (which the user can squelch, too), but that's a much bigger effort to\n get right.\n- Neither of these apply to the first hunk in do_exec(), but this\n two-line message looks just too much like a real sentence to not\n terminate it. Also, leaving it alone would make it asymmetrical to\n the other hunk.\n\nSigned-off-by: Oswald Buddenhagen \nCc: Junio C Hamano \n\n---\nv2:\n- tried to make commit message more convincing\n- put it last in the series, to make the less controversial changes\n easier to apply\n---\n builtin/pull.c | 2 +-\n sequencer.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 56f679d94a..bb2ddc93ab 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1044,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\tif (!opt_autostash)\n \t\t\trequire_clean_work_tree(the_repository,\n \t\t\t\tN_(\"pull with rebase\"),\n-\t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n+\t\t\t\t_(\"Please commit or stash them.\"), 1, 0);\n \n \t\tif (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n \t\t\toidclr(&rebase_fork_point);\ndiff --git a/sequencer.c b/sequencer.c\nindex 0677c9ab09..21748bbfb0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -3629,13 +3629,13 @@ static int do_exec(struct repository *r, const char *command_line)\n \t\t\t \"\\n\"),\n \t\t\tcommand_line,\n \t\t\tdirty ? _(\"and made changes to the index and/or the \"\n-\t\t\t\t\"working tree\\n\") : \"\");\n+\t\t\t\t\"working tree.\\n\") : \"\");\n \t\tif (status == 127)\n \t\t\t/* command not found */\n \t\t\tstatus = 1;\n \t} else if (dirty) {\n \t\twarning(_(\"execution succeeded: %s\\nbut \"\n-\t\t\t \"left changes to the index and/or the working tree\\n\"\n+\t\t\t \"left changes to the index and/or the working tree.\\n\"\n \t\t\t \"Commit or stash your changes, and then run\\n\"\n \t\t\t \"\\n\"\n \t\t\t \" git rebase --continue\\n\"\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"476271","messageId":"20230428125649.1719796-1-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":"20230323162234.995485-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v2 1/3] advice: handle \"rebase\" in error_resolve_conflict()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-28T12:56:47Z","receivedAt":"2023-04-28T12:56:57Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This makes sure that we get a properly translated message rather than\ninserting the command (which we failed to translate) into a generic\nfallback message.\n\nWe now also BUG() out when encountering an unexpected command.\n\nArguably, it would be cleaner to pass the command as an enum in the\nfirst place ...\n\nSigned-off-by: Oswald Buddenhagen \nCc: Junio C Hamano \n---\n advice.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex d6232439c3..c35ae82e7d 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -190,9 +190,10 @@ int error_resolve_conflict(const char *me)\n \t\terror(_(\"Pulling is not possible because you have unmerged files.\"));\n \telse if (!strcmp(me, \"revert\"))\n \t\terror(_(\"Reverting is not possible because you have unmerged files.\"));\n+\telse if (!strcmp(me, \"rebase\"))\n+\t\terror(_(\"Rebasing is not possible because you have unmerged files.\"));\n \telse\n-\t\terror(_(\"It is not possible to %s because you have unmerged files.\"),\n-\t\t\tme);\n+\t\tBUG(\"Unhandled conflict reason '%s'\", me);\n \n \tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n \t\t/*\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"476286","messageId":"xmqqttwzded6.fsf@gitster.g","threadId":"59447","inReplyTo":"20230428125649.1719796-3-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T18:57:09Z","receivedAt":"2023-04-28T18:58:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> These are conscious violations of the usual rules for error messages,\n> based on this reasoning:\n> - If an error message is directly followed by another sentence, it needs\n> to be properly terminated with a period, lest the grammar looks broken\n> and becomes hard to read.\n> - That second sentence isn't actually an error message any more, so it\n> should abide to conventional language rules for good looks and\n> legibility. Arguably, these should be converted to advice messages\n> (which the user can squelch, too), but that's a much bigger effort to\n> get right.\n\nI think both of the above are good guidelines to follow, with a hint\nfor a longer-term plan. Good description.\n\n> - Neither of these apply to the first hunk in do_exec(), but this\n> two-line message looks just too much like a real sentence to not\n> terminate it. Also, leaving it alone would make it asymmetrical to\n> the other hunk.\n\nI do not have a strong opinion on this one, in the sense that if\nsomebody writes a patch to terminate the sentence with the above\njustification, I do not think I would object to it, and if somebody\nomits this change from a patch like this, because the above two\nguidelines do not apply to it, I would not object to it, either.\n\n> Signed-off-by: Oswald Buddenhagen \n> Cc: Junio C Hamano \n\nWe generally do not want Cc: in the trailers. Can you move them\nafter the three-dash lines (which I think is still read by\nsend-email) next time you create more patches and throw them at the\nlist?\n\nWill queue.\n\n> ---\n> v2:\n> - tried to make commit message more convincing\n> - put it last in the series, to make the less controversial changes\n> easier to apply\n> ---\n> builtin/pull.c | 2 +-\n> sequencer.c | 4 ++--\n> 2 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 56f679d94a..bb2ddc93ab 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -1044,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n> \t\tif (!opt_autostash)\n> \t\t\trequire_clean_work_tree(the_repository,\n> \t\t\t\tN_(\"pull with rebase\"),\n> -\t\t\t\t_(\"please commit or stash them.\"), 1, 0);\n> +\t\t\t\t_(\"Please commit or stash them.\"), 1, 0);\n> \n> \t\tif (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs))\n> \t\t\toidclr(&rebase_fork_point);\n> diff --git a/sequencer.c b/sequencer.c\n> index 0677c9ab09..21748bbfb0 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3629,13 +3629,13 @@ static int do_exec(struct repository *r, const char *command_line)\n> \t\t\t \"\\n\"),\n> \t\t\tcommand_line,\n> \t\t\tdirty ? _(\"and made changes to the index and/or the \"\n> -\t\t\t\t\"working tree\\n\") : \"\");\n> +\t\t\t\t\"working tree.\\n\") : \"\");\n> \t\tif (status == 127)\n> \t\t\t/* command not found */\n> \t\t\tstatus = 1;\n> \t} else if (dirty) {\n> \t\twarning(_(\"execution succeeded: %s\\nbut \"\n> -\t\t\t \"left changes to the index and/or the working tree\\n\"\n> +\t\t\t \"left changes to the index and/or the working tree.\\n\"\n> \t\t\t \"Commit or stash your changes, and then run\\n\"\n> \t\t\t \"\\n\"\n> \t\t\t \" git rebase --continue\\n\"\n"},{"id":"476287","messageId":"xmqqjzxvde6p.fsf@gitster.g","threadId":"59447","inReplyTo":"20230428125649.1719796-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2 1/3] advice: handle \"rebase\" in error_resolve_conflict()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T19:01:02Z","receivedAt":"2023-04-28T19:01:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> This makes sure that we get a properly translated message rather than\n> inserting the command (which we failed to translate) into a generic\n> fallback message.\n\nHmph, can this be accompanied with a change to add a test to an\nexisting test script to demonstrate that the function can be called\nwith me set to \"rebase\" and results in a generic message?\n\n> We now also BUG() out when encountering an unexpected command.\n\nThis needs to be reviewed by somebody who is more familiar with the\nrebase/chrry-pick/revert/sequencer codepaths so that they can give a\ndefinitive \"good--I know that we never call this function with any\nother value in 'me'\" and that person would not be me.\n\n> Arguably, it would be cleaner to pass the command as an enum in the\n> first place ...\n\nTrue, but that can be left to a different topic, I would think.\n\nThanks.\n\n> Signed-off-by: Oswald Buddenhagen \n> Cc: Junio C Hamano \n> ---\n> advice.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/advice.c b/advice.c\n> index d6232439c3..c35ae82e7d 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -190,9 +190,10 @@ int error_resolve_conflict(const char *me)\n> \t\terror(_(\"Pulling is not possible because you have unmerged files.\"));\n> \telse if (!strcmp(me, \"revert\"))\n> \t\terror(_(\"Reverting is not possible because you have unmerged files.\"));\n> +\telse if (!strcmp(me, \"rebase\"))\n> +\t\terror(_(\"Rebasing is not possible because you have unmerged files.\"));\n> \telse\n> -\t\terror(_(\"It is not possible to %s because you have unmerged files.\"),\n> -\t\t\tme);\n> +\t\tBUG(\"Unhandled conflict reason '%s'\", me);\n> \n> \tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n> \t\t/*\n"},{"id":"476288","messageId":"xmqqcz3nde3h.fsf@gitster.g","threadId":"59447","inReplyTo":"20230428125649.1719796-2-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v2 2/3] sequencer: actually translate report in do_exec()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T19:02:58Z","receivedAt":"2023-04-28T19:03:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> N_() is meant to be used on strings that are subsequently _()'d, which\n> isn't the case here.\n\nGood eyes.\n\n>\n> The affected construct is a bit questionable from an i18n perspective,\n> as it pieces together a sentence from separate strings. However, it\n> doesn't appear to be that bad, as the \"assembly instructions\" are in a\n> translatable message as well. Lacking specific complaints from\n> translators, it doesn't seem worth changing this.\n\nTrue that we frown upon sentence Legos like this. At least the\noriginal message in C locale does not break the flow too badly, so\nhpoefully all the supported languages are happy with the existing\ncomposition.\n\nWill queue.\n\nThanks.\n\n\n>\n> Signed-off-by: Oswald Buddenhagen \n> Cc: Junio C Hamano \n>\n> ---\n> v2:\n> - mention the word puzzle in the commit message\n> ---\n> sequencer.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 3be23d7ca2..0677c9ab09 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -3628,7 +3628,7 @@ static int do_exec(struct repository *r, const char *command_line)\n> \t\t\t \" git rebase --continue\\n\"\n> \t\t\t \"\\n\"),\n> \t\t\tcommand_line,\n> -\t\t\tdirty ? N_(\"and made changes to the index and/or the \"\n> +\t\t\tdirty ? _(\"and made changes to the index and/or the \"\n> \t\t\t\t\"working tree\\n\") : \"\");\n> \t\tif (status == 127)\n> \t\t\t/* command not found */\n"},{"id":"476289","messageId":"xmqq7ctvddsb.fsf@gitster.g","threadId":"59447","inReplyTo":"xmqqttwzded6.fsf@gitster.g","subject":"Re: [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-28T19:09:40Z","receivedAt":"2023-04-28T19:09:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Oswald Buddenhagen writes:\n>\n>> These are conscious violations of the usual rules for error messages,\n>> based on this reasoning:\n>> - If an error message is directly followed by another sentence, it needs\n>> to be properly terminated with a period, lest the grammar looks broken\n>> and becomes hard to read.\n>> - That second sentence isn't actually an error message any more, so it\n>> should abide to conventional language rules for good looks and\n>> legibility. Arguably, these should be converted to advice messages\n>> (which the user can squelch, too), but that's a much bigger effort to\n>> get right.\n>\n> I think both of the above are good guidelines to follow, with a hint\n> for a longer-term plan. Good description.\n\nI think the above two deserves to be added (in some rephrased form\nto fit better) to the Documentation/CodingGuidelines, somewhere in\nthe following section:\n\n--- >8 ---\nError Messages\n\n - Do not end error messages 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 \"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\n--- 8< ---\n\nVolunteers?\n\nThanks.\n"},{"id":"476304","messageId":"ZEzErKvmBPwP0Hsp@ugly","threadId":"59447","inReplyTo":"xmqqjzxvde6p.fsf@gitster.g","subject":"Re: [PATCH v2 1/3] advice: handle \"rebase\" in error_resolve_conflict()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-04-29T07:18:04Z","receivedAt":"2023-04-29T07:18:11Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"On Fri, Apr 28, 2023 at 12:01:02PM -0700, Junio C Hamano wrote:\n>Oswald Buddenhagen writes:\n>\n>> This makes sure that we get a properly translated message rather than\n>> inserting the command (which we failed to translate) into a generic\n>> fallback message.\n>\n>Hmph, can this be accompanied with a change to add a test to an\n>existing test script to demonstrate that the function can be called\n>with me set to \"rebase\" and results in a generic message?\n>\ni suppose it could, but see next paragraph.\n\n>> We now also BUG() out when encountering an unexpected command.\n>\n>This needs to be reviewed by somebody who is more familiar with the\n>rebase/chrry-pick/revert/sequencer codepaths so that they can give a\n>definitive \"good--I know that we never call this function with any\n>other value in 'me'\" and that person would not be me.\n>\nassuming we care only about in-tree code, i'm just about as confident \nabout this as one can reasonably be - because i grepped through the \ncode, recursively looking for entry points. there are several calls via \ndie_resolve_conflict() which have hard-coded `me`s (none of which is \nrebase), and two from the sequencer, where `me` comes from \naction_name(), which in turn returns one of three hard-coded strings \n(one of which is rebase). the latter is also kinda the test case, \nbecause it is obvious that this will be actually invoked when the \nsituation occurs. it's probably also how i actually ran into the problem \nin the first place (i surely wasn't *looking* for it ...).\n\n>> Arguably, it would be cleaner to pass the command as an enum in the\n>> first place ...\n>\n>True, but that can be left to a different topic, I would think.\n>\nyes, otherwise i'd have already done it. ^^\ni can make it more explicit if you prefer that.\n\nif you agree with the reasoning, i'll prepare an update to the commit \nmessage and leave the patch as-is.\n\n-- ossi\n\n"},{"id":"476312","messageId":"xmqqv8he9cx6.fsf@gitster.g","threadId":"59447","inReplyTo":"ZEzErKvmBPwP0Hsp@ugly","subject":"Re: [PATCH v2 1/3] advice: handle \"rebase\" in error_resolve_conflict()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-04-30T05:06:29Z","receivedAt":"2023-04-30T05:06:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> On Fri, Apr 28, 2023 at 12:01:02PM -0700, Junio C Hamano wrote:\n>>Oswald Buddenhagen writes:\n>>\n>>> This makes sure that we get a properly translated message rather than\n>>> inserting the command (which we failed to translate) into a generic\n>>> fallback message.\n>>\n>>Hmph, can this be accompanied with a change to add a test to an\n>>existing test script to demonstrate that the function can be called\n>>with me set to \"rebase\" and results in a generic message?\n>>\n> i suppose it could, but see next paragraph.\n>\n>>> We now also BUG() out when encountering an unexpected command.\n>>\n>>This needs to be reviewed by somebody who is more familiar with the\n>>rebase/chrry-pick/revert/sequencer codepaths so that they can give a\n>>definitive \"good--I know that we never call this function with any\n>>other value in 'me'\" and that person would not be me.\n>>\n> assuming we care only about in-tree code, i'm just about as confident\n> about this as one can reasonably be - because i grepped through the\n> code, recursively looking for entry points. there are several calls\n> via die_resolve_conflict() which have hard-coded `me`s (none of which\n> is rebase), and two from the sequencer, where `me` comes from\n> action_name(), which in turn returns one of three hard-coded strings\n> (one of which is rebase). the latter is also kinda the test case,\n> because it is obvious that this will be actually invoked when the\n> situation occurs. it's probably also how i actually ran into the\n> problem in the first place (i surely wasn't *looking* for it ...).\n>\n>>> Arguably, it would be cleaner to pass the command as an enum in the\n>>> first place ...\n>>\n>>True, but that can be left to a different topic, I would think.\n>>\n> yes, otherwise i'd have already done it. ^^\n> i can make it more explicit if you prefer that.\n>\n> if you agree with the reasoning, i'll prepare an update to the commit\n> message and leave the patch as-is.\n\nYeah, how you arrived the conclusion that just covering \"rebase\" in\naddition to what the code already covers would make the if/elseif\ncascade complete is a very helpful addition to the log message.\n"},{"id":"480230","messageId":"20230807170935.2336715-1-oswald.buddenhagen@gmx.de","threadId":"59447","inReplyTo":"20230428125649.1719796-1-oswald.buddenhagen@gmx.de","subject":"[PATCH v3] advice: handle \"rebase\" in error_resolve_conflict()","fromName":"Oswald Buddenhagen","fromEmail":"oswald.buddenhagen@gmx.de","sentAt":"2023-08-07T17:09:35Z","receivedAt":"2023-08-07T17:09:40Z","isPatch":true,"sender":{"key":"oswald.buddenhagen@gmx.de","avatar":"https://avatars.githubusercontent.com/u/812380?v=4"},"body":"This makes sure that we get a properly translated message rather than\ninserting the command (which we failed to translate) into a generic\nfallback message.\n\nThe function is called indirectly via die_resolve_conflict() with fixed\nstrings, and directly with the string obtained via action_name(), which\nin turn returns a string from a fixed set. Hence we know that the now\ncovered set of strings is exhausitive, and will therefore BUG() out when\nencountering an unexpected string. We also know that all covered strings\nare actually used.\n\nArguably, the above suggests that it would be cleaner to pass the\ncommand as an enum in the first place, but that's left for another time.\n\nSigned-off-by: Oswald Buddenhagen \n---\nv2:\n- more verbose description\n\nCc: Junio C Hamano \n---\n advice.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/advice.c b/advice.c\nindex e5a9bb9b44..50c79443ba 100644\n--- a/advice.c\n+++ b/advice.c\n@@ -191,9 +191,10 @@ int error_resolve_conflict(const char *me)\n \t\terror(_(\"Pulling is not possible because you have unmerged files.\"));\n \telse if (!strcmp(me, \"revert\"))\n \t\terror(_(\"Reverting is not possible because you have unmerged files.\"));\n+\telse if (!strcmp(me, \"rebase\"))\n+\t\terror(_(\"Rebasing is not possible because you have unmerged files.\"));\n \telse\n-\t\terror(_(\"It is not possible to %s because you have unmerged files.\"),\n-\t\t\tme);\n+\t\tBUG(\"Unhandled conflict reason '%s'\", me);\n \n \tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n \t\t/*\n-- \n2.40.0.152.g15d061e6df\n\n"},{"id":"480255","messageId":"xmqq5y5qeh96.fsf@gitster.g","threadId":"59447","inReplyTo":"20230807170935.2336715-1-oswald.buddenhagen@gmx.de","subject":"Re: [PATCH v3] advice: handle \"rebase\" in error_resolve_conflict()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-08-07T20:20:53Z","receivedAt":"2023-08-07T20:21:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Oswald Buddenhagen writes:\n\n> This makes sure that we get a properly translated message rather than\n> inserting the command (which we failed to translate) into a generic\n> fallback message.\n\nMakes sense.\n\n>\n> The function is called indirectly via die_resolve_conflict() with fixed\n> strings, and directly with the string obtained via action_name(), which\n> in turn returns a string from a fixed set. Hence we know that the now\n> covered set of strings is exhausitive, and will therefore BUG() out when\n> encountering an unexpected string. We also know that all covered strings\n> are actually used.\n>\n> Arguably, the above suggests that it would be cleaner to pass the\n> command as an enum in the first place, but that's left for another time.\n>\n> Signed-off-by: Oswald Buddenhagen \n> ---\n> v2:\n> - more verbose description\n>\n> Cc: Junio C Hamano \n> ---\n> advice.c | 5 +++--\n> 1 file changed, 3 insertions(+), 2 deletions(-)\n>\n> diff --git a/advice.c b/advice.c\n> index e5a9bb9b44..50c79443ba 100644\n> --- a/advice.c\n> +++ b/advice.c\n> @@ -191,9 +191,10 @@ int error_resolve_conflict(const char *me)\n> \t\terror(_(\"Pulling is not possible because you have unmerged files.\"));\n> \telse if (!strcmp(me, \"revert\"))\n> \t\terror(_(\"Reverting is not possible because you have unmerged files.\"));\n> +\telse if (!strcmp(me, \"rebase\"))\n> +\t\terror(_(\"Rebasing is not possible because you have unmerged files.\"));\n> \telse\n> -\t\terror(_(\"It is not possible to %s because you have unmerged files.\"),\n> -\t\t\tme);\n> +\t\tBUG(\"Unhandled conflict reason '%s'\", me);\n> \n> \tif (advice_enabled(ADVICE_RESOLVE_CONFLICT))\n> \t\t/*\n"}]}