Re: [PATCH 1/3] rev-parse: support OPT_NUMBER_CALLBACK in --parseopt
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Mar 4, 2014, 18:28 UTC
- Message-ID
- <xmqqiortlsxl.fsf@gitster.dls.corp.google.com>
- In-Reply-To
- <1393728794-29566-2-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 5 quoted lines
> If the option spec is > > -NUM Help string > > then rev-parse will accept and parse -([0-9]+) and return "-NUM $1"
Even though the hardcoded "NUM" token initially gave me a knee-jerk "Yuck" reaction, that literal option name is very unlikely to be desired by scripts/commands for their real option names, and being in all uppercase it is very clear that it is magic convention between the parsing mechanism and the script it uses.
It however felt funny to me without a matching (possibly hidden) mechanism to allow parse-options machinery to consume such an output as its input. In a script that uses this mechanism to parse out the numeric option "-NUM 3" out of "git script -3" and uses that "three" to drive an underlying command (e.g. "git grep -3"), wouldn't it be more natural if that underlying command can be told to accept the same notation (i.e. "git grep -NUM 3")? For that to be consistent with the rest of the system, "-NUM" would not be a good token; being it multi-character, it must be "--NUM" or something with double-dash prefix.
I kind of like the basic idea, the capability it tries to give scripted Porcelain implementations. But my impression is that "rebase -i -4", which this mechanism was invented for, is not progressing, so perhaps we should wait until the real user of this feature appears.
Thanks.
Show 39 quoted lines
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
> builtin/rev-parse.c | 9 +++++++--
> 1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index 45901df..b37676f 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -331,6 +331,8 @@ static int parseopt_dump(const struct option *o, const char *arg, int unset)
> struct strbuf *parsed = o->value;
> if (unset)
> strbuf_addf(parsed, " --no-%s", o->long_name);
> + else if (o->type == OPTION_NUMBER)
> + strbuf_addf(parsed, " -NUM");
> else if (o->short_name && (o->long_name == NULL || !stuck_long))
> strbuf_addf(parsed, " -%c", o->short_name);
> else
> @@ -338,7 +340,7 @@ static int parseopt_dump(const struct option *o, const char *arg, int unset)
> if (arg) {
> if (!stuck_long)
> strbuf_addch(parsed, ' ');
> - else if (o->long_name)
> + else if (o->long_name || o->type == OPTION_NUMBER)
> strbuf_addch(parsed, '=');
> sq_quote_buf(parsed, arg);
> }
> @@ -439,7 +441,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>
> if (s - sb.buf == 1) /* short option only */
> o->short_name = *sb.buf;
> - else if (sb.buf[1] != ',') /* long option only */
> + else if (s - sb.buf == 4 && !strncmp(sb.buf, "-NUM", 4)) {
> + o->type = OPTION_NUMBER;
> + o->flags = PARSE_OPT_NOARG | PARSE_OPT_NONEG;
> + } else if (sb.buf[1] != ',') /* long option only */
> o->long_name = xmemdupz(sb.buf, s - sb.buf);
> else {
> o->short_name = *sb.buf;