patch, 2 parts[GSOC] Convert signed flags to unsigned

 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)

Signed-off-by: Robert Stanca <robert.stanca7@gmail.com>
---
 bisect.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

 As rev_list_info's flag is unsigned int , var flags should have proper type.Also var cnt could be unsigned as there's no negative number of commits (all-reaches)

Signed-off-by: Robert Stanca <robert.stanca7@gmail.com>
---
 builtin/rev-list.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Robert Stanca <robert.stanca7@gmail.com> writes:

OK.  I would have squashed these two commits into one, though.

I am used to 1change per patch so it's easier to redo specific patches...if there are small changes(10 lines max) can i send them as 1 patch?

On Sat, Apr 1, 2017 at 10:13 PM, Junio C Hamano <gitster@pobox.com> wrote:

Robert Stanca <robert.stanca7@gmail.com> writes:

It's not number of lines. One line per patch does not make sense if you need to make corresponding changes to two places, one line each, in order to make the end result consistent. If you change a type of a structure field, and that field is assigned to a variable somewhere, you would change the type of both that field and the variable that receives its value at the same time in a single commit, as that would be the logical unit of a smallest change that still makes sense (i.e. either half of that change alone would not make sense).

One more quesestion regarding flags used in structures : for example: update_callback_data has a flags field (type int) , in function void update_callback() the field flags of that structure is used as param for add_file_to_index(..., data->flags) and this function is define as: int add_file_to_index(..., int flags) in read-cache.c The question is : If the flags field of the structure is used in function calls should i update flags that deep?(there are other cases where the field is used in nested calls )

On Sun, Apr 2, 2017 at 6:30 AM, Junio C Hamano <gitster@pobox.com> wrote:

Robert Stanca <robert.stanca7@gmail.com> writes:

[administrivia: please don't top-post around here].

There won't be any fast and clear rule and you'd need to grow and use common sense and good taste, but it probably is helpful to go back and think from the first principle, i.e. why you are doing this conversion.

For example, in your PATCH 2/2 that we are discussing, you updated the local variable "flags" to unsigned in show_bisect_vars(), because it receives the value from "info->flags", which is becoming an "unsigned" because it is a collection of independent bits.

The function uses this "flags" (now unsigned) twice, and one is to pass it to filter_skipped() as its 3rd parameter. This helper function takes "int", but you didn't update it to "unsigned". And you made the right decision to stop there.

The reason why it is the right place to stop is because the function does not use its 3rd parameter as a collection of bits; it wants its callers to give Yes/No there--anything non-zero is yes. Because you know "flags & BISECT_SHOW_ALL", which is unsigned, would be passed as a non-zero "int" to filter_skipped(), iff flags has that bit set, you know you do not have to touch that function and you stop there.

There will be similar places in the callchain that stop propagating the "collection-of-independent-bits"-ness. And that is where you would stop--because beyond that point there is no "arrgh, we use signed int to represent a collection of bits?" problem, which is what you are cleaning up.

Robert Stanca <robert.stanca7@gmail.com> writes:

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.

Overlong lines, without space after full-stop before the second sentence, without full-stop at the end of the sentence.

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.

Regarding the first part , i can resend those 2 patches rewriting the commit message if you want.

On Sat, Apr 1, 2017 at 10:12 PM, Junio C Hamano <gitster@pobox.com> wrote: