Re: [PATCH v3 0/4] breaking changes: switch default initial branch name to "main"

[Junio C Hamano <gitster@pobox.com>]

Phillip Wood <phillip.wood123@gmail.com> writes:

> The test changes up to this point strike me as somewhat unfortunate as
> these tests are perfectly content with the existing advice. I assume
> this stems from us not setting GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> anymore when WITH_BREAKING_CHANGES is enabled. I think we should do
> something like
>
> diff --git b/t/test-lib.sh b/t/test-lib.sh
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -127,13 +127,15 @@ then
>  	export GIT_TEST_DISALLOW_ABBREVIATED_OPTIONS
>  fi
>  +# Explicitly set the default branch name for testing, to avoid the
> +# transitory "git init" warning under --verbose.
>  if test -z "$WITH_BREAKING_CHANGES"
>  then
> -	# Explicitly set the default branch name for testing, to avoid the
> -	# transitory "git init" warning under --verbose.
>  	: ${GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME:=master}
> -	export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
> +else
> +	: ${GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME:=main}
>  fi
> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

That does make it more in line in spirit with what your
"breaking-changes: switch default branch to main" does here.
Instead of assuming that we won't have "no default set" hint after
3.0, we do assume the "no default set" hint will be there and
squelch for a bit longer.

OK.

> To address that which will also stop the advice appearing when the
> tests are run with --verbose as well.
>
>>   	git -C repo rev-parse --show-ref-format >actual &&
>>   	echo $GIT_DEFAULT_REF_FORMAT >expected &&
>> @@ -868,18 +868,18 @@ test_expect_success 'overridden default initial branch name (config)' '
>>   	grep nmb actual
>>   '
>>   -test_expect_success !WITH_BREAKING_CHANGES 'advice on
>> unconfigured init.defaultBranch' '
>> +test_expect_success 'advice on unconfigured init.defaultBranch' '
>>   	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= git -c color.advice=always \
>>   		init unconfigured-default-branch-name 2>err &&
>>   	test_decode_color <err >decoded &&
>>   	test_grep "<YELLOW>hint: " decoded
>>   '
>>   -test_expect_success 'advice on unconfigured init.defaultBranch
>> disabled' '
>> +test_expect_success 'advice on unconfigured init.defaultBranch can be disabled' '
>>   	test_when_finished "rm -rf no-advice" &&
>>     	GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME= \
>> -		git -c advice.defaultBranchName=false init no-advice 2>err &&
>> +	git -c advice.defaultBranchName=false init no-advice 2>err &&
>
> The indentation is changed here but not above. I'd be happy to leave
> this alone, but if we're going to remove the indentation here shouldn't
> we do the same above?

QUite honestly, I didn't even notice that the line that begins with
"init unconfigured..." was a continuation line.  We definitely
should fix it to match.

>>   	test_grep ! "hint: " err
>
> Do we want to check the advice that is printed now that we have two
> different messages?

I am not sure; isn't this checking that there is no advice, not two
advice messages?

Ahh, you mean two new tests, one with !WITH_BREAKING_CHANGES and one
with WITH_BREAKING_CHANGES, to see default_branch_name_advice[]
appears in the advice output?  I am not sure if that is needed here.
Without [PATCH 5/4] there is no test that checks the exact wording
of "... will change to main in Git 3.0" message, either, and I think
that is a sensible thing *not* to have a test that insists on the
exact phrasing for such things.