Re: [PATCH] push: comment on a funny unbalanced option help

Am 02.08.2018 um 18:54 schrieb Jeff King:

Good point. We could also extend it further and check if it contains any special character, which would allow us to convert the remaining user of the flag as well:

	{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
		N_("override the executable bit of the listed files"),
		PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
		chmod_callback},

Special characters are (, ), <, >, [, ], and |.

The idea is that we shouldn't automatically treat a string as a simple replacement specifier if it looks like it has some structure to it.

Side note: "(+/-)x" is marked for translation above. Any translation that is not identical would be wrong, though, because the command only accepts a literal "+x" or "-x" in any locale. So the N_ wrapper is bogus, right?

Checked the output with that extended check by generating all help pages with:

	for cmd in $(git --list-cmds=parseopt)
	do git-$cmd -h
	done

... and found a few differences:

git add:
-    --chmod <(+/-)x>      override the executable bit of the listed files
+    --chmod (+/-)x        override the executable bit of the listed files

Good change.  We also should change the slash to a pipe.

git checkout-index:
-    --stage <1-3|all>     copy out the files from named stage
+    --stage 1-3|all       copy out the files from named stage

Good change, but perhaps mention number two explicitly?

git difftool:
-    -t, --tool <<tool>>   use the specified diff tool
+    -t, --tool <tool>     use the specified diff tool
-    -x, --extcmd <<command>>
+    -x, --extcmd <command>

Aha, double angle brackets in the wild! Good change. We could also remove the explicit pairs from the option definitions.

git pack-objects:
-    --index-version <version[,offset]>
+    --index-version version[,offset]

Not good before, worse after. Should be to "<version>[,<offset>]".

git pull:
-    -r, --rebase[=<false|true|merges|preserve|interactive>]
+    -r, --rebase[=false|true|merges|preserve|interactive]

Good change, but wouldn't we want to add a pair of parentheses around the list of alternatives?

git push:
-    --force-with-lease[=<refname>:<expect>]
+    --force-with-lease[=refname>:<expect]

Bad change, needs explicit angular brackets (Junio's patch).

-    --recurse-submodules[=<check|on-demand|no>]
+    --recurse-submodules[=check|on-demand|no]
-    --signed[=<yes|no|if-asked>]
+    --signed[=yes|no|if-asked]

git send-pack:
-    --signed[=<yes|no|if-asked>]
+    --signed[=yes|no|if-asked]

Good changes all three, but need parentheses..

-    --force-with-lease[=<refname>:<expect>]
+    --force-with-lease[=refname>:<expect]

Bad change, needs explicit angular brackets (same as in Junio's patch).

git shortlog:
-    -w[<w[,i1[,i2]]>]     Linewrap output
+    -w[w[,i1[,i2]]]       Linewrap output

Not good before, worse after.  Should be "[<w>[,<i1>[,<i2>]]]".

git update-index:
-    --cacheinfo <mode>,<object>,<path>
-                          add the specified entry to the index
+    --cacheinfo           add the specified entry to the index

Eh, what? Ah, that option is defined with PARSE_OPT_NOARG, and we only show argument help because PARSE_OPT_LITERAL_ARGHELP is also given, so we need to keep that flag for this special option.

René