{"thread":{"id":"57427","subject":"[PATCH] add usage-strings ci check and amend remaining usage strings","startedAt":"2022-02-16T17:02:36Z","lastAt":"2022-03-08T05:45:46Z","messageCount":58,"participants":["Abhradeep Chakraborty via GitGitGadget","Abhradeep Chakraborty","Ævar Arnfjörð Bjarmason","Junio C Hamano","Johannes Schindelin","Julia Lawall","Eric Sunshine","Abhra303 via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"448618","messageId":"pull.1147.git.1645030949730.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":null,"subject":"[PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-16T17:02:29Z","receivedAt":"2022-02-16T17:02:36Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"From: Abhra303 \n\nUsage strings for git (sub)command flags has a style guide that\nsuggests - first letter should not capitalized (unless requied)\nand it should skip full-stop at the end of line. But there are\nsome files where usage-strings do not follow the above mentioned\nguide. Moreover, there are no checks to verify if all usage strings\nare following the guide/convention or not.\n\nAmend the usage strings that don't follow the convention/guide and\nadd a `CI` check for checking the usage strings (whether the first\nletter is capital or it ends with full-stop). If the `check` find\nsuch strings then print those strings and return a non-zero status.\n\nAlso provide a script that takes an optional argument (a valid \nstring), to check the usage strings in the given (`HEAD` is\nthe default argument).\n\nSigned-off-by: Abhradeep Chakraborty \n---\n add usage-strings ci check and amend remaining usage strings\n \n This patch series completely fixes #636.\n \n The issue is about amending the usage-strings (for command flags such as\n -h, -v etc.) which do not follow the style convention/guide. There was a\n PR [https://github.com/gitgitgadget/git/pull/920] addressing this issue\n but as Johannes [https://github.com/dscho] said in his comment\n [https://github.com/gitgitgadget/git/issues/636#issuecomment-1018660439],\n there are some files that still have those kind of usage strings.\n Johannes also suggested to add a CI check under ci/test-documentation.sh\n to check the usage strings.\n \n So, in this patch, all remaining usage strings are corrected. I also\n added a check-usage-strings target in Makefile which can be used to\n check usage strings. It uses check-usage-strings.sh.\n \n 1. If check-usage-strings.sh is run on a valid git repo - it will check\n the validity of usage-strings in the tree specified by an argument\n or in the HEAD if no argument provided.\n 2. If the current repo is not a git repo (i.e. if it doesn't find any\n .git folder), it will check the usage string in the current root\n directory.\n \n For the first case, output of make check-usage-strings or\n ./check-usage-strings.sh would be similar to -\n \n HEAD:builtin/bisect--helper.c:1212 N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n HEAD:builtin/submodule--helper.c:1877 OPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n \n \n If an argument provided - ./check-usage-strings.sh 'v2.34.0' , it will\n search for usage-strings in v2.34.0 and v2.34.0 will be prefixed before\n filenames instead of HEAD.\n \n In the second case, output would be similar to -\n \n diff.c:5596 N_(\"select files by diff type.\"),\n diff.c:5599 N_(\"Output to a specific file\"),\n builtin/branch.c:666 OPT_BIT('C', NULL, ©, N_(\"copy a branch, even if target exists.\"), 2),\n make: *** [check-usage-strings] Error 1\n \n \n Note in the last case - arguments provided to it will be useless.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1147\n\n Makefile | 5 +++++\n builtin/bisect--helper.c | 2 +-\n builtin/reflog.c | 6 +++---\n builtin/submodule--helper.c | 2 +-\n check-usage-strings.sh | 33 +++++++++++++++++++++++++++++++++\n ci/test-documentation.sh | 1 +\n diff.c | 2 +-\n t/helper/test-run-command.c | 6 +++---\n 8 files changed, 48 insertions(+), 9 deletions(-)\n create mode 100755 check-usage-strings.sh\n\ndiff --git a/Makefile b/Makefile\nindex 186f9ab6190..93faed51da0 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -3416,6 +3416,11 @@ check-docs::\n check-builtins::\n \t./check-builtins.sh\n \n+### Make sure all the usage strings follow usage string style guide\n+#\n+check-usage-strings::\n+\t./check-usage-strings.sh\n+\n ### Test suite coverage testing\n #\n .PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 28a2e6a5750..614d95b022c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n-\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n+\t\t\t N_(\"use ... to automatically bisect\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 85b838720c3..28372c5e2b5 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_CALLBACK_F(0, \"expire\", &cmd, N_(\"timestamp\"),\n \t\t\t N_(\"prune entries older than the specified time\"),\n \t\t\t PARSE_OPT_NONEG,\n@@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"prune any reflog entries that point to broken commits\")),\n \t\tOPT_BOOL(0, \"all\", &do_all, N_(\"process the reflogs of all references\")),\n \t\tOPT_BOOL(1, \"single-worktree\", &all_worktrees,\n-\t\t\t N_(\"limits processing to reflogs from the current worktree only.\")),\n+\t\t\t N_(\"limits processing to reflogs from the current worktree only\")),\n \t\tOPT_END()\n \t};\n \n@@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex c5d3fc3817f..9864ec1427d 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1874,7 +1874,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n-\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT__QUIET(&quiet, \"suppress output for cloning a submodule\"),\n \t\tOPT_BOOL(0, \"progress\", &progress,\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\ndiff --git a/check-usage-strings.sh b/check-usage-strings.sh\nnew file mode 100755\nindex 00000000000..a4028e0d00d\n--- /dev/null\n+++ b/check-usage-strings.sh\n@@ -0,0 +1,33 @@\n+{\n+ if test -d \".git\"\n+ then\n+ rev=${1:-\"HEAD\"}\n+ for entry in $(git grep -l 'struct option .* = {$' \"$rev\" -- \\*.c);\n+ do\n+ git show \"$entry\" |\n+ sed -n '/struct option .* = {/,/OPT_END/{=;p;}' |\n+ sed \"N;s/^\\\\([0-9]*\\\\)\\\\n/$(echo \"$entry\" | sed 's/\\//\\\\&/g'):\\\\1/\";\n+ done\n+ else\n+ for entry in $(grep -rl --include=\"*.c\" 'struct option .* = {$' . );\n+ do\n+ cat \"$entry\" |\n+ sed -n '/struct option .* = {/,/OPT_END/{=;p;}' |\n+ sed \"N;s/^\\\\([0-9]*\\\\)\\\\n/$(echo \"$entry\" | sed -e 's/\\//\\\\&/g' -e 's/^\\.\\\\\\///'):\\\\1/\";\n+ done\n+ fi\n+} |\n+grep -Pe '((?\"),\n-\t\t N_(\"Output to a specific file\"),\n+\t\t N_(\"output to a specific file\"),\n \t\t PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n \n \t\tOPT_END()\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 913775a14b7..8f370cd89f1 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct strvec args = STRVEC_INIT;\n \tstruct option options[] = {\n-\t\tOPT_INTEGER('n', \"trials\", &trials, \"Number of trials\"),\n-\t\tOPT_INTEGER('s', \"skip\", &skip, \"Skip trials\"),\n-\t\tOPT_BOOL('m', \"msys2\", &msys2, \"Test quoting for MSYS2's sh\"),\n+\t\tOPT_INTEGER('n', \"trials\", &trials, \"number of trials\"),\n+\t\tOPT_INTEGER('s', \"skip\", &skip, \"skip trials\"),\n+\t\tOPT_BOOL('m', \"msys2\", &msys2, \"test quoting for MSYS2's sh\"),\n \t\tOPT_END()\n \t};\n \tconst char * const usage[] = {\n\nbase-commit: b80121027d1247a0754b3cc46897fee75c050b44\n-- \ngitgitgadget\n"},{"id":"449006","messageId":"20220221145146.6224-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"pull.1147.git.1645030949730.gitgitgadget@gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-21T14:51:46Z","receivedAt":"2022-02-21T14:52:39Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Hello reviewers and community members,\n\nThis patch request is not reviewed yet (i.e. no response/suggestions came about this patch request; most probably because it was lost in the PR ocean).\n\nSo, could you please review my patch request?\n\nthanks :)\n"},{"id":"449008","messageId":"220221.86tucsb4oy.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"pull.1147.git.1645030949730.gitgitgadget@gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T15:39:54Z","receivedAt":"2022-02-21T15:49:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 16 2022, Abhradeep Chakraborty via GitGitGadget wrote:\n\n> From: Abhra303 \n>\n> Usage strings for git (sub)command flags has a style guide that\n> suggests - first letter should not capitalized (unless requied)\n> and it should skip full-stop at the end of line. But there are\n> some files where usage-strings do not follow the above mentioned\n> guide. Moreover, there are no checks to verify if all usage strings\n> are following the guide/convention or not.\n>\n> Amend the usage strings that don't follow the convention/guide and\n> add a `CI` check for checking the usage strings (whether the first\n> letter is capital or it ends with full-stop). If the `check` find\n> such strings then print those strings and return a non-zero status.\n>\n> Also provide a script that takes an optional argument (a valid \n> string), to check the usage strings in the given (`HEAD` is\n> the default argument).\n>\n> Signed-off-by: Abhradeep Chakraborty \n> ---\n> add usage-strings ci check and amend remaining usage strings\n> \n> This patch series completely fixes #636.\n> \n> The issue is about amending the usage-strings (for command flags such as\n> -h, -v etc.) which do not follow the style convention/guide. There was a\n> PR [https://github.com/gitgitgadget/git/pull/920] addressing this issue\n> but as Johannes [https://github.com/dscho] said in his comment\n> [https://github.com/gitgitgadget/git/issues/636#issuecomment-1018660439],\n> there are some files that still have those kind of usage strings.\n> Johannes also suggested to add a CI check under ci/test-documentation.sh\n> to check the usage strings.\n> \n> So, in this patch, all remaining usage strings are corrected. I also\n> added a check-usage-strings target in Makefile which can be used to\n> check usage strings. It uses check-usage-strings.sh.\n> \n> 1. If check-usage-strings.sh is run on a valid git repo - it will check\n> the validity of usage-strings in the tree specified by an argument\n> or in the HEAD if no argument provided.\n> 2. If the current repo is not a git repo (i.e. if it doesn't find any\n> .git folder), it will check the usage string in the current root\n> directory.\n> \n> For the first case, output of make check-usage-strings or\n> ./check-usage-strings.sh would be similar to -\n> \n> HEAD:builtin/bisect--helper.c:1212 N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n> HEAD:builtin/submodule--helper.c:1877 OPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n> \n> \n> If an argument provided - ./check-usage-strings.sh 'v2.34.0' , it will\n> search for usage-strings in v2.34.0 and v2.34.0 will be prefixed before\n> filenames instead of HEAD.\n> \n> In the second case, output would be similar to -\n> \n> diff.c:5596 N_(\"select files by diff type.\"),\n> diff.c:5599 N_(\"Output to a specific file\"),\n> builtin/branch.c:666 OPT_BIT('C', NULL, ©, N_(\"copy a branch, even if target exists.\"), 2),\n> make: *** [check-usage-strings] Error 1\n> \n> \n> Note in the last case - arguments provided to it will be useless.\n>\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v1\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1147\n\nSorry about leaving this patch submission hanging. I read this at the\ntime, but forgot to find time to loop back to it.\n\n> Makefile | 5 +++++\n> builtin/bisect--helper.c | 2 +-\n> builtin/reflog.c | 6 +++---\n> builtin/submodule--helper.c | 2 +-\n> check-usage-strings.sh | 33 +++++++++++++++++++++++++++++++++\n> ci/test-documentation.sh | 1 +\n> diff.c | 2 +-\n> t/helper/test-run-command.c | 6 +++---\n> 8 files changed, 48 insertions(+), 9 deletions(-)\n> create mode 100755 check-usage-strings.sh\n>\n> diff --git a/Makefile b/Makefile\n> index 186f9ab6190..93faed51da0 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -3416,6 +3416,11 @@ check-docs::\n> check-builtins::\n> \t./check-builtins.sh\n> \n> +### Make sure all the usage strings follow usage string style guide\n> +#\n> +check-usage-strings::\n> +\t./check-usage-strings.sh\n> +\n> ### Test suite coverage testing\n> #\n> .PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report\n> diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\n> index 28a2e6a5750..614d95b022c 100644\n> --- a/builtin/bisect--helper.c\n> +++ b/builtin/bisect--helper.c\n> @@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n> \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n> \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n> \t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n> -\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n> +\t\t\t N_(\"use ... to automatically bisect\"), BISECT_RUN),\n> \t\tOPT_BOOL(0, \"no-log\", &nolog,\n> \t\t\t N_(\"no log for BISECT_WRITE\")),\n> \t\tOPT_END()\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 85b838720c3..28372c5e2b5 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BIT(0, \"updateref\", &flags,\n> \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n> \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n> -\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n> +\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n> \t\tOPT_CALLBACK_F(0, \"expire\", &cmd, N_(\"timestamp\"),\n> \t\t\t N_(\"prune entries older than the specified time\"),\n> \t\t\t PARSE_OPT_NONEG,\n> @@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n> \t\t\t N_(\"prune any reflog entries that point to broken commits\")),\n> \t\tOPT_BOOL(0, \"all\", &do_all, N_(\"process the reflogs of all references\")),\n> \t\tOPT_BOOL(1, \"single-worktree\", &all_worktrees,\n> -\t\t\t N_(\"limits processing to reflogs from the current worktree only.\")),\n> +\t\t\t N_(\"limits processing to reflogs from the current worktree only\")),\n> \t\tOPT_END()\n> \t};\n> \n> @@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n> \t\tOPT_BIT(0, \"updateref\", &flags,\n> \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n> \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n> -\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n> +\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n> \t\tOPT_END()\n> \t};\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index c5d3fc3817f..9864ec1427d 100644\n> --- a/builtin/submodule--helper.c\n> +++ b/builtin/submodule--helper.c\n> @@ -1874,7 +1874,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n> \t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n> \t\t\t N_(\"string\"),\n> \t\t\t N_(\"depth for shallow clones\")),\n> -\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n> +\t\tOPT__QUIET(&quiet, \"suppress output for cloning a submodule\"),\n> \t\tOPT_BOOL(0, \"progress\", &progress,\n> \t\t\t N_(\"force cloning progress\")),\n> \t\tOPT_BOOL(0, \"require-init\", &require_init,\n\nIt's really good to have these fixed! Ditto for the remaining ones I\nelided.\n\n> diff --git a/check-usage-strings.sh b/check-usage-strings.sh\n> new file mode 100755\n> index 00000000000..a4028e0d00d\n> --- /dev/null\n> +++ b/check-usage-strings.sh\n> @@ -0,0 +1,33 @@\n> +{\n> + if test -d \".git\"\n> + then\n> + rev=${1:-\"HEAD\"}\n> + for entry in $(git grep -l 'struct option .* = {$' \"$rev\" -- \\*.c);\n> + do\n> + git show \"$entry\" |\n> + sed -n '/struct option .* = {/,/OPT_END/{=;p;}' |\n> + sed \"N;s/^\\\\([0-9]*\\\\)\\\\n/$(echo \"$entry\" | sed 's/\\//\\\\&/g'):\\\\1/\";\n> + done\n> + else\n> + for entry in $(grep -rl --include=\"*.c\" 'struct option .* = {$' . );\n> + do\n> + cat \"$entry\" |\n> + sed -n '/struct option .* = {/,/OPT_END/{=;p;}' |\n> + sed \"N;s/^\\\\([0-9]*\\\\)\\\\n/$(echo \"$entry\" | sed -e 's/\\//\\\\&/g' -e 's/^\\.\\\\\\///'):\\\\1/\";\n> + done\n> + fi\n> +} |\n> +grep -Pe '((? +{\n> + status=0\n> + while read content;\n> + do\n> + if test -n \"$content\"\n> + then\n> + echo \"$content\";\n> + status=1;\n> + fi\n> + done\n> +\n> + exit $status\n> +}\n> diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh\n> index de41888430a..f66848dfc66 100755\n> --- a/ci/test-documentation.sh\n> +++ b/ci/test-documentation.sh\n> @@ -15,6 +15,7 @@ filter_log () {\n> }\n\nAs much as I like the idea, I really don't want us to have this method\nof doing it though, i.e. to start parsing our C code with a\nhard-to-maintain shellscript.\n\nBut the good news is that there's much easier way to add this!\n\nAside: if we did want to do the \"parse C\" method the right way to do it\nwould be to have a coccinelle script do it. We don't currently, but we\nuse coccicheck, and if you look at the linux kernel's use of it there's\nmultiple such checks there. I.e. you can have it parse the C and run\nyour checks with an arbitrary script.\n\nBut in this case there's really a much easier way to do this, to just\nextend something like this:\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2437ad3bcdd..90d8da6ad4c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -492,6 +492,8 @@ static void parse_options_check(const struct option *opts)\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n+\t\tif (opts->help && ends_with(opts->help, \".\"))\n+\t\t\terr |= optbug(opts, xstrfmt(\"argh should not end with a dot: %s\", opts->help));\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n\nThen the t0012-help.sh test will catch these, and that's where these\nsorts of checks belong in our tree.\n\nSee b6c2a0d45d4 (parse-options: make sure argh string does not have SP\nor _, 2014-03-23) for the existing code shown in the context where we\nalready check \"argh\" like that, i.e. we're just missing a test for\n\"help\".\n\nObviously such a function would need to hardcode some of the logic you\nadded in your shellscript. E.g. this fires on a string ending in \"...\",\nbut yours doesn't.\n\nThat should be fairly easy to do though, and if not we could always just\ndump these to stderr or something if a\ngit_env_bool(\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\", 0) was true, and\ndo the testing itself in t0012-help.sh.\n\n> make check-builtins\n> +make check-usage-strings\n> make check-docs\n\nGood to have this as a \"make\" target, not something e.g. peculiar to CI.\n"},{"id":"449020","messageId":"xmqqsfscta3l.fsf@gitster.g","threadId":"57427","inReplyTo":"220221.86tucsb4oy.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-21T17:15:10Z","receivedAt":"2022-02-21T17:15:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh\n>> index de41888430a..f66848dfc66 100755\n>> --- a/ci/test-documentation.sh\n>> +++ b/ci/test-documentation.sh\n>> @@ -15,6 +15,7 @@ filter_log () {\n>> }\n>\n> As much as I like the idea, I really don't want us to have this method\n> of doing it though, i.e. to start parsing our C code with a\n> hard-to-maintain shellscript.\n>\n> But the good news is that there's much easier way to add this!\n\n;-) Good suggestions. I have nothing to add.\n\n"},{"id":"449024","messageId":"20220221173357.8622-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"220221.86tucsb4oy.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-21T17:33:57Z","receivedAt":"2022-02-21T17:34:50Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> Sorry about leaving this patch submission hanging. I read this at the\n> time, but forgot to find time to loop back to it.\n\nNo worries. Thanks for reviewing :)\n\n> But in this case there's really a much easier way to do this, to just\n> extend something like this:\n> ...\n> See b6c2a0d45d4 (parse-options: make sure argh string does not have SP\n> or _, 2014-03-23) for the existing code shown in the context where we\n> already check \"argh\" like that, i.e. we're just missing a test for\n> \"help\".\n>\n> Obviously such a function would need to hardcode some of the logic you\n> added in your shellscript. E.g. this fires on a string ending in \"...\",\n> but yours doesn't.\n\nThank you so much for the suggestion. Didn't aware of it before. I will\ntry to implement the logic in parse-options.c` (as you suggested).\n\n> That should be fairly easy to do though, and if not we could always just\n> dump these to stderr or something if a\n> git_env_bool(\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\", 0) was true, and\n> do the testing itself in t0012-help.sh.\n\nOkay but if the logic can't be implented in the `parse-options.c` file\n(most probably I will be able to implement the logic), would you allow me\nto try the `coccinelle script` method you mentioned?\n\nThanks :)\n"},{"id":"449037","messageId":"220221.86h78saw4f.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"20220221173357.8622-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-21T18:52:26Z","receivedAt":"2022-02-21T18:54:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 21 2022, Abhradeep Chakraborty wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>\n>> Sorry about leaving this patch submission hanging. I read this at the\n>> time, but forgot to find time to loop back to it.\n>\n> No worries. Thanks for reviewing :)\n>\n>> But in this case there's really a much easier way to do this, to just\n>> extend something like this:\n>> ...\n>> See b6c2a0d45d4 (parse-options: make sure argh string does not have SP\n>> or _, 2014-03-23) for the existing code shown in the context where we\n>> already check \"argh\" like that, i.e. we're just missing a test for\n>> \"help\".\n>>\n>> Obviously such a function would need to hardcode some of the logic you\n>> added in your shellscript. E.g. this fires on a string ending in \"...\",\n>> but yours doesn't.\n>\n> Thank you so much for the suggestion. Didn't aware of it before. I will\n> try to implement the logic in parse-options.c` (as you suggested).\n>\n>> That should be fairly easy to do though, and if not we could always just\n>> dump these to stderr or something if a\n>> git_env_bool(\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\", 0) was true, and\n>> do the testing itself in t0012-help.sh.\n>\n> Okay but if the logic can't be implented in the `parse-options.c` file\n> (most probably I will be able to implement the logic), would you allow me\n> to try the `coccinelle script` method you mentioned?\n\nIn this case I think there's definitely no reason for why it can't/won't\nwork in parse-options.c.\n\nIf you're doing something like that with coccicheck I'm afraid I can't\nhelp much, I've only seen that the kernel is doing it (it's referenced\nin some of the coccinelle docs), but I haven't personally used it for\nanything close to that.\n\n"},{"id":"449099","messageId":"20220222102536.83705-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"220221.86tucsb4oy.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-22T10:25:36Z","receivedAt":"2022-02-22T10:26:18Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Ævar Arnfjörð Bjarmason wrote:\n\n> But in this case there's really a much easier way to do this, to just\n> extend something like this:\n> ...\n> See b6c2a0d45d4 (parse-options: make sure argh string does not have SP\n> or _, 2014-03-23) for the existing code shown in the context where we\n> already check \"argh\" like that, i.e. we're just missing a test for\n> \"help\".\n>\n> Obviously such a function would need to hardcode some of the logic you\n> added in your shellscript. E.g. this fires on a string ending in \"...\",\n> but yours doesn't.\n\nHello Ævar, I have some query related to this method. I have implemented\nthe logic locally and tests are also passing. However, I think the test\nyou mentioned is only running against the builtin files and files that\nare used in builtin commands (e.g. `diff.c`, `builtin/add.c` etc.). But\nsome files from `t/helper` (e.g. t/helper/test-run-command.c) also uses\nparse option API and it seems that there are no test files (pardon me if I\nam wrong) for checking `parse option usage strings check` for `t/helper`\ntest-tool commands.\n\nE.g. `grep -r --include=\"*.c\" 'struct option .*\\[] = {$' .` command gives\nthe following output - \n\n./helper/test-parse-options.c: struct option options[] = {\n./helper/test-lazy-init-name-hash.c: struct option options[] = {\n./helper/test-serve-v2.c: struct option options[] = {\n./helper/test-simple-ipc.c: struct option options[] = {\n./helper/test-parse-pathspec-file.c: struct option options[] = {\n./helper/test-getcwd.c: struct option options[] = {\n./helper/test-run-command.c: struct option options[] = {\n./helper/test-run-command.c: struct option options[] = {\n./helper/test-proc-receive.c: struct option options[] = {\n./helper/test-progress.c: struct option options[] = {\n./helper/test-tool.c: struct option options[] = {\n\nSo, these files are using parse-options and there is a chance that in\nfuture, usage strings from these files may violate the style guide. In\nthis case, all tests will be passing even if there are some style\nviolations. What do you think?\n\nThanks :)\n"},{"id":"449106","messageId":"nycvar.QRO.7.76.6.2202221152230.11118@tvgsbejvaqbjf.bet","threadId":"57427","inReplyTo":"20220221173357.8622-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-22T10:57:54Z","receivedAt":"2022-02-22T10:58:04Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Julia,\n\nI would like to loop you in here because you have helped us with\nCoccinelle questions in the past.\n\nOn Mon, 21 Feb 2022, Abhradeep Chakraborty wrote:\n\n> Ævar Arnfjörð Bjarmason wrote:\n>\n> > That should be fairly easy to do though, and if not we could always\n> > just dump these to stderr or something if a\n> > git_env_bool(\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\", 0) was true,\n> > and do the testing itself in t0012-help.sh.\n>\n> Okay but if the logic can't be implented in the `parse-options.c` file\n> (most probably I will be able to implement the logic), would you allow\n> me to try the `coccinelle script` method you mentioned?\n\nThe task at hand is to identify calls to the macro `OPT_CMDMODE()` (and\nother, similar macros) that get a fourth argument of the form\n\n\tN_(\"\")\n\nThe problem is to identify `` that ends in a `.` (which we\nwant to avoid) or that starts with some prefix and a colon but follows\nwith an upper-case character.\n\nIn other words, we want to suggest replacing\n\n\tN_(\"log: Use something\")\n\nor\n\n\tN_(\"log: use something.\")\n\nby\n\n\tN_(\"log: use something\")\n\nÆvar suggested that Coccinelle can do that. Could you give us a hand how\nthis would be possible using `spatch`?\n\nThank you,\nJohannes\n"},{"id":"449120","messageId":"220222.867d9n83ir.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2202221152230.11118@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-22T12:37:12Z","receivedAt":"2022-02-22T12:55:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Feb 22 2022, Johannes Schindelin wrote:\n\n> Hi Julia,\n>\n> I would like to loop you in here because you have helped us with\n> Coccinelle questions in the past.\n\nThanks. Probably better to CC the relevant ML, adding it.\n\n> On Mon, 21 Feb 2022, Abhradeep Chakraborty wrote:\n>\n>> Ævar Arnfjörð Bjarmason wrote:\n>>\n>> > That should be fairly easy to do though, and if not we could always\n>> > just dump these to stderr or something if a\n>> > git_env_bool(\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\", 0) was true,\n>> > and do the testing itself in t0012-help.sh.\n>>\n>> Okay but if the logic can't be implented in the `parse-options.c` file\n>> (most probably I will be able to implement the logic), would you allow\n>> me to try the `coccinelle script` method you mentioned?\n>\n> The task at hand is to identify calls to the macro `OPT_CMDMODE()` (and\n> other, similar macros) that get a fourth argument of the form\n>\n> \tN_(\"\")\n>\n> The problem is to identify `` that ends in a `.` (which we\n> want to avoid) or that starts with some prefix and a colon but follows\n> with an upper-case character.\n>\n> In other words, we want to suggest replacing\n>\n> \tN_(\"log: Use something\")\n>\n> or\n>\n> \tN_(\"log: use something.\")\n>\n> by\n>\n> \tN_(\"log: use something\")\n>\n> Ævar suggested that Coccinelle can do that. Could you give us a hand how\n> this would be possible using `spatch`?\n\nI probably shouldn't have mentioned that at all, and I think this is\nacademic in this context, because as noted we can just add this to\nparse_options_check() (linking it here again for off-git-ml context):\n\n https://lore.kernel.org/git/220221.86tucsb4oy.gmgdl@evledraar.gmail.com/\n\nWe now pay that trivial runtime overhead every time, we could run it\nonly during the tests if we ever got worried about it.\n\nAnd it's a lot less fragile and easy to understand than running\ncoccicheck, i.e. as nice as it is it's still takes a while to run, is\nits own mini-language, needs to be kept in sync with code changes etc.\n\nSo by doing it at runtime we can adjust messages, code & tests in an\natomic patch more easily (i.e. not assume that you ran some cocci target\nto validate it).\n\nIt also makes it really easy to do things that are really hard (or\nimpossible?) with coccinelle. I.e. some of these checks are run as a\nfunction of what flag gets passed into some function later on, which in\nthe general case would require coccinelle to have some runtime emulator\nfor C code just to see *what* checks it wants to run.\n\nThat being said (and with the caveat that I've only looked at this code,\nnot done this myself) if you clone linux.git and browse through:\n\n git grep -C100 -F coccilib.report '*.cocci'\n\nYou can see a lot of examples of using cocci for these sorts of checks.\n\nAnd the same goes if you clone coccinelle.git and do:\n\n git grep -C100 @script: -- tests\n\nFor linux.git it's documented here:\nhttps://github.com/torvalds/linux/blob/master/Documentation/dev-tools/coccinelle.rst\n\nI.e. it's basically writing the sort of cocci rules we have in-tree with\na callback script that complaints about the required change.\n\nFor our use it would probably better (in lieu of parse_options_check(),\nwhich is the right thing here) to just have a normal *.cocci file and\ncomplain if it applies changes. We already error in the CI if those need\nto apply any changes.\n\nBut I don't off-hand know how to do that. E.g. I was trying the other\nday to come up with some coccinelle rule that converted:\n\n die(\"BUG: blah blah\");\n\nTo:\n\n BUG(\"blah blah\");\n\nAnd while I'm sure there's some way to do this, couldn't find a way to\nwrite a rule to \"reach in\" to a constant string, apply some check or\nsearch/replacement on it, and to do a subsequent transformation on it.\n\nIn this case the OPT_BLAH(1, 2, 3, \"string here\") has OPT_BLAH() as a\nmacro. I can't remember if there's extra caveats around using coccinelle\nfor macros v.s. symbols.\n\nDisclaimer: By \"couldn't\" I mean I grepped the above examples for all of\na few minutes quickly skimmed the coccinelle docs, didn't find a\ntemplate I could copy, then ended up writing some nasty grep/xargs/perl\nfor-loop instead :)\n"},{"id":"449128","messageId":"alpine.DEB.2.22.394.2202221436320.2556@hadrien","threadId":"57427","inReplyTo":"220222.867d9n83ir.gmgdl@evledraar.gmail.com","subject":"Re: [cocci] [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Julia Lawall","fromEmail":"julia.lawall@inria.fr","sentAt":"2022-02-22T13:42:33Z","receivedAt":"2022-02-22T13:43:43Z","isPatch":true,"sender":{"key":"julia.lawall@inria.fr","avatar":null},"body":"\n\nOn Tue, 22 Feb 2022, Ævar Arnfjörð Bjarmason wrote:\n\n>\n> On Tue, Feb 22 2022, Johannes Schindelin wrote:\n>\n> > Hi Julia,\n> >\n> > I would like to loop you in here because you have helped us with\n> > Coccinelle questions in the past.\n>\n> Thanks. Probably better to CC the relevant ML, adding it.\n>\n> > On Mon, 21 Feb 2022, Abhradeep Chakraborty wrote:\n> >\n> >> Ævar Arnfjörð Bjarmason wrote:\n> >>\n> >> > That should be fairly easy to do though, and if not we could always\n> >> > just dump these to stderr or something if a\n> >> > git_env_bool(\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\", 0) was true,\n> >> > and do the testing itself in t0012-help.sh.\n> >>\n> >> Okay but if the logic can't be implented in the `parse-options.c` file\n> >> (most probably I will be able to implement the logic), would you allow\n> >> me to try the `coccinelle script` method you mentioned?\n> >\n> > The task at hand is to identify calls to the macro `OPT_CMDMODE()` (and\n> > other, similar macros) that get a fourth argument of the form\n> >\n> > \tN_(\"\")\n> >\n> > The problem is to identify `` that ends in a `.` (which we\n> > want to avoid) or that starts with some prefix and a colon but follows\n> > with an upper-case character.\n> >\n> > In other words, we want to suggest replacing\n> >\n> > \tN_(\"log: Use something\")\n> >\n> > or\n> >\n> > \tN_(\"log: use something.\")\n> >\n> > by\n> >\n> > \tN_(\"log: use something\")\n> >\n> > Ævar suggested that Coccinelle can do that. Could you give us a hand how\n> > this would be possible using `spatch`?\n\nHello,\n\nI'm not sure to follow all of the following.\n\nOf there are some cases that are useful to do statically, with only local\ninformation, then using Coccinelle could be useful to get the problem out\nof the way once and for all. Coccinelle doesn't support much processing\nof strings directly, but you can always write some python code to test the\ncontents of a string and to create a new one.\n\nLet me know if you want to try this. You can also check, eg the demo\ndemos/pythontococci.cocci to see how to create code in a python script and\nthen use it in a normal SmPL rule.\n\nIf some context has to be taken into account and the context in the same\nfunction, then that can also be done with Coccinelle, eg\n\nA\n...\nB\n\nmatches the case where after an A there is a B on all execution paths\n(except perhaps those that end in an error exit) and\n\nA\n... when exists\nB\n\nmatches the case where there is a B sometime after executing A, even if\nthat does not always occur.\n\nIf the context that you are interested in is in a called function or is in\nthe calling context, then Coccinelle might not be the ideal choice.\nCoccinelle works on one function at a time, so to do anything\ninterprocedural, you have to do some hacks.\n\njulia\n>\n> I probably shouldn't have mentioned that at all, and I think this is\n> academic in this context, because as noted we can just add this to\n> parse_options_check() (linking it here again for off-git-ml context):\n>\n> https://lore.kernel.org/git/220221.86tucsb4oy.gmgdl@evledraar.gmail.com/\n>\n> We now pay that trivial runtime overhead every time, we could run it\n> only during the tests if we ever got worried about it.\n>\n> And it's a lot less fragile and easy to understand than running\n> coccicheck, i.e. as nice as it is it's still takes a while to run, is\n> its own mini-language, needs to be kept in sync with code changes etc.\n>\n> So by doing it at runtime we can adjust messages, code & tests in an\n> atomic patch more easily (i.e. not assume that you ran some cocci target\n> to validate it).\n>\n> It also makes it really easy to do things that are really hard (or\n> impossible?) with coccinelle. I.e. some of these checks are run as a\n> function of what flag gets passed into some function later on, which in\n> the general case would require coccinelle to have some runtime emulator\n> for C code just to see *what* checks it wants to run.\n>\n> That being said (and with the caveat that I've only looked at this code,\n> not done this myself) if you clone linux.git and browse through:\n>\n> git grep -C100 -F coccilib.report '*.cocci'\n>\n> You can see a lot of examples of using cocci for these sorts of checks.\n>\n> And the same goes if you clone coccinelle.git and do:\n>\n> git grep -C100 @script: -- tests\n>\n> For linux.git it's documented here:\n> https://github.com/torvalds/linux/blob/master/Documentation/dev-tools/coccinelle.rst\n>\n> I.e. it's basically writing the sort of cocci rules we have in-tree with\n> a callback script that complaints about the required change.\n>\n> For our use it would probably better (in lieu of parse_options_check(),\n> which is the right thing here) to just have a normal *.cocci file and\n> complain if it applies changes. We already error in the CI if those need\n> to apply any changes.\n>\n> But I don't off-hand know how to do that. E.g. I was trying the other\n> day to come up with some coccinelle rule that converted:\n>\n> die(\"BUG: blah blah\");\n>\n> To:\n>\n> BUG(\"blah blah\");\n>\n> And while I'm sure there's some way to do this, couldn't find a way to\n> write a rule to \"reach in\" to a constant string, apply some check or\n> search/replacement on it, and to do a subsequent transformation on it.\n>\n> In this case the OPT_BLAH(1, 2, 3, \"string here\") has OPT_BLAH() as a\n> macro. I can't remember if there's extra caveats around using coccinelle\n> for macros v.s. symbols.\n>\n> Disclaimer: By \"couldn't\" I mean I grepped the above examples for all of\n> a few minutes quickly skimmed the coccinelle docs, didn't find a\n> template I could copy, then ended up writing some nasty grep/xargs/perl\n> for-loop instead :)\n>"},{"id":"449132","messageId":"20220222140347.85321-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"alpine.DEB.2.22.394.2202221436320.2556@hadrien","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-22T14:03:47Z","receivedAt":"2022-02-22T14:04:39Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Julia Lawall wrote:\n\n> Of there are some cases that are useful to do statically, with only local\n> information, then using Coccinelle could be useful to get the problem out\n> of the way once and for all. Coccinelle doesn't support much processing\n> of strings directly, but you can always write some python code to test the\n> contents of a string and to create a new one.\n>\n> Let me know if you want to try this. You can also check, eg the demo\n> demos/pythontococci.cocci to see how to create code in a python script and\n> then use it in a normal SmPL rule.\n> ...\n> If the context that you are interested in is in a called function or is in\n> the calling context, then Coccinelle might not be the ideal choice.\n> Coccinelle works on one function at a time, so to do anything\n> interprocedural, you have to do some hacks.\n\nThank you Julia for this helpful info. Looking at your description, I think\nthe `add check to parse-options.c` (that Ævar suggested as the most ideal\nmethod for it) method is more simpler than this. Moreover,as this is only\nabout checking usage-strings, so adding complexity to it will not be a\ngood idea.\n\nThanks :)\n"},{"id":"449142","messageId":"20220222154700.33928-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"alpine.DEB.2.22.394.2202221436320.2556@hadrien","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-22T15:47:00Z","receivedAt":"2022-02-22T15:49:26Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Julia Lawall wrote:\n\n> Of there are some cases that are useful to do statically, with only local\n> information, then using Coccinelle could be useful to get the problem out\n> of the way once and for all. Coccinelle doesn't support much processing\n> of strings directly, but you can always write some python code to test the\n> contents of a string and to create a new one.\n>\n> Let me know if you want to try this. You can also check, eg the demo\n> demos/pythontococci.cocci to see how to create code in a python script and\n> then use it in a normal SmPL rule.\n> ...\n> If the context that you are interested in is in a called function or is in\n> the calling context, then Coccinelle might not be the ideal choice.\n> Coccinelle works on one function at a time, so to do anything\n> interprocedural, you have to do some hacks.\n\nThough in this case, `parse-options.c check` method is better, but in other\ncases, this might be a good fit. In those cases, I would also like to help\nyou (i.e. you, Johannes, Ævar and other devs) to fix those cases.\n\nThanks :)\n"},{"id":"449147","messageId":"pull.1147.v2.git.1645545507689.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.git.1645030949730.gitgitgadget@gmail.com","subject":"[PATCH v2] add usage-strings check and amend remaining usage strings","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-22T15:58:27Z","receivedAt":"2022-02-22T15:58:36Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"From: Abhra303 \n\nUsage strings for git (sub)command flags has a style guide that\nsuggests - first letter should not capitalized (unless requied)\nand it should skip full-stop at the end of line. But there are\nsome files where usage-strings do not follow the above mentioned\nguide. Moreover, there are no checks to verify if all usage strings\nare following the guide/convention or not.\n\nAmend the usage strings that don't follow the convention/guide and\nadd a check in the `parse_options_check()` function in `parse-options.c`\nto check the usage strings against the style guide.\n\nSigned-off-by: Abhradeep Chakraborty \n---\n add usage-strings ci check and amend remaining usage strings\n \n This patch series completely fixes #636.\n \n The issue is about amending the usage-strings (for command flags such as\n -h, -v etc.) which do not follow the style convention/guide. There was a\n PR [https://github.com/gitgitgadget/git/pull/920] addressing this issue\n but as Johannes [https://github.com/dscho] said in his comment\n [https://github.com/gitgitgadget/git/issues/636#issuecomment-1018660439],\n there are some files that still have those kind of usage strings.\n Johannes also suggested to add a CI check under ci/test-documentation.sh\n to check the usage strings.\n \n So, in this patch, all remaining usage strings are corrected. I also\n added checks to parse_options_check() in parse-options.c (as suggested\n by Ævar).\n \n Changes since v1:\n \n 1. remove check-usage-strings.sh\n 2. remove CI check\n 3. add checks to parse-options.c\n 4. modify t/t1502-rev-parse-parseopt.sh to pass the test\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1147\n\nRange-diff vs v1:\n\n 1: ea0ba45d77a ! 1: 902937e768d add usage-strings ci check and amend remaining usage strings\n @@ Metadata\n Author: Abhra303 \n \n ## Commit message ##\n - add usage-strings ci check and amend remaining usage strings\n + add usage-strings check and amend remaining usage strings\n \n Usage strings for git (sub)command flags has a style guide that\n suggests - first letter should not capitalized (unless requied)\n @@ Commit message\n are following the guide/convention or not.\n \n Amend the usage strings that don't follow the convention/guide and\n - add a `CI` check for checking the usage strings (whether the first\n - letter is capital or it ends with full-stop). If the `check` find\n - such strings then print those strings and return a non-zero status.\n -\n - Also provide a script that takes an optional argument (a valid \n - string), to check the usage strings in the given (`HEAD` is\n - the default argument).\n + add a check in the `parse_options_check()` function in `parse-options.c`\n + to check the usage strings against the style guide.\n \n Signed-off-by: Abhradeep Chakraborty \n \n - ## Makefile ##\n -@@ Makefile: check-docs::\n - check-builtins::\n - \t./check-builtins.sh\n - \n -+### Make sure all the usage strings follow usage string style guide\n -+#\n -+check-usage-strings::\n -+\t./check-usage-strings.sh\n -+\n - ### Test suite coverage testing\n - #\n - .PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report\n -\n ## builtin/bisect--helper.c ##\n @@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n @@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\n \n - ## check-usage-strings.sh (new) ##\n -@@\n -+{\n -+ if test -d \".git\"\n -+ then\n -+ rev=${1:-\"HEAD\"}\n -+ for entry in $(git grep -l 'struct option .* = {$' \"$rev\" -- \\*.c);\n -+ do\n -+ git show \"$entry\" |\n -+ sed -n '/struct option .* = {/,/OPT_END/{=;p;}' |\n -+ sed \"N;s/^\\\\([0-9]*\\\\)\\\\n/$(echo \"$entry\" | sed 's/\\//\\\\&/g'):\\\\1/\";\n -+ done\n -+ else\n -+ for entry in $(grep -rl --include=\"*.c\" 'struct option .* = {$' . );\n -+ do\n -+ cat \"$entry\" |\n -+ sed -n '/struct option .* = {/,/OPT_END/{=;p;}' |\n -+ sed \"N;s/^\\\\([0-9]*\\\\)\\\\n/$(echo \"$entry\" | sed -e 's/\\//\\\\&/g' -e 's/^\\.\\\\\\///'):\\\\1/\";\n -+ done\n -+ fi\n -+} |\n -+grep -Pe '((?type != OPTION_GROUP && opts->help &&\n ++\t\t\t!(starts_with(opts->help, \"HEAD\") ||\n ++\t\t\t starts_with(opts->help, \"GPG\") ||\n ++\t\t\t starts_with(opts->help, \"DEPRECATED\") ||\n ++\t\t\t starts_with(opts->help, \"SHA1\")) &&\n ++\t\t\t (opts->help[0] >= 65 && opts->help[0] <= 90))\n ++\t\t\terr |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n ++\t\tif (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n ++\t\t\terr |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n + \t\tif (opts->argh &&\n + \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n + \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n +\n ## t/helper/test-run-command.c ##\n @@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char **argv)\n \tstruct strbuf out = STRBUF_INIT;\n @@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char *\n \t\tOPT_END()\n \t};\n \tconst char * const usage[] = {\n +\n + ## t/t1502-rev-parse-parseopt.sh ##\n +@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'setup optionspec-only-hidden-switches' '\n + |\n + |some-command does foo and bar!\n + |--\n +-|hidden1* A hidden switch\n ++|hidden1* a hidden switch\n + EOF\n + '\n + \n +@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '\n + |\n + | some-command does foo and bar!\n + |\n +-| --hidden1 A hidden switch\n ++| --hidden1 a hidden switch\n + |\n + |EOF\n + END_EXPECT\n\n\n builtin/bisect--helper.c | 2 +-\n builtin/reflog.c | 6 +++---\n builtin/submodule--helper.c | 2 +-\n diff.c | 2 +-\n parse-options.c | 9 +++++++++\n t/helper/test-run-command.c | 6 +++---\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 7 files changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 28a2e6a5750..614d95b022c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n-\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n+\t\t\t N_(\"use ... to automatically bisect\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 85b838720c3..28372c5e2b5 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_CALLBACK_F(0, \"expire\", &cmd, N_(\"timestamp\"),\n \t\t\t N_(\"prune entries older than the specified time\"),\n \t\t\t PARSE_OPT_NONEG,\n@@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"prune any reflog entries that point to broken commits\")),\n \t\tOPT_BOOL(0, \"all\", &do_all, N_(\"process the reflogs of all references\")),\n \t\tOPT_BOOL(1, \"single-worktree\", &all_worktrees,\n-\t\t\t N_(\"limits processing to reflogs from the current worktree only.\")),\n+\t\t\t N_(\"limits processing to reflogs from the current worktree only\")),\n \t\tOPT_END()\n \t};\n \n@@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 33c82c3ab91..6332d305983 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1875,7 +1875,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n-\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT__QUIET(&quiet, \"suppress output for cloning a submodule\"),\n \t\tOPT_BOOL(0, \"progress\", &progress,\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\ndiff --git a/diff.c b/diff.c\nindex 7d5cfd325ea..387435a4a45 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t N_(\"select files by diff type\"),\n \t\t\t PARSE_OPT_NONEG, diff_opt_diff_filter),\n \t\t{ OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n-\t\t N_(\"Output to a specific file\"),\n+\t\t N_(\"output to a specific file\"),\n \t\t PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n \n \t\tOPT_END()\ndiff --git a/parse-options.c b/parse-options.c\nindex 2437ad3bcdd..91cbfb0d7f7 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -492,6 +492,15 @@ static void parse_options_check(const struct option *opts)\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n+\t\tif (opts->type != OPTION_GROUP && opts->help &&\n+\t\t\t!(starts_with(opts->help, \"HEAD\") ||\n+\t\t\t starts_with(opts->help, \"GPG\") ||\n+\t\t\t starts_with(opts->help, \"DEPRECATED\") ||\n+\t\t\t starts_with(opts->help, \"SHA1\")) &&\n+\t\t\t (opts->help[0] >= 65 && opts->help[0] <= 90))\n+\t\t\terr |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n+\t\tif (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n+\t\t\terr |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 913775a14b7..8f370cd89f1 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct strvec args = STRVEC_INIT;\n \tstruct option options[] = {\n-\t\tOPT_INTEGER('n', \"trials\", &trials, \"Number of trials\"),\n-\t\tOPT_INTEGER('s', \"skip\", &skip, \"Skip trials\"),\n-\t\tOPT_BOOL('m', \"msys2\", &msys2, \"Test quoting for MSYS2's sh\"),\n+\t\tOPT_INTEGER('n', \"trials\", &trials, \"number of trials\"),\n+\t\tOPT_INTEGER('s', \"skip\", &skip, \"skip trials\"),\n+\t\tOPT_BOOL('m', \"msys2\", &msys2, \"test quoting for MSYS2's sh\"),\n \t\tOPT_END()\n \t};\n \tconst char * const usage[] = {\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 284fe18e726..2a07e130b96 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '\n |\n |some-command does foo and bar!\n |--\n-|hidden1* A hidden switch\n+|hidden1* a hidden switch\n EOF\n '\n \n@@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |\n | some-command does foo and bar!\n |\n-| --hidden1 A hidden switch\n+| --hidden1 a hidden switch\n |\n |EOF\n END_EXPECT\n\nbase-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e\n-- \ngitgitgadget\n"},{"id":"449172","messageId":"CAPig+cRq7H2bnkcU-V5uiWA9z=FLvxj3ji0bhO3DMX9HfptHtQ@mail.gmail.com","threadId":"57427","inReplyTo":"pull.1147.v2.git.1645545507689.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] add usage-strings check and amend remaining usage strings","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-02-22T17:16:33Z","receivedAt":"2022-02-22T17:16:48Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 22, 2022 at 11:27 AM Abhradeep Chakraborty via\nGitGitGadget wrote:\n> Usage strings for git (sub)command flags has a style guide that\n> suggests - first letter should not capitalized (unless requied)\n\ns/requied/required/\n\n> and it should skip full-stop at the end of line. But there are\n> some files where usage-strings do not follow the above mentioned\n> guide. Moreover, there are no checks to verify if all usage strings\n> are following the guide/convention or not.\n>\n> Amend the usage strings that don't follow the convention/guide and\n> add a check in the `parse_options_check()` function in `parse-options.c`\n> to check the usage strings against the style guide.\n\nThis is a relatively minor observation, but it might make sense to\nsplit this into two patches, the first of which fixes the offending\nusage strings, and the second which adds the check to parse-options.c\nto prevent more offending strings from entering the project in the\nfuture. Anyhow, not necessarily worth a reroll.\n\n> Signed-off-by: Abhradeep Chakraborty \n> ---\n> diff --git a/parse-options.c b/parse-options.c\n> @@ -492,6 +492,15 @@ static void parse_options_check(const struct option *opts)\n> + if (opts->type != OPTION_GROUP && opts->help &&\n> + !(starts_with(opts->help, \"HEAD\") ||\n> + starts_with(opts->help, \"GPG\") ||\n> + starts_with(opts->help, \"DEPRECATED\") ||\n> + starts_with(opts->help, \"SHA1\")) &&\n> + (opts->help[0] >= 65 && opts->help[0] <= 90))\n> + err |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n\nThis list of hardcoded exceptions may become a maintenance burden. I\ncan figure out why OPTION_GROUP is treated specially here, but why use\nmagic numbers 65 and 90 rather than a more obvious function like\nisupper()?\n\nPerhaps instead of hardcoding an exception list and magic numbers, we\ncan use a simple heuristic instead. For instance, if the first two\ncharacters of the help string are uppercase, then assume it is an\nacronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\nMaybe something like this:\n\n if (opts->type != OPTION_GROUP && opts->help &&\n opts->help[0] && isupper(opts->help[0]) &&\n !(opts->help[1] && isupper(opts->help[1])))\n\n> + if (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n> + err |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n"},{"id":"449237","messageId":"20220223115925.4081-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"CAPig+cRq7H2bnkcU-V5uiWA9z=FLvxj3ji0bhO3DMX9HfptHtQ@mail.gmail.com","subject":"Re: [PATCH v2] add usage-strings check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-23T11:59:25Z","receivedAt":"2022-02-23T11:59:40Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Eric Sunshine wrote:\n\n> s/requied/required/\n\nThanks for pointing out. Will fix it.\n\n> This is a relatively minor observation, but it might make sense to\n> split this into two patches, the first of which fixes the offending\n> usage strings, and the second which adds the check to parse-options.c\n> to prevent more offending strings from entering the project in the\n> future. Anyhow, not necessarily worth a reroll.\n\nI made a single commit because both changes are small. But I think You're\nright - I should split it into two.\n\n> This list of hardcoded exceptions may become a maintenance burden. I\n> can figure out why OPTION_GROUP is treated specially here, but why use\n> magic numbers 65 and 90 rather than a more obvious function like\n> isupper()?\n\nHmm, this was a quick fix came into my mind. So, I didn't look at other\n(and better) options for this. Will fix it :)\n\n> Perhaps instead of hardcoding an exception list and magic numbers, we\n> can use a simple heuristic instead. For instance, if the first two\n> characters of the help string are uppercase, then assume it is an\n> acronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\n> Maybe something like this:\n>\n> if (opts->type != OPTION_GROUP && opts->help &&\n> opts->help[0] && isupper(opts->help[0]) &&\n> !(opts->help[1] && isupper(opts->help[1])))\n\nOkay, got it!\n\nThanks :)\n"},{"id":"449253","messageId":"pull.1147.v3.git.1645626455.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.v2.git.1645545507689.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-23T14:27:33Z","receivedAt":"2022-02-23T14:27:41Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"This patch series completely fixes #636.\n\nThe issue is about amending the usage-strings (for command flags such as -h,\n-v etc.) which do not follow the style convention/guide. There was a PR\n[https://github.com/gitgitgadget/git/pull/920] addressing this issue but as\nJohannes [https://github.com/dscho] said in his comment\n[https://github.com/gitgitgadget/git/issues/636#issuecomment-1018660439],\nthere are some files that still have those kind of usage strings. Johannes\nalso suggested to add a CI check under ci/test-documentation.sh to check the\nusage strings.\n\nIn this version, the previously single commit is split into two commits (\none addressing amending of usage strings and another is for adding the style\nchecks to parse_options_check()) and the checks are simplified.\n\nChanges since v1:\n\n 1. remove check-usage-strings.sh\n 2. remove CI check\n 3. add checks to parse-options.c\n 4. modify t/t1502-rev-parse-parseopt.sh to pass the test\n\nUntil v1:\n\nA shell script check-usage-strings.sh was introduced to check the\nusage-strings. CI check for the same was also introduced.\n\nAbhra303 (1):\n amend remaining usage strings according to style guide\n\nAbhradeep Chakraborty (1):\n parse-options.c: add style checks for usage-strings\n\n builtin/bisect--helper.c | 2 +-\n builtin/reflog.c | 6 +++---\n builtin/submodule--helper.c | 2 +-\n diff.c | 2 +-\n parse-options.c | 6 ++++++\n t/helper/test-run-command.c | 6 +++---\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 7 files changed, 17 insertions(+), 11 deletions(-)\n\n\nbase-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1147\n\nRange-diff vs v2:\n\n 1: 902937e768d ! 1: f425e36b7ea add usage-strings check and amend remaining usage strings\n @@ Metadata\n Author: Abhra303 \n \n ## Commit message ##\n - add usage-strings check and amend remaining usage strings\n + amend remaining usage strings according to style guide\n \n Usage strings for git (sub)command flags has a style guide that\n - suggests - first letter should not capitalized (unless requied)\n + suggests - first letter should not capitalized (unless required)\n and it should skip full-stop at the end of line. But there are\n some files where usage-strings do not follow the above mentioned\n - guide. Moreover, there are no checks to verify if all usage strings\n - are following the guide/convention or not.\n + guide.\n \n - Amend the usage strings that don't follow the convention/guide and\n - add a check in the `parse_options_check()` function in `parse-options.c`\n - to check the usage strings against the style guide.\n + Amend the usage strings that don't follow the style convention/guide.\n \n Signed-off-by: Abhradeep Chakraborty \n \n @@ diff.c: static void prep_parse_options(struct diff_options *options)\n \n \t\tOPT_END()\n \n - ## parse-options.c ##\n -@@ parse-options.c: static void parse_options_check(const struct option *opts)\n - \t\tdefault:\n - \t\t\t; /* ok. (usually accepts an argument) */\n - \t\t}\n -+\t\tif (opts->type != OPTION_GROUP && opts->help &&\n -+\t\t\t!(starts_with(opts->help, \"HEAD\") ||\n -+\t\t\t starts_with(opts->help, \"GPG\") ||\n -+\t\t\t starts_with(opts->help, \"DEPRECATED\") ||\n -+\t\t\t starts_with(opts->help, \"SHA1\")) &&\n -+\t\t\t (opts->help[0] >= 65 && opts->help[0] <= 90))\n -+\t\t\terr |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n -+\t\tif (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n -+\t\t\terr |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n - \t\tif (opts->argh &&\n - \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n - \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n -\n ## t/helper/test-run-command.c ##\n @@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char **argv)\n \tstruct strbuf out = STRBUF_INIT;\n @@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char *\n \t\tOPT_END()\n \t};\n \tconst char * const usage[] = {\n -\n - ## t/t1502-rev-parse-parseopt.sh ##\n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'setup optionspec-only-hidden-switches' '\n - |\n - |some-command does foo and bar!\n - |--\n --|hidden1* A hidden switch\n -+|hidden1* a hidden switch\n - EOF\n - '\n - \n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '\n - |\n - | some-command does foo and bar!\n - |\n --| --hidden1 A hidden switch\n -+| --hidden1 a hidden switch\n - |\n - |EOF\n - END_EXPECT\n -: ----------- > 2: 9d42bdbff6c parse-options.c: add style checks for usage-strings\n\n-- \ngitgitgadget\n"},{"id":"449254","messageId":"f425e36b7ea4a310a8ad93d47ead4c1713117388.1645626455.git.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.v3.git.1645626455.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] amend remaining usage strings according to style guide","fromName":"Abhra303 via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-23T14:27:34Z","receivedAt":"2022-02-23T14:27:43Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"From: Abhra303 \n\nUsage strings for git (sub)command flags has a style guide that\nsuggests - first letter should not capitalized (unless required)\nand it should skip full-stop at the end of line. But there are\nsome files where usage-strings do not follow the above mentioned\nguide.\n\nAmend the usage strings that don't follow the style convention/guide.\n\nSigned-off-by: Abhradeep Chakraborty \n---\n builtin/bisect--helper.c | 2 +-\n builtin/reflog.c | 6 +++---\n builtin/submodule--helper.c | 2 +-\n diff.c | 2 +-\n t/helper/test-run-command.c | 6 +++---\n 5 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 28a2e6a5750..614d95b022c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n-\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n+\t\t\t N_(\"use ... to automatically bisect\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 85b838720c3..28372c5e2b5 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_CALLBACK_F(0, \"expire\", &cmd, N_(\"timestamp\"),\n \t\t\t N_(\"prune entries older than the specified time\"),\n \t\t\t PARSE_OPT_NONEG,\n@@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"prune any reflog entries that point to broken commits\")),\n \t\tOPT_BOOL(0, \"all\", &do_all, N_(\"process the reflogs of all references\")),\n \t\tOPT_BOOL(1, \"single-worktree\", &all_worktrees,\n-\t\t\t N_(\"limits processing to reflogs from the current worktree only.\")),\n+\t\t\t N_(\"limits processing to reflogs from the current worktree only\")),\n \t\tOPT_END()\n \t};\n \n@@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 33c82c3ab91..6332d305983 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1875,7 +1875,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n-\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT__QUIET(&quiet, \"suppress output for cloning a submodule\"),\n \t\tOPT_BOOL(0, \"progress\", &progress,\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\ndiff --git a/diff.c b/diff.c\nindex 7d5cfd325ea..387435a4a45 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t N_(\"select files by diff type\"),\n \t\t\t PARSE_OPT_NONEG, diff_opt_diff_filter),\n \t\t{ OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n-\t\t N_(\"Output to a specific file\"),\n+\t\t N_(\"output to a specific file\"),\n \t\t PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n \n \t\tOPT_END()\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 913775a14b7..8f370cd89f1 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct strvec args = STRVEC_INIT;\n \tstruct option options[] = {\n-\t\tOPT_INTEGER('n', \"trials\", &trials, \"Number of trials\"),\n-\t\tOPT_INTEGER('s', \"skip\", &skip, \"Skip trials\"),\n-\t\tOPT_BOOL('m', \"msys2\", &msys2, \"Test quoting for MSYS2's sh\"),\n+\t\tOPT_INTEGER('n', \"trials\", &trials, \"number of trials\"),\n+\t\tOPT_INTEGER('s', \"skip\", &skip, \"skip trials\"),\n+\t\tOPT_BOOL('m', \"msys2\", &msys2, \"test quoting for MSYS2's sh\"),\n \t\tOPT_END()\n \t};\n \tconst char * const usage[] = {\n-- \ngitgitgadget\n\n"},{"id":"449255","messageId":"9d42bdbff6ccaaf34952de9e6cc4ff2e7eef714d.1645626455.git.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.v3.git.1645626455.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-23T14:27:35Z","receivedAt":"2022-02-23T14:27:46Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"From: Abhradeep Chakraborty \n\n`parse-options.c` doesn't check if the usage strings for option flags\nare following the style guide or not. Style convention says, usage\nstrings should not start with capital letter (unless needed) and\nit should not end with `.`.\n\nAdd checks to the `parse_options_check()` function to check usage\nstrings against the style convention.\n\nSigned-off-by: Abhradeep Chakraborty \n---\n parse-options.c | 6 ++++++\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 2 files changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2437ad3bcdd..eb92290a63a 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -492,6 +492,12 @@ static void parse_options_check(const struct option *opts)\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n+\t\tif (opts->type != OPTION_GROUP && opts->help &&\n+\t\t\topts->help[0] && isupper(opts->help[0]) &&\n+\t\t\t!(opts->help[1] && isupper(opts->help[1])))\n+\t\t\terr |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n+\t\tif (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n+\t\t\terr |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 284fe18e726..2a07e130b96 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '\n |\n |some-command does foo and bar!\n |--\n-|hidden1* A hidden switch\n+|hidden1* a hidden switch\n EOF\n '\n \n@@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |\n | some-command does foo and bar!\n |\n-| --hidden1 A hidden switch\n+| --hidden1 a hidden switch\n |\n |EOF\n END_EXPECT\n-- \ngitgitgadget\n"},{"id":"449341","messageId":"xmqqilt5th8s.fsf@gitster.g","threadId":"57427","inReplyTo":"CAPig+cRq7H2bnkcU-V5uiWA9z=FLvxj3ji0bhO3DMX9HfptHtQ@mail.gmail.com","subject":"Re: [PATCH v2] add usage-strings check and amend remaining usage strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-23T21:17:39Z","receivedAt":"2022-02-23T21:17:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> This is a relatively minor observation, but it might make sense to\n> split this into two patches, the first of which fixes the offending\n> usage strings, and the second which adds the check to parse-options.c\n> to prevent more offending strings from entering the project in the\n> future.\n\nYeah, that sounds like a quite sensible split.\n\nI notice that the real-looking name\n\n>> Signed-off-by: Abhradeep Chakraborty \n\ndoes not match with the in-body \"From:\" that has less real-looking.\nPlease fix the in-body \"From:\" if this is rerolled so that both\nmention the same \"Human Readable Name \".\n\n>> ---\n>> diff --git a/parse-options.c b/parse-options.c\n>> @@ -492,6 +492,15 @@ static void parse_options_check(const struct option *opts)\n>> + if (opts->type != OPTION_GROUP && opts->help &&\n>> + !(starts_with(opts->help, \"HEAD\") ||\n>> + starts_with(opts->help, \"GPG\") ||\n>> + starts_with(opts->help, \"DEPRECATED\") ||\n>> + starts_with(opts->help, \"SHA1\")) &&\n>> + (opts->help[0] >= 65 && opts->help[0] <= 90))\n>> + err |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n>\n> This list of hardcoded exceptions may become a maintenance burden. I\n> can figure out why OPTION_GROUP is treated specially here, but why use\n> magic numbers 65 and 90 rather than a more obvious function like\n> isupper()?\n>\n> Perhaps instead of hardcoding an exception list and magic numbers, we\n> can use a simple heuristic instead. For instance, if the first two\n> characters of the help string are uppercase, then assume it is an\n> acronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\n> Maybe something like this:\n>\n> if (opts->type != OPTION_GROUP && opts->help &&\n> opts->help[0] && isupper(opts->help[0]) &&\n> !(opts->help[1] && isupper(opts->help[1])))\n>\n\nMuch better than what was posted, but such a heuristic deserves some\nin-code comment to check why we see the first two.\n\n>> + if (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n>> + err |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n"},{"id":"449342","messageId":"CAPig+cRUqtr3jgtL6t_fdm8T7mj1vZ3ONK8onvhv8aqqY87rLg@mail.gmail.com","threadId":"57427","inReplyTo":"xmqqilt5th8s.fsf@gitster.g","subject":"Re: [PATCH v2] add usage-strings check and amend remaining usage strings","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-02-23T21:20:49Z","receivedAt":"2022-02-23T21:21:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Feb 23, 2022 at 4:17 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> >> + if (opts->type != OPTION_GROUP && opts->help &&\n> >> + !(starts_with(opts->help, \"HEAD\") ||\n> >> + starts_with(opts->help, \"GPG\") ||\n> >> + starts_with(opts->help, \"DEPRECATED\") ||\n> >> + starts_with(opts->help, \"SHA1\")) &&\n> >> + (opts->help[0] >= 65 && opts->help[0] <= 90))\n> >\n> > This list of hardcoded exceptions may become a maintenance burden. I\n> > can figure out why OPTION_GROUP is treated specially here, but why use\n> > magic numbers 65 and 90 rather than a more obvious function like\n> > isupper()?\n> >\n> > Perhaps instead of hardcoding an exception list and magic numbers, we\n> > can use a simple heuristic instead. For instance, if the first two\n> > characters of the help string are uppercase, then assume it is an\n> > acronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\n> > Maybe something like this:\n> >\n> > if (opts->type != OPTION_GROUP && opts->help &&\n> > opts->help[0] && isupper(opts->help[0]) &&\n> > !(opts->help[1] && isupper(opts->help[1])))\n>\n> Much better than what was posted, but such a heuristic deserves some\n> in-code comment to check why we see the first two.\n\nYes, I had the same thought as soon as I walked away from the computer\nand was going to post a follow-up email to say as much but got\ndistracted by other things and never got around to it. Thanks for\nfilling in the gap.\n"},{"id":"449394","messageId":"20220224062604.1264-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqilt5th8s.fsf@gitster.g","subject":"Re: [PATCH v2] add usage-strings check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-24T06:26:04Z","receivedAt":"2022-02-24T06:26:18Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Junio C Hamano wrote:\n\n> I notice that the real-looking name\n>\n>>> Signed-off-by: Abhradeep Chakraborty \n>\n> does not match with the in-body \"From:\" that has less real-looking.\n> Please fix the in-body \"From:\" if this is rerolled so that both\n> mention the same \"Human Readable Name \".\n\nOkay, fixing it. Unfortunately your review came after sending the third\nversion[1], so this is not fixed in the newest version. But could you\nreview the newest version of this patch series so that I can make all the\nsuggested changes in one go?\n\n> Much better than what was posted, but such a heuristic deserves some\n> in-code comment to check why we see the first two.\n\nOkay, will add comments to it.\n\nThanks :)\n\n[1] https://lore.kernel.org/git/pull.1147.v3.git.1645626455.gitgitgadget@gmail.com/\n"},{"id":"449545","messageId":"pull.1147.v4.git.1645766599.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.v3.git.1645626455.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T05:23:17Z","receivedAt":"2022-02-25T05:23:40Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"This patch series completely fixes #636.\n\nThe issue is about amending the usage-strings (for command flags such as -h,\n-v etc.) which do not follow the style convention/guide. There was a PR\n[https://github.com/gitgitgadget/git/pull/920] addressing this issue but as\nJohannes [https://github.com/dscho] said in his comment\n[https://github.com/gitgitgadget/git/issues/636#issuecomment-1018660439],\nthere are some files that still have those kind of usage strings. Johannes\nalso suggested to add a CI check under ci/test-documentation.sh to check the\nusage strings.\n\nIn this version, comments added and the From field of the first commit\nmessage is updated (i.e. \"Abhradeep Chakraborty\" instead of \"Abhra303\")\n\nChanges since v2:\n\n 1. split the single commit into two logically separated commits ( one\n addressing amending of usage strings and another is for adding the style\n checks to parse_options_check())\n 2. the checks are simplified.\n\nChanges since v1:\n\n 1. remove check-usage-strings.sh\n 2. remove CI check\n 3. add checks to parse-options.c\n 4. modify t/t1502-rev-parse-parseopt.sh to pass the test\n\nUntil v1:\n\nA shell script check-usage-strings.sh was introduced to check the\nusage-strings. CI check for the same was also introduced.\n\nAbhradeep Chakraborty (2):\n amend remaining usage strings according to style guide\n parse-options.c: add style checks for usage-strings\n\n builtin/bisect--helper.c | 2 +-\n builtin/reflog.c | 6 +++---\n builtin/submodule--helper.c | 2 +-\n diff.c | 2 +-\n parse-options.c | 11 +++++++++++\n t/helper/test-run-command.c | 6 +++---\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 7 files changed, 22 insertions(+), 11 deletions(-)\n\n\nbase-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1147\n\nRange-diff vs v3:\n\n 1: f425e36b7ea ! 1: dc200d098ae amend remaining usage strings according to style guide\n @@\n ## Metadata ##\n -Author: Abhra303 \n +Author: Abhradeep Chakraborty \n \n ## Commit message ##\n amend remaining usage strings according to style guide\n 2: 9d42bdbff6c ! 2: e1c5a325826 parse-options.c: add style checks for usage-strings\n @@ parse-options.c: static void parse_options_check(const struct option *opts)\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n ++\n ++\t\t// OPTION_GROUP should be ignored\n ++\t\t// if the first two characters of the help string are uppercase, then assume it is an\n ++\t\t// acronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\n ++\t\t// else assume the usage string is violating the style convention and throw error.\n +\t\tif (opts->type != OPTION_GROUP && opts->help &&\n +\t\t\topts->help[0] && isupper(opts->help[0]) &&\n +\t\t\t!(opts->help[1] && isupper(opts->help[1])))\n\n-- \ngitgitgadget\n"},{"id":"449546","messageId":"dc200d098aefc0a66d6bfc304697a66f6904cc11.1645766599.git.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.v4.git.1645766599.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] amend remaining usage strings according to style guide","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T05:23:18Z","receivedAt":"2022-02-25T05:23:42Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"From: Abhradeep Chakraborty \n\nUsage strings for git (sub)command flags has a style guide that\nsuggests - first letter should not capitalized (unless required)\nand it should skip full-stop at the end of line. But there are\nsome files where usage-strings do not follow the above mentioned\nguide.\n\nAmend the usage strings that don't follow the style convention/guide.\n\nSigned-off-by: Abhradeep Chakraborty \n---\n builtin/bisect--helper.c | 2 +-\n builtin/reflog.c | 6 +++---\n builtin/submodule--helper.c | 2 +-\n diff.c | 2 +-\n t/helper/test-run-command.c | 6 +++---\n 5 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c\nindex 28a2e6a5750..614d95b022c 100644\n--- a/builtin/bisect--helper.c\n+++ b/builtin/bisect--helper.c\n@@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix)\n \t\tOPT_CMDMODE(0, \"bisect-visualize\", &cmdmode,\n \t\t\t N_(\"visualize the bisection\"), BISECT_VISUALIZE),\n \t\tOPT_CMDMODE(0, \"bisect-run\", &cmdmode,\n-\t\t\t N_(\"use ... to automatically bisect.\"), BISECT_RUN),\n+\t\t\t N_(\"use ... to automatically bisect\"), BISECT_RUN),\n \t\tOPT_BOOL(0, \"no-log\", &nolog,\n \t\t\t N_(\"no log for BISECT_WRITE\")),\n \t\tOPT_END()\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 85b838720c3..28372c5e2b5 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_CALLBACK_F(0, \"expire\", &cmd, N_(\"timestamp\"),\n \t\t\t N_(\"prune entries older than the specified time\"),\n \t\t\t PARSE_OPT_NONEG,\n@@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"prune any reflog entries that point to broken commits\")),\n \t\tOPT_BOOL(0, \"all\", &do_all, N_(\"process the reflogs of all references\")),\n \t\tOPT_BOOL(1, \"single-worktree\", &all_worktrees,\n-\t\t\t N_(\"limits processing to reflogs from the current worktree only.\")),\n+\t\t\t N_(\"limits processing to reflogs from the current worktree only\")),\n \t\tOPT_END()\n \t};\n \n@@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix)\n \t\tOPT_BIT(0, \"updateref\", &flags,\n \t\t\tN_(\"update the reference to the value of the top reflog entry\"),\n \t\t\tEXPIRE_REFLOGS_UPDATE_REF),\n-\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen.\")),\n+\t\tOPT_BOOL(0, \"verbose\", &verbose, N_(\"print extra information on screen\")),\n \t\tOPT_END()\n \t};\n \ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 33c82c3ab91..6332d305983 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1875,7 +1875,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)\n \t\tOPT_STRING(0, \"depth\", &clone_data.depth,\n \t\t\t N_(\"string\"),\n \t\t\t N_(\"depth for shallow clones\")),\n-\t\tOPT__QUIET(&quiet, \"Suppress output for cloning a submodule\"),\n+\t\tOPT__QUIET(&quiet, \"suppress output for cloning a submodule\"),\n \t\tOPT_BOOL(0, \"progress\", &progress,\n \t\t\t N_(\"force cloning progress\")),\n \t\tOPT_BOOL(0, \"require-init\", &require_init,\ndiff --git a/diff.c b/diff.c\nindex 7d5cfd325ea..387435a4a45 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)\n \t\t\t N_(\"select files by diff type\"),\n \t\t\t PARSE_OPT_NONEG, diff_opt_diff_filter),\n \t\t{ OPTION_CALLBACK, 0, \"output\", options, N_(\"\"),\n-\t\t N_(\"Output to a specific file\"),\n+\t\t N_(\"output to a specific file\"),\n \t\t PARSE_OPT_NONEG, NULL, 0, diff_opt_output },\n \n \t\tOPT_END()\ndiff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c\nindex 913775a14b7..8f370cd89f1 100644\n--- a/t/helper/test-run-command.c\n+++ b/t/helper/test-run-command.c\n@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct strvec args = STRVEC_INIT;\n \tstruct option options[] = {\n-\t\tOPT_INTEGER('n', \"trials\", &trials, \"Number of trials\"),\n-\t\tOPT_INTEGER('s', \"skip\", &skip, \"Skip trials\"),\n-\t\tOPT_BOOL('m', \"msys2\", &msys2, \"Test quoting for MSYS2's sh\"),\n+\t\tOPT_INTEGER('n', \"trials\", &trials, \"number of trials\"),\n+\t\tOPT_INTEGER('s', \"skip\", &skip, \"skip trials\"),\n+\t\tOPT_BOOL('m', \"msys2\", &msys2, \"test quoting for MSYS2's sh\"),\n \t\tOPT_END()\n \t};\n \tconst char * const usage[] = {\n-- \ngitgitgadget\n\n"},{"id":"449547","messageId":"e1c5a3258263d05530f236c247603c2f342dac85.1645766599.git.gitgitgadget@gmail.com","threadId":"57427","inReplyTo":"pull.1147.v4.git.1645766599.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-02-25T05:23:19Z","receivedAt":"2022-02-25T05:23:44Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"From: Abhradeep Chakraborty \n\n`parse-options.c` doesn't check if the usage strings for option flags\nare following the style guide or not. Style convention says, usage\nstrings should not start with capital letter (unless needed) and\nit should not end with `.`.\n\nAdd checks to the `parse_options_check()` function to check usage\nstrings against the style convention.\n\nSigned-off-by: Abhradeep Chakraborty \n---\n parse-options.c | 11 +++++++++++\n t/t1502-rev-parse-parseopt.sh | 4 ++--\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2437ad3bcdd..acd9ddbb372 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -492,6 +492,17 @@ static void parse_options_check(const struct option *opts)\n \t\tdefault:\n \t\t\t; /* ok. (usually accepts an argument) */\n \t\t}\n+\n+\t\t// OPTION_GROUP should be ignored\n+\t\t// if the first two characters of the help string are uppercase, then assume it is an\n+\t\t// acronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\n+\t\t// else assume the usage string is violating the style convention and throw error.\n+\t\tif (opts->type != OPTION_GROUP && opts->help &&\n+\t\t\topts->help[0] && isupper(opts->help[0]) &&\n+\t\t\t!(opts->help[1] && isupper(opts->help[1])))\n+\t\t\terr |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n+\t\tif (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n+\t\t\terr |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n \t\tif (opts->argh &&\n \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 284fe18e726..2a07e130b96 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '\n |\n |some-command does foo and bar!\n |--\n-|hidden1* A hidden switch\n+|hidden1* a hidden switch\n EOF\n '\n \n@@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |\n | some-command does foo and bar!\n |\n-| --hidden1 A hidden switch\n+| --hidden1 a hidden switch\n |\n |EOF\n END_EXPECT\n-- \ngitgitgadget\n"},{"id":"449548","messageId":"xmqqh78nh3sf.fsf@gitster.g","threadId":"57427","inReplyTo":"e1c5a3258263d05530f236c247603c2f342dac85.1645766599.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T06:13:36Z","receivedAt":"2022-02-25T06:13:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Abhradeep Chakraborty via GitGitGadget\" \nwrites:\n\n> +\n> +\t\t// OPTION_GROUP should be ignored\n> +\t\t// if the first two characters of the help string are uppercase, then assume it is an\n> +\t\t// acronym (i.e. \"GPG\") or special name (i.e. \"HEAD\"), thus allowed.\n> +\t\t// else assume the usage string is violating the style convention and throw error.\n\nStyle.\n\n\t/*\n\t * This is how our multi-line comments\n * look like; with slash-asterisk that opens\n * and asterisk-slash that closes one on their\n * own lines.\n\t */\n\nAlso avoid overly long lines.\n\n> +\t\tif (opts->type != OPTION_GROUP && opts->help &&\n> +\t\t\topts->help[0] && isupper(opts->help[0]) &&\n> +\t\t\t!(opts->help[1] && isupper(opts->help[1])))\n> +\t\t\terr |= optbug(opts, xstrfmt(\"help should not start with capital letter unless needed: %s\", opts->help));\n> +\t\tif (opts->help && !ends_with(opts->help, \"...\") && ends_with(opts->help, \".\"))\n> +\t\t\terr |= optbug(opts, xstrfmt(\"help should not end with a dot: %s\", opts->help));\n\nThese two calls to optbug() use xstrfmt() to grab allocated pieces\nof memory and pass it as a parameter to the function, which means\nthe string is leaked without any chance to be freed.\n\nDo we care?\n\n> \t\tif (opts->argh &&\n> \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n> \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n\nThe existing use of optbug() we see here does not share such a\nproblem.\n"},{"id":"449553","messageId":"20220225080811.8097-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqh78nh3sf.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-25T08:08:11Z","receivedAt":"2022-02-25T08:08:25Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Junio C Hamano wrote:\n\n> Style.\n>\n>\t/*\n> * This is how our multi-line comments\n> * look like; with slash-asterisk that opens\n> * and asterisk-slash that closes one on their\n> * own lines.\n>\t */\n>\n> Also avoid overly long lines.\n\nOh, sorry for that. I was in kind of a hurry ( today was\nmy semester exam), so I didn't look at the style guide.\nWill fix it.\n\n> These two calls to optbug() use xstrfmt() to grab allocated pieces\n> of memory and pass it as a parameter to the function, which means\n> the string is leaked without any chance to be freed.\n>\n> Do we care?\n>\n> > \t\tif (opts->argh &&\n> > \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n> > \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n>\n> The existing use of optbug() we see here does not share such a\n> problem.\n\nhmm, I wanted a formatting function to format (i.e. pass the\n`opt->help` dynamically) the output string. The existing use of\n`optbug()` that you specified has no `%s` formatter; it is a plain\nstring. That's why I used `xstrfmt()`. Moreover, it was in Ævar's\nsuggestion[1] -\n\n> +\t\tif (opts->help && ends_with(opts->help, \".\"))\n> +\t\t\terr |= optbug(opts, xstrfmt(\"argh should not end with a dot: %s\", opts->help));\n\nBut I think, you're right. There is some memory leakage here.\nShould I go with plain strings then? (i.e. \"help should not end\nwith a dot\" instead of `xstrfmt(\"help should not end with a dot:\n%s\", opts->help)`)\n\nThanks :)\n\n[1] https://lore.kernel.org/git/220221.86tucsb4oy.gmgdl@evledraar.gmail.com/\n"},{"id":"449584","messageId":"nycvar.QRO.7.76.6.2202251601040.11118@tvgsbejvaqbjf.bet","threadId":"57427","inReplyTo":"alpine.DEB.2.22.394.2202221436320.2556@hadrien","subject":"Re: [cocci] [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-25T15:03:55Z","receivedAt":"2022-02-25T15:04:07Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Julia,\n\nOn Tue, 22 Feb 2022, Julia Lawall wrote:\n\n> [I]f there are some cases that are useful to do statically, with only\n> local information, then using Coccinelle could be useful to get the\n> problem out of the way once and for all. Coccinelle doesn't support\n> much processing of strings directly, but you can always write some\n> python code to test the contents of a string and to create a new one.\n>\n> Let me know if you want to try this. You can also check, eg the demo\n> demos/pythontococci.cocci to see how to create code in a python script and\n> then use it in a normal SmPL rule.\n>\n> If some context has to be taken into account and the context in the same\n> function, then that can also be done with Coccinelle, eg\n>\n> A\n> ...\n> B\n>\n> matches the case where after an A there is a B on all execution paths\n> (except perhaps those that end in an error exit) and\n>\n> A\n> ... when exists\n> B\n>\n> matches the case where there is a B sometime after executing A, even if\n> that does not always occur.\n>\n> If the context that you are interested in is in a called function or is in\n> the calling context, then Coccinelle might not be the ideal choice.\n> Coccinelle works on one function at a time, so to do anything\n> interprocedural, you have to do some hacks.\n\nRight. The code in question is not actually calling a function, but a\nmacro, and passes a literal string to the macro that we would want to\ncheck statically.\n\nI did have my doubts that it would be easy with Coccinelle, but since Ævar\nseemed so confident, I tried it, struggled, and decided to follow up with\nyou.\n\nThank you for confirming my suspicion!\nJohannes\n"},{"id":"449586","messageId":"nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet","threadId":"57427","inReplyTo":"20220222154700.33928-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-25T15:30:35Z","receivedAt":"2022-02-25T15:30:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 22 Feb 2022, Abhradeep Chakraborty wrote:\n\n> Julia Lawall wrote:\n>\n> > Of there are some cases that are useful to do statically, with only local\n> > information, then using Coccinelle could be useful to get the problem out\n> > of the way once and for all. Coccinelle doesn't support much processing\n> > of strings directly, but you can always write some python code to test the\n> > contents of a string and to create a new one.\n> >\n> > Let me know if you want to try this. You can also check, eg the demo\n> > demos/pythontococci.cocci to see how to create code in a python script and\n> > then use it in a normal SmPL rule.\n> > ...\n> > If the context that you are interested in is in a called function or is in\n> > the calling context, then Coccinelle might not be the ideal choice.\n> > Coccinelle works on one function at a time, so to do anything\n> > interprocedural, you have to do some hacks.\n>\n> Though in this case, `parse-options.c check` method is better [...]\n\nI fear that this is incorrect.\n\nIn general, it is my experience that it is a mistake any time a static\ncheck is replaced by a runtime check.\n\nI was ready to let it slide in this instance, but in this case I now have\nproof that the `parse-options.c` check is worse than the originally\nsuggested `sed` chain.\n\nThat concrete proof is in the output of\nhttps://github.com/git/git/actions/runs/1890665968, where the combination\nof `ac/usage-string-fixups` and `jh/builtin-fsmonitor-part2` causes many,\nmany failures, but all of those failures have the same root cause: the\nruntime check.\n\nWith the original `check-usage-strings.sh`, the user inspecting any\nfailure would see precisely what the issue is, in the `static-analysis`\njob's logs. It would display something like this:\n\n\tHEAD:builtin/fsmonitor--daemon.c:1507: N_(\"Max seconds to wait for background daemon startup\")),\n\nWith v4 of the patch series, it does not spell out anything in\n`static-analysis`. Instead, it causes 8 separate jobs to fail,\nit causes failures not only in `t0012-help.sh` but also in\n`t7519-status-fsmonitor.sh` and in `t7527-builtin-fsmonitor.sh`.\n\nThe purpose of t7519 and t7527 is _not_ to verify those usage strings,\nthough.\n\nThe worst part? Look at the relevant output of t0012 (see\nhttps://github.com/git/git/runs/5312844492?check_suite_focus=true#step:5:4902):\n\n\t[...]\n\t++ git -C sub fsmonitor--daemon -h\n\t++ exit_code=128\n\t++ test 128 = 129\n\t++ echo 'test_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h'\n\ttest_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h\n\t++ return 1\n\terror: last command exited with $?=1\n\tnot ok 81 - fsmonitor--daemon can handle -h\n\t[...]\n\nDo you see what usage string caused the failure? You can't. And that's\neven by design:\n\n\t(\n\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n\t\texport GIT_CEILING_DIRECTORIES &&\n\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n\t) &&\n\ttest_i18ngrep usage output\n\nThe output is redirected, and since the runtime check added to\n`parse-options.c` causes the exit code to be different from the expected\none, the output is never shown.\n\nArguably the most important job of a regression test is to help software\nengineers to diagnose and fix the regression. As quickly and as\nconveniently as possible. That means that it is not enough to point out\nthat there is a regression, the output should be as helpful and concise as\npossible to facilitate fixing the problem.\n\nIn the above-mentioned case, it was neither as helpful nor as concise as\npossible because in the test case that was supposed to identify the\nproblem, the actual error message was swallowed, and instead of causing\none test failure, it caused a whopping 42 test cases to fail (some of\nwhich even show the error message, but that's not even the purpose of\nthose test cases).\n\nSince the entire point of this here patch series is to help enforce Git's\nrules regarding usage strings, it should expect things like the issue with\n`fsmonitor--daemon` _and_ make it as painless to address such an issue.\n\nI am afraid that I have to NAK the `parse-options.c` approach because v1\nof this patch series did so much better a job.\n\nCiao,\nDscho\n"},{"id":"449587","messageId":"alpine.DEB.2.22.394.2202251630510.2577@hadrien","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2202251601040.11118@tvgsbejvaqbjf.bet","subject":"Re: [cocci] [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Julia Lawall","fromEmail":"julia.lawall@inria.fr","sentAt":"2022-02-25T15:36:04Z","receivedAt":"2022-02-25T15:36:11Z","isPatch":true,"sender":{"key":"julia.lawall@inria.fr","avatar":null},"body":"\n\nOn Fri, 25 Feb 2022, Johannes Schindelin wrote:\n\n> Hi Julia,\n>\n> On Tue, 22 Feb 2022, Julia Lawall wrote:\n>\n> > [I]f there are some cases that are useful to do statically, with only\n> > local information, then using Coccinelle could be useful to get the\n> > problem out of the way once and for all. Coccinelle doesn't support\n> > much processing of strings directly, but you can always write some\n> > python code to test the contents of a string and to create a new one.\n> >\n> > Let me know if you want to try this. You can also check, eg the demo\n> > demos/pythontococci.cocci to see how to create code in a python script and\n> > then use it in a normal SmPL rule.\n> >\n> > If some context has to be taken into account and the context in the same\n> > function, then that can also be done with Coccinelle, eg\n> >\n> > A\n> > ...\n> > B\n> >\n> > matches the case where after an A there is a B on all execution paths\n> > (except perhaps those that end in an error exit) and\n> >\n> > A\n> > ... when exists\n> > B\n> >\n> > matches the case where there is a B sometime after executing A, even if\n> > that does not always occur.\n> >\n> > If the context that you are interested in is in a called function or is in\n> > the calling context, then Coccinelle might not be the ideal choice.\n> > Coccinelle works on one function at a time, so to do anything\n> > interprocedural, you have to do some hacks.\n>\n> Right. The code in question is not actually calling a function, but a\n> macro, and passes a literal string to the macro that we would want to\n> check statically.\n\nCoccinelle doesn't care about whether a function is called or whether a\nmacro is called. It considers everything to be a function.\n\n>\n> I did have my doubts that it would be easy with Coccinelle, but since Ævar\n> seemed so confident, I tried it, struggled, and decided to follow up with\n> you.\n\nSomething like this:\n\n@r1@\nexpression e;\n@@\n\nN(e)\n\n@script:python s@\ne << r1.e;\nreplacement;\n@@\n\nif string_ok e\nthen cocci.include_match(False)\nelse coccinelle.replacement = \"\\\"better string\\\"\"\n\n@@\nexpression r1.e;\nexpression s.replacement;\n@@\n- N(e)\n+ N(replacement)\n\n------------------\n\nYou can fill in the definition of string_ok and better string. I think\nthe \\\" will be necessary, because the value of an expression metavariable\nat the python level is a string, so there should be a string inside of it\nto make it a string expression.\n\njulia"},{"id":"449588","messageId":"nycvar.QRO.7.76.6.2202251632320.11118@tvgsbejvaqbjf.bet","threadId":"57427","inReplyTo":"e1c5a3258263d05530f236c247603c2f342dac85.1645766599.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-02-25T15:36:30Z","receivedAt":"2022-02-25T15:36:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Abhradeep,\n\nOn Fri, 25 Feb 2022, Abhradeep Chakraborty via GitGitGadget wrote:\n\n> From: Abhradeep Chakraborty \n>\n> `parse-options.c` doesn't check if the usage strings for option flags\n> are following the style guide or not. Style convention says, usage\n> strings should not start with capital letter (unless needed) and\n> it should not end with `.`.\n>\n> Add checks to the `parse_options_check()` function to check usage\n> strings against the style convention.\n\nAs I just pointed out in\nhttps://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet/,\nit seems that replacing the static check presented in v1 by a runtime\ncheck needs to be reverted.\n\nIn addition to the example I presented, there is another compelling reason\nto do so: with the static check, we can detect incorrect usage strings in\nall code, even in code that is platform-dependent (such as in\n`fsmonitor--daemon`).\n\nCiao,\nDscho\n"},{"id":"449592","messageId":"20220225160147.14824-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2202251632320.11118@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-25T16:01:47Z","receivedAt":"2022-02-25T16:02:27Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"Johannes Schindelin wrote:\n\n> As I just pointed out in\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejv=\n> aqbjf.bet/,\n> it seems that replacing the static check presented in v1 by a runtime\n> check needs to be reverted.\n>\n> In addition to the example I presented, there is another compelling reason\n> to do so: with the static check, we can detect incorrect usage strings in\n> all code, even in code that is platform-dependent (such as in\n> `fsmonitor--daemon`).\n\nFirst of all, thank you so much for putting so much time to look into\nmy PR. I appriciate your research about various possible outcomes of this\nPatch request.\n\nI saw your mail where you listed some of the disadvantages of the current\nversion. I also agree with the arguments you provided and it is also true\nthat one wouldn't find any clue by seeing the output of the `CI` link\nyou mentioned (i.e. https://github.com/git/git/runs/5312914410?check_suite_focus=true).\n\nLet's see what Junio, Ævar and others say about this.\n\nThanks :)\n"},{"id":"449595","messageId":"220225.86zgme7vxo.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-25T16:16:53Z","receivedAt":"2022-02-25T16:28:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Feb 25 2022, Johannes Schindelin wrote:\n\n> Hi,\n>\n> On Tue, 22 Feb 2022, Abhradeep Chakraborty wrote:\n>\n>> Julia Lawall wrote:\n>>\n>> > Of there are some cases that are useful to do statically, with only local\n>> > information, then using Coccinelle could be useful to get the problem out\n>> > of the way once and for all. Coccinelle doesn't support much processing\n>> > of strings directly, but you can always write some python code to test the\n>> > contents of a string and to create a new one.\n>> >\n>> > Let me know if you want to try this. You can also check, eg the demo\n>> > demos/pythontococci.cocci to see how to create code in a python script and\n>> > then use it in a normal SmPL rule.\n>> > ...\n>> > If the context that you are interested in is in a called function or is in\n>> > the calling context, then Coccinelle might not be the ideal choice.\n>> > Coccinelle works on one function at a time, so to do anything\n>> > interprocedural, you have to do some hacks.\n>>\n>> Though in this case, `parse-options.c check` method is better [...]\n>\n> I fear that this is incorrect.\n>\n> In general, it is my experience that it is a mistake any time a static\n> check is replaced by a runtime check.\n>\n> I was ready to let it slide in this instance, but in this case I now have\n> proof that the `parse-options.c` check is worse than the originally\n> suggested `sed` chain.\n>\n> That concrete proof is in the output of\n> https://github.com/git/git/actions/runs/1890665968, where the combination\n> of `ac/usage-string-fixups` and `jh/builtin-fsmonitor-part2` causes many,\n> many failures, but all of those failures have the same root cause: the\n> runtime check.\n>\n> With the original `check-usage-strings.sh`, the user inspecting any\n> failure would see precisely what the issue is, in the `static-analysis`\n> job's logs. It would display something like this:\n>\n> \tHEAD:builtin/fsmonitor--daemon.c:1507: N_(\"Max seconds to wait for background daemon startup\")),\n>\n> With v4 of the patch series, it does not spell out anything in\n> `static-analysis`. Instead, it causes 8 separate jobs to fail,\n> it causes failures not only in `t0012-help.sh` but also in\n> `t7519-status-fsmonitor.sh` and in `t7527-builtin-fsmonitor.sh`.\n>\n> The purpose of t7519 and t7527 is _not_ to verify those usage strings,\n> though.\n>\n> The worst part? Look at the relevant output of t0012 (see\n> https://github.com/git/git/runs/5312844492?check_suite_focus=true#step:5:4902):\n>\n> \t[...]\n> \t++ git -C sub fsmonitor--daemon -h\n> \t++ exit_code=128\n> \t++ test 128 = 129\n> \t++ echo 'test_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h'\n> \ttest_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h\n> \t++ return 1\n> \terror: last command exited with $?=1\n> \tnot ok 81 - fsmonitor--daemon can handle -h\n> \t[...]\n>\n> Do you see what usage string caused the failure? You can't. And that's\n> even by design:\n>\n> \t(\n> \t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n> \t\texport GIT_CEILING_DIRECTORIES &&\n> \t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n> \t) &&\n> \ttest_i18ngrep usage output\n>\n> The output is redirected, and since the runtime check added to\n> `parse-options.c` causes the exit code to be different from the expected\n> one, the output is never shown.\n>\n> Arguably the most important job of a regression test is to help software\n> engineers to diagnose and fix the regression. As quickly and as\n> conveniently as possible. That means that it is not enough to point out\n> that there is a regression, the output should be as helpful and concise as\n> possible to facilitate fixing the problem.\n>\n> In the above-mentioned case, it was neither as helpful nor as concise as\n> possible because in the test case that was supposed to identify the\n> problem, the actual error message was swallowed, and instead of causing\n> one test failure, it caused a whopping 42 test cases to fail (some of\n> which even show the error message, but that's not even the purpose of\n> those test cases).\n>\n> Since the entire point of this here patch series is to help enforce Git's\n> rules regarding usage strings, it should expect things like the issue with\n> `fsmonitor--daemon` _and_ make it as painless to address such an issue.\n>\n> I am afraid that I have to NAK the `parse-options.c` approach because v1\n> of this patch series did so much better a job.\n\nI think that's a bit of an overreaction to what I think is a solid v2 in\n, i.e. that we\nmust go back to v1 because we encountered this issue.\n\nA. I think you're right about the t0012-help.sh output being bad,\n but that's rather easily fixed with something like the [1] below.\n\n I've run into that a few times, wished it was better, and manually\n grepped or cat'd the \"output\" file.\n\n Part of that is ultimately because we're mixing and matching whether this\n \"usage\" output goes on stdout or stderr in various commands.\n\nB. The fsmonitor--daemon case is worse than most because it's only running on\n OS or Windows, i.e. the error we'd get in various other CI jobs is ifdef'd\n away, even though we could run the parse_options() part there.\n\n IIRC that's something I commented on in previous rounds of that series...\n\nC. The case of 42 tests failing because of this could be addressed by just having\n t0012-help.sh do these checks if we wanted, although in that case we'd need to\n make sure we deal with other test blind spots. I.e. the\n \"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\" suggestion I had.\n\nD. These sorts of check, by their nature, have an initial period of growing\n pains due to other in-flight topics. Once we move past that it's usually a\n non-issue going forward, as issues will be caught locally before patch\n submission.\n\n Data in favor of that is various other checks in parse_options_check() being\n mostly a non-issue, e.g. Junio's b6c2a0d45d4 (parse-options: make sure argh\n string does not have SP or _, 2014-03-23).\n\nIn this case I don't see how some minor issues when merging this with\n\"seen\" would have us abandon the v1 and commit to a fragile parsing of C\ncode in shellscript instead, or with some coccinelle check that would\nhave inherent issues finding the full context we need (passed-down flags\netc.).\n\n1. \n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 6c3e1f7159d..5474d463467 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -237,15 +237,24 @@ test_expect_success 'generate builtin list' '\n \tgit --list-cmds=builtins >builtins\n '\n \n+builtin_in_sub () {\n+\t(\n+\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n+\t\texport GIT_CEILING_DIRECTORIES &&\n+\t\t\"$@\"\n+\t)\n+}\n+\n+\n while read builtin\n do\n-\ttest_expect_success \"$builtin can handle -h\" '\n-\t\t(\n-\t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n-\t\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n-\t\t) &&\n-\t\ttest_i18ngrep usage output\n+\ttest_expect_success \"invoking '$builtin -h' yields exit code 129\" '\n+\t\tbuiltin_in_sub test_expect_code 129 git -C sub $builtin -h\n+\t'\n+\n+\ttest_expect_success \"invoking '$builtin -h' output\" '\n+\t\tbuiltin_in_sub test_expect_code 129 git -C sub $builtin -h >output 2>&1 &&\n+\t\tgrep usage output\n \t'\n done Hi Julia,\n>\n> On Tue, 22 Feb 2022, Julia Lawall wrote:\n>\n>> [I]f there are some cases that are useful to do statically, with only\n>> local information, then using Coccinelle could be useful to get the\n>> problem out of the way once and for all. Coccinelle doesn't support\n>> much processing of strings directly, but you can always write some\n>> python code to test the contents of a string and to create a new one.\n>>\n>> Let me know if you want to try this. You can also check, eg the demo\n>> demos/pythontococci.cocci to see how to create code in a python script and\n>> then use it in a normal SmPL rule.\n>>\n>> If some context has to be taken into account and the context in the same\n>> function, then that can also be done with Coccinelle, eg\n>>\n>> A\n>> ...\n>> B\n>>\n>> matches the case where after an A there is a B on all execution paths\n>> (except perhaps those that end in an error exit) and\n>>\n>> A\n>> ... when exists\n>> B\n>>\n>> matches the case where there is a B sometime after executing A, even if\n>> that does not always occur.\n>>\n>> If the context that you are interested in is in a called function or is in\n>> the calling context, then Coccinelle might not be the ideal choice.\n>> Coccinelle works on one function at a time, so to do anything\n>> interprocedural, you have to do some hacks.\n>\n> Right. The code in question is not actually calling a function, but a\n> macro, and passes a literal string to the macro that we would want to\n> check statically.\n>\n> I did have my doubts that it would be easy with Coccinelle, but since Ævar\n> seemed so confident, I tried it, struggled, and decided to follow up with\n> you.\n>\n> Thank you for confirming my suspicion!\n> Johannes\n\nIn case it's not clear from the upthread (and I thought my [1] explained\nit well enough) I never thought it would be easy or even possible to do\nthis particular thing with coccinelle.\n\nI.e. I mentioned in [1]:\n\n Aside: if we did want to do the \"parse C\" method the right way to do it\n would be to have a coccinelle script do it\n\nSo it's intended as a side-note to explain to a new contributor that\n*if* we do end up wanting to parse or transform C we have coccinelle,\nand it's a great tool for those cases where such static transformations\nare easy. I and others have added some in-tree in the past where\nappropriate.\n\nBut I then went on to say (and elaborated on later in [2]) that in this\ncase the right thing to do is runtime checking.\n\nSo, I'm sorry if you wasted time on it. In either case it seems we've\nended up in agreement in this case about appropriate uses of coccinelle.\n\nI.e. it's a fantastic tool as a semantic patch engine, but it\nunderstandably has limitations where you'd effectively need it to\nexecute your program to decide what to do, as is the case with the\nparse_options() API and the eventual parse_options_check() etc. doing\nassertions depending on flags that got passed down.\n\n1. https://lore.kernel.org/git/220221.86tucsb4oy.gmgdl@evledraar.gmail.com/\n2. https://lore.kernel.org/git/220222.867d9n83ir.gmgdl@evledraar.gmail.com/\n"},{"id":"449603","messageId":"xmqqo82ug9jx.fsf@gitster.g","threadId":"57427","inReplyTo":"20220225080811.8097-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-25T17:06:42Z","receivedAt":"2022-02-25T17:06:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhradeep Chakraborty writes:\n\n>> These two calls to optbug() use xstrfmt() to grab allocated pieces\n>> of memory and pass it as a parameter to the function, which means\n>> the string is leaked without any chance to be freed.\n>>\n>> Do we care?\n>>\n>> > \t\tif (opts->argh &&\n>> > \t\t strcspn(opts->argh, \" _\") != strlen(opts->argh))\n>> > \t\t\terr |= optbug(opts, \"multi-word argh should use dash to separate words\");\n>>\n>> The existing use of optbug() we see here does not share such a\n>> problem.\n>\n> hmm, I wanted a formatting function to format (i.e. pass the\n> `opt->help` dynamically) the output string. The existing use of\n> `optbug()` that you specified has no `%s` formatter; it is a plain\n> string. That's why I used `xstrfmt()`. Moreover, it was in Ævar's\n> suggestion[1] -\n>\n>> +\t\tif (opts->help && ends_with(opts->help, \".\"))\n>> +\t\t\terr |= optbug(opts, xstrfmt(\"argh should not end with a dot: %s\", opts->help));\n>\n> But I think, you're right. There is some memory leakage here.\n> Should I go with plain strings then? (i.e. \"help should not end\n> with a dot\" instead of `xstrfmt(\"help should not end with a dot:\n> %s\", opts->help)`)\n\nSorry that I've given you a trick question, when I know you are\nquite new to the community.\n\nI think the right answer to \"Do we care?\" is \"In this case, because\nwe are about to call exit(), we don't care. The extra complexity\nand code necessary to retain the memory we get from xstrfmt and free\nit is not worth it.\" It's not like we do this in a loop that iterates\nunbounded number of times before the exit() happens (in which case\nwe should care).\n\nThanks.\n\n\n\n\n"},{"id":"449648","messageId":"xmqqpmna76jz.fsf@gitster.g","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2202251632320.11118@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-26T01:36:16Z","receivedAt":"2022-02-26T01:36:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n>> Add checks to the `parse_options_check()` function to check usage\n>> strings against the style convention.\n>\n> As I just pointed out in\n> https://lore.kernel.org/git/nycvar.QRO.7.76.6.2202251600210.11118@tvgsbejvaqbjf.bet/,\n> it seems that replacing the static check presented in v1 by a runtime\n> check needs to be reverted.\n\nSorry, but I am not sure how that conclusion follows from a breakage\nin a topic in flight that was discovered by the check.\n\nI do not know if a coccinelle based solution is sufficiently easy,\nsimple and robust enough to encourage us to scrap what has already\nbeen proposed and reviewed, instead of leaving it as a topic for a\nfuture incremental improvement that we can make on top.\n\n> In addition to the example I presented, there is another compelling reason\n> to do so: with the static check, we can detect incorrect usage strings in\n> all code, even in code that is platform-dependent (such as in\n> `fsmonitor--daemon`).\n\nYes and no. \n\nI would imagine that large enough platforms that have their own\nconditionally compiled #ifdef/#endif block already have CI to build\ntheir conditionally compiled block in practice. I do not see the\nabove as a compelling reason to grow and shift the scope of these\ntwo patches.\n\nThanks.\n"},{"id":"449650","messageId":"20220226035721.1219-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqo82ug9jx.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-26T03:57:21Z","receivedAt":"2022-02-26T03:58:27Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJunio C Hamano wrote:\n\n> Sorry that I've given you a trick question, when I know you are\n> quite new to the community.\n\nThere is nothing to say `sorry`. Every review comment is teaching me\nnew things. E.g. If you didn't ask me this question, I would not go to\nthe codebase and see the proper handling of `xstrfmt`. So, thanks.\n\n> I think the right answer to \"Do we care?\" is \"In this case, because\n> we are about to call exit(), we don't care. The extra complexity\n> and code necessary to retain the memory we get from xstrfmt and free\n> it is not worth it.\" It's not like we do this in a loop that iterates\n> unbounded number of times before the exit() happens (in which case\n> we should care).\n>\n> Thanks.\n\nGot it.\n\nThanks :)\n"},{"id":"449651","messageId":"20220226042214.1413-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"220225.86zgme7vxo.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-26T04:22:14Z","receivedAt":"2022-02-26T04:22:45Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nÆvar Arnfjörð Bjarmason wrote:\n\n> A. I think you're right about the t0012-help.sh output being bad,\n> but that's rather easily fixed with something like the [1] below.\n\nDo you think the fix you suggested should be a part of this Patch series\nor a dedicated patch request is needed for this?\n\n> C. The case of 42 tests failing because of this could be addressed by just having\n> t0012-help.sh do these checks if we wanted, although in that case we'd need to\n> make sure we deal with other test blind spots. I.e. the\n> \"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\" suggestion I had.\n\nPardon me, I am having problem to understand the \n`\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\" suggestion I had.` part\nhere. Could you please explain a little bit?\n\nThanks :)\n"},{"id":"449656","messageId":"xmqqa6ee6txq.fsf@gitster.g","threadId":"57427","inReplyTo":"xmqqpmna76jz.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-26T06:08:49Z","receivedAt":"2022-02-26T06:08:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> I would imagine that large enough platforms that have their own\n> conditionally compiled #ifdef/#endif block already have CI to build\n> their conditionally compiled block in practice. I do not see the\n> above as a compelling reason to grow and shift the scope of these\n> two patches.\n\nLet's instead drop [2/2] for now. I do not want us to go back to\nshell script that pretends to know about C language, and I do not\nwant to block [1/2] by waiting for a replacement. Fixes in [1/2]\nare pretty much uncontroversial ones that can even be fast-tracked\ndown to 'master'.\n\n\n"},{"id":"449660","messageId":"20220226065704.7137-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqa6ee6txq.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-26T06:57:04Z","receivedAt":"2022-02-26T06:57:34Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJunio C Hamano wrote:\n\n> Let's instead drop [2/2] for now. I do not want us to go back to\n> shell script that pretends to know about C language, and I do not\n> want to block [1/2] by waiting for a replacement. Fixes in [1/2]\n> are pretty much uncontroversial ones that can even be fast-tracked\n> down to 'master'.\n\nThough, as a new contributor, I felt bad about dropping the last\npatch, but if you think the last patch request needs more discussion\n( which I think is needed) - I also in favour of dropping the last\ncommit.\n\nWould you do this on your side or I will re-submit it with the first\ncommit?\n\nThanks :)\n"},{"id":"449666","messageId":"alpine.DEB.2.22.394.2202260954341.3112@hadrien","threadId":"57427","inReplyTo":"20220226042214.1413-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH] add usage-strings ci check and amend remaining usage strings","fromName":"Julia Lawall","fromEmail":"julia.lawall@inria.fr","sentAt":"2022-02-26T08:55:17Z","receivedAt":"2022-02-26T08:55:26Z","isPatch":true,"sender":{"key":"julia.lawall@inria.fr","avatar":null},"body":"Hello,\n\nSince it seems that Coccinelle is not useful for your problem, could you\nremove me from the CC list on this discussion?\n\nthanks,\njulia\n\nOn Sat, 26 Feb 2022, Abhradeep Chakraborty wrote:\n\n>\n> Ævar Arnfjörð Bjarmason wrote:\n>\n> > A. I think you're right about the t0012-help.sh output being bad,\n> > but that's rather easily fixed with something like the [1] below.\n>\n> Do you think the fix you suggested should be a part of this Patch series\n> or a dedicated patch request is needed for this?\n>\n> > C. The case of 42 tests failing because of this could be addressed by just having\n> > t0012-help.sh do these checks if we wanted, although in that case we'd need to\n> > make sure we deal with other test blind spots. I.e. the\n> > \"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\" suggestion I had.\n>\n> Pardon me, I am having problem to understand the\n> `\"GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP\" suggestion I had.` part\n> here. Could you please explain a little bit?\n>\n> Thanks :)\n>"},{"id":"449704","messageId":"xmqqtuck3yv2.fsf@gitster.g","threadId":"57427","inReplyTo":"20220226065704.7137-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-27T19:15:13Z","receivedAt":"2022-02-27T19:18:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhradeep Chakraborty writes:\n\n> Junio C Hamano wrote:\n>\n>> Let's instead drop [2/2] for now. I do not want us to go back to\n>> shell script that pretends to know about C language, and I do not\n>> want to block [1/2] by waiting for a replacement. Fixes in [1/2]\n>> are pretty much uncontroversial ones that can even be fast-tracked\n>> down to 'master'.\n>\n> Though, as a new contributor, I felt bad about dropping the last\n> patch, but if you think the last patch request needs more discussion\n> ( which I think is needed) - I also in favour of dropping the last\n> commit.\n>\n> Would you do this on your side or I will re-submit it with the first\n> commit?\n\nNah, I can just discard the second commit and keep the first one.\n"},{"id":"449727","messageId":"20220228073908.20553-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqtuck3yv2.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-02-28T07:39:08Z","receivedAt":"2022-02-28T07:39:34Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJunio C Hamano wrote:\n\n> Nah, I can just discard the second commit and keep the first one.\n\nOkay, that's great. But one thing I want to ask - How the discussion\nfor `adding check for usage strings` will be held i.e. Whether the\nidea is discarded for now.\n\nIf it is not discarded, then how to proceed? Johannes prefers the first\nversion and Ævar prefers the `add check to parse-options.c` version.\n\nThanks :)\n"},{"id":"449788","messageId":"xmqqzgma287n.fsf@gitster.g","threadId":"57427","inReplyTo":"20220228073908.20553-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-02-28T17:48:28Z","receivedAt":"2022-02-28T18:10:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhradeep Chakraborty writes:\n\n> Okay, that's great. But one thing I want to ask - How the discussion\n> for `adding check for usage strings` will be held i.e. Whether the\n> idea is discarded for now.\n>\n> If it is not discarded, then how to proceed? Johannes prefers the first\n> version and Ævar prefers the `add check to parse-options.c` version.\n\nMy take on it is that the \"first version\" that uses an ad-hoc shell\nscript will not become acceptably robust. If coccinelle or other\nstatic analyzer can help us check more reliably, that would be great\nbecause we won't incur runtime cost of checking, like the embedded\ncheck we added in the latest version that we are tentatively removing.\n\nI also think Dscho simply overreacted only because the check broke\nan in-flight topic that is from his group, which is not universally\nbuilt, and the tests in it was written in such a way that the error\noutput from the embedded check was not immediately available when\nrun in the CI, making it harder to debug. None of that is a fault\nin the approach of using the embedded check.\n\nIf the embedded check were there from the beginning, together with\ntons of the existing checks done by parse_options_check(), the\ndevelopers themselves of the in-flight topic(s) would have caught\nthe problem, even before it hit the public CI. I am very sure Dscho\nwouldn't have complained or even noticed that you added a new check\nto the parse_options_check().\n\nSo from my point of view, plan should be\n\n (0) I have been assuming that the check we removed tentatively is\n correct and the breakage in in-flight topic caught usage\n strings that were malformed. If not, we need to tweak it to\n make sure it does not produce false positives.\n\n (1) Help Microsoft folks fix the in-flight topic with faulty usage\n strings.\n\n (2) Rethink if parse_options_check() can be made optional at\n runtime, which would (a) allow our test to enable it, and allow\n us to test all code paths that use parse_options() centrally,\n and (b) allow us to bypass the check while the end-user runs\n \"git\", to avoid overhead of checking the same option[] array,\n which does not change between invocations of \"git\", over and\n over again all over the world.\n\n We may add the check back to parse_options_check() after doing\n the above. There are already tons of \"check sanity of what is\n inside option[]\" in there, and it would be beneficial if we can\n separate out from parse_options_start() the sanity checking\n code, regardless of this topic.\n\n (3) While (2) is ongoing, we can let people also explore static\n analysis possibilities.\n"},{"id":"449802","messageId":"220228.86mtia3hqi.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"xmqqzgma287n.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-02-28T19:32:18Z","receivedAt":"2022-02-28T19:50:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Feb 28 2022, Junio C Hamano wrote:\n\n> [...]\n> So from my point of view, plan should be\n>\n> (0) I have been assuming that the check we removed tentatively is\n> correct and the breakage in in-flight topic caught usage\n> strings that were malformed. If not, we need to tweak it to\n> make sure it does not produce false positives.\n>\n> (1) Help Microsoft folks fix the in-flight topic with faulty usage\n> strings.\n\nAgreed.\n\n> (2) Rethink if parse_options_check() can be made optional at\n> runtime, which would (a) allow our test to enable it, and allow\n> us to test all code paths that use parse_options() centrally,\n> and (b) allow us to bypass the check while the end-user runs\n> \"git\", to avoid overhead of checking the same option[] array,\n> which does not change between invocations of \"git\", over and\n> over again all over the world.\n>\n> We may add the check back to parse_options_check() after doing\n> the above. There are already tons of \"check sanity of what is\n> inside option[]\" in there, and it would be beneficial if we can\n> separate out from parse_options_start() the sanity checking\n> code, regardless of this topic.\n\nThis is a good idea, but while t0012-help.sh catches most of it, it\ndoesn't cover e.g. sub-commands that call parse_options() in N functions\none after the other.\n\nWe could that in t0012-help.sh with (pseudocode):\n\n for subcmd write verify ...\n do\n test_expect_success '...' 'git commit-graph $subcmd -h'\n done\n\netc., but that still won't catch *all* of it, and we don't have a way to\nspew out \"what commands use subcommands\".\n\nHence why we need to run the rest of the test suite, and why these\nthings aren't some one-off GIT_TEST_ mode or t/helper/*.c code already.\n\n> (3) While (2) is ongoing, we can let people also explore static\n> analysis possibilities.\n\nI think with in-flight concerns with (0) and (1) addressed what we have\nhere is really good enough for now, and we could just add it to the\nexisting parse_options_check() without needing (2) and (3) at this\npoint.\n"},{"id":"449869","messageId":"20220301063801.26732-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqzgma287n.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-03-01T06:38:01Z","receivedAt":"2022-03-01T06:38:43Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJunio C Hamano wrote:\n\n> I also think Dscho simply overreacted only because the check broke\n> an in-flight topic that is from his group, which is not universally\n> built, and the tests in it was written in such a way that the error\n> output from the embedded check was not immediately available when\n> run in the CI, making it harder to debug. None of that is a fault\n> in the approach of using the embedded check.\n>\n> If the embedded check were there from the beginning, together with\n> tons of the existing checks done by parse_options_check(), the\n> developers themselves of the in-flight topic(s) would have caught\n> the problem, even before it hit the public CI. I am very sure Dscho\n> wouldn't have complained or even noticed that you added a new check\n> to the parse_options_check().\n\nHmm, that's true.\n\n> (2) Rethink if parse_options_check() can be made optional at\n> runtime, which would (a) allow our test to enable it, and allow\n> us to test all code paths that use parse_options() centrally,\n> and (b) allow us to bypass the check while the end-user runs\n> \"git\", to avoid overhead of checking the same option[] array,\n> which does not change between invocations of \"git\", over and\n> over again all over the world.\n>\n> We may add the check back to parse_options_check() after doing\n> the above. There are already tons of \"check sanity of what is\n> inside option[]\" in there, and it would be beneficial if we can\n> separate out from parse_options_start() the sanity checking\n> code, regardless of this topic.\n>\n> (3) While (2) is ongoing, we can let people also explore static\n> analysis possibilities.\n\nI agree with you. But I think these two points(specially (2)) deserve\na dedicated discussion/patch thread. Because, the latest version of this\npatch series (actually this patch series itself) only cares about the\n`usage strings`.\n\nSo, I argue you to not discard the last commit for now. As you said `There are\nalready tons of \"check sanity of what is inside option[]\"`, integrating\none more sanity check would not affect it. I am saying it not because\nI made that commit. The discussion or patch integration of (2) and (3)\nmay take few weeks (or more than a month may be; I also would like to\ntake part/contribute to that discussion/PR). I fear that another\nset of invalid usage-strings would be added in that time. In that case,\nwe have to make another commit/PR for correcting those strings - disrupting\nthe purpose of this first commit you are willing to merge.\n\nAs Ævar also said - \n\n> I think with in-flight concerns with (0) and (1) addressed what we have\n> here is really good enough for now, and we could just add it to the\n> existing parse_options_check() without needing (2) and (3) at this\n> point.\n\nThanks :)\n"},{"id":"449920","messageId":"xmqqsfs2rko1.fsf@gitster.g","threadId":"57427","inReplyTo":"20220301063801.26732-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-01T11:12:30Z","receivedAt":"2022-03-01T11:12:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhradeep Chakraborty writes:\n\n>> (2) Rethink if parse_options_check() can be made optional at\n>> ...\n>> (3) While (2) is ongoing, we can let people also explore static\n>> analysis possibilities.\n>\n> I agree with you. But I think these two points(specially (2)) deserve\n> a dedicated discussion/patch thread. Because, the latest version of this\n> patch series (actually this patch series itself) only cares about the\n> `usage strings`.\n\nYes, absolutely. So applying [2/2] in haste is not a good idea at\nall. Before we accumulate more cruft on top, we should stop and\nthink if the approach we are taking is sensible to begin with, or\nwe'll make an already bad situation even worse.\n\n"},{"id":"449991","messageId":"nycvar.QRO.7.76.6.2203012024210.11118@tvgsbejvaqbjf.bet","threadId":"57427","inReplyTo":"xmqqzgma287n.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-01T19:37:41Z","receivedAt":"2022-03-01T19:37:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 28 Feb 2022, Junio C Hamano wrote:\n\n> Abhradeep Chakraborty writes:\n>\n> > Okay, that's great. But one thing I want to ask - How the discussion\n> > for `adding check for usage strings` will be held i.e. Whether the\n> > idea is discarded for now.\n> >\n> > If it is not discarded, then how to proceed? Johannes prefers the first\n> > version and Ævar prefers the `add check to parse-options.c` version.\n>\n> My take on it is that the \"first version\" that uses an ad-hoc shell\n> script will not become acceptably robust.\n\nIt is unfortunate that the challenge had been characterized as \"parse C\",\nwhen in reality we are talking about highly idiomatic code. It's not like\nwe accept arbitrary input in the `OPT_...()` lines. We _really_ want the\noption usage string to be a string that is enclosed in `N_()`.\n\nAdditionally, this is about Git's own code, not arbitrary C code provided\nby users. That makes that shell script more on par with `t/chainlint.sed`\nthan with `contrib/coccinelle/*`.\n\nHaving said that...\n\n> If coccinelle or other static analyzer can help us check more reliably,\n> that would be great because we won't incur runtime cost of checking,\n> like the embedded check we added in the latest version that we are\n> tentatively removing.\n\nI think that Julia gave us enough to work with, so we can (ab-)use\nCoccinelle for static usage string checks, and we should probably do that,\ntoo.\n\n> I also think Dscho simply overreacted only because the check broke\n> an in-flight topic that is from his group, which is not universally\n> built, and the tests in it was written in such a way that the error\n> output from the embedded check was not immediately available when\n> run in the CI, making it harder to debug. None of that is a fault\n> in the approach of using the embedded check.\n\nNo, I would have reacted the same way if I had seen the failures in any\nother topic, with an equally trivial fix that blooms into 42 separate test\ncase failures.\n\nThis explosion made me realize _why_ I found the suggestion to patch\n`parse_options()` iffy in the first place: it replaces a static check with\na runtime check, which is almost always something that is regretted later.\n\nAnd since Abhradeep is a new contributor, I found it important to steer\nthe direction toward sound advice that they can use over and over again\nover the course of their career: whenever possible, prefer static checks\nover runtime ones.\n\n> If the embedded check were there from the beginning, together with\n> tons of the existing checks done by parse_options_check(), the\n> developers themselves of the in-flight topic(s) would have caught\n> the problem, even before it hit the public CI. I am very sure Dscho\n> wouldn't have complained or even noticed that you added a new check\n> to the parse_options_check().\n\nIndeed, if no static check had been proposed first, I would not have\ncaught on to the unfortunate suggestion to use a runtime check _instead_.\n\n> So from my point of view, plan should be\n>\n> (0) I have been assuming that the check we removed tentatively is\n> correct and the breakage in in-flight topic caught usage\n> strings that were malformed. If not, we need to tweak it to\n> make sure it does not produce false positives.\n>\n> (1) Help Microsoft folks fix the in-flight topic with faulty usage\n> strings.\n\nYou're so sweet, but I already did that in parallel.\n\n>\n> (2) Rethink if parse_options_check() can be made optional at\n> runtime, which would (a) allow our test to enable it, and allow\n> us to test all code paths that use parse_options() centrally,\n> and (b) allow us to bypass the check while the end-user runs\n> \"git\", to avoid overhead of checking the same option[] array,\n> which does not change between invocations of \"git\", over and\n> over again all over the world.\n>\n> We may add the check back to parse_options_check() after doing\n> the above. There are already tons of \"check sanity of what is\n> inside option[]\" in there, and it would be beneficial if we can\n> separate out from parse_options_start() the sanity checking\n> code, regardless of this topic.\n>\n> (3) While (2) is ongoing, we can let people also explore static\n> analysis possibilities.\n\nOf course, if we can convince Coccinelle (together with Python) to give us\nthe static check, we might very well be able to port more of\n`parse_options_check()` from runtime checks to static ones, which would be\na clear win.\n\nIf that is possible, we could save ourselves a lot of time by skipping (2)\naltogether.\n\nAnd as I said, Julia's advice looked really good. If only I wasn't\ndesperately short on time, I would have given it a try because it sounds\nnot only fun but also very, very useful in Git's context.\n\nCiao,\nDscho\n"},{"id":"450002","messageId":"xmqqr17lphav.fsf_-_@gitster.g","threadId":"57427","inReplyTo":"xmqqzgma287n.fsf@gitster.g","subject":"[PATCH] parse-options: make parse_options_check() test-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-01T20:08:08Z","receivedAt":"2022-03-01T20:08:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"The array of options given to the parse-options API is sanity\nchecked for reuse of a single-letter option for multiple entries and\nother programmer mistakes by calling parse_options_check() from\nparse_options_start(). This allows our developers to catch silly\nmistakes early, but all callers of parse-options API pays this cost.\nOnce the set of options in an array is validated and passes this\ncheck, until a programmer modifies the array, there is no way for it\nto fail the check, which is wasteful.\n\nIntroduce the GIT_TEST_PARSE_OPTIONS_CHECK environment variable and\nmake the sanity check only when it is set to true. Set it in\nt/test-lib.sh so that our tests will continue to catch buggy options\narrays.\n\nSigned-off-by: Junio C Hamano \n---\n\n > (2) Rethink if parse_options_check() can be made optional at\n > runtime, which would (a) allow our test to enable it, and allow\n > us to test all code paths that use parse_options() centrally,\n > and (b) allow us to bypass the check while the end-user runs\n > \"git\", to avoid overhead of checking the same option[] array,\n > which does not change between invocations of \"git\", over and\n > over again all over the world.\n >\n > We may add the check back to parse_options_check() after doing\n > the above. There are already tons of \"check sanity of what is\n > inside option[]\" in there, and it would be beneficial if we can\n > separate out from parse_options_start() the sanity checking\n > code, regardless of this topic.\n\n This looked too easy and there may be some pitfalls, but I am\n hoping that we will know soon enough by floating a weather\n balloon like this.\n\n parse-options.c | 12 +++++++++++-\n t/README | 5 +++++\n t/test-lib.sh | 3 +++\n 3 files changed, 19 insertions(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 6e57744fd2..02cfe3f2cd 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -439,6 +439,14 @@ static void check_typos(const char *arg, const struct option *options)\n \t}\n }\n \n+/*\n+ * Check the sanity of contents of opts[] array to find programmer\n+ * mistakes (like duplicated short options).\n+ *\n+ * This function is supposed to be no-op when it returns without\n+ * dying, making a call from parse_options_start_1() to it optional\n+ * in end-user builds.\n+ */\n static void parse_options_check(const struct option *opts)\n {\n \tint err = 0;\n@@ -523,7 +531,9 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n \tif ((flags & PARSE_OPT_ONE_SHOT) &&\n \t (flags & PARSE_OPT_KEEP_ARGV0))\n \t\tBUG(\"Can't keep argv0 if you don't have it\");\n-\tparse_options_check(options);\n+\n+\tif (git_env_bool(\"GIT_TEST_PARSE_OPTIONS_CHECK\", 0))\n+\t\tparse_options_check(options);\n }\n \n void parse_options_start(struct parse_opt_ctx_t *ctx,\ndiff --git a/t/README b/t/README\nindex f48e0542cd..b7285531f2 100644\n--- a/t/README\n+++ b/t/README\n@@ -472,6 +472,11 @@ a test and then fails then the whole test run will abort. This can help to make\n sure the expected tests are executed and not silently skipped when their\n dependency breaks or is simply not present in a new environment.\n \n+GIT_TEST_PARSE_OPTIONS_CHECK=, when true, makes all options\n+array passed to the parse-options API to be sanity checked. This\n+environment variable is set to true by test-lib.sh unless it is set.\n+\n+\n Naming Tests\n ------------\n \ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex e4716b0b86..805f495fd4 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -474,6 +474,9 @@ export GIT_DEFAULT_HASH\n GIT_TEST_MERGE_ALGORITHM=\"${GIT_TEST_MERGE_ALGORITHM:-ort}\"\n export GIT_TEST_MERGE_ALGORITHM\n \n+: ${GIT_TEST_PARSE_OPTIONS_CHECK:=1}\n+export GIT_TEST_PARSE_OPTIONS_CHECK\n+\n # Tests using GIT_TRACE typically don't want : output\n GIT_TRACE_BARE=1\n export GIT_TRACE_BARE\n-- \n2.35.1-354-g715d08a9e5\n\n"},{"id":"450023","messageId":"220301.86pmn5z5we.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"xmqqr17lphav.fsf_-_@gitster.g","subject":"Re: [PATCH] parse-options: make parse_options_check() test-only","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-01T21:57:17Z","receivedAt":"2022-03-01T22:04:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 01 2022, Junio C Hamano wrote:\n\n> The array of options given to the parse-options API is sanity\n> checked for reuse of a single-letter option for multiple entries and\n> other programmer mistakes by calling parse_options_check() from\n> parse_options_start(). This allows our developers to catch silly\n> mistakes early, but all callers of parse-options API pays this cost.\n> Once the set of options in an array is validated and passes this\n> check, until a programmer modifies the array, there is no way for it\n> to fail the check, which is wasteful.\n\nThat's not true due to the \"git rev-parse --parseopt\" interface. I'd be\nhappy to deprecate it, but I think the last time I brought it up you\nwere opposed, i.e. it's documented as plumbing in \"git-rev-parse\", and\nit's easy to have it hit some of these BUG()'s.\n\nI see the benifit of Johannes's suggestion of checking this once (but\nwith t0012-help.sh etc. we're nowhere near being able to do that).\n\nNow this runs for the whole test suite, so our tests will have the the\nsame behavior.\n\nSo it's just an optimization? Isn't it premature, if you run\nparse_options_check() in a loop how many checks/sec can we do? I haven't\ntested, but I'm betting it's a *lot*.\n\nSo aren't we shaving microseconds off the runtime here?\n"},{"id":"450025","messageId":"xmqqo82pnwoc.fsf@gitster.g","threadId":"57427","inReplyTo":"220301.86pmn5z5we.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] parse-options: make parse_options_check() test-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-01T22:18:59Z","receivedAt":"2022-03-01T22:19:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Tue, Mar 01 2022, Junio C Hamano wrote:\n>\n>> The array of options given to the parse-options API is sanity\n>> checked for reuse of a single-letter option for multiple entries and\n>> other programmer mistakes by calling parse_options_check() from\n>> parse_options_start(). This allows our developers to catch silly\n>> mistakes early, but all callers of parse-options API pays this cost.\n>> Once the set of options in an array is validated and passes this\n>> check, until a programmer modifies the array, there is no way for it\n>> to fail the check, which is wasteful.\n>\n> That's not true due to the \"git rev-parse --parseopt\" interface. I'd be\n\nMeaning that a parse-options array can be fed by \"rev-parse --parseopt\"\nand having the sanity check enabled does help the use case? Even there,\nI would say that once the script writer finishes developing the script\nthat uses \"rev-parse --parseopt\", setting the parseopt input in stone,\nthere is no need to check the same thing over and over again. Am I\nmistaken? Does \"rev-parse --parseopt\" that is fed the same input\nsometimes trigger the sanity check and sometimes not?\n\n> I see the benifit of Johannes's suggestion of checking this once (but\n> with t0012-help.sh etc. we're nowhere near being able to do that).\n>\n> Now this runs for the whole test suite, so our tests will have the the\n> same behavior.\n\nThe code for sanity check is there ONLY to help those who develop\nwhile they develop, and it is logical to enable it during our tests.\nThere is no reason to trigger the sanity check in the end-user\nenvironment, no?\n\n> So aren't we shaving microseconds off the runtime here?\n\nNo, the problem I have with the runtime check is more at the\nconceptual level. Those who remove assert() by setting _NDEBUG\nwould not be doing so to save nanoseconds, either.\n"},{"id":"450077","messageId":"220302.86r17k7gry.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"xmqqo82pnwoc.fsf@gitster.g","subject":"Re: [PATCH] parse-options: make parse_options_check() test-only","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-02T10:52:22Z","receivedAt":"2022-03-02T11:09:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Mar 01 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Tue, Mar 01 2022, Junio C Hamano wrote:\n>>\n>>> The array of options given to the parse-options API is sanity\n>>> checked for reuse of a single-letter option for multiple entries and\n>>> other programmer mistakes by calling parse_options_check() from\n>>> parse_options_start(). This allows our developers to catch silly\n>>> mistakes early, but all callers of parse-options API pays this cost.\n>>> Once the set of options in an array is validated and passes this\n>>> check, until a programmer modifies the array, there is no way for it\n>>> to fail the check, which is wasteful.\n>>\n>> That's not true due to the \"git rev-parse --parseopt\" interface. I'd be\n>\n> Meaning that a parse-options array can be fed by \"rev-parse --parseopt\"\n> and having the sanity check enabled does help the use case? Even there,\n> I would say that once the script writer finishes developing the script\n> that uses \"rev-parse --parseopt\", setting the parseopt input in stone,\n> there is no need to check the same thing over and over again. Am I\n> mistaken? Does \"rev-parse --parseopt\" that is fed the same input\n> sometimes trigger the sanity check and sometimes not?\n\nIf we're declaring that \"git rev-parse --parseopt\" is something that was\nonly ever intended for in-tree usage sure, that should hold true.\n\nI.e. \"git rev-parse\" is documented as plumbing, and we document\n--parseopt as a generic option parsing mechanism you can use in\nshellscripts.\n\nSo out-of-tree users wouldn't guard against\nGIT_TEST_PARSE_OPTIONS_CHECK, and I wouldn't be surprised if we could\ne.g. segfault on some subsequent code if some of the sanity checks\naren't happening anymore.\n\nNo, I'd be quite happy if we declared that it's for our use only, and\ncould remove it when the last in-tree *.sh user went away. there's a bit\nof complexity in parse_options() required only for its use....\n\n>> I see the benifit of Johannes's suggestion of checking this once (but\n>> with t0012-help.sh etc. we're nowhere near being able to do that).\n>>\n>> Now this runs for the whole test suite, so our tests will have the the\n>> same behavior.\n>\n> The code for sanity check is there ONLY to help those who develop\n> while they develop, and it is logical to enable it during our tests.\n> There is no reason to trigger the sanity check in the end-user\n> environment, no?\n\nI don't see the benefit of skipping it. Your commit message mentions\n\"but all callers of parse-options API pays this cost\". As a quick & dumb\nperf test I tried:\n\t\n\tdiff --git a/parse-options.c b/parse-options.c\n\tindex 6e57744fd22..cabea35e8b1 100644\n\t--- a/parse-options.c\n\t+++ b/parse-options.c\n\t@@ -523,7 +523,10 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,\n\t if ((flags & PARSE_OPT_ONE_SHOT) &&\n\t (flags & PARSE_OPT_KEEP_ARGV0))\n\t BUG(\"Can't keep argv0 if you don't have it\");\n\t- parse_options_check(options);\n\t+ while (1) {\n\t+ printf(\".\");\n\t+ parse_options_check(options);\n\t+ }\n\t }\n\t \n\t void parse_options_start(struct parse_opt_ctx_t *ctx,\n\nAnd:\n\n ./git [am|rebase] | pv >/dev/null\n\nGet around 4MiB/s. I.e. we can do this check ~4 million times/sec on my\ncomputer, with -O3, with -O0 -g it's ~3MiB/s.\n\nSo the performance cost is trivial & not worth worrying about.\n\n>> So aren't we shaving microseconds off the runtime here?\n>\n> No, the problem I have with the runtime check is more at the\n> conceptual level. Those who remove assert() by setting _NDEBUG\n> would not be doing so to save nanoseconds, either.\n\nI think the trade-off of not having to worry about the runtime\nv.s. \"development build\" checks is one we've done well with BUG(),\ni.e. not to have it be an assert().\n\nE.g. in this case we have parse_options_concat(), so you can dynamically\nconstruct the options to be checked.\n\nI happen to have looked in detail at all of that code in the past, and I\ndon't *think* it's doing something \"actually dynamic\". I.e. it should be\nthe same when the tests run and when git runs in the wild.\n\nBut having to know and check that when using or changing the API is just\nmore state to keep in your head.\n"},{"id":"450152","messageId":"xmqq1qzkmb8q.fsf@gitster.g","threadId":"57427","inReplyTo":"220302.86r17k7gry.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH] parse-options: make parse_options_check() test-only","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-02T18:59:33Z","receivedAt":"2022-03-02T18:59:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> Meaning that a parse-options array can be fed by \"rev-parse --parseopt\"\n>> and having the sanity check enabled does help the use case? Even there,\n>> I would say that once the script writer finishes developing the script\n>> that uses \"rev-parse --parseopt\", setting the parseopt input in stone,\n>> there is no need to check the same thing over and over again. Am I\n>> mistaken? Does \"rev-parse --parseopt\" that is fed the same input\n>> sometimes trigger the sanity check and sometimes not?\n>\n> If we're declaring that \"git rev-parse --parseopt\" is something that was\n> only ever intended for in-tree usage sure, that should hold true.\n\n> So out-of-tree users wouldn't guard against\n> GIT_TEST_PARSE_OPTIONS_CHECK, and I wouldn't be surprised if we could\n> e.g. segfault on some subsequent code if some of the sanity checks\n> aren't happening anymore.\n> ...\n> No, I'd be quite happy if we declared that it's for our use only, and\n> could remove it when the last in-tree *.sh user went away. there's a bit\n> of complexity in parse_options() required only for its use....\n\nI do not see any need for such a declaration. We are not changing\nthe behaviour of \"git rev-parse --parseopt\" plumbing command at all\nfor those who feed valid input to it.\n\n\"rev-parse --parseopt\" users can keep using their scripts just the\nsame as before, debugging their scripts to catch silly mistakes like\nduplicated short options may become slightly harder, but they still\nhave a way to ask for the same debugging support available.\n\nYes, I am saying that is perfectly fine, and both in-tree and\nout-of-tree users have a way to reinstate the sanity checks. I also\ndo not mind if your proposal were one of these:\n\n * introduce --parseopt-with-sanity-check to \"rev-parse\" and arrange\n the parse_options_check() call to be made when the command was\n invoked with it; or\n\n * introduce --parse-opt-without-sanity-check to \"rev-parse\", and\n arrange the parse_options_check() call to be still made when\n \"--parse-opt\" is used. Those who finished developing their\n scripts can rewrite their --parse-opt to \"without\" version for\n conceptual cleanliness.\n\n> So the performance cost is trivial & not worth worrying about.\n\nI already said I am not worried about it, didn't I? These numbers\ndo not matter in this discussion.\n\n"},{"id":"450156","messageId":"220302.861qzk6tz5.gmgdl@evledraar.gmail.com","threadId":"57427","inReplyTo":"xmqq1qzkmb8q.fsf@gitster.g","subject":"Re: [PATCH] parse-options: make parse_options_check() test-only","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-02T19:17:13Z","receivedAt":"2022-03-02T19:21:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Mar 02 2022, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n> [...]\n>> So the performance cost is trivial & not worth worrying about.\n>\n> I already said I am not worried about it, didn't I? These numbers\n> do not matter in this discussion.\n\nSorry, but I really don't see the point then.\n\nYou'd like to keep \"git rev-parse --parseopt\", but now if you feed bad\ninput to it you'll get worse error messages from it, and it's not for a\nperformance benefit then why? Why would we have worse error reporting\nwithout any upside?\n\nAnother common case would be locally hacking a command that uses\nparse_options(), having it do the wrong thing for some cryptic reason\nwe'd catch in parse_options_check().\n\nThen eventually remember to turn on this GIT_TEST_* knob (i.e. if\ntesting via the command-line/debugger instead of the test suite). I for\none do that a lot when working on the parse_options()-using commands\nin-tree, if this land I'll probably remember to add this knob to my\n.bashrc, but everyone else will find out the hard way...\n"},{"id":"450294","messageId":"20220303173456.3773-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2203012024210.11118@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-03-03T17:34:56Z","receivedAt":"2022-03-03T17:35:12Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJohannes Schindelin wrote:\n\n> And since Abhradeep is a new contributor, I found it important to steer\n> the direction toward sound advice that they can use over and over again\n> over the course of their career: whenever possible, prefer static checks\n> over runtime ones.\n\nThanks Johannes for the advice. I will always remember it ^^\n\n> Of course, if we can convince Coccinelle (together with Python) to give us\n> the static check, we might very well be able to port more of\n> `parse_options_check()` from runtime checks to static ones, which would be\n> a clear win.\n>\n> If that is possible, we could save ourselves a lot of time by skipping (2)\n> altogether.\n>\n> And as I said, Julia's advice looked really good. If only I wasn't\n> desperately short on time, I would have given it a try because it sounds\n> not only fun but also very, very useful in Git's context.\n\nSince Junio and you both have an interest in Coccinelle, if you allow,\nI want to look into it.\n\nThanks :)\n"},{"id":"450319","messageId":"xmqqv8wu7jpf.fsf@gitster.g","threadId":"57427","inReplyTo":"20220303173456.3773-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-03T22:30:20Z","receivedAt":"2022-03-03T22:30:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Abhradeep Chakraborty writes:\n\n> Johannes Schindelin wrote:\n>\n>> And since Abhradeep is a new contributor, I found it important to steer\n>> the direction toward sound advice that they can use over and over again\n>> over the course of their career: whenever possible, prefer static checks\n>> over runtime ones.\n>\n> Thanks Johannes for the advice. I will always remember it ^^\n\nYup, if we can have static and dynamic checks of the same quality,\nstatic checks are always better alternative. In this case, runtime\ncheck would probably be an expedite solution suitable for a shorter\nterm to fill the gap, as a static check with the same quality as it\nwould probably need some time to develop.\n\n> Since Junio and you both have an interest in Coccinelle, if you allow,\n> I want to look into it.\n\nI do not have any particular interest. If it is a tool fit for the\ntask, it would be good to use it, that's all ;-)\n"},{"id":"450399","messageId":"20220304142154.2350-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"xmqqv8wu7jpf.fsf@gitster.g","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-03-04T14:21:54Z","receivedAt":"2022-03-04T14:22:34Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJunio C Hamano < wrote:\n\n> Yup, if we can have static and dynamic checks of the same quality,\n> static checks are always better alternative. In this case, runtime\n> check would probably be an expedite solution suitable for a shorter\n> term to fill the gap, as a static check with the same quality as it\n> would probably need some time to develop.\n\nGot it!\n\n> I do not have any particular interest. If it is a tool fit for the\n> task, it would be good to use it, that's all ;-)\n\nOkay, then I would like to research if that is a good fit. Johannes\nis pretty confident about it though.\n\nThanks :)\n"},{"id":"450601","messageId":"nycvar.QRO.7.76.6.2203071709540.11118@tvgsbejvaqbjf.bet","threadId":"57427","inReplyTo":"20220304142154.2350-1-chakrabortyabhradeep79@gmail.com","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2022-03-07T16:12:28Z","receivedAt":"2022-03-07T16:12:42Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 4 Mar 2022, Abhradeep Chakraborty wrote:\n\n> Junio C Hamano < wrote:\n>\n> > Yup, if we can have static and dynamic checks of the same quality,\n> > static checks are always better alternative. In this case, runtime\n> > check would probably be an expedite solution suitable for a shorter\n> > term to fill the gap, as a static check with the same quality as it\n> > would probably need some time to develop.\n>\n> Got it!\n\nWhile the runtime check would address the concern in the short run, paving\nthe path for future static checks revolving around the same area will pay\noff quite happily.\n\n> > I do not have any particular interest. If it is a tool fit for the\n> > task, it would be good to use it, that's all ;-)\n>\n> Okay, then I would like to research if that is a good fit. Johannes\n> is pretty confident about it though.\n\nYes, he is.\n\nAnd he wishes he had the time to work on it himself because it sounds like\na really fun (if challenging) project.\n\nIn other words: If you ever get stuck somewhere along the lines, please do\npush up a work-in-progress branch and reach out here so that I or others\ncan help.\n\nCiao,\nDscho\n"},{"id":"450701","messageId":"20220308054416.65257-1-chakrabortyabhradeep79@gmail.com","threadId":"57427","inReplyTo":"nycvar.QRO.7.76.6.2203071709540.11118@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v4 2/2] parse-options.c: add style checks for usage-strings","fromName":"Abhradeep Chakraborty","fromEmail":"chakrabortyabhradeep79@gmail.com","sentAt":"2022-03-08T05:44:16Z","receivedAt":"2022-03-08T05:45:46Z","isPatch":true,"sender":{"key":"chakrabortyabhradeep79@gmail.com","avatar":"https://avatars.githubusercontent.com/u/75240995?v=4"},"body":"\nJohannes Schindelin wrote:\n\n> Yes, he is.\n\n> And he wishes he had the time to work on it himself because it sounds like\n> a really fun (if challenging) project.\n>\n> In other words: If you ever get stuck somewhere along the lines, please do\n> push up a work-in-progress branch and reach out here so that I or others\n> can help.\n\nThank you so much ^^\n"}]}