Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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.) -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
On Wed, Oct 16, 2013 at 02:40:07PM -0700, Junio C Hamano wrote: > Jeff King 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
Jeff King 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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 ?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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 ? as suggested by Jonathan. Now you can distinguish between unset and empty args, but you can't between and unset, but this is probably not a big problem as you can select 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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 ? to mean the option has an optional argument. If > the option is supplied without its argument and is > nonempty, the argument is taken to be . > > ). > > Sensible? Yes, I think it's sensible. I will look at this and propose an other patch. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
On Tue, 15 Oct 2013, Junio C Hamano wrote: > Nicolas Vigier 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
Jonathan Nieder 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 ? to mean the option has an optional argument. If > the option is supplied without its argument and is > nonempty, the argument is taken to be . > > ). > > 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
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 ? to mean the option has an optional argument. If the option is supplied without its argument and is nonempty, the argument is taken to be . ). Sensible? Thanks, Jonathan -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse --parseopt: fix handling of optional arguments
Nicolas Vigier 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? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] rev-parse --parseopt: fix handling of optional arguments
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
---
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
