t1309: use a non-loaded branch name in the `onbranch` test cases

From: Johannes Schindelin <[email protected]>

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.

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

Signed-off-by Johannes Schindelin <[email protected]>

---
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(-)

t/t1309-early-config.sh +2 -2

index ebb8e1aecb..b4a9158307 100755
@@ -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

Quoted 6 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Quoted 2 lines

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.

Quoted 34 lines

Signed-off-by Johannes Schindelin <[email protected]>

---
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(-)

t/t1309-early-config.sh +2 -2

index ebb8e1aecb..b4a9158307 100755
@@ -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

Hi Ævar,

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 6 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Quoted 1 line

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

Well, the entire idea of switching to a new default branch name is to avoid using words that we know cause undue emotional harm. In the grand scheme, therefore, I want to avoid having any mention of such words in our test suite.

Quoted 6 lines

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.

Yeah, I hadn't made it clear yet at the time you wrote this that my intention was to give in to your and Junio's suggestion to restrict the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME assignments to _just_ the test scripts that don't work with arbitrary default branch names.

I had hoped that mentioning gitgitgadget PR 762 (which is that 27-strong patch series) would be indicator enough that I was in the process of revamping it into a v3, and that this here patch is one part that I separated out into its own patch.

Quoted 5 lines

Quoted 2 lines

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.

Nope. Not at all. Because what we're exercising is the code paths when we _don't_ have a branch name to work with.

In the non-Git case, this is trivial to see. There is not even a repository! How can there be a branch?

In the early config case, it is too early to access the refs. I meant to reference (but forgot) the commit 85fe0e800ca (config: work around bug with includeif:onbranch and early config, 2019-07-31) because that commit's commit message describes the catch-22 that is the reason why the early config cannot see the current branch name (if any).

I should probably have thought of referencing 22932d9169f (config: stop checking whether the_repository is NULL, 2019-08-06) for the second test case, too.

So again, these two test cases do _not_ exercise the code path where another config file is included. To the contrary, they try to prevent a regression where onbranch would segfault in one case, and BUG in the other (in both cases because the now-fixed code used to try to look at the current branch name _anyway_).

Quoted 3 lines

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

Quoted 3 lines

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

No, if we regressed, the code would start to throw a BUG, or a segfault, respectively.

We never expect these two test cases to look at any branch name at all.

Quoted 41 lines · Johannes Schindelin

Quoted 9 lines · Ævar Arnfjörð Bjarmason

Quoted 4 lines · Johannes Schindelin via GitGitGadget

From: Johannes Schindelin <[email protected]>

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.

Nope. Not at all. Because what we're exercising is the code paths when we _don't_ have a branch name to work with.

In the non-Git case, this is trivial to see. There is not even a repository! How can there be a branch?

In the early config case, it is too early to access the refs. I meant to reference (but forgot) the commit 85fe0e800ca (config: work around bug with includeif:onbranch and early config, 2019-07-31) because that commit's commit message describes the catch-22 that is the reason why the early config cannot see the current branch name (if any).

I should probably have thought of referencing 22932d9169f (config:
stop checking whether the_repository is NULL, 2019-08-06) for the second test case, too.

So again, these two test cases do _not_ exercise the code path where another config file is included. To the contrary, they try to prevent a regression where onbranch would segfault in one case, and BUG in the other (in both cases because the now-fixed code used to try to look at the current branch name _anyway_).

Quoted 6 lines

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

Quoted 5 lines

No, if we regressed, the code would start to throw a BUG, or a segfault, respectively.

We never expect these two test cases to look at any branch name at all.

Thanks. I mis(understood|read) it.