{"thread":{"id":"58466","subject":"[PATCH] builtin/diagnose.c: don't translate the two mode values","startedAt":"2022-09-20T05:07:00Z","lastAt":"2022-09-20T17:54:49Z","messageCount":3,"participants":["Alex Henrie","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"463272","messageId":"20220920050632.326307-1-alexhenrie24@gmail.com","threadId":"58466","inReplyTo":null,"subject":"[PATCH] builtin/diagnose.c: don't translate the two mode values","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-09-20T05:06:32Z","receivedAt":"2022-09-20T05:07:00Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"These strings are not translatable in the diagnose_options array in\ndiagnose.c. Don't translate them in builtin/diagnose.c either.\n\nSigned-off-by: Alex Henrie \n---\n builtin/diagnose.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/diagnose.c b/builtin/diagnose.c\nindex cd260c2015..576e0e8e38 100644\n--- a/builtin/diagnose.c\n+++ b/builtin/diagnose.c\n@@ -22,7 +22,7 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"specify a destination for the diagnostics archive\")),\n \t\tOPT_STRING('s', \"suffix\", &option_suffix, N_(\"format\"),\n \t\t\t N_(\"specify a strftime format suffix for the filename\")),\n-\t\tOPT_CALLBACK_F(0, \"mode\", &mode, N_(\"(stats|all)\"),\n+\t\tOPT_CALLBACK_F(0, \"mode\", &mode, \"(stats|all)\",\n \t\t\t N_(\"specify the content of the diagnostic archive\"),\n \t\t\t PARSE_OPT_NONEG, option_parse_diagnose),\n \t\tOPT_END()\n-- \n2.37.3\n\n"},{"id":"463301","messageId":"84848326-5cd9-b5b9-2dcb-a6d49879f47b@github.com","threadId":"58466","inReplyTo":"20220920050632.326307-1-alexhenrie24@gmail.com","subject":"Re: [PATCH] builtin/diagnose.c: don't translate the two mode values","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-09-20T12:35:16Z","receivedAt":"2022-09-20T12:35:27Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2022 1:06 AM, Alex Henrie wrote:\n> These strings are not translatable in the diagnose_options array in\n> diagnose.c. Don't translate them in builtin/diagnose.c either.\n> \n> Signed-off-by: Alex Henrie \n> ---\n> builtin/diagnose.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/builtin/diagnose.c b/builtin/diagnose.c\n> index cd260c2015..576e0e8e38 100644\n> --- a/builtin/diagnose.c\n> +++ b/builtin/diagnose.c\n> @@ -22,7 +22,7 @@ int cmd_diagnose(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"specify a destination for the diagnostics archive\")),\n> \t\tOPT_STRING('s', \"suffix\", &option_suffix, N_(\"format\"),\n> \t\t\t N_(\"specify a strftime format suffix for the filename\")),\n> -\t\tOPT_CALLBACK_F(0, \"mode\", &mode, N_(\"(stats|all)\"),\n> +\t\tOPT_CALLBACK_F(0, \"mode\", &mode, \"(stats|all)\",\n> \t\t\t N_(\"specify the content of the diagnostic archive\"),\n\nIn terms of the existing string, this makes sense. Initially, I\nthought that maybe we would rather have N_(\"mode\") to match the\nN_(\"format\") in the above. Then, I went poking around to see what\npatterns we had for these across the codebase (never a good idea).\n\nAn example similar to what you propose exists in builtin/branch.c:\n\n\t\tOPT_CALLBACK_F('t', \"track\", &track, \"(direct|inherit)\",\n\t\t\tN_(\"set branch tracking configuration\"),\n\t\t\tPARSE_OPT_OPTARG,\n\t\t\tparse_opt_tracking_mode),\n\nor this instance in builtin/checkout-index.c:\n\n\t\tOPT_CALLBACK_F(0, \"stage\", NULL, \"(1|2|3|all)\",\n\t\t\tN_(\"copy out the files from named stage\"),\n\nIn diff.c, the descriptors exist in angle brackets, so the right thing\nwould be N_(\"\"). This seems non-standard compared to most other\nplaces.\n\nHere is a similar stale translatable regex in diff.c:\n\n\t\tOPT_CALLBACK_F(0, \"diff-filter\", options, N_(\"[(A|C|D|M|R|T|U|X|B)...[*]]\"),\n\t\t\t N_(\"select files by diff type\"),\n\nSo if you are looking into these kinds of replacements, it might be\ngood to add instances like this. They are less important to the 2.38.0\nrelease, though.\n\nThis long-winded email is all just to say that I've looked into the\nstandard way to handle this and agree that you are changing the code\nto match our best practices.\n\nThanks,\n-Stolee\n"},{"id":"463320","messageId":"CAMMLpeRJ=4ES6d7pRP1Y232GyLx-jhN-ktfVernXrDiJuAC+tQ@mail.gmail.com","threadId":"58466","inReplyTo":"84848326-5cd9-b5b9-2dcb-a6d49879f47b@github.com","subject":"Re: [PATCH] builtin/diagnose.c: don't translate the two mode values","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2022-09-20T17:54:33Z","receivedAt":"2022-09-20T17:54:49Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Tue, Sep 20, 2022 at 6:35 AM Derrick Stolee wrote:\n>\n> In diff.c, the descriptors exist in angle brackets, so the right thing\n> would be N_(\"\"). This seems non-standard compared to most other\n> places.\n\nI don't know which form is preferred in the code, but I did find that\nusage_argh adds the angle brackets if they are not already present:\n\nstatic int usage_argh(const struct option *opts, FILE *outfile)\n{\n const char *s;\n int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||\n !opts->argh || !!strpbrk(opts->argh, \"()<>[]|\");\n if (opts->flags & PARSE_OPT_OPTARG)\n if (opts->long_name)\n s = literal ? \"[=%s]\" : \"[=<%s>]\";\n else\n s = literal ? \"[%s]\" : \"[<%s>]\";\n else\n s = literal ? \" %s\" : \" <%s>\";\n return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _(\"...\"));\n}\n\n> Here is a similar stale translatable regex in diff.c:\n>\n> OPT_CALLBACK_F(0, \"diff-filter\", options, N_(\"[(A|C|D|M|R|T|U|X|B)...[*]]\"),\n> N_(\"select files by diff type\"),\n>\n> So if you are looking into these kinds of replacements, it might be\n> good to add instances like this. They are less important to the 2.38.0\n> release, though.\n\nYeah, that one probably shouldn't be translatable either.\n\n> This long-winded email is all just to say that I've looked into the\n> standard way to handle this and agree that you are changing the code\n> to match our best practices.\n\nThanks! I appreciate the feedback.\n\n-Alex\n"}]}