On 07-abr-2023 08:55:53, Junio C Hamano wrote:
> Rubén Justo <rjusto@xxxxxxxxx> 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.
> > 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).
>
> 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.
