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

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

[Junio C Hamano]

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 ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/push.c b/builtin/push.c
index 9cd8e8cd56..9608b0cc4f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -558,6 +558,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
 		{ OPTION_CALLBACK,
+		  /* N_() will get "<>" around, resulting in "<refname>:<expect>" */
 		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
 		  N_("require old value of ref to be at this value"),
 		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
-- 
2.18.0-321-gffc6fa0e39

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

[Ævar Arnfjörð Bjarmason]

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

> 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>/"),

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

>> +		  /* 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:

I know.  It is a short-hand for "what's inside N_() we see here".
Try to come up with an equivalent that fits on a 80-char line.

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

[Ævar Arnfjörð Bjarmason]

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

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> +		  /* 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:
>
> I know.  It is a short-hand for "what's inside N_() we see here".
> Try to come up with an equivalent that fits on a 80-char line.

I was going to say:

/* parse_options() will munge this to "<refname>:<expect>" */

or:

/* note: parse_options() will add surrounding <>'s for us */

or:

/* missing <>'s are not a bug, parse_options() adds them */

But looking at this again it looks like this whole thing should just be
replaced by:

    diff --git a/builtin/push.c b/builtin/push.c
    index 9cd8e8cd56..b8fa15c101 100644
    --- a/builtin/push.c
    +++ b/builtin/push.c
    @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
                    OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
                    OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
                    { OPTION_CALLBACK,
    -                 0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
    +                 0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
                      N_("require old value of ref to be at this value"),
    -                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
    +                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
    +                 parseopt_push_cas_option },
                    { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
                            N_("control recursive pushing of submodules"),
                            PARSE_OPT_OPTARG, option_parse_recurse_submodules },

I.e. the reason this is confusing is because the code originally added
in 28f5d17611 ("remote.c: add command line option parser for
"--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
which I also see is what read-tree etc. use already to not end up with
these double <>'s, see also 29f25d493c ("parse-options: add
PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).

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

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> Add a comment to save future readers from wasting time just like I
> did ;-)

Thanks.  I think we should go further, and start the comment with
TRANSLATORS so it shows up for the audience most affected by this as
well.  See the note on TRANSLATORS in po/README for details.

Sincerely,
Jonathan

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

[René Scharfe]

Am 02.08.2018 um 00:31 schrieb Ævar Arnfjörð Bjarmason:
> But looking at this again it looks like this whole thing should just be
> replaced by:
> 
>      diff --git a/builtin/push.c b/builtin/push.c
>      index 9cd8e8cd56..b8fa15c101 100644
>      --- a/builtin/push.c
>      +++ b/builtin/push.c
>      @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>                      OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>                      OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>                      { OPTION_CALLBACK,
>      -                 0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>      +                 0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
>                        N_("require old value of ref to be at this value"),
>      -                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
>      +                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>      +                 parseopt_push_cas_option },
>                      { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
>                              N_("control recursive pushing of submodules"),
>                              PARSE_OPT_OPTARG, option_parse_recurse_submodules },
> 
> I.e. the reason this is confusing is because the code originally added
> in 28f5d17611 ("remote.c: add command line option parser for
> "--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
> which I also see is what read-tree etc. use already to not end up with
> these double <>'s, see also 29f25d493c ("parse-options: add
> PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).

We could check if argh comes with its own angle brackets already and
not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
redundant in most cases, including the one above.  Any downsides?
Too magical?

-- >8 --
Subject: [PATCH] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

Avoid adding an extra pair of angular brackets if the argh string
already contains one.  Remove the flag PARSE_OPT_LITERAL_ARGHELP in the
cases where the new automatic handling suffices.  This shortens and
simplifies option definitions with special argument help strings a bit.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/read-tree.c    | 2 +-
 builtin/show-branch.c  | 2 +-
 builtin/update-index.c | 2 +-
 builtin/write-tree.c   | 5 ++---
 parse-options.c        | 3 ++-
 5 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index ebc43eb805..fbbc98e516 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -133,7 +133,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			 N_("same as -m, but discard unmerged entries")),
 		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
 		  N_("read the tree into the index under <subdirectory>/"),
-		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+		  PARSE_OPT_NONEG },
 		OPT_BOOL('u', NULL, &opts.update,
 			 N_("update working tree with merge result")),
 		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2e985c00a..9106da1985 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -673,7 +673,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		{ OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("<n>[,<base>]"),
 			    N_("show <n> most recent ref-log entries starting at "
 			       "base"),
-			    PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+			    PARSE_OPT_OPTARG,
 			    parse_reflog_param },
 		OPT_END()
 	};
diff --git a/builtin/update-index.c b/builtin/update-index.c
index a8709a26ec..22749fc2c7 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -969,7 +969,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			N_("<mode>,<object>,<path>"),
 			N_("add the specified entry to the index"),
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
-			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			PARSE_OPT_NONEG,
 			(parse_opt_cb *) cacheinfo_callback},
 		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
 			N_("override the executable bit of the listed files"),
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index c9d3c544e7..cdcbf8264e 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -24,9 +24,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	struct option write_tree_options[] = {
 		OPT_BIT(0, "missing-ok", &flags, N_("allow missing objects"),
 			WRITE_TREE_MISSING_OK),
-		{ OPTION_STRING, 0, "prefix", &prefix, N_("<prefix>/"),
-		  N_("write tree object for a subdirectory <prefix>") ,
-		  PARSE_OPT_LITERAL_ARGHELP },
+		OPT_STRING(0, "prefix", &prefix, N_("<prefix>/"),
+			   N_("write tree object for a subdirectory <prefix>")),
 		{ OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
 		  N_("only useful for debugging"),
 		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL,
diff --git a/parse-options.c b/parse-options.c
index 7db84227ab..fadfc6a833 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
-	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
+	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh ||
+		(opts->argh[0] == '<' && strchr(opts->argh, '>'));
 	if (opts->flags & PARSE_OPT_OPTARG)
 		if (opts->long_name)
 			s = literal ? "[=%s]" : "[=<%s>]";
-- 
2.18.0

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

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 02 2018, René Scharfe wrote:

> Am 02.08.2018 um 00:31 schrieb Ævar Arnfjörð Bjarmason:
>> But looking at this again it looks like this whole thing should just be
>> replaced by:
>>
>>      diff --git a/builtin/push.c b/builtin/push.c
>>      index 9cd8e8cd56..b8fa15c101 100644
>>      --- a/builtin/push.c
>>      +++ b/builtin/push.c
>>      @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>                      OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>>                      OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>>                      { OPTION_CALLBACK,
>>      -                 0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>>      +                 0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
>>                        N_("require old value of ref to be at this value"),
>>      -                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
>>      +                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>>      +                 parseopt_push_cas_option },
>>                      { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
>>                              N_("control recursive pushing of submodules"),
>>                              PARSE_OPT_OPTARG, option_parse_recurse_submodules },
>>
>> I.e. the reason this is confusing is because the code originally added
>> in 28f5d17611 ("remote.c: add command line option parser for
>> "--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
>> which I also see is what read-tree etc. use already to not end up with
>> these double <>'s, see also 29f25d493c ("parse-options: add
>> PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).
>
> We could check if argh comes with its own angle brackets already and
> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
> redundant in most cases, including the one above.  Any downsides?
> Too magical?

I'm more inclined to say that we should stop using
PARSE_OPT_LITERAL_ARGHELP in some of these cases, and change
"refname>:<expect" to "<refname>:<expect>" in push.c, so that the help
we emit is --force-with-lease[=<<refname>:<expect>>].

As noted in 29f25d493c this facility wasn't added with the intent
turning --refspec=<<refspec>> into --refspec=<refspec>, but to do stuff
like --option=<val1>[,<val2>] for options that take comma-delimited
options.

If we're magically removing <>'s we have no consistent convention to
tell apart --opt=<a|b|c> meaning "one of a, b or c", --refspec=<refspec>
meaning "the literal string 'refspec'" and --refspec=<<refspec>> meaning
add a <refspec> string, i.e. fill in your refspec here.

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

[René Scharfe]

Am 02.08.2018 um 16:21 schrieb Ævar Arnfjörð Bjarmason:
> 
> On Thu, Aug 02 2018, René Scharfe wrote:
> 
>> Am 02.08.2018 um 00:31 schrieb Ævar Arnfjörð Bjarmason:
>>> But looking at this again it looks like this whole thing should just be
>>> replaced by:
>>>
>>>       diff --git a/builtin/push.c b/builtin/push.c
>>>       index 9cd8e8cd56..b8fa15c101 100644
>>>       --- a/builtin/push.c
>>>       +++ b/builtin/push.c
>>>       @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>>                       OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>>>                       OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>>>                       { OPTION_CALLBACK,
>>>       -                 0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>>>       +                 0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
>>>                         N_("require old value of ref to be at this value"),
>>>       -                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
>>>       +                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>>>       +                 parseopt_push_cas_option },
>>>                       { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
>>>                               N_("control recursive pushing of submodules"),
>>>                               PARSE_OPT_OPTARG, option_parse_recurse_submodules },
>>>
>>> I.e. the reason this is confusing is because the code originally added
>>> in 28f5d17611 ("remote.c: add command line option parser for
>>> "--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
>>> which I also see is what read-tree etc. use already to not end up with
>>> these double <>'s, see also 29f25d493c ("parse-options: add
>>> PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).
>>
>> We could check if argh comes with its own angle brackets already and
>> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
>> redundant in most cases, including the one above.  Any downsides?
>> Too magical?
> 
> I'm more inclined to say that we should stop using
> PARSE_OPT_LITERAL_ARGHELP in some of these cases, and change
> "refname>:<expect" to "<refname>:<expect>" in push.c, so that the help
> we emit is --force-with-lease[=<<refname>:<expect>>].
> 
> As noted in 29f25d493c this facility wasn't added with the intent
> turning --refspec=<<refspec>> into --refspec=<refspec>, but to do stuff
> like --option=<val1>[,<val2>] for options that take comma-delimited
> options.
> 
> If we're magically removing <>'s we have no consistent convention to
> tell apart --opt=<a|b|c> meaning "one of a, b or c", --refspec=<refspec>
> meaning "the literal string 'refspec'" and --refspec=<<refspec>> meaning
> add a <refspec> string, i.e. fill in your refspec here.

The notation for requiring a literal string is to use no special markers:

	--option=literal_string

Alternatives can be grouped with parentheses:

	--option=(either|or)

In both cases you'd need PARSE_OPT_LITERAL_ARGHELP.

I haven't seen double angle brackets before in command line help strings.
The commit message of 29f25d493c doesn't mention them either.  A single
pair is used to indicate that users need to fill in a value of a certain
type:

	--refspec=<refspec>

Multi-part options aren't special in this syntax:

	--force-with-lease=<refname>:<expect>

NB: The "--refspec=" in the example before that is a literal string, so
this is also already a multi-part option if you will.

According to its manpage the option should rather be shown like this:

	--force-with-lease[=<refname>[:<expect>]]

... to indicate that all three forms are valid:

	--force-with-lease
	--force-with-lease=some_ref
	--force-with-lease=some_ref:but_not_this

The current code doesn't allow that to be expressed, while it's possible
with my patch.  And nothing is removed -- you can specify as many angle
brackets as you like, if that turns out to be useful; parseopt just won't
add any more on top automatically anymore if you do that.

Side note: The remaining user of PARSE_OPT_LITERAL_ARGHELP in
builtin/update-index.c uses a slash for alternatives; we should probably
use pipe instead:

	{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
		N_("override the executable bit of the listed files"),
		PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
		chmod_callback},

René

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

[René Scharfe]

Am 02.08.2018 um 17:06 schrieb René Scharfe:
> According to its manpage the option should rather be shown like this:
> 
> 	--force-with-lease[=<refname>[:<expect>]]
> 
> ... to indicate that all three forms are valid:
> 
> 	--force-with-lease
> 	--force-with-lease=some_ref
> 	--force-with-lease=some_ref:but_not_this
> 
> The current code doesn't allow that to be expressed, while it's possible
> with my patch.
Err, anything is possible with PARSE_OPT_LITERAL_ARGHELP, of course, but
with my patch that flag is automatically inferred if you use the argh
string "<refname>[:<expect>]".

René

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Aug 02 2018, René Scharfe wrote:
>
>> Am 02.08.2018 um 00:31 schrieb Ævar Arnfjörð Bjarmason:
>>> But looking at this again it looks like this whole thing should just be
>>> replaced by:
>>>
>>>      diff --git a/builtin/push.c b/builtin/push.c
>>>      index 9cd8e8cd56..b8fa15c101 100644
>>>      --- a/builtin/push.c
>>>      +++ b/builtin/push.c
>>>      @@ -558,9 +558,10 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>>                      OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>>>                      OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>>>                      { OPTION_CALLBACK,
>>>      -                 0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>>>      +                 0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
>>>                        N_("require old value of ref to be at this value"),
>>>      -                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
>>>      +                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>>>      +                 parseopt_push_cas_option },
>>>                      { OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
>>>                              N_("control recursive pushing of submodules"),
>>>                              PARSE_OPT_OPTARG, option_parse_recurse_submodules },
>>>
>>> I.e. the reason this is confusing is because the code originally added
>>> in 28f5d17611 ("remote.c: add command line option parser for
>>> "--force-with-lease"", 2013-07-08) didn't use PARSE_OPT_LITERAL_ARGHELP,
>>> which I also see is what read-tree etc. use already to not end up with
>>> these double <>'s, see also 29f25d493c ("parse-options: add
>>> PARSE_OPT_LITERAL_ARGHELP for complicated argh's", 2009-05-21).

Yup.  It shows that I did not know (or remember) about LIT-ARGH when
I wrote it (the line stayed in the same shape since its introduction
to the codebase), and I did not know (or remember) when I sent this
patch.  The above is the best solution to my puzzlement within the
framework of the current codebase.

>> We could check if argh comes with its own angle brackets already and
>> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
>> redundant in most cases, including the one above.  Any downsides?
>> Too magical?
>
> I'm more inclined to say that we should stop using
> PARSE_OPT_LITERAL_ARGHELP in some of these cases, and change
> "refname>:<expect" to "<refname>:<expect>" in push.c, so that the help
> we emit is --force-with-lease[=<<refname>:<expect>>].

I fail to see why the outermost <> pair could be a good idea.
Without them, i.e. in what the current output shows, I can see
<refname> and <expect> are something that I should supply real
values (i.e. placeholders) and I should have a colon (literal) in
between them.  It is an established convention that a token enclosed
in a <> pair is a placeholder.

But I am not sure what you mean by <<refname>:<expect>>.

> As noted in 29f25d493c this facility wasn't added with the intent
> turning --refspec=<<refspec>> into --refspec=<refspec>, but to do stuff
> like --option=<val1>[,<val2>] for options that take comma-delimited
> options.

There is no --refspec=<<refspec>> to begin with.

A single placeholder can be written in the source as "refspec" and
shown as "--refspec=<refspec>" because you get the surrounding <>
pair for free by default.  Nobody would want to write "<refspec>" in
the arg help, as most of the option arguments are a single value
placeholder.  But if you want "<val1>[,<val2>]" in the final output,
you do *not* want surrounding <> pair, so you use the option and
write everythnig manually in the source without magic.

Or are you saying that we should consistently write surrounding "<>"
to all placeholders, and stop special casing single-token ones?
IOW, get rid of literal-arghelp and instead make that the default?

> If we're magically removing <>'s we have no consistent convention to
> tell apart --opt=<a|b|c> meaning "one of a, b or c", --refspec=<refspec>
> meaning "the literal string 'refspec'" and --refspec=<<refspec>> meaning
> add a <refspec> string, i.e. fill in your refspec here.

Ah, this is where you are off-by-one.  "--refspec=<refspec>" means
"give your refspec as its value".

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> We could check if argh comes with its own angle brackets already and
> not add a second pair in that case, making PARSE_OPT_LITERAL_ARGHELP
> redundant in most cases, including the one above.  Any downsides?
> Too magical?

Hmph.

> -- >8 --
> Subject: [PATCH] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP
>
> Avoid adding an extra pair of angular brackets if the argh string
> already contains one.  Remove the flag PARSE_OPT_LITERAL_ARGHELP in the
> cases where the new automatic handling suffices.  This shortens and
> simplifies option definitions with special argument help strings a bit.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---

>  		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
>  		{ OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("<n>[,<base>]"),
>  			N_("<mode>,<object>,<path>"),
> +		OPT_STRING(0, "prefix", &prefix, N_("<prefix>/"),

Hmph.

> diff --git a/parse-options.c b/parse-options.c
> index 7db84227ab..fadfc6a833 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
>  static int usage_argh(const struct option *opts, FILE *outfile)
>  {
>  	const char *s;
> -	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> +	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh ||
> +		(opts->argh[0] == '<' && strchr(opts->argh, '>'));

You are greedier than what I thought ;-) I would have limited this
magic to the case where argh is surrounded by "<>", but you allow
one closing ">" anywhere, which allows us to grab more complex cases.

The lack of escape hatch to decline this auto-literal magic makes me
somewhat nervous, but I do not offhand think of a reason why I do
want an arg-help string that _begins_ with '<' to be enclosed by
another set of "<>", so perhaps this is a good kind of magic.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> +		  /* 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:
>
> I know.  It is a short-hand for "what's inside N_() we see here".
> Try to come up with an equivalent that fits on a 80-char line.

OK, let's scrap the comment patch but do this instead.

If we decide to take René's "auto-literal" change, the update to the
arg help string in this patch will become a necessary preparatory
step, even though "auto-literal" will make the addition of the
PARSE_OPT_LITERAL_ARGHELP bit redundant.

-- >8 --
Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced brackets
From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 02 Aug 2018 00:31:33 +0200

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 comes from having N_("refname>:<expect") as the argument help
text 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 "<>", to make it easier for majority
cases that uses a single token placeholder.  

The help string was written in a funnily unbalanced way knowing that
the end result would balance out, by somebody who forgot the
presence of PARSE_OPT_LITERAL_ARGHELP, which is the escape hatch
mechanism designed to help such a case.  We just should use the
official escape hatch instead.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/push.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 1c28427d82..ef4032a9ef 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
 		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
 		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
 		{ OPTION_CALLBACK,
-		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
+		  0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
 		  N_("require old value of ref to be at this value"),
-		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
+		  PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, parseopt_push_cas_option },
 		{ OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
 			N_("control recursive pushing of submodules"),
 			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
-- 
2.18.0-321-gffc6fa0e39

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

[René Scharfe]

Am 02.08.2018 um 17:44 schrieb Junio C Hamano:
> Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced brackets
> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> Date: Thu, 02 Aug 2018 00:31:33 +0200
> 
> 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 comes from having N_("refname>:<expect") as the argument help
> text 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 "<>", to make it easier for majority
> cases that uses a single token placeholder.
> 
> The help string was written in a funnily unbalanced way knowing that
> the end result would balance out, by somebody who forgot the
> presence of PARSE_OPT_LITERAL_ARGHELP, which is the escape hatch
> mechanism designed to help such a case.  We just should use the
> official escape hatch instead.
> 
> Helped-by: René Scharfe <l.s.r@web.de>

I didn't do anything for this particular patch so far?  But...

> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>   builtin/push.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/push.c b/builtin/push.c
> index 1c28427d82..ef4032a9ef 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>   		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>   		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>   		{ OPTION_CALLBACK,
> -		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
> +		  0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),

... shouldn't we use this opportunity to document that "expect" is
optional?

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

>   		  N_("require old value of ref to be at this value"),
> -		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
> +		  PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, parseopt_push_cas_option },
>   		{ OPTION_CALLBACK, 0, "recurse-submodules", &recurse_submodules, "check|on-demand|no",
>   			N_("control recursive pushing of submodules"),
>   			PARSE_OPT_OPTARG, option_parse_recurse_submodules },
> 

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 02.08.2018 um 17:44 schrieb Junio C Hamano:
>> Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced brackets
>> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> Date: Thu, 02 Aug 2018 00:31:33 +0200
>> ...
>> official escape hatch instead.
>> 
>> Helped-by: René Scharfe <l.s.r@web.de>
>
> I didn't do anything for this particular patch so far?  But...
>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Yeah, I realized it after I sent it out.  The line has been replaced
with a forged sign-off from Ævar.

>> ---
>>   builtin/push.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/builtin/push.c b/builtin/push.c
>> index 1c28427d82..ef4032a9ef 100644
>> --- a/builtin/push.c
>> +++ b/builtin/push.c
>> @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>   		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>>   		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>>   		{ OPTION_CALLBACK,
>> -		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>> +		  0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
>
> ... shouldn't we use this opportunity to document that "expect" is
> optional?

I consider that it is a separate topic.

I thought that we achieved a consensus that making the code guess
missing ":<expect>" is a misfeature that should be deprecated (in
which case we would not want to "s|:<expect>|[&]|"), but I may be
misremembering it.

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

[René Scharfe]

Am 02.08.2018 um 17:31 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
>> diff --git a/parse-options.c b/parse-options.c
>> index 7db84227ab..fadfc6a833 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
>>   static int usage_argh(const struct option *opts, FILE *outfile)
>>   {
>>   	const char *s;
>> -	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
>> +	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh ||
>> +		(opts->argh[0] == '<' && strchr(opts->argh, '>'));
> 
> You are greedier than what I thought ;-) I would have limited this
> magic to the case where argh is surrounded by "<>", but you allow
> one closing ">" anywhere, which allows us to grab more complex cases.

That's what I had initially, but only one of the current cases would
have benefited from that strict behavior, so it's not useful enough.

> The lack of escape hatch to decline this auto-literal magic makes me
> somewhat nervous, but I do not offhand think of a reason why I do
> want an arg-help string that _begins_ with '<' to be enclosed by
> another set of "<>", so perhaps this is a good kind of magic.

The escape hatch is to add the extra pair manually to the argh string.

René

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

[Jeff King]

On Thu, Aug 02, 2018 at 08:31:57AM -0700, Junio C Hamano wrote:

> > diff --git a/parse-options.c b/parse-options.c
> > index 7db84227ab..fadfc6a833 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
> >  static int usage_argh(const struct option *opts, FILE *outfile)
> >  {
> >  	const char *s;
> > -	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> > +	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh ||
> > +		(opts->argh[0] == '<' && strchr(opts->argh, '>'));
> 
> You are greedier than what I thought ;-) I would have limited this
> magic to the case where argh is surrounded by "<>", but you allow
> one closing ">" anywhere, which allows us to grab more complex cases.
> 
> The lack of escape hatch to decline this auto-literal magic makes me
> somewhat nervous, but I do not offhand think of a reason why I do
> want an arg-help string that _begins_ with '<' to be enclosed by
> another set of "<>", so perhaps this is a good kind of magic.

I think that case is actually easy; once the caller provides a "<>",
they are in "literal" mode, so they are free to add extra brackets if
you want. I.e., any "<foo>bar" that you do want enclosed could be
spelled "<<foo>bar>". It's the opposite that becomes impossible: you do
not have an opening bracket and nor do you want one.  But as long as we
retain LITERAL_ARGHELP for that case, we have an escape hatch.

I was going to suggest that this conversion has the downside of being
somewhat one-way. That is, it is easy for us to find affected sites now
since they contain the string LITERAL_ARGHELP. Whereas if we wanted to
back it out in the future, it is hard to know which sites are depending
on this new behavior.

But I am having trouble thinking of a reason we would want to back it
out. This makes most cases easier, and has a good escape hatch.

-Peff

PS I actually would have made the rule simply "does it begin with a
   '<'", which seems simpler still. If people accidentally write "<foo",
   forgetting to close their brackets, that is a bug under both the
   old and new behavior (just with slightly different outcomes).

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

[René Scharfe]

Am 02.08.2018 um 18:54 schrieb Jeff King:
> PS I actually would have made the rule simply "does it begin with a
>     '<'", which seems simpler still. If people accidentally write "<foo",
>     forgetting to close their brackets, that is a bug under both the
>     old and new behavior (just with slightly different outcomes).

Good point.  We could also extend it further and check if it contains
any special character, which would allow us to convert the remaining
user of the flag as well:

	{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
		N_("override the executable bit of the listed files"),
		PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
		chmod_callback},

Special characters are (, ), <, >, [, ], and |.

The idea is that we shouldn't automatically treat a string as a
simple replacement specifier if it looks like it has some structure
to it.

Side note: "(+/-)x" is marked for translation above.  Any translation
that is not identical would be wrong, though, because the command only
accepts a literal "+x" or "-x" in any locale.  So the N_ wrapper is
bogus, right?

Checked the output with that extended check by generating all help
pages with:

	for cmd in $(git --list-cmds=parseopt)
	do git-$cmd -h
	done

... and found a few differences:

git add:
-    --chmod <(+/-)x>      override the executable bit of the listed files
+    --chmod (+/-)x        override the executable bit of the listed files

Good change.  We also should change the slash to a pipe.

git checkout-index:
-    --stage <1-3|all>     copy out the files from named stage
+    --stage 1-3|all       copy out the files from named stage

Good change, but perhaps mention number two explicitly?

git difftool:
-    -t, --tool <<tool>>   use the specified diff tool
+    -t, --tool <tool>     use the specified diff tool
-    -x, --extcmd <<command>>
+    -x, --extcmd <command>

Aha, double angle brackets in the wild!  Good change.  We could
also remove the explicit pairs from the option definitions.

git pack-objects:
-    --index-version <version[,offset]>
+    --index-version version[,offset]

Not good before, worse after. Should be to "<version>[,<offset>]".

git pull:
-    -r, --rebase[=<false|true|merges|preserve|interactive>]
+    -r, --rebase[=false|true|merges|preserve|interactive]

Good change, but wouldn't we want to add a pair of parentheses around
the list of alternatives?

git push:
-    --force-with-lease[=<refname>:<expect>]
+    --force-with-lease[=refname>:<expect]

Bad change, needs explicit angular brackets (Junio's patch).

-    --recurse-submodules[=<check|on-demand|no>]
+    --recurse-submodules[=check|on-demand|no]
-    --signed[=<yes|no|if-asked>]
+    --signed[=yes|no|if-asked]

git send-pack:
-    --signed[=<yes|no|if-asked>]
+    --signed[=yes|no|if-asked]

Good changes all three, but need parentheses..

-    --force-with-lease[=<refname>:<expect>]
+    --force-with-lease[=refname>:<expect]

Bad change, needs explicit angular brackets (same as in Junio's patch).

git shortlog:
-    -w[<w[,i1[,i2]]>]     Linewrap output
+    -w[w[,i1[,i2]]]       Linewrap output

Not good before, worse after.  Should be "[<w>[,<i1>[,<i2>]]]".

git update-index:
-    --cacheinfo <mode>,<object>,<path>
-                          add the specified entry to the index
+    --cacheinfo           add the specified entry to the index

Eh, what?  Ah, that option is defined with PARSE_OPT_NOARG, and we only
show argument help because PARSE_OPT_LITERAL_ARGHELP is also given, so
we need to keep that flag for this special option.

René

[PATCH 1/6] add, update-index: fix --chmod argument help

[René Scharfe]

Don't translate the argument specification for --chmod; "+x" and "-x"
are the literal strings that the commands accept.

Separate alternatives using a pipe character instead of a slash, for
consistency.

Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
pair of angular brackets around the argument help string, as that would
wrongly indicate that users need to replace the literal strings with
some kind of value.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/add.c          | 4 +++-
 builtin/update-index.c | 2 +-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 8a155dd41e..84bfec9b73 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
 	OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
 	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
 	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
-	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
+	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
+	  N_("override the executable bit of the listed files"),
+	  PARSE_OPT_LITERAL_ARGHELP },
 	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
 			N_("warn when adding an embedded repository")),
 	OPT_END(),
diff --git a/builtin/update-index.c b/builtin/update-index.c
index a8709a26ec..7feda6e271 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			(parse_opt_cb *) cacheinfo_callback},
-		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
+		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
 			N_("override the executable bit of the listed files"),
 			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
 			chmod_callback},
-- 
2.18.0

[PATCH 2/6] difftool: remove angular brackets from argument help

[René Scharfe]

Parseopt wraps arguments in a pair of angular brackets by default,
signifying that the user needs to replace it with a value of the
documented type.  Remove the pairs from the option definitions to
duplication and confusion.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/difftool.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/difftool.c b/builtin/difftool.c
index 51f6c9cdb4..172af0448b 100644
--- a/builtin/difftool.c
+++ b/builtin/difftool.c
@@ -703,7 +703,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 			1, PARSE_OPT_NONEG | PARSE_OPT_HIDDEN),
 		OPT_BOOL(0, "symlinks", &symlinks,
 			 N_("use symlinks in dir-diff mode")),
-		OPT_STRING('t', "tool", &difftool_cmd, N_("<tool>"),
+		OPT_STRING('t', "tool", &difftool_cmd, N_("tool"),
 			   N_("use the specified diff tool")),
 		OPT_BOOL(0, "tool-help", &tool_help,
 			 N_("print a list of diff tools that may be used with "
@@ -711,7 +711,7 @@ int cmd_difftool(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "trust-exit-code", &trust_exit_code,
 			 N_("make 'git-difftool' exit when an invoked diff "
 			    "tool returns a non - zero exit code")),
-		OPT_STRING('x', "extcmd", &extcmd, N_("<command>"),
+		OPT_STRING('x', "extcmd", &extcmd, N_("command"),
 			   N_("specify a custom command for viewing diffs")),
 		OPT_END()
 	};
-- 
2.18.0

[PATCH 3/6] pack-objects: specify --index-version argument help explicitly

[René Scharfe]

Wrap both placeholders in the argument help string in angular brackets
to signal that users needs replace them with some actual value.  Use the
flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding another
pair.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/pack-objects.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ebc8cefb53..3a5d1fa317 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3110,9 +3110,9 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "all-progress-implied",
 			 &all_progress_implied,
 			 N_("similar to --all-progress when progress meter is shown")),
-		{ OPTION_CALLBACK, 0, "index-version", NULL, N_("version[,offset]"),
+		{ OPTION_CALLBACK, 0, "index-version", NULL, N_("<version>[,<offset>]"),
 		  N_("write the pack index file in the specified idx format version"),
-		  0, option_parse_index_version },
+		  PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version },
 		OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
 			      N_("maximum size of each output pack file")),
 		OPT_BOOL(0, "local", &local,
-- 
2.18.0

[PATCH 4/6] send-pack: specify --force-with-lease argument help explicitly

[René Scharfe]

Wrap each part of the argument help string in angular brackets to show
that users need to replace them with actual values.  Do that explicitly
to balance the pairs nicely in the code and avoid confusing casual
readers.  Add the flag PARSE_OPT_LITERAL_ARGHELP to keep parseopt from
adding another pair.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/send-pack.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 42fd8d1a35..0b517c9368 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -178,9 +178,10 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "stdin", &from_stdin, N_("read refs from stdin")),
 		OPT_BOOL(0, "helper-status", &helper_status, N_("print status from remote helper")),
 		{ OPTION_CALLBACK,
-		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
+		  0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
 		  N_("require old value of ref to be at this value"),
-		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
+		  PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+		  parseopt_push_cas_option },
 		OPT_END()
 	};
 
-- 
2.18.0

[PATCH 5/6] shortlog: correct option help for -w

[René Scharfe]

Wrap the placeholders in the option help string for -w in pairs of
angular brackets to document that users need to replace them with actual
numbers.  Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt
from adding another pair.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/shortlog.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 608d6ba77b..f555cf9e4f 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -268,8 +268,10 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 			 N_("Suppress commit descriptions, only provides commit count")),
 		OPT_BOOL('e', "email", &log.email,
 			 N_("Show the email address of each author")),
-		{ OPTION_CALLBACK, 'w', NULL, &log, N_("w[,i1[,i2]]"),
-			N_("Linewrap output"), PARSE_OPT_OPTARG, &parse_wrap_args },
+		{ OPTION_CALLBACK, 'w', NULL, &log, N_("<w>[,<i1>[,<i2>]]"),
+			N_("Linewrap output"),
+			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+			&parse_wrap_args },
 		OPT_END(),
 	};
 
-- 
2.18.0

[PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

[René Scharfe]

Parseopt wraps argument help strings in a pair of angular brackets by
default, to tell users that they need to replace it with an actual
value.  This is useful in most cases, because most option arguments
are indeed single values of a certain type.  The option
PARSE_OPT_LITERAL_ARGHELP needs to be used in option definitions with
arguments that have multiple parts or are literal strings.

Stop adding these angular brackets if special characters are present,
as they indicate that we don't deal with a simple placeholder.  This
simplifies the code a bit and makes defining special options slightly
easier.

Remove the flag PARSE_OPT_LITERAL_ARGHELP in the cases where the new
and more cautious handling suffices.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
The patch to add PARSE_OPT_LITERAL_ARGHELP for push --force-with-lease
should be applied before this one.

 builtin/add.c          | 5 ++---
 builtin/pack-objects.c | 2 +-
 builtin/read-tree.c    | 2 +-
 builtin/send-pack.c    | 3 +--
 builtin/shortlog.c     | 3 +--
 builtin/show-branch.c  | 2 +-
 builtin/update-index.c | 2 +-
 builtin/write-tree.c   | 5 ++---
 parse-options.c        | 3 ++-
 9 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 84bfec9b73..ba1ff5689d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,9 +304,8 @@ static struct option builtin_add_options[] = {
 	OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
 	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
 	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
-	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
-	  N_("override the executable bit of the listed files"),
-	  PARSE_OPT_LITERAL_ARGHELP },
+	OPT_STRING(0, "chmod", &chmod_arg, "(+|-)x",
+		   N_("override the executable bit of the listed files")),
 	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
 			N_("warn when adding an embedded repository")),
 	OPT_END(),
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3a5d1fa317..b2323613bc 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3112,7 +3112,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
 			 N_("similar to --all-progress when progress meter is shown")),
 		{ OPTION_CALLBACK, 0, "index-version", NULL, N_("<version>[,<offset>]"),
 		  N_("write the pack index file in the specified idx format version"),
-		  PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version },
+		  0, option_parse_index_version },
 		OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
 			      N_("maximum size of each output pack file")),
 		OPT_BOOL(0, "local", &local,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index ebc43eb805..fbbc98e516 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -133,7 +133,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
 			 N_("same as -m, but discard unmerged entries")),
 		{ OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
 		  N_("read the tree into the index under <subdirectory>/"),
-		  PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+		  PARSE_OPT_NONEG },
 		OPT_BOOL('u', NULL, &opts.update,
 			 N_("update working tree with merge result")),
 		{ OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 0b517c9368..724b484850 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -180,8 +180,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 		{ OPTION_CALLBACK,
 		  0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
 		  N_("require old value of ref to be at this value"),
-		  PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
-		  parseopt_push_cas_option },
+		  PARSE_OPT_OPTARG, parseopt_push_cas_option },
 		OPT_END()
 	};
 
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index f555cf9e4f..3898a2c9c4 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -269,8 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 		OPT_BOOL('e', "email", &log.email,
 			 N_("Show the email address of each author")),
 		{ OPTION_CALLBACK, 'w', NULL, &log, N_("<w>[,<i1>[,<i2>]]"),
-			N_("Linewrap output"),
-			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+			N_("Linewrap output"), PARSE_OPT_OPTARG,
 			&parse_wrap_args },
 		OPT_END(),
 	};
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2e985c00a..9106da1985 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -673,7 +673,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		{ OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("<n>[,<base>]"),
 			    N_("show <n> most recent ref-log entries starting at "
 			       "base"),
-			    PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+			    PARSE_OPT_OPTARG,
 			    parse_reflog_param },
 		OPT_END()
 	};
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7feda6e271..293f59247b 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -973,7 +973,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
 			(parse_opt_cb *) cacheinfo_callback},
 		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
 			N_("override the executable bit of the listed files"),
-			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+			PARSE_OPT_NONEG,
 			chmod_callback},
 		{OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
 			N_("mark files as \"not changing\""),
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index c9d3c544e7..cdcbf8264e 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -24,9 +24,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
 	struct option write_tree_options[] = {
 		OPT_BIT(0, "missing-ok", &flags, N_("allow missing objects"),
 			WRITE_TREE_MISSING_OK),
-		{ OPTION_STRING, 0, "prefix", &prefix, N_("<prefix>/"),
-		  N_("write tree object for a subdirectory <prefix>") ,
-		  PARSE_OPT_LITERAL_ARGHELP },
+		OPT_STRING(0, "prefix", &prefix, N_("<prefix>/"),
+			   N_("write tree object for a subdirectory <prefix>")),
 		{ OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
 		  N_("only useful for debugging"),
 		  PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL,
diff --git a/parse-options.c b/parse-options.c
index 7db84227ab..3b874a83a0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
 	const char *s;
-	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
+	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
+		!opts->argh || !!strpbrk(opts->argh, "()<>[]|");
 	if (opts->flags & PARSE_OPT_OPTARG)
 		if (opts->long_name)
 			s = literal ? "[=%s]" : "[=<%s>]";
-- 
2.18.0

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 02.08.2018 um 18:54 schrieb Jeff King:
>> PS I actually would have made the rule simply "does it begin with a
>>     '<'", which seems simpler still. If people accidentally write "<foo",
>>     forgetting to close their brackets, that is a bug under both the
>>     old and new behavior (just with slightly different outcomes).
>
> Good point.

Straying sideways into a tangent, but do we know if any locale wants
to use something other than "<>" as an enclosing braket around a
placeholder?  This arg-help text, for example,

	N_("refspec")			without LIT-ARG-HELP

would be irritating for such a locale's translator, who cannot
defeat the "<>" that is hardcoded and not inside _()

	s = literal ? "%s" : "<%s>";
                        
that appear in parse-options.c::usage_argh().

Perhaps we should do _("<%s>") here?  That way, the result would
hopefully be made consistent with

	N_("<refspec>[:<expect>]")	with LIT-ARG-HELP

which allows translator to use the bracket of the locale's choice.

Re: [PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> diff --git a/parse-options.c b/parse-options.c
> index 7db84227ab..3b874a83a0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
>  static int usage_argh(const struct option *opts, FILE *outfile)
>  {
>  	const char *s;
> -	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
> +	int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
> +		!opts->argh || !!strpbrk(opts->argh, "()<>[]|");

Good that you did not include '-' in there, as that would have
broken a multi-word-placeholder.

All other changes in this patch looked sensible, too.

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

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 02 2018, Junio C Hamano wrote:

> René Scharfe <l.s.r@web.de> writes:
>
>> Am 02.08.2018 um 17:44 schrieb Junio C Hamano:
>>> Subject: [PATCH] push: use PARSE_OPT_LITERAL_ARGHELP instead of unbalanced brackets
>>> From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>>> Date: Thu, 02 Aug 2018 00:31:33 +0200
>>> ...
>>> official escape hatch instead.
>>>
>>> Helped-by: René Scharfe <l.s.r@web.de>
>>
>> I didn't do anything for this particular patch so far?  But...
>>
>>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> Yeah, I realized it after I sent it out.  The line has been replaced
> with a forged sign-off from Ævar.

Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
patch of mine and replace it with something René came up with (I have
not yet read his 1-6 patches submitted on this topic, so maybe they're
not mutually exclusive).

>>> ---
>>>   builtin/push.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/builtin/push.c b/builtin/push.c
>>> index 1c28427d82..ef4032a9ef 100644
>>> --- a/builtin/push.c
>>> +++ b/builtin/push.c
>>> @@ -542,9 +542,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
>>>   		OPT_BIT( 0,  "porcelain", &flags, N_("machine-readable output"), TRANSPORT_PUSH_PORCELAIN),
>>>   		OPT_BIT('f', "force", &flags, N_("force updates"), TRANSPORT_PUSH_FORCE),
>>>   		{ OPTION_CALLBACK,
>>> -		  0, CAS_OPT_NAME, &cas, N_("refname>:<expect"),
>>> +		  0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
>>
>> ... shouldn't we use this opportunity to document that "expect" is
>> optional?
>
> I consider that it is a separate topic.
>
> I thought that we achieved a consensus that making the code guess
> missing ":<expect>" is a misfeature that should be deprecated (in
> which case we would not want to "s|:<expect>|[&]|"), but I may be
> misremembering it.

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

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
> patch of mine and replace it with something René came up with (I have
> not yet read his 1-6 patches submitted on this topic, so maybe they're
> not mutually exclusive).

I think the 6-patch series supersedes this one.  Thanks anyway.

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Ævar Arnfjörð Bjarmason]

On Thu, Aug 02 2018, René Scharfe wrote:

> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
>
> Separate alternatives using a pipe character instead of a slash, for
> consistency.
>
> Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
> pair of angular brackets around the argument help string, as that would
> wrongly indicate that users need to replace the literal strings with
> some kind of value.

Good change.

Let's mention in the commit message thath we ended up with this because
of 4a4838b46a ("i18n: update-index: mark parseopt strings for
translation", 2012-08-20) and 4e55ed32db ("add: add --chmod=+x /
--chmod=-x options", 2016-05-31) , both of which obviously didn't intend
for this to happen, but were either mass-replacing "..." with N_("..."),
or blindly copy/pasting an existing option whose argument should have
been translated.

> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/add.c          | 4 +++-
>  builtin/update-index.c | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/add.c b/builtin/add.c
> index 8a155dd41e..84bfec9b73 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
>  	OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
>  	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
>  	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
> -	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
> +	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
> +	  N_("override the executable bit of the listed files"),
> +	  PARSE_OPT_LITERAL_ARGHELP },
>  	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
>  			N_("warn when adding an embedded repository")),
>  	OPT_END(),
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a8709a26ec..7feda6e271 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
>  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>  			(parse_opt_cb *) cacheinfo_callback},
> -		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> +		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
>  			N_("override the executable bit of the listed files"),
>  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>  			chmod_callback},

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Ramsay Jones]

On 02/08/18 20:17, René Scharfe wrote:
> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
> 
> Separate alternatives using a pipe character instead of a slash, for
> consistency.
> 
> Use the flag PARSE_OPT_LITERAL_ARGHELP to prevent parseopt from adding a
> pair of angular brackets around the argument help string, as that would
> wrongly indicate that users need to replace the literal strings with
> some kind of value.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  builtin/add.c          | 4 +++-
>  builtin/update-index.c | 2 +-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/builtin/add.c b/builtin/add.c
> index 8a155dd41e..84bfec9b73 100644
> --- a/builtin/add.c
> +++ b/builtin/add.c
> @@ -304,7 +304,9 @@ static struct option builtin_add_options[] = {
>  	OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
>  	OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
>  	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
> -	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
> +	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",

Am I alone in thinking that "(+x|-x)" is more readable?

ATB,
Ramsay Jones

> +	  N_("override the executable bit of the listed files"),
> +	  PARSE_OPT_LITERAL_ARGHELP },
>  	OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
>  			N_("warn when adding an embedded repository")),
>  	OPT_END(),
> diff --git a/builtin/update-index.c b/builtin/update-index.c
> index a8709a26ec..7feda6e271 100644
> --- a/builtin/update-index.c
> +++ b/builtin/update-index.c
> @@ -971,7 +971,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
>  			PARSE_OPT_NOARG | /* disallow --cacheinfo=<mode> form */
>  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>  			(parse_opt_cb *) cacheinfo_callback},
> -		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, N_("(+/-)x"),
> +		{OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
>  			N_("override the executable bit of the listed files"),
>  			PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
>  			chmod_callback},
> 

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Andrei Rybak]

On 2018-08-02 21:17, René Scharfe wrote:
> Don't translate the argument specification for --chmod; "+x" and "-x"
> are the literal strings that the commands accept.
> > [...]
> 
> -	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
> +	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
> +	  N_("override the executable bit of the listed files"),
> +	  PARSE_OPT_LITERAL_ARGHELP },

Would it make sense to drop the localizations in po/* as well?
Or should such things be handled with l10n rounds?

Can be found using:

    grep '(+/-)x' po/*

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Junio C Hamano]

Andrei Rybak <rybak.a.v@gmail.com> writes:

> On 2018-08-02 21:17, René Scharfe wrote:
>> Don't translate the argument specification for --chmod; "+x" and "-x"
>> are the literal strings that the commands accept.
>> > [...]
>> 
>> -	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
>> +	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
>> +	  N_("override the executable bit of the listed files"),
>> +	  PARSE_OPT_LITERAL_ARGHELP },
>
> Would it make sense to drop the localizations in po/* as well?
> Or should such things be handled with l10n rounds?

It should happen "automatically" (eh, rather, thanks to hard work by
Jiang); for the rest of us, when the l10n coordinator updates the
master .*pot file next time.

Re: [PATCH 1/6] add, update-index: fix --chmod argument help

[Junio C Hamano]

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:

>>  	OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
>> -	OPT_STRING( 0 , "chmod", &chmod_arg, N_("(+/-)x"), N_("override the executable bit of the listed files")),
>> +	{ OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
>
> Am I alone in thinking that "(+x|-x)" is more readable?

I think I am guilty of that, and I agree yours is much easier to
read.

It can of course come on top of the series.

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

[René Scharfe]

Am 02.08.2018 um 22:01 schrieb Junio C Hamano:
> René Scharfe <l.s.r@web.de> writes:
> 
>> Am 02.08.2018 um 18:54 schrieb Jeff King:
>>> PS I actually would have made the rule simply "does it begin with a
>>>      '<'", which seems simpler still. If people accidentally write "<foo",
>>>      forgetting to close their brackets, that is a bug under both the
>>>      old and new behavior (just with slightly different outcomes).
>>
>> Good point.
> 
> Straying sideways into a tangent, but do we know if any locale wants
> to use something other than "<>" as an enclosing braket around a
> placeholder?

Bulgarian seems to use capitals instead; here are some examples found
with git grep -A1 'msgid "<' po/:

po/bg.po:msgid "<remote>"
po/bg.po-msgstr "ОТДАЛЕЧЕНО_ХРАНИЛИЩЕ"
--
po/bg.po:msgid "<branch>"
po/bg.po-msgstr "КЛОН"
--
po/bg.po:msgid "<subdirectory>/"
po/bg.po-msgstr "ПОДДИРЕКТОРИЯ/"
--
po/bg.po:msgid "<n>[,<base>]"
po/bg.po-msgstr "БРОЙ[,БАЗА]"

>  This arg-help text, for example,
> 
> 	N_("refspec")			without LIT-ARG-HELP
> 
> would be irritating for such a locale's translator, who cannot
> defeat the "<>" that is hardcoded and not inside _()
> 
> 	s = literal ? "%s" : "<%s>";
>                          
> that appear in parse-options.c::usage_argh().
> 
> Perhaps we should do _("<%s>") here?  That way, the result would
> hopefully be made consistent with
> 
> 	N_("<refspec>[:<expect>]")	with LIT-ARG-HELP
> 
> which allows translator to use the bracket of the locale's choice.

@Alexander: Would that help you?

René

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

[René Scharfe]

Am 02.08.2018 um 22:36 schrieb Junio C Hamano:
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
>> Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
>> patch of mine and replace it with something René came up with (I have
>> not yet read his 1-6 patches submitted on this topic, so maybe they're
>> not mutually exclusive).
> 
> I think the 6-patch series supersedes this one.  Thanks anyway.

Actually no, I specifically avoided touching builtin/push.c because this
patch already handled it; it should still be applied before patch 6 from
my series.

René

Re: [PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

[Kerry, Richard]

[-- Attachment #1: Type: text/plain, Size: 10892 bytes --]


s/angular brackets/angle brackets/



I've never seen these called "angular brackets".


Richard Kerry
BNCS Engineer, SI SOL Telco & Media Vertical Practice
M: +44 (0)7812 325518
2nd Floor, MidCity Place, 71 High Holborn, London, WC1V 6EA
richard.kerry@atos.net<https://webmail.siemens-it-solutions.com/owa/redir.aspx?C=9fb20d019e3e4cb99344d708709a3177&URL=mailto%3arichard.kerry%40atos.net>

This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos group liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted.


________________________________
From: git-owner@vger.kernel.org <git-owner@vger.kernel.org> on behalf of René Scharfe <l.s.r@web.de>
Sent: 02 August 2018 20:18
To: Jeff King; Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason; git@vger.kernel.org
Subject: [PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

Parseopt wraps argument help strings in a pair of angular brackets by
default, to tell users that they need to replace it with an actual
value.  This is useful in most cases, because most option arguments
are indeed single values of a certain type.  The option
PARSE_OPT_LITERAL_ARGHELP needs to be used in option definitions with
arguments that have multiple parts or are literal strings.

Stop adding these angular brackets if special characters are present,
as they indicate that we don't deal with a simple placeholder.  This
simplifies the code a bit and makes defining special options slightly
easier.

Remove the flag PARSE_OPT_LITERAL_ARGHELP in the cases where the new
and more cautious handling suffices.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
The patch to add PARSE_OPT_LITERAL_ARGHELP for push --force-with-lease
should be applied before this one.

 builtin/add.c          | 5 ++---
 builtin/pack-objects.c | 2 +-
 builtin/read-tree.c    | 2 +-
 builtin/send-pack.c    | 3 +--
 builtin/shortlog.c     | 3 +--
 builtin/show-branch.c  | 2 +-
 builtin/update-index.c | 2 +-
 builtin/write-tree.c   | 5 ++---
 parse-options.c        | 3 ++-
 9 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 84bfec9b73..ba1ff5689d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,9 +304,8 @@ static struct option builtin_add_options[] = {
         OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
         OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
         OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
-       { OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
-         N_("override the executable bit of the listed files"),
-         PARSE_OPT_LITERAL_ARGHELP },
+       OPT_STRING(0, "chmod", &chmod_arg, "(+|-)x",
+                  N_("override the executable bit of the listed files")),
         OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
                         N_("warn when adding an embedded repository")),
         OPT_END(),
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3a5d1fa317..b2323613bc 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3112,7 +3112,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
                          N_("similar to --all-progress when progress meter is shown")),
                 { OPTION_CALLBACK, 0, "index-version", NULL, N_("<version>[,<offset>]"),
                   N_("write the pack index file in the specified idx format version"),
-                 PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version },
+                 0, option_parse_index_version },
                 OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
                               N_("maximum size of each output pack file")),
                 OPT_BOOL(0, "local", &local,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index ebc43eb805..fbbc98e516 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -133,7 +133,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
                          N_("same as -m, but discard unmerged entries")),
                 { OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
                   N_("read the tree into the index under <subdirectory>/"),
-                 PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+                 PARSE_OPT_NONEG },
                 OPT_BOOL('u', NULL, &opts.update,
                          N_("update working tree with merge result")),
                 { OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 0b517c9368..724b484850 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -180,8 +180,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
                 { OPTION_CALLBACK,
                   0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
                   N_("require old value of ref to be at this value"),
-                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
-                 parseopt_push_cas_option },
+                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
                 OPT_END()
         };

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index f555cf9e4f..3898a2c9c4 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -269,8 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
                 OPT_BOOL('e', "email", &log.email,
                          N_("Show the email address of each author")),
                 { OPTION_CALLBACK, 'w', NULL, &log, N_("<w>[,<i1>[,<i2>]]"),
-                       N_("Linewrap output"),
-                       PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+                       N_("Linewrap output"), PARSE_OPT_OPTARG,
                         &parse_wrap_args },
                 OPT_END(),
         };
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2e985c00a..9106da1985 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -673,7 +673,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
                 { OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("<n>[,<base>]"),
                             N_("show <n> most recent ref-log entries starting at "
                                "base"),
-                           PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+                           PARSE_OPT_OPTARG,
                             parse_reflog_param },
                 OPT_END()
         };
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7feda6e271..293f59247b 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -973,7 +973,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
                         (parse_opt_cb *) cacheinfo_callback},
                 {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
                         N_("override the executable bit of the listed files"),
-                       PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+                       PARSE_OPT_NONEG,
                         chmod_callback},
                 {OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
                         N_("mark files as \"not changing\""),
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index c9d3c544e7..cdcbf8264e 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -24,9 +24,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
         struct option write_tree_options[] = {
                 OPT_BIT(0, "missing-ok", &flags, N_("allow missing objects"),
                         WRITE_TREE_MISSING_OK),
-               { OPTION_STRING, 0, "prefix", &prefix, N_("<prefix>/"),
-                 N_("write tree object for a subdirectory <prefix>") ,
-                 PARSE_OPT_LITERAL_ARGHELP },
+               OPT_STRING(0, "prefix", &prefix, N_("<prefix>/"),
+                          N_("write tree object for a subdirectory <prefix>")),
                 { OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
                   N_("only useful for debugging"),
                   PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL,
diff --git a/parse-options.c b/parse-options.c
index 7db84227ab..3b874a83a0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
         const char *s;
-       int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
+       int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
+               !opts->argh || !!strpbrk(opts->argh, "()<>[]|");
         if (opts->flags & PARSE_OPT_OPTARG)
                 if (opts->long_name)
                         s = literal ? "[=%s]" : "[=<%s>]";
--
2.18.0
Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at Second Floor, Mid City Place, 71 High Holborn, London, WC1V 6EA.  The VAT No. for each is: GB232327983.

This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email.

[-- Attachment #2: Type: text/html, Size: 23320 bytes --]

Re: [PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP

[Kerry, Richard]

s/angular brackets/angle brackets/

I've never seen these called "angular brackets".  Maybe a non-native English speaker issue.



Regards,
Richard.

PS. Please excuse my sending this twice, I don't seem to have default settings that are compatible with the list.




Richard Kerry
BNCS Engineer, SI SOL Telco & Media Vertical  Practice
M: +44 (0)7812 325518
2nd Floor, MidCity Place, 71 High Holborn, London, WC1V 6EA
richard.kerry@atos.net



 This e-mail and the documents attached are confidential and intended solely for the addressee; it may also be privileged. If you receive  this e-mail in error, please notify the sender immediately and destroy it. As its integrity cannot be secured on the Internet, the Atos group liability cannot be triggered for the message content. Although the sender endeavours to maintain a computer virus-free  network, the sender does not warrant that this transmission is virus-free and will not be liable for any damages resulting from any virus transmitted.




From: git-owner@vger.kernel.org <git-owner@vger.kernel.org> on behalf of René Scharfe <l.s.r@web.de>
Sent: 02 August 2018 20:18
To: Jeff King; Junio C Hamano
Cc: Ævar Arnfjörð Bjarmason; git@vger.kernel.org
Subject: [PATCH 6/6] parse-options: automatically infer PARSE_OPT_LITERAL_ARGHELP


Parseopt wraps argument help strings in a pair of angular brackets by
default, to tell users that they need to replace it with an actual
value.  This is useful in most cases, because most option arguments
are indeed single values of a certain type.  The option
PARSE_OPT_LITERAL_ARGHELP needs to be used in option definitions with
arguments that have multiple parts or are literal strings.

Stop adding these angular brackets if special characters are present,
as they indicate that we don't deal with a simple placeholder.  This
simplifies the code a bit and makes defining special options slightly
easier.

Remove the flag PARSE_OPT_LITERAL_ARGHELP in the cases where the new
and more cautious handling suffices.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
The patch to add PARSE_OPT_LITERAL_ARGHELP for push --force-with-lease
should be applied before this one.

 builtin/add.c          | 5 ++---
 builtin/pack-objects.c | 2 +-
 builtin/read-tree.c    | 2 +-
 builtin/send-pack.c    | 3 +--
 builtin/shortlog.c     | 3 +--
 builtin/show-branch.c  | 2 +-
 builtin/update-index.c | 2 +-
 builtin/write-tree.c   | 5 ++---
 parse-options.c        | 3 ++-
 9 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/builtin/add.c b/builtin/add.c
index 84bfec9b73..ba1ff5689d 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -304,9 +304,8 @@ static struct option builtin_add_options[] = {
         OPT_BOOL( 0 , "refresh", &refresh_only, N_("don't add, only refresh the index")),
         OPT_BOOL( 0 , "ignore-errors", &ignore_add_errors, N_("just skip files which cannot be added because of errors")),
         OPT_BOOL( 0 , "ignore-missing", &ignore_missing, N_("check if - even missing - files are ignored in dry run")),
-       { OPTION_STRING, 0, "chmod", &chmod_arg, "(+|-)x",
-         N_("override the executable bit of the listed files"),
-         PARSE_OPT_LITERAL_ARGHELP },
+       OPT_STRING(0, "chmod", &chmod_arg, "(+|-)x",
+                  N_("override the executable bit of the listed files")),
         OPT_HIDDEN_BOOL(0, "warn-embedded-repo", &warn_on_embedded_repo,
                         N_("warn when adding an embedded repository")),
         OPT_END(),
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 3a5d1fa317..b2323613bc 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -3112,7 +3112,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)
                          N_("similar to --all-progress when progress meter is shown")),
                 { OPTION_CALLBACK, 0, "index-version", NULL, N_("<version>[,<offset>]"),
                   N_("write the pack index file in the specified idx format version"),
-                 PARSE_OPT_LITERAL_ARGHELP, option_parse_index_version },
+                 0, option_parse_index_version },
                 OPT_MAGNITUDE(0, "max-pack-size", &pack_size_limit,
                               N_("maximum size of each output pack file")),
                 OPT_BOOL(0, "local", &local,
diff --git a/builtin/read-tree.c b/builtin/read-tree.c
index ebc43eb805..fbbc98e516 100644
--- a/builtin/read-tree.c
+++ b/builtin/read-tree.c
@@ -133,7 +133,7 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)
                          N_("same as -m, but discard unmerged entries")),
                 { OPTION_STRING, 0, "prefix", &opts.prefix, N_("<subdirectory>/"),
                   N_("read the tree into the index under <subdirectory>/"),
-                 PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP },
+                 PARSE_OPT_NONEG },
                 OPT_BOOL('u', NULL, &opts.update,
                          N_("update working tree with merge result")),
                 { OPTION_CALLBACK, 0, "exclude-per-directory", &opts,
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 0b517c9368..724b484850 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -180,8 +180,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
                 { OPTION_CALLBACK,
                   0, CAS_OPT_NAME, &cas, N_("<refname>:<expect>"),
                   N_("require old value of ref to be at this value"),
-                 PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
-                 parseopt_push_cas_option },
+                 PARSE_OPT_OPTARG, parseopt_push_cas_option },
                 OPT_END()
         };

diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index f555cf9e4f..3898a2c9c4 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -269,8 +269,7 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
                 OPT_BOOL('e', "email", &log.email,
                          N_("Show the email address of each author")),
                 { OPTION_CALLBACK, 'w', NULL, &log, N_("<w>[,<i1>[,<i2>]]"),
-                       N_("Linewrap output"),
-                       PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+                       N_("Linewrap output"), PARSE_OPT_OPTARG,
                         &parse_wrap_args },
                 OPT_END(),
         };
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index f2e985c00a..9106da1985 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -673,7 +673,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
                 { OPTION_CALLBACK, 'g', "reflog", &reflog_base, N_("<n>[,<base>]"),
                             N_("show <n> most recent ref-log entries starting at "
                                "base"),
-                           PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
+                           PARSE_OPT_OPTARG,
                             parse_reflog_param },
                 OPT_END()
         };
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7feda6e271..293f59247b 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -973,7 +973,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)
                         (parse_opt_cb *) cacheinfo_callback},
                 {OPTION_CALLBACK, 0, "chmod", &set_executable_bit, "(+|-)x",
                         N_("override the executable bit of the listed files"),
-                       PARSE_OPT_NONEG | PARSE_OPT_LITERAL_ARGHELP,
+                       PARSE_OPT_NONEG,
                         chmod_callback},
                 {OPTION_SET_INT, 0, "assume-unchanged", &mark_valid_only, NULL,
                         N_("mark files as \"not changing\""),
diff --git a/builtin/write-tree.c b/builtin/write-tree.c
index c9d3c544e7..cdcbf8264e 100644
--- a/builtin/write-tree.c
+++ b/builtin/write-tree.c
@@ -24,9 +24,8 @@ int cmd_write_tree(int argc, const char **argv, const char *unused_prefix)
         struct option write_tree_options[] = {
                 OPT_BIT(0, "missing-ok", &flags, N_("allow missing objects"),
                         WRITE_TREE_MISSING_OK),
-               { OPTION_STRING, 0, "prefix", &prefix, N_("<prefix>/"),
-                 N_("write tree object for a subdirectory <prefix>") ,
-                 PARSE_OPT_LITERAL_ARGHELP },
+               OPT_STRING(0, "prefix", &prefix, N_("<prefix>/"),
+                          N_("write tree object for a subdirectory <prefix>")),
                 { OPTION_BIT, 0, "ignore-cache-tree", &flags, NULL,
                   N_("only useful for debugging"),
                   PARSE_OPT_HIDDEN | PARSE_OPT_NOARG, NULL,
diff --git a/parse-options.c b/parse-options.c
index 7db84227ab..3b874a83a0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -660,7 +660,8 @@ int parse_options(int argc, const char **argv, const char *prefix,
 static int usage_argh(const struct option *opts, FILE *outfile)
 {
         const char *s;
-       int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) || !opts->argh;
+       int literal = (opts->flags & PARSE_OPT_LITERAL_ARGHELP) ||
+               !opts->argh || !!strpbrk(opts->argh, "()<>[]|");
         if (opts->flags & PARSE_OPT_OPTARG)
                 if (opts->long_name)
                         s = literal ? "[=%s]" : "[=<%s>]";
--
2.18.0

Atos, Atos Consulting, Worldline and Canopy The Open Cloud Company are trading names used by the Atos group. The following trading entities are registered in England and Wales: Atos IT Services UK Limited (registered number 01245534), Atos Consulting Limited (registered number 04312380), Atos Worldline UK Limited (registered number 08514184) and Canopy The Open Cloud Company Limited (registration number 08011902). The registered office for each is at Second Floor, Mid City Place, 71 High Holborn, London, WC1V 6EA.  The VAT No. for each is: GB232327983.

This e-mail and the documents attached are confidential and intended solely for the addressee, and may contain confidential or privileged information. If you receive this e-mail in error, you are not authorised to copy, disclose, use or retain it. Please notify the sender immediately and delete this email from your systems. As emails may be intercepted, amended or lost, they are not secure. Atos therefore can accept no liability for any errors or their content. Although Atos endeavours to maintain a virus-free network, we do not warrant that this transmission is virus-free and can accept no liability for any damages resulting from any virus transmitted. The risks are deemed to be accepted by everyone who communicates with Atos by email.

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 02.08.2018 um 22:01 schrieb Junio C Hamano:
>> 
>> Straying sideways into a tangent, but do we know if any locale wants
>> to use something other than "<>" as an enclosing braket around a
>> placeholder?
>
> Bulgarian seems to use capitals instead; here are some examples found
> with git grep -A1 'msgid "<' po/:
>
> po/bg.po:msgid "<remote>"
> po/bg.po-msgstr "ОТДАЛЕЧЕНО_ХРАНИЛИЩЕ"
> ...
>> 
>> Perhaps we should do _("<%s>") here?  That way, the result would
>> hopefully be made consistent with
>> 
>> 	N_("<refspec>[:<expect>]")	with LIT-ARG-HELP
>> 
>> which allows translator to use the bracket of the locale's choice.

I did not consider locales that do not use any kind of bracket, but
I guess _("<%s>") would work for them, too, in this context.  When
the locale's convention is to upcase, for example, the arg-help text
that is not marked with PARSE_OPT_LITERAL_ARGHELP would be upcased,
and _("<%s>") in the usage_argh() can be translated to "%s", which
passes the translated (i.e. upcased) arg-help text through.

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

[Junio C Hamano]

René Scharfe <l.s.r@web.de> writes:

> Am 02.08.2018 um 22:36 schrieb Junio C Hamano:
>> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>> 
>>> Thanks, FWIW that's fine by me, and also if you want to drop this "fake"
>>> patch of mine and replace it with something René came up with (I have
>>> not yet read his 1-6 patches submitted on this topic, so maybe they're
>>> not mutually exclusive).
>> 
>> I think the 6-patch series supersedes this one.  Thanks anyway.
>
> Actually no, I specifically avoided touching builtin/push.c because this
> patch already handled it; it should still be applied before patch 6 from
> my series.

You're of course correct.  Thanks.