Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned

> Robert Stanca <robert.stanca7@gmail.com> writes:
>
>> Subject: Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned
>
> Try
>
>     git shortlog --no-merges -40
>
> to learn how commits are typically titled.  And then imagine this
> patch makes into our codebase and appear in the output.
>
> Can you tell what this commit is about from that single line title?
> No.  You wouldn't even know that it is only touching bisect.h and
> nothing else.
>
> What do your readers want "shortlog" output to tell them about your
> commit?  What are the most important thing (other than giving you an
> excuse to say "I have completed a microproject and now I am eligible
> to apply to GSoC" ;-)?  Your proposed commit log message, especially
> its title, must be written to help future readers of the project
> history.
>
> Perhaps
>
>     bisect.h: make flags field in rev_list_info unsigned
>
> would help them better.
>
>>  Unsigned int is a closer representation of bitflags rather than signed int that uses 1 special bit for sign.This shouldn't make much difference because rev_list_info.flags uses only 2 bits(BISECT_SHOW_ALL and REV_LIST_QUIET)
>
> Overlong lines, without space after full-stop before the second
> sentence, without full-stop at the end of the sentence.
>
>>
>> Signed-off-by: Robert Stanca <robert.stanca7@gmail.com>
>> ---
>>  bisect.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/bisect.h b/bisect.h
>> index acd12ef80..a979a7f11 100644
>> --- a/bisect.h
>> +++ b/bisect.h
>> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct commit_list *list,
>>
>>  struct rev_list_info {
>>       struct rev_info *revs;
>> -     int flags;
>> +     unsigned int flags;
>
> Have you checked how this field is used?  For example, there is this
> line somewhere in rev-list.c
>
>         int cnt, flags = info->flags;
>
> and the reason why the code copies the value to a local variable
> "flags" must be because it is going to use it, just like it and
> other codepaths use info->flags, no?  It makes the code inconsistent
> by leaving the local variable a signed int, I suspect.