git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Philip Oakley" <philipoakley@iee.org>
To: <ilya.bobyr@gmail.com>, <git@vger.kernel.org>
Cc: "Junio C Hamano" <gitster@pobox.com>,
	"Pierre Habouzit" <madcoder@debian.org>,
	"Ilya Bobyr" <ilya.bobyr@gmail.com>
Subject: Re: [PATCH] rev-parse --parseopt: allow [*=?!] in argument hints
Date: Sun, 12 Jul 2015 19:22:34 +0100	[thread overview]
Message-ID: <E1A07B6921124F5B8B6537C21556BD86@PhilipOakley> (raw)
In-Reply-To: 1436693990-2908-1-git-send-email-ilya.bobyr@gmail.com

From: <ilya.bobyr@gmail.com> Sent: Sunday, July 12, 2015 10:39 AM
> 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.

I found this difficult to parse, and had to check the man pages to 
understand the proposal, which were nearly as bad (for me).

Perhaps:
"In the 'git rev-parse' parseopt mode, the the short and long options 
<opt-spec> cannot contain any of the terminating flag characters "*=?!". 
This restriction does not apply to the argument name hint <arg-hint> 
which is space terminated.

Allow flag characters in the <arg-hint>, after the arg-hint's initial 
non flag character."

Would that be a correct understanding?
--
Philip

>
> Added test case with equals sign in the argument hint and updated the
> test to perform all the operations in test_expect_success matching the
> t\README requirements and allowing commands like
>
>    ./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;
> - 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;
> +
> + 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 '=':
>  o->flags &= ~PARSE_OPT_NOARG;
>  break;
> @@ -450,14 +458,8 @@ 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 */
> - 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);
> - }
> + if (s < end)
> + o->argh = xmemdupz(s, end - s);
>  }
>  strbuf_release(&sb);
>
> diff --git a/t/t1502-rev-parse-parseopt.sh 
> b/t/t1502-rev-parse-parseopt.sh
> index ebe7c3b..d5e5720 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -3,7 +3,39 @@
> test_description='test git rev-parse --parseopt'
> . ./test-lib.sh
>
> -sed -e 's/^|//' >expect <<\END_EXPECT
> +test_expect_success 'setup optionspec' '
> + sed -e "s/^|//" >optionspec <<\EOF
> +|some-command [options] <args>...
> +|
> +|some-command does foo and bar!
> +|--
> +|h,help    show the help
> +|
> +|foo       some nifty option --foo
> +|bar=      some cool option --bar with an argument
> +|b,baz     a short and long option
> +|
> +| An option group Header
> +|C?        option C with an optional argument
> +|d,data?   short and long option with an optional argument
> +|
> +| Argument hints
> +|B=arg     short option required argument
> +|bar2=arg  long option required argument
> +|e,fuz=with-space  short and long option required argument
> +|s?some    short option optional argument
> +|long?data long option optional argument
> +|g,fluf?path     short and long option optional argument
> +|longest=very-long-argument-hint  a very long argument hint
> +|pair=key=value  with an equals sign in the hint
> +|
> +|Extras
> +|extra1    line above used to cause a segfault but no longer does
> +EOF
> +'
> +
> +test_expect_success 'test --parseopt help output' '
> + sed -e "s/^|//" >expect <<\END_EXPECT &&
> |cat <<\EOF
> |usage: some-command [options] <args>...
> |
> @@ -28,49 +60,22 @@ sed -e 's/^|//' >expect <<\END_EXPECT
> |    -g, --fluf[=<path>]   short and long option optional argument
> |    --longest <very-long-argument-hint>
> |                          a very long argument hint
> +|    --pair <key=value>    with an equals sign in the hint
> |
> |Extras
> |    --extra1              line above used to cause a segfault but no 
> longer does
> |
> |EOF
> END_EXPECT
> -
> -sed -e 's/^|//' >optionspec <<\EOF
> -|some-command [options] <args>...
> -|
> -|some-command does foo and bar!
> -|--
> -|h,help    show the help
> -|
> -|foo       some nifty option --foo
> -|bar=      some cool option --bar with an argument
> -|b,baz     a short and long option
> -|
> -| An option group Header
> -|C?        option C with an optional argument
> -|d,data?   short and long option with an optional argument
> -|
> -| Argument hints
> -|B=arg     short option required argument
> -|bar2=arg  long option required argument
> -|e,fuz=with-space  short and long option required argument
> -|s?some    short option optional argument
> -|long?data long option optional argument
> -|g,fluf?path     short and long option optional argument
> -|longest=very-long-argument-hint  a very long argument hint
> -|
> -|Extras
> -|extra1    line above used to cause a segfault but no longer does
> -EOF
> -
> -test_expect_success 'test --parseopt help output' '
>  test_expect_code 129 git rev-parse --parseopt -- -h > output < 
> optionspec &&
>  test_i18ncmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.1' "
> + cat > expect <<EOF
> set -- --foo --bar 'ham' -b -- 'arg'
> EOF
> +"
>
> test_expect_success 'test --parseopt' '
>  git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > 
> output &&
> @@ -82,9 +87,11 @@ test_expect_success 'test --parseopt with mixed 
> options and arguments' '
>  test_cmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.2' "
> + cat > expect <<EOF
> set -- --foo -- 'arg' '--bar=ham'
> EOF
> +"
>
> test_expect_success 'test --parseopt with --' '
>  git rev-parse --parseopt -- --foo -- arg --bar=ham < optionspec > 
> output &&
> @@ -96,54 +103,66 @@ test_expect_success 
> 'test --parseopt --stop-at-non-option' '
>  test_cmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.3' "
> + cat > expect <<EOF
> set -- --foo -- '--' 'arg' '--bar=ham'
> EOF
> +"
>
> test_expect_success 'test --parseopt --keep-dashdash' '
>  git rev-parse --parseopt --keep-dashdash -- --foo -- arg --bar=ham < 
> optionspec > output &&
>  test_cmp expect output
> '
>
> -cat >expect <<EOF
> +test_expect_success 'setup expect.4' "
> + cat >expect <<EOF
> set -- --foo -- '--' 'arg' '--spam=ham'
> EOF
> +"
>
> test_expect_success 
> 'test --parseopt --keep-dashdash --stop-at-non-option with --' '
>  git 
> rev-parse --parseopt --keep-dashdash --stop-at-non-option -- --foo --  
> arg --spam=ham <optionspec >output &&
>  test_cmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.5' "
> + cat > expect <<EOF
> set -- --foo -- 'arg' '--spam=ham'
> EOF
> +"
>
> test_expect_success 
> 'test --parseopt --keep-dashdash --stop-at-non-option without --' '
>  git 
> rev-parse --parseopt --keep-dashdash --stop-at-non-option -- --foo 
> arg --spam=ham <optionspec >output &&
>  test_cmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.6' "
> + cat > expect <<EOF
> set -- --foo --bar='z' --baz -C'Z' --data='A' -- 'arg'
> EOF
> +"
>
> test_expect_success 'test --parseopt --stuck-long' '
>  git rev-parse --parseopt --stuck-long -- --foo --bar=z -b arg -CZ -dA 
> <optionspec >output &&
>  test_cmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.7' "
> + cat > expect <<EOF
> set -- --data='' -C --baz -- 'arg'
> EOF
> +"
>
> test_expect_success 'test --parseopt --stuck-long and empty optional 
> argument' '
>  git rev-parse --parseopt --stuck-long -- --data= arg -C -b 
> <optionspec >output &&
>  test_cmp expect output
> '
>
> -cat > expect <<EOF
> +test_expect_success 'setup expect.8' "
> + cat > expect <<EOF
> set -- --data --baz -- 'arg'
> EOF
> +"
>
> test_expect_success 'test --parseopt --stuck-long and long option with 
> unset optional argument' '
>  git rev-parse --parseopt --stuck-long -- --data arg -b <optionspec 
>  >output &&
> -- 
> 2.4.5
>

  parent reply	other threads:[~2015-07-12 18:22 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-12  9:39 [PATCH] rev-parse --parseopt: allow [*=?!] in argument hints ilya.bobyr
2015-07-12 17:28 ` Junio C Hamano
2015-07-12 18:22 ` Philip Oakley [this message]
2015-07-13 10:12 ` [PATCHv2] " ilya.bobyr
2015-07-13 11:19   ` Philip Oakley
2015-07-13 21:55   ` Junio C Hamano
2015-07-14  8:17   ` [PATCHv3] " Ilya Bobyr

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=E1A07B6921124F5B8B6537C21556BD86@PhilipOakley \
    --to=philipoakley@iee.org \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=ilya.bobyr@gmail.com \
    --cc=madcoder@debian.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).