git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] rev-parse --parseopt: fix handling of optional arguments
@ 2013-10-15 12:00 Nicolas Vigier
  2013-10-15 22:55 ` Junio C Hamano
  2013-10-15 23:14 ` Jonathan Nieder
  0 siblings, 2 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-15 12:00 UTC (permalink / raw)
  To: git; +Cc: Pierre Habouzit, Nicolas Vigier

git rev-parse --parseopt does not allow us to see the difference
between an option with an optional argument starting with a dash, and an
option with an unset optional argument followed by an other option.

If I use this script :

  $ cat /tmp/opt.sh
  #!/bin/sh
  OPTIONS_SPEC="\
  git [options]
  --
  q,quiet         be quiet
  S,gpg-sign?     GPG-sign commit"
  echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@"

Then the following two commands give us the same result :

  $ /tmp/opt.sh -S -q
  set -- -S -q --
  $ /tmp/opt.sh -S-q
  set -- -S '-q' --

We cannot know if '-q' is an argument to '-S' or a new option.

With this patch, rev-parse --parseopt will always give an argument to
optional options, as an empty string if the argument is unset.

The same two commands now give us :

  $ /tmp/opt.sh -S -q
  set -- -S '' -q --
  $ /tmp/opt.sh -S-q
  set -- -S '-q' --

We can now see if '-q' is an argument to '-S' or an other option.

Also adding two tests in t1502.

There does not seem to be any shell script git command included in git
sources tree that is currently using optional arguments and could be
affected by this change.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
---
 builtin/rev-parse.c           |  3 +++
 t/t1502-rev-parse-parseopt.sh | 18 ++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index de894c7..25e8c74 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -327,6 +327,9 @@ static int parseopt_dump(const struct option *o, const char *arg, int unset)
 	if (arg) {
 		strbuf_addch(parsed, ' ');
 		sq_quote_buf(parsed, arg);
+	} else if (o->flags & PARSE_OPT_OPTARG) {
+		const char empty_arg[] = " ''";
+		strbuf_add(parsed, empty_arg, strlen(empty_arg));
 	}
 	return 0;
 }
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 13c88c9..abe7c2f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -99,4 +99,22 @@ test_expect_success 'test --parseopt --keep-dashdash --stop-at-non-option withou
 	test_cmp expect output
 '
 
+cat > expect <<EOF
+set -- -C '' --foo --
+EOF
+
+test_expect_success 'test --parseopt -C --foo' '
+	git rev-parse --parseopt -- -C --foo <optionspec >output &&
+	test_cmp expect output
+'
+
+cat > expect <<EOF
+set -- -C '--foo' --
+EOF
+
+test_expect_success 'test --parseopt -C--foo' '
+	git rev-parse --parseopt -- -C--foo <optionspec >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 12:00 [PATCH] rev-parse --parseopt: fix handling of optional arguments Nicolas Vigier
@ 2013-10-15 22:55 ` Junio C Hamano
  2013-10-15 23:47   ` Nicolas Vigier
  2013-10-15 23:14 ` Jonathan Nieder
  1 sibling, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2013-10-15 22:55 UTC (permalink / raw)
  To: Nicolas Vigier; +Cc: git, Pierre Habouzit

Nicolas Vigier <boklm@mars-attacks.org> writes:

> git rev-parse --parseopt does not allow us to see the difference
> between an option with an optional argument starting with a dash, and an
> option with an unset optional argument followed by an other option.
>
> If I use this script :
>
>   $ cat /tmp/opt.sh
>   #!/bin/sh
>   OPTIONS_SPEC="\
>   git [options]
>   --
>   q,quiet         be quiet
>   S,gpg-sign?     GPG-sign commit"
>   echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@"
>
> Then the following two commands give us the same result :
>
>   $ /tmp/opt.sh -S -q
>   set -- -S -q --
>   $ /tmp/opt.sh -S-q
>   set -- -S '-q' --
>
> We cannot know if '-q' is an argument to '-S' or a new option.
>
> With this patch, rev-parse --parseopt will always give an argument to
> optional options, as an empty string if the argument is unset.
>
> The same two commands now give us :
>
>   $ /tmp/opt.sh -S -q
>   set -- -S '' -q --
>   $ /tmp/opt.sh -S-q
>   set -- -S '-q' --

Two are different, but the former "set -- -S '' -q --" is not what
you want, either, no?  -S with an explicit empty argument and -S
alone without argument may mean two totally different things, which
is the whole point of "option with an optional parameter".  If some
code that have been using "rev-parse --parseopt" was happy with

	$ /tmp/opt.sh -S
        set -- -S --

and then your updated version gave it this instead:

	$ /tmp/opt.sh -S
        set -- -S '' --

wouldn't it be a regression to them?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 12:00 [PATCH] rev-parse --parseopt: fix handling of optional arguments Nicolas Vigier
  2013-10-15 22:55 ` Junio C Hamano
@ 2013-10-15 23:14 ` Jonathan Nieder
  2013-10-15 23:33   ` Junio C Hamano
  2013-10-15 23:53   ` [PATCH] rev-parse --parseopt: fix handling of optional arguments Nicolas Vigier
  1 sibling, 2 replies; 25+ messages in thread
From: Jonathan Nieder @ 2013-10-15 23:14 UTC (permalink / raw)
  To: Nicolas Vigier; +Cc: git, Pierre Habouzit

Nicolas Vigier wrote:

>   $ cat /tmp/opt.sh
>   #!/bin/sh
>   OPTIONS_SPEC="\
>   git [options]
>   --
>   q,quiet         be quiet
>   S,gpg-sign?     GPG-sign commit"
>   echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@"
>
> Then the following two commands give us the same result :
>
>   $ /tmp/opt.sh -S -q
>   set -- -S -q --
>   $ /tmp/opt.sh -S-q
>   set -- -S '-q' --
>
> We cannot know if '-q' is an argument to '-S' or a new option.

Hmph.

As Junio mentioned, inserting '' would be a backward-incompatible
change.  I don't think it's worth breaking existing scripts.  Probably
what is needed is a new parseopt special character with the new
semantics (e.g.,

	Use ?? to mean the option has an optional argument.  If the
	option is supplied without its argument, the argument is taken
	to be ''.

or something like

	Use ?<default> to mean the option has an optional argument.  If
	the option is supplied without its argument and <default> is
	nonempty, the argument is taken to be <default>.

).

Sensible?

Thanks,
Jonathan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 23:14 ` Jonathan Nieder
@ 2013-10-15 23:33   ` Junio C Hamano
  2013-10-15 23:57     ` Jonathan Nieder
  2013-10-15 23:53   ` [PATCH] rev-parse --parseopt: fix handling of optional arguments Nicolas Vigier
  1 sibling, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2013-10-15 23:33 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Nicolas Vigier, git, Pierre Habouzit

Jonathan Nieder <jrnieder@gmail.com> writes:

> Nicolas Vigier wrote:
>
>>   $ cat /tmp/opt.sh
>>   #!/bin/sh
>>   OPTIONS_SPEC="\
>>   git [options]
>>   --
>>   q,quiet         be quiet
>>   S,gpg-sign?     GPG-sign commit"
>>   echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@"
>>
>> Then the following two commands give us the same result :
>>
>>   $ /tmp/opt.sh -S -q
>>   set -- -S -q --
>>   $ /tmp/opt.sh -S-q
>>   set -- -S '-q' --
>>
>> We cannot know if '-q' is an argument to '-S' or a new option.
>
> Hmph.
>
> As Junio mentioned, inserting '' would be a backward-incompatible
> change.  I don't think it's worth breaking existing scripts.  Probably
> what is needed is a new parseopt special character with the new
> semantics (e.g.,
>
> 	Use ?? to mean the option has an optional argument.  If the
> 	option is supplied without its argument, the argument is taken
> 	to be ''.
>
> or something like
>
> 	Use ?<default> to mean the option has an optional argument.  If
> 	the option is supplied without its argument and <default> is
> 	nonempty, the argument is taken to be <default>.
>
> ).
>
> Sensible?

You just made these two that the user clearly meant to express two
different things indistinguishable.

	opt.sh -S
        opt.sh -S ''

So I do not think it is sensible. In fact, I do not think there is
any sensible way to handle a shortopt with optional parameter that
is not at the end of the command line.

And that is exactly why gitcli.txt tells users to use the 'sticked'
form, and ends the bullet point with:

   An option that takes optional option-argument must be written in
   the 'sticked' form.

That still does not give the command line a way to express an option
that could take an optional argument without the optional argument
in the middle of the command line, though.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 22:55 ` Junio C Hamano
@ 2013-10-15 23:47   ` Nicolas Vigier
  0 siblings, 0 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-15 23:47 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Pierre Habouzit

On Tue, 15 Oct 2013, Junio C Hamano wrote:

> Nicolas Vigier <boklm@mars-attacks.org> writes:
> 
> > git rev-parse --parseopt does not allow us to see the difference
> > between an option with an optional argument starting with a dash, and an
> > option with an unset optional argument followed by an other option.
> >
> > If I use this script :
> >
> >   $ cat /tmp/opt.sh
> >   #!/bin/sh
> >   OPTIONS_SPEC="\
> >   git [options]
> >   --
> >   q,quiet         be quiet
> >   S,gpg-sign?     GPG-sign commit"
> >   echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@"
> >
> > Then the following two commands give us the same result :
> >
> >   $ /tmp/opt.sh -S -q
> >   set -- -S -q --
> >   $ /tmp/opt.sh -S-q
> >   set -- -S '-q' --
> >
> > We cannot know if '-q' is an argument to '-S' or a new option.
> >
> > With this patch, rev-parse --parseopt will always give an argument to
> > optional options, as an empty string if the argument is unset.
> >
> > The same two commands now give us :
> >
> >   $ /tmp/opt.sh -S -q
> >   set -- -S '' -q --
> >   $ /tmp/opt.sh -S-q
> >   set -- -S '-q' --
> 
> Two are different, but the former "set -- -S '' -q --" is not what
> you want, either, no?  -S with an explicit empty argument and -S
> alone without argument may mean two totally different things, which
> is the whole point of "option with an optional parameter".  If some
> code that have been using "rev-parse --parseopt" was happy with
> 
> 	$ /tmp/opt.sh -S
>         set -- -S --
> 
> and then your updated version gave it this instead:
> 
> 	$ /tmp/opt.sh -S
>         set -- -S '' --
> 
> wouldn't it be a regression to them?

Indeed, this could be a regression to them. I couldn't find any script
using "rev-parse --parseopt" with an option with an optional argument,
but yes, it doesn't mean that nobody uses that.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 23:14 ` Jonathan Nieder
  2013-10-15 23:33   ` Junio C Hamano
@ 2013-10-15 23:53   ` Nicolas Vigier
  1 sibling, 0 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-15 23:53 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: git, Pierre Habouzit

On Tue, 15 Oct 2013, Jonathan Nieder wrote:

> Nicolas Vigier wrote:
> 
> >   $ cat /tmp/opt.sh
> >   #!/bin/sh
> >   OPTIONS_SPEC="\
> >   git [options]
> >   --
> >   q,quiet         be quiet
> >   S,gpg-sign?     GPG-sign commit"
> >   echo "$OPTIONS_SPEC" | git rev-parse --parseopt $parseopt_extra -- "$@"
> >
> > Then the following two commands give us the same result :
> >
> >   $ /tmp/opt.sh -S -q
> >   set -- -S -q --
> >   $ /tmp/opt.sh -S-q
> >   set -- -S '-q' --
> >
> > We cannot know if '-q' is an argument to '-S' or a new option.
> 
> Hmph.
> 
> As Junio mentioned, inserting '' would be a backward-incompatible
> change.  I don't think it's worth breaking existing scripts.  Probably
> what is needed is a new parseopt special character with the new
> semantics (e.g.,
> 
> 	Use ?? to mean the option has an optional argument.  If the
> 	option is supplied without its argument, the argument is taken
> 	to be ''.
> 
> or something like
> 
> 	Use ?<default> to mean the option has an optional argument.  If
> 	the option is supplied without its argument and <default> is
> 	nonempty, the argument is taken to be <default>.
> 
> ).
> 
> Sensible?

Yes, I think it's sensible. I will look at this and propose an other
patch. Thanks.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 23:33   ` Junio C Hamano
@ 2013-10-15 23:57     ` Jonathan Nieder
  2013-10-16  7:04       ` Johannes Sixt
  2013-10-16 14:14       ` Nicolas Vigier
  0 siblings, 2 replies; 25+ messages in thread
From: Jonathan Nieder @ 2013-10-15 23:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Nicolas Vigier, git, Pierre Habouzit

Junio C Hamano wrote:

> You just made these two that the user clearly meant to express two
> different things indistinguishable.
>
> 	opt.sh -S
>       opt.sh -S ''
[...]
> And that is exactly why gitcli.txt tells users to use the 'sticked'
> form, and ends the bullet point with:
>
>    An option that takes optional option-argument must be written in
>    the 'sticked' form.

Yes, another possibility in that vein would be to teach rev-parse
--parseopt an OPTIONS_LONG_STICKED output format, and then parse with

	while :
	do
		case $1 in
		--gpg-sign)
			... no keyid ...
			;;
		--gpg-sign=*)
			keyid=${1#--gpg-sign=}
			...
			;;
		esac
		shift
	done

This still leaves

	opt.sh -S
	
and

	opt.sh -S''

indistinguishable.  Given what the shell passes to execve, I think
that's ok.

The analagous method without preferring long options could work almost
as well:

	while :
	do
		case $1 in
		-S)
			... no keyid ...
			;;
		-S?*)
			keyid=${1#-S}
			...
			;;
		esac
		shift
	done

but it mishandles "--gpg-sign=" with empty argument.

Jonathan

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 23:57     ` Jonathan Nieder
@ 2013-10-16  7:04       ` Johannes Sixt
  2013-10-16  8:53         ` Jeff King
  2013-10-16 10:58         ` Nicolas Vigier
  2013-10-16 14:14       ` Nicolas Vigier
  1 sibling, 2 replies; 25+ messages in thread
From: Johannes Sixt @ 2013-10-16  7:04 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, Nicolas Vigier, git, Pierre Habouzit

Am 10/16/2013 1:57, schrieb Jonathan Nieder:
> Junio C Hamano wrote:
> 
>> You just made these two that the user clearly meant to express two
>> different things indistinguishable.
>>
>> 	opt.sh -S
>>       opt.sh -S ''
> [...]
>> And that is exactly why gitcli.txt tells users to use the 'sticked'
>> form, and ends the bullet point with:
>>
>>    An option that takes optional option-argument must be written in
>>    the 'sticked' form.
> 
> Yes, another possibility in that vein would be to teach rev-parse
> --parseopt an OPTIONS_LONG_STICKED output format, and then parse with

Aren't you people trying to solve something that can't besolved? What does

  git commit -S LICENSE

mean? Commit the index and sign with the key-id "LICENSE" or commit just
the file "LICENSE" with the default key-id?

-- Hannes

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-16  7:04       ` Johannes Sixt
@ 2013-10-16  8:53         ` Jeff King
  2013-10-16 21:40           ` Junio C Hamano
  2013-10-16 10:58         ` Nicolas Vigier
  1 sibling, 1 reply; 25+ messages in thread
From: Jeff King @ 2013-10-16  8:53 UTC (permalink / raw)
  To: Johannes Sixt
  Cc: Jonathan Nieder, Junio C Hamano, Nicolas Vigier, git,
	Pierre Habouzit

On Wed, Oct 16, 2013 at 09:04:32AM +0200, Johannes Sixt wrote:

> > Yes, another possibility in that vein would be to teach rev-parse
> > --parseopt an OPTIONS_LONG_STICKED output format, and then parse with
> 
> Aren't you people trying to solve something that can't besolved? What does
> 
>   git commit -S LICENSE
> 
> mean? Commit the index and sign with the key-id "LICENSE" or commit just
> the file "LICENSE" with the default key-id?

It means the latter. Because the argument is optional, you must use the
"sticked" form:

  git commit -SLICENSE

If the caller does not know whether the argument is optional or not,
they should use the sticked form to be on the safe side.

The problem, as I understand it, arises from the fact that shell scripts
can use "git rev-parse --parseopt" to check and normalize their
arguments. So for example:

  # pretend we got "-bs" on our command line
  set -- -bs

  git rev-parse --parseopt -- "$@" <<\EOF
  usage...
  --
  b!    the "b" option
  s!    the "s" option
  EOF

would produce:

  set -- -b -s --

The latter is much easier to parse in the shell, because options are
split, it ends with "--", etc. But what is the normalized form for an
optional argument? It either needs to be consistently "sticked" or
"unsticked", either:

  set -- -S '' --     ;# default
  set -- -S 'foo' --  ;# not default

or

  set -- -S --    ;# default
  set -- -Sfoo -- ;# not default

so that reading the normalized form is unambiguous.

-Peff

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-16  7:04       ` Johannes Sixt
  2013-10-16  8:53         ` Jeff King
@ 2013-10-16 10:58         ` Nicolas Vigier
  1 sibling, 0 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-16 10:58 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Jonathan Nieder, Junio C Hamano, git, Pierre Habouzit

On Wed, 16 Oct 2013, Johannes Sixt wrote:

> Am 10/16/2013 1:57, schrieb Jonathan Nieder:
> > Junio C Hamano wrote:
> > 
> >> You just made these two that the user clearly meant to express two
> >> different things indistinguishable.
> >>
> >> 	opt.sh -S
> >>       opt.sh -S ''
> > [...]
> >> And that is exactly why gitcli.txt tells users to use the 'sticked'
> >> form, and ends the bullet point with:
> >>
> >>    An option that takes optional option-argument must be written in
> >>    the 'sticked' form.
> > 
> > Yes, another possibility in that vein would be to teach rev-parse
> > --parseopt an OPTIONS_LONG_STICKED output format, and then parse with
> 
> Aren't you people trying to solve something that can't besolved? What does
> 
>   git commit -S LICENSE
> 
> mean? Commit the index and sign with the key-id "LICENSE" or commit just
> the file "LICENSE" with the default key-id?

The later, as optional arguments needs to be sticked, but I think this
is not related to the problems with --parseopt.

Here is a summary the problems I think we have with --parseopt and
proposed solutions. This command makes it possible to parse arguments
with a loop like this :

  while test $# != 0
  do
        case "$1" in
        -q)
                GIT_QUIET=t ;;
        -S)
                # do something with $2
                # shift if you think $2 is an optional arg
                ;;
        --)
                shift; break ;;
        esac
        shift
  done
  # do something with the other arguments

And I think the problems with the '?' flag when using this kind of loop
are :

 - You don't know if $2 is the optional argument for -S, or the next option
 
 - You can't use '--' as an optional argument to -S, because you don't
   know if '--' is the optional argument to -S, or the separator between
   options and non options.

To fix this, solution 1) is to always include the optional argument in
$2, and set it to '' if it is not set. However this brings the problem
that you can't distinguish between unset argument and empty argument.
The following two become the same :
  ./opt.sh --gpg-id
  ./opt.sh --gpg-id=

Solution 2) is to use a flag like ?<default> as suggested by Jonathan.
Now you can distinguish between unset and empty args, but you can't
between <default> and unset, but this is probably not a big problem as
you can select <default> so that it is a value nobody would want to use.

Solution 3) is the OPTIONS_LONG_STICKED output format suggested by
Jonathan. I can't see any problem with this one.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-15 23:57     ` Jonathan Nieder
  2013-10-16  7:04       ` Johannes Sixt
@ 2013-10-16 14:14       ` Nicolas Vigier
  2013-10-16 22:33         ` Jonathan Nieder
  1 sibling, 1 reply; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-16 14:14 UTC (permalink / raw)
  To: Jonathan Nieder; +Cc: Junio C Hamano, git, Pierre Habouzit

On Tue, 15 Oct 2013, Jonathan Nieder wrote:

> Junio C Hamano wrote:
> 
> > You just made these two that the user clearly meant to express two
> > different things indistinguishable.
> >
> > 	opt.sh -S
> >       opt.sh -S ''
> [...]
> > And that is exactly why gitcli.txt tells users to use the 'sticked'
> > form, and ends the bullet point with:
> >
> >    An option that takes optional option-argument must be written in
> >    the 'sticked' form.
> 
> Yes, another possibility in that vein would be to teach rev-parse
> --parseopt an OPTIONS_LONG_STICKED output format, and then parse with
> 
> 	while :
> 	do
> 		case $1 in
> 		--gpg-sign)
> 			... no keyid ...
> 			;;
> 		--gpg-sign=*)
> 			keyid=${1#--gpg-sign=}
> 			...
> 			;;
> 		esac
> 		shift
> 	done
> 
> This still leaves
> 
> 	opt.sh -S
> 	
> and
> 
> 	opt.sh -S''
> 
> indistinguishable.  Given what the shell passes to execve, I think
> that's ok.
> 
> The analagous method without preferring long options could work almost
> as well:
> 
> 	while :
> 	do
> 		case $1 in
> 		-S)
> 			... no keyid ...
> 			;;
> 		-S?*)
> 			keyid=${1#-S}
> 			...
> 			;;
> 		esac
> 		shift
> 	done
> 
> but it mishandles "--gpg-sign=" with empty argument.

I'm thinking about a patch to add the following two options to rev-parse :

--sticked-opt-args::
	Only meaningful in --parseopt mode. Tells the options parser to
	output options with optional arguments in sticked form. The
	default is to output them in non-sticked mode, which can be
	difficult to parse unambiguously.

--long-options::
	Only meaningful in --parseopt mode. Tells the options parser to
	output long option names, when available. The default is to use
	short option names when available.


When you want to handle optional args unambiguously, you use the
--sticked-opt-args option. And if you think an empty value can be
a meaningful value, you add the --long-options option to be able to
distinguish them.

Would it make sense ?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-16  8:53         ` Jeff King
@ 2013-10-16 21:40           ` Junio C Hamano
  2013-10-16 21:50             ` Jeff King
  0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2013-10-16 21:40 UTC (permalink / raw)
  To: Jeff King
  Cc: Johannes Sixt, Jonathan Nieder, Nicolas Vigier, git,
	Pierre Habouzit

Jeff King <peff@peff.net> writes:

> ... But what is the normalized form for an
> optional argument? It either needs to be consistently "sticked" or
> "unsticked", either:
>
>   set -- -S '' --     ;# default
>   set -- -S 'foo' --  ;# not default
>
> or
>
>   set -- -S --    ;# default
>   set -- -Sfoo -- ;# not default
>
> so that reading the normalized form is unambiguous.

The analysis makes sense.  Either form do not let you distinguish
between the case where the end user wanted to explicitly pass "" as
the optional parameter to -S and the case where she gave -S without
any optional parameter, though.

Which pretty much agrees with j6t's (and my earlier) comment that
there is no way to solve this issue completely, I think.

It is an acceptable compromise to use your suggestion as a solution
that works for cases other than passing an explicit empty string as
an optional parameter, I would say, if the limitation is clearly
documented.

Thanks.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-16 21:40           ` Junio C Hamano
@ 2013-10-16 21:50             ` Jeff King
  0 siblings, 0 replies; 25+ messages in thread
From: Jeff King @ 2013-10-16 21:50 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Johannes Sixt, Jonathan Nieder, Nicolas Vigier, git,
	Pierre Habouzit

On Wed, Oct 16, 2013 at 02:40:07PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... But what is the normalized form for an
> > optional argument? It either needs to be consistently "sticked" or
> > "unsticked", either:
> >
> >   set -- -S '' --     ;# default
> >   set -- -S 'foo' --  ;# not default
> >
> > or
> >
> >   set -- -S --    ;# default
> >   set -- -Sfoo -- ;# not default
> >
> > so that reading the normalized form is unambiguous.
> 
> The analysis makes sense.  Either form do not let you distinguish
> between the case where the end user wanted to explicitly pass "" as
> the optional parameter to -S and the case where she gave -S without
> any optional parameter, though.

I almost mentioned that, but I am not sure that it matters. Keep in mind
that:

  git my-script -S foo

already does not involve "foo" with S, because it is not "sticked". So
there is no way for the _user_ to distinguish between "I want the
default" and "I passed you an empty string"; because the argument must
be sticked they both look like "-S". And that distinction is already
impossible in the definition of optional arguments, and is not a problem
with going through the "git rev-parse --parseopt" channel at all.

So the only bug is the ambiguity in the current normalized form. Of the
two forms above, I think I prefer:

  set -- -S '' --

because it more closely matches the non-optional form we produce
today, and because it is slightly less work to parse (you can check that
$1 is "-S", and then store or check "$2", rather than having to match
"-S*" and parse off the beginning).

> Which pretty much agrees with j6t's (and my earlier) comment that
> there is no way to solve this issue completely, I think.

I guess it depends on what the issue is. :)

No, I do not think you can ever "fix" the options to let those two cases
be distinguishable. But I do not think anybody is really asking for
that; the real concern is that the "rev-parse --parseopt" normalization
is ambiguous, and that is easily fixable.

-Peff

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
  2013-10-16 14:14       ` Nicolas Vigier
@ 2013-10-16 22:33         ` Jonathan Nieder
  2013-10-25 20:18           ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Nicolas Vigier
  0 siblings, 1 reply; 25+ messages in thread
From: Jonathan Nieder @ 2013-10-16 22:33 UTC (permalink / raw)
  To: Nicolas Vigier; +Cc: Junio C Hamano, git, Pierre Habouzit

Nicolas Vigier wrote:

> I'm thinking about a patch to add the following two options to rev-parse :
>
> --sticked-opt-args::
> 	Only meaningful in --parseopt mode. Tells the options parser to
> 	output options with optional arguments in sticked form. The
> 	default is to output them in non-sticked mode, which can be
> 	difficult to parse unambiguously.
>
> --long-options::
> 	Only meaningful in --parseopt mode. Tells the options parser to
> 	output long option names, when available. The default is to use
> 	short option names when available.
>
> When you want to handle optional args unambiguously, you use the
> --sticked-opt-args option. And if you think an empty value can be
> a meaningful value, you add the --long-options option to be able to
> distinguish them.
>
> Would it make sense ?

That would make four distinct output formats:

 --sticked --long:
	Doesn't lose any information that normal use of C parse_options
	would have kept, as long as every short option with optional
	argument has a corresponding long option.


 --sticked --no-long:
	Loses the distinction between --gpg-sign and --gpg-sign=

 --no-sticked --long:
	Semantically equivalent to the existing output, just noisier.

 --no-sticked --no-long:
 	The existing output.

Are all of them needed?  Is it worth tempting people to use --sticked
--no-long when we know its pitfalls?

I would think that only the current normalized form and --sticked
--long would need to be supported.

The fix you originally proposed seems tolerable to me, too --- it is
not very invasive, and while it doesn't distinguish the empty-argument
form "--gpg-sign=", that's a bit of an edge case.

The main reason I slightly prefer the solution that makes the output
use long, sticked options on request is that the "normalized"
commandline would start being an actual equivalent command line the
command expects, instead of a weird, subtly different syntax.  (That
problem already exists with or without your patch --- the patch just
draws attention to it.)

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-16 22:33         ` Jonathan Nieder
@ 2013-10-25 20:18           ` Nicolas Vigier
  2013-10-25 22:01             ` Junio C Hamano
  2013-10-27  5:45             ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Michael Haggerty
  0 siblings, 2 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-25 20:18 UTC (permalink / raw)
  To: git; +Cc: Nicolas Vigier

Add the --sticked-long option to output the options in their long form
if available, and with their arguments sticked.

Contrary to the default form (non sticked arguments and short options),
this can be parsed unambiguously when using options with optional
arguments :

 - in the non sticked form, when an option is taking an optional argument
   you cannot know if the next argument is its optional argument, or the
   next option.

 - the long options form allows to differenciate between an empty argument
   '--option=' and an unset argument '--option', which is not possible
   with short options.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
---
 Documentation/git-rev-parse.txt |  8 +++++++-
 builtin/rev-parse.c             | 11 +++++++++--
 t/t1502-rev-parse-parseopt.sh   | 42 ++++++++++++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index d068a65..d3bad9d 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -50,6 +50,10 @@ Options for --parseopt
 	the first non-option argument.  This can be used to parse sub-commands
 	that take options themselves.
 
+--sticked-long::
+	Only meaningful in `--parseopt` mode. Output the options in their
+	long form if available, and with their arguments sticked.
+
 Options for Filtering
 ~~~~~~~~~~~~~~~~~~~~~
 
@@ -285,7 +289,9 @@ Each line of options has this format:
 	`<flags>` are of `*`, `=`, `?` or `!`.
 	* Use `=` if the option takes an argument.
 
-	* Use `?` to mean that the option is optional (though its use is discouraged).
+	* Use `?` to mean that the option takes an optional argument. You
+	  probably want to use the `--sticked-long` mode to be able to
+	  unambiguously parse the optional argument.
 
 	* Use `*` to mean that this option should not be listed in the usage
 	  generated for the `-h` argument. It's shown for `--help-all` as
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c76b89d..418b7f7 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -30,6 +30,8 @@ static int abbrev_ref;
 static int abbrev_ref_strict;
 static int output_sq;
 
+static int sticked_long;
+
 /*
  * Some arguments are relevant "revision" arguments,
  * others are about output format or other details.
@@ -320,12 +322,15 @@ 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->short_name)
+	else if (o->short_name && (o->long_name == NULL || !sticked_long))
 		strbuf_addf(parsed, " -%c", o->short_name);
 	else
 		strbuf_addf(parsed, " --%s", o->long_name);
 	if (arg) {
-		strbuf_addch(parsed, ' ');
+		if (!sticked_long)
+			strbuf_addch(parsed, ' ');
+		else if (o->long_name)
+			strbuf_addch(parsed, '=');
 		sq_quote_buf(parsed, arg);
 	}
 	return 0;
@@ -351,6 +356,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "stop-at-non-option", &stop_at_non_option,
 					N_("stop parsing after the "
 					   "first non-option argument")),
+		OPT_BOOL(0, "sticked-long", &sticked_long,
+					N_("output in sticked long form")),
 		OPT_END(),
 	};
 
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 13c88c9..7e12d9b 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -12,9 +12,11 @@ usage: some-command [options] <args>...
     -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
 
 Extras
     --extra1              line above used to cause a segfault but no longer does
@@ -31,9 +33,11 @@ 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
 
 Extras
 extra1    line above used to cause a segfault but no longer does
@@ -45,16 +49,16 @@ test_expect_success 'test --parseopt help output' '
 '
 
 cat > expect <<EOF
-set -- --foo --bar 'ham' -- 'arg'
+set -- --foo --bar 'ham' -b -- 'arg'
 EOF
 
 test_expect_success 'test --parseopt' '
-	git rev-parse --parseopt -- --foo --bar=ham arg < optionspec > output &&
+	git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > output &&
 	test_cmp expect output
 '
 
 test_expect_success 'test --parseopt with mixed options and arguments' '
-	git rev-parse --parseopt -- --foo arg --bar=ham < optionspec > output &&
+	git rev-parse --parseopt -- --foo arg --bar=ham --baz < optionspec > output &&
 	test_cmp expect output
 '
 
@@ -99,4 +103,36 @@ test_expect_success 'test --parseopt --keep-dashdash --stop-at-non-option withou
 	test_cmp expect output
 '
 
+cat > expect <<EOF
+set -- --foo --bar='z' --baz -C'Z' --data='A' -- 'arg'
+EOF
+
+test_expect_success 'test --parseopt --sticked-long' '
+	git rev-parse --parseopt --sticked-long -- --foo --bar=z -b arg -CZ -dA <optionspec >output &&
+	test_cmp expect output
+'
+
+cat > expect <<EOF
+set -- --data='' -C --baz -- 'arg'
+EOF
+
+test_expect_success 'test --parseopt --sticked-long and empty optional argument' '
+	git rev-parse --parseopt --sticked-long -- --data= arg -C -b <optionspec >output &&
+	test_cmp expect output
+'
+
+cat > expect <<EOF
+set -- --data --baz -- 'arg'
+EOF
+
+test_expect_success 'test --parseopt --sticked-long and long option with unset optional argument' '
+	git rev-parse --parseopt --sticked-long -- --data arg -b <optionspec >output &&
+	test_cmp expect output
+'
+
+test_expect_success 'test --parseopt --sticked-long and short option with unset optional argument' '
+	git rev-parse --parseopt --sticked-long -- -d arg -b <optionspec >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-25 20:18           ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Nicolas Vigier
@ 2013-10-25 22:01             ` Junio C Hamano
  2013-10-25 22:52               ` Nicolas Vigier
  2013-10-31 11:08               ` sticked -> stuck Nicolas Vigier
  2013-10-27  5:45             ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Michael Haggerty
  1 sibling, 2 replies; 25+ messages in thread
From: Junio C Hamano @ 2013-10-25 22:01 UTC (permalink / raw)
  To: Nicolas Vigier; +Cc: git

Nicolas Vigier <boklm@mars-attacks.org> writes:

> Add the --sticked-long option to output the options in their long form
> if available, and with their arguments sticked.

Hmph, doesn't verb "stick" conjugate to "(present) stick (past) stuck
(pp) stuck"?

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-25 22:01             ` Junio C Hamano
@ 2013-10-25 22:52               ` Nicolas Vigier
  2013-10-25 22:55                 ` Junio C Hamano
  2013-10-31 11:08               ` sticked -> stuck Nicolas Vigier
  1 sibling, 1 reply; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-25 22:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Fri, 25 Oct 2013, Junio C Hamano wrote:

> Nicolas Vigier <boklm@mars-attacks.org> writes:
> 
> > Add the --sticked-long option to output the options in their long form
> > if available, and with their arguments sticked.
> 
> Hmph, doesn't verb "stick" conjugate to "(present) stick (past) stuck
> (pp) stuck"?

Ah, yes it seems. I don't know if 'sticked' is also correct or not.

However, 'sticked' is the word that is used in Documentation/gitcli.txt
and Documentation/technical/api-parse-options.txt.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-25 22:52               ` Nicolas Vigier
@ 2013-10-25 22:55                 ` Junio C Hamano
  2013-10-26 21:55                   ` Philip Oakley
  0 siblings, 1 reply; 25+ messages in thread
From: Junio C Hamano @ 2013-10-25 22:55 UTC (permalink / raw)
  To: Nicolas Vigier; +Cc: git

Nicolas Vigier <boklm@mars-attacks.org> writes:

> On Fri, 25 Oct 2013, Junio C Hamano wrote:
>
>> Nicolas Vigier <boklm@mars-attacks.org> writes:
>> 
>> > Add the --sticked-long option to output the options in their long form
>> > if available, and with their arguments sticked.
>> 
>> Hmph, doesn't verb "stick" conjugate to "(present) stick (past) stuck
>> (pp) stuck"?
>
> Ah, yes it seems. I don't know if 'sticked' is also correct or not.
>
> However, 'sticked' is the word that is used in Documentation/gitcli.txt
> and Documentation/technical/api-parse-options.txt.

Yes, I know. That is why I brought it up before we inflict more
damage to us.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-25 22:55                 ` Junio C Hamano
@ 2013-10-26 21:55                   ` Philip Oakley
  2013-10-28 15:47                     ` Junio C Hamano
  0 siblings, 1 reply; 25+ messages in thread
From: Philip Oakley @ 2013-10-26 21:55 UTC (permalink / raw)
  To: Junio C Hamano, Nicolas Vigier; +Cc: git

From: "Junio C Hamano" <gitster@pobox.com>
> Nicolas Vigier <boklm@mars-attacks.org> writes:
>
>> On Fri, 25 Oct 2013, Junio C Hamano wrote:
>>
>>> Nicolas Vigier <boklm@mars-attacks.org> writes:
>>>
>>> > Add the --sticked-long option to output the options in their long 
>>> > form
>>> > if available, and with their arguments sticked.
>>>
>>> Hmph, doesn't verb "stick" conjugate to "(present) stick (past) 
>>> stuck
>>> (pp) stuck"?
>>
>> Ah, yes it seems. I don't know if 'sticked' is also correct or not.
>>
>> However, 'sticked' is the word that is used in 
>> Documentation/gitcli.txt
>> and Documentation/technical/api-parse-options.txt.
>
> Yes, I know. That is why I brought it up before we inflict more
> damage to us.
> --

Isn't the origin of the description that it looks like a stick (cane), 
and 'sticked' is a modern verbing of that form? That's what I'd assumed 
anyway.

Googleing "Sticked option" only linked back to Git.

Philip 

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-25 20:18           ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Nicolas Vigier
  2013-10-25 22:01             ` Junio C Hamano
@ 2013-10-27  5:45             ` Michael Haggerty
  1 sibling, 0 replies; 25+ messages in thread
From: Michael Haggerty @ 2013-10-27  5:45 UTC (permalink / raw)
  To: Nicolas Vigier, git

On 10/25/2013 10:18 PM, Nicolas Vigier wrote:
> Add the --sticked-long option to output the options in their long form
> if available, and with their arguments sticked.
> 
> Contrary to the default form (non sticked arguments and short options),
> this can be parsed unambiguously when using options with optional
> arguments :
> 
>  - in the non sticked form, when an option is taking an optional argument
>    you cannot know if the next argument is its optional argument, or the
>    next option.
> 
>  - the long options form allows to differenciate between an empty argument
>    '--option=' and an unset argument '--option', which is not possible
>    with short options.

Trivial: s/differenciate/differentiate/

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

^ permalink raw reply	[flat|nested] 25+ messages in thread

* Re: [PATCH] rev-parse --parseopt: add the --sticked-long mode
  2013-10-26 21:55                   ` Philip Oakley
@ 2013-10-28 15:47                     ` Junio C Hamano
  0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2013-10-28 15:47 UTC (permalink / raw)
  To: Philip Oakley; +Cc: Nicolas Vigier, git

"Philip Oakley" <philipoakley@iee.org> writes:

> Isn't the origin of the description that it looks like a stick (cane),
> and 'sticked' is a modern verbing of that form? That's what I'd
> assumed anyway.
>
> Googleing "Sticked option" only linked back to Git.

I know web is not the authoritative source of information ;-) but it
seems that looking for "sticked vs stuck" seems to give me many
explanations that boils down to what this says:

    http://en.wiktionary.org/wiki/Talk:sticked

and http://en.wiktionary.org/wiki/stick#English lists "sticked" as
"archaic" when the word is used for "To glue; to attach; to adhere".

^ permalink raw reply	[flat|nested] 25+ messages in thread

* sticked -> stuck
  2013-10-25 22:01             ` Junio C Hamano
  2013-10-25 22:52               ` Nicolas Vigier
@ 2013-10-31 11:08               ` Nicolas Vigier
  2013-10-31 11:08                 ` [PATCH 1/2] Use the word 'stuck' instead of 'sticked' Nicolas Vigier
  2013-10-31 11:08                 ` [PATCH 2/2] rev-parse --parseopt: add the --stuck-long mode Nicolas Vigier
  1 sibling, 2 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-31 11:08 UTC (permalink / raw)
  To: gitster, git

Here is a patch to replace the word 'sticked' with 'stuck' in existing
documentation. And the patch for nv/parseopt-opt-arg changed to use the
word 'stuck' too.

^ permalink raw reply	[flat|nested] 25+ messages in thread

* [PATCH 1/2] Use the word 'stuck' instead of 'sticked'
  2013-10-31 11:08               ` sticked -> stuck Nicolas Vigier
@ 2013-10-31 11:08                 ` Nicolas Vigier
  2013-10-31 19:35                   ` Junio C Hamano
  2013-10-31 11:08                 ` [PATCH 2/2] rev-parse --parseopt: add the --stuck-long mode Nicolas Vigier
  1 sibling, 1 reply; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-31 11:08 UTC (permalink / raw)
  To: gitster, git; +Cc: Nicolas Vigier

The past participle of 'stick' is 'stuck'.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
---
 Documentation/gitcli.txt                      | 6 +++---
 Documentation/technical/api-parse-options.txt | 6 +++---
 diff.c                                        | 2 +-
 diff.h                                        | 2 +-
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
index 3146413cce0d..c87f0ae88d6c 100644
--- a/Documentation/gitcli.txt
+++ b/Documentation/gitcli.txt
@@ -72,11 +72,11 @@ scripting Git:
  * splitting short options to separate words (prefer `git foo -a -b`
    to `git foo -ab`, the latter may not even work).
 
- * when a command line option takes an argument, use the 'sticked' form.  In
+ * when a command line option takes an argument, use the 'stuck' form.  In
    other words, write `git foo -oArg` instead of `git foo -o Arg` for short
    options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg`
    for long options.  An option that takes optional option-argument must be
-   written in the 'sticked' form.
+   written in the 'stuck' form.
 
  * when you give a revision parameter to a command, make sure the parameter is
    not ambiguous with a name of a file in the work tree.  E.g. do not write
@@ -165,7 +165,7 @@ $ git foo -o Arg
 ----------------------------
 
 However, this is *NOT* allowed for switches with an optional value, where the
-'sticked' form must be used:
+'stuck' form must be used:
 ----------------------------
 $ git describe --abbrev HEAD     # correct
 $ git describe --abbrev=10 HEAD  # correct
diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
index 0be2b5159f1b..be50cf4de35c 100644
--- a/Documentation/technical/api-parse-options.txt
+++ b/Documentation/technical/api-parse-options.txt
@@ -29,9 +29,9 @@ that allow to change the behavior of a command.
 
 The parse-options API allows:
 
-* 'sticked' and 'separate form' of options with arguments.
-  `-oArg` is sticked, `-o Arg` is separate form.
-  `--option=Arg` is sticked, `--option Arg` is separate form.
+* 'stuck' and 'separate form' of options with arguments.
+  `-oArg` is stuck, `-o Arg` is separate form.
+  `--option=Arg` is stuck, `--option Arg` is separate form.
 
 * Long options may be 'abbreviated', as long as the abbreviation
   is unambiguous.
diff --git a/diff.c b/diff.c
index e34bf971207f..3950e0191067 100644
--- a/diff.c
+++ b/diff.c
@@ -3394,7 +3394,7 @@ int parse_long_opt(const char *opt, const char **argv,
 	if (prefixcmp(arg, opt))
 		return 0;
 	arg += strlen(opt);
-	if (*arg == '=') { /* sticked form: --option=value */
+	if (*arg == '=') { /* stuck form: --option=value */
 		*optarg = arg + 1;
 		return 1;
 	}
diff --git a/diff.h b/diff.h
index e34232501ee8..1c05b3a6bec6 100644
--- a/diff.h
+++ b/diff.h
@@ -244,7 +244,7 @@ extern struct diff_filepair *diff_unmerge(struct diff_options *, const char *pat
 #define DIFF_SETUP_USE_SIZE_CACHE	4
 
 /*
- * Poor man's alternative to parse-option, to allow both sticked form
+ * Poor man's alternative to parse-option, to allow both stuck form
  * (--option=value) and separate form (--option value).
  */
 extern int parse_long_opt(const char *opt, const char **argv,
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* [PATCH 2/2] rev-parse --parseopt: add the --stuck-long mode
  2013-10-31 11:08               ` sticked -> stuck Nicolas Vigier
  2013-10-31 11:08                 ` [PATCH 1/2] Use the word 'stuck' instead of 'sticked' Nicolas Vigier
@ 2013-10-31 11:08                 ` Nicolas Vigier
  1 sibling, 0 replies; 25+ messages in thread
From: Nicolas Vigier @ 2013-10-31 11:08 UTC (permalink / raw)
  To: gitster, git; +Cc: Nicolas Vigier

Add the --stuck-long option to output the options in their long form
if available, and with their arguments stuck.

Contrary to the default form (non stuck arguments and short options),
this can be parsed unambiguously when using options with optional
arguments :

 - in the non stuck form, when an option is taking an optional argument
   you cannot know if the next argument is its optional argument, or the
   next option.

 - the long options form allows to differentiate between an empty argument
   '--option=' and an unset argument '--option', which is not possible
   with short options.

Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
---
 Documentation/git-rev-parse.txt |  8 +++++++-
 builtin/rev-parse.c             | 11 +++++++++--
 t/t1502-rev-parse-parseopt.sh   | 42 ++++++++++++++++++++++++++++++++++++++---
 3 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index d068a653778d..a436b24cc406 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -50,6 +50,10 @@ Options for --parseopt
 	the first non-option argument.  This can be used to parse sub-commands
 	that take options themselves.
 
+--stuck-long::
+	Only meaningful in `--parseopt` mode. Output the options in their
+	long form if available, and with their arguments stuck.
+
 Options for Filtering
 ~~~~~~~~~~~~~~~~~~~~~
 
@@ -285,7 +289,9 @@ Each line of options has this format:
 	`<flags>` are of `*`, `=`, `?` or `!`.
 	* Use `=` if the option takes an argument.
 
-	* Use `?` to mean that the option is optional (though its use is discouraged).
+	* Use `?` to mean that the option takes an optional argument. You
+	  probably want to use the `--stuck-long` mode to be able to
+	  unambiguously parse the optional argument.
 
 	* Use `*` to mean that this option should not be listed in the usage
 	  generated for the `-h` argument. It's shown for `--help-all` as
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index c76b89dc5bcc..3e8c4cce060e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -30,6 +30,8 @@ static int abbrev_ref;
 static int abbrev_ref_strict;
 static int output_sq;
 
+static int stuck_long;
+
 /*
  * Some arguments are relevant "revision" arguments,
  * others are about output format or other details.
@@ -320,12 +322,15 @@ 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->short_name)
+	else if (o->short_name && (o->long_name == NULL || !stuck_long))
 		strbuf_addf(parsed, " -%c", o->short_name);
 	else
 		strbuf_addf(parsed, " --%s", o->long_name);
 	if (arg) {
-		strbuf_addch(parsed, ' ');
+		if (!stuck_long)
+			strbuf_addch(parsed, ' ');
+		else if (o->long_name)
+			strbuf_addch(parsed, '=');
 		sq_quote_buf(parsed, arg);
 	}
 	return 0;
@@ -351,6 +356,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "stop-at-non-option", &stop_at_non_option,
 					N_("stop parsing after the "
 					   "first non-option argument")),
+		OPT_BOOL(0, "stuck-long", &stuck_long,
+					N_("output in stuck long form")),
 		OPT_END(),
 	};
 
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 13c88c9aae7f..83b1300cef91 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -12,9 +12,11 @@ usage: some-command [options] <args>...
     -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
 
 Extras
     --extra1              line above used to cause a segfault but no longer does
@@ -31,9 +33,11 @@ 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
 
 Extras
 extra1    line above used to cause a segfault but no longer does
@@ -45,16 +49,16 @@ test_expect_success 'test --parseopt help output' '
 '
 
 cat > expect <<EOF
-set -- --foo --bar 'ham' -- 'arg'
+set -- --foo --bar 'ham' -b -- 'arg'
 EOF
 
 test_expect_success 'test --parseopt' '
-	git rev-parse --parseopt -- --foo --bar=ham arg < optionspec > output &&
+	git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > output &&
 	test_cmp expect output
 '
 
 test_expect_success 'test --parseopt with mixed options and arguments' '
-	git rev-parse --parseopt -- --foo arg --bar=ham < optionspec > output &&
+	git rev-parse --parseopt -- --foo arg --bar=ham --baz < optionspec > output &&
 	test_cmp expect output
 '
 
@@ -99,4 +103,36 @@ test_expect_success 'test --parseopt --keep-dashdash --stop-at-non-option withou
 	test_cmp expect output
 '
 
+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
+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
+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 &&
+	test_cmp expect output
+'
+
+test_expect_success 'test --parseopt --stuck-long and short option with unset optional argument' '
+	git rev-parse --parseopt --stuck-long -- -d arg -b <optionspec >output &&
+	test_cmp expect output
+'
+
 test_done
-- 
1.8.4

^ permalink raw reply related	[flat|nested] 25+ messages in thread

* Re: [PATCH 1/2] Use the word 'stuck' instead of 'sticked'
  2013-10-31 11:08                 ` [PATCH 1/2] Use the word 'stuck' instead of 'sticked' Nicolas Vigier
@ 2013-10-31 19:35                   ` Junio C Hamano
  0 siblings, 0 replies; 25+ messages in thread
From: Junio C Hamano @ 2013-10-31 19:35 UTC (permalink / raw)
  To: Nicolas Vigier; +Cc: git

Nicolas Vigier <boklm@mars-attacks.org> writes:

> The past participle of 'stick' is 'stuck'.
>
> Signed-off-by: Nicolas Vigier <boklm@mars-attacks.org>
> ---

Thanks.

It was good that we caught this before introducing the option; the
documentation update does not hurt the users much, but if we unleash
a misspelled option in a release, it is much harder to correct it
later.

Will queue both patches.

>  Documentation/gitcli.txt                      | 6 +++---
>  Documentation/technical/api-parse-options.txt | 6 +++---
>  diff.c                                        | 2 +-
>  diff.h                                        | 2 +-
>  4 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/gitcli.txt b/Documentation/gitcli.txt
> index 3146413cce0d..c87f0ae88d6c 100644
> --- a/Documentation/gitcli.txt
> +++ b/Documentation/gitcli.txt
> @@ -72,11 +72,11 @@ scripting Git:
>   * splitting short options to separate words (prefer `git foo -a -b`
>     to `git foo -ab`, the latter may not even work).
>  
> - * when a command line option takes an argument, use the 'sticked' form.  In
> + * when a command line option takes an argument, use the 'stuck' form.  In
>     other words, write `git foo -oArg` instead of `git foo -o Arg` for short
>     options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg`
>     for long options.  An option that takes optional option-argument must be
> -   written in the 'sticked' form.
> +   written in the 'stuck' form.
>  
>   * when you give a revision parameter to a command, make sure the parameter is
>     not ambiguous with a name of a file in the work tree.  E.g. do not write
> @@ -165,7 +165,7 @@ $ git foo -o Arg
>  ----------------------------
>  
>  However, this is *NOT* allowed for switches with an optional value, where the
> -'sticked' form must be used:
> +'stuck' form must be used:
>  ----------------------------
>  $ git describe --abbrev HEAD     # correct
>  $ git describe --abbrev=10 HEAD  # correct
> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
> index 0be2b5159f1b..be50cf4de35c 100644
> --- a/Documentation/technical/api-parse-options.txt
> +++ b/Documentation/technical/api-parse-options.txt
> @@ -29,9 +29,9 @@ that allow to change the behavior of a command.
>  
>  The parse-options API allows:
>  
> -* 'sticked' and 'separate form' of options with arguments.
> -  `-oArg` is sticked, `-o Arg` is separate form.
> -  `--option=Arg` is sticked, `--option Arg` is separate form.
> +* 'stuck' and 'separate form' of options with arguments.
> +  `-oArg` is stuck, `-o Arg` is separate form.
> +  `--option=Arg` is stuck, `--option Arg` is separate form.
>  
>  * Long options may be 'abbreviated', as long as the abbreviation
>    is unambiguous.
> diff --git a/diff.c b/diff.c
> index e34bf971207f..3950e0191067 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3394,7 +3394,7 @@ int parse_long_opt(const char *opt, const char **argv,
>  	if (prefixcmp(arg, opt))
>  		return 0;
>  	arg += strlen(opt);
> -	if (*arg == '=') { /* sticked form: --option=value */
> +	if (*arg == '=') { /* stuck form: --option=value */
>  		*optarg = arg + 1;
>  		return 1;
>  	}
> diff --git a/diff.h b/diff.h
> index e34232501ee8..1c05b3a6bec6 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -244,7 +244,7 @@ extern struct diff_filepair *diff_unmerge(struct diff_options *, const char *pat
>  #define DIFF_SETUP_USE_SIZE_CACHE	4
>  
>  /*
> - * Poor man's alternative to parse-option, to allow both sticked form
> + * Poor man's alternative to parse-option, to allow both stuck form
>   * (--option=value) and separate form (--option value).
>   */
>  extern int parse_long_opt(const char *opt, const char **argv,

^ permalink raw reply	[flat|nested] 25+ messages in thread

end of thread, other threads:[~2013-10-31 19:35 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-15 12:00 [PATCH] rev-parse --parseopt: fix handling of optional arguments Nicolas Vigier
2013-10-15 22:55 ` Junio C Hamano
2013-10-15 23:47   ` Nicolas Vigier
2013-10-15 23:14 ` Jonathan Nieder
2013-10-15 23:33   ` Junio C Hamano
2013-10-15 23:57     ` Jonathan Nieder
2013-10-16  7:04       ` Johannes Sixt
2013-10-16  8:53         ` Jeff King
2013-10-16 21:40           ` Junio C Hamano
2013-10-16 21:50             ` Jeff King
2013-10-16 10:58         ` Nicolas Vigier
2013-10-16 14:14       ` Nicolas Vigier
2013-10-16 22:33         ` Jonathan Nieder
2013-10-25 20:18           ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Nicolas Vigier
2013-10-25 22:01             ` Junio C Hamano
2013-10-25 22:52               ` Nicolas Vigier
2013-10-25 22:55                 ` Junio C Hamano
2013-10-26 21:55                   ` Philip Oakley
2013-10-28 15:47                     ` Junio C Hamano
2013-10-31 11:08               ` sticked -> stuck Nicolas Vigier
2013-10-31 11:08                 ` [PATCH 1/2] Use the word 'stuck' instead of 'sticked' Nicolas Vigier
2013-10-31 19:35                   ` Junio C Hamano
2013-10-31 11:08                 ` [PATCH 2/2] rev-parse --parseopt: add the --stuck-long mode Nicolas Vigier
2013-10-27  5:45             ` [PATCH] rev-parse --parseopt: add the --sticked-long mode Michael Haggerty
2013-10-15 23:53   ` [PATCH] rev-parse --parseopt: fix handling of optional arguments Nicolas Vigier

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).