On Wed, Feb 16 2022, Abhradeep Chakraborty via GitGitGadget wrote: > From: Abhra303 <chakrabortyabhradeep79@xxxxxxxxx> > > Usage strings for git (sub)command flags has a style guide that > suggests - first letter should not capitalized (unless requied) > and it should skip full-stop at the end of line. But there are > some files where usage-strings do not follow the above mentioned > guide. Moreover, there are no checks to verify if all usage strings > are following the guide/convention or not. > > Amend the usage strings that don't follow the convention/guide and > add a `CI` check for checking the usage strings (whether the first > letter is capital or it ends with full-stop). If the `check` find > such strings then print those strings and return a non-zero status. > > Also provide a script that takes an optional argument (a valid <tree> > string), to check the usage strings in the given <tree> (`HEAD` is > the default argument). > > Signed-off-by: Abhradeep Chakraborty <chakrabortyabhradeep79@xxxxxxxxx> > --- > add usage-strings ci check and amend remaining usage strings > > This patch series completely fixes #636. > > The issue is about amending the usage-strings (for command flags such as > -h, -v etc.) which do not follow the style convention/guide. There was a > PR [https://github.com/gitgitgadget/git/pull/920] addressing this issue > but as Johannes [https://github.com/dscho] said in his comment > [https://github.com/gitgitgadget/git/issues/636#issuecomment-1018660439], > there are some files that still have those kind of usage strings. > Johannes also suggested to add a CI check under ci/test-documentation.sh > to check the usage strings. > > So, in this patch, all remaining usage strings are corrected. I also > added a check-usage-strings target in Makefile which can be used to > check usage strings. It uses check-usage-strings.sh. > > 1. If check-usage-strings.sh is run on a valid git repo - it will check > the validity of usage-strings in the tree specified by an argument > or in the HEAD if no argument provided. > 2. If the current repo is not a git repo (i.e. if it doesn't find any > .git folder), it will check the usage string in the current root > directory. > > For the first case, output of make check-usage-strings or > ./check-usage-strings.sh would be similar to - > > HEAD:builtin/bisect--helper.c:1212 N_("use <cmd>... to automatically bisect."), BISECT_RUN), > HEAD:builtin/submodule--helper.c:1877 OPT__QUIET(&quiet, "Suppress output for cloning a submodule"), > > > If an argument provided - ./check-usage-strings.sh 'v2.34.0' , it will > search for usage-strings in v2.34.0 and v2.34.0 will be prefixed before > filenames instead of HEAD. > > In the second case, output would be similar to - > > diff.c:5596 N_("select files by diff type."), > diff.c:5599 N_("Output to a specific file"), > builtin/branch.c:666 OPT_BIT('C', NULL, ©, N_("copy a branch, even if target exists."), 2), > make: *** [check-usage-strings] Error 1 > > > Note in the last case - arguments provided to it will be useless. > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v1 > Pull-Request: https://github.com/gitgitgadget/git/pull/1147 Sorry about leaving this patch submission hanging. I read this at the time, but forgot to find time to loop back to it. > Makefile | 5 +++++ > builtin/bisect--helper.c | 2 +- > builtin/reflog.c | 6 +++--- > builtin/submodule--helper.c | 2 +- > check-usage-strings.sh | 33 +++++++++++++++++++++++++++++++++ > ci/test-documentation.sh | 1 + > diff.c | 2 +- > t/helper/test-run-command.c | 6 +++--- > 8 files changed, 48 insertions(+), 9 deletions(-) > create mode 100755 check-usage-strings.sh > > diff --git a/Makefile b/Makefile > index 186f9ab6190..93faed51da0 100644 > --- a/Makefile > +++ b/Makefile > @@ -3416,6 +3416,11 @@ check-docs:: > check-builtins:: > ./check-builtins.sh > > +### Make sure all the usage strings follow usage string style guide > +# > +check-usage-strings:: > + ./check-usage-strings.sh > + > ### Test suite coverage testing > # > .PHONY: coverage coverage-clean coverage-compile coverage-test coverage-report > diff --git a/builtin/bisect--helper.c b/builtin/bisect--helper.c > index 28a2e6a5750..614d95b022c 100644 > --- a/builtin/bisect--helper.c > +++ b/builtin/bisect--helper.c > @@ -1209,7 +1209,7 @@ int cmd_bisect__helper(int argc, const char **argv, const char *prefix) > OPT_CMDMODE(0, "bisect-visualize", &cmdmode, > N_("visualize the bisection"), BISECT_VISUALIZE), > OPT_CMDMODE(0, "bisect-run", &cmdmode, > - N_("use <cmd>... to automatically bisect."), BISECT_RUN), > + N_("use <cmd>... to automatically bisect"), BISECT_RUN), > OPT_BOOL(0, "no-log", &nolog, > N_("no log for BISECT_WRITE")), > OPT_END() > diff --git a/builtin/reflog.c b/builtin/reflog.c > index 85b838720c3..28372c5e2b5 100644 > --- a/builtin/reflog.c > +++ b/builtin/reflog.c > @@ -600,7 +600,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) > OPT_BIT(0, "updateref", &flags, > N_("update the reference to the value of the top reflog entry"), > EXPIRE_REFLOGS_UPDATE_REF), > - OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen.")), > + OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen")), > OPT_CALLBACK_F(0, "expire", &cmd, N_("timestamp"), > N_("prune entries older than the specified time"), > PARSE_OPT_NONEG, > @@ -613,7 +613,7 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix) > N_("prune any reflog entries that point to broken commits")), > OPT_BOOL(0, "all", &do_all, N_("process the reflogs of all references")), > OPT_BOOL(1, "single-worktree", &all_worktrees, > - N_("limits processing to reflogs from the current worktree only.")), > + N_("limits processing to reflogs from the current worktree only")), > OPT_END() > }; > > @@ -736,7 +736,7 @@ static int cmd_reflog_delete(int argc, const char **argv, const char *prefix) > OPT_BIT(0, "updateref", &flags, > N_("update the reference to the value of the top reflog entry"), > EXPIRE_REFLOGS_UPDATE_REF), > - OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen.")), > + OPT_BOOL(0, "verbose", &verbose, N_("print extra information on screen")), > OPT_END() > }; > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index c5d3fc3817f..9864ec1427d 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1874,7 +1874,7 @@ static int module_clone(int argc, const char **argv, const char *prefix) > OPT_STRING(0, "depth", &clone_data.depth, > N_("string"), > N_("depth for shallow clones")), > - OPT__QUIET(&quiet, "Suppress output for cloning a submodule"), > + OPT__QUIET(&quiet, "suppress output for cloning a submodule"), > OPT_BOOL(0, "progress", &progress, > N_("force cloning progress")), > OPT_BOOL(0, "require-init", &require_init, It's really good to have these fixed! Ditto for the remaining ones I elided. > diff --git a/check-usage-strings.sh b/check-usage-strings.sh > new file mode 100755 > index 00000000000..a4028e0d00d > --- /dev/null > +++ b/check-usage-strings.sh > @@ -0,0 +1,33 @@ > +{ > + if test -d ".git" > + then > + rev=${1:-"HEAD"} > + for entry in $(git grep -l 'struct option .* = {$' "$rev" -- \*.c); > + do > + git show "$entry" | > + sed -n '/struct option .* = {/,/OPT_END/{=;p;}' | > + sed "N;s/^\\([0-9]*\\)\\n/$(echo "$entry" | sed 's/\//\\&/g'):\\1/"; > + done > + else > + for entry in $(grep -rl --include="*.c" 'struct option .* = {$' . ); > + do > + cat "$entry" | > + sed -n '/struct option .* = {/,/OPT_END/{=;p;}' | > + sed "N;s/^\\([0-9]*\\)\\n/$(echo "$entry" | sed -e 's/\//\\&/g' -e 's/^\.\\\///'):\\1/"; > + done > + fi > +} | > +grep -Pe '((?<!OPT_GROUP\(N_\(|OPT_GROUP\()"(?!GPG|DEPRECATED|SHA1|HEAD)[A-Z]|(?<!"|\.\.)\.")' | > +{ > + status=0 > + while read content; > + do > + if test -n "$content" > + then > + echo "$content"; > + status=1; > + fi > + done > + > + exit $status > +} > diff --git a/ci/test-documentation.sh b/ci/test-documentation.sh > index de41888430a..f66848dfc66 100755 > --- a/ci/test-documentation.sh > +++ b/ci/test-documentation.sh > @@ -15,6 +15,7 @@ filter_log () { > } As much as I like the idea, I really don't want us to have this method of doing it though, i.e. to start parsing our C code with a hard-to-maintain shellscript. But the good news is that there's much easier way to add this! Aside: if we did want to do the "parse C" method the right way to do it would be to have a coccinelle script do it. We don't currently, but we use coccicheck, and if you look at the linux kernel's use of it there's multiple such checks there. I.e. you can have it parse the C and run your checks with an arbitrary script. But in this case there's really a much easier way to do this, to just extend something like this: diff --git a/parse-options.c b/parse-options.c index 2437ad3bcdd..90d8da6ad4c 100644 --- a/parse-options.c +++ b/parse-options.c @@ -492,6 +492,8 @@ static void parse_options_check(const struct option *opts) default: ; /* ok. (usually accepts an argument) */ } + if (opts->help && ends_with(opts->help, ".")) + err |= optbug(opts, xstrfmt("argh should not end with a dot: %s", opts->help)); if (opts->argh && strcspn(opts->argh, " _") != strlen(opts->argh)) err |= optbug(opts, "multi-word argh should use dash to separate words"); Then the t0012-help.sh test will catch these, and that's where these sorts of checks belong in our tree. See b6c2a0d45d4 (parse-options: make sure argh string does not have SP or _, 2014-03-23) for the existing code shown in the context where we already check "argh" like that, i.e. we're just missing a test for "help". Obviously such a function would need to hardcode some of the logic you added in your shellscript. E.g. this fires on a string ending in "...", but yours doesn't. That should be fairly easy to do though, and if not we could always just dump these to stderr or something if a git_env_bool("GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP", 0) was true, and do the testing itself in t0012-help.sh. > make check-builtins > +make check-usage-strings > make check-docs Good to have this as a "make" target, not something e.g. peculiar to CI.
