Rubén Justo <rjusto@xxxxxxxxx> writes:
> 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)
Citing commits in the past is not an optimal way to justify a
recommendation, though. It does not show that these recommendations
made earlier are still current. Phrasing it this way
Among them, the comment in remote.h for "struct branch"
recommends branch_get(NULL) for HEAD.
might be a way to say that it comes from the current source, and
when future readers of "git log" stumbles on this commit, they will
also understand why the change was made.
> 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)
> +
I am not sure about these rules. Noybody is passing "" to ask for
HEAD in the current code. Neither
$ git log -S'branch_get("")'
shows anything. The first one does modify existing calls, but there
are many calls to branch_get() that pass a computed value in a
strbuf or a variable. Do we know they are not passing "HEAD" or ""?
Stepping back a bit. What is the ultimate goal for this change?
Are we going to insist that the currently checked out branch MUST be
asked for by passing NULL and not "" or "HEAD" to branch_get()? If
that is the goal, then it almost makes me wonder if we should just
do the attached patch, plus your changes to the callers, without
adding any new Coccinelle rules, and finding and fixing the fallouts
by inspecting all the call graph that leads to branch_get().
If that is not the goal, and we will keep acepting NULL, "", and "HEAD"
as equivalents, the value of updating callers with literal "HEAD" to
pass NULL is rather dubious.
To be fair, I do not think we would not see if "branch_get(NULL)
must be the only way to ask for the current branch" is a good goal,
until we at least try a little. Maybe during such a conversion that
starts by erroring when the function is called with "HEAD" or ""
(attached below), we might find that we need to change an existing
caller (or many of them) to do something silly like this:
return_type a_caller(const char *branch_name, ...)
{
- struct branch *branch = branch_get(branch_name);
+ struct branch *branch;
+
+ if (branch_name &&
+ (!strcmp(branch_name, "HEAD") || !*branch_name))
+ branch = branch_get(NULL);
+ else
+ branch = branch_get(branch_name);
... use branch_name ...
... use branch ...
in which case we may start doubtint the goal to always use NULL for
"HEAD".
So, I dunno. The patch does not make anything worse (other than
adding two extra Coccinelle rules), but to me, the ultimate goal
("where does it want to take us") is unclear.
remote.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git i/remote.c w/remote.c
index 3a831cb530..3788dd3fa0 100644
--- i/remote.c
+++ w/remote.c
@@ -1829,8 +1829,10 @@ struct branch *branch_get(const char *name)
struct branch *ret;
read_config(the_repository);
- if (!name || !*name || !strcmp(name, "HEAD"))
+ if (!name)
ret = the_repository->remote_state->current_branch;
+ else if (!*name || !strcmp(name, "HEAD"))
+ BUG("use NULL for HEAD to call branch_get()");
else
ret = make_branch(the_repository->remote_state, name,
strlen(name));
