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

From: René Scharfe <l.s.r@web.de>
Date: Jul 21, 2023, 12:41 UTC
Message-ID: <a6326aaa-4f05-4d00-8906-2f50ea8e1e7a@web.de>
In-Reply-To: <xmqqo7k9fa5x.fsf@gitster.g>

Am 18.07.23 um 18:37 schrieb Junio C Hamano:

Show 5 quoted lines

> 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é