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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 6813 bytes --]

Hi Ævar,

On Fri, 13 Nov 2020, Ævar Arnfjörð Bjarmason wrote:

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

Of course, this is _the one_ patch series mentioned in
https://github.com/gitgitgadget/git/pull/655 that does _not_ link back to
the meta-PR.

So you're right, on its own, these 28 patches do not make sense.

And if you split off the 28th patch, the remaining 27 patches make even
less sense.

The reason why I partitioned the changes this way was that it is a _ton_
of patches to review, and in many ways, these 28 patches are the most
important ones to review (I didn't want to just rely on the CI builds
passing, I reviewed those patches carefully, multiple times, to make sure
that they made sense in the goal to transition to `main`).

I will respond to the rest of your concerns later,
Dscho

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