Am 18.07.23 um 18:37 schrieb Junio C Hamano:

René Scharfe [off-list ref] 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é