Am 18.07.23 um 18:37 schrieb Junio C Hamano:
> René Scharfe <l.s.r@xxxxxx> 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é
