public inbox for git@vger.kernel.org 
 help / color / mirror / Atom feed

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail•com>
To: "Rubén Justo" <rjusto@gmail•com>
Cc: Git List <git@vger•kernel.org>
Subject: Re: [PATCH] coccinelle: add and apply branch_get() rules
Date: Sun, 16 Apr 2023 15:56:56 +0200	[thread overview]
Message-ID: <230416.86ildvsyt6.gmgdl@evledraar.gmail.com> (raw)
In-Reply-To: <4cb4b69c-bd14-dfbd-6d06-59a7cd7e8c94@gmail.com>


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.

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

>  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()?
> 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.

next prev parent reply	other threads:[~2023-04-16 14:10 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-04-06 20:34 [PATCH] coccinelle: add and apply branch_get() rules Rubén Justo
2023-04-07 15:55 ` Junio C Hamano
2023-04-07 16:05   ` Junio C Hamano
2023-04-07 19:09   ` Rubén Justo
2023-04-08 22:45     ` Junio C Hamano
2023-04-09  7:43       ` Rubén Justo
2023-04-16 13:56 ` Ævar Arnfjörð Bjarmason [this message]
2023-04-16 23:26   ` Rubén Justo
2023-04-22 22:27 ` [PATCH v2] follow usage recommendations for branch_get() Rubén Justo

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=230416.86ildvsyt6.gmgdl@evledraar.gmail.com \
    --to=avarab@gmail$(echo .)com \
    --cc=git@vger$(echo .)kernel.org \
    --cc=rjusto@gmail$(echo .)com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox