tests: use main as default branch name
#762
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
The pull request has 40 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
dscho
force-pushed
the
use-main-as-default-branch-name
branch
from
2da7fa2
to
8264d1d
Compare
|
The pull request has 61 commits. The max allowed is 30. Please split the patch series into multiple pull requests. Also consider squashing related commits. |
|
There are merge commits in this Pull Request: Please rebase the branch and force-push. |
dscho
force-pushed
the
use-main-as-default-branch-name
branch
from
8264d1d
to
c071acb
Compare
This comment has been minimized.
This comment has been minimized.
dscho
force-pushed
the
use-main-as-default-branch-name
branch
from
c071acb
to
00bc391
Compare
This comment has been minimized.
This comment has been minimized.
dscho
force-pushed
the
use-main-as-default-branch-name
branch
from
00bc391
to
735b7ab
Compare
This comment has been minimized.
This comment has been minimized.
dscho
force-pushed
the
use-main-as-default-branch-name
branch
from
735b7ab
to
f853fa9
Compare
|
/submit |
|
Submitted as pull.762.git.1605221038.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
On the Git mailing list, Felipe Contreras wrote (reply to this): |
|
User |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
refs.c
Outdated
| @@ -575,7 +575,7 @@ char *repo_default_branch_name(struct repository *r) | |||
| die(_("could not retrieve `%s`"), config_display_key); | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Thu, Nov 12 2020, Don Goodman-Wilson via GitGitGadget wrote:
> The current default name for the initial branch is a loaded term, and
> many Open Source projects renamed their principal branches already. A
> common choice appears to be `main`.
>
> Let's follow their lead and change the default of `init.defaultBranch`.
I think it makes sense to split this change off from a 28-series test
cleanup series.
> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> index bd3fa3c6da..1b0abcb0f8 100644
> --- a/t/lib-submodule-update.sh
> +++ b/t/lib-submodule-update.sh
> @@ -144,7 +144,7 @@ create_lib_submodule_repo () {
> git checkout -b valid_sub1 &&
> git revert HEAD &&
>
> - git checkout "${GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME-master}"
> + git checkout "${GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME-main}"
Earlier in that function we're doing a "git init". With all this test
cleanup I wonder why not just "git rev-parse --abbrev-ref" to get the
default name, instead of carrying the hardcoding forward.
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-169209211-1605276555=:18437
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi =C3=86var,
On Fri, 13 Nov 2020, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:
>
> On Thu, Nov 12 2020, Don Goodman-Wilson via GitGitGadget wrote:
>
> > The current default name for the initial branch is a loaded term, and
> > many Open Source projects renamed their principal branches already. A
> > common choice appears to be `main`.
> >
> > Let's follow their lead and change the default of `init.defaultBranch`=
.
>
> I think it makes sense to split this change off from a 28-series test
> cleanup series.
It is not a test cleanup. It is a series of 27 patches preparing the test
suite for the change made in the 28th patch.
I don't think that it is a good idea to split off that 28th patch from
the patches whose entire purpose is to prepare for that 28th patch.
>
> > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh
> > index bd3fa3c6da..1b0abcb0f8 100644
> > --- a/t/lib-submodule-update.sh
> > +++ b/t/lib-submodule-update.sh
> > @@ -144,7 +144,7 @@ create_lib_submodule_repo () {
> > git checkout -b valid_sub1 &&
> > git revert HEAD &&
> >
> > - git checkout "${GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME-master}"
> > + git checkout "${GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME-main}"
>
> Earlier in that function we're doing a "git init". With all this test
> cleanup I wonder why not just "git rev-parse --abbrev-ref" to get the
> default name, instead of carrying the hardcoding forward.
My goal was to keep everything as close to its original as possible. In
v2.29.2, this line reads:
git checkout master
See https://github.com/git/git/blob/v2.29.2/t/lib-submodule-update.sh#L147
to convince yourself.
Personally, I would have used something like
main=3D$(git symbolic-ref --short HEAD) &&
[...]
git checkout $main
instead of what you suggested. That's a topic for another patch (series),
though.
Ciao,
Dscho
--8323328-169209211-1605276555=:18437--
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Ævar,
>
> On Fri, 13 Nov 2020, Ævar Arnfjörð Bjarmason wrote:
>
>>
>> On Thu, Nov 12 2020, Don Goodman-Wilson via GitGitGadget wrote:
>>
>> > The current default name for the initial branch is a loaded term, and
>> > many Open Source projects renamed their principal branches already. A
>> > common choice appears to be `main`.
>> >
>> > Let's follow their lead and change the default of `init.defaultBranch`.
>>
>> I think it makes sense to split this change off from a 28-series test
>> cleanup series.
>
> It is not a test cleanup. It is a series of 27 patches preparing the test
> suite for the change made in the 28th patch.
>
> I don't think that it is a good idea to split off that 28th patch from
> the patches whose entire purpose is to prepare for that 28th patch.
Well, I personally think that the "purpose" of the first 27 patches
must be updated if that is the case.
The test should NOT be prepared only to work in the post "switch
from master to main" world.
Instead, what we want to see is that the test would work whether the
fallback default value for init.defaultBranch is 'master' (i.e. old
world) or 'main (i.e. a possible new world). Perhaps by using the
GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME mechanism for tests that relies
on a particular value to be the fallback default. So in the sense,
the goal the first 26 patches support is the 27th one, which is the
most important one in the series from _my_ point of view. We get
ourselves prepared so that 28th one can happen at any time.
That way, as long as the first 27 patches land, we will keep the
same test coverage as we've always had, regardless of the timing
we actually ship the 28th one to the production environment.
> Personally, I would have used something like
>
> main=$(git symbolic-ref --short HEAD) &&
> [...]
> git checkout $main
>
> instead of what you suggested. That's a topic for another patch (series),
> though.
Yeah, I would have used $primary or some word that is neither these
m* names, but I think that is a good alternative, when workable, to
the GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME mechanism. When the golden
master compared with actual output hardcodes the expected branch
names, however, the approach becomes awkward, though.
Thanks.
|
User |
| @@ -48,11 +48,11 @@ test_expect_success 'pulling into void' ' | |||
| test_cmp file cloned/file | |||
There was a problem hiding this comment.
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this):
On Thu, Nov 12 2020, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> This trick was performed via
>
> $ (cd t &&
> sed -i -e 's/master/main/g' -e 's/MASTER/MAIN/g' \
> -e 's/Master/Main/g' -e 's/naster/nain/g' -- t55[23]*.sh)
>
> Note that t5533 contains a variation of the name `master` (`naster`)
> that we rename here, too.
>
> This commit allows us to define
> `GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main` for that range of tests.
There's also a "naster" in t/t1402-check-ref-format.sh that's not
changed here and missed by 02/28 of this series.
Is there some meaning to the name "nain" and "naster" that I'm missing?
If not can we just call this "topic" or something while we're at it?
I.e. this on top (just s/nain/topic/g):
diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
index 9fcec604c3..4e33ec1fb9 100755
--- a/t/t5533-push-cas.sh
+++ b/t/t5533-push-cas.sh
@@ -201,12 +201,12 @@ test_expect_success 'cover everything with default force-with-lease (protected)'
setup_srcdst_basic &&
(
cd src &&
- git branch nain main^
+ git branch topic main^
) &&
git ls-remote src refs/heads/\* >expect &&
(
cd dst &&
- test_must_fail git push --force-with-lease origin main main:nain
+ test_must_fail git push --force-with-lease origin main main:topic
) &&
git ls-remote src refs/heads/\* >actual &&
test_cmp expect actual
@@ -216,16 +216,16 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
setup_srcdst_basic &&
(
cd src &&
- git branch nain main^
+ git branch topic main^
) &&
(
cd dst &&
git fetch &&
- git push --force-with-lease origin main main:nain
+ git push --force-with-lease origin main main:topic
) &&
git ls-remote dst refs/heads/main |
- sed -e "s/main/nain/" >expect &&
- git ls-remote src refs/heads/nain >actual &&
+ sed -e "s/main/topic/" >expect &&
+ git ls-remote src refs/heads/topic >actual &&
test_cmp expect actual
'
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
This message is in MIME format. The first part should be readable text,
while the remaining parts are likely unreadable without MIME-aware tools.
--8323328-371731460-1605277095=:18437
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Hi =C3=86var,
On Fri, 13 Nov 2020, =C3=86var Arnfj=C3=B6r=C3=B0 Bjarmason wrote:
>
> On Thu, Nov 12 2020, Johannes Schindelin via GitGitGadget wrote:
>
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > This trick was performed via
> >
> > $ (cd t &&
> > sed -i -e 's/master/main/g' -e 's/MASTER/MAIN/g' \
> > -e 's/Master/Main/g' -e 's/naster/nain/g' -- t55[23]*.sh)
> >
> > Note that t5533 contains a variation of the name `master` (`naster`)
> > that we rename here, too.
> >
> > This commit allows us to define
> > `GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=3Dmain` for that range of tests.
>
> There's also a "naster" in t/t1402-check-ref-format.sh that's not
> changed here and missed by 02/28 of this series.
Whoops, you're right. It's not even new: 7c3f847aad7 (check-ref-format
=2D-branch: do not expand @{...} outside repository, 2017-10-17) introduce=
d
it. I missed that one, thank you for pointing it out.
I will follow up with a separate patch to fix that, once the dust settles
on this here patch series.
> Is there some meaning to the name "nain" and "naster" that I'm missing?
I guess the original reasoning was to stay close to the original default
branch name. I'm not sure whether it is worth changing that idea. Like, if
you ask me whether it hurts to use "nain"? I looked up possible
connotations, and the most common one was "grandmother", and that's rather
endearing to me.
Ciao,
Dscho
> If not can we just call this "topic" or something while we're at it?
> I.e. this on top (just s/nain/topic/g):
>
> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index 9fcec604c3..4e33ec1fb9 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -201,12 +201,12 @@ test_expect_success 'cover everything with default=
force-with-lease (protected)'
> setup_srcdst_basic &&
> (
> cd src &&
> - git branch nain main^
> + git branch topic main^
> ) &&
> git ls-remote src refs/heads/\* >expect &&
> (
> cd dst &&
> - test_must_fail git push --force-with-lease origin main main:nain
> + test_must_fail git push --force-with-lease origin main main:topic
> ) &&
> git ls-remote src refs/heads/\* >actual &&
> test_cmp expect actual
> @@ -216,16 +216,16 @@ test_expect_success 'cover everything with default=
force-with-lease (allowed)' '
> setup_srcdst_basic &&
> (
> cd src &&
> - git branch nain main^
> + git branch topic main^
> ) &&
> (
> cd dst &&
> git fetch &&
> - git push --force-with-lease origin main main:nain
> + git push --force-with-lease origin main main:topic
> ) &&
> git ls-remote dst refs/heads/main |
> - sed -e "s/main/nain/" >expect &&
> - git ls-remote src refs/heads/nain >actual &&
> + sed -e "s/main/topic/" >expect &&
> + git ls-remote src refs/heads/topic >actual &&
> test_cmp expect actual
> '
>
>
--8323328-371731460-1605277095=:18437--
There was a problem hiding this comment.
On the Git mailing list, Junio C Hamano wrote (reply to this):
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> Is there some meaning to the name "nain" and "naster" that I'm missing?
Not for "naster", which came from me who just chose "any single
letter" followed by "aster" to match 'master', and there is no other
reason. I think Dscho just followed suit.
> If not can we just call this "topic" or something while we're at it?
> I.e. this on top (just s/nain/topic/g):
>
> diff --git a/t/t5533-push-cas.sh b/t/t5533-push-cas.sh
> index 9fcec604c3..4e33ec1fb9 100755
> --- a/t/t5533-push-cas.sh
> +++ b/t/t5533-push-cas.sh
> @@ -201,12 +201,12 @@ test_expect_success 'cover everything with default force-with-lease (protected)'
> setup_srcdst_basic &&
> (
> cd src &&
> - git branch nain main^
> + git branch topic main^
> ) &&
> git ls-remote src refs/heads/\* >expect &&
> (
> cd dst &&
> - test_must_fail git push --force-with-lease origin main main:nain
> + test_must_fail git push --force-with-lease origin main main:topic
> ) &&
> git ls-remote src refs/heads/\* >actual &&
> test_cmp expect actual
> @@ -216,16 +216,16 @@ test_expect_success 'cover everything with default force-with-lease (allowed)' '
> setup_srcdst_basic &&
> (
> cd src &&
> - git branch nain main^
> + git branch topic main^
> ) &&
> (
> cd dst &&
> git fetch &&
> - git push --force-with-lease origin main main:nain
> + git push --force-with-lease origin main main:topic
> ) &&
> git ls-remote dst refs/heads/main |
> - sed -e "s/main/nain/" >expect &&
> - git ls-remote src refs/heads/nain >actual &&
> + sed -e "s/main/topic/" >expect &&
> + git ls-remote src refs/heads/topic >actual &&
> test_cmp expect actual
> '
>
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
User |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
On the Git mailing list, Felipe Contreras wrote (reply to this): |
|
On the Git mailing list, Jeff King wrote (reply to this): |
|
User |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Johannes Schindelin wrote (reply to this): |
|
On the Git mailing list, Ævar Arnfjörð Bjarmason wrote (reply to this): |
|
This patch series was integrated into seen via git@efe08e5. |
|
This patch series was integrated into seen via git@30f6f31. |
|
This patch series was integrated into seen via git@aaba185. |
|
This patch series was integrated into seen via git@b3b7f06. |
|
This patch series was integrated into seen via git@61bafb6. |
|
This patch series was integrated into seen via git@14417fd. |
|
This patch series was integrated into seen via git@daef6a6. |
|
This patch series was integrated into seen via git@857d74e. |
|
This patch series was integrated into seen via git@34011a8. |
|
This patch series was integrated into seen via git@cccc3fa. |
|
This patch series was integrated into seen via git@c2340ae. |
|
This patch series was integrated into seen via git@c81baf0. |
|
This patch series was integrated into seen via git@0b9caa7. |
|
This patch series was integrated into seen via git@1861eb3. |
|
This patch series was integrated into next via git@0e93f0d. |
|
This patch series was integrated into seen via git@0e93f0d. |
|
This patch series was integrated into seen via git@ec55cb8. |
|
This patch series was integrated into seen via git@0e93f0d. |
|
This patch series was integrated into seen via git@b91fd79. |
|
This patch series was integrated into seen via git@174b178. |
|
This patch series was integrated into seen via git@b2b6ecb. |
|
This patch series was integrated into seen via git@27d7c85. |
|
This patch series was integrated into next via git@27d7c85. |
|
This patch series was integrated into master via git@27d7c85. |
|
Closed via 27d7c85. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This (big) patch series (almost) concludes the transition of Git's test suite to use
init.defaultBranch=mainthat was started injs/default-branch-name-part-4-minus-1, continued injs/default-branch-name-adjust-t5411and injs/default-branch-name-adjust-t5515. This transition prepares for changing the fall-back value ofinit.defaultBranchaccordingly (which will be done in a different set of patch series, with an appropriate deprecation period), reflecting what many open source projects already did followed by GitHub, Azure Repos and others.Before doing anything else, the series starts out with a patch that marks all the test scripts that expect a specific default branch name and won't pass with any other name.
Instead of one huge patch that reflects essentially a search-and-replace in the test suite, this patch series then splits the changes up into chunks that are intended to be smaller than 100kB so that they are not rejected by the Git mailing list. Interspersed between those changes are adjustments e.g. in alignment, to make it easier to review (or recreate) the search-and-replace patches.
Note that this branch is based on
next, mostly because it would otherwise conflict withen/merge-tests,jk/diff-release-filespec-fixandds/maintenance-part-3.To avoid even more conflicts with topics that did not even make it to
seenyet, this patch series specifically excludes t1309, t2106, t3040, t3404, t4013, t4015, t5310, t5526, t6300, t7064, t7817, t9902: in those test scripts, we will still usemasterfor the time being. Once the topics in question have settled, I will send the appropriate follow-up patches to adjust them to usemaininstead.Note that even after this patch series,
masteris still found int/, even outside of the tests we excluded specifically to avoid conflicts with other patch series that are currently in flight:t/perf/, thegit p4tests (becausegit p4still usesp4/masterto refer to the remote main branch), and some comments still refer to this name. I intend to address at least some of those, in patch series separate from the current one.Changes since v2:
casestatement setting the default branch name for each test script was replaced by an initial patch that sets the default branch name in those test scripts that need it (and only in those). This patch also modifies thelinux-gccjob to verify that the test suite runs with overridden default branch name (which the now-marked test scripts over-override).Changes since v1:
git init.nasterbynain.pk/subsub-fetch-fix.cc: Felipe Contreras felipe.contreras@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Jeff King peff@peff.net
cc: Jonathan Nieder jrnieder@gmail.com
cc: Philip Oakley philipoakley@iee.email
cc: Peter Hadlaw hadlawp@gmail.com
cc: Sergey Organov sorganov@gmail.com