Re: [PATCH v2] var: add GIT_DEFAULT_BRANCH variable

Thomas Weißschuh [off-list ref] 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`".

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

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

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.

Introduce the logical variable GIT_DEFAULT_BRANCH which represents the
the default branch name that will be used by "git init".

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.

Helped-by: Johannes Schindelin [off-list ref]
Signed-off-by: Thomas Weißschuh <redacted>
---

Changes from v1 ( https://lore.kernel.org/git/20211030140112.834650-1-thomas@t-8ch.de/ ):
* Replaced the custom subcommand with an internal variable
* Cleaned up the tests

Changes from v2 ( https://lore.kernel.org/git/20211102164434.1005707-1-thomas@t-8ch.de/ ):
* Removed superfluous error handling
* Switched to better fitting function
* Reworded commit message to be more consistent with the rest of git

 Documentation/git-var.txt |  3 +++
 builtin/var.c             |  7 +++++++
 t/t0007-git-var.sh        | 19 +++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 6072f936ab..387cc1b914 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt

@@ -59,6 +59,9 @@ ifdef::git-default-pager[]
     The build you are using chose '{git-default-pager}' as the default.
 endif::git-default-pager[]
 
+GIT_DEFAULT_BRANCH::
+    The name of the first branch created in newly initialized repositories.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]

diff --git a/builtin/var.c b/builtin/var.c
index 6c6f46b4ae..491db27429 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,11 @@ static const char *pager(int flag)
 	return pgm;
 }
 
+static const char *default_branch(int flag)
+{
+	return git_default_branch_name(1);
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);

@@ -36,6 +42,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
 	{ "GIT_PAGER", pager },
+	{ "GIT_DEFAULT_BRANCH", default_branch },
 	{ "", NULL },
 };
 

diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 53af92d571..6b6852e35e 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh

@@ -27,6 +27,25 @@ test_expect_success !FAIL_PREREQS,!AUTOIDENT 'requested identities are strict' '
 	)
 '
 
+test_expect_success 'get GIT_DEFAULT_BRANCH without configuration' '
+	(
+		sane_unset GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+		echo master >expect &&
+		git var GIT_DEFAULT_BRANCH >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
+	test_config init.defaultbranch foo &&
+	(
+		sane_unset GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+		echo foo >expect &&
+		git var GIT_DEFAULT_BRANCH >actual &&
+		test_cmp expect actual
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

base-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49
-- 
2.33.1

On Wed, Nov 3, 2021 at 2:53 PM Thomas Weißschuh [off-list ref] wrote:

So that we don't have to worry about this test breaking if the default
branch ever is changed, would it make sense to stop hard-coding
"master" here and instead employ the trick that Dscho mentioned
earlier in the thread (i.e. create a dummy repo and ask it for its
default branch)? Something like this (untested):

    (
        sane_unset GIT_TEST_... &&
        git init defbranch &&
        git -C defbranch symbolic-ref --short HEAD >expect &&
        git var GIT_DEFAULT_BRANCH >actual &&
        test_cmp expect actual
    )

+test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
+       test_config init.defaultbranch foo &&
+       (
+               sane_unset GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+               echo foo >expect &&
+               git var GIT_DEFAULT_BRANCH >actual &&
+               test_cmp expect actual
+       )
+'

Introduce the logical variable GIT_DEFAULT_BRANCH which represents the
the default branch name that will be used by "git init".

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.

Helped-by: Johannes Schindelin [off-list ref]
Helped-by: Eric Sunshine [off-list ref]
Helped-by: Ævar Arnfjörð Bjarmason [off-list ref]
Signed-off-by: Thomas Weißschuh <redacted>
---

Changes from v1 ( https://lore.kernel.org/git/20211030140112.834650-1-thomas@t-8ch.de/ ):
* Replaced the custom subcommand with an internal variable
* Cleaned up the tests

Changes from v2 ( https://lore.kernel.org/git/20211102164434.1005707-1-thomas@t-8ch.de/ ):
* Removed superfluous error handling
* Switched to better fitting function
* Reworded commit message to be more consistent with the rest of git

Changes from v3 ( https://lore.kernel.org/git/20211103185331.599463-1-thomas@t-8ch.de/ ):
* Future-proof test by testing against the actual name of a new repository

 Documentation/git-var.txt |  3 +++
 builtin/var.c             |  7 +++++++
 t/t0007-git-var.sh        | 20 ++++++++++++++++++++
 3 files changed, 30 insertions(+)

diff --git a/Documentation/git-var.txt b/Documentation/git-var.txt
index 6072f936ab..387cc1b914 100644
--- a/Documentation/git-var.txt
+++ b/Documentation/git-var.txt

@@ -59,6 +59,9 @@ ifdef::git-default-pager[]
     The build you are using chose '{git-default-pager}' as the default.
 endif::git-default-pager[]
 
+GIT_DEFAULT_BRANCH::
+    The name of the first branch created in newly initialized repositories.
+
 SEE ALSO
 --------
 linkgit:git-commit-tree[1]

diff --git a/builtin/var.c b/builtin/var.c
index 6c6f46b4ae..491db27429 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,11 @@ static const char *pager(int flag)
 	return pgm;
 }
 
+static const char *default_branch(int flag)
+{
+	return git_default_branch_name(1);
+}
+
 struct git_var {
 	const char *name;
 	const char *(*read)(int);

@@ -36,6 +42,7 @@ static struct git_var git_vars[] = {
 	{ "GIT_AUTHOR_IDENT",   git_author_info },
 	{ "GIT_EDITOR", editor },
 	{ "GIT_PAGER", pager },
+	{ "GIT_DEFAULT_BRANCH", default_branch },
 	{ "", NULL },
 };
 

diff --git a/t/t0007-git-var.sh b/t/t0007-git-var.sh
index 53af92d571..e56f4b9ac5 100755
--- a/t/t0007-git-var.sh
+++ b/t/t0007-git-var.sh

@@ -27,6 +27,26 @@ test_expect_success !FAIL_PREREQS,!AUTOIDENT 'requested identities are strict' '
 	)
 '
 
+test_expect_success 'get GIT_DEFAULT_BRANCH without configuration' '
+	(
+		sane_unset GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+		git init defbranch &&
+		git -C defbranch symbolic-ref --short HEAD >expect &&
+		git var GIT_DEFAULT_BRANCH >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'get GIT_DEFAULT_BRANCH with configuration' '
+	test_config init.defaultbranch foo &&
+	(
+		sane_unset GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME &&
+		echo foo >expect &&
+		git var GIT_DEFAULT_BRANCH >actual &&
+		test_cmp expect actual
+	)
+'
+
 # For git var -l, we check only a representative variable;
 # testing the whole output would make our test too brittle with
 # respect to unrelated changes in the test suite's environment.

base-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49
-- 
2.33.1