[PATCH 0/7] Improve git-pull test coverage

[PATCH 0/7] Improve git-pull test coverage

[Paul Tan]

This patch series improves test coverage of git-pull.sh from 50.9%[1] to
73.4%[2]. Take the test coverage reports with a grain of salt though, as there
are known deficiencies[3].

This is part of my GSoC project to rewrite git-pull into a builtin. Improving
test coverage helps to prevent regressions that could occur due to the rewrite.

[1] http://pyokagan.github.io/git/20150430132408-a75942b//kcov-merged/git-pull.360f32c2.html
[2] http://pyokagan.github.io/git/20150502145725-c94dfc2//kcov-merged/git-pull.360f32c2.html
[3] https://github.com/pyokagan/git/commit/a7d6ab4677b97afd789b0ce860280c80f70f6e32

Paul Tan (7):
  t5520: test pulling multiple branches into an empty repository
  t5520: implement tests for no merge candidates cases
  t5520: test for failure if index has unresolved entries
  t5520: test work tree fast-forward when fetch updates head
  t5520: test --rebase with multiple branches
  t5520: test --rebase failure on unborn branch with index
  t5521: test --dry-run does not make any changes

 t/t5520-pull.sh         | 119 ++++++++++++++++++++++++++++++++++++++++++++++++
 t/t5521-pull-options.sh |  13 ++++++
 2 files changed, 132 insertions(+)

-- 
2.1.4

[PATCH 1/7] t5520: test pulling multiple branches into an empty repository

[Paul Tan]

Commit d09e79cb ("allow pulling into an empty repository") introduced
the ability to pull into an empty head. As pulling in multiple branches
does not make sense in this context, git-pull explicitly fails when
there is an empty head and multiple branches are specified.

Add a test to ensure that this safeguard works.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t5520-pull.sh | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 227d293..01ae1bf 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -86,6 +86,18 @@ test_expect_success 'pulling into void does not remove new staged files' '
 	)
 '
 
+test_expect_success 'refuse to pull multiple branches into void' '
+	git branch test master &&
+	test_when_finished "git branch -D test" &&
+	git init cloned-multiple-branches &&
+	test_when_finished "rm -rf cloned-multiple-branches" &&
+	(
+		cd cloned-multiple-branches &&
+		test_must_fail git pull .. master test 2>out &&
+		test_i18ngrep "Cannot merge multiple branches into empty head" out
+	)
+'
+
 test_expect_success 'test . as a remote' '
 
 	git branch copy master &&
-- 
2.1.4

[PATCH 2/7] t5520: implement tests for no merge candidates cases

[Paul Tan]

Commit a8c9bef4 fully established the current advices given by git-pull
for the different cases where git-fetch will not have anything marked
for merge:

1. We're not on a branch, so there is no branch
   configuration.

2. We're on a branch, but there is no configuration for
   this branch.

3. We fetched from the configured remote, but the
   configured branch to merge didn't get fetched (either
   it doesn't exist, or wasn't part of the fetch refspec).

4. We fetched from the non-default remote, but didn't
   specify a branch to merge. We can't use the configured
   one because it applies to the default remote.

5. We fetched from a specified remote, and a refspec was
   given, but it ended up not fetching anything.

Implement tests for the above 5 cases to ensure that the correct code
paths are triggered for each of these cases.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

I'm having trouble hitting the 1st case without resorting to the hack below. A
detached HEAD will always have no remote configured, and the code flow would
make it such that case (4) is hit in the detached HEAD case instead of case
(1).

 t/t5520-pull.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 01ae1bf..635ec02 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -122,6 +122,58 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	test `cat file` = modified
 '
 
+test_expect_success 'fail if not on a branch' '
+	cp .git/config .git/config.bak &&
+	test_when_finished "cp .git/config.bak .git/config" &&
+	git remote add test_remote . &&
+	git checkout HEAD^{} &&
+	test_when_finished "git checkout -f copy" &&
+	cat >>.git/config <<-\EOF &&
+	[branch ""]
+	remote = test_remote
+	EOF
+	test_must_fail git pull test_remote 2>out &&
+	test_i18ngrep "You are not currently on a branch" out
+'
+
+test_expect_success 'fail if no configuration for current branch' '
+	git remote add test_remote . &&
+	test_when_finished "git remote remove test_remote" &&
+	git checkout -b test copy &&
+	test_when_finished "git checkout -f copy && git branch -D test" &&
+	test_config branch.test.remote test_remote &&
+	test_must_fail git pull 2>out &&
+	test_i18ngrep "There is no tracking information" out
+'
+
+test_expect_success 'fail if upstream branch does not exist' "
+	git checkout -b test copy &&
+	test_when_finished 'git checkout -f copy && git branch -D test' &&
+	test_config branch.test.remote . &&
+	test_config branch.test.merge refs/heads/nonexisting &&
+	test_must_fail git pull 2>out &&
+	test_i18ngrep \"Your configuration specifies to merge with the ref 'nonexisting'\" out
+"
+
+test_expect_success 'fail if no branches specified with non-default remote' '
+	git clone --bare . test_repo &&
+	test_when_finished "rm -rf test_repo" &&
+	git remote add test_remote test_repo &&
+	test_when_finished "git remote remove test_remote" &&
+	git checkout -b test master &&
+	test_when_finished "git checkout -f master && git branch -D test" &&
+	test_config branch.test.remote . &&
+	test_must_fail git pull test_remote 2>out &&
+	test_i18ngrep "you must specify a branch on the command line" out
+'
+
+test_expect_success 'fail if wildcard spec does not match any refs' "
+	git checkout -b test copy &&
+	test_when_finished 'git checkout -f copy && git branch -D test' &&
+	test_must_fail git pull . 'refs/nonexisting1/*:refs/nonexisting2/*' 2>out &&
+	test_i18ngrep 'There are no candidates for merging' out
+"
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
-- 
2.1.4

[PATCH 3/7] t5520: test for failure if index has unresolved entries

[Paul Tan]

Commit d38a30df (Be more user-friendly when refusing to do something
because of conflict) introduced code paths to git-pull which will error
out with user-friendly advices if the user is in the middle of a merge
or has unmerged files.

Implement tests to ensure that git-pull will not run, and will print
these advices, if the user is in the middle of a merge or has unmerged
files in the index.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t5520-pull.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 635ec02..9f57e0d 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -174,6 +174,21 @@ test_expect_success 'fail if wildcard spec does not match any refs' "
 	test_i18ngrep 'There are no candidates for merging' out
 "
 
+test_expect_success 'fail if the index has unresolved entries' '
+	git checkout -b third master^ &&
+	test_when_finished "git checkout -f copy && git branch -D third" &&
+	echo file >expected &&
+	test_cmp expected file &&
+	echo modified2 >file &&
+	git commit -a -m modified2 &&
+	test_must_fail git pull . second &&
+	test_must_fail git pull . second 2>out &&
+	test_i18ngrep "Pull is not possible because you have unmerged files" out &&
+	git add file &&
+	test_must_fail git pull . second 2>out &&
+	test_i18ngrep "You have not concluded your merge" out
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
-- 
2.1.4

[PATCH 4/7] t5520: test work tree fast-forward when fetch updates head

[Paul Tan]

Since commit b10ac50f (Fix pulling into the same branch), git-pull,
upon detecting that git-fetch updated the current head, will
fast-forward the working tree to the updated head commit.

Implement tests to ensure that the fast-forward occurs in such a case,
as well as to ensure that the user-friendly advice is printed upon
failure.
---
 t/t5520-pull.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9f57e0d..25d519d 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -189,6 +189,27 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_i18ngrep "You have not concluded your merge" out
 '
 
+test_expect_success 'fast-forwards working tree if branch head is updated' '
+	git checkout -b third master^ &&
+	test_when_finished "git checkout -f copy && git branch -D third" &&
+	echo file >expected &&
+	test_cmp expected file &&
+	git pull . second:third 2>out &&
+	test_i18ngrep "fetch updated the current branch head" out &&
+	echo modified >expected &&
+	test_cmp expected file
+'
+
+test_expect_success 'fast-forward fails with conflicting work tree' '
+	git checkout -b third master^ &&
+	test_when_finished "git checkout -f copy && git branch -D third" &&
+	echo file >expected &&
+	test_cmp expected file &&
+	echo conflict >file &&
+	test_must_fail git pull . second:third 2>out &&
+	test_i18ngrep "Cannot fast-forward your working tree" out
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
-- 
2.1.4

[PATCH 5/7] t5520: test --rebase with multiple branches

[Paul Tan]

Since rebasing on top of multiple upstream branches does not make sense,
since commit 51b2ead0 ("disallow providing multiple upstream branches
to rebase, pull --rebase"), git-pull explicitly disallowed specifying
multiple branches in the rebase case.

Implement tests to ensure that git-pull fails and prints out the
user-friendly error message in such a case.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t5520-pull.sh | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 25d519d..17c63ff 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -223,6 +223,14 @@ test_expect_success '--rebase' '
 	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
 	test new = $(git show HEAD:file2)
 '
+
+test_expect_success '--rebase fails with multiple branches' '
+	git reset --hard before-rebase &&
+	test_must_fail git pull --rebase . copy master 2>out &&
+	test_when_finished "rm -f out" &&
+	test_i18ngrep "Cannot rebase onto multiple branches" out
+'
+
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&
-- 
2.1.4

[PATCH 6/7] t5520: test --rebase failure on unborn branch with index

[Paul Tan]

Commit 19a7fcbf (allow pull --rebase on branch yet to be born) special
cases git-pull on an unborn branch in a different code path such that
git-pull --rebase is still valid even though there is no HEAD yet.

This code path still ensures that there is no index in order not to lose
any staged changes. Implement a test to ensure that this check is
triggered.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

I wonder if the .git/index check is still required, as git-read-tree -m
will take into account entries in the index as well.

 t/t5520-pull.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 17c63ff..34f3780 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -417,6 +417,17 @@ test_expect_success 'pull --rebase works on branch yet to be born' '
 	test_cmp expect actual
 '
 
+test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
+	git init empty_repo2 &&
+	test_when_finished "rm -rf empty_repo2" &&
+	(
+		cd empty_repo2 &&
+		echo staged-file >staged-file &&
+		git add staged-file &&
+		test_must_fail git pull --rebase .. master
+	)
+'
+
 test_expect_success 'setup for detecting upstreamed changes' '
 	mkdir src &&
 	(cd src &&
-- 
2.1.4

[PATCH 7/7] t5521: test --dry-run does not make any changes

[Paul Tan]

Test that when --dry-run is provided to git-pull, it does not make any
changes, namely:

* --dry-run gets passed to git-fetch, so no FETCH_HEAD will be created
  and no refs will be fetched.

* The index and work tree will not be modified.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
 t/t5521-pull-options.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 453aba5..7c4d624 100755
--- a/t/t5521-pull-options.sh
+++ b/t/t5521-pull-options.sh
@@ -117,4 +117,17 @@ test_expect_success 'git pull --all' '
 	)
 '
 
+test_expect_success 'git pull --dry-run' '
+	git init clonedry &&
+	test_when_finished "rm -rf clonedry" &&
+	(
+		cd clonedry &&
+		git pull --dry-run "../parent" &&
+		test_path_is_missing .git/FETCH_HEAD &&
+		test_path_is_missing .git/refs/heads/master &&
+		test_path_is_missing .git/index &&
+		test_path_is_missing "file"
+	)
+'
+
 test_done
-- 
2.1.4

Re: [PATCH 4/7] t5520: test work tree fast-forward when fetch updates head

[Eric Sunshine]

On Sat, May 2, 2015 at 11:37 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Since commit b10ac50f (Fix pulling into the same branch), git-pull,
> upon detecting that git-fetch updated the current head, will
> fast-forward the working tree to the updated head commit.
>
> Implement tests to ensure that the fast-forward occurs in such a case,
> as well as to ensure that the user-friendly advice is printed upon
> failure.

Missing sign-off.

> ---
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 9f57e0d..25d519d 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -189,6 +189,27 @@ test_expect_success 'fail if the index has unresolved entries' '
>         test_i18ngrep "You have not concluded your merge" out
>  '
>
> +test_expect_success 'fast-forwards working tree if branch head is updated' '
> +       git checkout -b third master^ &&
> +       test_when_finished "git checkout -f copy && git branch -D third" &&
> +       echo file >expected &&
> +       test_cmp expected file &&
> +       git pull . second:third 2>out &&
> +       test_i18ngrep "fetch updated the current branch head" out &&
> +       echo modified >expected &&
> +       test_cmp expected file
> +'
> +
> +test_expect_success 'fast-forward fails with conflicting work tree' '
> +       git checkout -b third master^ &&
> +       test_when_finished "git checkout -f copy && git branch -D third" &&
> +       echo file >expected &&
> +       test_cmp expected file &&
> +       echo conflict >file &&
> +       test_must_fail git pull . second:third 2>out &&
> +       test_i18ngrep "Cannot fast-forward your working tree" out
> +'
> +
>  test_expect_success '--rebase' '
>         git branch to-rebase &&
>         echo modified again > file &&
> --
> 2.1.4

Re: [PATCH 4/7] t5520: test work tree fast-forward when fetch updates head

[Paul Tan]

On Sun, May 3, 2015 at 10:42 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> Missing sign-off.

Right, whoops.

Signed-off-by: Paul Tan <pyokagan@gmail.com>

Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases

[Matthieu Moy]

Thanks for the patch. On overall, it looks good to me. Some minor
comments below.

Paul Tan <pyokagan@gmail.com> writes:

> Commit a8c9bef4 fully established the current advices given by git-pull
> for the different cases where git-fetch will not have anything marked
> for merge:
>
> 1. We're not on a branch, so there is no branch
>    configuration.

Nit: you seem to be wrapping your lines with a rather short length. I
would prefer reading this as a single line:

1. We're not on a branch, so there is no branch configuration.

(62 columns)

> ---
>
> I'm having trouble hitting the 1st case without resorting to the hack below. A
> detached HEAD will always have no remote configured, and the code flow would
> make it such that case (4) is hit in the detached HEAD case instead of case
> (1).

This should appear in comments in the test 'fail if not on a branch'.
People reading your [branch ""] in the future won't look for
below-triple-dash comments in the mailing-list archives ...

And actually, it would be more user-friendly to trigger this error
message in the normal senario, i.e. check for 1. before 4. in the code.
This was most likely the intension of the programmer who wrote this
error message. You may want to fix this now, or add a
test_expect_failure which will become a test_expect_success when you
replace git-pull.sh with builtin/pull.c.

>  t/t5520-pull.sh | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 01ae1bf..635ec02 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -122,6 +122,58 @@ test_expect_success 'the default remote . should not break explicit pull' '
>  	test `cat file` = modified
>  '
>  
> +test_expect_success 'fail if not on a branch' '
> +	cp .git/config .git/config.bak &&
> +	test_when_finished "cp .git/config.bak .git/config" &&
> +	git remote add test_remote . &&
> +	git checkout HEAD^{} &&
> +	test_when_finished "git checkout -f copy" &&
> +	cat >>.git/config <<-\EOF &&
> +	[branch ""]
> +	remote = test_remote
> +	EOF
> +	test_must_fail git pull test_remote 2>out &&
> +	test_i18ngrep "You are not currently on a branch" out

It may make sense to grep only a subset of the string, to be less
sensitive to rewords of error message. For example, just:

test_i18ngrep "not currently on a branch"

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 3/7] t5520: test for failure if index has unresolved entries

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> Commit d38a30df (Be more user-friendly when refusing to do something
> because of conflict) introduced code paths to git-pull which will error
> out with user-friendly advices if the user is in the middle of a merge
> or has unmerged files.

When referencing a commit like this, you may want to add the author of
the commit in Cc: to make sure he reads. I did read, so it's OK. And
sorry for not testing my code enough ;-).

The patch looks good to me.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 0/7] Improve git-pull test coverage

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> Paul Tan (7):
>   t5520: test pulling multiple branches into an empty repository
>   t5520: implement tests for no merge candidates cases
>   t5520: test for failure if index has unresolved entries
>   t5520: test work tree fast-forward when fetch updates head
>   t5520: test --rebase with multiple branches
>   t5520: test --rebase failure on unborn branch with index
>   t5521: test --dry-run does not make any changes

I did a semi-thourough review of the whole series, it looks good to me.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH 5/7] t5520: test --rebase with multiple branches

[Stefan Beller]

On Sat, May 2, 2015 at 8:37 AM, Paul Tan <pyokagan@gmail.com> wrote:
> Since rebasing on top of multiple upstream branches does not make sense,
> since commit 51b2ead0 ("disallow providing multiple upstream branches
> to rebase, pull --rebase"), git-pull explicitly disallowed specifying
> multiple branches in the rebase case.
>
> Implement tests to ensure that git-pull fails and prints out the
> user-friendly error message in such a case.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>  t/t5520-pull.sh | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 25d519d..17c63ff 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -223,6 +223,14 @@ test_expect_success '--rebase' '
>         test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
>         test new = $(git show HEAD:file2)
>  '
> +
> +test_expect_success '--rebase fails with multiple branches' '
> +       git reset --hard before-rebase &&
> +       test_must_fail git pull --rebase . copy master 2>out &&
> +       test_when_finished "rm -f out" &&

I think it would make sense to switch the previous 2 lines, because the
test_when_finished should also be run if the test actually fails.
The actual tested part may segfault, which would interrupt the && chain
from reaching the test_when_finished line. And here, the line to test
is meant to be the "git pull --rebase" line, so we'd assume it may be
stopping there due to failures in the tested program.

If you grep through the code base, you'll find lots of test_when_finished
commands as the very first command (or the earliest spot where it makes
sense).

> +       test_i18ngrep "Cannot rebase onto multiple branches" out
> +'
> +
>  test_expect_success 'pull.rebase' '
>         git reset --hard before-rebase &&
>         test_config pull.rebase true &&
> --
> 2.1.4
>

Re: [PATCH 0/7] Improve git-pull test coverage

[Junio C Hamano]

Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
>> Paul Tan (7):
>>   t5520: test pulling multiple branches into an empty repository
>>   t5520: implement tests for no merge candidates cases
>>   t5520: test for failure if index has unresolved entries
>>   t5520: test work tree fast-forward when fetch updates head
>>   t5520: test --rebase with multiple branches
>>   t5520: test --rebase failure on unborn branch with index
>>   t5521: test --dry-run does not make any changes
>
> I did a semi-thourough review of the whole series, it looks good to me.

Thanks. I did the same, and it looked OK.

One test was duplicated with jc/merge series and caused a textual
conflict, but that was nothing major.

I didn't like the grepping of error messages alone as a way to
diagnose the failure, though.  When we expect the pull to fail
without touching anything in the working tree, I'd prefer to see
that tested explicitly (e.g. "if pull mistakenly tried to go ahead
and touch this file that would be involved in the merge, its
contents would change---let's make sure that does not happen by
making sure the contents before and after are the same" kind of
thing).

Re: [PATCH 5/7] t5520: test --rebase with multiple branches

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>> +test_expect_success '--rebase fails with multiple branches' '
>> +       git reset --hard before-rebase &&
>> +       test_must_fail git pull --rebase . copy master 2>out &&
>> +       test_when_finished "rm -f out" &&
>
> I think it would make sense to switch the previous 2 lines, because the
> test_when_finished should also be run if the test actually fails.

Excellent.  Thanks for carefully reading patches.

Re: [PATCH 0/7] Improve git-pull test coverage

[Paul Tan]

Hi Junio,

On Tue, May 5, 2015 at 1:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I didn't like the grepping of error messages alone as a way to
> diagnose the failure, though.  When we expect the pull to fail
> without touching anything in the working tree, I'd prefer to see
> that tested explicitly (e.g. "if pull mistakenly tried to go ahead
> and touch this file that would be involved in the merge, its
> contents would change---let's make sure that does not happen by
> making sure the contents before and after are the same" kind of
> thing).

Yes, you're right. Some tests do need these additional checks. Will
add them in the re-roll.

Just to be sure, aside from checking that git-pull does not touch
files it should not touch in the event of an error, I do believe we
also need to check its error message, as printing out the correct
user-friendly error messages is one of the features of git-pull.

Regards,
Paul

Re: [PATCH 5/7] t5520: test --rebase with multiple branches

[Paul Tan]

Hi,

On Tue, May 5, 2015 at 1:09 AM, Stefan Beller <sbeller@google.com> wrote:
> On Sat, May 2, 2015 at 8:37 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index 25d519d..17c63ff 100755
>> --- a/t/t5520-pull.sh
>> +++ b/t/t5520-pull.sh
>> @@ -223,6 +223,14 @@ test_expect_success '--rebase' '
>>         test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
>>         test new = $(git show HEAD:file2)
>>  '
>> +
>> +test_expect_success '--rebase fails with multiple branches' '
>> +       git reset --hard before-rebase &&
>> +       test_must_fail git pull --rebase . copy master 2>out &&
>> +       test_when_finished "rm -f out" &&
>
> I think it would make sense to switch the previous 2 lines, because the
> test_when_finished should also be run if the test actually fails.
> The actual tested part may segfault, which would interrupt the && chain
> from reaching the test_when_finished line. And here, the line to test
> is meant to be the "git pull --rebase" line, so we'd assume it may be
> stopping there due to failures in the tested program.
>
> If you grep through the code base, you'll find lots of test_when_finished
> commands as the very first command (or the earliest spot where it makes
> sense).

Hmm, thinking about it again, I actually don't really think there's
that much of a need to "rm -f out" in this case ;-)

But yes, I should go review the placements of the test_when_finished calls :-).

Thanks.

Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases

[Paul Tan]

Hi,

On Mon, May 4, 2015 at 4:04 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>> I'm having trouble hitting the 1st case without resorting to the hack below. A
>> detached HEAD will always have no remote configured, and the code flow would
>> make it such that case (4) is hit in the detached HEAD case instead of case
>> (1).
>
> This should appear in comments in the test 'fail if not on a branch'.
> People reading your [branch ""] in the future won't look for
> below-triple-dash comments in the mailing-list archives ...
>
> And actually, it would be more user-friendly to trigger this error
> message in the normal senario, i.e. check for 1. before 4. in the code.
> This was most likely the intension of the programmer who wrote this
> error message. You may want to fix this now, or add a
> test_expect_failure which will become a test_expect_success when you
> replace git-pull.sh with builtin/pull.c.

Ah, I just figured out how to trigger this error. Just for reference,
before I forget:

1. HEAD must be detached (such that there is no configured remote)

2. The default remote "origin" must exist, so that git-fetch will
succeed even though there is no configured remote for the detached
HEAD.

3. git pull must be called with no arguments, to avoid (4).

I guess everything is working as intended. I copied the different
cases from the commit message of a8c9bef4, but I think that they
should be re-ordered to match the logic in the code. Furthermore,
cases (1) and (2) could probably be explained as:

(1) git-fetch succeeded in fetching from the branch's or repo's
default remote, but:

(1a) We are not on a branch, so there will never be a configured
upstream branch to merge with.

(1b) We are on a branch, but there is no configured upstream branch to
merge with.

Re: [PATCH 2/7] t5520: implement tests for no merge candidates cases

[Paul Tan]

On Wed, May 6, 2015 at 2:04 PM, Paul Tan <pyokagan@gmail.com> wrote:
> Ah, I just figured out how to trigger this error. Just for reference,
> before I forget:

Whoops, hit send too early.

The test for case (1) would look something like:

test_expect_success 'fail if not on a branch' '
    git remote add origin . &&
    test_when_finished "git remote rm origin" &&
    git checkout HEAD^{} &&
    test_when_finished "git checkout -f copy" &&
    test_must_fail git pull 2>out &&
    test_i18ngrep "You are not currently on a branch" out
'

Re: [PATCH 0/7] Improve git-pull test coverage

[Paul Tan]

Hi Junio,

On Tue, May 5, 2015 at 1:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> One test was duplicated with jc/merge series and caused a textual
> conflict, but that was nothing major.

I will be rebasing the git-pull tests on top of your jc/merge patch
series (as it makes the git-pull rewrite easier), so I will remove
that offending patch.

Thanks,
Paul