Re: [PATCH 30/32] ident.c: fix LGTM warning on the possible abuse of the '=' operator

From: Elia Pinto <gitter.spiros@gmail.com>
Date: Nov 4, 2019, 19:55 UTC
Message-ID: <CA+EOSBkRF_dYMHD3LRhGfGemqa-AsEoYT04-dntWR-bcj1ZzdA@mail.gmail.com>
In-Reply-To: <98f12b0a-d16b-13e2-e413-9230d3d31783@gmail.com>

Il giorno lun 4 nov 2019 alle ore 16:11 Phillip Wood <phillip.wood123@gmail.com> ha scritto:

Show 25 quoted lines

>
> Hi Elia
>
> On 04/11/2019 13:55, Elia Pinto wrote:
> > Il giorno lun 4 nov 2019 alle ore 11:26 SZEDER Gábor
> > <szeder.dev@gmail.com> ha scritto:
> >>
> >> On Mon, Nov 04, 2019 at 09:59:21AM +0000, Elia Pinto wrote:
> >>> Fix the LGTM warning of the rule that finds uses of the assignment
> >>
> >> What is an "LGTM warning"?
> >>
> >> I think including the actual compiler warning in the commit message
> >> would be great.
> > Yes indeed. I thought I did it, do you think i can do better? Thanks
> >
> > https://lgtm.com/rules/2158670641/
>
> It would have been helpful to explain that LGTM was a static analyser
> for those of us who did not know. As far the patch is concerned I'm not
> convinced it is an improvement. There are loads of places where git uses
> this pattern ("git grep 'if (([^=)]*=[^)]*)' | wc" shows 212). So long
> as the assignment is inside its own set of parentheses it's safe and gcc
> will warn if the parentheses are missing.
>

LGTM only expresses a warning (actually it signals a possible error) that refers directly to this https://cwe.mitre.org/data/definitions/481.html. On which we can hardly disagree, beyond the authority of the source. The reason LGTM only identifies this as an error, and not the various apparently analogous cases in the code I don't know, but it would certainly be interesting to find out.

Thanks for the review

Show 41 quoted lines

> Best Wishes
>
> Phillip
>
> >>
> >>> operator = in places where the equality operator == would
> >>> make more sense.
> >>>
> >>> Signed-off-by: Elia Pinto <gitter.spiros@gmail.com>
> >>> ---
> >>>   ident.c | 13 ++++++++-----
> >>>   1 file changed, 8 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/ident.c b/ident.c
> >>> index e666ee4e59..07f2f03b0a 100644
> >>> --- a/ident.c
> >>> +++ b/ident.c
> >>> @@ -172,12 +172,15 @@ const char *ident_default_email(void)
> >>>                        strbuf_addstr(&git_default_email, email);
> >>>                        committer_ident_explicitly_given |= IDENT_MAIL_GIVEN;
> >>>                        author_ident_explicitly_given |= IDENT_MAIL_GIVEN;
> >>> -             } else if ((email = query_user_email()) && email[0]) {
> >>> -                     strbuf_addstr(&git_default_email, email);
> >>> -                     free((char *)email);
> >>> -             } else
> >>> -                     copy_email(xgetpwuid_self(&default_email_is_bogus),
> >>> +             } else {
> >>> +                     email = query_user_email();
> >>> +                     if (email && email[0]) {
> >>> +                             strbuf_addstr(&git_default_email, email);
> >>> +                             free((char *)email);
> >>> +                     } else
> >>> +                             copy_email(xgetpwuid_self(&default_email_is_bogus),
> >>>                                   &git_default_email, &default_email_is_bogus);
> >>> +             }
> >>>                strbuf_trim(&git_default_email);
> >>>        }
> >>>        return git_default_email.buf;
> >>> --
> >>> 2.24.0.rc0.467.g566ccdd3e4.dirty
> >>>