add usage-strings ci check and amend remaining usage strings

From: Abhra303 <[email protected]>

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).

---
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, &copy, 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

 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

index 186f9ab6190..93faed51da0 100644
@@ -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

index 28a2e6a5750..614d95b022c 100644
@@ -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()

index 85b838720c3..28372c5e2b5 100644
@@ -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()
 	};
 

index c5d3fc3817f..9864ec1427d 100644
@@ -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,

new file mode 100755
index 00000000000..a4028e0d00d
@@ -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
+}

index de41888430a..f66848dfc66 100755
@@ -15,6 +15,7 @@ filter_log () {
 }
 
 make check-builtins
+make check-usage-strings
 make check-docs
 
 # Build docs with AsciiDoc

index c862771a589..000be3bf232 100644
@@ -5596,7 +5596,7 @@ static void prep_parse_options(struct diff_options *options)
 			       N_("select files by diff type"),
 			       PARSE_OPT_NONEG, diff_opt_diff_filter),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
-		  N_("Output to a specific file"),
+		  N_("output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
 
 		OPT_END()

index 913775a14b7..8f370cd89f1 100644
@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)
 	struct strbuf out = STRBUF_INIT;
 	struct strvec args = STRVEC_INIT;
 	struct option options[] = {
-		OPT_INTEGER('n', "trials", &trials, "Number of trials"),
-		OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
-		OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
+		OPT_INTEGER('n', "trials", &trials, "number of trials"),
+		OPT_INTEGER('s', "skip", &skip, "skip <n> trials"),
+		OPT_BOOL('m', "msys2", &msys2, "test quoting for MSYS2's sh"),
 		OPT_END()
 	};
 	const char * const usage[] = {

base-commit: b80121027d1247a0754b3cc46897fee75c050b44

gitgitgadget

Hello reviewers and community members,

This 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).

So, could you please review my patch request?

thanks :)

Sorry about leaving this patch submission hanging. I read this at the time, but forgot to find time to loop back to it.

index 2437ad3bcdd..90d8da6ad4c 100644
@@ -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");

;-) Good suggestions. I have nothing to add.

No worries. Thanks for reviewing :)

Thank you so much for the suggestion. Didn't aware of it before. I will try to implement the logic in parse-options.c` (as you suggested).

Okay but if the logic can't be implented in the parse-options.c file (most probably I will be able to implement the logic), would you allow me to try the coccinelle script method you mentioned?

Thanks :)

In this case I think there's definitely no reason for why it can't/won't work in parse-options.c.

If you're doing something like that with coccicheck I'm afraid I can't help much, I've only seen that the kernel is doing it (it's referenced in some of the coccinelle docs), but I haven't personally used it for anything close to that.

Hi Julia,

I would like to loop you in here because you have helped us with Coccinelle questions in the past.

The task at hand is to identify calls to the macro OPT_CMDMODE() (and other, similar macros) that get a fourth argument of the form

	N_("<some string>")

The problem is to identify <some string> that ends in a . (which we want to avoid) or that starts with some prefix and a colon but follows with an upper-case character.

In other words, we want to suggest replacing

N_("log: Use something")

or

	N_("log: use something.")

by

N_("log: use something")

Ævar suggested that Coccinelle can do that. Could you give us a hand how this would be possible using spatch?

Thank you,
Johannes

Thanks. Probably better to CC the relevant ML, adding it.

I probably shouldn't have mentioned that at all, and I think this is academic in this context, because as noted we can just add this to parse_options_check() (linking it here again for off-git-ml context):

    https://lore.kernel.org/git/[email protected]/

We now pay that trivial runtime overhead every time, we could run it only during the tests if we ever got worried about it.

And it's a lot less fragile and easy to understand than running coccicheck, i.e. as nice as it is it's still takes a while to run, is its own mini-language, needs to be kept in sync with code changes etc.

So by doing it at runtime we can adjust messages, code & tests in an atomic patch more easily (i.e. not assume that you ran some cocci target to validate it).

It also makes it really easy to do things that are really hard (or impossible?) with coccinelle. I.e. some of these checks are run as a function of what flag gets passed into some function later on, which in the general case would require coccinelle to have some runtime emulator for C code just to see *what* checks it wants to run.

That being said (and with the caveat that I've only looked at this code, not done this myself) if you clone linux.git and browse through:

git grep -C100 -F coccilib.report '*.cocci'

You can see a lot of examples of using cocci for these sorts of checks.

And the same goes if you clone coccinelle.git and do:

git grep -C100 @script: -- tests

For linux.git it's documented here:
https://github.com/torvalds/linux/blob/master/Documentation/dev-tools/coccinelle.rst

I.e. it's basically writing the sort of cocci rules we have in-tree with a callback script that complaints about the required change.

For our use it would probably better (in lieu of parse_options_check(), which is the right thing here) to just have a normal *.cocci file and complain if it applies changes. We already error in the CI if those need to apply any changes.

But I don't off-hand know how to do that. E.g. I was trying the other day to come up with some coccinelle rule that converted:

    die("BUG: blah blah");

To:

    BUG("blah blah");

And while I'm sure there's some way to do this, couldn't find a way to write a rule to "reach in" to a constant string, apply some check or search/replacement on it, and to do a subsequent transformation on it.

In this case the OPT_BLAH(1, 2, 3, "string here") has OPT_BLAH() as a macro. I can't remember if there's extra caveats around using coccinelle for macros v.s. symbols.

Disclaimer: By "couldn't" I mean I grepped the above examples for all of a few minutes quickly skimmed the coccinelle docs, didn't find a template I could copy, then ended up writing some nasty grep/xargs/perl for-loop instead :)

Hello,

I'm not sure to follow all of the following.

Of there are some cases that are useful to do statically, with only local information, then using Coccinelle could be useful to get the problem out of the way once and for all. Coccinelle doesn't support much processing of strings directly, but you can always write some python code to test the contents of a string and to create a new one.

Let me know if you want to try this. You can also check, eg the demo demos/pythontococci.cocci to see how to create code in a python script and then use it in a normal SmPL rule.

If some context has to be taken into account and the context in the same function, then that can also be done with Coccinelle, eg

A
...
B

matches the case where after an A there is a B on all execution paths (except perhaps those that end in an error exit) and

A
... when exists
B

matches the case where there is a B sometime after executing A, even if that does not always occur.

If the context that you are interested in is in a called function or is in the calling context, then Coccinelle might not be the ideal choice. Coccinelle works on one function at a time, so to do anything interprocedural, you have to do some hacks.

julia

Thank you Julia for this helpful info. Looking at your description, I think the add check to parse-options.c (that Ævar suggested as the most ideal method for it) method is more simpler than this. Moreover,as this is only about checking usage-strings, so adding complexity to it will not be a good idea.

Thanks :)

Though in this case, parse-options.c check method is better, but in other cases, this might be a good fit. In those cases, I would also like to help you (i.e. you, Johannes, Ævar and other devs) to fix those cases.

Thanks :)

Hi,

I fear that this is incorrect.

In general, it is my experience that it is a mistake any time a static check is replaced by a runtime check.

I was ready to let it slide in this instance, but in this case I now have proof that the parse-options.c check is worse than the originally suggested sed chain.

That concrete proof is in the output of
https://github.com/git/git/actions/runs/1890665968, where the combination of ac/usage-string-fixups and jh/builtin-fsmonitor-part2 causes many, many failures, but all of those failures have the same root cause: the runtime check.

With the original check-usage-strings.sh, the user inspecting any failure would see precisely what the issue is, in the static-analysis job's logs. It would display something like this:

HEAD:builtin/fsmonitor--daemon.c:1507: N_("Max seconds to wait for background daemon startup")),

With v4 of the patch series, it does not spell out anything in static-analysis. Instead, it causes 8 separate jobs to fail, it causes failures not only in t0012-help.sh but also in t7519-status-fsmonitor.sh and in t7527-builtin-fsmonitor.sh.

The purpose of t7519 and t7527 is _not_ to verify those usage strings, though.

The worst part? Look at the relevant output of t0012 (see
https://github.com/git/git/runs/5312844492?check_suite_focus=true#step:5:4902):

	[...]
	++ git -C sub fsmonitor--daemon -h
	++ exit_code=128
	++ test 128 = 129
	++ echo 'test_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h'
	test_expect_code: command exited with 128, we wanted 129 git -C sub fsmonitor--daemon -h
	++ return 1
	error: last command exited with $?=1
	not ok 81 - fsmonitor--daemon can handle -h
	[...]

Do you see what usage string caused the failure? You can't. And that's even by design:

	(
		GIT_CEILING_DIRECTORIES=$(pwd) &&
		export GIT_CEILING_DIRECTORIES &&
		test_expect_code 129 git -C sub $builtin -h >output 2>&1
	) &&
	test_i18ngrep usage output

The output is redirected, and since the runtime check added to parse-options.c causes the exit code to be different from the expected one, the output is never shown.

Arguably the most important job of a regression test is to help software engineers to diagnose and fix the regression. As quickly and as conveniently as possible. That means that it is not enough to point out that there is a regression, the output should be as helpful and concise as possible to facilitate fixing the problem.

In the above-mentioned case, it was neither as helpful nor as concise as possible because in the test case that was supposed to identify the problem, the actual error message was swallowed, and instead of causing one test failure, it caused a whopping 42 test cases to fail (some of which even show the error message, but that's not even the purpose of those test cases).

Since the entire point of this here patch series is to help enforce Git's rules regarding usage strings, it should expect things like the issue with fsmonitor--daemon _and_ make it as painless to address such an issue.

I am afraid that I have to NAK the parse-options.c approach because v1 of this patch series did so much better a job.

Ciao,
Dscho

I think that's a bit of an overreaction to what I think is a solid v2 in <[email protected]>, i.e. that we must go back to v1 because we encountered this issue.

A. I think you're right about the t0012-help.sh output being bad,
but that's rather easily fixed with something like the [1] below.

I've run into that a few times, wished it was better, and manually grepped or cat'd the "output" file.

Part of that is ultimately because we're mixing and matching whether this "usage" output goes on stdout or stderr in various commands.

B. The fsmonitor--daemon case is worse than most because it's only running on
OS or Windows, i.e. the error we'd get in various other CI jobs is ifdef'd away, even though we could run the parse_options() part there.

IIRC that's something I commented on in previous rounds of that series...

C. The case of 42 tests failing because of this could be addressed by just having
t0012-help.sh do these checks if we wanted, although in that case we'd need to make sure we deal with other test blind spots. I.e. the "GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP" suggestion I had.

D. These sorts of check, by their nature, have an initial period of growing
pains due to other in-flight topics. Once we move past that it's usually a non-issue going forward, as issues will be caught locally before patch submission.

Data in favor of that is various other checks in parse_options_check() being mostly a non-issue, e.g. Junio's b6c2a0d45d4 (parse-options: make sure argh string does not have SP or _, 2014-03-23).

In this case I don't see how some minor issues when merging this with "seen" would have us abandon the v1 and commit to a fragile parsing of C code in shellscript instead, or with some coccinelle check that would have inherent issues finding the full context we need (passed-down flags etc.).

1.

index 6c3e1f7159d..5474d463467 100755
@@ -237,15 +237,24 @@ test_expect_success 'generate builtin list' '
 	git --list-cmds=builtins >builtins
 '
 
+builtin_in_sub () {
+	(
+		GIT_CEILING_DIRECTORIES=$(pwd) &&
+		export GIT_CEILING_DIRECTORIES &&
+		"$@"
+	)
+}
+
+
 while read builtin
 do
-	test_expect_success "$builtin can handle -h" '
-		(
-			GIT_CEILING_DIRECTORIES=$(pwd) &&
-			export GIT_CEILING_DIRECTORIES &&
-			test_expect_code 129 git -C sub $builtin -h >output 2>&1
-		) &&
-		test_i18ngrep usage output
+	test_expect_success "invoking '$builtin -h' yields exit code 129" '
+		builtin_in_sub test_expect_code 129 git -C sub $builtin -h
+	'
+
+	test_expect_success "invoking '$builtin -h' output" '
+		builtin_in_sub test_expect_code 129 git -C sub $builtin -h >output 2>&1 &&
+		grep usage output
 	'
 done <builtins
 

Do you think the fix you suggested should be a part of this Patch series or a dedicated patch request is needed for this?

Pardon me, I am having problem to understand the "GIT_TEST_PARSE_OPTIONS_DUMP_FIELD_HELP" suggestion I had. part here. Could you please explain a little bit?

Thanks :)

Hello,

Since it seems that Coccinelle is not useful for your problem, could you remove me from the CC list on this discussion?

thanks,
julia

Hi Julia,

Right. The code in question is not actually calling a function, but a macro, and passes a literal string to the macro that we would want to check statically.

I did have my doubts that it would be easy with Coccinelle, but since Ævar seemed so confident, I tried it, struggled, and decided to follow up with you.

Thank you for confirming my suspicion!
Johannes

Coccinelle doesn't care about whether a function is called or whether a macro is called. It considers everything to be a function.

Something like this:

@r1@
expression e;
@@

N(e)

@script:python s@
e << r1.e;
replacement;
@@

if string_ok e
then cocci.include_match(False)
else coccinelle.replacement = "\"better string\""

@@
expression r1.e;
expression s.replacement;
@@
- N(e)
+ N(replacement)

------------------

You can fill in the definition of string_ok and better string. I think the \" will be necessary, because the value of an expression metavariable at the python level is a string, so there should be a string inside of it to make it a string expression.

julia

In case it's not clear from the upthread (and I thought my [1] explained it well enough) I never thought it would be easy or even possible to do this particular thing with coccinelle.

I.e. I mentioned in [1]:

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

So it's intended as a side-note to explain to a new contributor that *if* we do end up wanting to parse or transform C we have coccinelle, and it's a great tool for those cases where such static transformations are easy. I and others have added some in-tree in the past where appropriate.

But I then went on to say (and elaborated on later in [2]) that in this case the right thing to do is runtime checking.

So, I'm sorry if you wasted time on it. In either case it seems we've ended up in agreement in this case about appropriate uses of coccinelle.

I.e. it's a fantastic tool as a semantic patch engine, but it understandably has limitations where you'd effectively need it to execute your program to decide what to do, as is the case with the parse_options() API and the eventual parse_options_check() etc. doing assertions depending on flags that got passed down.

1. https://lore.kernel.org/git/[email protected]/
2. https://lore.kernel.org/git/[email protected]/

Hello Ævar, I have some query related to this method. I have implemented the logic locally and tests are also passing. However, I think the test you mentioned is only running against the builtin files and files that are used in builtin commands (e.g. diff.c, builtin/add.c etc.). But some files from t/helper (e.g. t/helper/test-run-command.c) also uses parse option API and it seems that there are no test files (pardon me if I am wrong) for checking parse option usage strings check for t/helper test-tool commands.

E.g. grep -r --include="*.c" 'struct option .*\[] = {$' . command gives the following output -

./helper/test-parse-options.c: struct option options[] = {
./helper/test-lazy-init-name-hash.c: struct option options[] = {
./helper/test-serve-v2.c: struct option options[] = {
./helper/test-simple-ipc.c: struct option options[] = {
./helper/test-parse-pathspec-file.c: struct option options[] = {
./helper/test-getcwd.c: struct option options[] = {

./helper/test-run-command.c:    struct option options[] = {
./helper/test-run-command.c:    struct option options[] = {
./helper/test-proc-receive.c:   struct option options[] = {
./helper/test-progress.c:       struct option options[] = {

./helper/test-tool.c: struct option options[] = {

So, these files are using parse-options and there is a chance that in future, usage strings from these files may violate the style guide. In this case, all tests will be passing even if there are some style violations. What do you think?

Thanks :)

From: Abhra303 <[email protected]>

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 check in the parse_options_check() function in parse-options.c to check the usage strings against the style guide.

---
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 checks to parse_options_check() in parse-options.c (as suggested by Ævar).

Changes since v1:

1. remove check-usage-strings.sh
2. remove CI check
3. add checks to parse-options.c
4. modify t/t1502-rev-parse-parseopt.sh to pass the test

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1147

Range-diff vs v1:

1: ea0ba45d77a ! 1: 902937e768d add usage-strings ci check and amend remaining usage strings
@@ Metadata
Author: Abhra303 <[email protected]>

## Commit message ##
- add usage-strings ci check and amend remaining usage strings
+ add usage-strings check and amend remaining usage strings

Usage strings for git (sub)command flags has a style guide that suggests - first letter should not capitalized (unless requied)
@@ Commit message
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).
+ add a check in the parse_options_check() function in parse-options.c
+ to check the usage strings against the style guide.

- ## Makefile ##
-@@ Makefile: 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
-
## builtin/bisect--helper.c ##
@@ builtin/bisect--helper.c: int cmd_bisect__helper(int argc, const char **argv, const char *prefix)
OPT_CMDMODE(0, "bisect-visualize", &cmdmode,
@@ builtin/submodule--helper.c: static int module_clone(int argc, const char **argv
N_("force cloning progress")),
OPT_BOOL(0, "require-init", &require_init,

- ## check-usage-strings.sh (new) ##
-@@
-+{
-+ 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
-+}
-
- ## ci/test-documentation.sh ##
-@@ ci/test-documentation.sh: filter_log () {
- }
-
- make check-builtins
-+make check-usage-strings
- make check-docs
-
- # Build docs with AsciiDoc
-
## diff.c ##
@@ diff.c: static void prep_parse_options(struct diff_options *options)
N_("select files by diff type"),
@@ diff.c: static void prep_parse_options(struct diff_options *options)

OPT_END()

+ ## parse-options.c ##
+@@ parse-options.c: static void parse_options_check(const struct option *opts)
+ default:
+ ; /* ok. (usually accepts an argument) */
+ }
++ if (opts->type != OPTION_GROUP && opts->help &&
++ !(starts_with(opts->help, "HEAD") ||
++ starts_with(opts->help, "GPG") ||
++ starts_with(opts->help, "DEPRECATED") ||
++ starts_with(opts->help, "SHA1")) &&
++ (opts->help[0] >= 65 && opts->help[0] <= 90))
++ err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
++ if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
++ err |= optbug(opts, xstrfmt("help 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");
+
## t/helper/test-run-command.c ##
@@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char **argv)
struct strbuf out = STRBUF_INIT;
@@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char *
OPT_END()
};
const char * const usage[] = {
+
+ ## t/t1502-rev-parse-parseopt.sh ##
+@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'setup optionspec-only-hidden-switches' '
+ |
+ |some-command does foo and bar!
+ |--
+-|hidden1* A hidden switch
++|hidden1* a hidden switch
+ EOF
+ '
+
+@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '
+ |
+ | some-command does foo and bar!
+ |
+-| --hidden1 A hidden switch
++| --hidden1 a hidden switch
+ |
+ |EOF
+ END_EXPECT

 builtin/bisect--helper.c      | 2 +-
 builtin/reflog.c              | 6 +++---
 builtin/submodule--helper.c   | 2 +-
 diff.c                        | 2 +-
 parse-options.c               | 9 +++++++++
 t/helper/test-run-command.c   | 6 +++---
 t/t1502-rev-parse-parseopt.sh | 4 ++--
 7 files changed, 20 insertions(+), 11 deletions(-)

index 28a2e6a5750..614d95b022c 100644
@@ -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()

index 85b838720c3..28372c5e2b5 100644
@@ -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()
 	};
 

index 33c82c3ab91..6332d305983 100644
@@ -1875,7 +1875,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,

index 7d5cfd325ea..387435a4a45 100644
@@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)
 			       N_("select files by diff type"),
 			       PARSE_OPT_NONEG, diff_opt_diff_filter),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
-		  N_("Output to a specific file"),
+		  N_("output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
 
 		OPT_END()

index 2437ad3bcdd..91cbfb0d7f7 100644
@@ -492,6 +492,15 @@ static void parse_options_check(const struct option *opts)
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
+		if (opts->type != OPTION_GROUP && opts->help &&
+			!(starts_with(opts->help, "HEAD") ||
+			  starts_with(opts->help, "GPG") ||
+			  starts_with(opts->help, "DEPRECATED") ||
+			  starts_with(opts->help, "SHA1")) &&
+			  (opts->help[0] >= 65 && opts->help[0] <= 90))
+			err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
+		if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
+			err |= optbug(opts, xstrfmt("help 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");

index 913775a14b7..8f370cd89f1 100644
@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)
 	struct strbuf out = STRBUF_INIT;
 	struct strvec args = STRVEC_INIT;
 	struct option options[] = {
-		OPT_INTEGER('n', "trials", &trials, "Number of trials"),
-		OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
-		OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
+		OPT_INTEGER('n', "trials", &trials, "number of trials"),
+		OPT_INTEGER('s', "skip", &skip, "skip <n> trials"),
+		OPT_BOOL('m', "msys2", &msys2, "test quoting for MSYS2's sh"),
 		OPT_END()
 	};
 	const char * const usage[] = {

index 284fe18e726..2a07e130b96 100755
@@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '
 |
 |some-command does foo and bar!
 |--
-|hidden1* A hidden switch
+|hidden1* a hidden switch
 EOF
 '
 

@@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --hidden1             a hidden switch
 |
 |EOF
 END_EXPECT

base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e

gitgitgadget

s/requied/required/

This is a relatively minor observation, but it might make sense to split this into two patches, the first of which fixes the offending usage strings, and the second which adds the check to parse-options.c to prevent more offending strings from entering the project in the future. Anyhow, not necessarily worth a reroll.

    if (opts->type != OPTION_GROUP && opts->help &&
        opts->help[0] && isupper(opts->help[0]) &&
        !(opts->help[1] && isupper(opts->help[1])))

Thanks for pointing out. Will fix it.

I made a single commit because both changes are small. But I think You're right - I should split it into two.

Hmm, this was a quick fix came into my mind. So, I didn't look at other (and better) options for this. Will fix it :)

Okay, got it!

Thanks :)

Yeah, that sounds like a quite sensible split.

I notice that the real-looking name

does not match with the in-body "From:" that has less real-looking. Please fix the in-body "From:" if this is rerolled so that both mention the same "Human Readable Name <[email protected]>".

Much better than what was posted, but such a heuristic deserves some in-code comment to check why we see the first two.

Yes, I had the same thought as soon as I walked away from the computer and was going to post a follow-up email to say as much but got distracted by other things and never got around to it. Thanks for filling in the gap.

Okay, fixing it. Unfortunately your review came after sending the third version[1], so this is not fixed in the newest version. But could you review the newest version of this patch series so that I can make all the suggested changes in one go?

Okay, will add comments to it.

Thanks :)

[1] https://lore.kernel.org/git/[email protected]/

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.

In this version, the previously single commit is split into two commits ( one addressing amending of usage strings and another is for adding the style checks to parse_options_check()) and the checks are simplified.

Changes since v1:

1. remove check-usage-strings.sh
2. remove CI check
3. add checks to parse-options.c
4. modify t/t1502-rev-parse-parseopt.sh to pass the test

Until v1:

A shell script check-usage-strings.sh was introduced to check the usage-strings. CI check for the same was also introduced.

Abhra303 (1):
amend remaining usage strings according to style guide

Abhradeep Chakraborty (1):
parse-options.c: add style checks for usage-strings

 builtin/bisect--helper.c      | 2 +-
 builtin/reflog.c              | 6 +++---
 builtin/submodule--helper.c   | 2 +-
 diff.c                        | 2 +-
 parse-options.c               | 6 ++++++
 t/helper/test-run-command.c   | 6 +++---
 t/t1502-rev-parse-parseopt.sh | 4 ++--
 7 files changed, 17 insertions(+), 11 deletions(-)

base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1147

Range-diff vs v2:

1: 902937e768d ! 1: f425e36b7ea add usage-strings check and amend remaining usage strings
@@ Metadata
Author: Abhra303 <[email protected]>

## Commit message ##
- add usage-strings check and amend remaining usage strings
+ amend remaining usage strings according to style guide

Usage strings for git (sub)command flags has a style guide that
- suggests - first letter should not capitalized (unless requied)
+ suggests - first letter should not capitalized (unless required)
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.
+ guide.

- Amend the usage strings that don't follow the convention/guide and
- add a check in the parse_options_check() function in parse-options.c
- to check the usage strings against the style guide.
+ Amend the usage strings that don't follow the style convention/guide.

@@ diff.c: static void prep_parse_options(struct diff_options *options)

OPT_END()

- ## parse-options.c ##
-@@ parse-options.c: static void parse_options_check(const struct option *opts)
- default:
- ; /* ok. (usually accepts an argument) */
- }
-+ if (opts->type != OPTION_GROUP && opts->help &&
-+ !(starts_with(opts->help, "HEAD") ||
-+ starts_with(opts->help, "GPG") ||
-+ starts_with(opts->help, "DEPRECATED") ||
-+ starts_with(opts->help, "SHA1")) &&
-+ (opts->help[0] >= 65 && opts->help[0] <= 90))
-+ err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
-+ if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
-+ err |= optbug(opts, xstrfmt("help 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");
-
## t/helper/test-run-command.c ##
@@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char **argv)
struct strbuf out = STRBUF_INIT;
@@ t/helper/test-run-command.c: static int quote_stress_test(int argc, const char *
OPT_END()
};
const char * const usage[] = {
-
- ## t/t1502-rev-parse-parseopt.sh ##
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'setup optionspec-only-hidden-switches' '
- |
- |some-command does foo and bar!
- |--
--|hidden1* A hidden switch
-+|hidden1* a hidden switch
- EOF
- '
-
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '
- |
- | some-command does foo and bar!
- |
--| --hidden1 A hidden switch
-+| --hidden1 a hidden switch
- |
- |EOF
- END_EXPECT
-: ----------- > 2: 9d42bdbff6c parse-options.c: add style checks for usage-strings

gitgitgadget

From: Abhra303 <[email protected]>

Usage strings for git (sub)command flags has a style guide that suggests - first letter should not capitalized (unless required) 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.

Amend the usage strings that don't follow the style convention/guide.

---
 builtin/bisect--helper.c    | 2 +-
 builtin/reflog.c            | 6 +++---
 builtin/submodule--helper.c | 2 +-
 diff.c                      | 2 +-
 t/helper/test-run-command.c | 6 +++---
 5 files changed, 9 insertions(+), 9 deletions(-)

index 28a2e6a5750..614d95b022c 100644
@@ -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()

index 85b838720c3..28372c5e2b5 100644
@@ -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()
 	};
 

index 33c82c3ab91..6332d305983 100644
@@ -1875,7 +1875,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,

index 7d5cfd325ea..387435a4a45 100644
@@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)
 			       N_("select files by diff type"),
 			       PARSE_OPT_NONEG, diff_opt_diff_filter),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
-		  N_("Output to a specific file"),
+		  N_("output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
 
 		OPT_END()

index 913775a14b7..8f370cd89f1 100644
@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)
 	struct strbuf out = STRBUF_INIT;
 	struct strvec args = STRVEC_INIT;
 	struct option options[] = {
-		OPT_INTEGER('n', "trials", &trials, "Number of trials"),
-		OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
-		OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
+		OPT_INTEGER('n', "trials", &trials, "number of trials"),
+		OPT_INTEGER('s', "skip", &skip, "skip <n> trials"),
+		OPT_BOOL('m', "msys2", &msys2, "test quoting for MSYS2's sh"),
 		OPT_END()
 	};
 	const char * const usage[] = {

gitgitgadget

From: Abhradeep Chakraborty <[email protected]>

parse-options.c doesn't check if the usage strings for option flags are following the style guide or not. Style convention says, usage strings should not start with capital letter (unless needed) and it should not end with ..

Add checks to the parse_options_check() function to check usage strings against the style convention.

---
 parse-options.c               | 6 ++++++
 t/t1502-rev-parse-parseopt.sh | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

index 2437ad3bcdd..eb92290a63a 100644
@@ -492,6 +492,12 @@ static void parse_options_check(const struct option *opts)
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
+		if (opts->type != OPTION_GROUP && opts->help &&
+			opts->help[0] && isupper(opts->help[0]) &&
+			!(opts->help[1] && isupper(opts->help[1])))
+			err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
+		if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
+			err |= optbug(opts, xstrfmt("help 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");

index 284fe18e726..2a07e130b96 100755
@@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '
 |
 |some-command does foo and bar!
 |--
-|hidden1* A hidden switch
+|hidden1* a hidden switch
 EOF
 '
 

@@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --hidden1             a hidden switch
 |
 |EOF
 END_EXPECT

gitgitgadget

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.

In this version, comments added and the From field of the first commit message is updated (i.e. "Abhradeep Chakraborty" instead of "Abhra303")

Changes since v2:

1. split the single commit into two logically separated commits ( one addressing amending of usage strings and another is for adding the style checks to parse_options_check())
2. the checks are simplified.

Changes since v1:

1. remove check-usage-strings.sh
2. remove CI check
3. add checks to parse-options.c
4. modify t/t1502-rev-parse-parseopt.sh to pass the test

Until v1:

A shell script check-usage-strings.sh was introduced to check the usage-strings. CI check for the same was also introduced.

Abhradeep Chakraborty (2):
amend remaining usage strings according to style guide parse-options.c: add style checks for usage-strings

 builtin/bisect--helper.c      |  2 +-
 builtin/reflog.c              |  6 +++---
 builtin/submodule--helper.c   |  2 +-
 diff.c                        |  2 +-
 parse-options.c               | 11 +++++++++++
 t/helper/test-run-command.c   |  6 +++---
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 7 files changed, 22 insertions(+), 11 deletions(-)

base-commit: e6ebfd0e8cbbd10878070c8a356b5ad1b3ca464e
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1147%2FAbhra303%2Fusage_command_amend-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1147/Abhra303/usage_command_amend-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1147

Range-diff vs v3:

1: f425e36b7ea ! 1: dc200d098ae amend remaining usage strings according to style guide
@@
## Metadata ##
-Author: Abhra303 <[email protected]>
+Author: Abhradeep Chakraborty <[email protected]>

## Commit message ##
amend remaining usage strings according to style guide
2: 9d42bdbff6c ! 2: e1c5a325826 parse-options.c: add style checks for usage-strings

     @@ parse-options.c: static void parse_options_check(const struct option *opts)
       		default:
       			; /* ok. (usually accepts an argument) */
       		}
     ++
     ++		// OPTION_GROUP should be ignored
     ++		// if the first two characters of the help string are uppercase, then assume it is an
     ++		// acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
     ++		// else assume the usage string is violating the style convention and throw error.
      +		if (opts->type != OPTION_GROUP && opts->help &&
      +			opts->help[0] && isupper(opts->help[0]) &&
      +			!(opts->help[1] && isupper(opts->help[1])))

gitgitgadget

From: Abhradeep Chakraborty <[email protected]>

Usage strings for git (sub)command flags has a style guide that suggests - first letter should not capitalized (unless required) 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.

Amend the usage strings that don't follow the style convention/guide.

---
 builtin/bisect--helper.c    | 2 +-
 builtin/reflog.c            | 6 +++---
 builtin/submodule--helper.c | 2 +-
 diff.c                      | 2 +-
 t/helper/test-run-command.c | 6 +++---
 5 files changed, 9 insertions(+), 9 deletions(-)

index 28a2e6a5750..614d95b022c 100644
@@ -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()

index 85b838720c3..28372c5e2b5 100644
@@ -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()
 	};
 

index 33c82c3ab91..6332d305983 100644
@@ -1875,7 +1875,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,

index 7d5cfd325ea..387435a4a45 100644
@@ -5630,7 +5630,7 @@ static void prep_parse_options(struct diff_options *options)
 			       N_("select files by diff type"),
 			       PARSE_OPT_NONEG, diff_opt_diff_filter),
 		{ OPTION_CALLBACK, 0, "output", options, N_("<file>"),
-		  N_("Output to a specific file"),
+		  N_("output to a specific file"),
 		  PARSE_OPT_NONEG, NULL, 0, diff_opt_output },
 
 		OPT_END()

index 913775a14b7..8f370cd89f1 100644
@@ -221,9 +221,9 @@ static int quote_stress_test(int argc, const char **argv)
 	struct strbuf out = STRBUF_INIT;
 	struct strvec args = STRVEC_INIT;
 	struct option options[] = {
-		OPT_INTEGER('n', "trials", &trials, "Number of trials"),
-		OPT_INTEGER('s', "skip", &skip, "Skip <n> trials"),
-		OPT_BOOL('m', "msys2", &msys2, "Test quoting for MSYS2's sh"),
+		OPT_INTEGER('n', "trials", &trials, "number of trials"),
+		OPT_INTEGER('s', "skip", &skip, "skip <n> trials"),
+		OPT_BOOL('m', "msys2", &msys2, "test quoting for MSYS2's sh"),
 		OPT_END()
 	};
 	const char * const usage[] = {

gitgitgadget

From: Abhradeep Chakraborty <[email protected]>

parse-options.c doesn't check if the usage strings for option flags are following the style guide or not. Style convention says, usage strings should not start with capital letter (unless needed) and it should not end with ..

Add checks to the parse_options_check() function to check usage strings against the style convention.

---
 parse-options.c               | 11 +++++++++++
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 13 insertions(+), 2 deletions(-)

index 2437ad3bcdd..acd9ddbb372 100644
@@ -492,6 +492,17 @@ static void parse_options_check(const struct option *opts)
 		default:
 			; /* ok. (usually accepts an argument) */
 		}
+
+		// OPTION_GROUP should be ignored
+		// if the first two characters of the help string are uppercase, then assume it is an
+		// acronym (i.e. "GPG") or special name (i.e. "HEAD"), thus allowed.
+		// else assume the usage string is violating the style convention and throw error.
+		if (opts->type != OPTION_GROUP && opts->help &&
+			opts->help[0] && isupper(opts->help[0]) &&
+			!(opts->help[1] && isupper(opts->help[1])))
+			err |= optbug(opts, xstrfmt("help should not start with capital letter unless needed: %s", opts->help));
+		if (opts->help && !ends_with(opts->help, "...") && ends_with(opts->help, "."))
+			err |= optbug(opts, xstrfmt("help 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");

index 284fe18e726..2a07e130b96 100755
@@ -53,7 +53,7 @@ test_expect_success 'setup optionspec-only-hidden-switches' '
 |
 |some-command does foo and bar!
 |--
-|hidden1* A hidden switch
+|hidden1* a hidden switch
 EOF
 '
 

@@ -131,7 +131,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --hidden1             a hidden switch
 |
 |EOF
 END_EXPECT

gitgitgadget

"Abhradeep Chakraborty via GitGitGadget" <[email protected]>

Oh, sorry for that. I was in kind of a hurry ( today was my semester exam), so I didn't look at the style guide. Will fix it.

hmm, I wanted a formatting function to format (i.e. pass the opt->help dynamically) the output string. The existing use of optbug() that you specified has no %s formatter; it is a plain string. That's why I used xstrfmt(). Moreover, it was in Ævar's suggestion[1] -

Sorry that I've given you a trick question, when I know you are quite new to the community.

I think the right answer to "Do we care?" is "In this case, because we are about to call exit(), we don't care. The extra complexity and code necessary to retain the memory we get from xstrfmt and free it is not worth it." It's not like we do this in a loop that iterates unbounded number of times before the exit() happens (in which case we should care).

Thanks.

There is nothing to say sorry. Every review comment is teaching me new things. E.g. If you didn't ask me this question, I would not go to the codebase and see the proper handling of xstrfmt. So, thanks.

Got it.

Thanks :)

Hi Abhradeep,

As I just pointed out in
https://lore.kernel.org/git/[email protected]/,
it seems that replacing the static check presented in v1 by a runtime check needs to be reverted.

In addition to the example I presented, there is another compelling reason to do so: with the static check, we can detect incorrect usage strings in all code, even in code that is platform-dependent (such as in fsmonitor--daemon).

Ciao,
Dscho

First of all, thank you so much for putting so much time to look into my PR. I appriciate your research about various possible outcomes of this Patch request.

I saw your mail where you listed some of the disadvantages of the current version. I also agree with the arguments you provided and it is also true that one wouldn't find any clue by seeing the output of the CI link
you mentioned (i.e. https://github.com/git/git/runs/5312914410?check_suite_focus=true).

Let's see what Junio, Ævar and others say about this.

Thanks :)

Sorry, but I am not sure how that conclusion follows from a breakage in a topic in flight that was discovered by the check.

I do not know if a coccinelle based solution is sufficiently easy, simple and robust enough to encourage us to scrap what has already been proposed and reviewed, instead of leaving it as a topic for a future incremental improvement that we can make on top.

Yes and no.

I would imagine that large enough platforms that have their own conditionally compiled #ifdef/#endif block already have CI to build their conditionally compiled block in practice. I do not see the above as a compelling reason to grow and shift the scope of these two patches.

Thanks.

Let's instead drop [2/2] for now. I do not want us to go back to shell script that pretends to know about C language, and I do not want to block [1/2] by waiting for a replacement. Fixes in [1/2] are pretty much uncontroversial ones that can even be fast-tracked down to 'master'.

Though, as a new contributor, I felt bad about dropping the last patch, but if you think the last patch request needs more discussion ( which I think is needed) - I also in favour of dropping the last commit.

Would you do this on your side or I will re-submit it with the first commit?

Thanks :)

Nah, I can just discard the second commit and keep the first one.

Okay, that's great. But one thing I want to ask - How the discussion for adding check for usage strings will be held i.e. Whether the idea is discarded for now.

If it is not discarded, then how to proceed? Johannes prefers the first version and Ævar prefers the add check to parse-options.c version.

Thanks :)

My take on it is that the "first version" that uses an ad-hoc shell script will not become acceptably robust. If coccinelle or other static analyzer can help us check more reliably, that would be great because we won't incur runtime cost of checking, like the embedded check we added in the latest version that we are tentatively removing.

I also think Dscho simply overreacted only because the check broke an in-flight topic that is from his group, which is not universally built, and the tests in it was written in such a way that the error output from the embedded check was not immediately available when run in the CI, making it harder to debug. None of that is a fault in the approach of using the embedded check.

If the embedded check were there from the beginning, together with tons of the existing checks done by parse_options_check(), the developers themselves of the in-flight topic(s) would have caught the problem, even before it hit the public CI. I am very sure Dscho wouldn't have complained or even noticed that you added a new check to the parse_options_check().

So from my point of view, plan should be

(0) I have been assuming that the check we removed tentatively is correct and the breakage in in-flight topic caught usage strings that were malformed. If not, we need to tweak it to make sure it does not produce false positives.

(1) Help Microsoft folks fix the in-flight topic with faulty usage strings.

(2) Rethink if parse_options_check() can be made optional at runtime, which would (a) allow our test to enable it, and allow us to test all code paths that use parse_options() centrally, and (b) allow us to bypass the check while the end-user runs "git", to avoid overhead of checking the same option[] array, which does not change between invocations of "git", over and over again all over the world.

We may add the check back to parse_options_check() after doing the above. There are already tons of "check sanity of what is inside option[]" in there, and it would be beneficial if we can separate out from parse_options_start() the sanity checking code, regardless of this topic.

(3) While (2) is ongoing, we can let people also explore static analysis possibilities.

Agreed.

This is a good idea, but while t0012-help.sh catches most of it, it doesn't cover e.g. sub-commands that call parse_options() in N functions one after the other.

We could that in t0012-help.sh with (pseudocode):

for subcmd write verify ...
do
test_expect_success '...' 'git commit-graph $subcmd -h'
done

etc., but that still won't catch *all* of it, and we don't have a way to spew out "what commands use subcommands".

Hence why we need to run the rest of the test suite, and why these things aren't some one-off GIT_TEST_ mode or t/helper/*.c code already.

I think with in-flight concerns with (0) and (1) addressed what we have here is really good enough for now, and we could just add it to the existing parse_options_check() without needing (2) and (3) at this point.

Hmm, that's true.

I agree with you. But I think these two points(specially (2)) deserve a dedicated discussion/patch thread. Because, the latest version of this patch series (actually this patch series itself) only cares about the usage strings.

So, I argue you to not discard the last commit for now. As you said There are already tons of "check sanity of what is inside option[]", integrating one more sanity check would not affect it. I am saying it not because I made that commit. The discussion or patch integration of (2) and (3) may take few weeks (or more than a month may be; I also would like to take part/contribute to that discussion/PR). I fear that another set of invalid usage-strings would be added in that time. In that case, we have to make another commit/PR for correcting those strings - disrupting the purpose of this first commit you are willing to merge.

As Ævar also said -

Thanks :)

Yes, absolutely. So applying [2/2] in haste is not a good idea at all. Before we accumulate more cruft on top, we should stop and think if the approach we are taking is sensible to begin with, or we'll make an already bad situation even worse.

Hi Junio,

It is unfortunate that the challenge had been characterized as "parse C", when in reality we are talking about highly idiomatic code. It's not like we accept arbitrary input in the OPT_...() lines. We _really_ want the option usage string to be a string that is enclosed in N_().

Additionally, this is about Git's own code, not arbitrary C code provided by users. That makes that shell script more on par with t/chainlint.sed than with contrib/coccinelle/*.

Having said that...

I think that Julia gave us enough to work with, so we can (ab-)use Coccinelle for static usage string checks, and we should probably do that, too.

No, I would have reacted the same way if I had seen the failures in any other topic, with an equally trivial fix that blooms into 42 separate test case failures.

This explosion made me realize _why_ I found the suggestion to patch parse_options() iffy in the first place: it replaces a static check with a runtime check, which is almost always something that is regretted later.

And since Abhradeep is a new contributor, I found it important to steer the direction toward sound advice that they can use over and over again over the course of their career: whenever possible, prefer static checks over runtime ones.

Indeed, if no static check had been proposed first, I would not have caught on to the unfortunate suggestion to use a runtime check _instead_.

You're so sweet, but I already did that in parallel.

Of course, if we can convince Coccinelle (together with Python) to give us the static check, we might very well be able to port more of parse_options_check() from runtime checks to static ones, which would be a clear win.

If that is possible, we could save ourselves a lot of time by skipping (2) altogether.

And as I said, Julia's advice looked really good. If only I wasn't desperately short on time, I would have given it a try because it sounds not only fun but also very, very useful in Git's context.

Ciao,
Dscho

Thanks Johannes for the advice. I will always remember it ^^

Since Junio and you both have an interest in Coccinelle, if you allow, I want to look into it.

Thanks :)

Yup, if we can have static and dynamic checks of the same quality, static checks are always better alternative. In this case, runtime check would probably be an expedite solution suitable for a shorter term to fill the gap, as a static check with the same quality as it would probably need some time to develop.

I do not have any particular interest. If it is a tool fit for the task, it would be good to use it, that's all ;-)

Got it!

Okay, then I would like to research if that is a good fit. Johannes is pretty confident about it though.

Thanks :)

Hi,

While the runtime check would address the concern in the short run, paving the path for future static checks revolving around the same area will pay off quite happily.

Yes, he is.

And he wishes he had the time to work on it himself because it sounds like a really fun (if challenging) project.

In other words: If you ever get stuck somewhere along the lines, please do push up a work-in-progress branch and reach out here so that I or others can help.

Ciao,
Dscho

Thank you so much ^^

The array of options given to the parse-options API is sanity checked for reuse of a single-letter option for multiple entries and other programmer mistakes by calling parse_options_check() from parse_options_start(). This allows our developers to catch silly mistakes early, but all callers of parse-options API pays this cost. Once the set of options in an array is validated and passes this check, until a programmer modifies the array, there is no way for it to fail the check, which is wasteful.

Introduce the GIT_TEST_PARSE_OPTIONS_CHECK environment variable and make the sanity check only when it is set to true. Set it in t/test-lib.sh so that our tests will continue to catch buggy options arrays.

---

This looked too easy and there may be some pitfalls, but I am hoping that we will know soon enough by floating a weather balloon like this.

 parse-options.c | 12 +++++++++++-
 t/README        |  5 +++++
 t/test-lib.sh   |  3 +++
 3 files changed, 19 insertions(+), 1 deletion(-)

index 6e57744fd2..02cfe3f2cd 100644
@@ -439,6 +439,14 @@ static void check_typos(const char *arg, const struct option *options)
 	}
 }
 
+/*
+ * Check the sanity of contents of opts[] array to find programmer
+ * mistakes (like duplicated short options).
+ *
+ * This function is supposed to be no-op when it returns without
+ * dying, making a call from parse_options_start_1() to it optional
+ * in end-user builds.
+ */
 static void parse_options_check(const struct option *opts)
 {
 	int err = 0;

@@ -523,7 +531,9 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
 	if ((flags & PARSE_OPT_ONE_SHOT) &&
 	    (flags & PARSE_OPT_KEEP_ARGV0))
 		BUG("Can't keep argv0 if you don't have it");
-	parse_options_check(options);
+
+	if (git_env_bool("GIT_TEST_PARSE_OPTIONS_CHECK", 0))
+		parse_options_check(options);
 }
 
 void parse_options_start(struct parse_opt_ctx_t *ctx,

index f48e0542cd..b7285531f2 100644
@@ -472,6 +472,11 @@ a test and then fails then the whole test run will abort. This can help to make
 sure the expected tests are executed and not silently skipped when their
 dependency breaks or is simply not present in a new environment.
 
+GIT_TEST_PARSE_OPTIONS_CHECK=<boolean>, when true, makes all options
+array passed to the parse-options API to be sanity checked.  This
+environment variable is set to true by test-lib.sh unless it is set.
+
+
 Naming Tests
 ------------
 

index e4716b0b86..805f495fd4 100644
@@ -474,6 +474,9 @@ export GIT_DEFAULT_HASH
 GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"
 export GIT_TEST_MERGE_ALGORITHM
 
+: ${GIT_TEST_PARSE_OPTIONS_CHECK:=1}
+export GIT_TEST_PARSE_OPTIONS_CHECK
+
 # Tests using GIT_TRACE typically don't want <timestamp> <file>:<line> output
 GIT_TRACE_BARE=1
 export GIT_TRACE_BARE

2.35.1-354-g715d08a9e5

That's not true due to the "git rev-parse --parseopt" interface. I'd be happy to deprecate it, but I think the last time I brought it up you were opposed, i.e. it's documented as plumbing in "git-rev-parse", and it's easy to have it hit some of these BUG()'s.

I see the benifit of Johannes's suggestion of checking this once (but with t0012-help.sh etc. we're nowhere near being able to do that).

Now this runs for the whole test suite, so our tests will have the the same behavior.

So it's just an optimization? Isn't it premature, if you run parse_options_check() in a loop how many checks/sec can we do? I haven't tested, but I'm betting it's a *lot*.

So aren't we shaving microseconds off the runtime here?

Meaning that a parse-options array can be fed by "rev-parse --parseopt" and having the sanity check enabled does help the use case? Even there, I would say that once the script writer finishes developing the script that uses "rev-parse --parseopt", setting the parseopt input in stone, there is no need to check the same thing over and over again. Am I mistaken? Does "rev-parse --parseopt" that is fed the same input sometimes trigger the sanity check and sometimes not?

The code for sanity check is there ONLY to help those who develop while they develop, and it is logical to enable it during our tests. There is no reason to trigger the sanity check in the end-user environment, no?

No, the problem I have with the runtime check is more at the conceptual level. Those who remove assert() by setting _NDEBUG would not be doing so to save nanoseconds, either.

If we're declaring that "git rev-parse --parseopt" is something that was only ever intended for in-tree usage sure, that should hold true.

I.e. "git rev-parse" is documented as plumbing, and we document --parseopt as a generic option parsing mechanism you can use in shellscripts.

So out-of-tree users wouldn't guard against GIT_TEST_PARSE_OPTIONS_CHECK, and I wouldn't be surprised if we could e.g. segfault on some subsequent code if some of the sanity checks aren't happening anymore.

No, I'd be quite happy if we declared that it's for our use only, and could remove it when the last in-tree *.sh user went away. there's a bit of complexity in parse_options() required only for its use....

I don't see the benefit of skipping it. Your commit message mentions "but all callers of parse-options API pays this cost". As a quick & dumb perf test I tried:

	diff --git a/parse-options.c b/parse-options.c
	index 6e57744fd22..cabea35e8b1 100644
	--- a/parse-options.c
	+++ b/parse-options.c
	@@ -523,7 +523,10 @@ static void parse_options_start_1(struct parse_opt_ctx_t *ctx,
	        if ((flags & PARSE_OPT_ONE_SHOT) &&
	            (flags & PARSE_OPT_KEEP_ARGV0))
	                BUG("Can't keep argv0 if you don't have it");
	-       parse_options_check(options);
	+       while (1) {
	+               printf(".");
	+               parse_options_check(options);
	+       }
	 }
	 
	 void parse_options_start(struct parse_opt_ctx_t *ctx,

And:

    ./git [am|rebase] | pv >/dev/null

Get around 4MiB/s. I.e. we can do this check ~4 million times/sec on my computer, with -O3, with -O0 -g it's ~3MiB/s.

So the performance cost is trivial & not worth worrying about.

I think the trade-off of not having to worry about the runtime v.s. "development build" checks is one we've done well with BUG(), i.e. not to have it be an assert().

E.g. in this case we have parse_options_concat(), so you can dynamically construct the options to be checked.

I happen to have looked in detail at all of that code in the past, and I don't *think* it's doing something "actually dynamic". I.e. it should be the same when the tests run and when git runs in the wild.

But having to know and check that when using or changing the API is just more state to keep in your head.

I do not see any need for such a declaration. We are not changing the behaviour of "git rev-parse --parseopt" plumbing command at all for those who feed valid input to it.

"rev-parse --parseopt" users can keep using their scripts just the same as before, debugging their scripts to catch silly mistakes like duplicated short options may become slightly harder, but they still have a way to ask for the same debugging support available.

Yes, I am saying that is perfectly fine, and both in-tree and out-of-tree users have a way to reinstate the sanity checks. I also do not mind if your proposal were one of these:

* introduce --parseopt-with-sanity-check to "rev-parse" and arrange the parse_options_check() call to be made when the command was invoked with it; or

* introduce --parse-opt-without-sanity-check to "rev-parse", and arrange the parse_options_check() call to be still made when "--parse-opt" is used. Those who finished developing their scripts can rewrite their --parse-opt to "without" version for conceptual cleanliness.

I already said I am not worried about it, didn't I? These numbers do not matter in this discussion.

Sorry, but I really don't see the point then.

You'd like to keep "git rev-parse --parseopt", but now if you feed bad input to it you'll get worse error messages from it, and it's not for a performance benefit then why? Why would we have worse error reporting without any upside?

Another common case would be locally hacking a command that uses parse_options(), having it do the wrong thing for some cryptic reason we'd catch in parse_options_check().

Then eventually remember to turn on this GIT_TEST_* knob (i.e. if testing via the command-line/debugger instead of the test suite). I for one do that a lot when working on the parse_options()-using commands in-tree, if this land I'll probably remember to add this knob to my .bashrc, but everyone else will find out the hard way...