| Message ID | 20211102164434.1005707-1-thomas@t-8ch.de (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] var: add GIT_DEFAULT_BRANCH variable | expand |
On Tue, Nov 02 2021, Thomas Weißschuh wrote: > Introduce the builtin 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. > > Co-developed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de> > --- > > 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 > > @Johannes: I replaced BUG() with die() from your example because that seems to be > nicer for user facing messages. > > Documentation/git-var.txt | 3 +++ > builtin/var.c | 13 +++++++++++++ > t/t0007-git-var.sh | 19 +++++++++++++++++++ > 3 files changed, 35 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..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); > + > + if (!name) > + die("could not determine the default branch name"); Isn't this die() unrechable given the similar logic in repo_default_branch_name()? Hence the previous BUG(...)? I really don't see how it makes sense to add this to "git var", we have that to correspond to environment variables we use. *Maybe* if we renamed GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to GIT_INITIAL_BRANCH_NAME and made it a non-test thing like GIT_TEMPLATE_DIR, but even then shouldn't we be adding "GIT_TEMPLATE_DIR" and any number of other things to this as well? I'm not saying that your patch needs to do that, but we really should think about the interface & future implications if we're going in this direction. The reason I suggested extending "git config" in [1] is because it seems like a natural thing for "git config" to learn to spew out our idea of default hardcoded config values to the user. But creating a variable form of that existing config just so we can have "git var" spew it out just seems weird. We don't have or need such a variable now for anything else, so why go through that indirection, instead of something that closes the feature gap of asking what a config variable default is? In any case whatever we do here this really should be updating the documentation of init.defaultbranch & the relevant bits in the "git init" manpage to add cross-references, similar to how we discuss GIT_TEMPLATE_DIR now. 1. https://lore.kernel.org/git/211030.86ilxe4edm.gmgdl@evledraar.gmail.com/
On 2021-11-02 17:53+0100, Ævar Arnfjörð Bjarmason wrote: > On Tue, Nov 02 2021, Thomas Weißschuh wrote: > > > Introduce the builtin 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. > > > > Co-developed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > > Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de> > > --- > > > > 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 > > > > @Johannes: I replaced BUG() with die() from your example because that seems to be > > nicer for user facing messages. > > > > Documentation/git-var.txt | 3 +++ > > builtin/var.c | 13 +++++++++++++ > > t/t0007-git-var.sh | 19 +++++++++++++++++++ > > 3 files changed, 35 insertions(+) > > > > > > +static const char *default_branch(int flag) > > +{ > > + const char *name = repo_default_branch_name(the_repository, 1); > > + > > + if (!name) > > + die("could not determine the default branch name"); > > Isn't this die() unrechable given the similar logic in > repo_default_branch_name()? Hence the previous BUG(...)? Ok. Good point. > I really don't see how it makes sense to add this to "git var", we have > that to correspond to environment variables we use. > > *Maybe* if we renamed GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to > GIT_INITIAL_BRANCH_NAME and made it a non-test thing like > GIT_TEMPLATE_DIR, but even then shouldn't we be adding > "GIT_TEMPLATE_DIR" and any number of other things to this as well? > > I'm not saying that your patch needs to do that, but we really should > think about the interface & future implications if we're going in this > direction. > > The reason I suggested extending "git config" in [1] is because it seems > like a natural thing for "git config" to learn to spew out our idea of > default hardcoded config values to the user. > > But creating a variable form of that existing config just so we can have > "git var" spew it out just seems weird. > > We don't have or need such a variable now for anything else, so why go > through that indirection, instead of something that closes the feature > gap of asking what a config variable default is? > > In any case whatever we do here this really should be updating the > documentation of init.defaultbranch & the relevant bits in the "git > init" manpage to add cross-references, similar to how we discuss > GIT_TEMPLATE_DIR now. > > 1. https://lore.kernel.org/git/211030.86ilxe4edm.gmgdl@evledraar.gmail.com/ I'll then wait for a consensus of the git devs. The actual implementation shouldn't be the issue afterwards. Thanks for looking into this! Thomas
On Tue, Nov 02 2021, Thomas Weißschuh wrote: > On 2021-11-02 17:53+0100, Ævar Arnfjörð Bjarmason wrote: >> On Tue, Nov 02 2021, Thomas Weißschuh wrote: >> >> > Introduce the builtin 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. >> > >> > Co-developed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> >> > Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de> >> > --- >> > >> > 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 >> > >> > @Johannes: I replaced BUG() with die() from your example because that seems to be >> > nicer for user facing messages. >> > >> > Documentation/git-var.txt | 3 +++ >> > builtin/var.c | 13 +++++++++++++ >> > t/t0007-git-var.sh | 19 +++++++++++++++++++ >> > 3 files changed, 35 insertions(+) >> > >> > >> > +static const char *default_branch(int flag) >> > +{ >> > + const char *name = repo_default_branch_name(the_repository, 1); >> > + >> > + if (!name) >> > + die("could not determine the default branch name"); >> >> Isn't this die() unrechable given the similar logic in >> repo_default_branch_name()? Hence the previous BUG(...)? > > Ok. Good point. > >> I really don't see how it makes sense to add this to "git var", we have >> that to correspond to environment variables we use. >> >> *Maybe* if we renamed GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to >> GIT_INITIAL_BRANCH_NAME and made it a non-test thing like >> GIT_TEMPLATE_DIR, but even then shouldn't we be adding >> "GIT_TEMPLATE_DIR" and any number of other things to this as well? >> >> I'm not saying that your patch needs to do that, but we really should >> think about the interface & future implications if we're going in this >> direction. >> >> The reason I suggested extending "git config" in [1] is because it seems >> like a natural thing for "git config" to learn to spew out our idea of >> default hardcoded config values to the user. >> >> But creating a variable form of that existing config just so we can have >> "git var" spew it out just seems weird. >> >> We don't have or need such a variable now for anything else, so why go >> through that indirection, instead of something that closes the feature >> gap of asking what a config variable default is? >> >> In any case whatever we do here this really should be updating the >> documentation of init.defaultbranch & the relevant bits in the "git >> init" manpage to add cross-references, similar to how we discuss >> GIT_TEMPLATE_DIR now. >> >> 1. https://lore.kernel.org/git/211030.86ilxe4edm.gmgdl@evledraar.gmail.com/ > > I'll then wait for a consensus of the git devs. The actual implementation > shouldn't be the issue afterwards. > > Thanks for looking into this! Please don't take that message as me or anyone else "pulling rank" just because we've got some previous commits in git.git. That applies to both me and Johannes, and clearly we disagree on this minor bit of UX direction. I'd think if anything the opinion of someone who's not overly familiar with the system would be more valuable, i.e. yours, especially since you tried & failed to find a way to do this recently. Would you find it more intuitive to look in say "git var" over "git config" for this sort of information? A further weirdness is that another effective source of config for this is the "unborn" ls-refs feature[1]. I'm not sure what that means for any query interface, i.e. would a user want to know what branch a freshly cloned repo would end up with in advance, taking into account all of the local config, remote "unborn" etc? 1. https://lore.kernel.org/git/878s8apthr.fsf@evledraar.gmail.com/
On 2021-11-02 20:14+0100, Ævar Arnfjörð Bjarmason wrote: > On Tue, Nov 02 2021, Thomas Weißschuh wrote: > > > On 2021-11-02 17:53+0100, Ævar Arnfjörð Bjarmason wrote: > >> On Tue, Nov 02 2021, Thomas Weißschuh wrote: > >> > >> > Introduce the builtin 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. > >> > > >> > Co-developed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> > >> > Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de> > >> > --- > >> > > >> > 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 > >> > > >> > @Johannes: I replaced BUG() with die() from your example because that seems to be > >> > nicer for user facing messages. > >> > > >> > Documentation/git-var.txt | 3 +++ > >> > builtin/var.c | 13 +++++++++++++ > >> > t/t0007-git-var.sh | 19 +++++++++++++++++++ > >> > 3 files changed, 35 insertions(+) > >> > > >> > > >> > +static const char *default_branch(int flag) > >> > +{ > >> > + const char *name = repo_default_branch_name(the_repository, 1); > >> > + > >> > + if (!name) > >> > + die("could not determine the default branch name"); > >> > >> Isn't this die() unrechable given the similar logic in > >> repo_default_branch_name()? Hence the previous BUG(...)? > > > > Ok. Good point. > > > >> I really don't see how it makes sense to add this to "git var", we have > >> that to correspond to environment variables we use. > >> > >> *Maybe* if we renamed GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to > >> GIT_INITIAL_BRANCH_NAME and made it a non-test thing like > >> GIT_TEMPLATE_DIR, but even then shouldn't we be adding > >> "GIT_TEMPLATE_DIR" and any number of other things to this as well? > >> > >> I'm not saying that your patch needs to do that, but we really should > >> think about the interface & future implications if we're going in this > >> direction. > >> > >> The reason I suggested extending "git config" in [1] is because it seems > >> like a natural thing for "git config" to learn to spew out our idea of > >> default hardcoded config values to the user. > >> > >> But creating a variable form of that existing config just so we can have > >> "git var" spew it out just seems weird. > >> > >> We don't have or need such a variable now for anything else, so why go > >> through that indirection, instead of something that closes the feature > >> gap of asking what a config variable default is? > >> > >> In any case whatever we do here this really should be updating the > >> documentation of init.defaultbranch & the relevant bits in the "git > >> init" manpage to add cross-references, similar to how we discuss > >> GIT_TEMPLATE_DIR now. > >> > >> 1. https://lore.kernel.org/git/211030.86ilxe4edm.gmgdl@evledraar.gmail.com/ > > > > I'll then wait for a consensus of the git devs. The actual implementation > > shouldn't be the issue afterwards. > > > > Thanks for looking into this! > > Please don't take that message as me or anyone else "pulling rank" just > because we've got some previous commits in git.git. That applies to both > me and Johannes, and clearly we disagree on this minor bit of UX > direction. This was not my impression, although my message has a bit of a resignated tone. That was not the intention. > I'd think if anything the opinion of someone who's not overly familiar > with the system would be more valuable, i.e. yours, especially since you > tried & failed to find a way to do this recently. Would you find it more > intuitive to look in say "git var" over "git config" for this sort of > information? To be honest I was not aware of "git var" before Johannes before proposed it. And I am still not sure how to understand the "logical" aspect of "git-var". (git-var - Show a Git logical variable) A "git config" variable using a generic config default framework looks like the generally cleanest interface. The appeal of "git var" was the easy and quick implementation. > A further weirdness is that another effective source of config for this > is the "unborn" ls-refs feature[1]. I'm not sure what that means for any > query interface, i.e. would a user want to know what branch a freshly > cloned repo would end up with in advance, taking into account all of the > local config, remote "unborn" etc? I have no idea how that should work. > 1. https://lore.kernel.org/git/878s8apthr.fsf@evledraar.gmail.com/ If you and Johannes think it would help the design-process I would also volunteer to implement your original proposal: git config --get-or-git-default init.defaultBranch Please note that I'm not doing (primarily) doing this to get commits into git.git, so if somebody with more knowledge about the git architecture wants to bring this forward, please feel free.
On Tue, Nov 02, 2021 at 05:53:46PM +0100, Ævar Arnfjörð Bjarmason wrote: > The reason I suggested extending "git config" in [1] is because it seems > like a natural thing for "git config" to learn to spew out our idea of > default hardcoded config values to the user. > > But creating a variable form of that existing config just so we can have > "git var" spew it out just seems weird. > > We don't have or need such a variable now for anything else, so why go > through that indirection, instead of something that closes the feature > gap of asking what a config variable default is? To me, the point is that the user is not asking "what is the default value of this config variable?". They are asking "if I were to init a new repository, what would Git decide the default branch name is?". Right now that is very tied to the config mechanism (modulo the GIT_TEST_* bits, but I think we can ignore those for regular users), so those two are basically the same question. But it doesn't have to be. Abstracting it to the question the user actually wants to ask future-proofs the mechanism. I.e., I don't think introducing a new variable into "git var" is that big a deal. They don't have to be related to an environment variable; the documentation calls them "logical variables". This is exactly the kind of thing it's designed for. -Peff
On Wed, Nov 3, 2021 at 7:37 AM Jeff King <peff@peff.net> wrote: > I.e., I don't think introducing a new variable into "git var" is that > big a deal. They don't have to be related to an environment variable; > the documentation calls them "logical variables". This is exactly the > kind of thing it's designed for. I almost don't want to mention it since it's not very discoverable (and is never the first place I think to look), but git-rev-parse already contains a grab-bag of options unrelated to revision parsing. Many of the options relate to querying paths (--show-toplevel, --show-cdup, etc.) or other information related to a repository (--local-env-vars, --show-object-format, etc.). So, adding --show-default-branch may be one possibility.
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`". > 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. > 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.
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..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); + + if (!name) + die("could not determine the default branch name"); + + return name; +} + + struct git_var { const char *name; const char *(*read)(int); @@ -36,6 +48,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.
Introduce the builtin 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. Co-developed-by: Johannes Schindelin <Johannes.Schindelin@gmx.de> Signed-off-by: Thomas Weißschuh <thomas@t-8ch.de> --- 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 @Johannes: I replaced BUG() with die() from your example because that seems to be nicer for user facing messages. Documentation/git-var.txt | 3 +++ builtin/var.c | 13 +++++++++++++ t/t0007-git-var.sh | 19 +++++++++++++++++++ 3 files changed, 35 insertions(+) base-commit: 0cddd84c9f3e9c3d793ec93034ef679335f35e49