* [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
@ 2014-02-28 9:07 Sun He
2014-02-28 10:15 ` Michael Haggerty
0 siblings, 1 reply; 5+ messages in thread
From: Sun He @ 2014-02-28 9:07 UTC (permalink / raw)
To: git; +Cc: Sun He
Signed-off-by: Sun He <sunheehnus@gmail.com>
---
parse-options.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/parse-options.c b/parse-options.c
index 7b8d3fa..59a52b0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
case OPTION_NEGBIT:
case OPTION_SET_INT:
case OPTION_SET_PTR:
- case OPTION_NUMBER:
+ case OPTION_CMDMODE:
if ((opts->flags & PARSE_OPT_OPTARG) ||
!(opts->flags & PARSE_OPT_NOARG))
err |= optbug(opts, "should not accept an argument");
--
1.9.0.138.g2de3478.dirty
Hi,
When I was reading code yesterday, I came across this protential bug.
parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.
Thanks.
He Sun
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
2014-02-28 9:07 [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE Sun He
@ 2014-02-28 10:15 ` Michael Haggerty
2014-02-28 10:49 ` 罗丹岳
2014-02-28 19:42 ` Junio C Hamano
0 siblings, 2 replies; 5+ messages in thread
From: Michael Haggerty @ 2014-02-28 10:15 UTC (permalink / raw)
To: Sun He; +Cc: git
On 02/28/2014 10:07 AM, Sun He wrote:
> Signed-off-by: Sun He <sunheehnus@gmail.com>
> ---
> parse-options.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/parse-options.c b/parse-options.c
> index 7b8d3fa..59a52b0 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
> case OPTION_NEGBIT:
> case OPTION_SET_INT:
> case OPTION_SET_PTR:
> - case OPTION_NUMBER:
> + case OPTION_CMDMODE:
> if ((opts->flags & PARSE_OPT_OPTARG) ||
> !(opts->flags & PARSE_OPT_NOARG))
> err |= optbug(opts, "should not accept an argument");
>
> --
> 1.9.0.138.g2de3478.dirty
>
> Hi,
> When I was reading code yesterday, I came across this protential bug.
> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
>
> According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.
> Thanks.
>
> He Sun
Please resubmit this change in the proper format (as described by Eric
Sunshine WRT another one of your patches). Also please remember to
distinguish between information that should go in the commit log
message, which will be stored permanently to the repository and help
future developers understand your change, vs. side notes meant only for
the mailing list. For example, for the log message, stuff like:
OPTION_CMDMODE should be used when ... So change the mode to
OPTION_CMDMODE so that ...
vs. for the mailing list discussion:
When I was reading code yesterday ...
The former goes above the "---" line and the latter goes directly below it.
BTW, none of my comments should be taken to indicate whether the commit
itself makes sense or not. I haven't checked that far.
Michael
--
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
2014-02-28 10:15 ` Michael Haggerty
@ 2014-02-28 10:49 ` 罗丹岳
2014-02-28 19:42 ` Junio C Hamano
1 sibling, 0 replies; 5+ messages in thread
From: 罗丹岳 @ 2014-02-28 10:49 UTC (permalink / raw)
To: git
On Fri, Feb 28, 2014 at 6:15 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:
>
> On 02/28/2014 10:07 AM, Sun He wrote:
> > Signed-off-by: Sun He <sunheehnus@gmail.com>
> > ---
> > parse-options.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/parse-options.c b/parse-options.c
> > index 7b8d3fa..59a52b0 100644
> > --- a/parse-options.c
> > +++ b/parse-options.c
> > @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
> > case OPTION_NEGBIT:
> > case OPTION_SET_INT:
> > case OPTION_SET_PTR:
> > - case OPTION_NUMBER:
> > + case OPTION_CMDMODE:
> > if ((opts->flags & PARSE_OPT_OPTARG) ||
> > !(opts->flags & PARSE_OPT_NOARG))
> > err |= optbug(opts, "should not accept an argument");
> >
> > --
> > 1.9.0.138.g2de3478.dirty
> >
> > Hi,
> > When I was reading code yesterday, I came across this protential bug.
> > parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
> >
> > According to the information the program says (Should not accept an argument), I think you should take this patch into consideration.
> > Thanks.
> >
> > He Sun
>
> Please resubmit this change in the proper format (as described by Eric
> Sunshine WRT another one of your patches). Also please remember to
> distinguish between information that should go in the commit log
> message, which will be stored permanently to the repository and help
> future developers understand your change, vs. side notes meant only for
> the mailing list. For example, for the log message, stuff like:
>
> OPTION_CMDMODE should be used when ... So change the mode to
> OPTION_CMDMODE so that ...
>
> vs. for the mailing list discussion:
>
> When I was reading code yesterday ...
>
> The former goes above the "---" line and the latter goes directly below it.
>
> BTW, none of my comments should be taken to indicate whether the commit
> itself makes sense or not. I haven't checked that far.
>
> Michael
>
> --
> Michael Haggerty
> mhagger@alum.mit.edu
> http://softwareswirl.blogspot.com/
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>.<
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
2014-02-28 10:15 ` Michael Haggerty
2014-02-28 10:49 ` 罗丹岳
@ 2014-02-28 19:42 ` Junio C Hamano
2014-02-28 23:40 ` He Sun
1 sibling, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2014-02-28 19:42 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Sun He, git
Michael Haggerty <mhagger@alum.mit.edu> writes:
> On 02/28/2014 10:07 AM, Sun He wrote:
>> Signed-off-by: Sun He <sunheehnus@gmail.com>
>> ---
>> parse-options.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/parse-options.c b/parse-options.c
>> index 7b8d3fa..59a52b0 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
>> case OPTION_NEGBIT:
>> case OPTION_SET_INT:
>> case OPTION_SET_PTR:
>> - case OPTION_NUMBER:
>> + case OPTION_CMDMODE:
>> if ((opts->flags & PARSE_OPT_OPTARG) ||
>> !(opts->flags & PARSE_OPT_NOARG))
>> err |= optbug(opts, "should not accept an argument");
>>
>> --
>> 1.9.0.138.g2de3478.dirty
>>
>> Hi,
>> When I was reading code yesterday, I came across this protential bug.
>> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
Please, no overlong line in the text part that makes things
unnecessarily hard to read.
I agree with all the comments in the message I am responding to.
> BTW, none of my comments should be taken to indicate whether the commit
> itself makes sense or not. I haven't checked that far.
While I think it is sensible to make sure CMDMODE is not marked to
allow arguments, I do not understand why "special type" would imply
"it is allowed to be marked to take an argument". Why is it a
good change to remove "case OPTION_NUMBER:" here?
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE
2014-02-28 19:42 ` Junio C Hamano
@ 2014-02-28 23:40 ` He Sun
0 siblings, 0 replies; 5+ messages in thread
From: He Sun @ 2014-02-28 23:40 UTC (permalink / raw)
To: Junio C Hamano, git
2014-03-01 3:42 GMT+08:00 Junio C Hamano <gitster@pobox.com>:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>
>> On 02/28/2014 10:07 AM, Sun He wrote:
>>> Signed-off-by: Sun He <sunheehnus@gmail.com>
>>> ---
>>> parse-options.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/parse-options.c b/parse-options.c
>>> index 7b8d3fa..59a52b0 100644
>>> --- a/parse-options.c
>>> +++ b/parse-options.c
>>> @@ -371,7 +371,7 @@ static void parse_options_check(const struct option *opts)
>>> case OPTION_NEGBIT:
>>> case OPTION_SET_INT:
>>> case OPTION_SET_PTR:
>>> - case OPTION_NUMBER:
>>> + case OPTION_CMDMODE:
>>> if ((opts->flags & PARSE_OPT_OPTARG) ||
>>> !(opts->flags & PARSE_OPT_NOARG))
>>> err |= optbug(opts, "should not accept an argument");
>>>
>>> --
>>> 1.9.0.138.g2de3478.dirty
>>>
>>> Hi,
>>> When I was reading code yesterday, I came across this protential bug.
>>> parse-options.h says that OPTION_CMDMODE is an option with no arguments and OPTION_NUMBER is special type option.
>
> Please, no overlong line in the text part that makes things
> unnecessarily hard to read.
>
> I agree with all the comments in the message I am responding to.
>
OK, got it. Thanks.
>> BTW, none of my comments should be taken to indicate whether the commit
>> itself makes sense or not. I haven't checked that far.
>
> While I think it is sensible to make sure CMDMODE is not marked to
> allow arguments, I do not understand why "special type" would imply
> "it is allowed to be marked to take an argument". Why is it a
> good change to remove "case OPTION_NUMBER:" here?
The comments in parse-options.h (Line 10-16) says that CMDMODE is not
marked to allow arguments. And I am not sure if removing OPTION_NUMBER
is a proper way.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2014-02-28 23:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-02-28 9:07 [PATCH] parse-options.c:parse_options_check() change OPTION_NUMBER to OPTION_CMDMODE Sun He
2014-02-28 10:15 ` Michael Haggerty
2014-02-28 10:49 ` 罗丹岳
2014-02-28 19:42 ` Junio C Hamano
2014-02-28 23:40 ` He Sun
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).