Re: [PATCH v2] var: add GIT_DEFAULT_BRANCH variable

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 3, 2021, 18:21 UTC
Message-ID: <xmqqa6il6qxu.fsf@gitster.g>
In-Reply-To: <20211102164434.1005707-1-thomas@t-8ch.de>

Thomas Weißschuh <thomas@t-8ch.de> writes:

> Introduce the builtin variable GIT_DEFAULT_BRANCH which represents the

"builtin" -> "logical", as that is how "git-var" describes these things.

It is totally outside the scope of this patch, but I think we'd better think of a way to make it clear to the readers of the documentation that it would not do anything if they did something like:

    $ GIT_DEFAULT_BRANCH=foobar git init

I say this is outside the scope because there are other existing logical variables that are different from the environment variables that can affect the behaviour of git.

> the default branch name that will be used by git-init.

"git-init" -> "git init", or inside a pair of backquotes, i.e. "`git init`".

Show 8 quoted lines

> Currently this variable is equivalent to
>     git config init.defaultbranch || 'master'
>
> This however will break if at one point the default branch is changed as
> indicated by `default_branch_name_advice` in `refs.c`.
>
> By providing this command ahead of time users of git can make their
> code forward-compatible.

Makes sense.  Thanks for cleanly explaining the motivation.

> Co-developed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>

I would use "Helped-by:" here, as I do not want to see one-off trailers invented left and right.

Show 19 quoted lines

> diff --git a/builtin/var.c b/builtin/var.c
> index 6c6f46b4ae..d1d82b6c93 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -5,6 +5,7 @@
>   */
>  #include "builtin.h"
>  #include "config.h"
> +#include "refs.h"
>  
>  static const char var_usage[] = "git var (-l | <variable>)";
>  
> @@ -27,6 +28,17 @@ static const char *pager(int flag)
>  	return pgm;
>  }
>  
> +static const char *default_branch(int flag)
> +{
> +	const char *name = repo_default_branch_name(the_repository, 1);

Calling

        git_default_branch_name(1)

is much shorter and clear. It's not like using the_repository is always better. For a single and simple purpose command like "git var" that does not run around multiple repositories and do things in them, sticking to the "we work in _the_ repository given to us" simple API is better.

> +	if (!name)
> +		die("could not determine the default branch name");
> +
> +	return name;

Should we even die? What does "init" and "clone" do when they ask for the same information and get a NULL pointer?

    ... goes and looks ...

They know the call cannot fail that way.  So I would do either

 (1) follow suit and just return whatever we get back from the API
     call to the caller (which knows how to handle a NULL return); or

 (2) call BUG("...")  instead of die().  The name being NULL at this
     point means that git_default_branch_name() returned NULL, which
     the callers do not allow to happen, so it is a BUG for it to
     return NULL, and this caller noticed it.

I only raise the latter as a possibility. I think just assuming that name is never NULL like other callers is fine.

Thanks.