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?

Shawn.

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.

Nicolas Sebrecht

Heya,

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.

Cheers,

Sverre Rabbelier

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

Shawn.

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>...
  [...]

---

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

index a64a4d6..4f16f37 100644
@@ -5,6 +5,7 @@
 
 #define OPT_SHORT 1
 #define OPT_UNSET 2
+#define OPT_MANY 4
 
 static int opterror(const struct option *opt, const char *reason, int flags)
 {

@@ -43,9 +44,12 @@ static int get_value(struct parse_opt_ctx_t *p,
 		     const struct option *opt, int flags)
 {
 	const char *s, *arg;
+	const int many = flags & OPT_MANY;
 	const int unset = flags & OPT_UNSET;
 	int err;
 
+	if (many && !(opt->flags & PARSE_OPT_NOARG))
+		return opterror(opt, "must not be mangled with other options", flags);
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);
 	if (unset && (opt->flags & PARSE_OPT_NONEG))

@@ -149,14 +153,18 @@ static int get_value(struct parse_opt_ctx_t *p,
 	}
 }
 
-static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)
+static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option
+		*options, int many)
 {
 	const struct option *numopt = NULL;
+	int flags = OPT_SHORT;
+	if (many)
+		flags |= OPT_MANY;
 
 	for (; options->type != OPTION_END; options++) {
 		if (options->short_name == *p->opt) {
 			p->opt = p->opt[1] ? p->opt + 1 : NULL;
-			return get_value(p, options, OPT_SHORT);
+			return get_value(p, options, flags);
 		}
 
 		/*

@@ -374,7 +382,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			ctx->opt = arg + 1;
 			if (internal_help && *ctx->opt == 'h')
 				return parse_options_usage(usagestr, options);
-			switch (parse_short_opt(ctx, options)) {
+			switch (parse_short_opt(ctx, options, 0)) {
 			case -1:
 				return parse_options_usage(usagestr, options);
 			case -2:

@@ -385,7 +393,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,
 			while (ctx->opt) {
 				if (internal_help && *ctx->opt == 'h')
 					return parse_options_usage(usagestr, options);
-				switch (parse_short_opt(ctx, options)) {
+				switch (parse_short_opt(ctx, options, 1)) {
 				case -1:
 					return parse_options_usage(usagestr, options);
 				case -2:

index bbc821e..86eb350 100755
@@ -315,4 +315,16 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '
 	test_cmp expect output
 '
 
+cat > mangle.err << EOF
+error: switch \`s' must not be mangled with other options
+EOF
+
+cat mangle.err expect.err > expect-mangle.err
+
+test_expect_success 'do not mangle options which require arguments' '
+	test_must_fail test-parse-options -bs123 > output 2> output.err &&
+	! test -s output &&
+	test_cmp expect-mangle.err output.err
+'
+
 test_done

index 62fd65e..208a134 100755
@@ -207,7 +207,7 @@ index b6f2c08..61b9053 100755
 EOF
 # Test splitting the first patch, then adding both
 test_expect_success 'add first line works' '
-	git commit -am "clear local changes" &&
+	git commit -a -m "clear local changes" &&
 	git apply patch &&
 	(echo s; echo y; echo y) | git add -p file &&
 	git diff --cached > diff &&

1.6.5.rc1.214.g13c5a

This patch allows you to do things like for example

  $ git <cmd> -bn100

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

---

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

index 4f16f37..bfe01ee 100644
@@ -48,7 +48,7 @@ static int get_value(struct parse_opt_ctx_t *p,
 	const int unset = flags & OPT_UNSET;
 	int err;
 
-	if (many && !(opt->flags & PARSE_OPT_NOARG))
+	if (many && !(opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_MANY)))
 		return opterror(opt, "must not be mangled with other options", flags);
 	if (unset && p->opt)
 		return opterror(opt, "takes no value", flags);

index f295a2c..33ce529 100644
@@ -37,6 +37,7 @@ enum parse_opt_option_flags {
 	PARSE_OPT_NODASH = 32,
 	PARSE_OPT_LITERAL_ARGHELP = 64,
 	PARSE_OPT_NEGHELP = 128,
+	PARSE_OPT_MANY = 256,
 };
 
 struct option;

@@ -84,6 +85,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);
  *   PARSE_OPT_NEGHELP: says that the long option should always be shown with
  *				the --no prefix in the usage message. Sometimes
  *				useful for users of OPTION_NEGBIT.
+ *   PARSE_OPT_MANY: the short option may be mangled, despite a possible
+ *				argument.
  *
  * `callback`::
  *   pointer to the callback to use for OPTION_CALLBACK.

@@ -121,7 +124,7 @@ struct option {
 				      (h), PARSE_OPT_NOARG, NULL, (i) }
 #define OPT_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, \
 				      (h), PARSE_OPT_NOARG, NULL, (p) }
-#define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), "n", (h) }
+#define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), "n", (h), PARSE_OPT_MANY }
 #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }
 #define OPT_DATE(s, l, v, h) \
 	{ OPTION_CALLBACK, (s), (l), (v), "time",(h), 0, \

1.6.5.rc1.214.g13c5a

Hi,

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

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

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

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

Clemens

Hi,

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

Ciao,
Dscho

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.

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

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

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

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

Hi,

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.

---
 git.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

index 9883009..e7cb946 100644
@@ -122,6 +122,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--shoot-me-in-the-foot")) {
+			warning ("Bang, bang!");
 		} else {
 			fprintf(stderr, "Unknown option: %s\n", cmd);
 			usage(git_usage_string);

1.6.4.297.gcb4cc

Hi,

Don't make me envious :-(

Ciao,
Dscho

Hi,

Sweet! And the patch looks good to me!

Ciao,
Dscho