Re: [PATCH] ls-tree: fix --no-full-name

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

Am 21.07.23 um 16:37 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>
>> Overall I get the impression that having the negative form enabled by
>> default was not a good idea.  For boolean options it makes sense, for
>> options with arguments perhaps as well, but for OPT_SET_INT we would
>> have less confusion if the negated form was opt-in.
>>
>> To make it easier discoverable we could let the short help include
>> the optional "no-" part, which would look like this:
>>
>> usage: git ls-tree [<options>] <tree-ish> [<path>...]
>>
>>     -d                    only show trees
>>     -r                    recurse into subtrees
>>     -t                    show trees when recursing
>>     -z                    terminate entries with NUL byte
>>     -l, --long            include object size
>>     --name-only           list only filenames
>>     --name-status         list only filenames
>>     --object-only         list only objects
>>     --[no-]full-name      use full path names
>>     --[no-]full-tree      list entire tree; not just current directory (implies --full-name)
>>     --format <format>     format to use for the output
>>     --[no-]abbrev[=<n>]   use <n> digits to display object names
>>
>> Thoughts?
>
> I like the "optional no- accepted" markings, but I suspect there may
> be quite a lot of fallouts.

Some test expectations in t0040 and t1502 would have to be adjusted.

This reveals, by the way, that t1502 doesn't yet exercise the "!" flag
of "git rev-parse --parseopt" that turns on PARSE_OPT_NONEG.  I find
the "-h, --[no-]help" option strangely amusing..

--- >8 ----
Subject: [PATCH] parse-options: show negatability of options in short help

Add a "[no-]" prefix to options without the flag PARSE_OPT_NONEG to
document the fact that you can negate them.

This looks a bit strange for options that already start with "no-", e.g.
for the option --no-name of git show-branch:

    --[no-]no-name        suppress naming strings

You can actually use --no-no-name as an alias of --name, so the short
help is not wrong.  If we strip off any of the "no-"s, we lose either
the ability to see if the remaining one belongs to the documented
variant or to see if it can be negated.

Signed-off-by: René Scharfe <l.s.r@web.de>
---
 parse-options.c               | 10 ++++-
 t/t0040-parse-options.sh      | 44 ++++++++++---------
 t/t1502-rev-parse-parseopt.sh | 80 ++++++++++++++++++++---------------
 3 files changed, 77 insertions(+), 57 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index f8a155ee13..6323ca191d 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1136,8 +1136,14 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 		}
 		if (opts->long_name && opts->short_name)
 			pos += fprintf(outfile, ", ");
-		if (opts->long_name)
-			pos += fprintf(outfile, "--%s", opts->long_name);
+		if (opts->long_name) {
+			const char *long_name = opts->long_name;
+			if (opts->flags & PARSE_OPT_NONEG)
+				pos += fprintf(outfile, "--%s", long_name);
+			else
+				pos += fprintf(outfile, "--[no-]%s", long_name);
+		}
+
 		if (opts->type == OPTION_NUMBER)
 			pos += utf8_fprintf(outfile, _("-NUM"));

diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 7d7ecfd571..f39758d2ef 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -13,29 +13,32 @@ usage: test-tool parse-options <options>

     A helper function for the parse-options API.

-    --yes                 get a boolean
-    -D, --no-doubt        begins with 'no-'
+    --[no-]yes            get a boolean
+    -D, --[no-]no-doubt   begins with 'no-'
     -B, --no-fear         be brave
-    -b, --boolean         increment by one
-    -4, --or4             bitwise-or boolean with ...0100
-    --neg-or4             same as --no-or4
+    -b, --[no-]boolean    increment by one
+    -4, --[no-]or4        bitwise-or boolean with ...0100
+    --[no-]neg-or4        same as --no-or4

-    -i, --integer <n>     get a integer
+    -i, --[no-]integer <n>
+                          get a integer
     -j <n>                get a integer, too
     -m, --magnitude <n>   get a magnitude
-    --set23               set integer to 23
+    --[no-]set23          set integer to 23
     --mode1               set integer to 1 (cmdmode option)
     --mode2               set integer to 2 (cmdmode option)
-    -L, --length <str>    get length of <str>
-    -F, --file <file>     set file to <file>
+    -L, --[no-]length <str>
+                          get length of <str>
+    -F, --[no-]file <file>
+                          set file to <file>

 String options
-    -s, --string <string>
+    -s, --[no-]string <string>
                           get a string
-    --string2 <str>       get another string
-    --st <st>             get another string (pervert ordering)
+    --[no-]string2 <str>  get another string
+    --[no-]st <st>        get another string (pervert ordering)
     -o <str>              get another string
-    --list <str>          add str to list
+    --[no-]list <str>     add str to list

 Magic arguments
     -NUM                  set integer to NUM
@@ -44,16 +47,17 @@ Magic arguments
     --no-ambiguous        negative ambiguity

 Standard options
-    --abbrev[=<n>]        use <n> digits to display object names
-    -v, --verbose         be verbose
-    -n, --dry-run         dry run
-    -q, --quiet           be quiet
-    --expect <string>     expected output in the variable dump
+    --[no-]abbrev[=<n>]   use <n> digits to display object names
+    -v, --[no-]verbose    be verbose
+    -n, --[no-]dry-run    dry run
+    -q, --[no-]quiet      be quiet
+    --[no-]expect <string>
+                          expected output in the variable dump

 Alias
-    -A, --alias-source <string>
+    -A, --[no-]alias-source <string>
                           get a string
-    -Z, --alias-target <string>
+    -Z, --[no-]alias-target <string>
                           alias of --alias-source

 EOF
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index dd811b7fb4..0a67e2dd4f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -64,33 +64,38 @@ test_expect_success 'test --parseopt help output' '
 |
 |    some-command does foo and bar!
 |
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
+|    -h, --[no-]help       show the help
+|    --[no-]foo            some nifty option --foo
+|    --[no-]bar ...        some cool option --bar with an argument
+|    -b, --[no-]baz        a short and long option
 |
 |An option group Header
 |    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
+|    -d, --[no-]data[=...]
+|                          short and long option with an optional argument
 |
 |Argument hints
 |    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
+|    --[no-]bar2 <arg>     long option required argument
+|    -e, --[no-]fuz <with-space>
 |                          short and long option required argument
 |    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
+|    --[no-]long[=<data>]  long option optional argument
+|    -g, --[no-]fluf[=<path>]
+|                          short and long option optional argument
+|    --[no-]longest <very-long-argument-hint>
 |                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
+|    --[no-]pair <key=value>
+|                          with an equals sign in the hint
+|    --[no-]aswitch        help te=t contains? fl*g characters!`
+|    --[no-]bswitch <hint>
+|                          hint has trailing tab character
+|    --[no-]cswitch        switch has trailing tab character
+|    --[no-]short-hint <a>
+|                          with a one symbol hint
 |
 |Extras
-|    --extra1              line above used to cause a segfault but no longer does
+|    --[no-]extra1         line above used to cause a segfault but no longer does
 |
 |EOF
 END_EXPECT
@@ -131,7 +136,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |
 |    some-command does foo and bar!
 |
-|    --hidden1             A hidden switch
+|    --[no-]hidden1        A hidden switch
 |
 |EOF
 END_EXPECT
@@ -146,33 +151,38 @@ test_expect_success 'test --parseopt invalid switch help output' '
 |
 |    some-command does foo and bar!
 |
-|    -h, --help            show the help
-|    --foo                 some nifty option --foo
-|    --bar ...             some cool option --bar with an argument
-|    -b, --baz             a short and long option
+|    -h, --[no-]help       show the help
+|    --[no-]foo            some nifty option --foo
+|    --[no-]bar ...        some cool option --bar with an argument
+|    -b, --[no-]baz        a short and long option
 |
 |An option group Header
 |    -C[...]               option C with an optional argument
-|    -d, --data[=...]      short and long option with an optional argument
+|    -d, --[no-]data[=...]
+|                          short and long option with an optional argument
 |
 |Argument hints
 |    -B <arg>              short option required argument
-|    --bar2 <arg>          long option required argument
-|    -e, --fuz <with-space>
+|    --[no-]bar2 <arg>     long option required argument
+|    -e, --[no-]fuz <with-space>
 |                          short and long option required argument
 |    -s[<some>]            short option optional argument
-|    --long[=<data>]       long option optional argument
-|    -g, --fluf[=<path>]   short and long option optional argument
-|    --longest <very-long-argument-hint>
+|    --[no-]long[=<data>]  long option optional argument
+|    -g, --[no-]fluf[=<path>]
+|                          short and long option optional argument
+|    --[no-]longest <very-long-argument-hint>
 |                          a very long argument hint
-|    --pair <key=value>    with an equals sign in the hint
-|    --aswitch             help te=t contains? fl*g characters!`
-|    --bswitch <hint>      hint has trailing tab character
-|    --cswitch             switch has trailing tab character
-|    --short-hint <a>      with a one symbol hint
+|    --[no-]pair <key=value>
+|                          with an equals sign in the hint
+|    --[no-]aswitch        help te=t contains? fl*g characters!`
+|    --[no-]bswitch <hint>
+|                          hint has trailing tab character
+|    --[no-]cswitch        switch has trailing tab character
+|    --[no-]short-hint <a>
+|                          with a one symbol hint
 |
 |Extras
-|    --extra1              line above used to cause a segfault but no longer does
+|    --[no-]extra1         line above used to cause a segfault but no longer does
 |
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
@@ -297,7 +307,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|   or:     [--another-option]
 	|   or: cmd [--yet-another-option]
 	|
-	|    -h, --help            show the help
+	|    -h, --[no-]help       show the help
 	|
 	|EOF
 	END_EXPECT
@@ -334,7 +344,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|    line
 	|    blurb
 	|
-	|    -h, --help            show the help
+	|    -h, --[no-]help       show the help
 	|
 	|EOF
 	END_EXPECT
--
2.41.0