{"thread":{"id":"52181","subject":"[PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","startedAt":"2019-11-04T09:59:32Z","lastAt":"2019-11-07T12:49:51Z","messageCount":15,"participants":["Elia Pinto","SZEDER Gábor","Phillip Wood","Junio C Hamano","Johannes Schindelin","Danh Doan"],"isPatch":true,"patchVersion":1,"patchTotal":32},"messages":[{"id":"385372","messageId":"20191104095923.116086-1-gitter.spiros@gmail.com","threadId":"52181","inReplyTo":null,"subject":"[PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2019-11-04T09:59:21Z","receivedAt":"2019-11-04T09:59:32Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Fix the LGTM warning of the rule that finds uses of the assignment\noperator = in places where the equality operator == would\nmake more sense.\n\nSigned-off-by: Elia Pinto \n---\n ident.c | 13 ++++++++-----\n 1 file changed, 8 insertions(+), 5 deletions(-)\n\ndiff --git a/ident.c b/ident.c\nindex e666ee4e59..07f2f03b0a 100644\n--- a/ident.c\n+++ b/ident.c\n@@ -172,12 +172,15 @@ const char *ident_default_email(void)\n \t\t\tstrbuf_addstr(&git_default_email, email);\n \t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n \t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n-\t\t} else if ((email = query_user_email()) && email[0]) {\n-\t\t\tstrbuf_addstr(&git_default_email, email);\n-\t\t\tfree((char *)email);\n-\t\t} else\n-\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n+\t\t} else {\n+\t\t\temail = query_user_email();\n+\t\t\tif (email && email[0]) {\n+\t\t\t\tstrbuf_addstr(&git_default_email, email);\n+\t\t\t\tfree((char *)email);\n+\t\t\t} else\n+\t\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n \t\t\t\t &git_default_email, &default_email_is_bogus);\n+\t\t}\n \t\tstrbuf_trim(&git_default_email);\n \t}\n \treturn git_default_email.buf;\n-- \n2.24.0.rc0.467.g566ccdd3e4.dirty\n\n"},{"id":"385373","messageId":"20191104095923.116086-2-gitter.spiros@gmail.com","threadId":"52181","inReplyTo":"20191104095923.116086-1-gitter.spiros@gmail.com","subject":"[PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2019-11-04T09:59:22Z","receivedAt":"2019-11-04T09:59:46Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Fix the LGTM warning fired by the rule that finds code that could convert the result of an integer\nmultiplication to a larger type. Since the conversion applies after the multiplication,\narithmetic overflow may still occur.\n\nSigned-off-by: Elia Pinto \n---\n commit-graph.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/commit-graph.c b/commit-graph.c\nindex a0f868522b..0be15f1cd4 100644\n--- a/commit-graph.c\n+++ b/commit-graph.c\n@@ -1415,8 +1415,8 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \n \tchunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;\n \tchunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;\n-\tchunk_offsets[2] = chunk_offsets[1] + hashsz * ctx->commits.nr;\n-\tchunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * ctx->commits.nr;\n+\tchunk_offsets[2] = chunk_offsets[1] + (uint64_t)hashsz * ctx->commits.nr;\n+\tchunk_offsets[3] = chunk_offsets[2] + ((uint64_t)hashsz + 16) * ctx->commits.nr;\n \n \tnum_chunks = 3;\n \tif (ctx->num_extra_edges) {\n@@ -1426,7 +1426,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t}\n \tif (ctx->num_commit_graphs_after > 1) {\n \t\tchunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n-\t\t\t\t\t\thashsz * (ctx->num_commit_graphs_after - 1);\n+\t\t\t\t\t\t(uint64_t)hashsz * (ctx->num_commit_graphs_after - 1);\n \t\tnum_chunks++;\n \t}\n \n@@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n \t\t\t num_chunks);\n \t\tctx->progress = start_delayed_progress(\n \t\t\tprogress_title.buf,\n-\t\t\tnum_chunks * ctx->commits.nr);\n+\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n \t}\n \twrite_graph_chunk_fanout(f, ctx);\n \twrite_graph_chunk_oids(f, hashsz, ctx);\n-- \n2.24.0.rc0.467.g566ccdd3e4.dirty\n\n"},{"id":"385374","messageId":"20191104095923.116086-3-gitter.spiros@gmail.com","threadId":"52181","inReplyTo":"20191104095923.116086-1-gitter.spiros@gmail.com","subject":"[PATCH 32/32] date.c: fix code that may overflow 'int' before it is converted to 'time_t'","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2019-11-04T09:59:23Z","receivedAt":"2019-11-04T09:59:48Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Fix the LGTM warning fired by the rule that finds code that could convert the result of an integer\nmultiplication to a larger type. Since the conversion applies after the multiplication,\narithmetic overflow may still occur.\n\nSigned-off-by: Elia Pinto \n---\n date.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/date.c b/date.c\nindex 041db7db4e..b8dcbdbb0e 100644\n--- a/date.c\n+++ b/date.c\n@@ -1172,7 +1172,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm\n \twhile (tl->type) {\n \t\tint len = strlen(tl->type);\n \t\tif (match_string(date, tl->type) >= len-1) {\n-\t\t\tupdate_tm(tm, now, tl->length * *num);\n+\t\t\tupdate_tm(tm, now, (time_t)tl->length * *num);\n \t\t\t*num = 0;\n \t\t\t*touched = 1;\n \t\t\treturn end;\n-- \n2.24.0.rc0.467.g566ccdd3e4.dirty\n\n"},{"id":"385377","messageId":"20191104102612.GE4348@szeder.dev","threadId":"52181","inReplyTo":"20191104095923.116086-1-gitter.spiros@gmail.com","subject":"Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-11-04T10:26:13Z","receivedAt":"2019-11-04T10:26:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Nov 04, 2019 at 09:59:21AM +0000, Elia Pinto wrote:\n> Fix the LGTM warning of the rule that finds uses of the assignment\n\nWhat is an \"LGTM warning\"?\n\nI think including the actual compiler warning in the commit message\nwould be great.\n\n> operator = in places where the equality operator == would\n> make more sense.\n> \n> Signed-off-by: Elia Pinto \n> ---\n> ident.c | 13 ++++++++-----\n> 1 file changed, 8 insertions(+), 5 deletions(-)\n> \n> diff --git a/ident.c b/ident.c\n> index e666ee4e59..07f2f03b0a 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -172,12 +172,15 @@ const char *ident_default_email(void)\n> \t\t\tstrbuf_addstr(&git_default_email, email);\n> \t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> \t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> -\t\t} else if ((email = query_user_email()) && email[0]) {\n> -\t\t\tstrbuf_addstr(&git_default_email, email);\n> -\t\t\tfree((char *)email);\n> -\t\t} else\n> -\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n> +\t\t} else {\n> +\t\t\temail = query_user_email();\n> +\t\t\tif (email && email[0]) {\n> +\t\t\t\tstrbuf_addstr(&git_default_email, email);\n> +\t\t\t\tfree((char *)email);\n> +\t\t\t} else\n> +\t\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n> \t\t\t\t &git_default_email, &default_email_is_bogus);\n> +\t\t}\n> \t\tstrbuf_trim(&git_default_email);\n> \t}\n> \treturn git_default_email.buf;\n> -- \n> 2.24.0.rc0.467.g566ccdd3e4.dirty\n> \n"},{"id":"385393","messageId":"CA+EOSBncQa0h1of+R2BBSw=tSNFmSASi2V5EJPaRNEO_ctvbkw@mail.gmail.com","threadId":"52181","inReplyTo":"20191104102612.GE4348@szeder.dev","subject":"Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2019-11-04T13:55:47Z","receivedAt":"2019-11-04T13:56:01Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Il giorno lun 4 nov 2019 alle ore 11:26 SZEDER Gábor\n ha scritto:\n>\n> On Mon, Nov 04, 2019 at 09:59:21AM +0000, Elia Pinto wrote:\n> > Fix the LGTM warning of the rule that finds uses of the assignment\n>\n> What is an \"LGTM warning\"?\n>\n> I think including the actual compiler warning in the commit message\n> would be great.\nYes indeed. I thought I did it, do you think i can do better? Thanks\n\nhttps://lgtm.com/rules/2158670641/\n>\n> > operator = in places where the equality operator == would\n> > make more sense.\n> >\n> > Signed-off-by: Elia Pinto \n> > ---\n> > ident.c | 13 ++++++++-----\n> > 1 file changed, 8 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/ident.c b/ident.c\n> > index e666ee4e59..07f2f03b0a 100644\n> > --- a/ident.c\n> > +++ b/ident.c\n> > @@ -172,12 +172,15 @@ const char *ident_default_email(void)\n> > strbuf_addstr(&git_default_email, email);\n> > committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> > author_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> > - } else if ((email = query_user_email()) && email[0]) {\n> > - strbuf_addstr(&git_default_email, email);\n> > - free((char *)email);\n> > - } else\n> > - copy_email(xgetpwuid_self(&default_email_is_bogus),\n> > + } else {\n> > + email = query_user_email();\n> > + if (email && email[0]) {\n> > + strbuf_addstr(&git_default_email, email);\n> > + free((char *)email);\n> > + } else\n> > + copy_email(xgetpwuid_self(&default_email_is_bogus),\n> > &git_default_email, &default_email_is_bogus);\n> > + }\n> > strbuf_trim(&git_default_email);\n> > }\n> > return git_default_email.buf;\n> > --\n> > 2.24.0.rc0.467.g566ccdd3e4.dirty\n> >\n"},{"id":"385397","messageId":"98f12b0a-d16b-13e2-e413-9230d3d31783@gmail.com","threadId":"52181","inReplyTo":"CA+EOSBncQa0h1of+R2BBSw=tSNFmSASi2V5EJPaRNEO_ctvbkw@mail.gmail.com","subject":"Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-11-04T15:11:03Z","receivedAt":"2019-11-04T15:11:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elia\n\nOn 04/11/2019 13:55, Elia Pinto wrote:\n> Il giorno lun 4 nov 2019 alle ore 11:26 SZEDER Gábor\n> ha scritto:\n>>\n>> On Mon, Nov 04, 2019 at 09:59:21AM +0000, Elia Pinto wrote:\n>>> Fix the LGTM warning of the rule that finds uses of the assignment\n>>\n>> What is an \"LGTM warning\"?\n>>\n>> I think including the actual compiler warning in the commit message\n>> would be great.\n> Yes indeed. I thought I did it, do you think i can do better? Thanks\n> \n> https://lgtm.com/rules/2158670641/\n\nIt would have been helpful to explain that LGTM was a static analyser \nfor those of us who did not know. As far the patch is concerned I'm not \nconvinced it is an improvement. There are loads of places where git uses \nthis pattern (\"git grep 'if (([^=)]*=[^)]*)' | wc\" shows 212). So long \nas the assignment is inside its own set of parentheses it's safe and gcc \nwill warn if the parentheses are missing.\n\nBest Wishes\n\nPhillip\n\n>>\n>>> operator = in places where the equality operator == would\n>>> make more sense.\n>>>\n>>> Signed-off-by: Elia Pinto \n>>> ---\n>>> ident.c | 13 ++++++++-----\n>>> 1 file changed, 8 insertions(+), 5 deletions(-)\n>>>\n>>> diff --git a/ident.c b/ident.c\n>>> index e666ee4e59..07f2f03b0a 100644\n>>> --- a/ident.c\n>>> +++ b/ident.c\n>>> @@ -172,12 +172,15 @@ const char *ident_default_email(void)\n>>> strbuf_addstr(&git_default_email, email);\n>>> committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n>>> author_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n>>> - } else if ((email = query_user_email()) && email[0]) {\n>>> - strbuf_addstr(&git_default_email, email);\n>>> - free((char *)email);\n>>> - } else\n>>> - copy_email(xgetpwuid_self(&default_email_is_bogus),\n>>> + } else {\n>>> + email = query_user_email();\n>>> + if (email && email[0]) {\n>>> + strbuf_addstr(&git_default_email, email);\n>>> + free((char *)email);\n>>> + } else\n>>> + copy_email(xgetpwuid_self(&default_email_is_bogus),\n>>> &git_default_email, &default_email_is_bogus);\n>>> + }\n>>> strbuf_trim(&git_default_email);\n>>> }\n>>> return git_default_email.buf;\n>>> --\n>>> 2.24.0.rc0.467.g566ccdd3e4.dirty\n>>>\n"},{"id":"385418","messageId":"CA+EOSBkRF_dYMHD3LRhGfGemqa-AsEoYT04-dntWR-bcj1ZzdA@mail.gmail.com","threadId":"52181","inReplyTo":"98f12b0a-d16b-13e2-e413-9230d3d31783@gmail.com","subject":"Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"Elia Pinto","fromEmail":"gitter.spiros@gmail.com","sentAt":"2019-11-04T19:55:16Z","receivedAt":"2019-11-04T19:55:31Z","isPatch":true,"sender":{"key":"gitter.spiros@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158490?v=4"},"body":"Il giorno lun 4 nov 2019 alle ore 16:11 Phillip Wood\n ha scritto:\n>\n> Hi Elia\n>\n> On 04/11/2019 13:55, Elia Pinto wrote:\n> > Il giorno lun 4 nov 2019 alle ore 11:26 SZEDER Gábor\n> > ha scritto:\n> >>\n> >> On Mon, Nov 04, 2019 at 09:59:21AM +0000, Elia Pinto wrote:\n> >>> Fix the LGTM warning of the rule that finds uses of the assignment\n> >>\n> >> What is an \"LGTM warning\"?\n> >>\n> >> I think including the actual compiler warning in the commit message\n> >> would be great.\n> > Yes indeed. I thought I did it, do you think i can do better? Thanks\n> >\n> > https://lgtm.com/rules/2158670641/\n>\n> It would have been helpful to explain that LGTM was a static analyser\n> for those of us who did not know. As far the patch is concerned I'm not\n> convinced it is an improvement. There are loads of places where git uses\n> this pattern (\"git grep 'if (([^=)]*=[^)]*)' | wc\" shows 212). So long\n> as the assignment is inside its own set of parentheses it's safe and gcc\n> will warn if the parentheses are missing.\n>\nLGTM only expresses a warning (actually it signals a possible error)\nthat refers directly to this\nhttps://cwe.mitre.org/data/definitions/481.html. On which we can\nhardly disagree, beyond the authority of the source. The reason LGTM\nonly identifies this as an error, and not the various apparently\nanalogous cases in the code I don't know, but it would certainly be\ninteresting to find out.\n\nThanks for the review\n> Best Wishes\n>\n> Phillip\n>\n> >>\n> >>> operator = in places where the equality operator == would\n> >>> make more sense.\n> >>>\n> >>> Signed-off-by: Elia Pinto \n> >>> ---\n> >>> ident.c | 13 ++++++++-----\n> >>> 1 file changed, 8 insertions(+), 5 deletions(-)\n> >>>\n> >>> diff --git a/ident.c b/ident.c\n> >>> index e666ee4e59..07f2f03b0a 100644\n> >>> --- a/ident.c\n> >>> +++ b/ident.c\n> >>> @@ -172,12 +172,15 @@ const char *ident_default_email(void)\n> >>> strbuf_addstr(&git_default_email, email);\n> >>> committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> >>> author_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> >>> - } else if ((email = query_user_email()) && email[0]) {\n> >>> - strbuf_addstr(&git_default_email, email);\n> >>> - free((char *)email);\n> >>> - } else\n> >>> - copy_email(xgetpwuid_self(&default_email_is_bogus),\n> >>> + } else {\n> >>> + email = query_user_email();\n> >>> + if (email && email[0]) {\n> >>> + strbuf_addstr(&git_default_email, email);\n> >>> + free((char *)email);\n> >>> + } else\n> >>> + copy_email(xgetpwuid_self(&default_email_is_bogus),\n> >>> &git_default_email, &default_email_is_bogus);\n> >>> + }\n> >>> strbuf_trim(&git_default_email);\n> >>> }\n> >>> return git_default_email.buf;\n> >>> --\n> >>> 2.24.0.rc0.467.g566ccdd3e4.dirty\n> >>>\n"},{"id":"385514","messageId":"xmqqbltpwxyd.fsf@gitster-ct.c.googlers.com","threadId":"52181","inReplyTo":"20191104095923.116086-1-gitter.spiros@gmail.com","subject":"Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T02:16:58Z","receivedAt":"2019-11-06T02:17:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\nDid I miss the first 29 patches (with what I see in this patch, I\ndo not know if I want to see them immediately, though ;-))?\n\n> Fix the LGTM warning of the rule that finds uses of the assignment\n> operator = in places where the equality operator == would\n> make more sense.\n\nI know you did not mean that existing\n\n\t} else if ((email = query_user_email()) && email[0]) {\n\nbetter reads if it were written like so:\n\n\t} else if ((email == query_user_email()) && email[0]) {\n\nbut that is the only way how that sentence can be read (at least to\nme) without looking at what the patch actually does.\n\nAs \"email\" has already been assigned to at this point in the\ncodeflow, I agree that, to an eye that does not (and is not willing\nto spend cycles to) understand what the code is doing, the latter do\nlook more natural: \"If the value of the variable is the same as the\nreturn value of the query_user_email() function, and ...\". And if\n\"email\" were a simpler arithmetic type it would have been even more\n(iow, it is clear \"email\" is a character string from \"&& email[0]\",\nso it is unlikely that \"email == que()\" is what the user intended).\n\nSo I am somewhat sympathetic to the \"warnings\" here, but not all\nthat much, especially if squelching makes the codeflow harder to\nfollow by introducing otherwise unnecessary nesting levels (like\nthis patch did). I suspect that it might be possible to futher\nrestructure the code in such a way that we do not have to do an\nassignment in a conditional without making the code deeply nested,\nand that may perhaps be worth doing.\n\nBut the thing is, assignment in a cascading conditional is so useful\nin avoiding pointless nesting of the code (imagine a reverse patch\nof this one---which is easy to sell as cleaning-up and streamlining\nthe code).\n\nSo, I dunno.\n\n> Signed-off-by: Elia Pinto \n> ---\n> ident.c | 13 ++++++++-----\n> 1 file changed, 8 insertions(+), 5 deletions(-)\n>\n> diff --git a/ident.c b/ident.c\n> index e666ee4e59..07f2f03b0a 100644\n> --- a/ident.c\n> +++ b/ident.c\n> @@ -172,12 +172,15 @@ const char *ident_default_email(void)\n> \t\t\tstrbuf_addstr(&git_default_email, email);\n> \t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> \t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> -\t\t} else if ((email = query_user_email()) && email[0]) {\n> -\t\t\tstrbuf_addstr(&git_default_email, email);\n> -\t\t\tfree((char *)email);\n> -\t\t} else\n> -\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n> +\t\t} else {\n> +\t\t\temail = query_user_email();\n> +\t\t\tif (email && email[0]) {\n> +\t\t\t\tstrbuf_addstr(&git_default_email, email);\n> +\t\t\t\tfree((char *)email);\n> +\t\t\t} else\n> +\t\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n> \t\t\t\t &git_default_email, &default_email_is_bogus);\n> +\t\t}\n> \t\tstrbuf_trim(&git_default_email);\n> \t}\n> \treturn git_default_email.buf;\n"},{"id":"385516","messageId":"xmqq7e4dwxob.fsf@gitster-ct.c.googlers.com","threadId":"52181","inReplyTo":"20191104095923.116086-2-gitter.spiros@gmail.com","subject":"Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-06T02:23:00Z","receivedAt":"2019-11-06T02:23:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elia Pinto writes:\n\n> Fix the LGTM warning fired by the rule that finds code that could convert the result of an integer\n> multiplication to a larger type. Since the conversion applies after the multiplication,\n> arithmetic overflow may still occur.\n\nOverly long?\n\n> \tchunk_offsets[0] = 8 + (num_chunks + 1) * GRAPH_CHUNKLOOKUP_WIDTH;\n> \tchunk_offsets[1] = chunk_offsets[0] + GRAPH_FANOUT_SIZE;\n> -\tchunk_offsets[2] = chunk_offsets[1] + hashsz * ctx->commits.nr;\n> -\tchunk_offsets[3] = chunk_offsets[2] + (hashsz + 16) * ctx->commits.nr;\n> +\tchunk_offsets[2] = chunk_offsets[1] + (uint64_t)hashsz * ctx->commits.nr;\n> +\tchunk_offsets[3] = chunk_offsets[2] + ((uint64_t)hashsz + 16) * ctx->commits.nr;\n\nchunk_offsets[] is u64[], while hashsz and is uint and\nctx->commits.nr is int. OK.\n\n> @@ -1426,7 +1426,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> \t}\n> \tif (ctx->num_commit_graphs_after > 1) {\n> \t\tchunk_offsets[num_chunks + 1] = chunk_offsets[num_chunks] +\n> -\t\t\t\t\t\thashsz * (ctx->num_commit_graphs_after - 1);\n> +\t\t\t\t\t\t(uint64_t)hashsz * (ctx->num_commit_graphs_after - 1);\n\nLikewise.\n\n> @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> \t\t\t num_chunks);\n> \t\tctx->progress = start_delayed_progress(\n> \t\t\tprogress_title.buf,\n> -\t\t\tnum_chunks * ctx->commits.nr);\n> +\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n\nHmph, do we need this? I understand that the second parameter to\nthe callee is u64, so the caller needs to come up with u64 without\noverflow, but doesn't that automatically get promoted?\n\n> \t}\n> \twrite_graph_chunk_fanout(f, ctx);\n> \twrite_graph_chunk_oids(f, hashsz, ctx);\n"},{"id":"385585","messageId":"nycvar.QRO.7.76.6.1911061231420.46@tvgsbejvaqbjf.bet","threadId":"52181","inReplyTo":"xmqqbltpwxyd.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-06T11:32:58Z","receivedAt":"2019-11-06T11:33:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 6 Nov 2019, Junio C Hamano wrote:\n\n> Elia Pinto writes:\n>\n> Did I miss the first 29 patches (with what I see in this patch, I\n> do not know if I want to see them immediately, though ;-))?\n>\n> > Fix the LGTM warning of the rule that finds uses of the assignment\n> > operator = in places where the equality operator == would\n> > make more sense.\n>\n> I know you did not mean that existing\n>\n> \t} else if ((email = query_user_email()) && email[0]) {\n>\n> better reads if it were written like so:\n>\n> \t} else if ((email == query_user_email()) && email[0]) {\n>\n> but that is the only way how that sentence can be read (at least to\n> me) without looking at what the patch actually does.\n>\n> As \"email\" has already been assigned to at this point in the\n> codeflow, I agree that, to an eye that does not (and is not willing\n> to spend cycles to) understand what the code is doing, the latter do\n> look more natural: \"If the value of the variable is the same as the\n> return value of the query_user_email() function, and ...\". And if\n> \"email\" were a simpler arithmetic type it would have been even more\n> (iow, it is clear \"email\" is a character string from \"&& email[0]\",\n> so it is unlikely that \"email == que()\" is what the user intended).\n>\n> So I am somewhat sympathetic to the \"warnings\" here, but not all\n> that much, especially if squelching makes the codeflow harder to\n> follow by introducing otherwise unnecessary nesting levels (like\n> this patch did). I suspect that it might be possible to futher\n> restructure the code in such a way that we do not have to do an\n> assignment in a conditional without making the code deeply nested,\n> and that may perhaps be worth doing.\n>\n> But the thing is, assignment in a cascading conditional is so useful\n> in avoiding pointless nesting of the code (imagine a reverse patch\n> of this one---which is easy to sell as cleaning-up and streamlining\n> the code).\n>\n> So, I dunno.\n\nFor what it's worth, my reaction was exactly the same: I understand\nhow some developers might deem the assignment inside an `if ()`\ncondition undesirable, in Git's context I do strongly prefer the current\ncode over the version proposed in this patch.\n\nThanks,\nJohannes\n\n>\n> > Signed-off-by: Elia Pinto \n> > ---\n> > ident.c | 13 ++++++++-----\n> > 1 file changed, 8 insertions(+), 5 deletions(-)\n> >\n> > diff --git a/ident.c b/ident.c\n> > index e666ee4e59..07f2f03b0a 100644\n> > --- a/ident.c\n> > +++ b/ident.c\n> > @@ -172,12 +172,15 @@ const char *ident_default_email(void)\n> > \t\t\tstrbuf_addstr(&git_default_email, email);\n> > \t\t\tcommitter_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> > \t\t\tauthor_ident_explicitly_given |= IDENT_MAIL_GIVEN;\n> > -\t\t} else if ((email = query_user_email()) && email[0]) {\n> > -\t\t\tstrbuf_addstr(&git_default_email, email);\n> > -\t\t\tfree((char *)email);\n> > -\t\t} else\n> > -\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n> > +\t\t} else {\n> > +\t\t\temail = query_user_email();\n> > +\t\t\tif (email && email[0]) {\n> > +\t\t\t\tstrbuf_addstr(&git_default_email, email);\n> > +\t\t\t\tfree((char *)email);\n> > +\t\t\t} else\n> > +\t\t\t\tcopy_email(xgetpwuid_self(&default_email_is_bogus),\n> > \t\t\t\t &git_default_email, &default_email_is_bogus);\n> > +\t\t}\n> > \t\tstrbuf_trim(&git_default_email);\n> > \t}\n> > \treturn git_default_email.buf;\n>\n"},{"id":"385637","messageId":"20191107022347.GE6351@danh.dev","threadId":"52181","inReplyTo":"xmqq7e4dwxob.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-07T02:23:47Z","receivedAt":"2019-11-07T02:23:54Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2019-11-06 11:23:00 +0900, Junio C Hamano wrote:\n> > @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> > \t\t\t num_chunks);\n> > \t\tctx->progress = start_delayed_progress(\n> > \t\t\tprogress_title.buf,\n> > -\t\t\tnum_chunks * ctx->commits.nr);\n> > +\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n> \n> Hmph, do we need this? I understand that the second parameter to\n> the callee is u64, so the caller needs to come up with u64 without\n> overflow, but doesn't that automatically get promoted?\n\nNeither num_chunks nor ctx->commits.nr is promoted because both of\nthem are int. The result of `num_chunks * ctx->commits.nr' will be int\nand will be promoted to u64 to pass to caller.\n\n-- \nDanh\n"},{"id":"385648","messageId":"xmqqtv7gs6g3.fsf@gitster-ct.c.googlers.com","threadId":"52181","inReplyTo":"20191107022347.GE6351@danh.dev","subject":"Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-07T03:37:00Z","receivedAt":"2019-11-07T03:37:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Danh Doan writes:\n\n> On 2019-11-06 11:23:00 +0900, Junio C Hamano wrote:\n>> > @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n>> > \t\t\t num_chunks);\n>> > \t\tctx->progress = start_delayed_progress(\n>> > \t\t\tprogress_title.buf,\n>> > -\t\t\tnum_chunks * ctx->commits.nr);\n>> > +\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n>> \n>> Hmph, do we need this? I understand that the second parameter to\n>> the callee is u64, so the caller needs to come up with u64 without\n>> overflow, but doesn't that automatically get promoted?\n>\n> Neither num_chunks nor ctx->commits.nr is promoted because both of\n> them are int. The result of `num_chunks * ctx->commits.nr' will be int\n> and will be promoted to u64 to pass to caller.\n\nAh, yes. Thanks.\n\nThe commit title is about \"integer multiplication\", but can the same\nissue arise with addition and subtraction as well, by the way?\n\n\n"},{"id":"385649","messageId":"20191107040605.GA8096@danh.dev","threadId":"52181","inReplyTo":"xmqqtv7gs6g3.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Danh Doan","fromEmail":"congdanhqx@gmail.com","sentAt":"2019-11-07T04:06:05Z","receivedAt":"2019-11-07T04:06:11Z","isPatch":true,"sender":{"key":"congdanhqx@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42673067?v=4"},"body":"On 2019-11-07 12:37:00 +0900, Junio C Hamano wrote:\n> Danh Doan writes:\n> \n> > On 2019-11-06 11:23:00 +0900, Junio C Hamano wrote:\n> >> > @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> >> > \t\t\t num_chunks);\n> >> > \t\tctx->progress = start_delayed_progress(\n> >> > \t\t\tprogress_title.buf,\n> >> > -\t\t\tnum_chunks * ctx->commits.nr);\n> >> > +\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n> >> \n> >> Hmph, do we need this? I understand that the second parameter to\n> >> the callee is u64, so the caller needs to come up with u64 without\n> >> overflow, but doesn't that automatically get promoted?\n> >\n> > Neither num_chunks nor ctx->commits.nr is promoted because both of\n> > them are int. The result of `num_chunks * ctx->commits.nr' will be int\n> > and will be promoted to u64 to pass to caller.\n> \n> Ah, yes. Thanks.\n> \n> The commit title is about \"integer multiplication\", but can the same\n> issue arise with addition and subtraction as well, by the way?\n\nYes, the same issue will arise with all binary (and ternary) arithmetic operators\n(+, -, *, /, %, ^, &, |, <<, >> and ?:).\n\nIIRC, gcc doesn't have any warning for this kind of issue.\n\nMicrosoft Visual Studio (2017+) has C26451 for this.\nhttps://docs.microsoft.com/en-us/visualstudio/code-quality/c26451?view=vs-2017\nIf our friends at Microsoft could help, we can check the remaining one\nin our codebase.\n\n-- \nDanh\n"},{"id":"385701","messageId":"nycvar.QRO.7.76.6.1911071344050.46@tvgsbejvaqbjf.bet","threadId":"52181","inReplyTo":"20191107022347.GE6351@danh.dev","subject":"Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-07T12:45:23Z","receivedAt":"2019-11-07T12:45:48Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Danh,\n\nOn Thu, 7 Nov 2019, Danh Doan wrote:\n\n> On 2019-11-06 11:23:00 +0900, Junio C Hamano wrote:\n> > > @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> > > \t\t\t num_chunks);\n> > > \t\tctx->progress = start_delayed_progress(\n> > > \t\t\tprogress_title.buf,\n> > > -\t\t\tnum_chunks * ctx->commits.nr);\n> > > +\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n> >\n> > Hmph, do we need this? I understand that the second parameter to\n> > the callee is u64, so the caller needs to come up with u64 without\n> > overflow, but doesn't that automatically get promoted?\n>\n> Neither num_chunks nor ctx->commits.nr is promoted because both of\n> them are int. The result of `num_chunks * ctx->commits.nr' will be int\n> and will be promoted to u64 to pass to caller.\n\nIf you up-cast, maybe do the second operand so that nobody has to spend\ncycles trying to remember whether the cast or the arithmetic operand\nbind stronger? I.e. `num_chunks * (uint64_t)ctx->commits.nr`.\n\nCiao,\nDscho\n"},{"id":"385702","messageId":"nycvar.QRO.7.76.6.1911071345440.46@tvgsbejvaqbjf.bet","threadId":"52181","inReplyTo":"20191107040605.GA8096@danh.dev","subject":"Re: [PATCH 31/32] commit-graph.c: fix code that could convert the result of an integer multiplication to a larger type","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-11-07T12:49:30Z","receivedAt":"2019-11-07T12:49:51Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Danh,\n\nOn Thu, 7 Nov 2019, Danh Doan wrote:\n\n> On 2019-11-07 12:37:00 +0900, Junio C Hamano wrote:\n> > Danh Doan writes:\n> >\n> > > On 2019-11-06 11:23:00 +0900, Junio C Hamano wrote:\n> > >> > @@ -1454,7 +1454,7 @@ static int write_commit_graph_file(struct write_commit_graph_context *ctx)\n> > >> > \t\t\t num_chunks);\n> > >> > \t\tctx->progress = start_delayed_progress(\n> > >> > \t\t\tprogress_title.buf,\n> > >> > -\t\t\tnum_chunks * ctx->commits.nr);\n> > >> > +\t\t\t(uint64_t)num_chunks * ctx->commits.nr);\n> > >>\n> > >> Hmph, do we need this? I understand that the second parameter to\n> > >> the callee is u64, so the caller needs to come up with u64 without\n> > >> overflow, but doesn't that automatically get promoted?\n> > >\n> > > Neither num_chunks nor ctx->commits.nr is promoted because both of\n> > > them are int. The result of `num_chunks * ctx->commits.nr' will be int\n> > > and will be promoted to u64 to pass to caller.\n> >\n> > Ah, yes. Thanks.\n> >\n> > The commit title is about \"integer multiplication\", but can the same\n> > issue arise with addition and subtraction as well, by the way?\n>\n> Yes, the same issue will arise with all binary (and ternary) arithmetic operators\n> (+, -, *, /, %, ^, &, |, <<, >> and ?:).\n>\n> IIRC, gcc doesn't have any warning for this kind of issue.\n>\n> Microsoft Visual Studio (2017+) has C26451 for this.\n> https://docs.microsoft.com/en-us/visualstudio/code-quality/c26451?view=vs-2017\n> If our friends at Microsoft could help, we can check the remaining one\n> in our codebase.\n\nI am a bit busy right now, but it _was_ my hope that adding a Visual\nStudio job to our Azure Pipeline would enable everybody to perform tests\nlike this one.\n\nIn other words, I _think_ that you can add something like\n\n\t#pragma warning(enable: 26451)\n\nto `compat/msvc.h` and then open a PR at https://github.com/git/git, the\nAzure Pipeline should produce precisely what you want.\n\n(If I were you, I would also try to save some CO2 by ripping out all\njobs except the `vs_build` one from `azure-pipelines.yml`.)\n\nCiao,\nDscho\n"}]}