From: Junio C Hamano <gitster@pobox•com>
To: Felipe Contreras <felipe.contreras@gmail•com>
Cc: git@vger•kernel.org
Subject: Re: [PATCH 4/4] t: branch: improve test rollback
Date: Tue, 03 Sep 2013 12:32:16 -0700 [thread overview]
Message-ID: <xmqqwqmxr9a7.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <1377923511-20787-5-git-send-email-felipe.contreras@gmail.com> (Felipe Contreras's message of "Fri, 30 Aug 2013 23:31:51 -0500")
Felipe Contreras <felipe.contreras@gmail•com> writes:
> After every test the environment should be as close as to how it was
> before as possible.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail•com>
> ---
Very good in general; a few points (not "the patch breaks things",
but more like "tests after the patch are still depending on the
previous state") below, though.
> t/t3200-branch.sh | 71 +++++++++++++++++++++++++++----------------------------
> 1 file changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> index d85306f..3d4f634 100755
> --- a/t/t3200-branch.sh
> +++ b/t/t3200-branch.sh
> @@ -328,7 +328,7 @@ test_expect_success 'tracking setup fails on non-matching refspec' '
> '
>
> test_expect_success 'test tracking setup via config' '
> - git config branch.autosetupmerge true &&
> + test_config branch.autosetupmerge true &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -338,20 +338,18 @@ test_expect_success 'test tracking setup via config' '
> '
>
> test_expect_success 'test overriding tracking setup via --no-track' '
> - git config branch.autosetupmerge true &&
> + test_config branch.autosetupmerge true &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> git branch --no-track my2 local/master &&
> - git config branch.autosetupmerge false &&
> ! test "$(git config branch.my2.remote)" = local &&
> ! test "$(git config branch.my2.merge)" = refs/heads/master
> '
>
> test_expect_success 'no tracking without .fetch entries' '
> - git config branch.autosetupmerge true &&
> + test_config branch.autosetupmerge true &&
> git branch my6 s &&
> - git config branch.autosetupmerge false &&
> test -z "$(git config branch.my6.remote)" &&
> test -z "$(git config branch.my6.merge)"
> '
The four tests after this one used to expect to start with
branch.autosetupmerge explicitly set to false, but this change
leaves the variable unset---as a side effect we start testing a
different thing. As long as we do not introduce a bug that allows
an exlicit branch.autosetupmerge=false defeat --track on the command
line, this side effect does not hurt, but I think the 'test tracking
setup via --track but deeper' was the only test that checks this
interaction between 'false in config, --track on the command line'
combination.
> @@ -386,9 +384,8 @@ test_expect_success 'test --track without .fetch entries' '
> '
>
> test_expect_success 'branch from non-branch HEAD w/autosetupmerge=always' '
> - git config branch.autosetupmerge always &&
> - git branch my9 HEAD^ &&
> - git config branch.autosetupmerge false
> + test_config branch.autosetupmerge always &&
> + git branch my9 HEAD^
> '
Likewise for three subsequent tests but what they test are not
primarily whether tracking is done, so it does not matter as much as
the previous.
> test_expect_success 'branch from non-branch HEAD w/--track causes failure' '
> @@ -405,9 +402,9 @@ test_expect_success '--set-upstream-to fails on multiple branches' '
> '
>
> test_expect_success '--set-upstream-to fails on detached HEAD' '
> + test_when_finished "git checkout -" &&
> git checkout HEAD^{} &&
> - test_must_fail git branch --set-upstream-to master &&
> - git checkout -
> + test_must_fail git branch --set-upstream-to master
> '
>
> test_expect_success '--set-upstream-to fails on a missing dst branch' '
> @@ -459,9 +456,9 @@ test_expect_success '--unset-upstream should fail on multiple branches' '
> '
>
> test_expect_success '--unset-upstream should fail on detached HEAD' '
> + test_when_finished "git checkout -" &&
> git checkout HEAD^{} &&
> - test_must_fail git branch --unset-upstream &&
> - git checkout -
> + test_must_fail git branch --unset-upstream
> '
>
> test_expect_success 'test --unset-upstream on a particular branch' '
> @@ -540,7 +537,8 @@ test_expect_success 'checkout -b with -l makes reflog when core.logAllRefUpdates
> '
>
> test_expect_success 'avoid ambiguous track' '
> - git config branch.autosetupmerge true &&
> + test_when_finished "git remote rm ambi1 && git remote rm ambi2" &&
> + test_config branch.autosetupmerge true &&
> git config remote.ambi1.url lalala &&
> git config remote.ambi1.fetch refs/heads/lalala:refs/heads/master &&
> git config remote.ambi2.url lilili &&
> @@ -552,7 +550,7 @@ test_expect_success 'avoid ambiguous track' '
> test_expect_success 'autosetuprebase local on a tracked local branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase local &&
> + test_config branch.autosetuprebase local &&
> (git show-ref -q refs/remotes/local/o || git fetch local) &&
> git branch mybase &&
> git branch --track myr1 mybase &&
> @@ -564,7 +562,7 @@ test_expect_success 'autosetuprebase local on a tracked local branch' '
> test_expect_success 'autosetuprebase always on a tracked local branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase always &&
> + test_config branch.autosetuprebase always &&
> (git show-ref -q refs/remotes/local/o || git fetch local) &&
> git branch mybase2 &&
> git branch --track myr2 mybase &&
> @@ -576,7 +574,7 @@ test_expect_success 'autosetuprebase always on a tracked local branch' '
> test_expect_success 'autosetuprebase remote on a tracked local branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase remote &&
> + test_config branch.autosetuprebase remote &&
> (git show-ref -q refs/remotes/local/o || git fetch local) &&
> git branch mybase3 &&
> git branch --track myr3 mybase2 &&
> @@ -588,7 +586,7 @@ test_expect_success 'autosetuprebase remote on a tracked local branch' '
> test_expect_success 'autosetuprebase never on a tracked local branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase never &&
> + test_config branch.autosetuprebase never &&
> (git show-ref -q refs/remotes/local/o || git fetch local) &&
> git branch mybase4 &&
> git branch --track myr4 mybase2 &&
> @@ -600,7 +598,7 @@ test_expect_success 'autosetuprebase never on a tracked local branch' '
> test_expect_success 'autosetuprebase local on a tracked remote branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase local &&
> + test_config branch.autosetuprebase local &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> git branch --track myr5 local/master &&
> test "$(git config branch.myr5.remote)" = local &&
> @@ -611,7 +609,7 @@ test_expect_success 'autosetuprebase local on a tracked remote branch' '
> test_expect_success 'autosetuprebase never on a tracked remote branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase never &&
> + test_config branch.autosetuprebase never &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> git branch --track myr6 local/master &&
> test "$(git config branch.myr6.remote)" = local &&
> @@ -622,7 +620,7 @@ test_expect_success 'autosetuprebase never on a tracked remote branch' '
> test_expect_success 'autosetuprebase remote on a tracked remote branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase remote &&
> + test_config branch.autosetuprebase remote &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> git branch --track myr7 local/master &&
> test "$(git config branch.myr7.remote)" = local &&
> @@ -633,7 +631,7 @@ test_expect_success 'autosetuprebase remote on a tracked remote branch' '
> test_expect_success 'autosetuprebase always on a tracked remote branch' '
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> - git config branch.autosetuprebase remote &&
> + test_config branch.autosetuprebase remote &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> git branch --track myr8 local/master &&
> test "$(git config branch.myr8.remote)" = local &&
> @@ -642,7 +640,7 @@ test_expect_success 'autosetuprebase always on a tracked remote branch' '
> '
>
> test_expect_success 'autosetuprebase unconfigured on a tracked remote branch' '
> - git config --unset branch.autosetuprebase &&
> + test_unconfig branch.autosetuprebase &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -684,7 +682,7 @@ test_expect_success 'autosetuprebase unconfigured on untracked remote branch' '
> '
>
> test_expect_success 'autosetuprebase never on an untracked local branch' '
> - git config branch.autosetuprebase never &&
> + test_config branch.autosetuprebase never &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -695,7 +693,7 @@ test_expect_success 'autosetuprebase never on an untracked local branch' '
> '
>
> test_expect_success 'autosetuprebase local on an untracked local branch' '
> - git config branch.autosetuprebase local &&
> + test_config branch.autosetuprebase local &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -706,7 +704,7 @@ test_expect_success 'autosetuprebase local on an untracked local branch' '
> '
>
> test_expect_success 'autosetuprebase remote on an untracked local branch' '
> - git config branch.autosetuprebase remote &&
> + test_config branch.autosetuprebase remote &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -717,7 +715,7 @@ test_expect_success 'autosetuprebase remote on an untracked local branch' '
> '
>
> test_expect_success 'autosetuprebase always on an untracked local branch' '
> - git config branch.autosetuprebase always &&
> + test_config branch.autosetuprebase always &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -728,7 +726,7 @@ test_expect_success 'autosetuprebase always on an untracked local branch' '
> '
>
> test_expect_success 'autosetuprebase never on an untracked remote branch' '
> - git config branch.autosetuprebase never &&
> + test_config branch.autosetuprebase never &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -739,7 +737,7 @@ test_expect_success 'autosetuprebase never on an untracked remote branch' '
> '
>
> test_expect_success 'autosetuprebase local on an untracked remote branch' '
> - git config branch.autosetuprebase local &&
> + test_config branch.autosetuprebase local &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -750,7 +748,7 @@ test_expect_success 'autosetuprebase local on an untracked remote branch' '
> '
>
> test_expect_success 'autosetuprebase remote on an untracked remote branch' '
> - git config branch.autosetuprebase remote &&
> + test_config branch.autosetuprebase remote &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -761,7 +759,7 @@ test_expect_success 'autosetuprebase remote on an untracked remote branch' '
> '
>
> test_expect_success 'autosetuprebase always on an untracked remote branch' '
> - git config branch.autosetuprebase always &&
> + test_config branch.autosetuprebase always &&
> git config remote.local.url . &&
> git config remote.local.fetch refs/heads/*:refs/remotes/local/* &&
> (git show-ref -q refs/remotes/local/master || git fetch local) &&
> @@ -772,8 +770,8 @@ test_expect_success 'autosetuprebase always on an untracked remote branch' '
> '
>
> test_expect_success 'autosetuprebase always on detached HEAD' '
> - git config branch.autosetupmerge always &&
> - test_when_finished git checkout master &&
> + test_when_finished "git checkout -" &&
I think the explicit 'master' was better. We are not in the
business of checking @{-1} completion here, and depending on the
outcome of the "git checkout" in the test, "checkout -" will take us
to a different place, no?
> + test_config branch.autosetupmerge always &&
This used to be propagated down, but now branch.autosetupmerge is
kept unset after this. The tests after this one do not seem to
newly create branches that need auto-setup-merge, so I think this
change is very good.
> git checkout HEAD^0 &&
> git branch my11 &&
> test -z "$(git config branch.my11.remote)" &&
> @@ -781,15 +779,15 @@ test_expect_success 'autosetuprebase always on detached HEAD' '
> '
>
> test_expect_success 'detect misconfigured autosetuprebase (bad value)' '
> - git config branch.autosetuprebase garbage &&
> + test_config branch.autosetuprebase garbage &&
> test_must_fail git branch
> '
>
> test_expect_success 'detect misconfigured autosetuprebase (no value)' '
> - git config --unset branch.autosetuprebase &&
> + test_when_finished "test_unconfig branch.autosetuprebase" &&
> + test_unconfig branch.autosetuprebase &&
> echo "[branch] autosetuprebase" >>.git/config &&
> - test_must_fail git branch &&
> - git config --unset branch.autosetuprebase
> + test_must_fail git branch
> '
>
> test_expect_success 'attempt to delete a branch without base and unmerged to HEAD' '
> @@ -856,6 +854,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' '
> '
>
> test_expect_success 'refuse --edit-description on unborn branch for now' '
> + test_when_finished "git checkout -" &&
I am not sure if this is a good change. Depending on the outcome of
the "git checkout" in the test (it may succeed and set @{-1} to the
branch we were on when we entered the test, or it may fail and leave
@{-1} to the branch before we were on when we entered the test),
this will take us to a different place, no?
> write_script editor <<-\EOF &&
> echo "New contents" >"$1"
> EOF
next prev parent reply other threads:[~2013-09-03 19:32 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-31 4:31 [PATCH 0/4] t: branch: fixes and cleanups Felipe Contreras
2013-08-31 4:31 ` [PATCH 1/4] t: branch: trivial style fix Felipe Contreras
2013-09-03 19:13 ` Junio C Hamano
2013-08-31 4:31 ` [PATCH 2/4] t: branch: fix typo Felipe Contreras
2013-08-31 4:31 ` [PATCH 3/4] t: branch: fix broken && chains Felipe Contreras
2013-08-31 4:31 ` [PATCH 4/4] t: branch: improve test rollback Felipe Contreras
2013-09-03 19:32 ` Junio C Hamano [this message]
2013-09-03 22:10 ` Felipe Contreras
2013-09-03 22:59 ` Junio C Hamano
2013-09-03 23:03 ` Felipe Contreras
2013-09-04 17:19 ` Junio C Hamano
2013-09-08 3:02 ` Felipe Contreras
2013-09-08 4:56 ` Jeff King
2013-09-08 5:01 ` Felipe Contreras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqwqmxr9a7.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=felipe.contreras@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox