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

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

[Paul Tan]

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

[PATCH v2 01/12] 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 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 <pyokagan@gmail.com>
---

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

[PATCH v2 02/12] 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>
---

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

[PATCH v2 03/12] 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.

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

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

[PATCH v2 04/12] 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>
---

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

[PATCH v2 05/12] 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>
---

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

[PATCH v2 06/12] 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>
---

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

[PATCH v2 07/12] t4013: call git-merge instead of git-pull

[Paul Tan]

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 <pyokagan@gmail.com>
---

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

[PATCH v2 08/12] t5520: ensure origin refs are updated

[Paul Tan]

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 <pyokagan@gmail.com>
---

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

[PATCH v2 09/12] t7406: use "git pull" instead of "git pull --rebase"

[Paul Tan]

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 <pyokagan@gmail.com>
---

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

[PATCH v2 10/12] t5520: failing test for pull --all with no configured upstream

[Paul Tan]

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 <stephen.robin@gmail.com>
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---

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

[PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Paul Tan]

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 <pyokagan@gmail.com>
---

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

[PATCH v2 12/12] t5520: check reflog action in fast-forward merge

[Paul Tan]

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 <pyokagan@gmail.com>
---

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

Re: [PATCH v2 01/12] t5520: implement tests for no merge candidates cases

[Torsten Bögershausen]

On 05/07/2015 10:43 AM, Paul Tan wrote:
> 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 <pyokagan@gmail.com>
> ---
>
> 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 &&
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 &&

Re: [PATCH v2 01/12] t5520: implement tests for no merge candidates cases

[Junio C Hamano]

Torsten Bögershausen <tboegi@web.de> writes:

> In other words:
> test $(cat file) = file &&

Is there a guarantee that file has a single word?  Can it be empty?
Can it contain "foo bar\n"?

Re: [PATCH v2 01/12] t5520: implement tests for no merge candidates cases

[Paul Tan]

Hi Junio,

On Thu, May 7, 2015 at 10:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> In other words:
>> test $(cat file) = file &&
>
> Is there a guarantee that file has a single word?  Can it be empty?
> Can it contain "foo bar\n"?

It can, but it should not ;-). But yes, this will need to be quoted as
well to be safe. Whoops.

Thanks,
Paul

Re: [PATCH v2 01/12] t5520: implement tests for no merge candidates cases

[Torsten Bögershausen]

On 07/05/15 16:04, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
>> In other words:
>> test $(cat file) = file &&
> Is there a guarantee that file has a single word?  Can it be empty?
> Can it contain "foo bar\n"?
So true, will this work ?

test "$(cat file)" = file &&

Re: [PATCH v2 01/12] t5520: implement tests for no merge candidates cases

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Hi Junio,
>
> On Thu, May 7, 2015 at 10:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Torsten Bögershausen <tboegi@web.de> writes:
>>
>>> In other words:
>>> test $(cat file) = file &&
>>
>> Is there a guarantee that file has a single word?  Can it be empty?
>> Can it contain "foo bar\n"?
>
> It can, but it should not ;-). But yes, this will need to be quoted as
> well to be safe. Whoops.

Yup.  I see that existing test (this script is ancient, isn't it?)
has the same issue of using backticks and not quoting sufficiently.

Perhaps we would want a clean-up-and-modernise step before this
patch to reduce patch noise.

Thanks.

Re: [PATCH v2 03/12] t5520: test work tree fast-forward when fetch updates head

[Stefan Beller]

On Thu, May 7, 2015 at 1:43 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.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> 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

same comments as in patch 1 apply here.

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

Re: [PATCH v2 07/12] t4013: call git-merge instead of git-pull

[Johannes Schindelin]

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

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Johannes Schindelin]

Hi Paul,

On 2015-05-07 10:44, Paul Tan wrote:
> @@ -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?

Ciao,
Dscho

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

Re: [PATCH v2 05/12] t5520: test --rebase failure on unborn branch with index

[Stefan Beller]

On Thu, May 7, 2015 at 1:44 AM, Paul Tan <pyokagan@gmail.com> wrote:
> 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>
> ---
>
> 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 &&

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
>

Re: [PATCH v2 12/12] t5520: check reflog action in fast-forward merge

[Stefan Beller]

On Thu, May 7, 2015 at 1:44 AM, Paul Tan <pyokagan@gmail.com> wrote:
> 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 <pyokagan@gmail.com>
> ---
>
> 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 &&

same as in patch 1

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


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

Thanks,
Stefan

Re: [PATCH v2 07/12] t4013: call git-merge instead of git-pull

[Paul Tan]

Hi Dscho,

On Fri, May 8, 2015 at 12:26 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> 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

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Paul Tan]

Hi Dscho,

On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> Hi Paul,
>
> On 2015-05-07 10:44, Paul Tan wrote:
>> @@ -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

Re: [PATCH v2 03/12] t5520: test work tree fast-forward when fetch updates head

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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

At this point, HEAD would match either master^ (as initially checked
out) or second (as fetch fast-forwarded), but I cannot read what this
test is expecting to happen.  

Should the HEAD move or stay?

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

Re: [PATCH v2 07/12] t4013: call git-merge instead of git-pull

[Junio C Hamano]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

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

Yes.  I do not mind changing t4013 that is not about testing 'git pull'
to lose 'git pull . other-branch', but then we would want to have a
corresponding test somewhere in the test script that is about
testing 'git pull' to cover that (ancient) pattern of use.

Thanks.

Re: [PATCH v2 09/12] t7406: use "git pull" instead of "git pull --rebase"

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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

Almost the same comment as 07/12 applies here.  "when the pull would
result in fast-forward, 'pull' and 'pull --rebase' results in
exactly the same history" is something we would want to have in the
test suite for 'git pull', and it is perfectly fine to lose the
invocation of 'pull --rebase' from here.

Having said all that.

If 'git pull' gets broken, it will break this test _anyway_.  Unless
the operating assumption is "it is OK to break 'git pull --rebase',
as long as we do not break 'git pull', while rewriting it", I am not
sure the value of the change in this patch.  We'd need to keep both
form working, no?

>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> 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"

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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

A cleaner alternative may be to fix that while git-pull is still a
script, as you seem to already know what is broken and what in the
current code needs to be fixed in what way exactly.  Perhaps do that
at the earlier part of (or even as an independent patch outside)
this series and add this test to protect the fix from getting broken
later (with expect-failure flipped to expect-success)?

Thanks.

>
> Implement a failing test that demonstrates this.



>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> 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

Re: [PATCH v2 03/12] t5520: test work tree fast-forward when fetch updates head

[Paul Tan]

Hi Junio,

On Fri, May 8, 2015 at 1:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> +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
>
> At this point, HEAD would match either master^ (as initially checked
> out) or second (as fetch fast-forwarded), but I cannot read what this
> test is expecting to happen.
>
> Should the HEAD move or stay?

Ugh, it was probably me getting confused by all the test dependencies :-(.

Anyway, second^ == master^ == the initial commit at this point, so it
would be clearer to write "git checkout -b third second^", I think.

The HEAD has already moved because git-fetch updated it. I should
definitely add a check to make that clear.

Whether it should or not is something I have an answer to yet though.

Thanks,
Paul

Re: [PATCH v2 10/12] t5520: failing test for pull --all with no configured upstream

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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

Interesting.

I do not think "pull [origin] --all" makes much sense for the same
reason why we error out when you say "pull [origin] --tag", so I am
not sure "There is no tracking information" is the right diag we
would want to give the user, but I agree that "--all" is not a
remote name.

Does the same comment as 11/12 applies to this as well?

>
> Add a failing test that shows this case.
>
> Reported-by: Stephen Robin <stephen.robin@gmail.com>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
>
> 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" &&

Re: [PATCH v2 05/12] t5520: test --rebase failure on unborn branch with index

[Paul Tan]

Hi,

On Fri, May 8, 2015 at 12:32 AM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, May 7, 2015 at 1:44 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> +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

Re: [PATCH v2 10/12] t5520: failing test for pull --all with no configured upstream

[Paul Tan]

Hi,

On Fri, May 8, 2015 at 1:38 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> 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.
>
> Interesting.
>
> I do not think "pull [origin] --all" makes much sense for the same
> reason why we error out when you say "pull [origin] --tag", so I am
> not sure "There is no tracking information" is the right diag we
> would want to give the user, but I agree that "--all" is not a
> remote name.
>
> Does the same comment as 11/12 applies to this as well?

This is actually only just one symptom of the problem of git-pull
assuming in many places that "$1" is the remote. There are other
possible ones, such as git pull --rebase failing silently at finding
the upstream branch to pass to git-rebase when git-fetch flags are
passed as well (see git-pull.sh:253), though crafting a test case for
that would be a bit more involved. Maybe if I have extra time to look
into it.

Then there is the problem where command line flag parsing stops when
an unidentified flag is encountered, assuming that it is for
git-fetch. E.g. git-pull --all --dry-run will pass --dry-run to
git-fetch, but will still run git-merge. Now, I haven't written this
up as a test case yet because I haven't the time yet, and it could be
argued that this is expected behavior, as the git-pull docs say that
options meant for git-fetch must come after git-pull.

However, this is ultimately really confusing for end-users, so I
propose that we just drop the distinction between git-pull's flags and
git-fetch's flags, and just parse them all at git-pull. This can be
done with the current git-pull.sh, but would be easier with the
superior C parse-options API.

Thanks,
Paul

Re: [PATCH v2 09/12] t7406: use "git pull" instead of "git pull --rebase"

[Paul Tan]

Hi,

On Fri, May 8, 2015 at 1:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
> If 'git pull' gets broken, it will break this test _anyway_.

You have a point, perhaps this should be changed to git-fetch + git-merge?

Well my reasoning is that it's because git-pull --rebase requires more
code to be implemented than git-pull. So I'm thinking that git-pull
--rebase is more likely to break than git-pull.

> Unless
> the operating assumption is "it is OK to break 'git pull --rebase',
> as long as we do not break 'git pull', while rewriting it", I am not
> sure the value of the change in this patch.  We'd need to keep both
> form working, no?

Yes, ultimately the git-pull rewrite must re-implement everything, but
if this test suite is affected by any git-pull (--rebase) breakage,
then there will be lots of patch noise as this test suite's tests get
disabled/re-enabled in the git-pull rewrite patches.

But if the patch noise is okay, then I'm fine with dropping this patch
and 07/12.

Thanks,
Paul

Re: [PATCH v2 05/12] t5520: test --rebase failure on unborn branch with index

[Eric Sunshine]

On Thu, May 7, 2015 at 1:44 PM, Paul Tan <pyokagan@gmail.com> wrote:
> On Fri, May 8, 2015 at 12:32 AM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, May 7, 2015 at 1:44 AM, Paul Tan <pyokagan@gmail.com> wrote:
>>> +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.

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.

Re: [PATCH v2 02/12] t5520: test for failure if index has unresolved entries

[Eric Sunshine]

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

On Thu, May 7, 2015 at 4:43 AM, Paul Tan <pyokagan@gmail.com> 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

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

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

Re: [PATCH v2 10/12] t5520: failing test for pull --all with no configured upstream

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> .... This can be
> done with the current git-pull.sh, but would be easier with the
> superior C parse-options API.

Just this part, but I thought parse-options API is available to
scripts via "rev-parse --parseopt".

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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

Overall, I like what I saw in this series.  Good first progress ;-)

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Johannes Sixt]

Am 07.05.2015 um 19:06 schrieb Paul Tan:
> Hi Dscho,
>
> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> wrote:
>> Hi Paul,
>>
>> On 2015-05-07 10:44, Paul Tan wrote:
>>> @@ -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

Re: [PATCH v2 09/12] t7406: use "git pull" instead of "git pull --rebase"

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Hi,
>
> On Fri, May 8, 2015 at 1:24 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Paul Tan <pyokagan@gmail.com> writes:
>> If 'git pull' gets broken, it will break this test _anyway_.
>
> You have a point, perhaps this should be changed to git-fetch + git-merge?
>
> Well my reasoning is that it's because git-pull --rebase requires more
> code to be implemented than git-pull. So I'm thinking that git-pull
> --rebase is more likely to break than git-pull.
>
>> Unless
>> the operating assumption is "it is OK to break 'git pull --rebase',
>> as long as we do not break 'git pull', while rewriting it", I am not
>> sure the value of the change in this patch.  We'd need to keep both
>> form working, no?
>
> Yes, ultimately the git-pull rewrite must re-implement everything, but
> if this test suite is affected by any git-pull (--rebase) breakage,
> then there will be lots of patch noise as this test suite's tests get
> disabled/re-enabled in the git-pull rewrite patches.
>
> But if the patch noise is okay, then I'm fine with dropping this patch
> and 07/12.

I am not sure what you mean by "patch noise".

I do not think touching this test which does not have anything to do
with "git pull" in your series is sensible at all, and you shouldn't
flip test_expect_success temporarily to _expect_failure, if that is
what you have in mind.

Just don't run unrelated tests while your series is in flux.

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Johannes Schindelin]

Hi Hannes,

On 2015-05-07 21:12, Johannes Sixt wrote:
> Am 07.05.2015 um 19:06 schrieb Paul Tan:
>
>> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
>> <johannes.schindelin@gmx.de> wrote:
>>
>>> On 2015-05-07 10:44, Paul Tan wrote:
>>>> @@ -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

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Thomas Gummerer]

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

> Hi Hannes,
>
> On 2015-05-07 21:12, Johannes Sixt wrote:
>> Am 07.05.2015 um 19:06 schrieb Paul Tan:
>>
>>> On Fri, May 8, 2015 at 12:28 AM, Johannes Schindelin
>>> <johannes.schindelin@gmx.de> wrote:
>>>
>>>> On 2015-05-07 10:44, Paul Tan wrote:
>>>>> @@ -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

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Johannes Schindelin]

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

Re: [PATCH v2 07/12] t4013: call git-merge instead of git-pull

[Johannes Schindelin]

Hi Paul,

On 2015-05-07 18:55, Paul Tan wrote:
> Hi Dscho,
> 
> On Fri, May 8, 2015 at 12:26 AM, Johannes Schindelin
> <johannes.schindelin@gmx.de> 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.

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

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Johannes Sixt]

Am 08.05.2015 um 12:07 schrieb Johannes Schindelin:
> On 2015-05-07 21:12, Johannes Sixt wrote:
>> 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

Re: [PATCH v2 11/12] t5524: test --log=1 limits shortlog length

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> Am 08.05.2015 um 12:07 schrieb Johannes Schindelin:
>> On 2015-05-07 21:12, Johannes Sixt wrote:
>>> 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

FWIW, I think Michael Gruber gave the rationale better than I did
in that thread.

Re: [PATCH v2 10/12] t5520: failing test for pull --all with no configured upstream

[Paul Tan]

Hi Junio,

On Fri, May 8, 2015 at 2:48 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> .... This can be
>> done with the current git-pull.sh, but would be easier with the
>> superior C parse-options API.
>
> Just this part, but I thought parse-options API is available to
> scripts via "rev-parse --parseopt".

Yes, it kind of is, but my impression is that it only validates
command-line arguments -- we still have to loop over each of them
which means more code has to be written as compared to the C version
(especially if we want to validate all the git-fetch switches as
well).

Thanks,
Paul

Re: [PATCH v2 09/12] t7406: use "git pull" instead of "git pull --rebase"

[Paul Tan]

Hi Junio,

On Fri, May 8, 2015 at 4:15 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I do not think touching this test which does not have anything to do
> with "git pull" in your series is sensible at all, and you shouldn't
> flip test_expect_success temporarily to _expect_failure, if that is
> what you have in mind.
>
> Just don't run unrelated tests while your series is in flux.

Yes, you are right, these patches are not related to the topic at all,
and I will drop them.

Just to make myself clear though, the issue I have is that
git-submodule and the various diff commands do not depend on
git-pull's functionality at all. If only git-pull breaks, these
command will continue to work perfectly. However, if all the tests in
t7406 and t4013 break as well, I would consider that misrepresenting
the problem.

Not that this is really a big problem anyway, the entire failing test
suites just surprised me, that's all. Sorry for the noise.

Thanks,
Paul

Re: [PATCH v2 02/12] t5520: test for failure if index has unresolved entries

[Paul Tan]

Hi Eric,

On Fri, May 8, 2015 at 2:28 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> A couple very minor comments applying to the entire patch series...
>
> On Thu, May 7, 2015 at 4:43 AM, Paul Tan <pyokagan@gmail.com> 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

This is really useful, thanks!

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