Re: [PATCH] coccinelle: add and apply branch_get() rules

From: Rubén Justo <rjusto@gmail.com>
Date: Apr 7, 2023, 19:09 UTC
Message-ID: <376aca6d-1b09-9bf9-c258-81e8ed2443c2@gmail.com>
In-Reply-To: <xmqqjzynlm9i.fsf@gitster.g>

On 07-abr-2023 08:55:53, Junio C Hamano wrote:

Show 15 quoted lines

> Rubén Justo <rjusto@gmail.com> writes:
> 
> > There are three supported ways to obtain a "struct branch *" for the
> > currently checked out branch, in the current worktree, using the API
> > branch_get(): branch_get(NULL), branch_get("") and branch_get("HEAD").
> >
> > The first one is the recommended [1][2] and optimal usage.  Let's add
> > two coccinelle rules to convert the latter two into the first one.
> >
> >   1. f019d08ea6 (API documentation for remote.h, 2008-02-19)
> >
> >   2. d27eb356bf (remote: move doc to remote.h and refspec.h, 2019-11-17)
> 
> Citing commits in the past is not an optimal way to justify a
> recommendation, though.

Well, my intention is to state that the recommendation is not recent. Perhaps it is confusing to not state clearly that it is also current.

Show 20 quoted lines

> > diff --git a/contrib/coccinelle/branch_get.cocci b/contrib/coccinelle/branch_get.cocci
> > new file mode 100644
> > index 0000000000..3ec5b59723
> > --- /dev/null
> > +++ b/contrib/coccinelle/branch_get.cocci
> > @@ -0,0 +1,10 @@
> > +@@
> > +@@
> > +- branch_get("HEAD")
> > ++ branch_get(NULL)
> > +@@
> > +@@
> > +- branch_get("")
> > ++ branch_get(NULL)
> > +
> 
> I am not sure about these rules.  Noybody is passing "" to ask for
> HEAD in the current code.  Neither
> 
>     $ git log -S'branch_get("")'

I'm not sure if there is any path that might use "", but the consideration is there since introduced in cf818348f1 (Report information on branches from remote.h, 2007-09-10).

Show 6 quoted lines

> 
> shows anything.  The first one does modify existing calls, but there
> are many calls to branch_get() that pass a computed value in a
> strbuf or a variable.  Do we know they are not passing "HEAD" or ""?
> 
> Stepping back a bit.  What is the ultimate goal for this change?

Of course, as you pointed out, there are usages where a computed value is used, perhaps coming from the user, which might end up specifying "HEAD". Those usages of branch_get() are not considered here. Not even indirect ones.

Having said that, the goal in this change is to aid following, now and in the future, when using a literal with branch_get(), the recommendation we already have. Which, IMHO, is also the optimal usage.

As a collateral, we save some cycles; either at runtime, avoiding the if (!strcmp("HEAD", "HEAD")) to the user; or better, at compile time, saving the compiler from optimizing out that strcmp.

I have to admit I have this change in mind, not in the current form, but in the same direction, since my patches for builtin/branch.c, a few months ago. When, reviewing the use of branch_get() I was a bit confused.

Thanks.