{"thread":{"id":"35991","subject":"[PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE","startedAt":"2014-02-28T09:07:22Z","lastAt":"2014-02-28T23:40:18Z","messageCount":5,"participants":["Sun He","Michael Haggerty","罗丹岳","Junio C Hamano","He Sun"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235564","messageId":"1393578442-2822-1-git-send-email-sunheehnus@gmail.com","threadId":"35991","inReplyTo":null,"subject":"[PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE","fromName":"Sun He","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-02-28T09:07:22Z","receivedAt":"2014-02-28T09:07:22Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"Signed-off-by: Sun He \n---\n parse-options.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 7b8d3fa..59a52b0 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)\n \t\tcase OPTION_NEGBIT:\n \t\tcase OPTION_SET_INT:\n \t\tcase OPTION_SET_PTR:\n-\t\tcase OPTION_NUMBER:\n+\t\tcase OPTION_CMDMODE:\n \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n \t\t\t\terr |= optbug(opts, \"should not accept an argument\");\n-- \n1.9.0.138.g2de3478.dirty\n\nHi,\nWhen I was reading code yesterday, I came across this protential bug.\nparse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.\n\nAccording to the information the program says (Should not accept an argument), I think you should take this patch into consideration.\nThanks.\n\nHe Sun\n"},{"id":"235588","messageId":"531061D1.2060206@alum.mit.edu","threadId":"35991","inReplyTo":"1393578442-2822-1-git-send-email-sunheehnus@gmail.com","subject":"Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-02-28T10:15:45Z","receivedAt":"2014-02-28T10:15:45Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/28/2014 10:07 AM, Sun He wrote:\n> Signed-off-by: Sun He \n> ---\n> parse-options.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git a/parse-options.c b/parse-options.c\n> index 7b8d3fa..59a52b0 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)\n> \t\tcase OPTION_NEGBIT:\n> \t\tcase OPTION_SET_INT:\n> \t\tcase OPTION_SET_PTR:\n> -\t\tcase OPTION_NUMBER:\n> +\t\tcase OPTION_CMDMODE:\n> \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n> \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n> \t\t\t\terr |= optbug(opts, \"should not accept an argument\");\n> \n> -- \n> 1.9.0.138.g2de3478.dirty\n> \n> Hi,\n> When I was reading code yesterday, I came across this protential bug.\n> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.\n> \n> According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.\n> Thanks.\n> \n> He Sun\n\nPlease resubmit this change in the proper format (as described by Eric\nSunshine WRT another one of your patches). Also please remember to\ndistinguish between information that should go in the commit log\nmessage, which will be stored permanently to the repository and help\nfuture developers understand your change, vs. side notes meant only for\nthe mailing list. For example, for the log message, stuff like:\n\n OPTION_CMDMODE should be used when ... So change the mode to\n OPTION_CMDMODE so that ...\n\nvs. for the mailing list discussion:\n\n When I was reading code yesterday ...\n\nThe former goes above the \"---\" line and the latter goes directly below it.\n\nBTW, none of my comments should be taken to indicate whether the commit\nitself makes sense or not. I haven't checked that far.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"235590","messageId":"CAFvDSgJZ_Ku9EoBO8UoG5iugf-0dLNwfMFqRz=z-KeJD4Tsa3g@mail.gmail.com","threadId":"35991","inReplyTo":"531061D1.2060206@alum.mit.edu","subject":"Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE","fromName":"罗丹岳","fromEmail":"dirkdyluo@gmail.com","sentAt":"2014-02-28T10:49:30Z","receivedAt":"2014-02-28T10:49:30Z","isPatch":true,"sender":{"key":"dirkdyluo@gmail.com","avatar":null},"body":"On Fri, Feb 28, 2014 at 6:15 PM, Michael Haggerty wrote:\n>\n> On 02/28/2014 10:07 AM, Sun He wrote:\n> > Signed-off-by: Sun He \n> > ---\n> > parse-options.c | 2 +-\n> > 1 file changed, 1 insertion(+), 1 deletion(-)\n> >\n> > diff --git a/parse-options.c b/parse-options.c\n> > index 7b8d3fa..59a52b0 100644\n> > --- a/parse-options.c\n> > +++ b/parse-options.c\n> > @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)\n> > case OPTION_NEGBIT:\n> > case OPTION_SET_INT:\n> > case OPTION_SET_PTR:\n> > - case OPTION_NUMBER:\n> > + case OPTION_CMDMODE:\n> > if ((opts->flags & PARSE_OPT_OPTARG) ||\n> > !(opts->flags & PARSE_OPT_NOARG))\n> > err |= optbug(opts, \"should not accept an argument\");\n> >\n> > --\n> > 1.9.0.138.g2de3478.dirty\n> >\n> > Hi,\n> > When I was reading code yesterday, I came across this protential bug.\n> > parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.\n> >\n> > According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.\n> > Thanks.\n> >\n> > He Sun\n>\n> Please resubmit this change in the proper format (as described by Eric\n> Sunshine WRT another one of your patches). Also please remember to\n> distinguish between information that should go in the commit log\n> message, which will be stored permanently to the repository and help\n> future developers understand your change, vs. side notes meant only for\n> the mailing list. For example, for the log message, stuff like:\n>\n> OPTION_CMDMODE should be used when ... So change the mode to\n> OPTION_CMDMODE so that ...\n>\n> vs. for the mailing list discussion:\n>\n> When I was reading code yesterday ...\n>\n> The former goes above the \"---\" line and the latter goes directly below it.\n>\n> BTW, none of my comments should be taken to indicate whether the commit\n> itself makes sense or not. I haven't checked that far.\n>\n> Michael\n>\n> --\n> Michael Haggerty\n> mhagger@alum.mit.edu\n> http://softwareswirl.blogspot.com/\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n\n\n>.<\n"},{"id":"235660","messageId":"xmqq1tynuiqh.fsf@gitster.dls.corp.google.com","threadId":"35991","inReplyTo":"531061D1.2060206@alum.mit.edu","subject":"Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-02-28T19:42:30Z","receivedAt":"2014-02-28T19:42:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 02/28/2014 10:07 AM, Sun He wrote:\n>> Signed-off-by: Sun He \n>> ---\n>> parse-options.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>> \n>> diff --git a/parse-options.c b/parse-options.c\n>> index 7b8d3fa..59a52b0 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)\n>> \t\tcase OPTION_NEGBIT:\n>> \t\tcase OPTION_SET_INT:\n>> \t\tcase OPTION_SET_PTR:\n>> -\t\tcase OPTION_NUMBER:\n>> +\t\tcase OPTION_CMDMODE:\n>> \t\t\tif ((opts->flags & PARSE_OPT_OPTARG) ||\n>> \t\t\t !(opts->flags & PARSE_OPT_NOARG))\n>> \t\t\t\terr |= optbug(opts, \"should not accept an argument\");\n>> \n>> -- \n>> 1.9.0.138.g2de3478.dirty\n>> \n>> Hi,\n>> When I was reading code yesterday, I came across this protential bug.\n>> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.\n\nPlease, no overlong line in the text part that makes things\nunnecessarily hard to read.\n\nI agree with all the comments in the message I am responding to.\n\n> BTW, none of my comments should be taken to indicate whether the commit\n> itself makes sense or not. I haven't checked that far.\n\nWhile I think it is sensible to make sure CMDMODE is not marked to\nallow arguments, I do not understand why \"special type\" would imply\n\"it is allowed to be marked to take an argument\". Why is it a\ngood change to remove \"case OPTION_NUMBER:\" here?\n"},{"id":"235680","messageId":"CAJr59C0jKYcee_wfzTrv+pg51DGuBKekeC9iz0TF_wzAuKjs7w@mail.gmail.com","threadId":"35991","inReplyTo":"xmqq1tynuiqh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-02-28T23:40:18Z","receivedAt":"2014-02-28T23:40:18Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 3:42 GMT+08:00 Junio C Hamano :\n> Michael Haggerty writes:\n>\n>> On 02/28/2014 10:07 AM, Sun He wrote:\n>>> Signed-off-by: Sun He \n>>> ---\n>>> parse-options.c | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/parse-options.c b/parse-options.c\n>>> index 7b8d3fa..59a52b0 100644\n>>> --- a/parse-options.c\n>>> +++ b/parse-options.c\n>>> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)\n>>> case OPTION_NEGBIT:\n>>> case OPTION_SET_INT:\n>>> case OPTION_SET_PTR:\n>>> - case OPTION_NUMBER:\n>>> + case OPTION_CMDMODE:\n>>> if ((opts->flags & PARSE_OPT_OPTARG) ||\n>>> !(opts->flags & PARSE_OPT_NOARG))\n>>> err |= optbug(opts, \"should not accept an argument\");\n>>>\n>>> --\n>>> 1.9.0.138.g2de3478.dirty\n>>>\n>>> Hi,\n>>> When I was reading code yesterday, I came across this protential bug.\n>>> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.\n>\n> Please, no overlong line in the text part that makes things\n> unnecessarily hard to read.\n>\n> I agree with all the comments in the message I am responding to.\n>\n\nOK, got it. Thanks.\n\n>> BTW, none of my comments should be taken to indicate whether the commit\n>> itself makes sense or not. I haven't checked that far.\n>\n> While I think it is sensible to make sure CMDMODE is not marked to\n> allow arguments, I do not understand why \"special type\" would imply\n> \"it is allowed to be marked to take an argument\". Why is it a\n> good change to remove \"case OPTION_NUMBER:\" here?\n\nThe comments in parse-options.h (Line 10-16) says that CMDMODE is not\nmarked to allow arguments. And I am not sure if removing OPTION_NUMBER\nis a proper way.\n"}]}