* [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 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 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 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 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 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 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-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
* 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
* [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] 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: 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
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).