[PATCH v3 0/9] Improve git-pull test coverage

[PATCH v3 0/9] Improve git-pull test coverage

[Paul Tan]

This is a re-roll of [1]. This series depends on jc/merge.

This patch series improves test coverage of git-pull.sh, and 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.

This re-roll includes the following changes across all patches:

* Added clean-up patch to fix file content comparisons in t5520

* All file content comparisons are quoted to prevent word splitting and use
  verbose() to provide better error messages.

* Style: References to commits are standardized with their summary and date.

* Failing tests have been removed. This series will be solely used to extend
  test coverage of already working functionality. Bugs will be fixed in their
  own patch series'.

* Style: Instead of redirecting stderr to a file named "out", redirect it to a
  file named "err".

Thanks Eric, Torsten, Junio, Stefan, Dscho and Johannes for your reviews last
round.

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

Paul Tan (9):
  t5520: fixup file contents comparisons
  t5520: ensure origin refs are updated
  t5520: test 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
  t5520: check reflog action in fast-forward merge

 t/t5520-pull.sh         | 214 ++++++++++++++++++++++++++++++++++++++----------
 t/t5521-pull-options.sh |  13 +++
 2 files changed, 184 insertions(+), 43 deletions(-)

-- 
2.1.4

[PATCH v3 1/9] t5520: fixup file contents comparisons

[Paul Tan]

Many tests in t5520 used the following to test the contents of files:

	test `cat file` = expected

or

	test $(cat file) = expected

These 2 forms, however, will be affected by field splitting and,
depending on the value of $IFS, may be split into multiple arguments,
making the test fail in mysterious ways.

Replace the above 2 forms with:

	verbose test "$(cat file)" = expected

as quoting the command substitution will prevent field splitting, and
the verbose function will print the failed test command on failure for
easier debugging.

Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
* This is a new patch

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 7efd45b..20ad373 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -81,7 +81,7 @@ test_expect_success 'pulling into void must not create an octopus' '
 	(
 		cd cloned-octopus &&
 		test_must_fail git pull .. master master &&
-		! test -f file
+		verbose test ! -f file
 	)
 '
 
@@ -93,9 +93,9 @@ test_expect_success 'test . as a remote' '
 	echo updated >file &&
 	git commit -a -m updated &&
 	git checkout copy &&
-	test `cat file` = file &&
+	verbose test "$(cat file)" = file &&
 	git pull &&
-	test `cat file` = updated
+	verbose test "$(cat file)" = updated
 '
 
 test_expect_success 'the default remote . should not break explicit pull' '
@@ -104,9 +104,9 @@ test_expect_success 'the default remote . should not break explicit pull' '
 	git commit -a -m modified &&
 	git checkout copy &&
 	git reset --hard HEAD^ &&
-	test `cat file` = file &&
+	verbose test "$(cat file)" = file &&
 	git pull . second &&
-	test `cat file` = modified
+	verbose test "$(cat file)" = modified
 '
 
 test_expect_success '--rebase' '
@@ -119,23 +119,23 @@ test_expect_success '--rebase' '
 	git commit -m "new file" &&
 	git tag before-rebase &&
 	git pull --rebase . copy &&
-	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
-	test new = $(git show HEAD:file2)
+	verbose test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	verbose test new = "$(git show HEAD:file2)"
 '
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
-	test new = $(git show HEAD:file2)
+	verbose test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	verbose test new = "$(git show HEAD:file2)"
 '
 
 test_expect_success 'branch.to-rebase.rebase' '
 	git reset --hard before-rebase &&
 	test_config branch.to-rebase.rebase true &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^) = $(git rev-parse copy) &&
-	test new = $(git show HEAD:file2)
+	verbose test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	verbose test new = "$(git show HEAD:file2)"
 '
 
 test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
@@ -143,8 +143,8 @@ test_expect_success 'branch.to-rebase.rebase should override pull.rebase' '
 	test_config pull.rebase true &&
 	test_config branch.to-rebase.rebase false &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^) != $(git rev-parse copy) &&
-	test new = $(git show HEAD:file2)
+	verbose test "$(git rev-parse HEAD^)" != "$(git rev-parse copy)" &&
+	verbose test new = "$(git show HEAD:file2)"
 '
 
 # add a feature branch, keep-merge, that is merged into master, so the
@@ -163,33 +163,33 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase false &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
-	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
+	verbose test "$(git rev-parse HEAD^1)" = "$(git rev-parse before-preserve-rebase)" &&
+	verbose test "$(git rev-parse HEAD^2)" = "$(git rev-parse copy)" &&
+	verbose test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success 'pull.rebase=true flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
+	verbose test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	verbose test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase 1 &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
+	verbose test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	verbose test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+	verbose test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	verbose test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
 '
 
 test_expect_success 'pull.rebase=invalid fails' '
@@ -202,25 +202,25 @@ test_expect_success '--rebase=false create a new merge commit' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=false . copy &&
-	test $(git rev-parse HEAD^1) = $(git rev-parse before-preserve-rebase) &&
-	test $(git rev-parse HEAD^2) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
+	verbose test "$(git rev-parse HEAD^1)" = "$(git rev-parse before-preserve-rebase)" &&
+	verbose test "$(git rev-parse HEAD^2)" = "$(git rev-parse copy)" &&
+	verbose test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success '--rebase=true rebases and flattens keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase=true . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
+	verbose test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	verbose test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success '--rebase=preserve rebases and merges keep-merge' '
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase true &&
 	git pull --rebase=preserve . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
+	verbose test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	verbose test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
 '
 
 test_expect_success '--rebase=invalid fails' '
@@ -232,8 +232,8 @@ test_expect_success '--rebase overrides pull.rebase=preserve and flattens keep-m
 	git reset --hard before-preserve-rebase &&
 	test_config pull.rebase preserve &&
 	git pull --rebase . copy &&
-	test $(git rev-parse HEAD^^) = $(git rev-parse copy) &&
-	test file3 = $(git show HEAD:file3.t)
+	verbose test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	verbose test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success '--rebase with rebased upstream' '
@@ -249,8 +249,8 @@ test_expect_success '--rebase with rebased upstream' '
 	git commit -m to-rebase file2 &&
 	git tag to-rebase-orig &&
 	git pull --rebase me copy &&
-	test "conflicting modification" = "$(cat file)" &&
-	test file = $(cat file2)
+	verbose test "conflicting modification" = "$(cat file)" &&
+	verbose test file = "$(cat file2)"
 
 '
 
@@ -260,8 +260,8 @@ test_expect_success '--rebase with rebased default upstream' '
 	git checkout --track -b to-rebase2 me/copy &&
 	git reset --hard to-rebase-orig &&
 	git pull --rebase &&
-	test "conflicting modification" = "$(cat file)" &&
-	test file = $(cat file2)
+	verbose test "conflicting modification" = "$(cat file)" &&
+	verbose test file = "$(cat file2)"
 
 '
 
@@ -282,7 +282,7 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
 
 	git checkout to-rebase &&
 	git update-ref refs/remotes/me/copy copy^ &&
-	COPY=$(git rev-parse --verify me/copy) &&
+	COPY="$(git rev-parse --verify me/copy)" &&
 	git rebase --onto $COPY copy &&
 	test_config branch.to-rebase.remote me &&
 	test_config branch.to-rebase.merge refs/heads/copy &&
@@ -290,10 +290,10 @@ test_expect_success 'pull --rebase dies early with dirty working directory' '
 	echo dirty >> file &&
 	git add file &&
 	test_must_fail git pull &&
-	test $COPY = $(git rev-parse --verify me/copy) &&
+	verbose test "$COPY" = "$(git rev-parse --verify me/copy)" &&
 	git checkout HEAD -- file &&
 	git pull &&
-	test $COPY != $(git rev-parse --verify me/copy)
+	verbose test "$COPY" != "$(git rev-parse --verify me/copy)"
 
 '
 
@@ -332,7 +332,7 @@ test_expect_success 'setup for detecting upstreamed changes' '
 test_expect_success 'git pull --rebase detects upstreamed changes' '
 	(cd dst &&
 	 git pull --rebase &&
-	 test -z "$(git ls-files -u)"
+	 verbose test -z "$(git ls-files -u)"
 	)
 '
 
@@ -361,15 +361,15 @@ test_expect_success 'setup for avoiding reapplying old patches' '
 test_expect_success 'git pull --rebase does not reapply old patches' '
 	(cd dst &&
 	 test_must_fail git pull --rebase &&
-	 test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
+	 verbose test 1 = "$(find .git/rebase-apply -name "000*" | wc -l)"
 	)
 '
 
 test_expect_success 'git pull --rebase against local branch' '
 	git checkout -b copy2 to-rebase-orig &&
 	git pull --rebase . to-rebase &&
-	test "conflicting modification" = "$(cat file)" &&
-	test file = "$(cat file2)"
+	verbose test "conflicting modification" = "$(cat file)" &&
+	verbose test file = "$(cat file2)"
 '
 
 test_done
-- 
2.1.4

[PATCH v3 2/9] 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>
---
* Hmm, no reviews the last round?

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 20ad373..14a9280 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -339,6 +339,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 v3 3/9] t5520: test no merge candidates cases

[Paul Tan]

a8c9bef (pull: improve advice for unconfigured error case, 2009-10-05)
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>
---

* File content comparisons are now quoted.


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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 14a9280..e53d8e9 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' '
 	verbose 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" &&
+	verbose test "$(cat file)" = file &&
+	test_must_fail git pull . "refs/nonexisting1/*:refs/nonexisting2/*" 2>err &&
+	test_i18ngrep "no candidates for merging" err &&
+	verbose 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" &&
+	verbose test "$(cat file)" = file &&
+	test_config branch.test.remote origin &&
+	test_must_fail git pull test_remote 2>err &&
+	test_i18ngrep "specify a branch on the command line" err &&
+	verbose 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" &&
+	verbose test "$(cat file)" = file &&
+	test_must_fail git pull 2>err &&
+	test_i18ngrep "not currently on a branch" err &&
+	verbose 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 &&
+	verbose test "$(cat file)" = file &&
+	test_must_fail git pull 2>err &&
+	test_i18ngrep "no tracking information" err &&
+	verbose 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 &&
+	verbose test "$(cat file)" = file &&
+	test_must_fail git pull 2>err &&
+	test_i18ngrep "no such ref was fetched" err &&
+	verbose test "$(cat file)" = file
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
-- 
2.1.4

[PATCH v3 4/9] t5520: test for failure if index has unresolved entries

[Paul Tan]

Commit d38a30d (Be more user-friendly when refusing to do something
because of conflict., 2010-01-12) 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>
---

* Command substitution is now quoted.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e53d8e9..8e95402 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -164,6 +164,26 @@ test_expect_success 'fail if upstream branch does not exist' '
 	verbose test "$(cat file)" = file
 '
 
+test_expect_success 'fail if the index has unresolved entries' '
+	git checkout -b third second^ &&
+	test_when_finished "git checkout -f copy && git branch -D third" &&
+	verbose test "$(cat file)" = file &&
+	echo modified2 >file &&
+	git commit -a -m modified2 &&
+	verbose test -z "$(git ls-files -u)" &&
+	test_must_fail git pull . second &&
+	verbose test -n "$(git ls-files -u)" &&
+	cp file expected &&
+	test_must_fail git pull . second 2>err &&
+	test_i18ngrep "you have unmerged files" err &&
+	test_cmp expected file &&
+	git add file &&
+	verbose test -z "$(git ls-files -u)" &&
+	test_must_fail git pull . second 2>err &&
+	test_i18ngrep "have not concluded your merge" err &&
+	test_cmp expected file
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
-- 
2.1.4

[PATCH v3 5/9] t5520: test work tree fast-forward when fetch updates head

[Paul Tan]

Since b10ac50 (Fix pulling into the same branch., 2005-08-25), 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>
---

* Added git rev-parse checks to make it explicit that the branch head has
  been updated.

* "git checkout -b third second^" to make it explicit we are fast-forwarding.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 8e95402..954e581 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -184,6 +184,27 @@ 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 second^ &&
+	test_when_finished "git checkout -f copy && git branch -D third" &&
+	verbose test "$(cat file)" = file &&
+	git pull . second:third 2>err &&
+	test_i18ngrep "fetch updated the current branch head" err &&
+	verbose test "$(cat file)" = modified &&
+	verbose test "$(git rev-parse third)" = "$(git rev-parse second)"
+'
+
+test_expect_success 'fast-forward fails with conflicting work tree' '
+	git checkout -b third second^ &&
+	test_when_finished "git checkout -f copy && git branch -D third" &&
+	verbose test "$(cat file)" = file &&
+	echo conflict >file &&
+	test_must_fail git pull . second:third 2>err &&
+	test_i18ngrep "Cannot fast-forward your working tree" err &&
+	verbose test "$(cat file)" = conflict &&
+	verbose test "$(git rev-parse third)" = "$(git rev-parse second)"
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&
-- 
2.1.4

[PATCH v3 6/9] t5520: test --rebase with multiple branches

[Paul Tan]

Since rebasing on top of multiple upstream branches does not make sense,
since 51b2ead (disallow providing multiple upstream branches to rebase,
pull --rebase, 2009-02-18), 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>
---

* Quoted file content comparisons.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 954e581..e957368 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -218,6 +218,15 @@ test_expect_success '--rebase' '
 	verbose test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
 	verbose 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>err &&
+	verbose test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
+	test_i18ngrep "Cannot rebase onto multiple branches" err &&
+	verbose 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 v3 7/9] t5520: test --rebase failure on unborn branch with index

[Paul Tan]

Commit 19a7fcb (allow pull --rebase on branch yet to be born,
2009-08-11) 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>
---

* Renamed "out" to "err"

* Quoted command substitution and file content comparisons.

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

diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index e957368..96d2e7c 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -413,6 +413,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 &&
+		verbose test "$(git ls-files)" = staged-file &&
+		test_must_fail git pull --rebase .. master 2>../err &&
+		verbose test "$(git ls-files)" = staged-file &&
+		verbose test "$(git show :staged-file)" = staged-file
+	) &&
+	test_i18ngrep "unborn branch with changes added to the index" err
+'
+
 test_expect_success 'setup for detecting upstreamed changes' '
 	mkdir src &&
 	(cd src &&
-- 
2.1.4

[PATCH v3 8/9] t5521: test --dry-run does not make any changes

[Paul Tan]

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

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

* The index and work tree will not be modified.

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

diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 453aba5..37d6db6 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 v3 9/9] 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>
---
 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 96d2e7c..da120b2 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 &&
 	verbose test "$(cat file)" = file &&
 	git pull &&
-	verbose test "$(cat file)" = updated
+	verbose 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^ &&
 	verbose test "$(cat file)" = file &&
 	git pull . second &&
-	verbose test "$(cat file)" = modified
+	verbose 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 v3 4/9] t5520: test for failure if index has unresolved entries

[Matthieu Moy]

Paul Tan <pyokagan@gmail.com> writes:

> +test_expect_success 'fail if the index has unresolved entries' '
> +	git checkout -b third second^ &&
> +	test_when_finished "git checkout -f copy && git branch -D third" &&
> +	verbose test "$(cat file)" = file &&
> +	echo modified2 >file &&
> +	git commit -a -m modified2 &&
> +	verbose test -z "$(git ls-files -u)" &&
> +	test_must_fail git pull . second &&
> +	verbose test -n "$(git ls-files -u)" &&
> +	cp file expected &&
> +	test_must_fail git pull . second 2>err &&
> +	test_i18ngrep "you have unmerged files" err &&
> +	test_cmp expected file &&
> +	git add file &&
> +	verbose test -z "$(git ls-files -u)" &&
> +	test_must_fail git pull . second 2>err &&
> +	test_i18ngrep "have not concluded your merge" err &&

Reading this, I'm actually thinking that the message may have been
better written as "You have not concluded your previous merge". But
that's definitely out of the scope of this patch.

Anyway, the patch looks good to me, thanks.

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

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Many tests in t5520 used the following to test the contents of files:
>
> 	test `cat file` = expected
>
> or
>
> 	test $(cat file) = expected
>
> These 2 forms, however, will be affected by field splitting and,
> depending on the value of $IFS, may be split into multiple arguments,
> making the test fail in mysterious ways.
>
> Replace the above 2 forms with:
>
> 	verbose test "$(cat file)" = expected

Quoting is very much a good idea, but I am not enthused by the
vision of having to write verbose everywhere in our script.

After seeing a script fail, you can run it again with -i -x options;
wouldn't it be sufficient?

Re: [PATCH v3 2/9] t5520: ensure origin refs are updated

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> 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>
> ---
> * Hmm, no reviews the last round?

It is not unusual when the change is trivially correct.

I do not think this hurts, but I do not think that this is vastly
better, either.  If you suspect that the previous one may fail but
you at the same time are so trusting that the one before that one
would succeed, yes, this will help in that case.  But if you suspect
the previous one may fail, the one before that may have also failed,
in which case this may not be sufficient (the result of fetching may
not match what this test expects to see).

It all depends on where our paranoia ends. The current code is very
much trusting all previous ones equally, and accepts "upon the first
error, all bets are off for the later ones". With this patch, it
becomes slightly less trusting.

If everything else were equal, I would say this change is a "Meh" to
me, but I think the change improves this test in a different way.

It begins with "Run 'git rebase --abort', just in case"; which is a
signal that it does consider that the previous one may have failed
and attempts to prepare for that possibility, while trusting the one
before that would have succeeded.  And under that assumption, what
it currently does is _not_ consistent; the previous "pull --rebase"
may have failed in the "rebase" phase, in which case "abort just in
case" is a good measure to go back to the clean state, but it may
have failed in the "fetch" phase, in which case "abort" does not
help.  And this patch is needed to fix that inconsistency.

If justified in that way in the log message, then I wouldn't have
said "I do not think that this is vastly better", I think.

>  t/t5520-pull.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 20ad373..14a9280 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -339,6 +339,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 &&

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
>> Replace the above 2 forms with:
>>
>> 	verbose test "$(cat file)" = expected
>
> Quoting is very much a good idea, but I am not enthused by the
> vision of having to write verbose everywhere in our script.
>
> After seeing a script fail, you can run it again with -i -x options;
> wouldn't it be sufficient?

Just to avoid misunderstanding, I am unhappy if we have to keep
writing "verbose test" in that exact form.

Just like we invented to help debugging by wrapping "cmp" with
"test_cmp" (i.e. we want to see if the file contents are the same,
but a person who debugs can be helped by seeing the differences when
the expectation is not met), I do not mind if we had a shorter and
cleanly-named wrapper that we can use consistently.  E.g. I do not
mind something like this in test-lib-functions.sh

	test_file_contents () {
		if test "$(cat "$1")" != "$2"
		then
			echo "Contents of file $1 is not $2"
                        false
		fi
	}

and used like so:

	test_file_contents file expected_string

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Michael Blume]

On Wed, May 13, 2015 at 7:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Paul Tan <pyokagan@gmail.com> writes:
>>
>>> Replace the above 2 forms with:
>>>
>>>      verbose test "$(cat file)" = expected
>>
>> Quoting is very much a good idea, but I am not enthused by the
>> vision of having to write verbose everywhere in our script.
>>
>> After seeing a script fail, you can run it again with -i -x options;
>> wouldn't it be sufficient?
>
> Just to avoid misunderstanding, I am unhappy if we have to keep
> writing "verbose test" in that exact form.
>
> Just like we invented to help debugging by wrapping "cmp" with
> "test_cmp" (i.e. we want to see if the file contents are the same,
> but a person who debugs can be helped by seeing the differences when
> the expectation is not met), I do not mind if we had a shorter and
> cleanly-named wrapper that we can use consistently.  E.g. I do not
> mind something like this in test-lib-functions.sh
>
>         test_file_contents () {
>                 if test "$(cat "$1")" != "$2"
>                 then
>                         echo "Contents of file $1 is not $2"
>                         false
>                 fi
>         }
>
> and used like so:
>
>         test_file_contents file expected_string
>
> --
> 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

My build starts breaking from this commit, I'm on a mac.


expecting success:
    (cd dst &&
     test_must_fail git pull --rebase &&
     verbose test 1 = "$(find .git/rebase-apply -name "000*" | wc -l)"
    )

First, rewinding head to replay your work on top of it...
Applying: Modified Change 4
Using index info to reconstruct a base tree...
M    stuff
Falling back to patching base and 3-way merge...
Merging HEAD with Modified Change 4
Merging:
814f1c3 Change 4
virtual Modified Change 4
found 1 common ancestor:
virtual e369e858ec1aa73d497d02c4f23e5cf4ae2d3c3b
Auto-merging stuff
CONFLICT (content): Merge conflict in stuff
Failed to merge in the changes.
Patch failed at 0001 Modified Change 4
The copy of the patch that failed is found in:
   /Users/michael.blume/workspace/git/t/trash
directory.t5520-pull/dst/.git/rebase-apply/patch

When you have resolved this problem, run "git rebase --continue".
If you prefer to skip this patch, run "git rebase --skip" instead.
To check out the original branch and stop rebasing, run "git rebase --abort".

command failed:  'test' '1' '=' '       1'
not ok 33 - git pull --rebase does not reapply old patches
#
#        (cd dst &&
#         test_must_fail git pull --rebase &&
#         verbose test 1 = "$(find .git/rebase-apply -name "000*" | wc -l)"
#        )
#

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Michael Blume <blume.mike@gmail.com> writes:

> On Wed, May 13, 2015 at 7:42 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Paul Tan <pyokagan@gmail.com> writes:
>>>
>>>> Replace the above 2 forms with:
>>>>
>>>>      verbose test "$(cat file)" = expected
>>>
>>> Quoting is very much a good idea, but I am not enthused by the
>>> vision of having to write verbose everywhere in our script.
>>> 
>>> After seeing a script fail, you can run it again with -i -x options;
>>> wouldn't it be sufficient?
>> ...
>
> My build starts breaking from this commit, I'm on a mac.
>
>
> expecting success:
>     (cd dst &&
>      test_must_fail git pull --rebase &&
>      verbose test 1 = "$(find .git/rebase-apply -name "000*" | wc -l)"
>     )
>
> First, rewinding head to replay your work on top of it...
> ...
> To check out the original branch and stop rebasing, run "git rebase --abort".
>
> command failed:  'test' '1' '=' '       1'
> not ok 33 - git pull --rebase does not reapply old patches

Change that 'verbose test' line to

	verbose test 1 = $(find .git/rebase-apply -name "000*" | wc -l)

i.e. losing the double-quotes around $().

By the way, thanks for a fine demonstration that the 'verbose test'
is not very useful.

This output

> command failed:  'test' '1' '=' '       1'

and that you said "on a mac", _I_ can immediately guess that there
is "wc -l" involved, because it has been a frequent source of
portability headache.

But "verbose" is not helping very much to show there is "wc -l";
unless the person debugging the output has a pretty good idea what
can go wrong, that is.

Re: [PATCH v3 4/9] t5520: test for failure if index has unresolved entries

[Paul Tan]

On Wed, May 13, 2015 at 5:08 PM, Paul Tan <pyokagan@gmail.com> wrote:
> +test_expect_success 'fail if the index has unresolved entries' '
> +       git checkout -b third second^ &&
> +       test_when_finished "git checkout -f copy && git branch -D third" &&
> +       verbose test "$(cat file)" = file &&
> +       echo modified2 >file &&
> +       git commit -a -m modified2 &&
> +       verbose test -z "$(git ls-files -u)" &&
> +       test_must_fail git pull . second &&
> +       verbose test -n "$(git ls-files -u)" &&
> +       cp file expected &&
> +       test_must_fail git pull . second 2>err &&
> +       test_i18ngrep "you have unmerged files" err &&

Hmm, it appears that this is too loose, as git-merge will throw "merge
is not possible because you have unmerged files".

So it looks like we will have to go back to the stricter "Pull is not
possible because you have unmerged files".

> +       test_cmp expected file &&
> +       git add file &&
> +       verbose test -z "$(git ls-files -u)" &&
> +       test_must_fail git pull . second 2>err &&
> +       test_i18ngrep "have not concluded your merge" err &&
> +       test_cmp expected file
> +'
> +
>  test_expect_success '--rebase' '
>         git branch to-rebase &&
>         echo modified again > file &&
> --
> 2.1.4
>

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Paul Tan]

Hi Junio,

On Fri, May 15, 2015 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Change that 'verbose test' line to
>
>         verbose test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
>
> i.e. losing the double-quotes around $().

Noted and fixed. Interesting quirk though :-).

> By the way, thanks for a fine demonstration that the 'verbose test'
> is not very useful.
>
> This output
>
>> command failed:  'test' '1' '=' '       1'

Personally, I find that the quoting provided by "verbose" helps make
it clear that it's a whitespace issue, which might be a bit harder to
spot with the output of set -x I think.

Other than that, I'm also convinced that "verbose" doesn't really
offer much. Will remove in the re-roll.

Thanks,
Paul

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Hi Junio,
>
> On Fri, May 15, 2015 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Change that 'verbose test' line to
>>
>>         verbose test 1 = $(find .git/rebase-apply -name "000*" | wc -l)
>>
>> i.e. losing the double-quotes around $().
>
> Noted and fixed. Interesting quirk though :-).
>
>> By the way, thanks for a fine demonstration that the 'verbose test'
>> is not very useful.
>>
>> This output
>>
>>> command failed:  'test' '1' '=' '       1'
>
> Personally, I find that the quoting provided by "verbose" helps make
> it clear that it's a whitespace issue, which might be a bit harder to
> spot with the output of set -x I think.

To be fair, yes, because of the leading SP in the RHS, I immediately
knew that this was a "wc -l" from that without running the test one
more time with "-i -v -x".  The "rev-parse --sq-quote" did help.
Without the --sq-quote trick, i.e. "command failed: test 1 = 1",
would actually make the debugger suspect that there would be a
quoting issue anyway, so it is not a very big deal, though.

In any case, that "test 1 = 1" (with or without quoting) helped only
because I had to deal with "wc -l" issues in the past.  Without
telling how that ' 1' ended up compared with '1' by showing "wc -l"
on that 'command failed:' line, it wouldn't have helped much if the
debugger were not me.

> Other than that, I'm also convinced that "verbose" doesn't really
> offer much. Will remove in the re-roll.

Just to avoid misunderstanding, please do not remove 'verbose '
blindly without thinking while doing so, as you already did 1/3 of
the necessary job to make things better.

You might have noticed, while adding them, there were something
common that we currently do with a bare 'test' only because we
haven't identified common needs.  As I already said, it may be that
we often try to see a file has a known single line content (I didn't
check if that were the case; I am just giving you an example) and
only because there is no ready-made test_file_contents helper to be
used, the current tests say

	test expected_string = "$(cat file)"

And if that were the case, it is a good thing to have a new helper
like this

	test_file_contents () {
		if test "$(cat "$1")" != "$2"
		then
			echo "Contents of file '$1' is not '$2'"
                        false
		fi
	}

in t/test-lib-functions.sh and convert them to say

	test_file_contents file expected_string

That would be an improvement (and that is the remaining 2/3 ;-).

Thanks.

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> You might have noticed, while adding them, there were something
> common that we currently do with a bare 'test' only because we
> haven't identified common needs....
> ...
> in t/test-lib-functions.sh and convert them to say
>
> 	test_file_contents file expected_string
>
> That would be an improvement (and that is the remaining 2/3 ;-).

I haven't made up my mind on this other example, but since I started
writing it...

It may be that counting the number of lines in output, "cmd | wc -l",
is a common pattern.  We already have test_line_count to check the
number of lines in a file, but having test_output_count may help.

 t/test-lib-functions.sh | 11 +++++++++++
 t/t0000-basic.sh        |  3 +--
 t/t0030-stripspace.sh   | 11 +++++------
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 0d93e33..624a8c5 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -538,6 +538,17 @@ test_line_count () {
 	fi
 }
 
+test_output_count () {
+	if test $# != 3
+	then
+		error "bug in the test script: not 3 parameters to test_output_count"
+	elif ! test $(eval "$2" | wc -l) "$1" "$3"
+	then
+		echo "test_output_count: line count for output from '$2' !$1 $3"
+		return 1
+	fi
+}
+
 # This is not among top-level (test_expect_success | test_expect_failure)
 # but is a prefix that can be used in the test script, like:
 #

diff --git a/t/t0000-basic.sh b/t/t0000-basic.sh
index f10ba4a..bd5930d 100755
--- a/t/t0000-basic.sh
+++ b/t/t0000-basic.sh
@@ -1039,8 +1039,7 @@ test_expect_success 'update-index D/F conflict' '
 	mv path2 path0 &&
 	mv tmp path2 &&
 	git update-index --add --replace path2 path0/file2 &&
-	numpath0=$(git ls-files path0 | wc -l) &&
-	test $numpath0 = 1
+	test_output_count = "git ls-files path0" 1
 '
 
 test_expect_success 'very long name in the index handled sanely' '
diff --git a/t/t0030-stripspace.sh b/t/t0030-stripspace.sh
index 0333dd9..9502938 100755
--- a/t/t0030-stripspace.sh
+++ b/t/t0030-stripspace.sh
@@ -223,12 +223,11 @@ test_expect_success \
     test_cmp expect actual
 '
 
-test_expect_success \
-    'text without newline at end should end with newline' '
-    test $(printf "$ttt" | git stripspace | wc -l) -gt 0 &&
-    test $(printf "$ttt$ttt" | git stripspace | wc -l) -gt 0 &&
-    test $(printf "$ttt$ttt$ttt" | git stripspace | wc -l) -gt 0 &&
-    test $(printf "$ttt$ttt$ttt$ttt" | git stripspace | wc -l) -gt 0
+test_expect_success 'text without newline at end should end with newline' '
+	test_output_count -gt '\''printf "$ttt" | git stripspace'\'' 0 &&
+	test_output_count -gt '\''printf "$ttt$ttt" | git stripspace'\'' 0 &&
+	test_output_count -gt '\''printf "$ttt$ttt$ttt" | git stripspace'\'' 0 &&
+	test_output_count -gt '\''printf "$ttt$ttt$ttt$ttt" | git stripspace'\'' 0
 '
 
 # text plus spaces at the end:

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Paul Tan]

Hi Junio,

On Sat, May 16, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Just to avoid misunderstanding, please do not remove 'verbose '
> blindly without thinking while doing so, as you already did 1/3 of
> the necessary job to make things better.

Eh? I thought we established that using "verbose" does not provide
anything more than what "set -x" already provides. So at the very
least, its use should be removed completely.

Here is my understanding of the current situation, please correct me
if I am wrong:

When a test fails, it would be useful to know the exact, unexpanded,
unsubstituted command which failed (additionally with a nice stack
trace and line numbers). However, the output of -v and -x (and by
extension, the "verbose" function) is not very helpful, as it still
requires the debugger to understand the test script.

-v will print out the stdout and stderr of the executed commands, but
it requires the debugger to match up the output of the commands with
the test script to understand where the test failed. It is also not
very helpful in the case of "test", which does not print anything when
it fails.

-x will trace the commands being executed, but the tracing output is
so verbose it still requires the debugger to understand the test
script. e.g:

     test "$(cat file)" "expected"

If the above test fails (e.g. the content of the file is
"unexpected"), the tracing output will be:

     + cat file
     + test unexpected = expected
     error: last command exited with $?=1

Furthermore, the format of the tracing output is not specified by
POSIX, so we can't count on it being consistent among all shells (e.g.
for users who submit bug reports with the test output)

> You might have noticed, while adding them, there were something
> common that we currently do with a bare 'test' only because we
> haven't identified common needs.  As I already said, it may be that
> we often try to see a file has a known single line content (I didn't
> check if that were the case; I am just giving you an example) and
> only because there is no ready-made test_file_contents helper to be
> used, the current tests say
>
>         test expected_string = "$(cat file)"
>
> And if that were the case, it is a good thing to have a new helper
> like this
>
>         test_file_contents () {
>                 if test "$(cat "$1")" != "$2"
>                 then
>                         echo "Contents of file '$1' is not '$2'"
>                         false
>                 fi
>         }
>
> in t/test-lib-functions.sh and convert them to say
>
>         test_file_contents file expected_string
>
> That would be an improvement (and that is the remaining 2/3 ;-).

Yeah, this kind of comparison with file contents is something that is
done often in t5520, so I agree with adding it.

However, what about these kind of tests:

     test new = "$(git show HEAD:file2)"

or these:

     test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)

So, perhaps we could introduce a generic function like:

    # Compares that the output of $1 eval'ed is identical to $2.
    test_output () {
        output=$(eval $1)
        if "$output" != "$2"
        then
             echo >&2 "Output of '$1' ('$output') != '$2'"
             false
        fi
    }

So the first example would be:

    test_output "git show HEAD:file2" new

And the error output will thus be:

     Output of 'git show HEAD:file2' ('some unexpected output') != 'new'

So we know the exact comparison that failed, and we know how the
expected and actual output differs.

What do you think?

Thanks,
Paul

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

> Hi Junio,
>
> On Sat, May 16, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Just to avoid misunderstanding, please do not remove 'verbose '
>> blindly without thinking while doing so, as you already did 1/3 of
>> the necessary job to make things better.
>
> Eh? I thought we established that using "verbose" does not provide
> anything more than what "set -x" already provides. So at the very
> least, its use should be removed completely.

I did not mean "do not remove and keep them".  I meant "do not
remove without thinking; instead, take mental notes on patterns
these silent ones may have while removing them".

>> You might have noticed, while adding them, there were something
>> common that we currently do with a bare 'test' only because we
>> haven't identified common needs.  As I already said,...
>> ...
>> That would be an improvement (and that is the remaining 2/3 ;-).
>
> Yeah, this kind of comparison with file contents is something that is
> done often in t5520, so I agree with adding it.
>
> However, what about these kind of tests:
>
>      test new = "$(git show HEAD:file2)"
>
> or these:
>
>      test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
>
> So, perhaps we could introduce a generic function like:

It all depends on how common they are.

> So the first example would be:
>
>     test_output "git show HEAD:file2" new

Simple things like that look fine, but when a variable is involved,
use of eval combined with the fact that the test body is inside sq,
makes the callers unnecessarily ugly.

	test_expect_success 'some title' '
		var=$(...) &&
		test_output "git show \$var:file2 | sed -e \"s/$old/$new/\"" new
	'

Which is the concern this shares with the other one I sent about
counting the number of lines in the output from a command that made
me hesitate to suggest it.

So I dunno.

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Paul Tan <pyokagan@gmail.com> writes:
>
>> So the first example would be:
>>
>>     test_output "git show HEAD:file2" new
>
> Simple things like that look fine, but when a variable is involved,
> use of eval combined with the fact that the test body is inside sq,
> makes the callers unnecessarily ugly.
>
> 	test_expect_success 'some title' '
> 		var=$(...) &&
> 		test_output "git show \$var:file2 | sed -e \"s/$old/$new/\"" new
> 	'
>
> Which is the concern this shares with the other one I sent about
> counting the number of lines in the output from a command that made
> me hesitate to suggest it.
>
> So I dunno.

I actually think that "test" that compares output from command and a
constant string, and "test" that compares outputs from two commands
are lazyily written forms of these:

        echo constant string >expect &&
	command >actual &&
        test_cmp expect actual

	command1 >expect &&
        command2 >actual &&
        test_cmp expect actual

The examples you gave in the earlier message were

>
>      test new = "$(git show HEAD:file2)"
>
> or these:
>
>      test $(git rev-parse HEAD^2) = $(git rev-parse keep-merge)
>

and I suspect they match my observation.

My earlier test_output_count was probably in the same "lazy"
category.  "test $(command | wc -l) = 20" is better written
as

	command >output &&
        test_line_count = 20 output

instead of using the hypothetical

	test_output_count = 20 "command"

that evals the command argument, not only because the quoting of
'command part will become complex for real world uses, but because
the output itself would be the first thing we would want to inspect
once the command fails.  For that reason, I'd rather not to add the
test_output_count I suggested earlier, so that we would encourage
the more straight-forward form, i.e.

	command >output &&
        test_line_count = 20 output

to be used.

Re: [PATCH v3 1/9] t5520: fixup file contents comparisons

[Paul Tan]

Hi Junio,

On Sun, May 17, 2015 at 2:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Paul Tan <pyokagan@gmail.com> writes:
>
>> Hi Junio,
>>
>> On Sat, May 16, 2015 at 2:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Just to avoid misunderstanding, please do not remove 'verbose '
>>> blindly without thinking while doing so, as you already did 1/3 of
>>> the necessary job to make things better.
>>
>> Eh? I thought we established that using "verbose" does not provide
>> anything more than what "set -x" already provides. So at the very
>> least, its use should be removed completely.
>
> I did not mean "do not remove and keep them".  I meant "do not
> remove without thinking; instead, take mental notes on patterns
> these silent ones may have while removing them".

Okay. Just to keep things moving, for now I will send a re-roll to
remove "verbose" and to fix other issues like [1] and [2].

[1] http://thread.gmane.org/gmane.comp.version-control.git/268950/focus=269052
[2] http://thread.gmane.org/gmane.comp.version-control.git/268950/focus=269132

Thanks,
Paul

Re: [PATCH v3 2/9] t5520: ensure origin refs are updated

[Paul Tan]

Hi Junio,

On Wed, May 13, 2015 at 10:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
> It is not unusual when the change is trivially correct.
>
> I do not think this hurts, but I do not think that this is vastly
> better, either.  If you suspect that the previous one may fail but
> you at the same time are so trusting that the one before that one
> would succeed, yes, this will help in that case.  But if you suspect
> the previous one may fail, the one before that may have also failed,
> in which case this may not be sufficient (the result of fetching may
> not match what this test expects to see).
>
> It all depends on where our paranoia ends. The current code is very
> much trusting all previous ones equally, and accepts "upon the first
> error, all bets are off for the later ones". With this patch, it
> becomes slightly less trusting.
>
> If everything else were equal, I would say this change is a "Meh" to
> me,

Yeah, thinking about it, it's a "meh" to me too. While this patch will
improve consistency in the attempt to recover from failure, I don't
think it will be useful in practice, and it's also not relevant to the
goal of this series.

Will drop this patch.

> but I think the change improves this test in a different way.
>
> It begins with "Run 'git rebase --abort', just in case"; which is a
> signal that it does consider that the previous one may have failed
> and attempts to prepare for that possibility, while trusting the one
> before that would have succeeded.  And under that assumption, what
> it currently does is _not_ consistent; the previous "pull --rebase"
> may have failed in the "rebase" phase, in which case "abort just in
> case" is a good measure to go back to the clean state, but it may
> have failed in the "fetch" phase, in which case "abort" does not
> help.  And this patch is needed to fix that inconsistency.
>
> If justified in that way in the log message, then I wouldn't have
> said "I do not think that this is vastly better", I think.
>

Thanks,
Paul