Re: [PATCH] rev-parse --parseopt: allow [*=?!] in argument hints

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 12, 2015, 17:28 UTC
Message-ID: <xmqqzj31i8ts.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1436693990-2908-1-git-send-email-ilya.bobyr@gmail.com>

ilya.bobyr@gmail.com writes:

Show 7 quoted lines

> From: Ilya Bobyr <ilya.bobyr@gmail.com>
>
> It is not very likely that any of the "*=?!" Characters would be useful
> in the argument short or long names.  On the other hand, there are
> already argument hints that contain the "=" sign.  It used to be
> impossible to include any of the "*=?!" signs in the arguments hints
> before.

After reading this three times (without looking at the code), it is unclear to me what the change wants to achieve. A few points that confuse me:

 - "It is not very likely..."; so what does this change do to such
   an unlikely case?  Does it just forbid?  Or does it have escape
   hatches?

 - "... there are already ..."; so an unlikely case already exists?

 - "It used to be impossible..."; hmmmm, it earlier said there are
   already cases there---how they have been working?

Perhaps it would clarify the paragraph if you said upfront that a parseopt option specification is <opt-spec> (i.e. short and long names) optionally followed by <flags> (i.e. one or more of these "*=?!" characters) and the <arg-hint> string to remind the readers and reviewers, and phrase what you wrote to make the differences between them stand out?

    A line in the input to "rev-parse --parseopt" describes an
    option by listing a short and/or long name, optional flags
    [*=?!], argument hint, and then whitespace and help string.

    The code first finds the help string and scans backwards to find
    the flags, which would imply that [*=?!] is allowed in the
    option names but not in argument hint string.

    That is backwards; you do not want these special characters in
    option names, but you may want to be able to include them
    (especially '=', as in 'key=value') in the argument hint string.

    Change the parsing to go from the beginning to find the first
    occurrence of [*=?!] to find the flags and use the remainder as
    argument hint.

or something, perhaps.

> Added test case with equals sign in the argument hint and updated the

"Add a test case with ... and update the test ...". Write your log message as if you are giving somebody an order with your commit to do such and such.

> test to perform all the operations in test_expect_success matching the
> t\README requirements and allowing commands like

t/README.

Show 25 quoted lines

>
>     ./t1502-rev-parse-parseopt.sh --run=1-2
>
> to stop at the test case 2 without any further modification of the test
> state area.
>
> Signed-off-by: Ilya Bobyr <ilya.bobyr@gmail.com>
> ---
>  builtin/rev-parse.c           | 36 ++++++++--------
>  t/t1502-rev-parse-parseopt.sh | 97 ++++++++++++++++++++++++++-----------------
>  2 files changed, 77 insertions(+), 56 deletions(-)
>
> diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
> index b623239..205ea67 100644
> --- a/builtin/rev-parse.c
> +++ b/builtin/rev-parse.c
> @@ -423,17 +423,25 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
>  		o->flags = PARSE_OPT_NOARG;
>  		o->callback = &parseopt_dump;
>  
> -		/* Possible argument name hint */
> +		/* parse names, type and the hint */
>  		end = s;
> -		while (s > sb.buf && strchr("*=?!", s[-1]) == NULL)
> -			--s;

I must have overlooked this one long time ago when a patch added this; it is a horrible way to parse a thing from the tail. Good to see the code go ;-)

Show 9 quoted lines

> -		if (s != sb.buf && s != end)
> -			o->argh = xmemdupz(s, end - s);
> -		if (s == sb.buf)
> -			s = end;
> +		s = sb.buf;
> +
> +		/* name(s) */
> +		while (s < end && strchr("*=?!", *s) == NULL)
> +			++s;

In C, we usually pre-decrement and post-increment, unless the value is used.

More importantly, can't we write this more concisely by using strcspn(3)?

	const char *flags_chars = "*=?!";
        size_t leading = strcspn(s, flags_chars);

	if (s + leading < end)
        	... /* s + leading is the beginning of flags */
	else
        	... /* there was no flags before end */

Show 15 quoted lines

> +
> +		if (s - sb.buf == 1) /* short option only */
> +			o->short_name = *sb.buf;
> +		else if (sb.buf[1] != ',') /* long option only */
> +			o->long_name = xmemdupz(sb.buf, s - sb.buf);
> +		else {
> +			o->short_name = *sb.buf;
> +			o->long_name = xmemdupz(sb.buf + 2, s - sb.buf - 2);
> +		}
>  
> -		while (s > sb.buf && strchr("*=?!", s[-1])) {
> -			switch (*--s) {
> +		while (s < end && strchr("*=?!", *s)) {
> +			switch (*s++) {
>  			case '=':

No need for the strchr() dance, I think, because you will do different things depending on *s inside the loop anyway. Just

		while (s < end) {
                	switch (*s++) {
                        case '=':
                        	do the "equal" thing;
                                continue;
			case '*':
                        	do the "asterisk" thing;
                                continue;
                                ...
			}
                        break;
		}

or something.

Yes, I agree that the original is coded very incompetently, but there is no reason to inherit that to your fixed version ;-).

Thanks.