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