Re: [PATCH 6/6] breaking-changes: switch default branch to main

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Sep 2, 2025, 15:13 UTC
Message-ID: <96e128d9-e5e3-4bfc-9e33-3caa75cacfe6@gmail.com>
In-Reply-To: <aLbWuGQhriQCMFbO@pks.im>

Hi Patrick

On 02/09/2025 12:36, Patrick Steinhardt wrote:

Show 15 quoted lines

> On Wed, Aug 27, 2025 at 04:24:50PM +0100, Phillip Wood wrote:
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The advice printed when init.defaultBranch is not set is
>> updated to say that the default will change to "main" in Git
>> 3.0. Building with WITH_BREAKING_CHANGES enabled removes the advice,
>> changes the default branch name is "main" and removes support for
> 
> s/is/to/
> 
>> GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME. The code in guess_remote_head()
> 
> As Junio already mentioned, I don't really think it's necessary to drop
> `GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME`, making much of this commit
> series obsolete.

I'm happy to drop those patches if there is a consensus that GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME is still useful after we've switched the branch name. I'm still not quite sure what it is useful for though. Removing it makes it clear that the tests are relying on the builtin default branch name that users see rather than some special name used only for tests.

Show 7 quoted lines

>> that looks for "refs/heads/master" is left unchanged as that is only
>> called when the remote server does not support the symref capability
>> in the v0 protocol or the symref extension to the ls-refs list in the
>> v2 protocol. Such an old server is more likely to be using "master"
>> as the default branch name.
> 
> Hm, that's probably fair.

That's more or less a direct quote from Peff's mail <20250826111234.GA1925107@coredump.intra.peff.net>

Show 14 quoted lines

>> diff --git a/Documentation/BreakingChanges.adoc b/Documentation/BreakingChanges.adoc
>> index f8d2eba061c..93922299321 100644
>> --- a/Documentation/BreakingChanges.adoc
>> +++ b/Documentation/BreakingChanges.adoc
>> @@ -165,6 +165,11 @@ A prerequisite for this change is that the ecosystem is ready to support the
>>   "reftable" format. Most importantly, alternative implementations of Git like
>>   JGit, libgit2 and Gitoxide need to support it.
>>   
>> +* The default branch name will be `main`. We have been warning that the default
> 
> Let's explicitly mention here that this is only going to be the case for
> _new_ repositories. It's obvious to us, but for others it might not be
> that existing repositories will not see any change in behaviour due to
> this.

Sure

Show 17 quoted lines

>> diff --git a/advice.h b/advice.h
>> index 727dcecf4a3..fc1dc872049 100644
>> --- a/advice.h
>> +++ b/advice.h
>> @@ -18,7 +18,9 @@ enum advice_type {
>>   	ADVICE_AM_WORK_DIR,
>>   	ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME,
>>   	ADVICE_COMMIT_BEFORE_MERGE,
>> +#ifndef WITH_BREAKING_CHANGES
>>   	ADVICE_DEFAULT_BRANCH_NAME,
>> +#endif /* WITH_BREAKING_CHANGES */
>>   	ADVICE_DETACHED_HEAD,
>>   	ADVICE_DIVERGING,
>>   	ADVICE_FETCH_SET_HEAD_WARN,
> 
> Okay. I don't really think it's necessary to conditionally compile this,
> but it doesn't hurt much, either.

My thought was that by making it conditional, it makes it harder to forget to remove this after Git 3.0

Show 14 quoted lines

>> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
>> index 01823fd0f14..a21834043f3 100755
>> --- a/ci/run-build-and-tests.sh
>> +++ b/ci/run-build-and-tests.sh
>> @@ -9,7 +9,6 @@ run_tests=t
>>   
>>   case "$jobname" in
>>   linux-breaking-changes)
>> -	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
>>   	export WITH_BREAKING_CHANGES=YesPlease
>>   	;;
>>   linux-TEST-vars)
> 
> Nice.

Only if we remove GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME though? Part of the reason for removing it was to make it clear that the tests are using "main" when WITH_BREAKING_CHANGES is enabled.

>> diff --git a/refs.c b/refs.c
>> index 4ff55cf24f6..e73f63ff6b8 100644
>> --- a/refs.c
>> +++ b/refs.c

 >> [...]>> +#ifdef WITH_BREAKING_CHANGES

Show 5 quoted lines

>> +		ret = xstrdup("main");
>> +		(void) quiet; /* Silence -Wunused-parameter */
> 
> We have the `MAYBE_UNUSED` attribute that you can apply to `int
> quiet` to avoid this cast.

Thanks, I didn't know we had that

Show 7 quoted lines

>> diff --git a/t/t0001-init.sh b/t/t0001-init.sh
>> index f593c536874..7223a98773d 100755
>> --- a/t/t0001-init.sh
>> +++ b/t/t0001-init.sh
>> [...]
> Should we also add a test conditional on `WITH_BREAKING_CHANGES` to
> verify that the new branch name is `main` as expected?

If we remove GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME then all of the tests that rely on the branch name "main" are testing that. If we keep that variable then we should probably add a test that unsets GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME and ensures the default branch name is "main"

> Thanks for working on this change, I'm very much supportive of this. It
> feels like the ecosystem has already been moving into that direction, so
> it's time that Git catches up with that change.

Thanks for your comments, I'll try and re-roll later this week.

Phillip