[PATCH v2 00/12] Improve git-pull test coverage

This patch series improves test coverage of git-pull.sh.

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.

The previous patch series can be found at [1]. Note that it is now based on
jc/merge in pu.

Besides fixing issues raised in the last round, some failing tests have been
added to demonstrate some bugs in git-pull.sh, and some tests are modified to
reduce their dependence on git-pull's functionality so that irrelevant test
suites will not break during the rewrite.

[1] http://thread.gmane.org/gmane.comp.version-control.git/268231

Paul Tan (12):
  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
  t4013: call git-merge instead of git-pull
  t5520: ensure origin refs are updated
  t7406: use "git pull" instead of "git pull --rebase"
  t5520: failing test for pull --all with no configured upstream
  t5524: test --log=1 limits shortlog length
  t5520: check reflog action in fast-forward merge

 t/t4013-diff-various.sh     |   2 +-
 t/t5520-pull.sh             | 148 +++++++++++++++++++++++++++++++++++++++++++-
 t/t5521-pull-options.sh     |  13 ++++
 t/t5524-pull-msg.sh         |  17 +++++
 t/t7406-submodule-update.sh |   2 +-
 5 files changed, 177 insertions(+), 5 deletions(-)

-- 
2.1.4

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 <redacted>
---

Notes:
    * Check that when git-pull fails, HEAD was not moved.
    
    * Removed the not-really-required `test_when_finished "rm -f out"`.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 99b6f67..05a92a2 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -220,6 +220,15 @@ 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 $(git rev-parse HEAD) = $(git rev-parse before-rebase) &&
+	test_i18ngrep "Cannot rebase onto multiple branches" out &&
+	test modified = "$(git show HEAD:file)"
+'
+
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&

-- 
2.1.4

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 <redacted>
---

Notes:
    * Test that on merge conflict, git-pull will not reset conflict status,
      or modify the conflicted file.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 5add900..37ff45f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -164,6 +164,27 @@ test_expect_success 'fail if upstream branch does not exist' '
 	test `cat file` = file
 '
 
+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 -z "$(git ls-files -u)" &&
+	test_must_fail git pull . second &&
+	test -n "$(git ls-files -u)" &&
+	cp file expected &&
+	test_must_fail git pull . second 2>out &&
+	test_i18ngrep "Pull is not possible because you have unmerged files" out &&
+	test_cmp expected file &&
+	git add file &&
+	test -z "$(git ls-files -u)" &&
+	test_must_fail git pull . second 2>out &&
+	test_i18ngrep "You have not concluded your merge" out &&
+	test_cmp expected file
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&

-- 
2.1.4

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 fetched from a specific remote, and a refspec was given, but it
   ended up not fetching anything. This is usually because the user
   provided a wildcard refspec which had no matches on the remote end.

2. We fetched from a 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, and thus the user must specify the branches to merge.

3. We fetched from the branch's or repo's default remote, but:

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

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

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

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 <redacted>
---

Notes:
    * Re-worded commit message to match the logic used in git-pull.sh's
      error_on_no_merge_candidates().
    
    * The tests have thus also been reordered to match the commit message.
    
    * Non-hackish solution for case 3a.
    
    * Add more checks to ensure that git-pull does not touch any files it
      should not be touching on failure.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 7efd45b..5add900 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -109,6 +109,61 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	test `cat file` = modified
 '
 
+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 `cat file` = file &&
+	test_must_fail git pull . "refs/nonexisting1/*:refs/nonexisting2/*" 2>out &&
+	test_i18ngrep "no candidates for merging" out &&
+	test `cat file` = file
+'
+
+test_expect_success 'fail if no branches specified with non-default remote' '
+	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 `cat file` = file &&
+	test_config branch.test.remote origin &&
+	test_must_fail git pull test_remote 2>out &&
+	test_i18ngrep "specify a branch on the command line" out &&
+	test `cat file` = file
+'
+
+test_expect_success 'fail if not on a branch' '
+	git remote add origin . &&
+	test_when_finished "git remote remove origin" &&
+	git checkout HEAD^ &&
+	test_when_finished "git checkout -f copy" &&
+	test `cat file` = file &&
+	test_must_fail git pull 2>out &&
+	test_i18ngrep "not currently on a branch" out &&
+	test `cat file` = file
+'
+
+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 `cat file` = file &&
+	test_must_fail git pull 2>out &&
+	test_i18ngrep "no tracking information" out &&
+	test `cat file` = file
+'
+
+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 `cat file` = file &&
+	test_must_fail git pull 2>out &&
+	test_i18ngrep "no such ref was fetched" out &&
+	test `cat file` = file
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&

-- 
2.1.4

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.

Signed-off-by: Paul Tan <redacted>
---

Notes:
    * Ensure that on fast-forward failure, if there is a conflict, the work
      tree should not be touched.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 37ff45f..99b6f67 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -185,6 +185,28 @@ test_expect_success 'fail if the index has unresolved entries' '
 	test_cmp expected file
 '
 
+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 `cat file` = conflict
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&

-- 
2.1.4

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 <redacted>
---

Notes:
    * Moved test_when_finished to beginning of test

 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..21b1dbe 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' '
+	test_when_finished "rm -rf clonedry" &&
+	git init 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

Should all of the tests before "setup for avoiding reapplying old
patches" fail or be skipped, the repo "dst" will not have fetched the
updated refs from origin. To be resilient against such failures, run
"git fetch origin".

Signed-off-by: Paul Tan <redacted>
---

Notes:
    * This is a new patch in the patch series.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 9107991..d97a575 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -461,6 +461,7 @@ test_expect_success 'git pull --rebase detects upstreamed changes' '
 test_expect_success 'setup for avoiding reapplying old patches' '
 	(cd dst &&
 	 test_might_fail git rebase --abort &&
+	 git fetch origin &&
 	 git reset --hard origin/master
 	) &&
 	git clone --bare src src-replace.git &&

-- 
2.1.4

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 <redacted>
---

Notes:
    * Test for error message.
    
    * Ensure that when git-pull does not modify the index.
    
    * Moved test_when_finished

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 05a92a2..9107991 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -415,6 +415,21 @@ 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' '
+	test_when_finished "rm -rf empty_repo2" &&
+	git init empty_repo2 &&
+	(
+		cd empty_repo2 &&
+		echo staged-file >staged-file &&
+		git add staged-file &&
+		test "$(git ls-files)" = staged-file &&
+		test_must_fail git pull --rebase .. master 2>../out &&
+		test "$(git ls-files)" = staged-file &&
+		test "$(git show :staged-file)" = staged-file
+	) &&
+	test_i18ngrep "unborn branch with changes added to the index" out
+'
+
 test_expect_success 'setup for detecting upstreamed changes' '
 	mkdir src &&
 	(cd src &&

-- 
2.1.4

Since "git fetch ." does not update any refs, "git pull . side" is
equivalent to calling "git merge side".

As such, replace the call to git-pull with a call to git-merge to reduce
the dependence on git-pull's functionality to reduce irrelevant test
breakage when git-pull is rewritten to C.

Signed-off-by: Paul Tan <redacted>
---

Notes:
    * This is a new patch in the patch series.

 t/t4013-diff-various.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 6ec6072..48f2fe2 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh

@@ -64,7 +64,7 @@ test_expect_success setup '
 	export GIT_AUTHOR_DATE GIT_COMMITTER_DATE &&
 
 	git checkout master &&
-	git pull -s ours . side &&
+	git merge -s ours side &&
 
 	GIT_AUTHOR_DATE="2006-06-26 00:05:00 +0000" &&
 	GIT_COMMITTER_DATE="2006-06-26 00:05:00 +0000" &&

-- 
2.1.4

At this point, the HEAD of super/submodule/ is a direct descendent of
submodule/ and thus a fast-forward merge can occur. There is no need to
rebase.

Call "git pull" instead of "git pull --rebase" in order to reduce
dependence on git-pull's functionality, and thus prevent the whole test suite
from failing when git-pull is rewritten to C.

Signed-off-by: Paul Tan <redacted>
---

Notes:
    * This is a new patch in the patch series.

 t/t7406-submodule-update.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..e38d830 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh

@@ -44,7 +44,7 @@ test_expect_success 'setup a submodule tree' '
 	) &&
 	(cd super &&
 	 (cd submodule &&
-	  git pull --rebase origin
+	  git pull origin
 	 ) &&
 	 git add submodule &&
 	 git commit -m "submodule update"

-- 
2.1.4

error_on_no_merge_candidates() does not consider the case where "$#"
includes command-line flags that are passed to git-fetch.

As such, when the current branch has no configured upstream, and there
are no merge candidates because of that, git-pull --all erroneously reports
that we are pulling from "--all", as it believes that the first argument
is the remote name.

Add a failing test that shows this case.

Reported-by: Stephen Robin <redacted>
Signed-off-by: Paul Tan <redacted>
---

Notes:
    * Added this test to the patch series.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index d97a575..b93b735 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -153,6 +153,18 @@ test_expect_success 'fail if no configuration for current branch' '
 	test `cat file` = file
 '
 
+test_expect_failure 'pull --all: 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 `cat file` = file &&
+	test_must_fail git pull --all 2>out &&
+	test_i18ngrep "There is no tracking information" out &&
+	test `cat file` = file
+'
+
 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" &&

-- 
2.1.4

When testing a fast-forward merge with git-pull, check to see if the
reflog action is "pull" with the arguments passed to git-pull.

While we are in the vicinity, remove the empty line as well.

Signed-off-by: Paul Tan <redacted>
---

Notes:
    * Added this test to the patch series.

 t/t5520-pull.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index b93b735..6045491 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -86,7 +86,6 @@ test_expect_success 'pulling into void must not create an octopus' '
 '
 
 test_expect_success 'test . as a remote' '
-
 	git branch copy master &&
 	git config branch.copy.remote . &&
 	git config branch.copy.merge refs/heads/master &&

@@ -95,7 +94,11 @@ test_expect_success 'test . as a remote' '
 	git checkout copy &&
 	test `cat file` = file &&
 	git pull &&
-	test `cat file` = updated
+	test `cat file` = updated &&
+	git reflog -1 >reflog.actual &&
+	sed "s/$_x05[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy &&
+	echo "OBJID HEAD@{0}: pull: Fast-forward" >reflog.expected &&
+	test_cmp reflog.expected reflog.fuzzy
 '
 
 test_expect_success 'the default remote . should not break explicit pull' '

@@ -106,7 +109,11 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	git reset --hard HEAD^ &&
 	test `cat file` = file &&
 	git pull . second &&
-	test `cat file` = modified
+	test `cat file` = modified &&
+	git reflog -1 >reflog.actual &&
+	sed "s/$_x05[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy &&
+	echo "OBJID HEAD@{0}: pull . second: Fast-forward" >reflog.expected &&
+	test_cmp reflog.expected reflog.fuzzy
 '
 
 test_expect_success 'fail if wildcard spec does not match any refs' '

-- 
2.1.4

While git-pull supports --log and passes the switch to git-merge, it
does not support --log=<n>, ignoring the value <n>.

This is not only at odds with the documentation of git-pull, it's also a
undesirable limitation as <n> could simply be passed to git-merge as
well.

Implement a failing test that demonstrates this.

Signed-off-by: Paul Tan <redacted>
---

Notes:
    * Added this test to the patch series

 t/t5524-pull-msg.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/t/t5524-pull-msg.sh b/t/t5524-pull-msg.sh
index 8cccecc..5b7af07 100755
--- a/t/t5524-pull-msg.sh
+++ b/t/t5524-pull-msg.sh

@@ -17,6 +17,9 @@ test_expect_success setup '
 		git commit -m "add bfile"
 	) &&
 	test_tick && test_tick &&
+	echo "second" >afile &&
+	git add afile &&
+	git commit -m "second commit" &&
 	echo "original $dollar" >afile &&
 	git add afile &&
 	git commit -m "do not clobber $dollar signs"

@@ -32,4 +35,18 @@ test_expect_success pull '
 )
 '
 
+test_expect_failure '--log=1 limits shortlog length' '
+(
+	cd cloned &&
+	git reset --hard HEAD^ &&
+	test `cat afile` = original &&
+	test `cat bfile` = added &&
+	git pull --log &&
+	git log -3 &&
+	git cat-file commit HEAD >result &&
+	grep Dollar result &&
+	! grep "second commit" result
+)
+'
+
 test_done

-- 
2.1.4

On 05/07/2015 10:43 AM, Paul Tan wrote:

Minor nit, please see Documentation/CodingGuidelines:
 - We prefer $( ... ) for command substitution; unlike ``, it
   properly nests.  It should have been the way Bourne spelled
   it from day one, but unfortunately isn't.
In other words:
test $(cat file) = file &&

On Thu, May 7, 2015 at 1:43 AM, Paul Tan [off-list ref] wrote:

same comments as in patch 1 apply here.

+'
+
 test_expect_success '--rebase' '
        git branch to-rebase &&
        echo modified again > file &&
--
2.1.4

Hi Paul,

sorry for being so slow on reviewing your patches, but I saw that plenty of excellent feedback has made the second round of patches even better than the first one.

I am particularly impressed by the thoroughness of the commit messages, they make reviewing much more pleasant.

On 2015-05-07 10:44, Paul Tan wrote:

As such, replace the call to git-pull with a call to git-merge to reduce
the dependence on git-pull's functionality to reduce irrelevant test
breakage when git-pull is rewritten to C.

Both this patch and 9/12 change `git pull` invocations to equivalent non-pull ones, but I wonder whether it would not be a better idea to leave them as-are, so that we can make sure that scripts out there that might use similar `git pull` invocations would be unaffected by the rewrite?

Ciao,
Dscho

Hi Paul,

On 2015-05-07 10:44, Paul Tan wrote:

I think it might be better to use `test_must_fail` here, just for consistency (the `!` operator would also pass if `grep` itself could not be executed correctly, quite academic, I know, given that `grep` is exercised plenty of times by the test suite, but still...)

What do you think?

Ciao,
Dscho

P.S.: I missed 12/12 but the rest of the patches looked fine to these old eyes. Thanks!

On Thu, May 7, 2015 at 1:44 AM, Paul Tan [off-list ref] wrote:

I think usually people use

    git ls-files >actual
    echo staged-file >expected && # you have this already in your 2nd
    # line in the paragraph
    test_cmp staged-file actual

to make debugging easier as you can inspect the files (actual, expected)
after the test has failed.

Personally I don't mind the difference as when it comes to debugging
using the test suite I haven't found the silver bullet yet.

+               test_must_fail git pull --rebase .. master 2>../out &&
+               test "$(git ls-files)" = staged-file &&
+               test "$(git show :staged-file)" = staged-file
+       ) &&
+       test_i18ngrep "unborn branch with changes added to the index" out
+'
+
 test_expect_success 'setup for detecting upstreamed changes' '
        mkdir src &&
        (cd src &&
--
2.1.4

On Thu, May 7, 2015 at 1:44 AM, Paul Tan [off-list ref] wrote:

same as in patch 1

The series looks good to me apart from the minor nits.

Thanks,
Stefan

Hi Dscho,

On Fri, May 8, 2015 at 12:26 AM, Johannes Schindelin
[off-list ref] wrote:

Both this patch and 9/12 change `git pull` invocations to equivalent non-pull ones, but I wonder whether it would not be a better idea to leave them as-are, so that we can make sure that scripts out there that might use similar `git pull` invocations would be unaffected by the rewrite?

In the current state[1], I'm aiming for the git-pull rewrite patch
series to break all git-pull tests in the first patch, and then
subsequently make them pass again in later smaller patches by
implementing back the old features. This will make reviewing the code
much easier, as opposed to dumping a huge patch every single re-roll
;-).

For both patches and test suites, if the "setup" tests fail, the whole
test suite fails. Given that the test suites are about testing the
diff formatting options and submodules update implementation, which is
mostly irrelevant to git-pull, I think it would be better if the test
suite was not affected by the rewrite, especially since it only
requires changing one line.

[1] https://github.com/pyokagan/git/commit/bfdf5039d1627c9599051faf2ce34b007d4bfbea

Thanks,
Paul

Hi Dscho,

On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
[off-list ref] wrote:

Hi Paul,

On 2015-05-07 10:44, Paul Tan wrote:

quoted

@@ -32,4 +35,18 @@ test_expect_success pull '
 )
 '

+test_expect_failure '--log=1 limits shortlog length' '
+(
+     cd cloned &&
+     git reset --hard HEAD^ &&
+     test `cat afile` = original &&
+     test `cat bfile` = added &&
+     git pull --log &&
+     git log -3 &&
+     git cat-file commit HEAD >result &&
+     grep Dollar result &&
+     ! grep "second commit" result
+)

I think it might be better to use `test_must_fail` here, just for consistency (the `!` operator would also pass if `grep` itself could not be executed correctly, quite academic, I know, given that `grep` is exercised plenty of times by the test suite, but still...)

What do you think?

Yep, it's definitely better. Sometimes I forget about the existence of
some test utility functions :-/.

Thanks,
Paul

Hi,

On Fri, May 8, 2015 at 12:32 AM, Stefan Beller [off-list ref] wrote:

On Thu, May 7, 2015 at 1:44 AM, Paul Tan [off-list ref] wrote:

quoted

+test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
+       test_when_finished "rm -rf empty_repo2" &&
+       git init empty_repo2 &&
+       (
+               cd empty_repo2 &&
+               echo staged-file >staged-file &&
+               git add staged-file &&
+               test "$(git ls-files)" = staged-file &&

I think usually people use

    git ls-files >actual
    echo staged-file >expected && # you have this already in your 2nd
    # line in the paragraph
    test_cmp staged-file actual

to make debugging easier as you can inspect the files (actual, expected)
after the test has failed.

Personally I don't mind the difference as when it comes to debugging
using the test suite I haven't found the silver bullet yet.

Ehh, but using test_cmp will litter the test with lots of "echo X

expected" lines which I find quite distracting.

Just thinking aloud, but it would be great if there was a function to
compare a string and a file, or a string and a string.

But yeah, I guess if the patches are verified to be correct, then I
should change these comparisons to use test_cmp.

Thanks,
Paul

On Thu, May 7, 2015 at 1:44 PM, Paul Tan [off-list ref] wrote:

On Fri, May 8, 2015 at 12:32 AM, Stefan Beller [off-list ref] wrote:

quoted

On Thu, May 7, 2015 at 1:44 AM, Paul Tan [off-list ref] wrote:

quoted

+test_expect_success 'pull --rebase fails on unborn branch with staged changes' '
+       test_when_finished "rm -rf empty_repo2" &&
+       git init empty_repo2 &&
+       (
+               cd empty_repo2 &&
+               echo staged-file >staged-file &&
+               git add staged-file &&
+               test "$(git ls-files)" = staged-file &&

I think usually people use

    git ls-files >actual
    echo staged-file >expected && # you have this already in your 2nd
    # line in the paragraph
    test_cmp staged-file actual

to make debugging easier as you can inspect the files (actual, expected)
after the test has failed.

Personally I don't mind the difference as when it comes to debugging
using the test suite I haven't found the silver bullet yet.

Ehh, but using test_cmp will litter the test with lots of "echo X

quoted

expected" lines which I find quite distracting.

Just thinking aloud, but it would be great if there was a function to
compare a string and a file, or a string and a string.

But yeah, I guess if the patches are verified to be correct, then I
should change these comparisons to use test_cmp.

Check out verbose() in test-lib-functions.sh:643. It might be just
what you want. t0020-crlf.sh has a bunch of examples of its use.

A couple very minor comments applying to the entire patch series...

On Thu, May 7, 2015 at 4:43 AM, Paul Tan [off-list ref] wrote:

Commit d38a30df (Be more user-friendly when refusing to do something
because of conflict) introduced code paths to git-pull which will error

Custom for citing a commit is also to include the date:

    d38a30df (Be more user-friendly...conflict, 2010-01-12)

Some people use this git alias to help automate:

    whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short

Perhaps call this stderr capture file 'err' rather than 'out' to
clarify its nature and to distinguish it from a stdout capture which
someone might add in the future?

+       test_i18ngrep "Pull is not possible because you have unmerged files" out &&
+       test_cmp expected file &&
+       git add file &&
+       test -z "$(git ls-files -u)" &&
+       test_must_fail git pull . second 2>out &&
+       test_i18ngrep "You have not concluded your merge" out &&
+       test_cmp expected file
+'
+
 test_expect_success '--rebase' '
        git branch to-rebase &&
        echo modified again > file &&
--
2.1.4

Am 07.05.2015 um 19:06 schrieb Paul Tan:

Hi Dscho,

On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

Hi Paul,

On 2015-05-07 10:44, Paul Tan wrote:

quoted

@@ -32,4 +35,18 @@ test_expect_success pull '
  )
  '

+test_expect_failure '--log=1 limits shortlog length' '
+(
+     cd cloned &&
+     git reset --hard HEAD^ &&
+     test `cat afile` = original &&
+     test `cat bfile` = added &&
+     git pull --log &&
+     git log -3 &&
+     git cat-file commit HEAD >result &&
+     grep Dollar result &&
+     ! grep "second commit" result
+)

I think it might be better to use `test_must_fail` here, just for
consistency (the `!` operator would also pass if `grep` itself could not
be executed correctly, quite academic, I know, given that `grep` is
exercised plenty of times by the test suite, but still...)

What do you think?

Yep, it's definitely better. Sometimes I forget about the existence of
some test utility functions :-/.

Nope, it's not better. test_must_fail is explicitly only for git invocations. We do not expect 'grep' to segfault or something.

Cf. eg. http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752

-- Hannes

Hi Hannes,

On 2015-05-07 21:12, Johannes Sixt wrote:

Am 07.05.2015 um 19:06 schrieb Paul Tan:

quoted

On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

On 2015-05-07 10:44, Paul Tan wrote:

quoted

@@ -32,4 +35,18 @@ test_expect_success pull '
  )
  '

+test_expect_failure '--log=1 limits shortlog length' '
+(
+     cd cloned &&
+     git reset --hard HEAD^ &&
+     test `cat afile` = original &&
+     test `cat bfile` = added &&
+     git pull --log &&
+     git log -3 &&
+     git cat-file commit HEAD >result &&
+     grep Dollar result &&
+     ! grep "second commit" result
+)

I think it might be better to use `test_must_fail` here, just for
consistency (the `!` operator would also pass if `grep` itself could not
be executed correctly, quite academic, I know, given that `grep` is
exercised plenty of times by the test suite, but still...)

What do you think?

Yep, it's definitely better. Sometimes I forget about the existence of
some test utility functions :-/.

Nope, it's not better. test_must_fail is explicitly only for git
invocations. We do not expect 'grep' to segfault or something.

Cf. eg.
http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752

That link leads to a patch that changes `! grep` to a `test_must_fail grep` and is not contested, at least not in the thread visible on GMane. Would you have a link with a more convincing argument for me?

Thank you,
Johannes

Johannes Schindelin [off-list ref] writes:

Hi Hannes,

On 2015-05-07 21:12, Johannes Sixt wrote:

quoted

Am 07.05.2015 um 19:06 schrieb Paul Tan:

quoted

On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

On 2015-05-07 10:44, Paul Tan wrote:

quoted

@@ -32,4 +35,18 @@ test_expect_success pull '
  )
  '

+test_expect_failure '--log=1 limits shortlog length' '
+(
+     cd cloned &&
+     git reset --hard HEAD^ &&
+     test `cat afile` = original &&
+     test `cat bfile` = added &&
+     git pull --log &&
+     git log -3 &&
+     git cat-file commit HEAD >result &&
+     grep Dollar result &&
+     ! grep "second commit" result
+)

I think it might be better to use `test_must_fail` here, just for
consistency (the `!` operator would also pass if `grep` itself could not
be executed correctly, quite academic, I know, given that `grep` is
exercised plenty of times by the test suite, but still...)

What do you think?

Yep, it's definitely better. Sometimes I forget about the existence of
some test utility functions :-/.

Nope, it's not better. test_must_fail is explicitly only for git
invocations. We do not expect 'grep' to segfault or something.

Cf. eg.
http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752

That link leads to a patch that changes `! grep` to a `test_must_fail grep` and is not contested, at least not in the thread visible on GMane. Would you have a link with a more convincing argument for me?

t/README states:

   On the other hand, don't use test_must_fail for running regular
   platform commands; just use '! cmd'.  We are not in the business
   of verifying that the world given to us sanely works.

Except for a few cases that is also respected in the test scripts.

$ git grep "! grep" | wc -l
203
$ git grep "test_must_fail grep" | wc -l
19

So I think using ! grep is the right way to go.

Thank you,
Johannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Hi Thomas,

On 2015-05-08 12:59, Thomas Gummerer wrote:
 

t/README states:

   On the other hand, don't use test_must_fail for running regular
   platform commands; just use '! cmd'.  We are not in the business
   of verifying that the world given to us sanely works.

Okay, thanks for the reality check.

Ciao,
Dscho

Hi Paul,

On 2015-05-07 18:55, Paul Tan wrote:

Hi Dscho,

On Fri, May 8, 2015 at 12:26 AM, Johannes Schindelin
[off-list ref] wrote:

quoted

Both this patch and 9/12 change `git pull` invocations to equivalent non-pull ones, but I wonder whether it would not be a better idea to leave them as-are, so that we can make sure that scripts out there that might use similar `git pull` invocations would be unaffected by the rewrite?

In the current state[1], I'm aiming for the git-pull rewrite patch
series to break all git-pull tests in the first patch, and then
subsequently make them pass again in later smaller patches by
implementing back the old features. This will make reviewing the code
much easier, as opposed to dumping a huge patch every single re-roll
;-).

For both patches and test suites, if the "setup" tests fail, the whole
test suite fails. Given that the test suites are about testing the
diff formatting options and submodules update implementation, which is
mostly irrelevant to git-pull, I think it would be better if the test
suite was not affected by the rewrite, especially since it only
requires changing one line.

Ah, that makes sense. I just failed to understand what you were telling me.

Maybe it would be better to make that patch a part of the builtin pull instead, so that we do not forget to revert it with the commit that introduces support for `-s ours` in builtin pull?

Ciao,
Dscho

Am 08.05.2015 um 12:07 schrieb Johannes Schindelin:

On 2015-05-07 21:12, Johannes Sixt wrote:

quoted

Nope, it's not better. test_must_fail is explicitly only for git
invocations. We do not expect 'grep' to segfault or something.

Cf. eg.
http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258752

That link leads to a patch that changes `! grep` to a `test_must_fail
grep` and is not contested, at least not in the thread visible on
GMane. Would you have a link with a more convincing argument for me?

Gah! Sorry for sending you in circles. I see that others have brought forward sufficient arguments. Just to get my own argument straight, here is the message I wanted to direct you to:

http://thread.gmane.org/gmane.comp.version-control.git/258725/focus=258792

-- Hannes

Hi Eric,

On Fri, May 8, 2015 at 2:28 AM, Eric Sunshine [off-list ref] wrote:

A couple very minor comments applying to the entire patch series...

On Thu, May 7, 2015 at 4:43 AM, Paul Tan [off-list ref] wrote:

quoted

Commit d38a30df (Be more user-friendly when refusing to do something
because of conflict) introduced code paths to git-pull which will error

Custom for citing a commit is also to include the date:

    d38a30df (Be more user-friendly...conflict, 2010-01-12)

Some people use this git alias to help automate:

    whatis = show -s --pretty='tformat:%h (%s, %ad)' --date=short

This is really useful, thanks!

quoted

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 5add900..37ff45f 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh

@@ -164,6 +164,27 @@ test_expect_success 'fail if upstream branch does not exist' '
        test `cat file` = file
 '

+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 -z "$(git ls-files -u)" &&
+       test_must_fail git pull . second &&
+       test -n "$(git ls-files -u)" &&
+       cp file expected &&
+       test_must_fail git pull . second 2>out &&

Perhaps call this stderr capture file 'err' rather than 'out' to
clarify its nature and to distinguish it from a stdout capture which
someone might add in the future?

Will fix.

Regards,
Paul