{"thread":{"id":"24301","subject":"[PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","startedAt":"2010-07-06T14:46:05Z","lastAt":"2010-07-08T12:44:35Z","messageCount":6,"participants":["Uwe Kleine-König","Junio C Hamano","Pierre Habouzit"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"144910","messageId":"1278427565-11057-1-git-send-email-u.kleine-koenig@pengutronix.de","threadId":"24301","inReplyTo":null,"subject":"[PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2010-07-06T14:46:05Z","receivedAt":"2010-07-06T14:46:05Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"The ?: operator has a lower priority than |, so the implicit associativity\nmade the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if\nkeep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and\nPARSE_OPT_SHELL_EVAL.\n\nSigned-off-by: Uwe Kleine-König \n---\n builtin/rev-parse.c | 4 ++--\n t/t1502-rev-parse-parseopt.sh | 17 +++++++++++++++++\n 2 files changed, 19 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex b676e29..a5a1c86 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -407,8 +407,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \tALLOC_GROW(opts, onb + 1, osz);\n \tmemset(opts + onb, 0, sizeof(opts[onb]));\n \targc = parse_options(argc, argv, prefix, opts, usage,\n-\t\t\tkeep_dashdash ? PARSE_OPT_KEEP_DASHDASH : 0 |\n-\t\t\tstop_at_non_option ? PARSE_OPT_STOP_AT_NON_OPTION : 0 |\n+\t\t\t(keep_dashdash ? PARSE_OPT_KEEP_DASHDASH : 0) |\n+\t\t\t(stop_at_non_option ? PARSE_OPT_STOP_AT_NON_OPTION : 0) |\n \t\t\tPARSE_OPT_SHELL_EVAL);\n \n \tstrbuf_addf(&parsed, \" --\");\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex 4346795..17c210c 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -81,4 +81,21 @@ test_expect_success 'test --parseopt --keep-dashdash' '\n \ttest_cmp expect output\n '\n \n+cat > expect < output &&\n+\ttest_cmp expect output\n+'\n+\n+cat > expect < output &&\n+\ttest_cmp expect output\n+'\n test_done\n-- \n1.7.1\n"},{"id":"145061","messageId":"7vr5jfrmuq.fsf@alter.siamese.dyndns.org","threadId":"24301","inReplyTo":"1278427565-11057-1-git-send-email-u.kleine-koenig@pengutronix.de","subject":"Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-07T21:41:33Z","receivedAt":"2010-07-07T21:41:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Uwe Kleine-König writes:\n\n> The ?: operator has a lower priority than |, so the implicit associativity\n> made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if\n> keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and\n> PARSE_OPT_SHELL_EVAL.\n\nWow, this is an age-old breakage dating back to 6e0800e (parse-opt: make\nPARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that\ndates back to the very original --stop-at-non-option patch, isn't it?\n\nI wonder if I should issue an updated maintenance release v1.6.4.5 ;-)\n\nThanks.\n"},{"id":"145092","messageId":"20100708065118.GA21565@pengutronix.de","threadId":"24301","inReplyTo":"7vr5jfrmuq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2010-07-08T06:51:18Z","receivedAt":"2010-07-08T06:51:18Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote:\n> Uwe Kleine-König writes:\n> \n> > The ?: operator has a lower priority than |, so the implicit associativity\n> > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if\n> > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and\n> > PARSE_OPT_SHELL_EVAL.\n> \n> Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make\n> PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that\n> dates back to the very original --stop-at-non-option patch, isn't it?\nYeah, the author of 6e0800e should get a pat on his head and a lesson in\nC. :-)\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"},{"id":"145097","messageId":"20100708072623.GC21565@pengutronix.de","threadId":"24301","inReplyTo":"7vr5jfrmuq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2010-07-08T07:26:23Z","receivedAt":"2010-07-08T07:26:23Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote:\n> Uwe Kleine-König writes:\n> \n> > The ?: operator has a lower priority than |, so the implicit associativity\n> > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if\n> > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and\n> > PARSE_OPT_SHELL_EVAL.\n> \n> Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make\n> PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that\n> dates back to the very original --stop-at-non-option patch, isn't it?\nI made a quick C-quiz at my company asking what's wrong with 6e0800e.\n\nApart from the bug fixed in my patch a colleague wondered about\nstop_at_non_option being static. I think it doesn't do any harm, still\nI think being an automatic variable would be more common. Is the static\nintended here? This was introduced in\n21d4783538662143ef52ed6967c948ab27586232, so I cc:d Pierre.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"},{"id":"145106","messageId":"20100708101848.GA12789@madism.org","threadId":"24301","inReplyTo":"20100708072623.GC21565@pengutronix.de","subject":"Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","fromName":"Pierre Habouzit","fromEmail":"madcoder@debian.org","sentAt":"2010-07-08T10:18:48Z","receivedAt":"2010-07-08T10:18:48Z","isPatch":true,"sender":{"key":"madcoder@debian.org","avatar":"https://avatars.githubusercontent.com/u/44708?v=4"},"body":"On Thu, Jul 08, 2010 at 09:26:23AM +0200, Uwe Kleine-König wrote:\n> On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote:\n> > Uwe Kleine-König writes:\n> > \n> > > The ?: operator has a lower priority than |, so the implicit associativity\n> > > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if\n> > > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and\n> > > PARSE_OPT_SHELL_EVAL.\n> > \n> > Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make\n> > PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that\n> > dates back to the very original --stop-at-non-option patch, isn't it?\n> I made a quick C-quiz at my company asking what's wrong with 6e0800e.\n> \n> Apart from the bug fixed in my patch a colleague wondered about\n> stop_at_non_option being static. I think it doesn't do any harm, still\n> I think being an automatic variable would be more common. Is the static\n> intended here? This was introduced in\n> 21d4783538662143ef52ed6967c948ab27586232, so I cc:d Pierre.\n\nWell, the sole difference is that it makes &stop_at_non_option been\ncomputed at compile time instead of runtime, which is pretty much the\nsame. cmd_parseopt isn't meant to be reentrant so it's not important.\n-- \n·O· Pierre Habouzit\n··O madcoder@debian.org\nOOO http://www.madism.org\n"},{"id":"145121","messageId":"20100708124435.GA26404@pengutronix.de","threadId":"24301","inReplyTo":"20100708101848.GA12789@madism.org","subject":"Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option","fromName":"Uwe Kleine-König","fromEmail":"u.kleine-koenig@pengutronix.de","sentAt":"2010-07-08T12:44:35Z","receivedAt":"2010-07-08T12:44:35Z","isPatch":true,"sender":{"key":"u.kleine-koenig@pengutronix.de","avatar":"https://gravatar.com/avatar/354b5e3ceb2806a2f1e1e382ac29ddbdad18288654da62b61eb13583a857eee7?d=mp&s=160"},"body":"Hi Pierre,\n\nOn Thu, Jul 08, 2010 at 12:18:48PM +0200, Pierre Habouzit wrote:\n> On Thu, Jul 08, 2010 at 09:26:23AM +0200, Uwe Kleine-König wrote:\n> > On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote:\n> > > Uwe Kleine-König writes:\n> > > \n> > > > The ?: operator has a lower priority than |, so the implicit associativity\n> > > > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if\n> > > > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and\n> > > > PARSE_OPT_SHELL_EVAL.\n> > > \n> > > Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make\n> > > PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that\n> > > dates back to the very original --stop-at-non-option patch, isn't it?\n> > I made a quick C-quiz at my company asking what's wrong with 6e0800e.\n> > \n> > Apart from the bug fixed in my patch a colleague wondered about\n> > stop_at_non_option being static. I think it doesn't do any harm, still\n> > I think being an automatic variable would be more common. Is the static\n> > intended here? This was introduced in\n> > 21d4783538662143ef52ed6967c948ab27586232, so I cc:d Pierre.\n> \n> Well, the sole difference is that it makes &stop_at_non_option been\n> computed at compile time instead of runtime, which is pretty much the\n> same. cmd_parseopt isn't meant to be reentrant so it's not important.\nI don't know about x86, but I think on arm computing at compile time\nisn't cheaper than at runtime, it's just pc-relative instead of\nsp-relative. But having the variable automatic saves a bit of heap.\nProbably not worth to discuss about these two ints though.\n\nBest regards\nUwe\n\n-- \nPengutronix e.K. | Uwe Kleine-König |\nIndustrial Linux Solutions | http://www.pengutronix.de/ |\n"}]}