| Message ID | d392a005-4eba-7cc7-9554-cdb8dc53975e@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ls-tree: fix --no-full-name | expand |
René Scharfe <l.s.r@web.de> writes: > Since 61fdbcf98b (ls-tree: migrate to parse-options, 2009-11-13) git > ls-tree has accepted the option --no-full-name, but it does the same > as --full-name, contrary to convention. That's because it's defined > using OPT_SET_INT with a value of 0, where the negative variant sets > 0 as well. Ouch. Well spotted. > Turn --no-full-name into the opposite of --full-name by using OPT_BOOL > instead and storing the option's status directly in a variable named > "full_name" instead of in negated form in "chomp_prefix". Good solution, especially the flipping of the polarity of the variable is very sensible. I wonder if there are cases where it makes sense to allow the "--no-" variant to an option parsed with OPT_SET_INT() that sets '0' as the value? Some random findings while reading hits from "git grep OPT_SET_INT": * "git am --[no-]keep-cr" is implemented as a pair of explicit PARSE_OPT_NONEG entries in the option[] array, but wouldn't it be sufficient to have a single OPT_SET_INT("keep-cr")? * "git branch --list --no-all" is accepted, sets filter.kind to 0, and triggers "fatal: filter_refs: invalid type". Shouldn't we detect error much earlier? * "git bundle create --no-quiet" is accepted and sets the progress variable to 0, just like "--quiet" does, which is the same issue as the one fixed by your patch. * "git clone (--no-ipv4|--no-ipv6)" are accepted and uses TRANSPORT_FAMILY_ALL, presumably allowing both v4 and v6. Shouldn't we reject these? "fetch" and "push" share the same issue. * "git status --no-(short|long|porcelain)" are accepted and use STATUS_FORMAT_NONE, which probably is OK. * "git commit --[no-](short|long|porcelain)" are accepted and behave as "git status" without doing any "git commit" thing, which should be corrected, I think. * "git describe --no-exact-match" is the same as "--exact-match", which is the same issue as the one fixed by your patch. * "git remote add" has an OPT_SET_INT() entry whose short and long forms are (0, NULL). What is this supposed to do? Shouldn't parse-options.c:parse_options_check() notice it as an error? * "git reset --(soft|hard|mixed|merge|keep)" all take the negated form and they all become "--mixed". It may make sense to give all of them PARSE_OPT_NONEG. * "git show-branch --no-sparse" is the same as "--sparse", which is the same issue as the one fixed by your patch. * "git show-branch --no-topo-order" is the same as "--topo-order"; as it is the default, we probably should give PARSE_OPT_NONEG. * "git show-branch --no-date-order" is the same as "--topo-order", which does sort-of make sense. This (and the previous one) relies on REV_SORT_IN_GRAPH_ORDER enum being 0, which smells a bit brittle. * "git stash push --no-all" is the same as "--no-include-untracked", which smells iffy but probably is OK. Anyway, the patch looks good. Will queue. Thanks. > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > builtin/ls-tree.c | 7 +++---- > t/t3101-ls-tree-dirname.sh | 8 ++++++++ > 2 files changed, 11 insertions(+), 4 deletions(-) > > diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c > index 53073d64cb..f558db5f3b 100644 > --- a/builtin/ls-tree.c > +++ b/builtin/ls-tree.c > @@ -343,7 +343,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > struct object_id oid; > struct tree *tree; > int i, full_tree = 0; > - int chomp_prefix = prefix && *prefix; > + int full_name = !prefix || !*prefix; > read_tree_fn_t fn = NULL; > enum ls_tree_cmdmode cmdmode = MODE_DEFAULT; > int null_termination = 0; > @@ -365,8 +365,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > MODE_NAME_STATUS), > OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"), > MODE_OBJECT_ONLY), > - OPT_SET_INT(0, "full-name", &chomp_prefix, > - N_("use full path names"), 0), > + OPT_BOOL(0, "full-name", &full_name, N_("use full path names")), > OPT_BOOL(0, "full-tree", &full_tree, > N_("list entire tree; not just current directory " > "(implies --full-name)")), > @@ -387,7 +386,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) > > if (full_tree) > prefix = NULL; > - options.prefix = chomp_prefix ? prefix : NULL; > + options.prefix = full_name ? NULL : prefix; > > /* > * We wanted to detect conflicts between --name-only and > diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh > index 217006d1bf..5af2dac0e4 100755 > --- a/t/t3101-ls-tree-dirname.sh > +++ b/t/t3101-ls-tree-dirname.sh > @@ -154,6 +154,14 @@ EOF > test_output > ' > > +test_expect_success 'ls-tree --no-full-name' ' > + git -C path0 ls-tree --no-full-name $tree a >current && > + cat >expected <<-EOF && > + 040000 tree X a > + EOF > + test_output > +' > + > test_expect_success 'ls-tree --full-tree' ' > ( > cd path1/b/c && > -- > 2.41.0
Junio C Hamano <gitster@pobox.com> writes: > * "git commit --[no-](short|long|porcelain)" are accepted and > behave as "git status" without doing any "git commit" thing, > which should be corrected, I think. It turns out that this was very much deliberate that these options imply "--dry-run", implemented in 7c9f7038 (commit: support alternate status formats, 2009-09-05). So there is nothing to fix here.
Am 18.07.23 um 18:37 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > > I wonder if there are cases where it makes sense to allow the > "--no-" variant to an option parsed with OPT_SET_INT() that sets '0' > as the value? I doubt it. > Some random findings while reading hits from "git grep OPT_SET_INT": Woah, so many! > * "git branch --list --no-all" is accepted, sets filter.kind to 0, > and triggers "fatal: filter_refs: invalid type". Shouldn't we > detect error much earlier? Yes. And "git branch --no-copy" etc. are funny as well. > * "git bundle create --no-quiet" is accepted and sets the progress > variable to 0, just like "--quiet" does, which is the same issue > as the one fixed by your patch. The same in pack-objects. It's a bit trickier because of the presence of a third state (--quiet, --progress and --all-progress). The help text changes of 8b95521edb (bundle: turn on --all-progress-implied by default, 2023-03-04) state that only two states remain in git bundle (--quiet and --all-progress), but that's not fully true because the option --all-progress-implied is still wired up. "git bundle --no-all-progress-implied --progress" still gives git pack-objects a lone --progress. > * "git clone (--no-ipv4|--no-ipv6)" are accepted and uses > TRANSPORT_FAMILY_ALL, presumably allowing both v4 and v6. > Shouldn't we reject these? "fetch" and "push" share the same > issue. Either that, or we could turn them into OPT_BITs and let --no-ipv6 mean "give me anything but IPv6", which currently happens to be the same as --ipv4.. > * "git remote add" has an OPT_SET_INT() entry whose short and long > forms are (0, NULL). What is this supposed to do? Shouldn't > parse-options.c:parse_options_check() notice it as an error? It extends the help text of the previous option. Horrible. > * "git stash push --no-all" is the same as "--no-include-untracked", > which smells iffy but probably is OK. Hard to imagine a situation where a --no-all would be well-defined and intuitive. 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? René
Am 18.07.23 um 22:49 schrieb Junio C Hamano: > Junio C Hamano <gitster@pobox.com> writes: > >> * "git commit --[no-](short|long|porcelain)" are accepted and >> behave as "git status" without doing any "git commit" thing, >> which should be corrected, I think. > > It turns out that this was very much deliberate that these options > imply "--dry-run", implemented in 7c9f7038 (commit: support > alternate status formats, 2009-09-05). > > So there is nothing to fix here. The negated variants don't imply --dry-run and do actually commit something. Which kinda makes sense. René
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.
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
René Scharfe <l.s.r@web.de> writes: > 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); > + } This is a good starting point, but we should at least exempt OPT_BOOL from this exercise, I would think, because ... > A helper function for the parse-options API. > > - --yes get a boolean > + --[no-]yes get a boolean ... they are designed to be prefixed with an optional "no-". > - -D, --no-doubt begins with 'no-' > + -D, --[no-]no-doubt begins with 'no-' Hmph, I really really loved the neat trick to allow "no-doubt" option to be "positivised" by _dropping_ the leading "no-" at around 0f1930c5 (parse-options: allow positivation of options starting, with no-, 2012-02-25). > EOF Many of the above are amusing and served as good demonstration to show the blast radius, but it seems that most of them should be marked with PARSE_OPT_NONEG. > 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 Indeed it is amusing, but we probably should give PARSE_OPT_NONEG appropriately, instead of changing the expectations, for many of the changes we see here, I think.
Junio C Hamano <gitster@pobox.com> writes: > This is a good starting point, but we should at least exempt > OPT_BOOL from this exercise, I would think, because ... No, no no, that was nonsense. The literal "yes" fooled me. There is no need to special-case OPT_BOOL at all. Sorry for the noise.
Am 21.07.23 um 22:09 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> - -D, --no-doubt begins with 'no-' >> + -D, --[no-]no-doubt begins with 'no-' > > Hmph, I really really loved the neat trick to allow "no-doubt" > option to be "positivised" by _dropping_ the leading "no-" at around > 0f1930c5 (parse-options: allow positivation of options starting, > with no-, 2012-02-25). Yeah, if there is a better way to document A) that the "no-" is optional and B) whether it's present by default, I'm all ears. > Many of the above are amusing and served as good demonstration to > show the blast radius, but it seems that most of them should be > marked with PARSE_OPT_NONEG. Hard to say for me -- these are synthetic test cases and I lack context to make that decision. In t0040 (t/helper/test-parse-options.c rather) we do have a few PARSE_OPT_NONEG uses already. In t1502 we need to add some... > >> 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 > > Indeed it is amusing, but we probably should give PARSE_OPT_NONEG > appropriately, instead of changing the expectations, for many of the > changes we see here, I think. ... and --help is the one obvious choice for me, because --no-help is not supported, of course. But we can use some more dedicated tests of negation and double-negation. René
René Scharfe <l.s.r@web.de> writes: > Am 21.07.23 um 22:09 schrieb Junio C Hamano: >> René Scharfe <l.s.r@web.de> writes: >> >>> - -D, --no-doubt begins with 'no-' >>> + -D, --[no-]no-doubt begins with 'no-' >> >> Hmph, I really really loved the neat trick to allow "no-doubt" >> option to be "positivised" by _dropping_ the leading "no-" at around >> 0f1930c5 (parse-options: allow positivation of options starting, >> with no-, 2012-02-25). > > Yeah, if there is a better way to document A) that the "no-" is optional > and B) whether it's present by default, I'm all ears. Some options take "no-" prefix while some others do not, so indicating that "this can take negative forms" vs "this do not take negative forms" by "--[no-]xyzzy" and "--frotz" makes sense. Yikes. There are tons of options whose names begin with "no-" and marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not have the 'no-' part in [brackets] can drop 'no-' to make it positive" would not fly as a rule/convention. If we do not mind getting longer, we could say -D, --no-doubt, --doubt and explain in the description that --no-doubt is the same as -D and --doubt is the default. It is making the developers responsible for clarify, which is not very satisfying. We may not reject "--no-no-doubt" but with the positivization support, double negation is not something we'd encourage without feeling embarrassed. > Hard to say for me -- these are synthetic test cases and I lack context > to make that decision. In t0040 (t/helper/test-parse-options.c rather) > we do have a few PARSE_OPT_NONEG uses already. In t1502 we need to add > some... True. The test coverage will be hurt if we start futzing with OPT_NONEG bit "randomly". >>> 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 >> >> Indeed it is amusing, but we probably should give PARSE_OPT_NONEG >> appropriately, instead of changing the expectations, for many of the >> changes we see here, I think. > > ... and --help is the one obvious choice for me, because --no-help is > not supported, of course. But we can use some more dedicated tests of > negation and double-negation. Yeah.
Am 24.07.23 um 20:51 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Am 21.07.23 um 22:09 schrieb Junio C Hamano: >>> René Scharfe <l.s.r@web.de> writes: >>> >>>> - -D, --no-doubt begins with 'no-' >>>> + -D, --[no-]no-doubt begins with 'no-' >>> >>> Hmph, I really really loved the neat trick to allow "no-doubt" >>> option to be "positivised" by _dropping_ the leading "no-" at around >>> 0f1930c5 (parse-options: allow positivation of options starting, >>> with no-, 2012-02-25). >> >> Yeah, if there is a better way to document A) that the "no-" is optional >> and B) whether it's present by default, I'm all ears. > > Some options take "no-" prefix while some others do not, so > indicating that "this can take negative forms" vs "this do not take > negative forms" by "--[no-]xyzzy" and "--frotz" makes sense. > > Yikes. There are tons of options whose names begin with "no-" and > marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not > have the 'no-' part in [brackets] can drop 'no-' to make it > positive" would not fly as a rule/convention. > > If we do not mind getting longer, we could say > > -D, --no-doubt, --doubt > > and explain in the description that --no-doubt is the same as -D and > --doubt is the default. It is making the developers responsible for > clarify, which is not very satisfying. Adjusting all explanations manually seems quite tedious. > We may not reject "--no-no-doubt" but with the positivization > support, double negation is not something we'd encourage without > feeling embarrassed. Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested brackets, but it's more correct, because it documents all three accepted forms, including the no-less one. René
René Scharfe <l.s.r@web.de> writes: > Am 24.07.23 um 20:51 schrieb Junio C Hamano: >> René Scharfe <l.s.r@web.de> writes: >> >>> Am 21.07.23 um 22:09 schrieb Junio C Hamano: >>>> René Scharfe <l.s.r@web.de> writes: >>>> >>>>> - -D, --no-doubt begins with 'no-' >>>>> + -D, --[no-]no-doubt begins with 'no-' >>>> >>>> Hmph, I really really loved the neat trick to allow "no-doubt" >>>> option to be "positivised" by _dropping_ the leading "no-" at around >>>> 0f1930c5 (parse-options: allow positivation of options starting, >>>> with no-, 2012-02-25). >>> >>> Yeah, if there is a better way to document A) that the "no-" is optional >>> and B) whether it's present by default, I'm all ears. >> >> Some options take "no-" prefix while some others do not, so >> indicating that "this can take negative forms" vs "this do not take >> negative forms" by "--[no-]xyzzy" and "--frotz" makes sense. >> >> Yikes. There are tons of options whose names begin with "no-" and >> marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not >> have the 'no-' part in [brackets] can drop 'no-' to make it >> positive" would not fly as a rule/convention. >> >> If we do not mind getting longer, we could say >> >> -D, --no-doubt, --doubt >> >> and explain in the description that --no-doubt is the same as -D and >> --doubt is the default. It is making the developers responsible for >> clarify, which is not very satisfying. > > Adjusting all explanations manually seems quite tedious. > >> We may not reject "--no-no-doubt" but with the positivization >> support, double negation is not something we'd encourage without >> feeling embarrassed. > > Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested > brackets, but it's more correct, because it documents all three accepted > forms, including the no-less one. It may look a bit silly but looks very tempting. Also it is not much longer than "--[no-]no-doubt".
Am 24.07.23 um 22:50 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> Am 24.07.23 um 20:51 schrieb Junio C Hamano: >>> René Scharfe <l.s.r@web.de> writes: >>> >>>> Am 21.07.23 um 22:09 schrieb Junio C Hamano: >>>>> René Scharfe <l.s.r@web.de> writes: >>>>> >>>>>> - -D, --no-doubt begins with 'no-' >>>>>> + -D, --[no-]no-doubt begins with 'no-' >>>>> >>>>> Hmph, I really really loved the neat trick to allow "no-doubt" >>>>> option to be "positivised" by _dropping_ the leading "no-" at around >>>>> 0f1930c5 (parse-options: allow positivation of options starting, >>>>> with no-, 2012-02-25). >>>> >>>> Yeah, if there is a better way to document A) that the "no-" is optional >>>> and B) whether it's present by default, I'm all ears. >>> >>> Some options take "no-" prefix while some others do not, so >>> indicating that "this can take negative forms" vs "this do not take >>> negative forms" by "--[no-]xyzzy" and "--frotz" makes sense. >>> >>> Yikes. There are tons of options whose names begin with "no-" and >>> marked PARSE_OPT_NONEG, so "an option '--no-nitfol' that does not >>> have the 'no-' part in [brackets] can drop 'no-' to make it >>> positive" would not fly as a rule/convention. >>> >>> If we do not mind getting longer, we could say >>> >>> -D, --no-doubt, --doubt >>> >>> and explain in the description that --no-doubt is the same as -D and >>> --doubt is the default. It is making the developers responsible for >>> clarify, which is not very satisfying. >> >> Adjusting all explanations manually seems quite tedious. >> >>> We may not reject "--no-no-doubt" but with the positivization >>> support, double negation is not something we'd encourage without >>> feeling embarrassed. >> >> Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested >> brackets, but it's more correct, because it documents all three accepted >> forms, including the no-less one. > > It may look a bit silly but looks very tempting. Also it is not > much longer than "--[no-]no-doubt". Yes, it's quite compact. But is it they still legible? --no-index find in contents not managed by git --[no-]no-index find in contents not managed by git --[[no-]no-]index find in contents not managed by git --[no-[no-]]index find in contents not managed by git The last two document all three variants, but is it still obvious that the help text is supposed to be about the one with a single "no-"? That's something that has to be learned, I suspect. No good making the short help too cryptic. Hmm, how about: --no-index, --[no-[no-]]index find in contents not managed by git Somewhat redundant, but highlights the documented variant. René
On 28/07/2023 07:12, René Scharfe wrote: >>> Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested >>> brackets, but it's more correct, because it documents all three accepted >>> forms, including the no-less one. >> >> It may look a bit silly but looks very tempting. Also it is not >> much longer than "--[no-]no-doubt". > > Yes, it's quite compact. But is it they still legible? > > --no-index find in contents not managed by git > --[no-]no-index find in contents not managed by git > --[[no-]no-]index find in contents not managed by git > --[no-[no-]]index find in contents not managed by git > > The last two document all three variants, but is it still obvious that > the help text is supposed to be about the one with a single "no-"? > That's something that has to be learned, I suspect. No good making the > short help too cryptic. Hmm, how about: > > --no-index, --[no-[no-]]index > find in contents not managed by git I think spelling out the positive and negative options separately makes it much clearer, but in that case I think we'd be better just to show --no-index, --index adding "[no-[no]]" is just going to confuse users. If we had a convention of "[<short>, ]<positive long>; <negative long>" then I think it should be clear to users how to negate a given option -v, --invert-match; --no-invert-match show non-matching lines -I, --no-index; --index find in contents not managed by git Spelling out both versions is a bit verbose but I think it is worth it to avoid "--[no-]no-index" One other thought is to mark options that can be negated with a symbol like '*' and add a footnote saying those options can be negated. Best Wishes Phillip
Am 28.07.23 um 11:45 schrieb Phillip Wood: > On 28/07/2023 07:12, René Scharfe wrote: >>>> Right. Perhaps --[[no-]no-]doubt? Looks a bit silly with its nested >>>> brackets, but it's more correct, because it documents all three accepted >>>> forms, including the no-less one. >>> >>> It may look a bit silly but looks very tempting. Also it is not >>> much longer than "--[no-]no-doubt". >> >> Yes, it's quite compact. But is it they still legible? >> >> --no-index find in contents not managed by git >> --[no-]no-index find in contents not managed by git >> --[[no-]no-]index find in contents not managed by git >> --[no-[no-]]index find in contents not managed by git >> >> The last two document all three variants, but is it still obvious that >> the help text is supposed to be about the one with a single "no-"? >> That's something that has to be learned, I suspect. No good making the >> short help too cryptic. Hmm, how about: >> >> --no-index, --[no-[no-]]index >> find in contents not managed by git > > I think spelling out the positive and negative options separately > makes it much clearer, but in that case I think we'd be better just > to show > > --no-index, --index > > adding "[no-[no]]" is just going to confuse users. If we had a > convention of "[<short>, ]<positive long>; <negative long>" then I > think it should be clear to users how to negate a given option > > -v, --invert-match; --no-invert-match > show non-matching lines > -I, --no-index; --index find in contents not managed by git > > Spelling out both versions is a bit verbose but I think it is worth > it to avoid "--[no-]no-index" I kinda like it, even though it is quite verbose and adds a new syntax element. A bit more verbose still: Document the negative form on its own line with a generated description -- requires no new syntax: -v, --invert-match show non-matching lines --no-invert-match opposite of --invert-match, default -I, --no-index find in contents not managed by git --index opposite of --no-index, default > One other thought is to mark options that can be negated with a > symbol like '*' and add a footnote saying those options can be > negated. Sure, adding a layer of indirection would work. All these imperfect solutions make me wish we could get rid of the problem, e.g. by converting all "no-" options to their positive variants and mentioning that they are the default. Won't fly, though, because some of them have short forms, and we don't follow the convention of uppercase short options negating lowercase ones, so we have to document them anyway. René
René Scharfe <l.s.r@web.de> writes: > A bit more verbose still: Document the negative form on its own line > with a generated description -- requires no new syntax: > > -v, --invert-match show non-matching lines > --no-invert-match opposite of --invert-match, default > -I, --no-index find in contents not managed by git > --index opposite of --no-index, default I would expect _("opposite of %s, default") is acceptable by l10n folks, and assuming it is, the above would be a good approach. > All these imperfect solutions make me wish we could get rid of the > problem, e.g. by converting all "no-" options to their positive variants > and mentioning that they are the default. Won't fly, though, > because ... True. Thanks.
Junio C Hamano <gitster@pobox.com> writes: > René Scharfe <l.s.r@web.de> writes: > >> A bit more verbose still: Document the negative form on its own line >> with a generated description -- requires no new syntax: >> >> -v, --invert-match show non-matching lines >> --no-invert-match opposite of --invert-match, default >> -I, --no-index find in contents not managed by git >> --index opposite of --no-index, default > > I would expect _("opposite of %s, default") is acceptable by l10n > folks, and assuming it is, the above would be a good approach. I was seeing what is likely to be in the -rc1 that will happen in next week, and noticed that this discussion is left unconcluded. I am tempted to declare that the latest iteration that shows the negation of "--no-foo" as "--[no-]no-foo" is "good enough" for now, and leave the above improvement as one potential future direction. Objections? Otherwise the 5-patch series will be in 'next'. Thanks.
On 04/08/2023 17:40, Junio C Hamano wrote: > Junio C Hamano <gitster@pobox.com> writes: > >> René Scharfe <l.s.r@web.de> writes: >> >>> A bit more verbose still: Document the negative form on its own line >>> with a generated description -- requires no new syntax: >>> >>> -v, --invert-match show non-matching lines >>> --no-invert-match opposite of --invert-match, default >>> -I, --no-index find in contents not managed by git >>> --index opposite of --no-index, default >> >> I would expect _("opposite of %s, default") is acceptable by l10n >> folks, and assuming it is, the above would be a good approach. > > I was seeing what is likely to be in the -rc1 that will happen in > next week, and noticed that this discussion is left unconcluded. I > am tempted to declare that the latest iteration that shows the > negation of "--no-foo" as "--[no-]no-foo" is "good enough" for now, > and leave the above improvement as one potential future direction. While it could certainly be improved in the future I think "--[no-]no-foo" is probably good enough. I definitely prefer it over "--[[no-]no]-foo" Best Wishes Phillip > Objections? Otherwise the 5-patch series will be in 'next'. > > Thanks.
Am 04.08.23 um 21:48 schrieb Phillip Wood: > On 04/08/2023 17:40, Junio C Hamano wrote: >> Junio C Hamano <gitster@pobox.com> writes: >> >>> René Scharfe <l.s.r@web.de> writes: >>> >>>> A bit more verbose still: Document the negative form on its own line >>>> with a generated description -- requires no new syntax: >>>> >>>> -v, --invert-match show non-matching lines >>>> --no-invert-match opposite of --invert-match, default >>>> -I, --no-index find in contents not managed by git >>>> --index opposite of --no-index, default >>> >>> I would expect _("opposite of %s, default") is acceptable by l10n >>> folks, and assuming it is, the above would be a good approach. The ", default" part is not correct in all cases, though, in particular when the default depends on the output being a terminal, or arguably when it's dependent on a config setting. We better drop it from the generated message. >> I was seeing what is likely to be in the -rc1 that will happen in >> next week, and noticed that this discussion is left unconcluded. I >> am tempted to declare that the latest iteration that shows the >> negation of "--no-foo" as "--[no-]no-foo" is "good enough" for now, >> and leave the above improvement as one potential future direction. > > While it could certainly be improved in the future I think > "--[no-]no-foo" is probably good enough. I definitely prefer it over > "--[[no-]no]-foo" Generating help lines for the opposite variant of all negatable options feels quite spammy. It almost doubles the length of the short help because we have so many of them. Doing that only for --no-... options is much more compatc because most options are positive: -v, --[no-]invert-match show non-matching lines -I, --no-index find in contents not managed by git --index opposite of --no-index >> Objections? Otherwise the 5-patch series will be in 'next'. Patch 5 has a trivial merge conflict in t/t0040-parse-options.sh due to 448abbba63 (short help: allow multi-line opthelp, 2023-07-18), which is easily resolved by adding --longhelp, and a silent automatic mismerge of t/t1502/optionspec.help due to c512643e67 (short help: allow a gap smaller than USAGE_GAP, 2023-07-18), which requires removing three line newlines. How about dropping patch 5 for now? We don't need to rush this pretty intrusive change of output. And the first four patches are worthwhile in their own right, I think. René
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c index 53073d64cb..f558db5f3b 100644 --- a/builtin/ls-tree.c +++ b/builtin/ls-tree.c @@ -343,7 +343,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) struct object_id oid; struct tree *tree; int i, full_tree = 0; - int chomp_prefix = prefix && *prefix; + int full_name = !prefix || !*prefix; read_tree_fn_t fn = NULL; enum ls_tree_cmdmode cmdmode = MODE_DEFAULT; int null_termination = 0; @@ -365,8 +365,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) MODE_NAME_STATUS), OPT_CMDMODE(0, "object-only", &cmdmode, N_("list only objects"), MODE_OBJECT_ONLY), - OPT_SET_INT(0, "full-name", &chomp_prefix, - N_("use full path names"), 0), + OPT_BOOL(0, "full-name", &full_name, N_("use full path names")), OPT_BOOL(0, "full-tree", &full_tree, N_("list entire tree; not just current directory " "(implies --full-name)")), @@ -387,7 +386,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) if (full_tree) prefix = NULL; - options.prefix = chomp_prefix ? prefix : NULL; + options.prefix = full_name ? NULL : prefix; /* * We wanted to detect conflicts between --name-only and diff --git a/t/t3101-ls-tree-dirname.sh b/t/t3101-ls-tree-dirname.sh index 217006d1bf..5af2dac0e4 100755 --- a/t/t3101-ls-tree-dirname.sh +++ b/t/t3101-ls-tree-dirname.sh @@ -154,6 +154,14 @@ EOF test_output ' +test_expect_success 'ls-tree --no-full-name' ' + git -C path0 ls-tree --no-full-name $tree a >current && + cat >expected <<-EOF && + 040000 tree X a + EOF + test_output +' + test_expect_success 'ls-tree --full-tree' ' ( cd path1/b/c &&
Since 61fdbcf98b (ls-tree: migrate to parse-options, 2009-11-13) git ls-tree has accepted the option --no-full-name, but it does the same as --full-name, contrary to convention. That's because it's defined using OPT_SET_INT with a value of 0, where the negative variant sets 0 as well. Turn --no-full-name into the opposite of --full-name by using OPT_BOOL instead and storing the option's status directly in a variable named "full_name" instead of in negated form in "chomp_prefix". Signed-off-by: René Scharfe <l.s.r@web.de> --- builtin/ls-tree.c | 7 +++---- t/t3101-ls-tree-dirname.sh | 8 ++++++++ 2 files changed, 11 insertions(+), 4 deletions(-) -- 2.41.0