how optparse can go horribly wrong

how optparse can go horribly wrong

[Shawn O. Pearce]

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

Re: how optparse can go horribly wrong

[Nicolas Sebrecht]

The 25/09/09, Shawn O. Pearce wrote:
> *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 ?)

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

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

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

Re: how optparse can go horribly wrong

[Sverre Rabbelier]

Heya,

On Sat, Sep 26, 2009 at 03:51, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:
> 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.

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

Re: how optparse can go horribly wrong

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Sat, Sep 26, 2009 at 03:51, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:
> > 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.
> 
> 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.

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

Re: how optparse can go horribly wrong

[Clemens Buchacher]

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

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

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

[PATCH 1/2] do not mangle short options which take arguments

[Clemens Buchacher]

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:
> 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?

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

diff --git a/parse-options.c b/parse-options.c
index a64a4d6..4f16f37 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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:
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index bbc821e..86eb350 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -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
diff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh
index 62fd65e..208a134 100755
--- a/t/t3701-add-interactive.sh
+++ b/t/t3701-add-interactive.sh
@@ -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

[PATCH 2/2] allow mangling short options which take integer arguments

[Clemens Buchacher]

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

diff --git a/parse-options.c b/parse-options.c
index 4f16f37..bfe01ee 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -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);
diff --git a/parse-options.h b/parse-options.h
index f295a2c..33ce529 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -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

Re: [PATCH 1/2] do not mangle short options which take arguments

[Johannes Schindelin]

Hi,

On Thu, 1 Oct 2009, Clemens Buchacher wrote:

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

Sweet!  And the patch looks good to me!

Ciao,
Dscho

Re: [PATCH 2/2] allow mangling short options which take integer arguments

[Johannes Schindelin]

Hi,

On Thu, 1 Oct 2009, Clemens Buchacher wrote:

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

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

Re: [PATCH 1/2] do not mangle short options which take arguments

[Jeff King]

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

> 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:
> > 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?
> 
> Here we go.

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

Re: [PATCH 1/2] do not mangle short options which take arguments

[Clemens Buchacher]

On Fri, Oct 02, 2009 at 02:11:59AM -0400, Jeff King 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.

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.

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

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.

> 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

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

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

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

Re: [PATCH 2/2] allow mangling short options which take integer arguments

[Clemens Buchacher]

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

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

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

Re: [PATCH 1/2] do not mangle short options which take arguments

[Paolo Bonzini]

On 10/02/2009 09:36 AM, Clemens Buchacher wrote:
> Well, an error message is a lot safer than executing something you did not
> intend.

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

Re: [PATCH 2/2] allow mangling short options which take integer arguments

[Jeff King]

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

> On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:
> 
> > And this patch looks even more straight-forward than 1/2, _but_... what 
> > about cases where there are short options that are digits?
> 
> 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.

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

Re: [PATCH 1/2] do not mangle short options which take arguments

[Jeff King]

On Fri, Oct 02, 2009 at 09:36:28AM +0200, Clemens Buchacher 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.

Thanks.

> Yes, that can happen. On the other hand, the "-ammend" typo actually did
> happen.

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

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

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.

> > 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
> 
> Well, an error message is a lot safer than executing something you did not
> intend.

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

Re: [PATCH 2/2] allow mangling short options which take integer arguments

[Clemens Buchacher]

On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:
> On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:
> 
> > On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:
> > 
> > > And this patch looks even more straight-forward than 1/2, _but_... what 
> > > about cases where there are short options that are digits?
> > 
> > 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.
> 
> 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:

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

Clemens

Re: [PATCH 2/2] allow mangling short options which take integer arguments

[Johannes Schindelin]

Hi,

On Fri, 2 Oct 2009, Clemens Buchacher wrote:

> On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:
> > On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:
> > 
> > > On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:
> > > 
> > > > And this patch looks even more straight-forward than 1/2, _but_... what 
> > > > about cases where there are short options that are digits?
> > > 
> > > 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.
> > 
> > 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:
> 
> Oh, I mistakenly thought Dscho was asking about options with 
> single-digit arguments. Thanks for clearing this up.

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

Ciao,
Dscho

Re: [PATCH 1/2] do not mangle short options which take arguments

[Johannes Schindelin]

Hi,

On Fri, 2 Oct 2009, Clemens Buchacher wrote:

> On Fri, Oct 02, 2009 at 02:11:59AM -0400, Jeff King wrote:
> 
> > 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.
> 
> 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.

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

diff --git a/git.c b/git.c
index 9883009..e7cb946 100644
--- a/git.c
+++ b/git.c
@@ -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

Re: [PATCH 1/2] do not mangle short options which take arguments

[Jeff King]

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

> > 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.
> 
> Something like this?
> [...]
> +		} else if (!strcmp(cmd, "--shoot-me-in-the-foot")) {
> +			warning ("Bang, bang!");

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

-Peff

Re: [PATCH 1/2] do not mangle short options which take arguments

[Johannes Schindelin]

Hi,

On Fri, 2 Oct 2009, Jeff King wrote:

> On Fri, Oct 02, 2009 at 10:42:36AM +0200, Johannes Schindelin wrote:
> 
> > > 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.
> > 
> > Something like this?
> > [...]
> > +		} else if (!strcmp(cmd, "--shoot-me-in-the-foot")) {
> > +			warning ("Bang, bang!");
> 
> Doh! Now I have to come up with a new joke patch for the GitTogether!

Don't make me envious :-(

Ciao,
Dscho

Re: [PATCH 2/2] allow mangling short options which take integer arguments

[Clemens Buchacher]

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

> On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:
> 
> > On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:
> > 
> > > And this patch looks even more straight-forward than 1/2, _but_... what 
> > > about cases where there are short options that are digits?
> > 
> > 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.
> 
> 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",

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.