Re: [PATCH 00/28] Use main as default branch name
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Nov 13, 2020, 14:22 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2011131519170.18437@tvgsbejvaqbjf.bet>
- In-Reply-To
- <87pn4hfmc4.fsf@evledraar.gmail.com>
Hi Ævar,
On Fri, 13 Nov 2020, Ævar Arnfjörð Bjarmason wrote:
Show 13 quoted lines
> 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
Show 123 quoted lines
>
> 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.
>