On 16-abr-2023 15:56:56, Ævar Arnfjörð Bjarmason wrote:
>
> On Thu, Apr 06 2023, Rubén Justo wrote:
>
> > 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)
>
> I wondered why it is that we don't just make passing "HEAD" an error,
> and what I thought was the case is why: It's because we use this API
> both for "internal" callers like what you modify below, but also for
> passing e.g. a "HEAD" as an argv element directly to the API, and don't
> want every command-line interface to hardcode the "HEAD" == NULL. So
> that makes sense.
>
> But do we need to support "" at all? changing branch_get() so that we do:
>
> if (name && !*name)
> BUG("pass NULL, not \"\"");
>
> Passes all our tests, but perhaps we have insufficient coverage.
I haven't found a use case where it is used, but the consideration is
there since it was introduced in cf818348f1 (Report information on
branches from remote.h, 2007-09-10).
We can introduce that BUG() and see what happens. But I'm not sure the
change is worth the potential noise.
>
> > Signed-off-by: Rubén Justo <rjusto@xxxxxxxxx>
> > ---
> > builtin/fetch.c | 2 +-
> > builtin/pull.c | 8 ++++----
> > contrib/coccinelle/branch_get.cocci | 10 ++++++++++
>
> We've typically named these rules after the API itself, in this case
> this is in remote.c, maybe we can just add a remote.cocci?
Is the new rule worth the cost in the CI? That's what I'm thinking
about now. Maybe adding the rule to the message for future reference is
enough.
>
> > 3 files changed, 15 insertions(+), 5 deletions(-)
> > create mode 100644 contrib/coccinelle/branch_get.cocci
> >
> > diff --git a/builtin/fetch.c b/builtin/fetch.c
> > index 7221e57f35..45d81c8e02 100644
> > --- a/builtin/fetch.c
> > +++ b/builtin/fetch.c
> > @@ -1738,7 +1738,7 @@ static int do_fetch(struct transport *transport,
> > commit_fetch_head(&fetch_head);
> >
> > if (set_upstream) {
> > - struct branch *branch = branch_get("HEAD");
> > + struct branch *branch = branch_get(NULL);
> > struct ref *rm;
> > struct ref *source_ref = NULL;
>
> I wonder if we shouldn't just change all of thes to a new inline helper
> with a more obvious name, perhaps current_branch()?
I had the same idea and explored it a bit. I ended up thinking that I
was introducing a _new_ way of using the API. So, I dunno.
> > 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)
> > +
>
> You don't need this duplication, see
> contrib/coccinelle/the_repository.cocci.
>
> I think this should do the trick, although it's untested:
>
> @@
> @@
> branch_get(
> (
> - "HEAD"
> + NULL
> |
> - ""
> + NULL
> )
> )
>
> A rule structured like that makes it clear that we're not changing the
> name, but just the argument.
>
Thanks.
