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

[PATCH v4 0/8] 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 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

[PATCH v4 1/8] t5520: prevent field splitting in content 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:

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

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

Re: [PATCH v4 1/8] t5520: prevent field splitting in content 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:
>
> 	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.

[PATCH v4 2/8] 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>
---

* Removed use of "verbose".

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

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

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

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 15:32, Paul Tan wrote:

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

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

[Junio C Hamano]

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.

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

[Jeff King]

On Mon, May 18, 2015 at 10:46:50AM -0700, Junio C Hamano wrote:

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

> 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

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

[Johannes Schindelin]

Hi Peff,

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

Ciao,
Dscho

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

[Jeff King]

On Tue, May 19, 2015 at 03:29:23PM +0200, Johannes Schindelin wrote:

> 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

[PATCH v4 3/8] 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>
---

* 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(+)

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

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

[Johannes Schindelin]

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.

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

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

[Paul Tan]

Hi,

On Mon, May 18, 2015 at 11:13 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> 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.

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

[PATCH v4 4/8] 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>
---
* Removed use of "verbose".

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

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

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

[Johannes Schindelin]

Hi Paul,

On 2015-05-18 15:32, Paul Tan wrote:

> 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

[PATCH v4 5/8] 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>
---
* Removed use of "verbose".

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

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

[PATCH v4 6/8] 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>
---

* 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
+'
+
 test_expect_success 'setup for detecting upstreamed changes' '
 	mkdir src &&
 	(cd src &&
-- 
2.1.4

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

[Stefan Beller]

On Mon, May 18, 2015 at 6:32 AM, Paul Tan <pyokagan@gmail.com> wrote:
> 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.

> +'
> +
>  test_expect_success 'setup for detecting upstreamed changes' '
>         mkdir src &&
>         (cd src &&
> --
> 2.1.4
>

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

[Paul Tan]

Hi,

On Tue, May 19, 2015 at 2:00 AM, Stefan Beller <sbeller@google.com> wrote:
> 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

[PATCH v4 7/8] 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>
---
* No changes

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

[PATCH v4 8/8] 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>
---
* Removed use of "verbose".

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

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

[Johannes Schindelin]

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.

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

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

[Paul Tan]

Hi,

On Mon, May 18, 2015 at 11:20 PM, Johannes Schindelin
<johannes.schindelin@gmx.de> wrote:
> 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.

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

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

[Junio C Hamano]

Paul Tan <pyokagan@gmail.com> writes:

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