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

From: Rubén Justo <rjusto@gmail.com>
Date: Apr 16, 2023, 23:26 UTC
Message-ID: <c544be9f-896b-29d5-be26-fb7bcd0f2fc5@gmail.com>
In-Reply-To: <230416.86ildvsyt6.gmgdl@evledraar.gmail.com>

On 16-abr-2023 15:56:56, Ævar Arnfjörð Bjarmason wrote:

Show 27 quoted lines

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

Show 9 quoted lines

> 
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> >  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.

Show 19 quoted lines

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

Show 37 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)
> > +
> 
> 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.