threads / patch / 39353

v4, 8 partsImprove git-pull test coverage

Subject: [PATCH v4 0/8] Improve git-pull test coverage

## tl;dr

24 messages between May 18, 2015 and Jun 5, 2015. Diffs are folded; open one to read it.

replies: 23people: 5as markdown or json

Paul Tan· May 18, 2015, 13:32 UTC · lore
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 contains various small fixups. It also removes the use of "verbose" for now, since its use is currently under discussion.

Thanks Junio and Matthieu for the reviews last round, and thanks Michael for reporting the test failure.

[1] http://thread.gmane.org/gmane.comp.version-control.git/268950
Paul Tan (8):
  t5520: prevent field splitting in content comparisons
  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         | 199 +++++++++++++++++++++++++++++++++++++++---------
 t/t5521-pull-options.sh |  13 ++++
 2 files changed, 176 insertions(+), 36 deletions(-)
-- 
2.1.4
Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

[PATCH v4 1/8] t5520: prevent field splitting in content comparisons

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:
	test "$(cat file)" = expected
as quoting the command substitution will prevent field splitting.
Signed-off-by: Paul Tan <pyokagan@gmail.com>
---
* Removed use of "verbose"
* The use of "wc -l" is not quoted, as the output of "wc -l" on a Mac contains
  leading whitespace. See [1].
[1] http://thread.gmane.org/gmane.comp.version-control.git/268950/focus=269052
 t/t5520-pull.sh | 70 ++++++++++++++++++++++++++++-----------------------------
 1 file changed, 35 insertions(+), 35 deletions(-)
Show changes to t/t5520-pull.sh +35 −35
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 7efd45b..5e4db67 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -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 &&
+	test "$(cat file)" = file &&
 	git pull &&
-	test `cat file` = updated
+	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 &&
+	test "$(cat file)" = file &&
 	git pull . second &&
-	test `cat file` = modified
+	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)
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^)" != "$(git rev-parse copy)" &&
+	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)
+	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)"
 '
 
 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)
+	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	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)
+	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)"
 '
 
 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)
+	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	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)
+	test "$(git rev-parse HEAD^^)" = "$(git rev-parse copy)" &&
+	test file3 = "$(git show HEAD:file3.t)"
 '
 
 test_expect_success '--rebase with rebased upstream' '
@@ -250,7 +250,7 @@ test_expect_success '--rebase with rebased upstream' '
 	git tag to-rebase-orig &&
 	git pull --rebase me copy &&
 	test "conflicting modification" = "$(cat file)" &&
-	test file = $(cat file2)
+	test file = "$(cat file2)"
 
 '
 
@@ -261,7 +261,7 @@ test_expect_success '--rebase with rebased default upstream' '
 	git reset --hard to-rebase-orig &&
 	git pull --rebase &&
 	test "conflicting modification" = "$(cat file)" &&
-	test file = $(cat file2)
+	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) &&
+	test "$COPY" = "$(git rev-parse --verify me/copy)" &&
 	git checkout HEAD -- file &&
 	git pull &&
-	test $COPY != $(git rev-parse --verify me/copy)
+	test "$COPY" != "$(git rev-parse --verify me/copy)"
 
 '
 

-- 
2.1.4
Junio C Hamano· May 18, 2015, 18:07 UTC · re: Paul Tan · lore

Re: [PATCH v4 1/8] t5520: prevent field splitting in content comparisons

Paul Tan <pyokagan@gmail.com> writes:
Show 29 quoted lines
> 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:
>
> 	test "$(cat file)" = expected
>
> as quoting the command substitution will prevent field splitting.
>
> Signed-off-by: Paul Tan <pyokagan@gmail.com>
> ---
> * Removed use of "verbose"
>
> * The use of "wc -l" is not quoted, as the output of "wc -l" on a Mac contains
>   leading whitespace. See [1].
>
> [1] http://thread.gmane.org/gmane.comp.version-control.git/268950/focus=269052
>
>  t/t5520-pull.sh | 70 ++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
Overall the series was a pleasant read.

Some common patterns (my mind is still continuing the verbose_test topic) I noticed were

 (1) comparing output from two rev-parse output for object names. We
     have test_cmp_rev we can use for better readability (but its
     implementaiton may want to be updated not to contaminate the
     working tree unnecessarily). e.g
	test "$(git rev-parse HEAD^)" = "$(git rev-parse copy)"
     => test_cmp_rev HEAD^ copy
 
 (2) checking contents of a file, either from a working tree, from
     the index or from a rev, with a fixed short string.
	test_contents () {
                case "$1" in
                *:*)	git cat-file blob "$1" ;;
                *)	cat "$1" ;;
                esac >actual &&
		printf "%s" "$2" >expect &&
		if ! cmp -s expect actual
                then
			echo "Unexpected contents in '$1'"
			test_cmp expect actual
		else
			: good ;
		fi
	}
     or something.

I think it is a good idea *not* to address these patterns within this series, and have people come back to them _after_ the series settles, to reduce code churn and unnecessary conflicts.

Thanks.
Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

[PATCH v4 2/8] t5520: test no merge candidates cases

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>
---
* Removed use of "verbose".
 t/t5520-pull.sh | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)
Show changes to t/t5520-pull.sh +55 −0
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 5e4db67..4a2c0a1 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>err &&
+	test_i18ngrep "no candidates for merging" err &&
+	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>err &&
+	test_i18ngrep "specify a branch on the command line" err &&
+	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>err &&
+	test_i18ngrep "not currently on a branch" err &&
+	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>err &&
+	test_i18ngrep "no tracking information" err &&
+	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>err &&
+	test_i18ngrep "no such ref was fetched" err &&
+	test "$(cat file)" = file
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&

-- 
2.1.4
Johannes Schindelin· May 18, 2015, 15:08 UTC · re: Paul Tan · lore

Re: [PATCH v4 2/8] t5520: test no merge candidates cases

Hi Paul,
On 2015-05-18 15:32, Paul Tan wrote:
Show 12 quoted lines
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 5e4db67..4a2c0a1 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" &&
When I read this line, I immediately asked myself whether the branch would be deleted even if the test case failed. I then tested this theory by editing the first test case ("setup") like this:
-- snip --
Show changes to t/t5520-pull.sh +3 −1
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 227d293..3adc702 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -15,9 +15,12 @@ test_expect_success setup '
 
 	echo file >file &&
 	git add file &&
+test_when_finished "rm file" &&
+false &&
 	git commit -a -m original
 
 '
+exit 1
 
 test_expect_success 'pulling into void' '
 	mkdir cloned &&
-- snap --

and indeed, the file "file" was gone, even if the test case failed. I therefore believe that this "test_when_finished" cleanup might make debugging substantially harder. Maybe we can drop these lines from this patch?

Ciao,
Dscho
Junio C Hamano· May 18, 2015, 17:46 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4 2/8] t5520: test no merge candidates cases

Johannes Schindelin <johannes.schindelin@gmx.de> writes:
Show 13 quoted lines
>> +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" &&
>
> When I read this line, I immediately asked myself whether the
> branch would be deleted even if the test case failed. I then
> tested this theory by editing the first test case ("setup") like
> this:
> ...
> and indeed, the file "file" was gone, even if the test case
> failed. I therefore believe that this "test_when_finished" cleanup
> might make debugging substantially harder. Maybe we can drop these
> lines from this patch?

The test framework is aware of the fact that it needs to help the people who are debugging the scripts. The support is limited to the case in which you run it under the -i option, i.e.

	$ cd t
        $ sh ./t5520-pull.sh -i -v

will refrain from running test_when_finished scripts when the test piece fails. Even though this is only limited to -i, I found it often sufficient for debugging.

Jeff King· May 18, 2015, 18:55 UTC · re: Junio C Hamano · lore

debugging git tests, was: Re: [PATCH v4 2/8] t5520: test no merge candidates cases

On Mon, May 18, 2015 at 10:46:50AM -0700, Junio C Hamano wrote:
Show 26 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
> 
> >> +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" &&
> >
> > When I read this line, I immediately asked myself whether the
> > branch would be deleted even if the test case failed. I then
> > tested this theory by editing the first test case ("setup") like
> > this:
> > ...
> > and indeed, the file "file" was gone, even if the test case
> > failed. I therefore believe that this "test_when_finished" cleanup
> > might make debugging substantially harder. Maybe we can drop these
> > lines from this patch?
> 
> The test framework is aware of the fact that it needs to help the
> people who are debugging the scripts.  The support is limited to the
> case in which you run it under the -i option, i.e.
> 
> 	$ cd t
>         $ sh ./t5520-pull.sh -i -v
> 
> will refrain from running test_when_finished scripts when the test
> piece fails.  Even though this is only limited to -i, I found it
> often sufficient for debugging.

If you don't use "-i", you are pretty much screwed anyway, because the subsequent tests will stomp all over the state of the test directory. Many a head-scratching session has been caused by looking at the wrong state, and these days my go-to options for debugging a test are "-v -i". But since we are talking about it in a related thread, I will advertise the new "-x" here, too. :)

As a side note, I've also considered better support for running the debugger on git commands inside a test (right now, I usually stick a "gdb --args" in the pipeline, but you have to remember to run with "-v", and to redirect stdin appropriately). Do other people have this annoyance, too?

I'm vaguely thinking of something like putting debug support into bin-wrappers/git, but activating it only for certain tests (so you could say "t5520-pull.sh --gdb=10", and git would start under the debugger only for test 10). I think we'd also have to use gdbserver for I/O sanity, and maybe provide short script to do:

   gdb -ex "target remote localhost:$some_port" "$TEST_DIRECTORY"/../git

That still doesn't cover all cases (when git spawns an external command, you probably want to run the debugger on that; likewise, I have a git-remote-debug hack for debugging remote-curl). I suspect with clever use of gdb options that you could convince the original gdb invocation to end up tracing the process you care about, though.

-Peff
Junio C Hamano· May 18, 2015, 19:35 UTC · re: Jeff King · lore

Re: debugging git tests, was: Re: [PATCH v4 2/8] t5520: test no merge candidates cases

Jeff King <peff@peff.net> writes:
Show 15 quoted lines
> On Mon, May 18, 2015 at 10:46:50AM -0700, Junio C Hamano wrote:
>
>> The test framework is aware of the fact that it needs to help the
>> people who are debugging the scripts.  The support is limited to the
>> case in which you run it under the -i option, i.e.
>> 
>> 	$ cd t
>>         $ sh ./t5520-pull.sh -i -v
>> 
>> will refrain from running test_when_finished scripts when the test
>> piece fails.  Even though this is only limited to -i, I found it
>> often sufficient for debugging.
>
> If you don't use "-i", you are pretty much screwed anyway, because the
> subsequent tests will stomp all over the state of the test directory.
Yeah.
> Many a head-scratching session has been caused by looking at the wrong
> state, and these days my go-to options for debugging a test are "-v -i".
> But since we are talking about it in a related thread, I will advertise
> the new "-x" here, too.  :)
Yes, thanks for "-x".  That has been very helpful.
Show 5 quoted lines
> As a side note, I've also considered better support for running the
> debugger on git commands inside a test (right now, I usually stick a
> "gdb --args" in the pipeline, but you have to remember to run with "-v",
> and to redirect stdin appropriately). Do other people have this
> annoyance, too?

I usually tweak the script and have it stop before the offending test, and then go through the steps in the test manually X-<. If it can be more automated, that would be great.

I haven't been ambitious enough to even attempt it so do not have anything to add to the implementation ideas at this point.

Show 15 quoted lines
> I'm vaguely thinking of something like putting debug support into
> bin-wrappers/git, but activating it only for certain tests (so you could
> say "t5520-pull.sh --gdb=10", and git would start under the debugger
> only for test 10). I think we'd also have to use gdbserver for I/O
> sanity, and maybe provide short script to do:
>
>    gdb -ex "target remote localhost:$some_port" "$TEST_DIRECTORY"/../git
>
> That still doesn't cover all cases (when git spawns an external command,
> you probably want to run the debugger on that; likewise, I have a
> git-remote-debug hack for debugging remote-curl). I suspect with clever
> use of gdb options that you could convince the original gdb invocation
> to end up tracing the process you care about, though.
>
> -Peff
Johannes Schindelin· May 19, 2015, 13:29 UTC · re: Jeff King · lore

Re: debugging git tests, was: Re: [PATCH v4 2/8] t5520: test no merge candidates cases

Hi Peff,
On 2015-05-18 20:55, Jeff King wrote:
Show 5 quoted lines
> As a side note, I've also considered better support for running the
> debugger on git commands inside a test (right now, I usually stick a
> "gdb --args" in the pipeline, but you have to remember to run with "-v",
> and to redirect stdin appropriately). Do other people have this
> annoyance, too?
Yes, I have that annoyance, and I usually want to debug one very specific Git call in that script. Therefore, I made this patch:
https://github.com/git-for-windows/git/commit/0f50fd723beb5d27d2d799ef3a0cfc0b0bbd80a8
It really served me well during the past four months of Git for Windows 2.x work. Of course I also had to avoid the redirection:
https://github.com/git-for-windows/git/commit/d27cb6e0572475582677724545a71755d171ea76
So what I do these days is to run the failing test with `TEST_NO_REDIRECT=1` and with the Git call I want to debug prefixed with `TEST_GDB_GIT=1`.

Ciao, Dscho

Jeff King· Jun 5, 2015, 10:44 UTC · re: Johannes Schindelin · lore

Re: debugging git tests, was: Re: [PATCH v4 2/8] t5520: test no merge candidates cases

On Tue, May 19, 2015 at 03:29:23PM +0200, Johannes Schindelin wrote:
Show 17 quoted lines
> On 2015-05-18 20:55, Jeff King wrote:
> 
> > As a side note, I've also considered better support for running the
> > debugger on git commands inside a test (right now, I usually stick a
> > "gdb --args" in the pipeline, but you have to remember to run with "-v",
> > and to redirect stdin appropriately). Do other people have this
> > annoyance, too?
> 
> Yes, I have that annoyance, and I usually want to debug one very specific Git call in that script. Therefore, I made this patch:
> 
> https://github.com/git-for-windows/git/commit/0f50fd723beb5d27d2d799ef3a0cfc0b0bbd80a8
> 
> It really served me well during the past four months of Git for Windows 2.x work. Of course I also had to avoid the redirection:
> 
> https://github.com/git-for-windows/git/commit/d27cb6e0572475582677724545a71755d171ea76
> 
> So what I do these days is to run the failing test with `TEST_NO_REDIRECT=1` and with the Git call I want to debug prefixed with `TEST_GDB_GIT=1`.

Thanks for sharing this (I stuck it on my "to read" list and didn't get to it until today).

This is definitely the direction I'd like to go in. I think it would be cool to add in a test-harness option enable TEST_GIT_GDB for a specific test. So something like:

  ./t1234-foo.sh --gdb=8

to just enable it for a particular test (of course some test snippets run multiple git invocations, but in practice I think it is few enough that you could just ask gdb to run to completion until you hit the invocation you want).

I don't have any code to share yet. I'm still at the point of collecting ideas like this, which I'll rage-implement next time I have to do a lot of debugging. :)

-Peff
Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

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

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>
---
* Use the stricter "Pull is not possible because you have unmerged files"
  instead of you have unmerged files" as git-merge will also raise a similar
  error "merge is not possible because you have unmerged files"
* Also, grep for "You have not concluded your merge" instead of "have not
  concluded your merge" as the latter reads weird to me personally.
 t/t5520-pull.sh | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
Show changes to t/t5520-pull.sh +20 −0
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 4a2c0a1..3bc0594 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' '
 	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" &&
+	test "$(cat file)" = 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>err &&
+	test_i18ngrep "Pull is not possible because you have unmerged files" err &&
+	test_cmp expected file &&
+	git add file &&
+	test -z "$(git ls-files -u)" &&
+	test_must_fail git pull . second 2>err &&
+	test_i18ngrep "You 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
Johannes Schindelin· May 18, 2015, 15:13 UTC · re: Paul Tan · lore

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

Hi Paul,
On 2015-05-18 15:32, Paul Tan wrote:
Show 12 quoted lines
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 4a2c0a1..3bc0594 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' '
>  	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" &&
If you agree with my argument in 2/8, it would be nice to drop this line, too.
> +	test "$(cat file)" = file &&
> +	echo modified2 >file &&
> +	git commit -a -m modified2 &&
These two lines could be combined into "test_commit modified2 file".

Ciao, Dscho

Paul Tan· May 21, 2015, 08:15 UTC · re: Johannes Schindelin · lore

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

Hi,

On Mon, May 18, 2015 at 11:13 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 18 quoted lines
> Hi Paul,
>
> On 2015-05-18 15:32, Paul Tan wrote:
>
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index 4a2c0a1..3bc0594 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' '
>>       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" &&
>
> If you agree with my argument in 2/8, it would be nice to drop this line, too.

As mentioned by Junio in 2/8 the cleanup functions will not run under --immediate mode.

Besides, the use of test_when_finished is to send a clear signal that the "third" branch is temporary and is not meant to be used by other test cases. Furthermore, subsequent tests assume that the current branch is "copy", so it's best to preserve that.

Show 5 quoted lines
>> +     test "$(cat file)" = file &&
>> +     echo modified2 >file &&
>> +     git commit -a -m modified2 &&
>
> These two lines could be combined into "test_commit modified2 file".
Fixed, thanks.

Regards, Paul

Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

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

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>
---
* Removed use of "verbose".
 t/t5520-pull.sh | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
Show changes to t/t5520-pull.sh +21 −0
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 3bc0594..3a53a5e 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" &&
+	test "$(cat file)" = file &&
+	git pull . second:third 2>err &&
+	test_i18ngrep "fetch updated the current branch head" err &&
+	test "$(cat file)" = modified &&
+	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" &&
+	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 &&
+	test "$(cat file)" = conflict &&
+	test "$(git rev-parse third)" = "$(git rev-parse second)"
+'
+
 test_expect_success '--rebase' '
 	git branch to-rebase &&
 	echo modified again > file &&

-- 
2.1.4
Johannes Schindelin· May 18, 2015, 15:22 UTC · re: Paul Tan · lore

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

Hi Paul,
On 2015-05-18 15:32, Paul Tan wrote:
Show 12 quoted lines
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 3bc0594..3a53a5e 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" &&
If you follow my argument in 2/8, this line, and...
> [...]
> +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" &&
... this line should be dropped, too.
This comment concludes my review of v4 of this patch series. Well done!

Ciao, Dscho

Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

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

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>
---
* Removed use of "verbose".
 t/t5520-pull.sh | 9 +++++++++
 1 file changed, 9 insertions(+)
Show changes to t/t5520-pull.sh +9 −0
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 3a53a5e..f991439 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -218,6 +218,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>err &&
+	test "$(git rev-parse HEAD)" = "$(git rev-parse before-rebase)" &&
+	test_i18ngrep "Cannot rebase onto multiple branches" err &&
+	test modified = "$(git show HEAD:file)"
+'
+
 test_expect_success 'pull.rebase' '
 	git reset --hard before-rebase &&
 	test_config pull.rebase true &&

-- 
2.1.4
Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

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

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>
---
* Removed use of "verbose".
 t/t5520-pull.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
Show changes to t/t5520-pull.sh +15 −0
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index f991439..4d649a5 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 &&
+		test "$(git ls-files)" = staged-file &&
+		test_must_fail git pull --rebase .. master 2>../err &&
+		test "$(git ls-files)" = staged-file &&
+		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
Stefan Beller· May 18, 2015, 18:00 UTC · re: Paul Tan · lore

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

On Mon, May 18, 2015 at 6:32 AM, Paul Tan <pyokagan@gmail.com> wrote:
Show 39 quoted lines
> 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>
> ---
>
> * Removed use of "verbose".
>
>
>  t/t5520-pull.sh | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index f991439..4d649a5 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 &&
> +               test "$(git ls-files)" = staged-file &&
> +               test_must_fail git pull --rebase .. master 2>../err &&
> +               test "$(git ls-files)" = staged-file &&
> +               test "$(git show :staged-file)" = staged-file
> +       ) &&
> +       test_i18ngrep "unborn branch with changes added to the index" err

So when seeing this line outside the parenthesis section, I immediately thought there must be a reason you put it outside. The reason is not obvious to me though. So I'd suggest to move the test_i18ngrep inside the section above.

Show 8 quoted lines
> +'
> +
>  test_expect_success 'setup for detecting upstreamed changes' '
>         mkdir src &&
>         (cd src &&
> --
> 2.1.4
>
Paul Tan· May 21, 2015, 08:51 UTC · re: Stefan Beller · lore

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

Hi,
On Tue, May 19, 2015 at 2:00 AM, Stefan Beller <sbeller@google.com> wrote:
Show 27 quoted lines
> On Mon, May 18, 2015 at 6:32 AM, Paul Tan <pyokagan@gmail.com> wrote:
>> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
>> index f991439..4d649a5 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 &&
>> +               test "$(git ls-files)" = staged-file &&
>> +               test_must_fail git pull --rebase .. master 2>../err &&
>> +               test "$(git ls-files)" = staged-file &&
>> +               test "$(git show :staged-file)" = staged-file
>> +       ) &&
>> +       test_i18ngrep "unborn branch with changes added to the index" err
>
> So when seeing this line outside the parenthesis section, I immediately thought
> there must be a reason you put it outside. The reason is not obvious
> to me though.
> So I'd suggest to move the test_i18ngrep inside the section above.
Right. Fixed.

Regards, Paul

Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

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

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>
---
* No changes
 t/t5521-pull-options.sh | 13 +++++++++++++
 1 file changed, 13 insertions(+)
Show changes to t/t5521-pull-options.sh +13 −0
diff --git a/t/t5521-pull-options.sh b/t/t5521-pull-options.sh
index 453aba5..56e7377 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
Paul Tan· May 18, 2015, 13:32 UTC · re: Paul Tan · lore

[PATCH v4 8/8] t5520: check reflog action in fast-forward merge

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>
---
* Removed use of "verbose".
 t/t5520-pull.sh | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)
Show changes to t/t5520-pull.sh +10 −3
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index 4d649a5..62dbfb5 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
Johannes Schindelin· May 18, 2015, 15:20 UTC · re: Paul Tan · lore

Re: [PATCH v4 8/8] t5520: check reflog action in fast-forward merge

Hi Paul,
On 2015-05-18 15:32, Paul Tan wrote:
Show 8 quoted lines
> @@ -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 &&
Actually, let's use "s/^[0-9a-f]*/OBJID/" instead: you only want to replace the first few characters.
Show 9 quoted lines
> @@ -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 &&
Same here.

Ciao, Dscho

Paul Tan· May 21, 2015, 08:07 UTC · re: Johannes Schindelin · lore

Re: [PATCH v4 8/8] t5520: check reflog action in fast-forward merge

Hi,

On Mon, May 18, 2015 at 11:20 PM, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:

Show 14 quoted lines
> Hi Paul,
>
> On 2015-05-18 15:32, Paul Tan wrote:
>
>> @@ -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 &&
>
> Actually, let's use "s/^[0-9a-f]*/OBJID/" instead: you only want to replace the first few characters.

Did you mean "s/^$_x05[0-9a-f]*/OBJID/"? (with "$_x05" expanding to '[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' from test-lib.sh). If not, then it would match even if there was no SHA1 hash.

But yes, without the "^" there will very likely be false positives. Thanks for catching.

Show 11 quoted lines
>> @@ -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 &&
>
> Same here.

Regards, Paul

Junio C Hamano· May 21, 2015, 17:29 UTC · re: Paul Tan · lore

Re: [PATCH v4 8/8] t5520: check reflog action in fast-forward merge

Paul Tan <pyokagan@gmail.com> writes:
Show 13 quoted lines
> On Mon, May 18, 2015 at 11:20 PM, Johannes Schindelin
> ...
>>> +     sed "s/$_x05[0-9a-f]*/OBJID/g" reflog.actual >reflog.fuzzy &&
>>
>> Actually, let's use "s/^[0-9a-f]*/OBJID/" instead: you only want to
>> replace the first few characters.
>
> Did you mean "s/^$_x05[0-9a-f]*/OBJID/"? (with "$_x05" expanding to
> '[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]' from test-lib.sh). If not,
> then it would match even if there was no SHA1 hash.
>
> But yes, without the "^" there will very likely be false positives.
> Thanks for catching.

I think the suggestion was more about "do we guarantee that there would always be at least five?" It might happen to be the case with our current default, but these tests do not fundamentally rely on that default staying the same. s/^[0-9a-f][0-9a-f]*/OBJECTNAME/g is probably the right balance between cautiousness (you do not want to match an empty string) and future-proofing (you do not want to rely on us having at least five).

Thanks.

← back to recent threads