Re: Re* [PATCH] push: comment on a funny unbalanced option help
René Scharfe writes: > Am 02.08.2018 um 22:36 schrieb Junio C Hamano: >> Ævar Arnfjörð Bjarmason 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.
Re: [PATCH] push: comment on a funny unbalanced option help
René Scharfe 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 ""
> po/bg.po-msgstr "ОТДАЛЕЧЕНО_ХРАНИЛИЩЕ"
> ...
>>
>> Perhaps we should do _("<%s>") here? That way, the result would
>> hopefully be made consistent with
>>
>> N_("[:]") 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
Am 02.08.2018 um 22:36 schrieb Junio C Hamano: > Ævar Arnfjörð Bjarmason 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] push: comment on a funny unbalanced option help
Am 02.08.2018 um 22:01 schrieb Junio C Hamano:
> René Scharfe 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 ">> 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 ""
po/bg.po-msgstr "ОТДАЛЕЧЕНО_ХРАНИЛИЩЕ"
--
po/bg.po:msgid ""
po/bg.po-msgstr "КЛОН"
--
po/bg.po:msgid "/"
po/bg.po-msgstr "ПОДДИРЕКТОРИЯ/"
--
po/bg.po:msgid "[,]"
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_("[:]") 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
Ævar Arnfjörð Bjarmason 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: Re* [PATCH] push: comment on a funny unbalanced option help
On Thu, Aug 02 2018, Junio C Hamano wrote:
> René Scharfe 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
>>> Date: Thu, 02 Aug 2018 00:31:33 +0200
>>> ...
>>> official escape hatch instead.
>>>
>>> Helped-by: René Scharfe
>>
>> I didn't do anything for this particular patch so far? But...
>>
>>> Signed-off-by: Junio C Hamano
>
> 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>:>> + 0, CAS_OPT_NAME, &cas, N_(":"),
>>
>> ... 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 ":" is a misfeature that should be deprecated (in
> which case we would not want to "s|:|[&]|"), but I may be
> misremembering it.
Re: [PATCH] push: comment on a funny unbalanced option help
René Scharfe 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 "> 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_("[:]") with LIT-ARG-HELP
which allows translator to use the bracket of the locale's choice.
Re: [PATCH] push: comment on a funny unbalanced option help
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 " 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 (+/-)xoverride 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 <> use the specified diff tool
+-t, --tool use the specified diff tool
--x, --extcmd <>
+-x, --extcmd
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
+--index-version version[,offset]
Not good before, worse after. Should be to "[,]".
git pull:
--r, --rebase[=]
+-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[=:]
+--force-with-lease[=refname>:]
+--recurse-submodules[=check|on-demand|no]
---signed[=]
+--signed[=yes|no|if-asked]
git send-pack:
---signed[=]
+--signed[=yes|no|if-asked]
Good changes all three, but need parentheses..
---force-with-lease[=:]
+--force-with-lease[=refname>:] Linewrap output
+-w[w[,i1[,i2]]] Linewrap output
Not good before, worse after. Should be "[[,[,]]]".
git update-index:
---cacheinfo ,,
- 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é
Re: [PATCH] push: comment on a funny unbalanced option help
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 "bar" that you do want enclosed could be
spelled "". 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 "
Re: [PATCH] push: comment on a funny unbalanced option help
Am 02.08.2018 um 17:31 schrieb Junio C Hamano:
> René Scharfe 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: Re* [PATCH] push: comment on a funny unbalanced option help
René Scharfe 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
>> Date: Thu, 02 Aug 2018 00:31:33 +0200
>> ...
>> official escape hatch instead.
>>
>> Helped-by: René Scharfe
>
> I didn't do anything for this particular patch so far? But...
>
>> Signed-off-by: Junio C Hamano
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>:> + 0, CAS_OPT_NAME, &cas, N_(":"),
>
> ... 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 ":" is a misfeature that should be deprecated (in
which case we would not want to "s|:|[&]|"), but I may be
misremembering it.
Re: Re* [PATCH] push: comment on a funny unbalanced option help
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
> 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[=:]
>
> which comes from having N_("refname>: 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
I didn't do anything for this particular patch so far? But...
> Signed-off-by: Junio C Hamano
> ---
> 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>: + 0, CAS_OPT_NAME, &cas, N_(":"),
... shouldn't we use this opportunity to document that "expect" is
optional?
+ 0, CAS_OPT_NAME, &cas, N_("[:]"),
> 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* [PATCH] push: comment on a funny unbalanced option help
Junio C Hamano writes:
> Ævar Arnfjörð Bjarmason writes:
>
>>> + /* N_() will get "<>" around, resulting in
>>> ":" */
>>
>> ...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
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[=:]
which comes from having N_("refname>:" 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
Signed-off-by: Junio C Hamano
---
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>::"),
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: [PATCH] push: comment on a funny unbalanced option help
René Scharfe 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
> ---
> { OPTION_STRING, 0, "prefix", &opts.prefix,
> N_("/"),
> { OPTION_CALLBACK, 'g', "reflog", &reflog_base,
> N_("[,]"),
> N_(",,"),
> + OPT_STRING(0, "prefix", &prefix, N_("/"),
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
Ævar Arnfjörð Bjarmason 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>:>> + 0, CAS_OPT_NAME, &cas, N_(":"),
>>>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>::" in push.c, so that the help
> we emit is --force-with-lease[=<:>].
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
and 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 <:>.
> As noted in 29f25d493c this facility wasn't added with the intent
> turning --refspec=<> into --refspec=, but to do stuff
> like --option=[,] for options that take comma-delimited
> options.
There is no --refspec=<> to begin with.
A single placeholder can be written in the source as "refspec" and
shown as "--refspec=" because you get the surrounding <>
pair for free by default. Nobody would want to write "" in
the arg help, as most of the option arguments are a single value
placeholder. But if you want "[,]" 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= meaning "one of a, b or c", --refspec=
> meaning "the literal string 'refspec'" and --refspec=<> meaning
> add a string, i.e. fill in your refspec here.
Ah, this is where you are off-by-one. "--refspec=" means
"give your refspec as its value".
Re: [PATCH] push: comment on a funny unbalanced option help
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[=[:]] > > ... 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 "[:]". René
Re: [PATCH] push: comment on a funny unbalanced option help
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>:>> + 0, CAS_OPT_NAME, &cas, N_(":"),
>>> 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>::" in push.c, so that the help
> we emit is --force-with-lease[=<:>].
>
> As noted in 29f25d493c this facility wasn't added with the intent
> turning --refspec=<> into --refspec=, but to do stuff
> like --option=[,] for options that take comma-delimited
> options.
>
> If we're magically removing <>'s we have no consistent convention to
> tell apart --opt= meaning "one of a, b or c", --refspec=
> meaning "the literal string 'refspec'" and --refspec=<> meaning
> add a 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=
Multi-part options aren't special in this syntax:
--force-with-lease=:
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[=[:]]
... 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
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>:> + 0, CAS_OPT_NAME, &cas, N_(":"),
>>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>::" in push.c, so that the help
we emit is --force-with-lease[=<:>].
As noted in 29f25d493c this facility wasn't added with the intent
turning --refspec=<> into --refspec=, but to do stuff
like --option=[,] for options that take comma-delimited
options.
If we're magically removing <>'s we have no consistent convention to
tell apart --opt= meaning "one of a, b or c", --refspec=
meaning "the literal string 'refspec'" and --refspec=<> meaning
add a string, i.e. fill in your refspec here.
Re: [PATCH] push: comment on a funny unbalanced option help
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>: + 0, CAS_OPT_NAME, &cas, N_(":"),
>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
---
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_("/"),
N_("read the tree into the index under /"),
- 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_("show 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_(",,"),
N_("add the specified entry to the index"),
PARSE_OPT_NOARG | /* disallow --cacheinfo= 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[] = {
Re: [PATCH] push: comment on a funny unbalanced option help
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
On Wed, Aug 01 2018, Junio C Hamano wrote:
> Ævar Arnfjörð Bjarmason writes:
>
>>> + /* N_() will get "<>" around, resulting in
>>> ":" */
>>
>> ...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 ":" */
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>::"),
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
Ævar Arnfjörð Bjarmason writes: >> + /* N_() will get "<>" around, resulting in >> ":" */ > > ...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
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[=:]
>
> which come from this
>
> 0, CAS_OPT_NAME, &cas, N_("refname>:
> 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
> ":" */
...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>::&1 | grep -e force-with-lease
--force-with-lease[=:]
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_(""),
builtin/difftool.c:714: OPT_STRING('x', "extcmd", &extcmd,
N_(""),
Producing this bug:
$ git difftool -h 2>&1|grep '<<'
-t, --tool <> use the specified diff tool
-x, --extcmd <>
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_("/"),
builtin/show-branch.c:673: { OPTION_CALLBACK, 'g', "reflog",
&reflog_base, N_("[,]"),
builtin/update-index.c:969:
N_(",,"),
builtin/write-tree.c:27:{ OPTION_STRING, 0, "prefix",
&prefix, N_("/"),
[PATCH] push: comment on a funny unbalanced option help
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[=:]
which come from this
0, CAS_OPT_NAME, &cas, N_("refname>:" 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
---
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
":" */
0, CAS_OPT_NAME, &cas, N_("refname>:
