Re: [PATCH] push: comment on a funny unbalanced option help

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Aug 1, 2018, 20:46 UTC
Message-ID: <87k1p9u860.fsf@evledraar.gmail.com>
In-Reply-To: <xmqqh8ke6i7w.fsf@gitster-ct.c.googlers.com>

On Wed, Aug 01 2018, Junio C Hamano wrote:

Show 20 quoted lines

> The option help text for the force-with-lease option to "git push"
> reads like this:
>
>     $ git push -h 2>&1 | grep -e force-with-lease
>        --force-with-lease[=<refname>:<expect>]
>
> which come from this
>
>  		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>
> in the source code, with an aparent lack of "<" and ">" at both
> ends.
>
> It turns out that parse-options machinery takes the whole string and
> encloses it inside a pair of "<>", expecting that it is a single
> placeholder.  The help string was written in a funnily unbalanced
> way knowing that the end result would balance out.
>
> Add a comment to save future readers from wasting time just like I
> did ;-)

There's something worth fixing here for sure...

> +		  /* N_() will get "<>" around, resulting in "<refname>:<expect>" */

...but this comment isn't accurate at all, N_() doesn't wrap the string with <>'s, as can be seen by applying this patch:

    -                 0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
    +                 0, CAS_OPT_NAME, &cas, "refname>:<expect",

Resulting in the same output:

    $ ./git --exec-path=$PWD push -h 2>&1 | grep -e force-with-lease
        --force-with-lease[=<refname>:<expect>]

Rather, it's the usage_argh() function in parse-options.c that's doing this. Looks like the logic was added in 29f25d493c ("parse-options: add PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).

Also because of this I looked at:

    $ git grep -P 'N_\("<'

Which shows e.g.:

    builtin/difftool.c:706:         OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
    builtin/difftool.c:714:         OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),

Producing this bug:

    $ git difftool -h 2>&1|grep '<<'
        -t, --tool <<tool>>   use the specified diff tool
        -x, --extcmd <<command>>

But these all do the right thing for some reason, just looked briefly and didn't see how they're different & manage to avoid this:

    builtin/read-tree.c:134:                { OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
    builtin/show-branch.c:673:              { OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("<n>[,<base>]"),
    builtin/update-index.c:969:                     N_("<mode>,<object>,<path>"),
    builtin/write-tree.c:27:                { OPTION_STRING, 0, "prefix", &prefix, N_("<prefix>/"),