diff mbox series

[v2,01/32] rebase: 'make coccicheck' cleanup

Message ID 20190403113457.20399-2-pclouds@gmail.com (mailing list archive)
State New, archived
Headers show
Series nd/sha1-name-c-wo-the-repository updates | expand

Commit Message

Duy Nguyen April 3, 2019, 11:34 a.m. UTC 
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/rebase.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

Comments

SZEDER Gábor April 4, 2019, 5:25 p.m. UTC | #1 
On Wed, Apr 03, 2019 at 06:34:26PM +0700, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/rebase.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 77deebc65c..c064909329 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1592,8 +1592,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  				branch_name = options.head_name;
>  
>  		} else {
> -			free(options.head_name);
> -			options.head_name = NULL;
> +			FREE_AND_NULL(options.head_name);
>  			branch_name = "HEAD";
>  		}
>  		if (get_oid("HEAD", &options.orig_head))
> @@ -1793,7 +1792,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  	 * we just fast-forwarded.
>  	 */
>  	strbuf_reset(&msg);
> -	if (!oidcmp(&merge_base, &options.orig_head)) {
> +	if (oideq(&merge_base, &options.orig_head)) {
>  		printf(_("Fast-forwarded %s to %s.\n"),
>  			branch_name, options.onto_name);
>  		strbuf_addf(&msg, "rebase finished: %s onto %s",

You are already using Coccinelle v1.0.7, aren't you?

For some reason previous versions don't notice these two
transformations.  I have patches with these transformations lying
around here for some time now, but haven't submitted them yet, because
I don't really like the way I run Coccinelle v1.0.7 in our static
analysis CI build jobs [1]

Anyway, here are my commit messages for these transformations, please
feel free to re-use them:

  -- >8 --

Subject: builtin rebase: use oideq()

Use oideq() instead of !oidcmp(), as it is more idiomatic, and might
give the compiler more opportunities to optimize.

Patch generated with 'contrib/coccinelle/free.cocci' and Coccinelle
v1.0.7 (previous Coccinelle versions don't notice this).

  -- 8< --

Subject: builtin rebase: use FREE_AND_NULL
    
Use the macro FREE_AND_NULL to release memory allocated for
'head_name' and clear its pointer.
    
Patch generated with 'contrib/coccinelle/free.cocci' and Coccinelle
v1.0.7 (previous Coccinelle versions don't notice this).

  -- >8 --


[1] https://github.com/szeder/git/commits/travis-coccinelle
Duy Nguyen April 5, 2019, 9:26 a.m. UTC | #2 
On Fri, Apr 5, 2019 at 12:25 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Wed, Apr 03, 2019 at 06:34:26PM +0700, Nguyễn Thái Ngọc Duy wrote:
> > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> > ---
> >  builtin/rebase.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 77deebc65c..c064909329 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -1592,8 +1592,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >                               branch_name = options.head_name;
> >
> >               } else {
> > -                     free(options.head_name);
> > -                     options.head_name = NULL;
> > +                     FREE_AND_NULL(options.head_name);
> >                       branch_name = "HEAD";
> >               }
> >               if (get_oid("HEAD", &options.orig_head))
> > @@ -1793,7 +1792,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> >        * we just fast-forwarded.
> >        */
> >       strbuf_reset(&msg);
> > -     if (!oidcmp(&merge_base, &options.orig_head)) {
> > +     if (oideq(&merge_base, &options.orig_head)) {
> >               printf(_("Fast-forwarded %s to %s.\n"),
> >                       branch_name, options.onto_name);
> >               strbuf_addf(&msg, "rebase finished: %s onto %s",
>
> You are already using Coccinelle v1.0.7, aren't you?

No it's 1.0.5. I guess I should upgrade then.

> For some reason previous versions don't notice these two
> transformations.  I have patches with these transformations lying
> around here for some time now, but haven't submitted them yet, because
> I don't really like the way I run Coccinelle v1.0.7 in our static
> analysis CI build jobs [1]
>
> Anyway, here are my commit messages for these transformations, please
> feel free to re-use them:

I'm not sure if even more elaboration is needed. It looks so trivial
(and at least obvious to me). But if you or Junio (or anybody else)
insists, I'll resend with updated commit messages. Or I can drop this
patch too if you have more cocci patches to send soon.

>
>   -- >8 --
>
> Subject: builtin rebase: use oideq()
>
> Use oideq() instead of !oidcmp(), as it is more idiomatic, and might
> give the compiler more opportunities to optimize.
>
> Patch generated with 'contrib/coccinelle/free.cocci' and Coccinelle
> v1.0.7 (previous Coccinelle versions don't notice this).
>
>   -- 8< --
>
> Subject: builtin rebase: use FREE_AND_NULL
>
> Use the macro FREE_AND_NULL to release memory allocated for
> 'head_name' and clear its pointer.
>
> Patch generated with 'contrib/coccinelle/free.cocci' and Coccinelle
> v1.0.7 (previous Coccinelle versions don't notice this).
>
>   -- >8 --
>
>
> [1] https://github.com/szeder/git/commits/travis-coccinelle
>
SZEDER Gábor April 9, 2019, 10:58 a.m. UTC | #3 
On Fri, Apr 05, 2019 at 04:26:10PM +0700, Duy Nguyen wrote:
> On Fri, Apr 5, 2019 at 12:25 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Wed, Apr 03, 2019 at 06:34:26PM +0700, Nguyễn Thái Ngọc Duy wrote:
> > > Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> > > ---
> > >  builtin/rebase.c | 5 ++---
> > >  1 file changed, 2 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > > index 77deebc65c..c064909329 100644
> > > --- a/builtin/rebase.c
> > > +++ b/builtin/rebase.c
> > > @@ -1592,8 +1592,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> > >                               branch_name = options.head_name;
> > >
> > >               } else {
> > > -                     free(options.head_name);
> > > -                     options.head_name = NULL;
> > > +                     FREE_AND_NULL(options.head_name);
> > >                       branch_name = "HEAD";
> > >               }
> > >               if (get_oid("HEAD", &options.orig_head))
> > > @@ -1793,7 +1792,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
> > >        * we just fast-forwarded.
> > >        */
> > >       strbuf_reset(&msg);
> > > -     if (!oidcmp(&merge_base, &options.orig_head)) {
> > > +     if (oideq(&merge_base, &options.orig_head)) {
> > >               printf(_("Fast-forwarded %s to %s.\n"),
> > >                       branch_name, options.onto_name);
> > >               strbuf_addf(&msg, "rebase finished: %s onto %s",
> >
> > You are already using Coccinelle v1.0.7, aren't you?
> 
> No it's 1.0.5.

Oh, you are right, Coccinelle 1.0.5 does indeed find these.

When I saw that the self-built 1.0.7 found something that the
distro-shipped 1.0.4 didn't, I checked it with a self-built 1.0.6, and
as it didn't find these, either, I didn't bother with 1.0.5.  It seems
that it got fixed 1.0.5, then regressed in 1.0.6, to be fixed again in
1.0.7.

> I guess I should upgrade then.

I found 'make coccicheck' with 1.0.7 to be about 10-15% faster than
previous versions.
diff mbox series

Patch

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 77deebc65c..c064909329 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1592,8 +1592,7 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 				branch_name = options.head_name;
 
 		} else {
-			free(options.head_name);
-			options.head_name = NULL;
+			FREE_AND_NULL(options.head_name);
 			branch_name = "HEAD";
 		}
 		if (get_oid("HEAD", &options.orig_head))
@@ -1793,7 +1792,7 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 	 * we just fast-forwarded.
 	 */
 	strbuf_reset(&msg);
-	if (!oidcmp(&merge_base, &options.orig_head)) {
+	if (oideq(&merge_base, &options.orig_head)) {
 		printf(_("Fast-forwarded %s to %s.\n"),
 			branch_name, options.onto_name);
 		strbuf_addf(&msg, "rebase finished: %s onto %s",