Re: [PATCH] t1309: use a non-loaded branch name in the `onbranch` test cases

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 18, 2020, 14:52 UTC
Message-ID: <87a6vera3q.fsf@evledraar.gmail.com>
In-Reply-To: <pull.791.git.1605709410465.gitgitgadget@gmail.com>

On Wed, Nov 18 2020, Johannes Schindelin via GitGitGadget wrote:

Show 6 quoted lines

> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The `onbranch` test cases in question do not actually want to include
> anything; Instead, they want to verify that the `onbranch` code path
> does not regress in the early-config case or in the non-Git case, where
> the `onbranch` include is actually ignored.

It's unclear to me what this patch is for & why it's needed.

Yesterday in your v2 27/27 series you sent a different one that changed this from s/master/main/g: https://lore.kernel.org/git/b8fa037791683b50c3efb01aa6ac0d3f7b888a2b.1605629548.git.gitgitgadget@gmail.com/

That's on top of "next", but this one is on "master", the two would conflict, and the 02/27 one seems like the right thing to do.

> Therefore, the actual branch name does not matter at all. We might just
> as well avoid racially-charged names here.

It seems to me the actual name matters a lot, and it must whatever the default branch name is.

I.e. what the test is doing is producing intentionally broken config, and asserting that we don't read it at an early stage.

Therefore if we regressed and started doing that the test wouldn't catch it, because the default branch name is "master", or "main" if/when that refs.c change lands, neither of which is "topic".

Maybe I'm missing something but it seems 58ebccb478 ("t1309: use short branch name in includeIf.onbranch test", 2019-08-06) and your own 85fe0e800c ("config: work around bug with includeif:onbranch and early config", 2019-07-31) which added the test support reading.

Show 34 quoted lines

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>     t1309: use a non-loaded branch name in the onbranch test cases
>     
>     Just something I stumbled over while working on 
>     https://github.com/gitgitgadget/git/pull/762.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-791%2Fdscho%2Ft1309-onbranch-tests-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-791/dscho/t1309-onbranch-tests-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/791
>
>  t/t1309-early-config.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t1309-early-config.sh b/t/t1309-early-config.sh
> index ebb8e1aecb..b4a9158307 100755
> --- a/t/t1309-early-config.sh
> +++ b/t/t1309-early-config.sh
> @@ -91,11 +91,11 @@ test_expect_failure 'ignore .git/ with invalid config' '
>  
>  test_expect_success 'early config and onbranch' '
>  	echo "[broken" >broken &&
> -	test_with_config "[includeif \"onbranch:master\"]path=../broken"
> +	test_with_config "[includeif \"onbranch:topic\"]path=../broken"
>  '
>  
>  test_expect_success 'onbranch config outside of git repo' '
> -	test_config_global includeIf.onbranch:master.path non-existent &&
> +	test_config_global includeIf.onbranch:topic.path non-existent &&
>  	nongit git help
>  '
>  
>
> base-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500