# [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option 6 messages from 2010-07-06 to 2010-07-08. Participants: Uwe Kleine-König, Junio C Hamano, Pierre Habouzit. Thread: https://gitlist.dev/t/24301 ## Uwe Kleine-König, 2010-07-06 14:46 Subject: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option Message-ID: <1278427565-11057-1-git-send-email-u.kleine-koenig@pengutronix.de> URL: https://gitlist.dev/e/1278427565-11057-1-git-send-email-u.kleine-koenig%40pengutronix.de ``` The ?: operator has a lower priority than |, so the implicit associativity made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and PARSE_OPT_SHELL_EVAL. Signed-off-by: Uwe Kleine-König --- builtin/rev-parse.c | 4 ++-- t/t1502-rev-parse-parseopt.sh | 17 +++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c index b676e29..a5a1c86 100644 --- a/builtin/rev-parse.c +++ b/builtin/rev-parse.c @@ -407,8 +407,8 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix) ALLOC_GROW(opts, onb + 1, osz); memset(opts + onb, 0, sizeof(opts[onb])); argc = parse_options(argc, argv, prefix, opts, usage, - keep_dashdash ? PARSE_OPT_KEEP_DASHDASH : 0 | - stop_at_non_option ? PARSE_OPT_STOP_AT_NON_OPTION : 0 | + (keep_dashdash ? PARSE_OPT_KEEP_DASHDASH : 0) | + (stop_at_non_option ? PARSE_OPT_STOP_AT_NON_OPTION : 0) | PARSE_OPT_SHELL_EVAL); strbuf_addf(&parsed, " --"); diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh index 4346795..17c210c 100755 --- a/t/t1502-rev-parse-parseopt.sh +++ b/t/t1502-rev-parse-parseopt.sh @@ -81,4 +81,21 @@ test_expect_success 'test --parseopt --keep-dashdash' ' test_cmp expect output ' +cat > expect < output && + test_cmp expect output +' + +cat > expect < output && + test_cmp expect output +' test_done -- 1.7.1 ``` ## Junio C Hamano, 2010-07-07 21:41 Subject: Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option Message-ID: <7vr5jfrmuq.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vr5jfrmuq.fsf%40alter.siamese.dyndns.org In-Reply-To: <1278427565-11057-1-git-send-email-u.kleine-koenig@pengutronix.de> ``` Uwe Kleine-König writes: > The ?: operator has a lower priority than |, so the implicit associativity > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and > PARSE_OPT_SHELL_EVAL. Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that dates back to the very original --stop-at-non-option patch, isn't it? I wonder if I should issue an updated maintenance release v1.6.4.5 ;-) Thanks. ``` ## Uwe Kleine-König, 2010-07-08 06:51 Subject: Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option Message-ID: <20100708065118.GA21565@pengutronix.de> URL: https://gitlist.dev/e/20100708065118.GA21565%40pengutronix.de In-Reply-To: <7vr5jfrmuq.fsf@alter.siamese.dyndns.org> ``` On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote: > Uwe Kleine-König writes: > > > The ?: operator has a lower priority than |, so the implicit associativity > > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if > > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and > > PARSE_OPT_SHELL_EVAL. > > Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make > PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that > dates back to the very original --stop-at-non-option patch, isn't it? Yeah, the author of 6e0800e should get a pat on his head and a lesson in C. :-) Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ``` ## Uwe Kleine-König, 2010-07-08 07:26 Subject: Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option Message-ID: <20100708072623.GC21565@pengutronix.de> URL: https://gitlist.dev/e/20100708072623.GC21565%40pengutronix.de In-Reply-To: <7vr5jfrmuq.fsf@alter.siamese.dyndns.org> ``` On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote: > Uwe Kleine-König writes: > > > The ?: operator has a lower priority than |, so the implicit associativity > > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if > > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and > > PARSE_OPT_SHELL_EVAL. > > Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make > PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that > dates back to the very original --stop-at-non-option patch, isn't it? I made a quick C-quiz at my company asking what's wrong with 6e0800e. Apart from the bug fixed in my patch a colleague wondered about stop_at_non_option being static. I think it doesn't do any harm, still I think being an automatic variable would be more common. Is the static intended here? This was introduced in 21d4783538662143ef52ed6967c948ab27586232, so I cc:d Pierre. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ``` ## Pierre Habouzit, 2010-07-08 10:18 Subject: Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option Message-ID: <20100708101848.GA12789@madism.org> URL: https://gitlist.dev/e/20100708101848.GA12789%40madism.org In-Reply-To: <20100708072623.GC21565@pengutronix.de> ``` On Thu, Jul 08, 2010 at 09:26:23AM +0200, Uwe Kleine-König wrote: > On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote: > > Uwe Kleine-König writes: > > > > > The ?: operator has a lower priority than |, so the implicit associativity > > > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if > > > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and > > > PARSE_OPT_SHELL_EVAL. > > > > Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make > > PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that > > dates back to the very original --stop-at-non-option patch, isn't it? > I made a quick C-quiz at my company asking what's wrong with 6e0800e. > > Apart from the bug fixed in my patch a colleague wondered about > stop_at_non_option being static. I think it doesn't do any harm, still > I think being an automatic variable would be more common. Is the static > intended here? This was introduced in > 21d4783538662143ef52ed6967c948ab27586232, so I cc:d Pierre. Well, the sole difference is that it makes &stop_at_non_option been computed at compile time instead of runtime, which is pretty much the same. cmd_parseopt isn't meant to be reentrant so it's not important. -- ·O· Pierre Habouzit ··O madcoder@debian.org OOO http://www.madism.org ``` ## Uwe Kleine-König, 2010-07-08 12:44 Subject: Re: [PATCH] rev-parse: fix --parse-opt --keep-dashdash --stop-at-non-option Message-ID: <20100708124435.GA26404@pengutronix.de> URL: https://gitlist.dev/e/20100708124435.GA26404%40pengutronix.de In-Reply-To: <20100708101848.GA12789@madism.org> ``` Hi Pierre, On Thu, Jul 08, 2010 at 12:18:48PM +0200, Pierre Habouzit wrote: > On Thu, Jul 08, 2010 at 09:26:23AM +0200, Uwe Kleine-König wrote: > > On Wed, Jul 07, 2010 at 02:41:33PM -0700, Junio C Hamano wrote: > > > Uwe Kleine-König writes: > > > > > > > The ?: operator has a lower priority than |, so the implicit associativity > > > > made the 6th argument of parse_options be PARSE_OPT_KEEP_DASHDASH if > > > > keep_dashdash was true discarding PARSE_OPT_STOP_AT_NON_OPTION and > > > > PARSE_OPT_SHELL_EVAL. > > > > > > Wow, this is an age-old breakage dating back to 6e0800e (parse-opt: make > > > PARSE_OPT_STOP_AT_NON_OPTION available to git rev-parse, 2009-06-14) that > > > dates back to the very original --stop-at-non-option patch, isn't it? > > I made a quick C-quiz at my company asking what's wrong with 6e0800e. > > > > Apart from the bug fixed in my patch a colleague wondered about > > stop_at_non_option being static. I think it doesn't do any harm, still > > I think being an automatic variable would be more common. Is the static > > intended here? This was introduced in > > 21d4783538662143ef52ed6967c948ab27586232, so I cc:d Pierre. > > Well, the sole difference is that it makes &stop_at_non_option been > computed at compile time instead of runtime, which is pretty much the > same. cmd_parseopt isn't meant to be reentrant so it's not important. I don't know about x86, but I think on arm computing at compile time isn't cheaper than at runtime, it's just pc-relative instead of sp-relative. But having the variable automatic saves a bit of heap. Probably not worth to discuss about these two ints though. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ```