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

Junio C Hamano [off-list ref] writes:

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

quoted

+		  /* 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 <redacted>
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 [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 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

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 <redacted>
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 [off-list ref]

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

... 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 },