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