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

[Junio C Hamano <gitster@pobox.com>]

Rubén Justo <rjusto@gmail.com> writes:

> On 07-abr-2023 08:55:53, Junio C Hamano wrote:
>> 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.

No matter how long ago the recommendation was originally written, we
should by default consider that anything that appears in the current
set of sources is still current.

If it is stale and there is a better recommendation, you of course
are welcome to update it, and if you were writing such a patch, it
may make sense to explain the situation like:

    In the comment for "struct branch" in remote.h, we recommend to
    use branch_get(NULL) to find out the branch currently checked
    out.  This recommendation dates back to f019d08e (API
    documentation for remote.h, 2008-02-19) in a separate
    documentation but later moved by d27eb356 (remote: move doc to
    remote.h and refspec.h, 2019-11-17) to the current location.

    However, the recommendation is out of date because ...

or something.  But that is not what this patch is about, is it?

I do not think you really gain anything by showing that they date
back long time, without making your position clear between "yes, it
is a very long-standing tradition and majority of the existing code
conforms to it" and "this ancient recommendation is iffy, and I
think it should be updated".

What you need to justify this change is to say that the
recommendation _is_ current, anyway, so I do not know why you are
arguing against my suggestion to improve your proposed log message.

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

That is what I found problematic, because I do not think this
particular change will get us closer to the endgame of not feedling
"" or "HEAD", if ...

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

... it is the ultimate goal.  These Coccinelle rules would not help
us fish out existing callers that receive string "HEAD" from their
callers and pass them unmodified to call branch_get() and convert
them to pass NULL, for example.  And for doing something like that
and encode that into another set of Coccinelle rules, it would take
auditing more and more indirect callers that reach branch_get(), but
if we were doing that, we can do the "passing HEAD or empty is a
BUG" patch to protect the function from future callers mistakingly
passing "HEAD" or "" without Coccinelle rules that are rather costly
to the CI.

The above assumes that it is a good thing to declare that NULL is
the only permitted way to ask for the branch currently checked out.
I am a bit skeptical about that, though.