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

Message ID 20191104095923.116086-1-gitter.spiros@gmail.com
State New
Headers show
Series
  • Untitled series #197375
Related show

Commit Message

Elia Pinto Nov. 4, 2019, 9:59 a.m. UTC
Fix the LGTM warning of the rule that finds uses of the assignment
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(-)

Comments

SZEDER Gábor Nov. 4, 2019, 10:26 a.m. UTC | #1
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.

> 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
>
Elia Pinto Nov. 4, 2019, 1:55 p.m. UTC | #2
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/
>
> > 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
> >
Phillip Wood Nov. 4, 2019, 3:11 p.m. UTC | #3
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.

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
>>>
Elia Pinto Nov. 4, 2019, 7:55 p.m. UTC | #4
Il giorno lun 4 nov 2019 alle ore 16:11 Phillip Wood
<phillip.wood123@gmail.com> ha scritto:
>
> 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
> 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
> >>>
Junio C Hamano Nov. 6, 2019, 2:16 a.m. UTC | #5
Elia Pinto <gitter.spiros@gmail.com> writes:

Did I miss the first 29 patches (with what I see in this patch, I
do not know if I want to see them immediately, though ;-))?

> Fix the LGTM warning of the rule that finds uses of the assignment
> operator = in places where the equality operator == would
> make more sense.

I know you did not mean that existing

	} else if ((email = query_user_email()) && email[0]) {

better reads if it were written like so:

	} else if ((email == query_user_email()) && email[0]) {

but that is the only way how that sentence can be read (at least to
me) without looking at what the patch actually does.

As "email" has already been assigned to at this point in the
codeflow, I agree that, to an eye that does not (and is not willing
to spend cycles to) understand what the code is doing, the latter do
look more natural: "If the value of the variable is the same as the
return value of the query_user_email() function, and ...".  And if
"email" were a simpler arithmetic type it would have been even more
(iow, it is clear "email" is a character string from "&& email[0]",
so it is unlikely that "email == que()" is what the user intended).

So I am somewhat sympathetic to the "warnings" here, but not all
that much, especially if squelching makes the codeflow harder to
follow by introducing otherwise unnecessary nesting levels (like
this patch did).  I suspect that it might be possible to futher
restructure the code in such a way that we do not have to do an
assignment in a conditional without making the code deeply nested,
and that may perhaps be worth doing.

But the thing is, assignment in a cascading conditional is so useful
in avoiding pointless nesting of the code (imagine a reverse patch
of this one---which is easy to sell as cleaning-up and streamlining
the code).

So, I dunno.

> 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;
Johannes Schindelin Nov. 6, 2019, 11:32 a.m. UTC | #6
Hi,

On Wed, 6 Nov 2019, Junio C Hamano wrote:

> Elia Pinto <gitter.spiros@gmail.com> writes:
>
> Did I miss the first 29 patches (with what I see in this patch, I
> do not know if I want to see them immediately, though ;-))?
>
> > Fix the LGTM warning of the rule that finds uses of the assignment
> > operator = in places where the equality operator == would
> > make more sense.
>
> I know you did not mean that existing
>
> 	} else if ((email = query_user_email()) && email[0]) {
>
> better reads if it were written like so:
>
> 	} else if ((email == query_user_email()) && email[0]) {
>
> but that is the only way how that sentence can be read (at least to
> me) without looking at what the patch actually does.
>
> As "email" has already been assigned to at this point in the
> codeflow, I agree that, to an eye that does not (and is not willing
> to spend cycles to) understand what the code is doing, the latter do
> look more natural: "If the value of the variable is the same as the
> return value of the query_user_email() function, and ...".  And if
> "email" were a simpler arithmetic type it would have been even more
> (iow, it is clear "email" is a character string from "&& email[0]",
> so it is unlikely that "email == que()" is what the user intended).
>
> So I am somewhat sympathetic to the "warnings" here, but not all
> that much, especially if squelching makes the codeflow harder to
> follow by introducing otherwise unnecessary nesting levels (like
> this patch did).  I suspect that it might be possible to futher
> restructure the code in such a way that we do not have to do an
> assignment in a conditional without making the code deeply nested,
> and that may perhaps be worth doing.
>
> But the thing is, assignment in a cascading conditional is so useful
> in avoiding pointless nesting of the code (imagine a reverse patch
> of this one---which is easy to sell as cleaning-up and streamlining
> the code).
>
> So, I dunno.

For what it's worth, my reaction was exactly the same: I understand
how some developers might deem the assignment inside an `if ()`
condition undesirable, in Git's context I do strongly prefer the current
code over the version proposed in this patch.

Thanks,
Johannes

>
> > 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;
>

Patch
diff mbox series

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;