[PATCH 0/2] git-p4: Small updates to test cases

[PATCH 0/2] git-p4: Small updates to test cases

[Vitor Antunes]

This patch set includes two small fixes to the rename test case. The fix to
support dash should be trivial, but in the fix to the copy detection test case
it isn't obvious to me what changed in diff-tree to result in a different file
being detected as the origin of a copy.

Vitor Antunes (2):
  git-p4: Make rename test case runnable under dash
  git-p4: Fix copy detection test

 t/t9814-git-p4-rename.sh |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] git-p4: Make rename test case runnable under dash

[Vitor Antunes]

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9814-git-p4-rename.sh |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 95f4421..24008ff 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -178,9 +178,9 @@ test_expect_success 'detect copies' '
 		test -n "$level" && test "$level" -gt 0 && test "$level" -lt 98 &&
 		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
 		case "$src" in
-		file10 | file11) : ;; # happy
+		file10 | file11) true ;; # happy
 		*) false ;; # not
-		&&
+		esac &&
 		git config git-p4.detectCopies $(($level + 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file12 &&
@@ -195,9 +195,9 @@ test_expect_success 'detect copies' '
 		test -n "$level" && test "$level" -gt 2 && test "$level" -lt 100 &&
 		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
 		case "$src" in
-		file10 | file11 | file12) : ;; # happy
+		file10 | file11 | file12) true ;; # happy
 		*) false ;; # not
-		&&
+		esac &&
 		git config git-p4.detectCopies $(($level - 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file13 &&
-- 
1.7.10.4

[PATCH 2/2] git-p4: Fix copy detection test

[Vitor Antunes]

File file11 is copied from file2 and diff-tree correctly reports this file as
its the source, but the test expression was checking for file10 instead (which
was a file that also originated from file2). It is possible that the diff-tree
algorithm was updated in recent versions, which resulted in this mismatch in
behavior.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9814-git-p4-rename.sh |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 24008ff..018f01d 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -156,14 +156,14 @@ test_expect_success 'detect copies' '
 		git diff-tree -r -C HEAD &&
 		git p4 submit &&
 		p4 filelog //depot/file10 &&
-		p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
+		p4 filelog //depot/file10 | grep -q "branch from //depot/file2" &&
 
 		cp file2 file11 &&
 		git add file11 &&
 		git commit -a -m "Copy file2 to file11" &&
 		git diff-tree -r -C --find-copies-harder HEAD &&
 		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
-		test "$src" = file10 &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopiesHarder true &&
 		git p4 submit &&
 		p4 filelog //depot/file11 &&
-- 
1.7.10.4

Re: [PATCH 0/2] git-p4: Small updates to test cases

[Junio C Hamano]

Vitor Antunes <vitor.hda@gmail.com> writes:

> This patch set includes two small fixes to the rename test case. The fix to
> support dash should be trivial, but in the fix to the copy detection test case
> it isn't obvious to me what changed in diff-tree to result in a different file
> being detected as the origin of a copy.

Thanks.

As to 1/2 the lack of esac is clearly a bug---any self respecting
POSIX shell should have executed it without complaining.  But
changing from ':' to true should not be necessary---after all, the
colon is a more traditional way to spell true to Bourne shells, and
we use it in many places already.  Can you try reverting all the
"colon to true" bits, keeping only the "add missing esac" part, and
run your tests again?

Re: [PATCH 0/2] git-p4: Small updates to test cases

[Junio C Hamano]

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

> As to 1/2 the lack of esac is clearly a bug---any self respecting
> POSIX shell should have executed it without complaining.  But

s/should/shouldn't/; sorry for a noise.

Re: [PATCH 0/2] git-p4: Small updates to test cases

[Vitor Antunes]

On Fri, 27 Mar 2015 at 01:26 Junio C Hamano <gitster@pobox.com> wrote:

>As to 1/2 the lack of esac is clearly a bug---any self respecting
>POSIX shell should have executed it without complaining. But
>changing from ':' to true should not be necessary---after all, the
>colon is a more traditional way to spell true to Bourne shells, and
>we use it in many places already. Can you try reverting all the
>"colon to true" bits, keeping only the "add missing esac" part, and
>run your tests again?

I confirm that it still works with ':' instead of true; could swear I tested
that at the time... Anyway, I'll re-submit this patch with this fixed
tomorrow.

Thanks for taking the time to review the patch.

One more thing: was there any change in way diff-tree detects copies?

Re: [PATCH 2/2] git-p4: Fix copy detection test

[Junio C Hamano]

Vitor Antunes <vitor.hda@gmail.com> writes:

> File file11 is copied from file2 and diff-tree correctly reports this file as
> its the source, but the test expression was checking for file10 instead (which
> was a file that also originated from file2). It is possible that the diff-tree
> algorithm was updated in recent versions, which resulted in this mismatch in
> behavior.
>
> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>

Pete, these tests blame to your 9b6513ac (git p4 test: split up big
t9800 test, 2012-06-27).  I presume that you tested the result of
this splitting, but do you happen to know if we did something to
cause the test to break recently?

> ---
>  t/t9814-git-p4-rename.sh |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index 24008ff..018f01d 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -156,14 +156,14 @@ test_expect_success 'detect copies' '
>  		git diff-tree -r -C HEAD &&
>  		git p4 submit &&
>  		p4 filelog //depot/file10 &&
> -		p4 filelog //depot/file10 | grep -q "branch from //depot/file" &&
> +		p4 filelog //depot/file10 | grep -q "branch from //depot/file2" &&
>  
>  		cp file2 file11 &&
>  		git add file11 &&
>  		git commit -a -m "Copy file2 to file11" &&
>  		git diff-tree -r -C --find-copies-harder HEAD &&
>  		src=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f2) &&
> -		test "$src" = file10 &&
> +		test "$src" = file2 &&
>  		git config git-p4.detectCopiesHarder true &&
>  		git p4 submit &&
>  		p4 filelog //depot/file11 &&

Re: [PATCH 2/2] git-p4: Fix copy detection test

[Vitor Antunes]

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

>Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> File file11 is copied from file2 and diff-tree correctly reports this file as
>> its the source, but the test expression was checking for file10 instead (which
>> was a file that also originated from file2). It is possible that the diff-tree
>> algorithm was updated in recent versions, which resulted in this mismatch in
>> behavior.
>>
>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
>
>Pete, these tests blame to your 9b6513ac (git p4 test: split up big
>t9800 test, 2012-06-27).  I presume that you tested the result of
>this splitting, but do you happen to know if we did something to
>cause the test to break recently?

I also worked on these tests at that time and they were passing before and
after the reorganization. I'll prepare a bisect script and will try to find the
commit that started making this test fail.

Re: [PATCH 2/2] git-p4: Fix copy detection test

[Vitor Antunes]

Vitor Antunes <vitor.hda@gmail.com> wrote:

>Junio C Hamano <gitster@pobox.com> wrote:
>>Pete, these tests blame to your 9b6513ac (git p4 test: split up big
>>t9800 test, 2012-06-27).  I presume that you tested the result of
>>this splitting, but do you happen to know if we did something to
>>cause the test to break recently?
>
>I also worked on these tests at that time and they were passing before and
>after the reorganization. I'll prepare a bisect script and will try to find the
>commit that started making this test fail.

According to bisect, this is the first commit that makes the test fail:

7c85f8acb2282e3ed108c46b59fd5daa78bf17db

Does this make sense to you?

Re: [PATCH 2/2] git-p4: Fix copy detection test

[Junio C Hamano]

Vitor Antunes <vitor.hda@gmail.com> writes:

> Vitor Antunes <vitor.hda@gmail.com> wrote:
>
>>Junio C Hamano <gitster@pobox.com> wrote:
>>>Pete, these tests blame to your 9b6513ac (git p4 test: split up big
>>>t9800 test, 2012-06-27).  I presume that you tested the result of
>>>this splitting, but do you happen to know if we did something to
>>>cause the test to break recently?
>>
>>I also worked on these tests at that time and they were passing before and
>>after the reorganization. I'll prepare a bisect script and will try to find the
>>commit that started making this test fail.
>
> According to bisect, this is the first commit that makes the test fail:
>
> 7c85f8acb2282e3ed108c46b59fd5daa78bf17db
>
> Does this make sense to you?

Yeah, as the blamed commit changes the way the hashtable is used
record and choose the rename source candidates, it is not surprising
if it changes how two or more candidates with the same rename score
are tie-broken.

Thanks.