{"thread":{"id":"41815","subject":"[PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","startedAt":"2016-03-24T19:41:07Z","lastAt":"2016-03-25T16:54:07Z","messageCount":10,"participants":["Motroni Igor","Stefan Beller","work","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"281711","messageId":"1458848469-12663-1-git-send-email-motroniii@gmail.com","threadId":"41815","inReplyTo":null,"subject":"[PATCH 0/2] GSoC student","fromName":"Motroni Igor","fromEmail":"motroniii@gmail.com","sentAt":"2016-03-24T19:41:07Z","receivedAt":"2016-03-24T19:41:07Z","isPatch":true,"sender":{"key":"motroniii@gmail.com","avatar":null},"body":" Hi! My name is Motroni Igor and I'm a Russian student who wants to apply for the GSoC developing some cool stuff for Git. As a microproject, I've made two little changes in my Git fork.\n\n Modified flag field type in rev_list_info struct in bisect.h. There is\n no need for flag field to be signed, as it is not supposed to be\n used as decimal.\n Just a minor commit to trigger Travis Ci build\n\n bisect.h | 2 +-\n notes.c | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\n-- \n2.5.0\n"},{"id":"281709","messageId":"1458848469-12663-2-git-send-email-motroniii@gmail.com","threadId":"41815","inReplyTo":"1458848469-12663-1-git-send-email-motroniii@gmail.com","subject":"[PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"Motroni Igor","fromEmail":"motroniii@gmail.com","sentAt":"2016-03-24T19:41:08Z","receivedAt":"2016-03-24T19:41:08Z","isPatch":true,"sender":{"key":"motroniii@gmail.com","avatar":null},"body":"From: Pontifik \n\nSigned-off-by: Pontifik \n---\n bisect.h | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/bisect.h b/bisect.h\nindex acd12ef..a979a7f 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.5.0\n"},{"id":"281710","messageId":"1458848469-12663-3-git-send-email-motroniii@gmail.com","threadId":"41815","inReplyTo":"1458848469-12663-1-git-send-email-motroniii@gmail.com","subject":"[PATCH 2/2] Just a minor commit to trigger Travis Ci build","fromName":"Motroni Igor","fromEmail":"motroniii@gmail.com","sentAt":"2016-03-24T19:41:09Z","receivedAt":"2016-03-24T19:41:09Z","isPatch":true,"sender":{"key":"motroniii@gmail.com","avatar":null},"body":"From: Pontifik \n\nSigned-off-by: Pontifik \n---\n notes.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 88cf474..6d7da1e 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -539,8 +539,8 @@ static unsigned char determine_fanout(struct int_node *tree, unsigned char n,\n \treturn fanout + 1;\n }\n \n-/* hex SHA1 + 19 * '/' + NUL */\n-#define FANOUT_PATH_MAX 40 + 19 + 1\n+/* hex SHA1 + 19 * '/' + NUL = 60 */\n+#define FANOUT_PATH_MAX 60\n \n static void construct_path_with_fanout(const unsigned char *sha1,\n \t\tunsigned char fanout, char *path)\n-- \n2.5.0\n"},{"id":"281713","messageId":"CAGZ79kZmzo+f9YF1K1oC2GfOrzdaojmrca7bUm3cBRoDreeA-g@mail.gmail.com","threadId":"41815","inReplyTo":"1458848469-12663-2-git-send-email-motroniii@gmail.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T19:51:24Z","receivedAt":"2016-03-24T19:51:24Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 24, 2016 at 12:41 PM, Motroni Igor wrote:\n> From: Pontifik \n\nHere is a good place to put reasoning for why this is a good idea.\nI see you have a long subject, so maybe we can shorten the first line\n(down to less than ~ 80 characters) and put the longer explanation\nhere.\n\nHow about:\n\n bisect: use unsigned for flag field\n\n The flags are usually used as a unsigned variable, because it makes\n bit operations easier to follow.\n\n\n\n>\n> Signed-off-by: Pontifik \n\n>From Documentation/SubmittingPatches:\n> Also notice that a real name is used in the Signed-off-by: line. Please\n> don't hide your real name.\n\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 acd12ef..a979a7f 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\nYou can also drop the int here and make it just\nunsigned.\n\n> int show_timestamp;\n> int hdr_termination;\n> const char *header_prefix;\n> --\n> 2.5.0\n>\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"},{"id":"281716","messageId":"56F44788.2050509@gmail.com","threadId":"41815","inReplyTo":"CAGZ79kZmzo+f9YF1K1oC2GfOrzdaojmrca7bUm3cBRoDreeA-g@mail.gmail.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"work","fromEmail":"motroniii@gmail.com","sentAt":"2016-03-24T20:01:12Z","receivedAt":"2016-03-24T20:01:12Z","isPatch":true,"sender":{"key":"motroniii@gmail.com","avatar":null},"body":"On 03/24/2016 10:51 PM, Stefan Beller wrote:\n> On Thu, Mar 24, 2016 at 12:41 PM, Motroni Igor wrote:\n>> From: Pontifik \n> Here is a good place to put reasoning for why this is a good idea.\n> I see you have a long subject, so maybe we can shorten the first line\n> (down to less than ~ 80 characters) and put the longer explanation\n> here.\n>\n> How about:\n>\n> bisect: use unsigned for flag field\n>\n> The flags are usually used as a unsigned variable, because it makes\n> bit operations easier to follow.\nYep, it's definitely a good idea to shorten subject in order to put more \nexplanations in body of a message.\n>\n>\n>> Signed-off-by: Pontifik \n> From Documentation/SubmittingPatches:\n>> Also notice that a real name is used in the Signed-off-by: line. Please\n>> don't hide your real name.\nYep, I didn't mean to hide my real name, but coming across lots of new \nstuff confused me in distinguishing whether I'm asked to enter real name \nor nickname :)\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 acd12ef..a979a7f 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> You can also drop the int here and make it just\n> unsigned.\nIn fact, I just wanted to keep this code clear to understand (as I see \nthis). Unsigned short is also unsigned, but a reader should know that \n\"unsigned\" type stands for \"unsigned int\". Anyway, I'll keep this in \nmind in future, thanks a lot.\n>> int show_timestamp;\n>> int hdr_termination;\n>> const char *header_prefix;\n>> --\n>> 2.5.0\n>>\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"},{"id":"281743","messageId":"CAPig+cQcviTXYAGBTATgYXoaPtvxxeOUtCGYkLyW5oTFvLjEUA@mail.gmail.com","threadId":"41815","inReplyTo":"56F44788.2050509@gmail.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-24T22:56:43Z","receivedAt":"2016-03-24T22:56:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 24, 2016 at 4:01 PM, work wrote:\n> On 03/24/2016 10:51 PM, Stefan Beller wrote:\n>> On Thu, Mar 24, 2016 at 12:41 PM, Motroni Igor \n>> wrote:\n>>> From: Pontifik \n>>\n>> Here is a good place to put reasoning for why this is a good idea.\n>> I see you have a long subject, so maybe we can shorten the first line\n>> (down to less than ~ 80 characters) and put the longer explanation\n>> here.\n>>\n>> How about:\n>>\n>> bisect: use unsigned for flag field\n>>\n>> The flags are usually used as a unsigned variable, because it makes\n>> bit operations easier to follow.\n>\n> Yep, it's definitely a good idea to shorten subject in order to put more\n> explanations in body of a message.\n\nIt is also very important is to explain that you audited all clients\nof this field and found that none of them treat the sign bit specially\n(for instance, by checking the value with '< 0' or some such),\ntherefore such a change is safe, in addition to making the code\nclearer.\n\nAs an example, a diligent reviewer may wonder why you changed this\nfield to 'unsigned' but not the 'flags' variable in\nrev-list.c:show_bisect_vars() to which this field is assigned. This is\nsomething your re-roll of the patch should take into consideration.\n\n>>> diff --git a/bisect.h b/bisect.h\n>>> index acd12ef..a979a7f 100644\n>>> --- a/bisect.h\n>>> +++ b/bisect.h\n>>> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct\n>>> commit_list *list,\n>>>\n>>> struct rev_list_info {\n>>> struct rev_info *revs;\n>>> - int flags;\n>>> + unsigned int flags;\n>>\n>> You can also drop the int here and make it just\n>> unsigned.\n>\n> In fact, I just wanted to keep this code clear to understand (as I see\n> this). Unsigned short is also unsigned, but a reader should know that\n> \"unsigned\" type stands for \"unsigned int\". Anyway, I'll keep this in mind in\n> future, thanks a lot.\n\nWhile it's true that 'unsigned short' is indeed never negative, the\nclueful reader should never accidentally interpret bare 'unsigned' as\nanything other than the native word size. However, I think what Stefan\nreally meant is that, in this code base, it is (somewhat) more common\nto declare these \"flags\" variables as 'unsigned' rather than 'unsigned\nint':\n\n % git grep -E 'unsigned\\s+flags' | wc -l\n 80\n % git grep -E 'unsigned\\s+int\\s+flags' | wc -l\n 57\n"},{"id":"281744","messageId":"CAGZ79kaF42yCFWdvqqDSAZbWmUo0UVJGbXCci1LF=ne+cixZ0A@mail.gmail.com","threadId":"41815","inReplyTo":"CAPig+cQcviTXYAGBTATgYXoaPtvxxeOUtCGYkLyW5oTFvLjEUA@mail.gmail.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-03-24T23:04:46Z","receivedAt":"2016-03-24T23:04:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Mar 24, 2016 at 3:56 PM, Eric Sunshine wrote:\n> On Thu, Mar 24, 2016 at 4:01 PM, work wrote:\n>> On 03/24/2016 10:51 PM, Stefan Beller wrote:\n>>> On Thu, Mar 24, 2016 at 12:41 PM, Motroni Igor \n>>> wrote:\n>>>> From: Pontifik \n>>>\n>>> Here is a good place to put reasoning for why this is a good idea.\n>>> I see you have a long subject, so maybe we can shorten the first line\n>>> (down to less than ~ 80 characters) and put the longer explanation\n>>> here.\n>>>\n>>> How about:\n>>>\n>>> bisect: use unsigned for flag field\n>>>\n>>> The flags are usually used as a unsigned variable, because it makes\n>>> bit operations easier to follow.\n>>\n>> Yep, it's definitely a good idea to shorten subject in order to put more\n>> explanations in body of a message.\n>\n> It is also very important is to explain that you audited all clients\n> of this field and found that none of them treat the sign bit specially\n> (for instance, by checking the value with '< 0' or some such),\n> therefore such a change is safe, in addition to making the code\n> clearer.\n>\n> As an example, a diligent reviewer may wonder why you changed this\n> field to 'unsigned' but not the 'flags' variable in\n> rev-list.c:show_bisect_vars() to which this field is assigned. This is\n> something your re-roll of the patch should take into consideration.\n>\n>>>> diff --git a/bisect.h b/bisect.h\n>>>> index acd12ef..a979a7f 100644\n>>>> --- a/bisect.h\n>>>> +++ b/bisect.h\n>>>> @@ -16,7 +16,7 @@ extern struct commit_list *filter_skipped(struct\n>>>> commit_list *list,\n>>>>\n>>>> struct rev_list_info {\n>>>> struct rev_info *revs;\n>>>> - int flags;\n>>>> + unsigned int flags;\n>>>\n>>> You can also drop the int here and make it just\n>>> unsigned.\n>>\n>> In fact, I just wanted to keep this code clear to understand (as I see\n>> this). Unsigned short is also unsigned, but a reader should know that\n>> \"unsigned\" type stands for \"unsigned int\". Anyway, I'll keep this in mind in\n>> future, thanks a lot.\n>\n> While it's true that 'unsigned short' is indeed never negative, the\n> clueful reader should never accidentally interpret bare 'unsigned' as\n> anything other than the native word size. However, I think what Stefan\n> really meant is that, in this code base, it is (somewhat) more common\n> to declare these \"flags\" variables as 'unsigned' rather than 'unsigned\n> int':\n>\n> % git grep -E 'unsigned\\s+flags' | wc -l\n> 80\n> % git grep -E 'unsigned\\s+int\\s+flags' | wc -l\n> 57\n\nThis is what I meant. (I did not give my grep terms as reasoning as they were\nnot as exactly as yours. Redoing the search using your more exact terms\nhowever makes me wonder if my advice was the right thing.\n\n% git grep -E 'unsigned\\s+int\\s+flags' ./*.h | wc -l\n16\n% git grep -E 'unsigned\\s+flags' ./*.h | wc -l\n17\n\nSo there is no pattern that the version without int is more common.\nMaybe my exposure to the code was accidentally in a way such that\nI ever only saw the version without int.\n"},{"id":"281775","messageId":"xmqqr3ez5bcf.fsf@gitster.mtv.corp.google.com","threadId":"41815","inReplyTo":"CAGZ79kaF42yCFWdvqqDSAZbWmUo0UVJGbXCci1LF=ne+cixZ0A@mail.gmail.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-03-25T06:18:56Z","receivedAt":"2016-03-25T06:18:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Maybe my exposure to the code was accidentally in a way such that\n> I ever only saw the version without int.\n\nThe older part of the code tends to spell flag words with \"unsigned\"\nwithout \"int\", which is primarily historical fault of mine.\n"},{"id":"281809","messageId":"56F534D0.7050104@gmail.com","threadId":"41815","inReplyTo":"xmqqr3ez5bcf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"work","fromEmail":"motroniii@gmail.com","sentAt":"2016-03-25T12:53:36Z","receivedAt":"2016-03-25T12:53:36Z","isPatch":true,"sender":{"key":"motroniii@gmail.com","avatar":null},"body":"Yep. Thanks for your remarks. I have made a bit more research about \nusing old rev_list_info struct (with signed int flag) and realized, that \nit doesn't appear in expressions, where using signed integer will differ \nfrom unsigned one.\nI'll take using 'unsigned' instead of 'unsigned int' in account, so if \nneeded, I can remake the patch in order to get it accepted.\n\nOn 03/25/2016 09:18 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> Maybe my exposure to the code was accidentally in a way such that\n>> I ever only saw the version without int.\n> The older part of the code tends to spell flag words with \"unsigned\"\n> without \"int\", which is primarily historical fault of mine.\n>\n"},{"id":"281828","messageId":"CAPig+cRVBMbCikO2aCC0wXXgB4_P+XMNvNQfk+jhPWcZUbLfdQ@mail.gmail.com","threadId":"41815","inReplyTo":"56F534D0.7050104@gmail.com","subject":"Re: [PATCH 1/2] Modified flag field type in rev_list_info struct in bisect.h. There is no need for flag field to be signed, as it is not supposed to be used as decimal.","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-03-25T16:54:07Z","receivedAt":"2016-03-25T16:54:07Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[please respond inline rather than top-posting]\n\nOn Fri, Mar 25, 2016 at 8:53 AM, work wrote:\n> On 03/25/2016 09:18 AM, Junio C Hamano wrote:\n>> Stefan Beller writes:\n>>> Maybe my exposure to the code was accidentally in a way such that\n>>> I ever only saw the version without int.\n>>\n>> The older part of the code tends to spell flag words with \"unsigned\"\n>> without \"int\", which is primarily historical fault of mine.\n>\n> Yep. Thanks for your remarks. I have made a bit more research about using\n> old rev_list_info struct (with signed int flag) and realized, that it\n> doesn't appear in expressions, where using signed integer will differ from\n> unsigned one.\n> I'll take using 'unsigned' instead of 'unsigned int' in account, so if\n> needed, I can remake the patch in order to get it accepted.\n\nIf I read Junio's response correctly, \"unsigned int\" is indeed\npreferred over \"unsigned\", so no need to change that part, but the\ncommit message needs improvement, and other reviewer comments should\nbe addressed.\n\nAnd, yes, the expectation is that you will re-roll the patch (one or\nmore times) in response to issues pointed out by reviewers. As a GSoC\napplicant, this is especially important since a big part of working on\nthis project is being responsive to review comments, and GSoC mentors\nwill examine your reviewer interaction when selecting applicants.\n"}]}