Re: [PATCH 00/28] Use main as default branch name

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Thu, Nov 12 2020, Johannes Schindelin via GitGitGadget wrote:

> This is the big one. This changes the default of init.defaultBranch to main,
> reflecting what many open source projects already did (which was followed by
> GitHub, Azure Repos and others).

I don't have an issue with the end goal of s/master/main|$whatever/ as
UX here as noted in previous related review rounds. But I don't think
this series as it stands is anywhere near ready.

This is 27 patches of s/master/main/ in the tests, followed by making
the change to the default in refs.c without as much as a single
documentation update to inform users of the change.

I don't see the point in doing these sorts of test suite changes, it
just seems like needless churn. But I have not read the whole backlog of
previous iterations of this & related patches, so bear with me.

Why not instead do what we did when we added protocol v2 to the tests,
e.g. in 8a1b0978ab ("test: request GIT_TEST_PROTOCOL_VERSION=0 when
appropriate", 2019-12-23) we simply set the t5515 test to always run
with protocol v1. If you'd do this:
    
    diff --git a/t/t5515-fetch-merge-logic.sh b/t/t5515-fetch-merge-logic.sh
    index 70a9d2d8ab..939c8b2f83 100755
    --- a/t/t5515-fetch-merge-logic.sh
    +++ b/t/t5515-fetch-merge-logic.sh
    @@ -11,6 +11,9 @@ test_description='Merge logic in fetch'
     GIT_TEST_PROTOCOL_VERSION=0
     export GIT_TEST_PROTOCOL_VERSION
     
    +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
    +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
    +
     . ./test-lib.sh
     
     build_script () {
    
Of course that also needs:
    
    index fa347ed3e1..2d700a171b 100644
    --- a/t/test-lib.sh
    +++ b/t/test-lib.sh
    @@ -1711,10 +1711,8 @@ test_lazy_prereq SHA1 '
     test_lazy_prereq REBASE_P '
     	test -z "$GIT_TEST_SKIP_REBASE_P"
     '
    -# Special-purpose prereq for transitioning to a new default branch name:
    -# Some tests need more than just a mindless (case-preserving) s/master/main/g
    -# replacement. The non-trivial adjustments are guarded behind this
    -# prerequisite, acting kind of as a feature flag
    -test_lazy_prereq PREPARE_FOR_MAIN_BRANCH '
    -	test "$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME" = main
    +
    +test_lazy_prereq MAIN_BRANCH_IS_MASTER '
    +	test -z "$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME" ||
    +	test "$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME" = "master"
     '

Because (but see "later" a few paragraphs later), the logic as it stands
on "master" is entirely backwards & broken. I.e. if you don't set
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME or set it to "master" we now skip a
bunch of tests ... which only work if the value is "master". If you set
it to "main" they'll break.

Then instead of ending up with a hardcoding of "main" we'd be able to
run the tests with a new custom branch name (even one with whitespace in
it). That coverage is plenty to test any branch name hardcoding.

The remaining tests that would still have "master" would then just be
dealt with by the same thing we did for the too-protocol-v1-specific
tests, i.e.:
    
    diff --git a/t/t3502-cherry-pick-merge.sh b/t/t3502-cherry-pick-merge.sh
    index 8b635a196d..354f9bd851 100755
    --- a/t/t3502-cherry-pick-merge.sh
    +++ b/t/t3502-cherry-pick-merge.sh
    @@ -8,6 +8,8 @@ test_description='cherry picking and reverting a merge
     
     '
     
    +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master
    +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
     . ./test-lib.sh
     
     test_expect_success setup '
    
Except unlike protocol v1/v2 we're really not gaining anything in
coverage by going further, it's not the point of any of these tests to
test for the default branch name. It's a *lot* of churn, 

"Later": After writing the above I see from 704fed9ea2 ("tests: start
moving to a different default main branch name", 2020-10-23) that this
state of affairs is intentional. But doesn't discuss why the more usual
(because we've done it that way a few times before) v1/v2-esque
transition wasn't done instead.

Doing it the other way still seems better. We're now not running our
full tests in anticipation for this series landing on "master", and
after applying this we're still not running all of them.

Anyway, enough with discussing this detail of the test suite churn. My
main objection to this version of it is:

After this series as it stands lands what's a rather big UI change in
git isn't discussed *anywhere* in the docs.

I'm not saying we need some similar s/master/main/g in Documentation/ as
what's being done here in t/, but I think a bare minimum for a rather
big UI change like this is:

 1) Let's at least talk about it in some way in git-init(1), i.e. that
    it used to be "master" before version so-and-so and is now
    different. With this series applied we still say:

    Documentation/git-init.txt:If not specified, fall back to the default name: `master`.

 2) Ditto in the init.defaultBranch and things like that

 3) After all this effort at eliminating "master" entirely in the tests
    we're still shipping a sample hook that expects "master" on "git
    init", and breaks now that it's "main"

And maybe stuff like:

 4) Have "git init" emit some new advice message saying that the default
    has changed and we've init-ed a new repo with a "main" branch.

Our primary concern should be the vast majority of users who don't
follow this topic on Twitter, don't read this mailing list, and are just
going to waste 10 minutes of their day because some script they've had
working for years using "git init" did something unexpected.

Let's at least aim to make that 5 minutes instead by making the change
more discoverable.