threads / patch / 38922

patch, 2 partsgit-p4: Make rename test case runnable under dash

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

## tl;dr

10 messages between Mar 27, 2015 and Mar 28, 2015. Diffs are folded; open one to read it.

replies: 9people: 2as markdown or json

Vitor Antunes· Mar 27, 2015, 01:04 UTC · lore

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

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
Vitor Antunes· Mar 27, 2015, 01:04 UTC · re: Vitor Antunes · lore
Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9814-git-p4-rename.sh |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to t/t9814-git-p4-rename.sh +4 −4
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
Vitor Antunes· Mar 27, 2015, 01:04 UTC · re: Vitor Antunes · lore

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

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(-)
Show changes to t/t9814-git-p4-rename.sh +2 −2
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
Junio C Hamano· Mar 27, 2015, 22:23 UTC · re: Vitor Antunes · lore

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

Vitor Antunes <vitor.hda@gmail.com> writes:
Show 7 quoted lines
> 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?

Show 25 quoted lines
> ---
>  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 &&
Vitor Antunes· Mar 27, 2015, 23:59 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> wrote:
Show 14 quoted lines
>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.

Vitor Antunes· Mar 28, 2015, 00:36 UTC · re: Vitor Antunes · lore

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

Vitor Antunes <vitor.hda@gmail.com> wrote:
Show 9 quoted lines
>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?
Junio C Hamano· Mar 28, 2015, 16:12 UTC · re: Vitor Antunes · lore

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

Vitor Antunes <vitor.hda@gmail.com> writes:
Show 17 quoted lines
> 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.
Junio C Hamano· Mar 27, 2015, 01:26 UTC · re: Vitor Antunes · lore

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

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?

Junio C Hamano· Mar 27, 2015, 01:45 UTC · re: Junio C Hamano · lore

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

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.
Vitor Antunes· Mar 27, 2015, 01:54 UTC · re: Junio C Hamano · lore

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

On Fri, 27 Mar 2015 at 01:26 Junio C Hamano <gitster@pobox.com> wrote:
Show 7 quoted lines
>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?

← back to recent threads