Avoid main as branch name in the test suite #743
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
dscho
force-pushed
the
avoid-main-as-branch-name-in-tests
branch
from
3d38276
to
6045ceb
Compare
|
/submit |
|
Submitted as pull.743.git.1601888196.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -76,24 +76,24 @@ test_expect_success 'reflog of worktrees/xx/HEAD' ' | |||
| test_cmp expected actual.wt2 | |||
There was a problem hiding this comment.
On the Git mailing list, Jonathan Nieder wrote (reply to this):
Hi,
Johannes Schindelin wrote:
> In preparation for a patch series that will change the fall-back for
> `init.defaultBranch` to `main`, let's not use `main` as ref name in this
> test script.
Interesting. I assume the issue is this line?
- git -C fer1/repo for-each-ref --format="%(refname)" | grep main >actual &&
I.e., it's not actually that naming a worktree "main" will break
anything, but just that the test catches refs/heads/main in the same
net when it does grepping?
If the commit message explains that, then this patch looks good to me.
Without such an explanation, it would make me fear that we have some
underlying bug in "git worktree".
Thanks,
Jonathan
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Jonathan,
On Mon, 5 Oct 2020, Jonathan Nieder wrote:
>
> Johannes Schindelin wrote:
>
> > In preparation for a patch series that will change the fall-back for
> > `init.defaultBranch` to `main`, let's not use `main` as ref name in
> > this test script.
>
> Interesting. I assume the issue is this line?
>
> - git -C fer1/repo for-each-ref --format="%(refname)" | grep main >actual &&
>
> I.e., it's not actually that naming a worktree "main" will break
> anything, but just that the test catches refs/heads/main in the same
> net when it does grepping?
Right.
> If the commit message explains that, then this patch looks good to me.
> Without such an explanation, it would make me fear that we have some
> underlying bug in "git worktree".
Indeed. This is my current revision of the commit message:
t1415: avoid using `main` as ref name
In preparation for a patch series that will change the fall-back for
`init.defaultBranch` to `main`, let's not use `main` as ref name in this
test script.
Otherwise, the `git for-each-ref ... | grep main` which wants to catch
those refs would also unexpectedly catch `refs/heads/main`.
Since the refs in question are worktree-local ones (i.e. each worktree
has their own, just like `HEAD`), and since the test case already uses a
secondary worktree called "second", let's use the name "first" for those
refs instead.
While at it, adjust the test titles that talk about a "repo" when they
meant a "worktree" instead.
Ciao,
Dscho
|
User |
| @@ -171,7 +171,7 @@ test_expect_success '--full-diff is not affected by --parents' ' | |||
| test_expect_success 'rebuild repo' ' | |||
There was a problem hiding this comment.
On the Git mailing list, Jonathan Nieder wrote (reply to this):
Johannes Schindelin wrote:
> In the near future, we want to change Git's default branch name to
> `main`. In preparation for that, stop using it as a branch name in the
> test suite. Replace that branch name by `primary`.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> t/t6012-rev-list-simplify.sh | 8 ++++----
> t/t6400-merge-df.sh | 8 ++++----
> t/t6409-merge-subtree.sh | 12 ++++++------
> t/t6430-merge-recursive.sh | 4 ++--
> 4 files changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
> index b6fa43ace0..f1296c29e6 100755
> --- a/t/t6012-rev-list-simplify.sh
> +++ b/t/t6012-rev-list-simplify.sh
> @@ -171,7 +171,7 @@ test_expect_success '--full-diff is not affected by --parents' '
> test_expect_success 'rebuild repo' '
> rm -rf .git * &&
> git init &&
> - git switch -c main &&
> + git switch -c primary &&
Is there a secondary corresponding to this primary?
I guess the idea is that this is the trunk that other branches branch
from? Looking at the history, it seems that this test was added
relatively recently and it may have had the upcoming branch name change
in mind (or in other words if it were an older test it might be expected
to use "master").
That suggests an alternative that is agnostic to init.defaultBranch:
what if this uses "git switch -C main"?
Thanks,
Jonathan
There was a problem hiding this comment.
On the Git mailing list, Johannes Schindelin wrote (reply to this):
Hi Jonathan,
On Mon, 5 Oct 2020, Jonathan Nieder wrote:
> Johannes Schindelin wrote:
>
> > In the near future, we want to change Git's default branch name to
> > `main`. In preparation for that, stop using it as a branch name in the
> > test suite. Replace that branch name by `primary`.
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > t/t6012-rev-list-simplify.sh | 8 ++++----
> > t/t6400-merge-df.sh | 8 ++++----
> > t/t6409-merge-subtree.sh | 12 ++++++------
> > t/t6430-merge-recursive.sh | 4 ++--
> > 4 files changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/t/t6012-rev-list-simplify.sh b/t/t6012-rev-list-simplify.sh
> > index b6fa43ace0..f1296c29e6 100755
> > --- a/t/t6012-rev-list-simplify.sh
> > +++ b/t/t6012-rev-list-simplify.sh
> > @@ -171,7 +171,7 @@ test_expect_success '--full-diff is not affected by --parents' '
> > test_expect_success 'rebuild repo' '
> > rm -rf .git * &&
> > git init &&
> > - git switch -c main &&
> > + git switch -c primary &&
>
> Is there a secondary corresponding to this primary?
Nope, of course not ;-)
> I guess the idea is that this is the trunk that other branches branch
> from? Looking at the history, it seems that this test was added
> relatively recently and it may have had the upcoming branch name change
> in mind (or in other words if it were an older test it might be expected
> to use "master").
I guess that Stolee (Cc:ed) had something like that in mind.
When I look at 8d049e182e2 (revision: --show-pulls adds helpful
merges, 2020-04-10), I get the impression that does not _really_ care
about the name of the main branch, it just wants to know the name so it
can switch back and forth.
If I had had the presence of mind when reviewing that patch back in April,
I would probably have advocated for the use of `git switch -`...
In any case, I would like to keep this consistent with the remainder of
the test scripts modified by this patch, and use the relatively neutral
`topic` here.
Ciao,
Dscho
> That suggests an alternative that is agnostic to init.defaultBranch:
> what if this uses "git switch -C main"?
>
> Thanks,
> Jonathan
>
There was a problem hiding this comment.
On the Git mailing list, Derrick Stolee wrote (reply to this):
On 10/8/2020 4:05 AM, Johannes Schindelin wrote:
> On Mon, 5 Oct 2020, Jonathan Nieder wrote:
>> Johannes Schindelin wrote:
>>> - git switch -c main &&
>>> + git switch -c primary &&
>>
>> Is there a secondary corresponding to this primary?
>
> Nope, of course not ;-)
>
>> I guess the idea is that this is the trunk that other branches branch
>> from? Looking at the history, it seems that this test was added
>> relatively recently and it may have had the upcoming branch name change
>> in mind (or in other words if it were an older test it might be expected
>> to use "master").
>
> I guess that Stolee (Cc:ed) had something like that in mind.
>
> When I look at 8d049e182e2 (revision: --show-pulls adds helpful
> merges, 2020-04-10), I get the impression that does not _really_ care
> about the name of the main branch, it just wants to know the name so it
> can switch back and forth.
The branch name here is not important, so please replace it
with whatever works best for you.
What _is_ important is keeping the commit messages in agreement with
the commented ASCII-art DAG above this test. It does not mention the
branch names, so this change will not cause an inconsistent comment.
Thanks,
-Stolee
In preparation for a patch series that will change the fall-back for `init.defaultBranch` to `main`, let's not use `main` as ref name in this test script. Otherwise, the `git for-each-ref ... | grep main` which wants to catch those refs would also unexpectedly catch `refs/heads/main`. Since the refs in question are worktree-local ones (i.e. each worktree has their own, just like `HEAD`), and since the test case already uses a secondary worktree called "second", let's use the name "first" for those refs instead. While at it, adjust the test titles that talk about a "repo" when they meant a "worktree" instead. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
In the near future, we want to change Git's default branch name to `main`. In preparation for that, stop using it as a branch name in the test suite. Replace that branch name by `topic`, the same name we used to rename variations of `master` in b6211b8 (tests: avoid variations of the `master` branch name, 2020-09-26). Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
dscho
force-pushed
the
avoid-main-as-branch-name-in-tests
branch
from
6045ceb
to
b187571
Compare
|
User |
|
/submit |
|
Submitted as pull.743.v2.git.1602152027.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
User |
|
This branch is now known as |
|
This patch series was integrated into seen via git@8d3d3c6. |
|
This patch series was integrated into seen via git@cb1acb9. |
|
This patch series was integrated into seen via git@cfe2454. |
|
This patch series was integrated into next via git@1c02be0. |
|
This patch series was integrated into seen via git@62564ba. |
|
This patch series was integrated into next via git@62564ba. |
|
This patch series was integrated into master via git@62564ba. |
|
Closed via 62564ba. |
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.
In preparation for changing the default branch name to
main, let's stop using it as non-default branch name in the test suite.This is the last preparatory step before the patch series that intends to change the default branch name to
main. See #655 for the entire game plan.Changes since v1:
t1415patch elaborates a but more about the rationale for the change.primary, the second patch renames all of those branches totopic, imitating b6211b8 (tests: avoid variations of themasterbranch name, 2020-09-26).cc: Jonathan Nieder jrnieder@gmail.com
cc: Johannes Schindelin Johannes.Schindelin@gmx.de
cc: Derrick Stolee stolee@gmail.com