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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Apr 16, 2023, 13:56 UTC
Message-ID: <230416.86ildvsyt6.gmgdl@evledraar.gmail.com>
In-Reply-To: <4cb4b69c-bd14-dfbd-6d06-59a7cd7e8c94@gmail.com>

On Thu, Apr 06 2023, Rubén Justo wrote:

Show 10 quoted lines

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

Show 5 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?

Show 15 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()?

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