| Message ID | 20201118114834.11137-1-avarab@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tests: use main as default branch name | expand |
Hi Ævar, On Wed, 18 Nov 2020, Ævar Arnfjörð Bjarmason wrote: > Reinstate the test coverage lost due to PREPARE_FOR_MAIN_BRANCH. The > remaining impact of that prerequisite was mainly missing coverage from > submodule fetches being lost[1], e.g. impacting my in-flight > ab/retire-parse-remote. Now the prerequisite is effectively a > noop. This goes on top of [2]. > > I'm not removing the PREPARE_FOR_MAIN_BRANCH prerequisite to keep this > change small, instead it's now effectively a noop. It can be removed > in some later change. > > The only remaining occurrences in t5526-fetch-submodules.sh can be > removed without breakage with: > > perl -pi -e 's/PREPARE_FOR_MAIN_BRANCH //g' t/t5526-fetch-submodules.sh > > Which at this point leaves only the now-unused prerequisite > declaration in test-lib.sh. > > The coverage in t9902-completion.sh was restored by partially > reverting[3]. After that we were left with one test in a mixed > state. It setup "master" but tested for "mai". Change it back to > "mas", pending a more complete refactoring of that test. > > This change only conflicts between next..seen by clashing with Peter > Kaestle's in-flights submodule fix[4]. Fixing the resulting logic > error in t5526-fetch-submodules.sh is trivial, simply: > > - compare_refs_in_dir A origin/master B origin/master > + compare_refs_in_dir A origin/main B origin/main > > 1. 66713e84e7 ("tests: prepare aligned mentions of the default branch name", 2020-10-23) > 2. https://public-inbox.org/git/pull.762.v2.git.1605629547.gitgitgadget@gmail.com/ > 3. 8164360fc8 ("t9902: prepare a test for the upcoming default branch name", 2020-10-23) > 4. https://public-inbox.org/git/1605196853-37359-1-git-send-email-peter.kaestle@nokia.com/ > > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- Hmm. I have the feeling that doing this will just delay everything even further. I had really hoped that we could get this done without "three steps forward, one step back" dances. > On Tue, Nov 17 2020, Johannes Schindelin via GitGitGadget wrote: > > To avoid even more conflicts with topics that did not even make it to seen > > yet, this patch series specifically excludes t3404, t4013, t5310, t5526, > > t6300, t7064, t7817, t9902: in those test scripts, we will still use master > > for the time being. Once the topics in question have settled, I will send > > the appropriate follow-up patches to adjust them to use main instead. > > This is not a replacement for that subsequent cleanup, but seems like > a simple enough thing to have now to reinstate the missing test > coverage. > > Perhaps there's some topics not in "seen" that you have in mind as > conflicting, but as noted above the conflict produced here with > in-flight in "seen" is trivial. The conflicts might be trivial, but every conflict makes it harder to juggle branch thickets like Junio's `seen`. And those things add up. They're no fun, and they make life hard and tedious. t5526 conflicts _semantically_ with `pk/subsub-fetch-fix` (which touches t5526 and adds a new test case referring to `master`). t9902 conflicts with `fc/bash-completion-post-2.29`, and in contrast to the t5526 issue (which is trivial, even if it does require manual fixing), the t9902 is a bit more hairy to reason about. So yes, I would love to have that test coverage back, but not by making the transition to `main` even harder by reverting parts of it. That's why I promised publicly to take care of the loose ends as quickly as I can, after the conflicting issues graduate to `next` (or when they become stalled or even dropped from `seen`). Ciao, Dscho > > Seems worth having that sooner than later if Junio's willing juggle > that. > > t/t5526-fetch-submodules.sh | 6 +++--- > t/t9902-completion.sh | 6 +++--- > t/test-lib.sh | 9 +++------ > 3 files changed, 9 insertions(+), 12 deletions(-) > > diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh > index dd8e423d25..f45ba02b8a 100755 > --- a/t/t5526-fetch-submodules.sh > +++ b/t/t5526-fetch-submodules.sh > @@ -481,7 +481,7 @@ test_expect_success PREPARE_FOR_MAIN_BRANCH "don't fetch submodule when newly re > test_i18ncmp expect.err actual.err && > ( > cd submodule && > - git checkout -q master > + git checkout -q main > ) > ' > > @@ -663,9 +663,9 @@ test_expect_success 'fetch new submodule commits on-demand without .gitmodules e > git config -f .gitmodules --remove-section submodule.sub1 && > git add .gitmodules && > git commit -m "delete gitmodules file" && > - git checkout -B master && > + git checkout -B main && > git -C downstream fetch && > - git -C downstream checkout origin/master && > + git -C downstream checkout origin/main && > > C=$(git -C submodule commit-tree -m "yet another change outside refs/heads" HEAD^{tree}) && > git -C submodule update-ref refs/changes/7 $C && > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > index 5c01c75d40..3696b85acb 100755 > --- a/t/t9902-completion.sh > +++ b/t/t9902-completion.sh > @@ -1055,13 +1055,13 @@ test_expect_success 'teardown after filtering matching refs' ' > git -C otherrepo branch -D matching/branch-in-other > ' > > -test_expect_success PREPARE_FOR_MAIN_BRANCH '__git_refs - for-each-ref format specifiers in prefix' ' > +test_expect_success '__git_refs - for-each-ref format specifiers in prefix' ' > cat >expected <<-EOF && > evil-%%-%42-%(refname)..master > EOF > ( > - cur="evil-%%-%42-%(refname)..mai" && > - __git_refs "" "" "evil-%%-%42-%(refname).." mai >"$actual" > + cur="evil-%%-%42-%(refname)..mas" && > + __git_refs "" "" "evil-%%-%42-%(refname).." mas >"$actual" > ) && > test_cmp expected "$actual" > ' > diff --git a/t/test-lib.sh b/t/test-lib.sh > index d39bdf04ce..ed489b2213 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -257,7 +257,7 @@ case "$TRASH_DIRECTORY" in > esac > > case "$TEST_NUMBER" in > -3404|4013|5310|5526|6300|7064|7817|9902) > +3404|4013|5310|6300|7064|7817|9902) > # Avoid conflicts with patch series that are cooking at the same time > # as the patch series changing the default of `init.defaultBranch`. > GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master > @@ -1725,12 +1725,9 @@ 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 > +# Obsolete, do not use, removed soon! > test_lazy_prereq PREPARE_FOR_MAIN_BRANCH ' > - test "$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME" = main > + test "$TEST_NAME" = "t5526-fetch-submodules" > ' > > # Ensure that no test accidentally triggers a Git command > -- > 2.29.2.222.g5d2a92d10f8 > > >
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > The conflicts might be trivial, but every conflict makes it harder to > juggle branch thickets like Junio's `seen`. And those things add up. > They're no fun, and they make life hard and tedious. The worst part is they can easily be cause of unintended breakage due to mismerge. > t5526 conflicts _semantically_ with `pk/subsub-fetch-fix` (which touches > t5526 and adds a new test case referring to `master`). > > t9902 conflicts with `fc/bash-completion-post-2.29`, and in contrast to > the t5526 issue (which is trivial, even if it does require manual fixing), > the t9902 is a bit more hairy to reason about. > > So yes, I would love to have that test coverage back, but not by making > the transition to `main` even harder by reverting parts of it. Hmph, wouldn't the forcing with GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME to 'master' be the right way to keep test coverage? After all, the primary goal is to prepare tests so that we won't lose coverage when the fallback default of init.defaultbranch is switched to 'main', and it is a secondary goal of much lessor importance to reduce the number of hits from "git grep master t/". > That's why I promised publicly to take care of the loose ends as quickly > as I can, after the conflicting issues graduate to `next` (or when they > become stalled or even dropped from `seen`). Perhaps I should start to more aggressively drop topics from `seen` that are not sufficiently reviewed? The guiding principle ought to be "unreviewed patches are not worth applying", but I have a feeling that we have become more and more lax over time due to shortage of quality reviews. I dunno.
Hi Junio, On Thu, 19 Nov 2020, Junio C Hamano wrote: > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: > > > The conflicts might be trivial, but every conflict makes it harder to > > juggle branch thickets like Junio's `seen`. And those things add up. > > They're no fun, and they make life hard and tedious. > > The worst part is they can easily be cause of unintended breakage > due to mismerge. > > > t5526 conflicts _semantically_ with `pk/subsub-fetch-fix` (which touches > > t5526 and adds a new test case referring to `master`). > > > > t9902 conflicts with `fc/bash-completion-post-2.29`, and in contrast to > > the t5526 issue (which is trivial, even if it does require manual fixing), > > the t9902 is a bit more hairy to reason about. > > > > So yes, I would love to have that test coverage back, but not by making > > the transition to `main` even harder by reverting parts of it. > > Hmph, wouldn't the forcing with GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME > to 'master' be the right way to keep test coverage? Well, I was naive enough to think that this whole "let's transition the test suite to use `main` as default branch name" business would go over much quicker. With that expectation, I was content to already have `PREPARE_FOR_MAIN`-marked test cases in t5526 and t9902. They are currently skipped because that prereq is still waiting for the transition to complete. That's what my promise was about: as soon as the competing topics settle, I want to address those two test scripts in particular, moving them over to `GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main`. > After all, the primary goal is to prepare tests so that we won't lose > coverage when the fallback default of init.defaultbranch is switched to > 'main', and it is a secondary goal of much lessor importance to reduce > the number of hits from "git grep master t/". > > > That's why I promised publicly to take care of the loose ends as quickly > > as I can, after the conflicting issues graduate to `next` (or when they > > become stalled or even dropped from `seen`). > > Perhaps I should start to more aggressively drop topics from `seen` > that are not sufficiently reviewed? The guiding principle ought to > be "unreviewed patches are not worth applying", but I have a feeling > that we have become more and more lax over time due to shortage of > quality reviews. I dunno. FWIW I think you do a wonderful job of keeping the patch series in `seen`. I wish we could keep the CI build passing a bit more, but I'd rather have the branches that are in flight in one place, so that it is easy e.g. to find out whether `git diff next..seen -- t/t9902\*` is empty (to determine whether working on that script would cause conflicts right now). Ciao, Dscho
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes: >> Perhaps I should start to more aggressively drop topics from `seen` >> that are not sufficiently reviewed? The guiding principle ought to >> be "unreviewed patches are not worth applying", but I have a feeling >> that we have become more and more lax over time due to shortage of >> quality reviews. I dunno. > > FWIW I think you do a wonderful job of keeping the patch series in `seen`. > I wish we could keep the CI build passing a bit more, but I'd rather have > the branches that are in flight in one place, so that it is easy e.g. to > find out whether `git diff next..seen -- t/t9902\*` is empty (to determine > whether working on that script would cause conflicts right now). I have to disagree. There are a few topics, perhaps more than a few, that is age old in Git timescale nobody has bothered to ask the list why we still have it in 'seen' [*1*]. It may be a good thing to pick up as many potentially interesting new topics as possible and to merge them to 'seen' while resolving possible conflicts with other topics in flight. I am willing to continue doing so. But I doubt that a series lingering in 'seen' that has not been touched for more than say four weeks has much chance of gaining any new interest to push it forward. Perhaps we need a policy to discard a topic whose author timestamp is 8 weeks or older and not in 'next', without preventing those who care about the topic from resurrecting it in support, or something along that line. [Footnote] *1* The question can be asked in two ways. "Shouldn't it go 'next' already? I've read it over, here is the review and the thread shows a clear concensus that it is a good idea" is one happy outcome (even though that would indicate the maintainer doing a poor job). Alternatively, "Shouldn't it be discarded? Nobody seemed interested back then and now we have X instead, so it is not necessary." is also a possible happy outcome.
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh index dd8e423d25..f45ba02b8a 100755 --- a/t/t5526-fetch-submodules.sh +++ b/t/t5526-fetch-submodules.sh @@ -481,7 +481,7 @@ test_expect_success PREPARE_FOR_MAIN_BRANCH "don't fetch submodule when newly re test_i18ncmp expect.err actual.err && ( cd submodule && - git checkout -q master + git checkout -q main ) ' @@ -663,9 +663,9 @@ test_expect_success 'fetch new submodule commits on-demand without .gitmodules e git config -f .gitmodules --remove-section submodule.sub1 && git add .gitmodules && git commit -m "delete gitmodules file" && - git checkout -B master && + git checkout -B main && git -C downstream fetch && - git -C downstream checkout origin/master && + git -C downstream checkout origin/main && C=$(git -C submodule commit-tree -m "yet another change outside refs/heads" HEAD^{tree}) && git -C submodule update-ref refs/changes/7 $C && diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 5c01c75d40..3696b85acb 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -1055,13 +1055,13 @@ test_expect_success 'teardown after filtering matching refs' ' git -C otherrepo branch -D matching/branch-in-other ' -test_expect_success PREPARE_FOR_MAIN_BRANCH '__git_refs - for-each-ref format specifiers in prefix' ' +test_expect_success '__git_refs - for-each-ref format specifiers in prefix' ' cat >expected <<-EOF && evil-%%-%42-%(refname)..master EOF ( - cur="evil-%%-%42-%(refname)..mai" && - __git_refs "" "" "evil-%%-%42-%(refname).." mai >"$actual" + cur="evil-%%-%42-%(refname)..mas" && + __git_refs "" "" "evil-%%-%42-%(refname).." mas >"$actual" ) && test_cmp expected "$actual" ' diff --git a/t/test-lib.sh b/t/test-lib.sh index d39bdf04ce..ed489b2213 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -257,7 +257,7 @@ case "$TRASH_DIRECTORY" in esac case "$TEST_NUMBER" in -3404|4013|5310|5526|6300|7064|7817|9902) +3404|4013|5310|6300|7064|7817|9902) # Avoid conflicts with patch series that are cooking at the same time # as the patch series changing the default of `init.defaultBranch`. GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=master @@ -1725,12 +1725,9 @@ 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 +# Obsolete, do not use, removed soon! test_lazy_prereq PREPARE_FOR_MAIN_BRANCH ' - test "$GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME" = main + test "$TEST_NAME" = "t5526-fetch-submodules" ' # Ensure that no test accidentally triggers a Git command
Reinstate the test coverage lost due to PREPARE_FOR_MAIN_BRANCH. The remaining impact of that prerequisite was mainly missing coverage from submodule fetches being lost[1], e.g. impacting my in-flight ab/retire-parse-remote. Now the prerequisite is effectively a noop. This goes on top of [2]. I'm not removing the PREPARE_FOR_MAIN_BRANCH prerequisite to keep this change small, instead it's now effectively a noop. It can be removed in some later change. The only remaining occurrences in t5526-fetch-submodules.sh can be removed without breakage with: perl -pi -e 's/PREPARE_FOR_MAIN_BRANCH //g' t/t5526-fetch-submodules.sh Which at this point leaves only the now-unused prerequisite declaration in test-lib.sh. The coverage in t9902-completion.sh was restored by partially reverting[3]. After that we were left with one test in a mixed state. It setup "master" but tested for "mai". Change it back to "mas", pending a more complete refactoring of that test. This change only conflicts between next..seen by clashing with Peter Kaestle's in-flights submodule fix[4]. Fixing the resulting logic error in t5526-fetch-submodules.sh is trivial, simply: - compare_refs_in_dir A origin/master B origin/master + compare_refs_in_dir A origin/main B origin/main 1. 66713e84e7 ("tests: prepare aligned mentions of the default branch name", 2020-10-23) 2. https://public-inbox.org/git/pull.762.v2.git.1605629547.gitgitgadget@gmail.com/ 3. 8164360fc8 ("t9902: prepare a test for the upcoming default branch name", 2020-10-23) 4. https://public-inbox.org/git/1605196853-37359-1-git-send-email-peter.kaestle@nokia.com/ Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- On Tue, Nov 17 2020, Johannes Schindelin via GitGitGadget wrote: > To avoid even more conflicts with topics that did not even make it to seen > yet, this patch series specifically excludes t3404, t4013, t5310, t5526, > t6300, t7064, t7817, t9902: in those test scripts, we will still use master > for the time being. Once the topics in question have settled, I will send > the appropriate follow-up patches to adjust them to use main instead. This is not a replacement for that subsequent cleanup, but seems like a simple enough thing to have now to reinstate the missing test coverage. Perhaps there's some topics not in "seen" that you have in mind as conflicting, but as noted above the conflict produced here with in-flight in "seen" is trivial. Seems worth having that sooner than later if Junio's willing juggle that. t/t5526-fetch-submodules.sh | 6 +++--- t/t9902-completion.sh | 6 +++--- t/test-lib.sh | 9 +++------ 3 files changed, 9 insertions(+), 12 deletions(-)