how optparse can go horribly wrong

*sigh*.  Someone just ran into this today:

  $ git commit -a -ammend
  [work ce38944] mend
   1 files changed, 2 insertions(+), 0 deletions(-)

Omit one - and include an extra 'm', and instead of --amend you have -a -m mend. Which isn't exactly what you wanted.

We do catch -amend with an error though:

  $ git commit -amend
  error: did you mean `--amend` (with two dashes ?)

I wonder, should the -m flag on commit not allow cuddling its value against the switch when its combined in short form with other switches?

The 25/09/09, Shawn O. Pearce wrote:

OTOH, this is a bit odd because a commit with the message "end" makes perfect sense for a "fast and crappy commit local workflow".

And we allow -ammend (with two 'm')

  $ git commit -ammend
  [next 101f014] mend
   1 files changed, 1 insertions(+), 0 deletions(-)
  $

Doing this only to -m flag would break consistency. That said, I don't have any opinion in disallowing the sticked form for _all_ short options.

Heya,

On Sat, Sep 26, 2009 at 03:51, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:

Perhaps instead disallow it for short options that do not take a one-symbol argument, that is -n4 makes a lot of sense to me, but -m"my commit message here" not so much.

Sverre Rabbelier <srabbelier@gmail.com> wrote:

-1 on that, because long, long, long ago when I worked on -m support for commit I remember insisting that -mfoo and -m foo should be the same. I often do `git commit -a -mwip` or something to save my branch state and come back later.

What I think we should do is not allow cuddling of short options when the final option takes more than 1 character worth of argument. Thus `git commit -a -mfoo` is OK, but `git commit -amfoo` is not.

On Sat, Sep 26, 2009 at 12:25:27PM -0700, Shawn O. Pearce wrote:

I also think that would be the most sensible solution.

A little experimentation with git-log reveals that it does not behave very well either.

 git log -n1asdf	-> asdf is ignored
 git log -pn1		-> error
 git log -p1		-> error
 git log -1p		-> p is ignored

So I think this should instead behave just like you described above.

 git log -n1asdf	-> error: unknown option(s) -asdf
 git log -pn1		-> git log -p -n1
 git log -p1		-> git log -p -n1
 git log -1p		-> git log -p -n1

Clemens

Instead of

  $ git commit -a -ammend
  [work ce38944] mend
   1 files changed, 2 insertions(+), 0 deletions(-)

we now get

  $ git commit -a -ammend
  error: switch `m' must not be mangled with other options
  usage: git commit [options] [--] <filepattern>...
  [...]

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---
On Fri, Sep 25, 2009 at 04:32:26PM -0700, Shawn O. Pearce wrote:

Here we go.

Clemens

 parse-options.c            |   16 ++++++++++++----
 t/t0040-parse-options.sh   |   12 ++++++++++++
 t/t3701-add-interactive.sh |    2 +-
 3 files changed, 25 insertions(+), 5 deletions(-)

This patch allows you to do things like for example

  $ git <cmd> -bn100

where -b is a boolean and -n is an integer option.

Signed-off-by: Clemens Buchacher <drizzd@aon.at>
---

 parse-options.c |    2 +-
 parse-options.h |    5 ++++-
 2 files changed, 5 insertions(+), 2 deletions(-)

Hi,

On Thu, 1 Oct 2009, Clemens Buchacher wrote:

And this patch looks even more straight-forward than 1/2, _but_... what about cases where there are short options that are digits?

Ciao, Dscho

On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:

Could you point me to one of those? I did not find any during my non-exhaustive search. We should be able to handle them easily by adding PARSE_OPT_MANY.

Clemens

On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:

The one that comes readily to mind is "git log -1", but that is actually parsed by the revision options parser, which doesn't use parseopt. But there are a few done by parseopt:

  $ git grep "OPT_.*'[0-9]'"
  archive.c:              OPT__COMPR('1', &compression_level, "compress faster", 1),
  archive.c:              OPT__COMPR_HIDDEN('2', &compression_level, 2),
  archive.c:              OPT__COMPR_HIDDEN('3', &compression_level, 3),
  archive.c:              OPT__COMPR_HIDDEN('4', &compression_level, 4),
  archive.c:              OPT__COMPR_HIDDEN('5', &compression_level, 5),
  archive.c:              OPT__COMPR_HIDDEN('6', &compression_level, 6),
  archive.c:              OPT__COMPR_HIDDEN('7', &compression_level, 7),
  archive.c:              OPT__COMPR_HIDDEN('8', &compression_level, 8),
  archive.c:              OPT__COMPR('9', &compression_level, "compress better", 9),
  builtin-checkout.c:             OPT_SET_INT('2', "ours", &opts.writeout_stage, "stage",
  builtin-checkout.c:             OPT_SET_INT('3', "theirs", &opts.writeout_stage, "stage",

-Peff

On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:

Oh, I mistakenly thought Dscho was asking about options with single-digit arguments. Thanks for clearing this up.

Clemens

Hi,

On Fri, 2 Oct 2009, Clemens Buchacher wrote:

I was actually thinking both of "git log -11" as well as out-of-tree users of parse-options.

Ciao, Dscho

On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:

Those are not affected by this patch series. They can be cuddled with other short options just like before, since they don't take arguments.

Hi,

On Thu, 1 Oct 2009, Clemens Buchacher wrote:

Sweet!  And the patch looks good to me!

Ciao, Dscho

On Thu, Oct 01, 2009 at 10:16:48PM +0200, Clemens Buchacher wrote:

I thought the proposal was to disallow just cuddling of the value when the switch was combined with others. So you would disallow "git commit -ammend" but it would still be legal to do "git commit -am foo". Your patch disallows the latter.

I would prefer to allow the uncuddled form, as it is something I do occasionally (and I don't think "git commit -am foo" looks very much like a typo).

To be honest, I am not sure that even the more restricted proposal is that good an idea. You are introducing a heuristic to guess at what is a typo or error from the user; when your guess is wrong, the user will be annoyed (doubly so if it is buried deep in a script, which this change will also impact). So you are guessing that people don't use the cuddled form in this way. I suspect most don't. But I also wonder how many typos you are really helping to save. This was brought up to make "-ammend" DWYM. Is that really that common a double-typo?

On the other hand, the cuddled value already has some DWYM magic (it recognizes -amend), so it is already a little bit unsafe to use (interestingly, though, the gitcli(7) documentation actually recommends using the "sticked" form over the separated one. However, it also recommends splitting your short options).

So I don't feel _too_ strongly. I am just concerned that we are introducing some DWYM magic that is not really going to help many people out, and is just going to make understanding the rules for option parsing more complex, and introduce the possibility (albeit slim) of breaking people's scripts and trained fingers.

-Peff

On Fri, Oct 02, 2009 at 02:11:59AM -0400, Jeff King wrote:

Yes, that syntax looks reasonable. I expect this to be more involved, so I will rework the patch once we agree on whether or not we want it at all.

Yes, that can happen. On the other hand, the "-ammend" typo actually did happen. And what I'm even more worried about are ambiguities like

  $ git commit -uno <path>
  $ git commit -nou <path>

which are interpreted as one of

  $ git commit --untracked-files=no <path>
  $ git commit --untracked-files --no-verify --only <path>

depending only on the order of the switches. I was actually surprised that I could find an example so easily. But the fact alone that it's possible feels like an accident waiting to happen.

Well, an error message is a lot safer than executing something you did not intend.

But it makes the rules simpler, because it removes ambiguities such as the one above.

Yes, we deliberately allow users to shoot themselves in the foot. But they should have to use at least a long option to do it.

Clemens

On 10/02/2009 09:36 AM, Clemens Buchacher wrote:

If this is just for double --amend typos I don't see the need. What you intended is just one "git rebase -i HEAD^^" away.

Notice that in Shawn's original example the guy actually passed -a too, so he would not even have the problem of overwriting the index due to -a.

If you have bouncy fingers, you can just make an alias

    ammend-fix = git reset --soft HEAD^ && git gui citool

Paolo

On Fri, Oct 02, 2009 at 09:36:28AM +0200, Clemens Buchacher wrote:

Thanks.

It did, but we are only guessing at how many people will be disrupted by the new rule. That being said...

Making this clearer is a much more compelling argument to me. Though I thought it was customary (not just for git, but for other programs) that a short option that takes a parameter (even an optional one) would consume the rest of a short options string. Still, it is a potential source of confusion.

It's also an error exit code, which can affect how a script performs (e.g., "git diff --exit-code"). But I don't have any real examples off the top of my head of how this could be particularly disastrous, so feel free to dismiss that as pushing too far into the hypothetical.

-Peff

Hi,

On Fri, 2 Oct 2009, Clemens Buchacher wrote:

Something like this?

-- snipsnap -- [PATCH] Deliberately allow users to shoot themselves in the foot

But they should have to use at least a long option to do it.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 git.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

On Fri, Oct 02, 2009 at 10:42:36AM +0200, Johannes Schindelin wrote:

Doh! Now I have to come up with a new joke patch for the GitTogether!

-Peff

Hi,

On Fri, 2 Oct 2009, Jeff King wrote:

Don't make me envious :-(

Ciao, Dscho