{"thread":{"id":"45585","subject":"[PATCH 1/2] [GSOC] Convert signed flags to unsigned","startedAt":"2017-04-01T15:31:05Z","lastAt":"2017-04-02T17:15:05Z","messageCount":9,"participants":["Robert Stanca","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"316008","messageId":"20170401153049.21400-1-robert.stanca7@gmail.com","threadId":"45585","inReplyTo":null,"subject":"[PATCH 1/2] [GSOC] Convert signed flags to unsigned","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T15:30:48Z","receivedAt":"2017-04-01T15:31:05Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":" 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)\n\nSigned-off-by: Robert Stanca \n---\n bisect.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.h b/bisect.h\nindex acd12ef80..a979a7f11 100644\n--- a/bisect.h\n+++ b/bisect.h\n@@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct commit_list *list,\n \n struct rev_list_info {\n \tstruct rev_info *revs;\n-\tint flags;\n+\tunsigned int flags;\n \tint show_timestamp;\n \tint hdr_termination;\n \tconst char *header_prefix;\n-- \n2.12.2.575.gb14f27f91.dirty\n\n"},{"id":"316009","messageId":"20170401153049.21400-2-robert.stanca7@gmail.com","threadId":"45585","inReplyTo":"20170401153049.21400-1-robert.stanca7@gmail.com","subject":"[PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T15:30:49Z","receivedAt":"2017-04-01T15:31:21Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":" 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)\n\nSigned-off-by: Robert Stanca \n---\n builtin/rev-list.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 0aa93d589..eb4af53ab 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -211,7 +211,7 @@ static void print_var_int(const char *var, int val)\n \n static int show_bisect_vars(struct rev_list_info *info, int reaches, int all)\n {\n-\tint cnt, flags = info->flags;\n+\tunsigned int cnt, flags = info->flags;\n \tchar hex[GIT_SHA1_HEXSZ + 1] = \"\";\n \tstruct commit_list *tried;\n \tstruct rev_info *revs = info->revs;\n-- \n2.12.2.575.gb14f27f91.dirty\n\n"},{"id":"316024","messageId":"xmqqy3vkt008.fsf@gitster.mtv.corp.google.com","threadId":"45585","inReplyTo":"20170401153049.21400-1-robert.stanca7@gmail.com","subject":"Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T19:12:55Z","receivedAt":"2017-04-01T19:13:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Stanca writes:\n\n> Subject: Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned\n\nTry\n\n git shortlog --no-merges -40\n\nto learn how commits are typically titled. And then imagine this\npatch makes into our codebase and appear in the output.\n\nCan you tell what this commit is about from that single line title?\nNo. You wouldn't even know that it is only touching bisect.h and\nnothing else.\n\nWhat do your readers want \"shortlog\" output to tell them about your\ncommit? What are the most important thing (other than giving you an\nexcuse to say \"I have completed a microproject and now I am eligible\nto apply to GSoC\" ;-)? Your proposed commit log message, especially\nits title, must be written to help future readers of the project\nhistory.\n\nPerhaps\n\n bisect.h: make flags field in rev_list_info unsigned\n\nwould help them better.\n\n> 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)\n\nOverlong lines, without space after full-stop before the second\nsentence, without full-stop at the end of the sentence.\n\n>\n> Signed-off-by: Robert Stanca \n> ---\n> bisect.h | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/bisect.h b/bisect.h\n> index acd12ef80..a979a7f11 100644\n> --- a/bisect.h\n> +++ b/bisect.h\n> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct commit_list *list,\n> \n> struct rev_list_info {\n> \tstruct rev_info *revs;\n> -\tint flags;\n> +\tunsigned int flags;\n\nHave you checked how this field is used? For example, there is this\nline somewhere in rev-list.c\n\n\tint cnt, flags = info->flags;\n\nand the reason why the code copies the value to a local variable\n\"flags\" must be because it is going to use it, just like it and\nother codepaths use info->flags, no? It makes the code inconsistent\nby leaving the local variable a signed int, I suspect.\n"},{"id":"316025","messageId":"xmqqtw68szz1.fsf@gitster.mtv.corp.google.com","threadId":"45585","inReplyTo":"20170401153049.21400-2-robert.stanca7@gmail.com","subject":"Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-01T19:13:38Z","receivedAt":"2017-04-01T19:13:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Stanca writes:\n\n> 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)\n>\n> Signed-off-by: Robert Stanca \n> ---\n\nOK. I would have squashed these two commits into one, though.\n"},{"id":"316026","messageId":"CAJYcaSNAB+1gth2NkTjrcBV9TXT9bRsBQhwOsQCmnudYz5bTmg@mail.gmail.com","threadId":"45585","inReplyTo":"xmqqtw68szz1.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T19:19:26Z","receivedAt":"2017-04-01T19:19:33Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":"I am used to 1change per patch so it's easier to redo specific\npatches...if there are small changes(10 lines max) can i send them as\n1 patch?\n\nOn Sat, Apr 1, 2017 at 10:13 PM, Junio C Hamano wrote:\n> Robert Stanca writes:\n>\n>> 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)\n>>\n>> Signed-off-by: Robert Stanca \n>> ---\n>\n> OK. I would have squashed these two commits into one, though.\n"},{"id":"316028","messageId":"CAJYcaSOgcmZs9WOAGDm0DXonZooq2Uymcu0xkhcvwv-W6tkv0g@mail.gmail.com","threadId":"45585","inReplyTo":"xmqqy3vkt008.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-01T19:30:15Z","receivedAt":"2017-04-01T19:30:21Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":"Regarding the first part , i can resend those 2 patches rewriting the\ncommit message if you want.\n\nOn Sat, Apr 1, 2017 at 10:12 PM, Junio C Hamano wrote:\n> Robert Stanca writes:\n>\n>> Subject: Re: [PATCH 1/2] [GSOC] Convert signed flags to unsigned\n>\n> Try\n>\n> git shortlog --no-merges -40\n>\n> to learn how commits are typically titled. And then imagine this\n> patch makes into our codebase and appear in the output.\n>\n> Can you tell what this commit is about from that single line title?\n> No. You wouldn't even know that it is only touching bisect.h and\n> nothing else.\n>\n> What do your readers want \"shortlog\" output to tell them about your\n> commit? What are the most important thing (other than giving you an\n> excuse to say \"I have completed a microproject and now I am eligible\n> to apply to GSoC\" ;-)? Your proposed commit log message, especially\n> its title, must be written to help future readers of the project\n> history.\n>\n> Perhaps\n>\n> bisect.h: make flags field in rev_list_info unsigned\n>\n> would help them better.\n>\n>> 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)\n>\n> Overlong lines, without space after full-stop before the second\n> sentence, without full-stop at the end of the sentence.\n>\n>>\n>> Signed-off-by: Robert Stanca \n>> ---\n>> bisect.h | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/bisect.h b/bisect.h\n>> index acd12ef80..a979a7f11 100644\n>> --- a/bisect.h\n>> +++ b/bisect.h\n>> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct commit_list *list,\n>>\n>> struct rev_list_info {\n>> struct rev_info *revs;\n>> - int flags;\n>> + unsigned int flags;\n>\n> Have you checked how this field is used? For example, there is this\n> line somewhere in rev-list.c\n>\n> int cnt, flags = info->flags;\n>\n> and the reason why the code copies the value to a local variable\n> \"flags\" must be because it is going to use it, just like it and\n> other codepaths use info->flags, no? It makes the code inconsistent\n> by leaving the local variable a signed int, I suspect.\n"},{"id":"316032","messageId":"xmqqlgrjtrjl.fsf@gitster.mtv.corp.google.com","threadId":"45585","inReplyTo":"CAJYcaSNAB+1gth2NkTjrcBV9TXT9bRsBQhwOsQCmnudYz5bTmg@mail.gmail.com","subject":"Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-02T03:30:22Z","receivedAt":"2017-04-02T03:30:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Stanca writes:\n\n> I am used to 1change per patch so it's easier to redo specific\n> patches...if there are small changes(10 lines max) can i send them as\n> 1 patch?\n\nIt's not number of lines. One line per patch does not make sense if\nyou need to make corresponding changes to two places, one line each,\nin order to make the end result consistent. If you change a type of\na structure field, and that field is assigned to a variable\nsomewhere, you would change the type of both that field and the\nvariable that receives its value at the same time in a single\ncommit, as that would be the logical unit of a smallest change that\nstill makes sense (i.e. either half of that change alone would not\nmake sense).\n\n\n\n"},{"id":"316055","messageId":"CAJYcaSNJ0un1RgM3DNX=EOez5zP=Rko+BUt5SMeyBTb20K21oQ@mail.gmail.com","threadId":"45585","inReplyTo":"xmqqlgrjtrjl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags","fromName":"Robert Stanca","fromEmail":"robert.stanca7@gmail.com","sentAt":"2017-04-02T13:18:56Z","receivedAt":"2017-04-02T13:19:03Z","isPatch":true,"sender":{"key":"robert.stanca7@gmail.com","avatar":null},"body":"One more quesestion regarding flags used in structures :\nfor example: update_callback_data has a flags field (type int) ,\nin function void update_callback() the field flags of that structure\nis used as param for add_file_to_index(..., data->flags)\nand this function is define as: int add_file_to_index(..., int flags)\nin read-cache.c\nThe question is : If the flags field of the structure is used in\nfunction calls should i update flags that deep?(there are other\ncases where the field is used in nested calls )\n\nOn Sun, Apr 2, 2017 at 6:30 AM, Junio C Hamano wrote:\n> Robert Stanca writes:\n>\n>> I am used to 1change per patch so it's easier to redo specific\n>> patches...if there are small changes(10 lines max) can i send them as\n>> 1 patch?\n>\n> It's not number of lines. One line per patch does not make sense if\n> you need to make corresponding changes to two places, one line each,\n> in order to make the end result consistent. If you change a type of\n> a structure field, and that field is assigned to a variable\n> somewhere, you would change the type of both that field and the\n> variable that receives its value at the same time in a single\n> commit, as that would be the logical unit of a smallest change that\n> still makes sense (i.e. either half of that change alone would not\n> make sense).\n>\n>\n>\n"},{"id":"316059","messageId":"xmqqmvbyspdl.fsf@gitster.mtv.corp.google.com","threadId":"45585","inReplyTo":"CAJYcaSNJ0un1RgM3DNX=EOez5zP=Rko+BUt5SMeyBTb20K21oQ@mail.gmail.com","subject":"Re: [PATCH 2/2] [GSOC] show_bisect_vars(): Use unsigned int instead of signed int for flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-04-02T17:14:46Z","receivedAt":"2017-04-02T17:15:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Robert Stanca writes:\n\n> The question is : If the flags field of the structure is used in\n> function calls should i update flags that deep?(there are other\n> cases where the field is used in nested calls )\n\n[administrivia: please don't top-post around here].\n\nThere won't be any fast and clear rule and you'd need to grow and\nuse common sense and good taste, but it probably is helpful to go\nback and think from the first principle, i.e. why you are doing this\nconversion.\n\nFor example, in your PATCH 2/2 that we are discussing, you updated\nthe local variable \"flags\" to unsigned in show_bisect_vars(),\nbecause it receives the value from \"info->flags\", which is becoming\nan \"unsigned\" because it is a collection of independent bits.\n\nThe function uses this \"flags\" (now unsigned) twice, and one is to\npass it to filter_skipped() as its 3rd parameter. This helper\nfunction takes \"int\", but you didn't update it to \"unsigned\". And\nyou made the right decision to stop there.\n\nThe reason why it is the right place to stop is because the function\ndoes not use its 3rd parameter as a collection of bits; it wants its\ncallers to give Yes/No there--anything non-zero is yes. Because you\nknow \"flags & BISECT_SHOW_ALL\", which is unsigned, would be passed\nas a non-zero \"int\" to filter_skipped(), iff flags has that bit set,\nyou know you do not have to touch that function and you stop there.\n\nThere will be similar places in the callchain that stop propagating\nthe \"collection-of-independent-bits\"-ness. And that is where you\nwould stop--because beyond that point there is no \"arrgh, we use\nsigned int to represent a collection of bits?\" problem, which is\nwhat you are cleaning up.\n\n\n"}]}