{"thread":{"id":"21061","subject":"how optparse can go horribly wrong","startedAt":"2009-09-25T23:32:26Z","lastAt":"2009-10-03T09:23:32Z","messageCount":21,"participants":["Shawn O. Pearce","Nicolas Sebrecht","Sverre Rabbelier","Clemens Buchacher","Johannes Schindelin","Jeff King","Paolo Bonzini"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"123824","messageId":"20090925233226.GC14660@spearce.org","threadId":"21061","inReplyTo":null,"subject":"how optparse can go horribly wrong","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-09-25T23:32:26Z","receivedAt":"2009-09-25T23:32:26Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"*sigh*.  Someone just ran into this today:\n\n  $ git commit -a -ammend\n  [work ce38944] mend\n   1 files changed, 2 insertions(+), 0 deletions(-)\n\nOmit one - and include an extra 'm', and instead of --amend you\nhave -a -m mend.  Which isn't exactly what you wanted.\n\nWe do catch -amend with an error though:\n\n  $ git commit -amend\n  error: did you mean `--amend` (with two dashes ?)\n\nI wonder, should the -m flag on commit not allow cuddling its\nvalue against the switch when its combined in short form with\nother switches?\n\n-- \nShawn.\n"},{"id":"123830","messageId":"20090926015127.GA12994@vidovic","threadId":"21061","inReplyTo":"20090925233226.GC14660@spearce.org","subject":"Re: how optparse can go horribly wrong","fromName":"Nicolas Sebrecht","fromEmail":"nicolas.s.dev@gmx.fr","sentAt":"2009-09-26T01:51:27Z","receivedAt":"2009-09-26T01:51:27Z","isPatch":false,"sender":{"key":"nicolas.s.dev@gmx.fr","avatar":null},"body":"The 25/09/09, Shawn O. Pearce wrote:\n> *sigh*.  Someone just ran into this today:\n> \n>   $ git commit -a -ammend\n>   [work ce38944] mend\n>    1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> Omit one - and include an extra 'm', and instead of --amend you\n> have -a -m mend.  Which isn't exactly what you wanted.\n> \n> We do catch -amend with an error though:\n> \n>   $ git commit -amend\n>   error: did you mean `--amend` (with two dashes ?)\n\nOTOH, this is a bit odd because a commit with the message \"end\" makes\nperfect sense for a \"fast and crappy commit local workflow\".\n\nAnd we allow -ammend (with two 'm')\n\n  $ git commit -ammend\n  [next 101f014] mend\n   1 files changed, 1 insertions(+), 0 deletions(-)\n  $\n\n> I wonder, should the -m flag on commit not allow cuddling its\n> value against the switch when its combined in short form with\n> other switches?\n\nDoing this only to -m flag would break consistency. That said, I don't\nhave any opinion in disallowing the sticked form for _all_ short\noptions.\n\n-- \nNicolas Sebrecht\n"},{"id":"123844","messageId":"fabb9a1e0909260644w781d3c3h4fca22e3b7d97768@mail.gmail.com","threadId":"21061","inReplyTo":"20090926015127.GA12994@vidovic","subject":"Re: how optparse can go horribly wrong","fromName":"Sverre Rabbelier","fromEmail":"srabbelier@gmail.com","sentAt":"2009-09-26T13:44:01Z","receivedAt":"2009-09-26T13:44:01Z","isPatch":false,"sender":{"key":"srabbelier@gmail.com","avatar":"https://avatars.githubusercontent.com/u/3098?v=4"},"body":"Heya,\n\nOn Sat, Sep 26, 2009 at 03:51, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:\n> Doing this only to -m flag would break consistency. That said, I don't\n> have any opinion in disallowing the sticked form for _all_ short\n> options.\n\nPerhaps instead disallow it for short options that do not take a\none-symbol argument, that is -n4 makes a lot of sense to me, but -m\"my\ncommit message here\" not so much.\n\n\n-- \nCheers,\n\nSverre Rabbelier\n"},{"id":"123857","messageId":"20090926192527.GF14660@spearce.org","threadId":"21061","inReplyTo":"fabb9a1e0909260644w781d3c3h4fca22e3b7d97768@mail.gmail.com","subject":"Re: how optparse can go horribly wrong","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-09-26T19:25:27Z","receivedAt":"2009-09-26T19:25:27Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Sverre Rabbelier <srabbelier@gmail.com> wrote:\n> On Sat, Sep 26, 2009 at 03:51, Nicolas Sebrecht <nicolas.s.dev@gmx.fr> wrote:\n> > Doing this only to -m flag would break consistency. That said, I don't\n> > have any opinion in disallowing the sticked form for _all_ short\n> > options.\n> \n> Perhaps instead disallow it for short options that do not take a\n> one-symbol argument, that is -n4 makes a lot of sense to me, but -m\"my\n> commit message here\" not so much.\n\n-1 on that, because long, long, long ago when I worked on -m support\nfor commit I remember insisting that -mfoo and -m foo should be\nthe same.  I often do `git commit -a -mwip` or something to save my\nbranch state and come back later.\n\nWhat I think we should do is not allow cuddling of short options\nwhen the final option takes more than 1 character worth of argument.\nThus `git commit -a -mfoo` is OK, but `git commit -amfoo` is not.\n\n-- \nShawn.\n"},{"id":"123949","messageId":"20090928133707.GA3343@localhost","threadId":"21061","inReplyTo":"20090926192527.GF14660@spearce.org","subject":"Re: how optparse can go horribly wrong","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-09-28T13:37:07Z","receivedAt":"2009-09-28T13:37:07Z","isPatch":false,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Sat, Sep 26, 2009 at 12:25:27PM -0700, Shawn O. Pearce wrote:\n\n> What I think we should do is not allow cuddling of short options\n> when the final option takes more than 1 character worth of argument.\n> Thus `git commit -a -mfoo` is OK, but `git commit -amfoo` is not.\n\nI also think that would be the most sensible solution.\n\nA little experimentation with git-log reveals that it does not behave very\nwell either.\n\n git log -n1asdf\t-> asdf is ignored\n git log -pn1\t\t-> error\n git log -p1\t\t-> error\n git log -1p\t\t-> p is ignored\n\nSo I think this should instead behave just like you described above.\n\n git log -n1asdf\t-> error: unknown option(s) -asdf\n git log -pn1\t\t-> git log -p -n1\n git log -p1\t\t-> git log -p -n1\n git log -1p\t\t-> git log -p -n1\n\nClemens\n"},{"id":"124097","messageId":"20091001201648.GA12175@localhost","threadId":"21061","inReplyTo":"20090925233226.GC14660@spearce.org","subject":"[PATCH 1/2] do not mangle short options which take arguments","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-10-01T20:16:48Z","receivedAt":"2009-10-01T20:16:48Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Instead of\n\n  $ git commit -a -ammend\n  [work ce38944] mend\n   1 files changed, 2 insertions(+), 0 deletions(-)\n\nwe now get\n\n  $ git commit -a -ammend\n  error: switch `m' must not be mangled with other options\n  usage: git commit [options] [--] <filepattern>...\n  [...]\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\nOn Fri, Sep 25, 2009 at 04:32:26PM -0700, Shawn O. Pearce wrote:\n> I wonder, should the -m flag on commit not allow cuddling its\n> value against the switch when its combined in short form with\n> other switches?\n\nHere we go.\n\nClemens\n\n parse-options.c            |   16 ++++++++++++----\n t/t0040-parse-options.sh   |   12 ++++++++++++\n t/t3701-add-interactive.sh |    2 +-\n 3 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex a64a4d6..4f16f37 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -5,6 +5,7 @@\n \n #define OPT_SHORT 1\n #define OPT_UNSET 2\n+#define OPT_MANY 4\n \n static int opterror(const struct option *opt, const char *reason, int flags)\n {\n@@ -43,9 +44,12 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t\t     const struct option *opt, int flags)\n {\n \tconst char *s, *arg;\n+\tconst int many = flags & OPT_MANY;\n \tconst int unset = flags & OPT_UNSET;\n \tint err;\n \n+\tif (many && !(opt->flags & PARSE_OPT_NOARG))\n+\t\treturn opterror(opt, \"must not be mangled with other options\", flags);\n \tif (unset && p->opt)\n \t\treturn opterror(opt, \"takes no value\", flags);\n \tif (unset && (opt->flags & PARSE_OPT_NONEG))\n@@ -149,14 +153,18 @@ static int get_value(struct parse_opt_ctx_t *p,\n \t}\n }\n \n-static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option *options)\n+static int parse_short_opt(struct parse_opt_ctx_t *p, const struct option\n+\t\t*options, int many)\n {\n \tconst struct option *numopt = NULL;\n+\tint flags = OPT_SHORT;\n+\tif (many)\n+\t\tflags |= OPT_MANY;\n \n \tfor (; options->type != OPTION_END; options++) {\n \t\tif (options->short_name == *p->opt) {\n \t\t\tp->opt = p->opt[1] ? p->opt + 1 : NULL;\n-\t\t\treturn get_value(p, options, OPT_SHORT);\n+\t\t\treturn get_value(p, options, flags);\n \t\t}\n \n \t\t/*\n@@ -374,7 +382,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\tctx->opt = arg + 1;\n \t\t\tif (internal_help && *ctx->opt == 'h')\n \t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\tswitch (parse_short_opt(ctx, options)) {\n+\t\t\tswitch (parse_short_opt(ctx, options, 0)) {\n \t\t\tcase -1:\n \t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\tcase -2:\n@@ -385,7 +393,7 @@ int parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\twhile (ctx->opt) {\n \t\t\t\tif (internal_help && *ctx->opt == 'h')\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n-\t\t\t\tswitch (parse_short_opt(ctx, options)) {\n+\t\t\t\tswitch (parse_short_opt(ctx, options, 1)) {\n \t\t\t\tcase -1:\n \t\t\t\t\treturn parse_options_usage(usagestr, options);\n \t\t\t\tcase -2:\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex bbc821e..86eb350 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -315,4 +315,16 @@ test_expect_success 'OPT_NUMBER_CALLBACK() works' '\n \ttest_cmp expect output\n '\n \n+cat > mangle.err << EOF\n+error: switch \\`s' must not be mangled with other options\n+EOF\n+\n+cat mangle.err expect.err > expect-mangle.err\n+\n+test_expect_success 'do not mangle options which require arguments' '\n+\ttest_must_fail test-parse-options -bs123 > output 2> output.err &&\n+\t! test -s output &&\n+\ttest_cmp expect-mangle.err output.err\n+'\n+\n test_done\ndiff --git a/t/t3701-add-interactive.sh b/t/t3701-add-interactive.sh\nindex 62fd65e..208a134 100755\n--- a/t/t3701-add-interactive.sh\n+++ b/t/t3701-add-interactive.sh\n@@ -207,7 +207,7 @@ index b6f2c08..61b9053 100755\n EOF\n # Test splitting the first patch, then adding both\n test_expect_success 'add first line works' '\n-\tgit commit -am \"clear local changes\" &&\n+\tgit commit -a -m \"clear local changes\" &&\n \tgit apply patch &&\n \t(echo s; echo y; echo y) | git add -p file &&\n \tgit diff --cached > diff &&\n-- \n1.6.5.rc1.214.g13c5a\n"},{"id":"124098","messageId":"20091001202301.GB12175@localhost","threadId":"21061","inReplyTo":"20091001201648.GA12175@localhost","subject":"[PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-10-01T20:23:01Z","receivedAt":"2009-10-01T20:23:01Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"This patch allows you to do things like for example\n\n  $ git <cmd> -bn100\n\nwhere -b is a boolean and -n is an integer option.\n\nSigned-off-by: Clemens Buchacher <drizzd@aon.at>\n---\n\n parse-options.c |    2 +-\n parse-options.h |    5 ++++-\n 2 files changed, 5 insertions(+), 2 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 4f16f37..bfe01ee 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -48,7 +48,7 @@ static int get_value(struct parse_opt_ctx_t *p,\n \tconst int unset = flags & OPT_UNSET;\n \tint err;\n \n-\tif (many && !(opt->flags & PARSE_OPT_NOARG))\n+\tif (many && !(opt->flags & (PARSE_OPT_NOARG | PARSE_OPT_MANY)))\n \t\treturn opterror(opt, \"must not be mangled with other options\", flags);\n \tif (unset && p->opt)\n \t\treturn opterror(opt, \"takes no value\", flags);\ndiff --git a/parse-options.h b/parse-options.h\nindex f295a2c..33ce529 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -37,6 +37,7 @@ enum parse_opt_option_flags {\n \tPARSE_OPT_NODASH = 32,\n \tPARSE_OPT_LITERAL_ARGHELP = 64,\n \tPARSE_OPT_NEGHELP = 128,\n+\tPARSE_OPT_MANY = 256,\n };\n \n struct option;\n@@ -84,6 +85,8 @@ typedef int parse_opt_cb(const struct option *, const char *arg, int unset);\n  *   PARSE_OPT_NEGHELP: says that the long option should always be shown with\n  *\t\t\t\tthe --no prefix in the usage message. Sometimes\n  *\t\t\t\tuseful for users of OPTION_NEGBIT.\n+ *   PARSE_OPT_MANY: the short option may be mangled, despite a possible\n+ *\t\t\t\targument.\n  *\n  * `callback`::\n  *   pointer to the callback to use for OPTION_CALLBACK.\n@@ -121,7 +124,7 @@ struct option {\n \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (i) }\n #define OPT_SET_PTR(s, l, v, h, p)  { OPTION_SET_PTR, (s), (l), (v), NULL, \\\n \t\t\t\t      (h), PARSE_OPT_NOARG, NULL, (p) }\n-#define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), \"n\", (h) }\n+#define OPT_INTEGER(s, l, v, h)     { OPTION_INTEGER, (s), (l), (v), \"n\", (h), PARSE_OPT_MANY }\n #define OPT_STRING(s, l, v, a, h)   { OPTION_STRING,  (s), (l), (v), (a), (h) }\n #define OPT_DATE(s, l, v, h) \\\n \t{ OPTION_CALLBACK, (s), (l), (v), \"time\",(h), 0, \\\n-- \n1.6.5.rc1.214.g13c5a\n"},{"id":"124102","messageId":"alpine.DEB.1.00.0910012353010.4985@pacific.mpi-cbg.de","threadId":"21061","inReplyTo":"20091001201648.GA12175@localhost","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-01T21:53:23Z","receivedAt":"2009-10-01T21:53:23Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 1 Oct 2009, Clemens Buchacher wrote:\n\n> Instead of\n> \n>   $ git commit -a -ammend\n>   [work ce38944] mend\n>    1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> we now get\n> \n>   $ git commit -a -ammend\n>   error: switch `m' must not be mangled with other options\n>   usage: git commit [options] [--] <filepattern>...\n>   [...]\n> \n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n\nSweet!  And the patch looks good to me!\n\nCiao,\nDscho\n"},{"id":"124103","messageId":"alpine.DEB.1.00.0910012354080.4985@pacific.mpi-cbg.de","threadId":"21061","inReplyTo":"20091001202301.GB12175@localhost","subject":"Re: [PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-01T21:55:03Z","receivedAt":"2009-10-01T21:55:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 1 Oct 2009, Clemens Buchacher wrote:\n\n> This patch allows you to do things like for example\n> \n>   $ git <cmd> -bn100\n> \n> where -b is a boolean and -n is an integer option.\n> \n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n\nAnd this patch looks even more straight-forward than 1/2, _but_... what \nabout cases where there are short options that are digits?\n\nCiao,\nDscho\n"},{"id":"124110","messageId":"20091002061159.GA24892@coredump.intra.peff.net","threadId":"21061","inReplyTo":"20091001201648.GA12175@localhost","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-02T06:11:59Z","receivedAt":"2009-10-02T06:11:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Oct 01, 2009 at 10:16:48PM +0200, Clemens Buchacher wrote:\n\n> Instead of\n> \n>   $ git commit -a -ammend\n>   [work ce38944] mend\n>    1 files changed, 2 insertions(+), 0 deletions(-)\n> \n> we now get\n> \n>   $ git commit -a -ammend\n>   error: switch `m' must not be mangled with other options\n>   usage: git commit [options] [--] <filepattern>...\n>   [...]\n> \n> Signed-off-by: Clemens Buchacher <drizzd@aon.at>\n> ---\n> On Fri, Sep 25, 2009 at 04:32:26PM -0700, Shawn O. Pearce wrote:\n> > I wonder, should the -m flag on commit not allow cuddling its\n> > value against the switch when its combined in short form with\n> > other switches?\n> \n> Here we go.\n\nI thought the proposal was to disallow just cuddling of the value when\nthe switch was combined with others. So you would disallow \"git commit\n-ammend\" but it would still be legal to do \"git commit -am foo\". Your\npatch disallows the latter.\n\nI would prefer to allow the uncuddled form, as it is something I do\noccasionally (and I don't think \"git commit -am foo\" looks very much\nlike a typo).\n\nTo be honest, I am not sure that even the more restricted proposal is\nthat good an idea. You are introducing a heuristic to guess at what is a\ntypo or error from the user; when your guess is wrong, the user will be\nannoyed (doubly so if it is buried deep in a script, which this change\nwill also impact). So you are guessing that people don't use the\ncuddled form in this way. I suspect most don't. But I also wonder how\nmany typos you are really helping to save. This was brought up to make\n\"-ammend\" DWYM. Is that really that common a double-typo?\n\nOn the other hand, the cuddled value already has some DWYM magic (it\nrecognizes -amend), so it is already a little bit unsafe to use\n(interestingly, though, the gitcli(7) documentation actually recommends\nusing the \"sticked\" form over the separated one. However, it also\nrecommends splitting your short options).\n\nSo I don't feel _too_ strongly. I am just concerned that we are\nintroducing some DWYM magic that is not really going to help many people\nout, and is just going to make understanding the rules for option\nparsing more complex, and introduce the possibility (albeit slim) of\nbreaking people's scripts and trained fingers.\n\n-Peff\n"},{"id":"124111","messageId":"20091002073628.GA9444@localhost","threadId":"21061","inReplyTo":"20091002061159.GA24892@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-10-02T07:36:28Z","receivedAt":"2009-10-02T07:36:28Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Oct 02, 2009 at 02:11:59AM -0400, Jeff King wrote:\n\n> I thought the proposal was to disallow just cuddling of the value when\n> the switch was combined with others. So you would disallow \"git commit\n> -ammend\" but it would still be legal to do \"git commit -am foo\". Your\n> patch disallows the latter.\n\nYes, that syntax looks reasonable. I expect this to be more involved, so I\nwill rework the patch once we agree on whether or not we want it at all.\n\n> To be honest, I am not sure that even the more restricted proposal is\n> that good an idea. You are introducing a heuristic to guess at what is a\n> typo or error from the user; when your guess is wrong, the user will be\n> annoyed (doubly so if it is buried deep in a script, which this change\n> will also impact).\n\nYes, that can happen. On the other hand, the \"-ammend\" typo actually did\nhappen. And what I'm even more worried about are ambiguities like\n\n  $ git commit -uno <path>\n  $ git commit -nou <path>\n\nwhich are interpreted as one of\n\n  $ git commit --untracked-files=no <path>\n  $ git commit --untracked-files --no-verify --only <path>\n\ndepending only on the order of the switches. I was actually surprised that I\ncould find an example so easily. But the fact alone that it's possible feels\nlike an accident waiting to happen.\n\n> On the other hand, the cuddled value already has some DWYM magic (it\n> recognizes -amend), so it is already a little bit unsafe to use\n\nWell, an error message is a lot safer than executing something you did not\nintend.\n\n> So I don't feel _too_ strongly. I am just concerned that we are\n> introducing some DWYM magic that is not really going to help many people\n> out, and is just going to make understanding the rules for option\n> parsing more complex, and introduce the possibility (albeit slim) of\n> breaking people's scripts and trained fingers.\n\nBut it makes the rules simpler, because it removes ambiguities such as the\none above.\n\nYes, we deliberately allow users to shoot themselves in the foot. But they\nshould have to use at least a long option to do it.\n\nClemens\n"},{"id":"124112","messageId":"20091002074317.GB9444@localhost","threadId":"21061","inReplyTo":"alpine.DEB.1.00.0910012354080.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-10-02T07:43:17Z","receivedAt":"2009-10-02T07:43:17Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:\n\n> And this patch looks even more straight-forward than 1/2, _but_... what \n> about cases where there are short options that are digits?\n\nCould you point me to one of those? I did not find any during my\nnon-exhaustive search. We should be able to handle them easily by adding\nPARSE_OPT_MANY.\n\nClemens\n"},{"id":"124114","messageId":"4AC5AFC3.7000706@gnu.org","threadId":"21061","inReplyTo":"20091002073628.GA9444@localhost","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Paolo Bonzini","fromEmail":"bonzini@gnu.org","sentAt":"2009-10-02T07:46:11Z","receivedAt":"2009-10-02T07:46:11Z","isPatch":true,"sender":{"key":"bonzini@gnu.org","avatar":"https://avatars.githubusercontent.com/u/42082?v=4"},"body":"On 10/02/2009 09:36 AM, Clemens Buchacher wrote:\n> Well, an error message is a lot safer than executing something you did not\n> intend.\n\nIf this is just for double --amend typos I don't see the need.  What you \nintended is just one \"git rebase -i HEAD^^\" away.\n\nNotice that in Shawn's original example the guy actually passed -a too, \nso he would not even have the problem of overwriting the index due to -a.\n\nIf you have bouncy fingers, you can just make an alias\n\n    ammend-fix = git reset --soft HEAD^ && git gui citool\n\nPaolo\n"},{"id":"124115","messageId":"20091002075012.GB27664@coredump.intra.peff.net","threadId":"21061","inReplyTo":"20091002074317.GB9444@localhost","subject":"Re: [PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-02T07:50:12Z","receivedAt":"2009-10-02T07:50:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:\n\n> On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:\n> \n> > And this patch looks even more straight-forward than 1/2, _but_... what \n> > about cases where there are short options that are digits?\n> \n> Could you point me to one of those? I did not find any during my\n> non-exhaustive search. We should be able to handle them easily by adding\n> PARSE_OPT_MANY.\n\nThe one that comes readily to mind is \"git log -1\", but that is actually\nparsed by the revision options parser, which doesn't use parseopt. But\nthere are a few done by parseopt:\n\n  $ git grep \"OPT_.*'[0-9]'\"\n  archive.c:              OPT__COMPR('1', &compression_level, \"compress faster\", 1),\n  archive.c:              OPT__COMPR_HIDDEN('2', &compression_level, 2),\n  archive.c:              OPT__COMPR_HIDDEN('3', &compression_level, 3),\n  archive.c:              OPT__COMPR_HIDDEN('4', &compression_level, 4),\n  archive.c:              OPT__COMPR_HIDDEN('5', &compression_level, 5),\n  archive.c:              OPT__COMPR_HIDDEN('6', &compression_level, 6),\n  archive.c:              OPT__COMPR_HIDDEN('7', &compression_level, 7),\n  archive.c:              OPT__COMPR_HIDDEN('8', &compression_level, 8),\n  archive.c:              OPT__COMPR('9', &compression_level, \"compress better\", 9),\n  builtin-checkout.c:             OPT_SET_INT('2', \"ours\", &opts.writeout_stage, \"stage\",\n  builtin-checkout.c:             OPT_SET_INT('3', \"theirs\", &opts.writeout_stage, \"stage\",\n\n-Peff\n"},{"id":"124116","messageId":"20091002075724.GC27664@coredump.intra.peff.net","threadId":"21061","inReplyTo":"20091002073628.GA9444@localhost","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-02T07:57:24Z","receivedAt":"2009-10-02T07:57:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 02, 2009 at 09:36:28AM +0200, Clemens Buchacher wrote:\n\n> Yes, that syntax looks reasonable. I expect this to be more involved, so I\n> will rework the patch once we agree on whether or not we want it at all.\n\nThanks.\n\n> Yes, that can happen. On the other hand, the \"-ammend\" typo actually did\n> happen.\n\nIt did, but we are only guessing at how many people will be disrupted by\nthe new rule. That being said...\n\n> And what I'm even more worried about are ambiguities like\n> \n>   $ git commit -uno <path>\n>   $ git commit -nou <path>\n> \n> which are interpreted as one of\n> \n>   $ git commit --untracked-files=no <path>\n>   $ git commit --untracked-files --no-verify --only <path>\n\nMaking this clearer is a much more compelling argument to me. Though I\nthought it was customary (not just for git, but for other programs) that\na short option that takes a parameter (even an optional one) would\nconsume the rest of a short options string. Still, it is a potential\nsource of confusion.\n\n> > On the other hand, the cuddled value already has some DWYM magic (it\n> > recognizes -amend), so it is already a little bit unsafe to use\n> \n> Well, an error message is a lot safer than executing something you did not\n> intend.\n\nIt's also an error exit code, which can affect how a script performs\n(e.g., \"git diff --exit-code\"). But I don't have any real examples off\nthe top of my head of how this could be particularly disastrous, so feel\nfree to dismiss that as pushing too far into the hypothetical.\n\n-Peff\n"},{"id":"124120","messageId":"20091002082633.GA21168@localhost","threadId":"21061","inReplyTo":"20091002075012.GB27664@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-10-02T08:26:33Z","receivedAt":"2009-10-02T08:26:33Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:\n> On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:\n> \n> > On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:\n> > \n> > > And this patch looks even more straight-forward than 1/2, _but_... what \n> > > about cases where there are short options that are digits?\n> > \n> > Could you point me to one of those? I did not find any during my\n> > non-exhaustive search. We should be able to handle them easily by adding\n> > PARSE_OPT_MANY.\n> \n> The one that comes readily to mind is \"git log -1\", but that is actually\n> parsed by the revision options parser, which doesn't use parseopt. But\n> there are a few done by parseopt:\n\nOh, I mistakenly thought Dscho was asking about options with single-digit\narguments. Thanks for clearing this up.\n\nClemens\n"},{"id":"124122","messageId":"alpine.DEB.1.00.0910021040260.18640@intel-tinevez-2-302","threadId":"21061","inReplyTo":"20091002082633.GA21168@localhost","subject":"Re: [PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-02T08:41:05Z","receivedAt":"2009-10-02T08:41:05Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 2 Oct 2009, Clemens Buchacher wrote:\n\n> On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:\n> > On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:\n> > \n> > > On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:\n> > > \n> > > > And this patch looks even more straight-forward than 1/2, _but_... what \n> > > > about cases where there are short options that are digits?\n> > > \n> > > Could you point me to one of those? I did not find any during my\n> > > non-exhaustive search. We should be able to handle them easily by adding\n> > > PARSE_OPT_MANY.\n> > \n> > The one that comes readily to mind is \"git log -1\", but that is actually\n> > parsed by the revision options parser, which doesn't use parseopt. But\n> > there are a few done by parseopt:\n> \n> Oh, I mistakenly thought Dscho was asking about options with \n> single-digit arguments. Thanks for clearing this up.\n\nI was actually thinking both of \"git log -11\" as well as out-of-tree users \nof parse-options.\n\nCiao,\nDscho\n"},{"id":"124123","messageId":"alpine.DEB.1.00.0910021042110.18640@intel-tinevez-2-302","threadId":"21061","inReplyTo":"20091002073628.GA9444@localhost","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-02T08:42:36Z","receivedAt":"2009-10-02T08:42:36Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 2 Oct 2009, Clemens Buchacher wrote:\n\n> On Fri, Oct 02, 2009 at 02:11:59AM -0400, Jeff King wrote:\n> \n> > So I don't feel _too_ strongly. I am just concerned that we are \n> > introducing some DWYM magic that is not really going to help many \n> > people out, and is just going to make understanding the rules for \n> > option parsing more complex, and introduce the possibility (albeit \n> > slim) of breaking people's scripts and trained fingers.\n> \n> But it makes the rules simpler, because it removes ambiguities such as \n> the one above.\n> \n> Yes, we deliberately allow users to shoot themselves in the foot. But \n> they should have to use at least a long option to do it.\n\nSomething like this?\n\n-- snipsnap --\n[PATCH] Deliberately allow users to shoot themselves in the foot\n\nBut they should have to use at least a long option to do it.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n git.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/git.c b/git.c\nindex 9883009..e7cb946 100644\n--- a/git.c\n+++ b/git.c\n@@ -122,6 +122,8 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, sizeof(git_dir)), 0);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--shoot-me-in-the-foot\")) {\n+\t\t\twarning (\"Bang, bang!\");\n \t\t} else {\n \t\t\tfprintf(stderr, \"Unknown option: %s\\n\", cmd);\n \t\t\tusage(git_usage_string);\n-- \n1.6.4.297.gcb4cc\n"},{"id":"124124","messageId":"20091002084359.GA8878@coredump.intra.peff.net","threadId":"21061","inReplyTo":"alpine.DEB.1.00.0910021042110.18640@intel-tinevez-2-302","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-10-02T08:43:59Z","receivedAt":"2009-10-02T08:43:59Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 02, 2009 at 10:42:36AM +0200, Johannes Schindelin wrote:\n\n> > Yes, we deliberately allow users to shoot themselves in the foot. But \n> > they should have to use at least a long option to do it.\n> \n> Something like this?\n> [...]\n> +\t\t} else if (!strcmp(cmd, \"--shoot-me-in-the-foot\")) {\n> +\t\t\twarning (\"Bang, bang!\");\n\nDoh! Now I have to come up with a new joke patch for the GitTogether!\n\n-Peff\n"},{"id":"124126","messageId":"alpine.DEB.1.00.0910021104060.18640@intel-tinevez-2-302","threadId":"21061","inReplyTo":"20091002084359.GA8878@coredump.intra.peff.net","subject":"Re: [PATCH 1/2] do not mangle short options which take arguments","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-02T09:04:24Z","receivedAt":"2009-10-02T09:04:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 2 Oct 2009, Jeff King wrote:\n\n> On Fri, Oct 02, 2009 at 10:42:36AM +0200, Johannes Schindelin wrote:\n> \n> > > Yes, we deliberately allow users to shoot themselves in the foot. But \n> > > they should have to use at least a long option to do it.\n> > \n> > Something like this?\n> > [...]\n> > +\t\t} else if (!strcmp(cmd, \"--shoot-me-in-the-foot\")) {\n> > +\t\t\twarning (\"Bang, bang!\");\n> \n> Doh! Now I have to come up with a new joke patch for the GitTogether!\n\nDon't make me envious :-(\n\nCiao,\nDscho\n"},{"id":"124168","messageId":"20091003092332.GA10570@localhost","threadId":"21061","inReplyTo":"20091002075012.GB27664@coredump.intra.peff.net","subject":"Re: [PATCH 2/2] allow mangling short options which take integer arguments","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2009-10-03T09:23:32Z","receivedAt":"2009-10-03T09:23:32Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Fri, Oct 02, 2009 at 03:50:12AM -0400, Jeff King wrote:\n\n> On Fri, Oct 02, 2009 at 09:43:17AM +0200, Clemens Buchacher wrote:\n> \n> > On Thu, Oct 01, 2009 at 11:55:03PM +0200, Johannes Schindelin wrote:\n> > \n> > > And this patch looks even more straight-forward than 1/2, _but_... what \n> > > about cases where there are short options that are digits?\n> > \n> > Could you point me to one of those? I did not find any during my\n> > non-exhaustive search. We should be able to handle them easily by adding\n> > PARSE_OPT_MANY.\n> \n> The one that comes readily to mind is \"git log -1\", but that is actually\n> parsed by the revision options parser, which doesn't use parseopt. But\n> there are a few done by parseopt:\n> \n>   $ git grep \"OPT_.*'[0-9]'\"\n>   archive.c:              OPT__COMPR('1', &compression_level, \"compress faster\", 1),\n>   archive.c:              OPT__COMPR_HIDDEN('2', &compression_level, 2),\n>   archive.c:              OPT__COMPR_HIDDEN('3', &compression_level, 3),\n>   archive.c:              OPT__COMPR_HIDDEN('4', &compression_level, 4),\n>   archive.c:              OPT__COMPR_HIDDEN('5', &compression_level, 5),\n>   archive.c:              OPT__COMPR_HIDDEN('6', &compression_level, 6),\n>   archive.c:              OPT__COMPR_HIDDEN('7', &compression_level, 7),\n>   archive.c:              OPT__COMPR_HIDDEN('8', &compression_level, 8),\n>   archive.c:              OPT__COMPR('9', &compression_level, \"compress better\", 9),\n>   builtin-checkout.c:             OPT_SET_INT('2', \"ours\", &opts.writeout_stage, \"stage\",\n>   builtin-checkout.c:             OPT_SET_INT('3', \"theirs\", &opts.writeout_stage, \"stage\",\n\nThose are not affected by this patch series. They can be cuddled with other\nshort options just like before, since they don't take arguments.\n"}]}