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

["René Scharfe" <l.s.r@web.de>]

Am 02.08.2018 um 18:54 schrieb Jeff King:
> PS I actually would have made the rule simply "does it begin with a
>     '<'", which seems simpler still. If people accidentally write "<foo",
>     forgetting to close their brackets, that is a bug under both the
>     old and new behavior (just with slightly different outcomes).

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é