[PATCH 0/2] git-p4: Improve client path detection

[PATCH 0/2] git-p4: Improve client path detection

[Vitor Antunes]

I'm adding a test case for a scenario I was confronted with when using branch
detection and a client view specification. It is possible that the implemented
fix may not cover all possible scenarios, but there is no regression in the
available tests.

Vitor Antunes (2):
  git-p4: Check branch detection and client view together
  git-p4: Improve client path detection when branches are used

 git-p4.py                |   11 ++++--
 t/t9801-git-p4-branch.sh |   98 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 105 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH 1/2] git-p4: Check branch detection and client view together

[Vitor Antunes]

Add failing scenario where using branch detection and a client view will break
git p4 submit functionality.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 t/t9801-git-p4-branch.sh |   98 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 98 insertions(+)

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 2bf142d..2f0361a 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -504,6 +504,104 @@ test_expect_success 'use-client-spec detect-branches skips files in branches' '
 	)
 '
 
+test_expect_success 'restart p4d' '
+	kill_p4d &&
+	start_p4d
+'
+
+#
+# 1: //depot/branch1/base/file1
+#    //depot/branch1/base/file2
+# 2: integrate //depot/branch1/base/... -> //depot/branch2/base/...
+# 3: //depot/branch1/base/file3
+# 4: //depot/branch1/base/file2 (edit)
+# 5: integrate //depot/branch1/base/... -> //depot/branch3/base/...
+#
+# Note: the client view remove the "base" folder from the workspace
+test_expect_success 'add simple p4 branches with common base folder on each branch' '
+	(
+		cd "$cli" &&
+		client_view "//depot/branch1/base/... //client/branch1/..." \
+			    "//depot/branch2/base/... //client/branch2/..." \
+			    "//depot/branch3/base/... //client/branch3/..." &&
+		mkdir -p branch1 &&
+		cd branch1 &&
+		echo file1 >file1 &&
+		echo file2 >file2 &&
+		p4 add file1 file2 &&
+		p4 submit -d "Create branch1" &&
+		p4 integrate //depot/branch1/base/... //depot/branch2/base/... &&
+		p4 submit -d "Integrate branch2 from branch1" &&
+		echo file3 >file3 &&
+		p4 add file3 &&
+		p4 submit -d "add file3 in branch1" &&
+		p4 open file2 &&
+		echo update >>file2 &&
+		p4 submit -d "update file2 in branch1" &&
+		p4 integrate //depot/branch1/base/... //depot/branch3/base/... &&
+		p4 submit -d "Integrate branch3 from branch1"
+	)
+'
+
+# Configure branches through git-config and clone them.
+# All files are tested to make sure branches were cloned correctly.
+# Finally, make an update to branch1 on P4 side to check if it is imported
+# correctly by git p4.
+# git p4 is expected to use the client view to also not include the common
+# "base" folder in the imported directory structure.
+test_expect_success 'git p4 clone simple branches with base folder on server side' '
+	test_create_repo "$git" &&
+	(
+		cd "$git" &&
+		git config git-p4.branchList branch1:branch2 &&
+		git config --add git-p4.branchList branch1:branch3 &&
+		git p4 clone --dest=. --use-client-spec  --detect-branches //depot@all &&
+		git log --all --graph --decorate --stat &&
+		git reset --hard p4/depot/branch1 &&
+		test -f file1 &&
+		test -f file2 &&
+		test -f file3 &&
+		grep update file2 &&
+		git reset --hard p4/depot/branch2 &&
+		test -f file1 &&
+		test -f file2 &&
+		test ! -f file3 &&
+		! grep update file2 &&
+		git reset --hard p4/depot/branch3 &&
+		test -f file1 &&
+		test -f file2 &&
+		test -f file3 &&
+		grep update file2 &&
+		cd "$cli" &&
+		cd branch1 &&
+		p4 edit file2 &&
+		echo file2_ >>file2 &&
+		p4 submit -d "update file2 in branch1" &&
+		cd "$git" &&
+		git reset --hard p4/depot/branch1 &&
+		git p4 rebase &&
+		grep file2_ file2
+	)
+'
+
+# Now update a file in one of the branches in git and submit to P4
+test_expect_failure 'Update a file in git side and submit to P4 using client view' '
+	test_when_finished cleanup_git &&
+	(
+		cd "$git" &&
+		git reset --hard p4/depot/branch1 &&
+		echo "client spec" >> file1 &&
+		git add -u . &&
+		git commit -m "update file1 in branch1" &&
+		git config git-p4.skipSubmitEdit true &&
+		git p4 submit --verbose &&
+		cd "$cli" &&
+		p4 sync ... &&
+		cd branch1 &&
+		grep "client spec" file1
+	)
+'
+
 test_expect_success 'kill p4d' '
 	kill_p4d
 '
-- 
1.7.10.4

[PATCH 2/2] git-p4: Improve client path detection when branches are used

[Vitor Antunes]

A client view can be used to remap folder locations in the workspace. To support
this when branch detection is enabled it is necessary to get the client path
through "p4 where". This patch does two things to achieve this:

1. Force usage of "p4 where" when P4 branches exist in the git repository.
2. Search for mappings that contain the depot path, instead of requiring an
   exact match.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 git-p4.py                |   11 +++++++----
 t/t9801-git-p4-branch.sh |    2 +-
 2 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 549022e..6954549 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -502,12 +502,12 @@ def p4Cmd(cmd):
 def p4Where(depotPath):
     if not depotPath.endswith("/"):
         depotPath += "/"
-    depotPath = depotPath + "..."
-    outputList = p4CmdList(["where", depotPath])
+    depotPathLong = depotPath + "..."
+    outputList = p4CmdList(["where", depotPathLong])
     output = None
     for entry in outputList:
         if "depotFile" in entry:
-            if entry["depotFile"] == depotPath:
+            if entry["depotFile"].find(depotPath) >= 0:
                 output = entry
                 break
         elif "data" in entry:
@@ -1627,7 +1627,10 @@ class P4Submit(Command, P4UserMap):
         if self.useClientSpec:
             self.clientSpecDirs = getClientSpec()
 
-        if self.useClientSpec:
+        # Check for the existance of P4 branches
+        branchesDetected = (len(p4BranchesInGit().keys()) > 1)
+
+        if self.useClientSpec and not branchesDetected:
             # all files are relative to the client spec
             self.clientPath = getClientRoot()
         else:
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 2f0361a..4fe4e18 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -585,7 +585,7 @@ test_expect_success 'git p4 clone simple branches with base folder on server sid
 '
 
 # Now update a file in one of the branches in git and submit to P4
-test_expect_failure 'Update a file in git side and submit to P4 using client view' '
+test_expect_success 'Update a file in git side and submit to P4 using client view' '
 	test_when_finished cleanup_git &&
 	(
 		cd "$git" &&
-- 
1.7.10.4

[PATCH] t9814: Guarantee only one source exists in git-p4 copy tests

[Vitor Antunes]

* Modify source file (file2) before copying the file.
* Check that only file2 is the source in the output of "p4 filelog".
* Remove all "case" statements and replace them simple tests to check that
  source is "file2".

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

diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 8b9c295..d8fb22d 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEdit true &&
 
+		echo "file8" >> file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 		cp file2 file8 &&
 		git add file8 &&
 		git commit -a -m "Copy file2 to file8" &&
@@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
 		p4 filelog //depot/file8 &&
 		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
 
+		echo "file9" >> file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		cp file2 file9 &&
 		git add file9 &&
 		git commit -a -m "Copy file2 to file9" &&
@@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
 		p4 filelog //depot/file9 &&
 		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
 
+		echo "file10" >> file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		echo "file2" >>file2 &&
 		cp file2 file10 &&
 		git add file2 file10 &&
 		git commit -a -m "Modify and copy file2 to file10" &&
 		git diff-tree -r -C HEAD &&
+		src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
+		test "$src" = file2 &&
 		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" &&
+
+		echo "file11" >> file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 
 		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) &&
-		case "$src" in
-		file2 | file10) : ;; # happy
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopiesHarder true &&
 		git p4 submit &&
 		p4 filelog //depot/file11 &&
-		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
+		p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
+
+		echo "file12" >> file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 
 		cp file2 file12 &&
 		echo "some text" >>file12 &&
@@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		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
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopies $(($level + 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file12 &&
 		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
 
+		echo "file13" >> file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		cp file2 file13 &&
 		echo "different text" >>file13 &&
 		git add file13 &&
@@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		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
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopies $(($level - 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file13 &&
-		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
+		p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
 	)
 '
 
-- 
1.7.10.4

Re: [PATCH] t9814: Guarantee only one source exists in git-p4 copy tests

[Junio C Hamano]

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

> * Modify source file (file2) before copying the file.
> * Check that only file2 is the source in the output of "p4 filelog".
> * Remove all "case" statements and replace them simple tests to check that
>   source is "file2".
>
> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
> ---

I am not a Perfoce user, so I'd like to ask Pete's and Luke's
comments on these changes.

>  t/t9814-git-p4-rename.sh |   46 +++++++++++++++++++++++++++++++---------------
>  1 file changed, 31 insertions(+), 15 deletions(-)
>
> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
> index 8b9c295..d8fb22d 100755
> --- a/t/t9814-git-p4-rename.sh
> +++ b/t/t9814-git-p4-rename.sh
> @@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
>  		cd "$git" &&
>  		git config git-p4.skipSubmitEdit true &&
>  
> +		echo "file8" >> file2 &&

Style: please lose SP between redirection and its target, i.e.

	echo file8 >>file2 &&

The same comment applies to everywhere else.

> +		git commit -a -m "Differentiate file2" &&
> +		git p4 submit &&
>  		cp file2 file8 &&
>  		git add file8 &&
>  		git commit -a -m "Copy file2 to file8" &&
> @@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
>  		p4 filelog //depot/file8 &&
>  		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
>  
> +		echo "file9" >> file2 &&
> +		git commit -a -m "Differentiate file2" &&
> +		git p4 submit &&
> +
>  		cp file2 file9 &&
>  		git add file9 &&
>  		git commit -a -m "Copy file2 to file9" &&
> @@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
>  		p4 filelog //depot/file9 &&
>  		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
>  
> +		echo "file10" >> file2 &&
> +		git commit -a -m "Differentiate file2" &&
> +		git p4 submit &&
> +
>  		echo "file2" >>file2 &&
>  		cp file2 file10 &&
>  		git add file2 file10 &&
>  		git commit -a -m "Modify and copy file2 to file10" &&
>  		git diff-tree -r -C HEAD &&
> +		src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
> +		test "$src" = file2 &&
>  		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" &&
> +
> +		echo "file11" >> file2 &&
> +		git commit -a -m "Differentiate file2" &&
> +		git p4 submit &&
>  
>  		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) &&
> -		case "$src" in
> -		file2 | file10) : ;; # happy
> -		*) false ;; # not
> -		esac &&
> +		test "$src" = file2 &&
>  		git config git-p4.detectCopiesHarder true &&
>  		git p4 submit &&
>  		p4 filelog //depot/file11 &&
> -		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
> +		p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
> +
> +		echo "file12" >> file2 &&
> +		git commit -a -m "Differentiate file2" &&
> +		git p4 submit &&
>  
>  		cp file2 file12 &&
>  		echo "some text" >>file12 &&
> @@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
>  		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>  		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
> -		*) false ;; # not
> -		esac &&
> +		test "$src" = file2 &&
>  		git config git-p4.detectCopies $(($level + 2)) &&
>  		git p4 submit &&
>  		p4 filelog //depot/file12 &&
>  		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
>  
> +		echo "file13" >> file2 &&
> +		git commit -a -m "Differentiate file2" &&
> +		git p4 submit &&
> +
>  		cp file2 file13 &&
>  		echo "different text" >>file13 &&
>  		git add file13 &&
> @@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
>  		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>  		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
> -		*) false ;; # not
> -		esac &&
> +		test "$src" = file2 &&
>  		git config git-p4.detectCopies $(($level - 2)) &&
>  		git p4 submit &&
>  		p4 filelog //depot/file13 &&
> -		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
> +		p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
>  	)
>  '

Re: [PATCH] t9814: Guarantee only one source exists in git-p4 copy tests

[Luke Diamand]

I'm on holiday this week, so I'll not get a chance to look at these
properly until next week.

Luke


On 30 March 2015 at 04:03, Junio C Hamano <gitster@pobox.com> wrote:
> Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> * Modify source file (file2) before copying the file.
>> * Check that only file2 is the source in the output of "p4 filelog".
>> * Remove all "case" statements and replace them simple tests to check that
>>   source is "file2".
>>
>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
>> ---
>
> I am not a Perfoce user, so I'd like to ask Pete's and Luke's
> comments on these changes.
>
>>  t/t9814-git-p4-rename.sh |   46 +++++++++++++++++++++++++++++++---------------
>>  1 file changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
>> index 8b9c295..d8fb22d 100755
>> --- a/t/t9814-git-p4-rename.sh
>> +++ b/t/t9814-git-p4-rename.sh
>> @@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
>>               cd "$git" &&
>>               git config git-p4.skipSubmitEdit true &&
>>
>> +             echo "file8" >> file2 &&
>
> Style: please lose SP between redirection and its target, i.e.
>
>         echo file8 >>file2 &&
>
> The same comment applies to everywhere else.
>
>> +             git commit -a -m "Differentiate file2" &&
>> +             git p4 submit &&
>>               cp file2 file8 &&
>>               git add file8 &&
>>               git commit -a -m "Copy file2 to file8" &&
>> @@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
>>               p4 filelog //depot/file8 &&
>>               p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
>>
>> +             echo "file9" >> file2 &&
>> +             git commit -a -m "Differentiate file2" &&
>> +             git p4 submit &&
>> +
>>               cp file2 file9 &&
>>               git add file9 &&
>>               git commit -a -m "Copy file2 to file9" &&
>> @@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
>>               p4 filelog //depot/file9 &&
>>               p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
>>
>> +             echo "file10" >> file2 &&
>> +             git commit -a -m "Differentiate file2" &&
>> +             git p4 submit &&
>> +
>>               echo "file2" >>file2 &&
>>               cp file2 file10 &&
>>               git add file2 file10 &&
>>               git commit -a -m "Modify and copy file2 to file10" &&
>>               git diff-tree -r -C HEAD &&
>> +             src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
>> +             test "$src" = file2 &&
>>               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" &&
>> +
>> +             echo "file11" >> file2 &&
>> +             git commit -a -m "Differentiate file2" &&
>> +             git p4 submit &&
>>
>>               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) &&
>> -             case "$src" in
>> -             file2 | file10) : ;; # happy
>> -             *) false ;; # not
>> -             esac &&
>> +             test "$src" = file2 &&
>>               git config git-p4.detectCopiesHarder true &&
>>               git p4 submit &&
>>               p4 filelog //depot/file11 &&
>> -             p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
>> +             p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
>> +
>> +             echo "file12" >> file2 &&
>> +             git commit -a -m "Differentiate file2" &&
>> +             git p4 submit &&
>>
>>               cp file2 file12 &&
>>               echo "some text" >>file12 &&
>> @@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
>>               level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>>               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
>> -             *) false ;; # not
>> -             esac &&
>> +             test "$src" = file2 &&
>>               git config git-p4.detectCopies $(($level + 2)) &&
>>               git p4 submit &&
>>               p4 filelog //depot/file12 &&
>>               p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
>>
>> +             echo "file13" >> file2 &&
>> +             git commit -a -m "Differentiate file2" &&
>> +             git p4 submit &&
>> +
>>               cp file2 file13 &&
>>               echo "different text" >>file13 &&
>>               git add file13 &&
>> @@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
>>               level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>>               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
>> -             *) false ;; # not
>> -             esac &&
>> +             test "$src" = file2 &&
>>               git config git-p4.detectCopies $(($level - 2)) &&
>>               git p4 submit &&
>>               p4 filelog //depot/file13 &&
>> -             p4 filelog //depot/file13 | grep -q "branch from //depot/file"
>> +             p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
>>       )
>>  '

[PATCH V2] t9814: Guarantee only one source exists in git-p4 copy tests

[Vitor Antunes]

* Modify source file (file2) before copying the file.
* Check that only file2 is the source in the output of "p4 filelog".
* Remove all "case" statements and replace them with simple tests to check
  that source is "file2".

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

diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 8b9c295..99bb71b 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEdit true &&
 
+		echo "file8" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 		cp file2 file8 &&
 		git add file8 &&
 		git commit -a -m "Copy file2 to file8" &&
@@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
 		p4 filelog //depot/file8 &&
 		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
 
+		echo "file9" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		cp file2 file9 &&
 		git add file9 &&
 		git commit -a -m "Copy file2 to file9" &&
@@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
 		p4 filelog //depot/file9 &&
 		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
 
+		echo "file10" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		echo "file2" >>file2 &&
 		cp file2 file10 &&
 		git add file2 file10 &&
 		git commit -a -m "Modify and copy file2 to file10" &&
 		git diff-tree -r -C HEAD &&
+		src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
+		test "$src" = file2 &&
 		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" &&
+
+		echo "file11" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 
 		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) &&
-		case "$src" in
-		file2 | file10) : ;; # happy
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopiesHarder true &&
 		git p4 submit &&
 		p4 filelog //depot/file11 &&
-		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
+		p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
+
+		echo "file12" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 
 		cp file2 file12 &&
 		echo "some text" >>file12 &&
@@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		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
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopies $(($level + 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file12 &&
 		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
 
+		echo "file13" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		cp file2 file13 &&
 		echo "different text" >>file13 &&
 		git add file13 &&
@@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		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
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopies $(($level - 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file13 &&
-		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
+		p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
 	)
 '
 
-- 
1.7.10.4

Re: [PATCH] t9814: Guarantee only one source exists in git-p4 copy tests

[Luke Diamand]

On 30/03/15 04:03, Junio C Hamano wrote:
> Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> * Modify source file (file2) before copying the file.
>> * Check that only file2 is the source in the output of "p4 filelog".
>> * Remove all "case" statements and replace them simple tests to check that
>>    source is "file2".
>>
>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
>> ---
>
> I am not a Perfoce user, so I'd like to ask Pete's and Luke's
> comments on these changes.

It's much clearer now that the guessing of file source has been cleaned 
up, thanks. Ack.

Luke



>
>>   t/t9814-git-p4-rename.sh |   46 +++++++++++++++++++++++++++++++---------------
>>   1 file changed, 31 insertions(+), 15 deletions(-)
>>
>> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
>> index 8b9c295..d8fb22d 100755
>> --- a/t/t9814-git-p4-rename.sh
>> +++ b/t/t9814-git-p4-rename.sh
>> @@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
>>   		cd "$git" &&
>>   		git config git-p4.skipSubmitEdit true &&
>>
>> +		echo "file8" >> file2 &&
>
> Style: please lose SP between redirection and its target, i.e.
>
> 	echo file8 >>file2 &&
>
> The same comment applies to everywhere else.
>
>> +		git commit -a -m "Differentiate file2" &&
>> +		git p4 submit &&
>>   		cp file2 file8 &&
>>   		git add file8 &&
>>   		git commit -a -m "Copy file2 to file8" &&
>> @@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
>>   		p4 filelog //depot/file8 &&
>>   		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
>>
>> +		echo "file9" >> file2 &&
>> +		git commit -a -m "Differentiate file2" &&
>> +		git p4 submit &&
>> +
>>   		cp file2 file9 &&
>>   		git add file9 &&
>>   		git commit -a -m "Copy file2 to file9" &&
>> @@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
>>   		p4 filelog //depot/file9 &&
>>   		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
>>
>> +		echo "file10" >> file2 &&
>> +		git commit -a -m "Differentiate file2" &&
>> +		git p4 submit &&
>> +
>>   		echo "file2" >>file2 &&
>>   		cp file2 file10 &&
>>   		git add file2 file10 &&
>>   		git commit -a -m "Modify and copy file2 to file10" &&
>>   		git diff-tree -r -C HEAD &&
>> +		src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
>> +		test "$src" = file2 &&
>>   		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" &&
>> +
>> +		echo "file11" >> file2 &&
>> +		git commit -a -m "Differentiate file2" &&
>> +		git p4 submit &&
>>
>>   		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) &&
>> -		case "$src" in
>> -		file2 | file10) : ;; # happy
>> -		*) false ;; # not
>> -		esac &&
>> +		test "$src" = file2 &&
>>   		git config git-p4.detectCopiesHarder true &&
>>   		git p4 submit &&
>>   		p4 filelog //depot/file11 &&
>> -		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
>> +		p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
>> +
>> +		echo "file12" >> file2 &&
>> +		git commit -a -m "Differentiate file2" &&
>> +		git p4 submit &&
>>
>>   		cp file2 file12 &&
>>   		echo "some text" >>file12 &&
>> @@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
>>   		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>>   		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
>> -		*) false ;; # not
>> -		esac &&
>> +		test "$src" = file2 &&
>>   		git config git-p4.detectCopies $(($level + 2)) &&
>>   		git p4 submit &&
>>   		p4 filelog //depot/file12 &&
>>   		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
>>
>> +		echo "file13" >> file2 &&
>> +		git commit -a -m "Differentiate file2" &&
>> +		git p4 submit &&
>> +
>>   		cp file2 file13 &&
>>   		echo "different text" >>file13 &&
>>   		git add file13 &&
>> @@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
>>   		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>>   		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
>> -		*) false ;; # not
>> -		esac &&
>> +		test "$src" = file2 &&
>>   		git config git-p4.detectCopies $(($level - 2)) &&
>>   		git p4 submit &&
>>   		p4 filelog //depot/file13 &&
>> -		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
>> +		p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
>>   	)
>>   '

Re: [PATCH] t9814: Guarantee only one source exists in git-p4 copy tests

[Junio C Hamano]

Luke Diamand <luke@diamand.org> writes:

> On 30/03/15 04:03, Junio C Hamano wrote:
>> Vitor Antunes <vitor.hda@gmail.com> writes:
>>
>>> * Modify source file (file2) before copying the file.
>>> * Check that only file2 is the source in the output of "p4 filelog".
>>> * Remove all "case" statements and replace them simple tests to check that
>>>    source is "file2".
>>>
>>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
>>> ---
>>
>> I am not a Perfoce user, so I'd like to ask Pete's and Luke's
>> comments on these changes.
>
> It's much clearer now that the guessing of file source has been
> cleaned up, thanks. Ack.

Thanks.

Vitor, when resubmitting v2 to fix style nits, please add Luke's
Acked-by just after your Sign-off, perhaps like this:

    t9814: guarantee only one source exists in git-p4 copy tests
    
    By using a tree with multiple identical files and allowing copy
    detection to choose any one of them, the check in the test is
    unnecessarily complex.  We can simplify by:
    
     * Modify source file (file2) before copying the file.
    
     * Check that only file2 is the source in the output of "p4 filelog".
    
     * Remove all "case" statements and replace them simple tests to
       check that source is "file2".
    
    Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
    Acked-by: Luke Diamand <luke@diamand.org>


>>> diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
>>> index 8b9c295..d8fb22d 100755
>>> --- a/t/t9814-git-p4-rename.sh
>>> +++ b/t/t9814-git-p4-rename.sh
>>> @@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
>>>   		cd "$git" &&
>>>   		git config git-p4.skipSubmitEdit true &&
>>>
>>> +		echo "file8" >> file2 &&
>>
>> Style: please lose SP between redirection and its target, i.e.
>>
>> 	echo file8 >>file2 &&
>>
>> The same comment applies to everywhere else.
>>
>>> +		git commit -a -m "Differentiate file2" &&
>>> +		git p4 submit &&
>>>   		cp file2 file8 &&
>>>   		git add file8 &&
>>>   		git commit -a -m "Copy file2 to file8" &&
>>> @@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
>>>   		p4 filelog //depot/file8 &&
>>>   		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
>>>
>>> +		echo "file9" >> file2 &&
>>> +		git commit -a -m "Differentiate file2" &&
>>> +		git p4 submit &&
>>> +
>>>   		cp file2 file9 &&
>>>   		git add file9 &&
>>>   		git commit -a -m "Copy file2 to file9" &&
>>> @@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
>>>   		p4 filelog //depot/file9 &&
>>>   		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
>>>
>>> +		echo "file10" >> file2 &&
>>> +		git commit -a -m "Differentiate file2" &&
>>> +		git p4 submit &&
>>> +
>>>   		echo "file2" >>file2 &&
>>>   		cp file2 file10 &&
>>>   		git add file2 file10 &&
>>>   		git commit -a -m "Modify and copy file2 to file10" &&
>>>   		git diff-tree -r -C HEAD &&
>>> +		src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
>>> +		test "$src" = file2 &&
>>>   		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" &&
>>> +
>>> +		echo "file11" >> file2 &&
>>> +		git commit -a -m "Differentiate file2" &&
>>> +		git p4 submit &&
>>>
>>>   		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) &&
>>> -		case "$src" in
>>> -		file2 | file10) : ;; # happy
>>> -		*) false ;; # not
>>> -		esac &&
>>> +		test "$src" = file2 &&
>>>   		git config git-p4.detectCopiesHarder true &&
>>>   		git p4 submit &&
>>>   		p4 filelog //depot/file11 &&
>>> -		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
>>> +		p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
>>> +
>>> +		echo "file12" >> file2 &&
>>> +		git commit -a -m "Differentiate file2" &&
>>> +		git p4 submit &&
>>>
>>>   		cp file2 file12 &&
>>>   		echo "some text" >>file12 &&
>>> @@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
>>>   		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>>>   		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
>>> -		*) false ;; # not
>>> -		esac &&
>>> +		test "$src" = file2 &&
>>>   		git config git-p4.detectCopies $(($level + 2)) &&
>>>   		git p4 submit &&
>>>   		p4 filelog //depot/file12 &&
>>>   		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
>>>
>>> +		echo "file13" >> file2 &&
>>> +		git commit -a -m "Differentiate file2" &&
>>> +		git p4 submit &&
>>> +
>>>   		cp file2 file13 &&
>>>   		echo "different text" >>file13 &&
>>>   		git add file13 &&
>>> @@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
>>>   		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
>>>   		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
>>> -		*) false ;; # not
>>> -		esac &&
>>> +		test "$src" = file2 &&
>>>   		git config git-p4.detectCopies $(($level - 2)) &&
>>>   		git p4 submit &&
>>>   		p4 filelog //depot/file13 &&
>>> -		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
>>> +		p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
>>>   	)
>>>   '

Re: [PATCH 0/2] git-p4: Improve client path detection

[Luke Diamand]

On 28/03/15 12:28, Vitor Antunes wrote:
> I'm adding a test case for a scenario I was confronted with when using branch
> detection and a client view specification. It is possible that the implemented
> fix may not cover all possible scenarios, but there is no regression in the
> available tests.

Vitor, one thing I wondered about with this part of the change:

-            if entry["depotFile"] == depotPath:
+            if entry["depotFile"].find(depotPath) >= 0:

Does this mean that if 'p4 where' produces multiple lines of output that 
this will get confused, as it's just going to search for an instance of 
depotPath.

The example in the Perforce man page for 'p4 where' would trigger this 
for example:

http://www.perforce.com/perforce/r14.2/manuals/cmdref/p4_where.html

-//a/b/file.txt //client/a/b/file.txt //home/user/root/a/b/file.txt
//a/b/file.txt //client/b/file.txt /home/user/root/b/file.txt

As an experiment, I hacked git-p4 to always use p4Where rather than 
getClientRoot(), which I would have thought ought to work, but while 
most of the tests passed, Pete's client-spec torture tests failed.

Luke


>
> Vitor Antunes (2):
>    git-p4: Check branch detection and client view together
>    git-p4: Improve client path detection when branches are used
>
>   git-p4.py                |   11 ++++--
>   t/t9801-git-p4-branch.sh |   98 ++++++++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 105 insertions(+), 4 deletions(-)
>

Re: [PATCH 0/2] git-p4: Improve client path detection

[Vitor Antunes]

Luke Diamand <luke@diamand.org> wrote on Sun, 05 Apr 2015 20:27:11 +0100
> On 28/03/15 12:28, Vitor Antunes wrote:
> > I'm adding a test case for a scenario I was confronted with when using branch
> > detection and a client view specification. It is possible that the implemented
> > fix may not cover all possible scenarios, but there is no regression in the
> > available tests.
>
> Vitor, one thing I wondered about with this part of the change:
>
> -            if entry["depotFile"] == depotPath:
> +            if entry["depotFile"].find(depotPath) >= 0:
>
> Does this mean that if 'p4 where' produces multiple lines of output that
> this will get confused, as it's just going to search for an instance of
> depotPath.

The reason why I introduced that was because in the test case I implemented (and
which reflects a scenario I am confronted with in my workplace) the branches
have a base directory that is removed in the client view mapping.
As such, we will have a situation where depotPath is //depot/branch1/ while
runninng "p4 where" will result in //depot/branch1/base/. To overcome this I
used find() instead of a direct comparison. Now that I think about that, I could
probably have used the simpler `if depotPath in entry["depotFile"]`...

> The example in the Perforce man page for 'p4 where' would trigger this
> for example:
>
> http://www.perforce.com/perforce/r14.2/manuals/cmdref/p4_where.html
>
> -//a/b/file.txt //client/a/b/file.txt //home/user/root/a/b/file.txt
> //a/b/file.txt //client/b/file.txt /home/user/root/b/file.txt

These are examples where a simple comparison as was implemented would work.

> As an experiment, I hacked git-p4 to always use p4Where rather than
> getClientRoot(), which I would have thought ought to work, but while
> most of the tests passed, Pete's client-spec torture tests failed.

That was exactly my first approach and got to the same conclusion. I would have
investigated it further but since I haven't had much free time to invest in
solving this problem I decided to implement an intermediary solution that would
not introduce any regressions.

Vitor

[PATCH V3] t9814: Guarantee only one source exists in git-p4 copy tests

[Vitor Antunes]

By using a tree with multiple identical files and allowing copy detection to
choose any one of them, the check in the test is unnecessarily complex.  We can
simplify by:

* Modify source file (file2) before copying the file.
* Check that only file2 is the source in the output of "p4 filelog".
* Remove all "case" statements and replace them with simple tests to check
  that source is "file2".

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
Acked-by: Luke Diamand <luke@diamand.org>
---
 t/t9814-git-p4-rename.sh |   46 +++++++++++++++++++++++++++++++---------------
 1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/t/t9814-git-p4-rename.sh b/t/t9814-git-p4-rename.sh
index 8b9c295..99bb71b 100755
--- a/t/t9814-git-p4-rename.sh
+++ b/t/t9814-git-p4-rename.sh
@@ -132,6 +132,9 @@ test_expect_success 'detect copies' '
 		cd "$git" &&
 		git config git-p4.skipSubmitEdit true &&
 
+		echo "file8" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 		cp file2 file8 &&
 		git add file8 &&
 		git commit -a -m "Copy file2 to file8" &&
@@ -140,6 +143,10 @@ test_expect_success 'detect copies' '
 		p4 filelog //depot/file8 &&
 		p4 filelog //depot/file8 | test_must_fail grep -q "branch from" &&
 
+		echo "file9" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		cp file2 file9 &&
 		git add file9 &&
 		git commit -a -m "Copy file2 to file9" &&
@@ -149,28 +156,39 @@ test_expect_success 'detect copies' '
 		p4 filelog //depot/file9 &&
 		p4 filelog //depot/file9 | test_must_fail grep -q "branch from" &&
 
+		echo "file10" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		echo "file2" >>file2 &&
 		cp file2 file10 &&
 		git add file2 file10 &&
 		git commit -a -m "Modify and copy file2 to file10" &&
 		git diff-tree -r -C HEAD &&
+		src=$(git diff-tree -r -C HEAD | sed 1d | sed 2d | cut -f2) &&
+		test "$src" = file2 &&
 		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" &&
+
+		echo "file11" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 
 		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) &&
-		case "$src" in
-		file2 | file10) : ;; # happy
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopiesHarder true &&
 		git p4 submit &&
 		p4 filelog //depot/file11 &&
-		p4 filelog //depot/file11 | grep -q "branch from //depot/file" &&
+		p4 filelog //depot/file11 | grep -q "branch from //depot/file2" &&
+
+		echo "file12" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
 
 		cp file2 file12 &&
 		echo "some text" >>file12 &&
@@ -180,15 +198,16 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		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
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopies $(($level + 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file12 &&
 		p4 filelog //depot/file12 | test_must_fail grep -q "branch from" &&
 
+		echo "file13" >>file2 &&
+		git commit -a -m "Differentiate file2" &&
+		git p4 submit &&
+
 		cp file2 file13 &&
 		echo "different text" >>file13 &&
 		git add file13 &&
@@ -197,14 +216,11 @@ test_expect_success 'detect copies' '
 		level=$(git diff-tree -r -C --find-copies-harder HEAD | sed 1d | cut -f1 | cut -d" " -f5 | sed "s/C0*//") &&
 		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
-		*) false ;; # not
-		esac &&
+		test "$src" = file2 &&
 		git config git-p4.detectCopies $(($level - 2)) &&
 		git p4 submit &&
 		p4 filelog //depot/file13 &&
-		p4 filelog //depot/file13 | grep -q "branch from //depot/file"
+		p4 filelog //depot/file13 | grep -q "branch from //depot/file2"
 	)
 '
 
-- 
1.7.10.4

Re: [PATCH 0/2] git-p4: Improve client path detection

[Junio C Hamano]

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

> Luke Diamand <luke@diamand.org> wrote on Sun, 05 Apr 2015 20:27:11 +0100
>> On 28/03/15 12:28, Vitor Antunes wrote:
>> > I'm adding a test case for a scenario I was confronted with when using branch
>> > detection and a client view specification. It is possible that the implemented
>> > fix may not cover all possible scenarios, but there is no regression in the
>> > available tests.
>>
>> Vitor, one thing I wondered about with this part of the change:
>>
>> -            if entry["depotFile"] == depotPath:
>> +            if entry["depotFile"].find(depotPath) >= 0:
>>
>> Does this mean that if 'p4 where' produces multiple lines of output that
>> this will get confused, as it's just going to search for an instance of
>> depotPath.
>
> The reason why I introduced that was because in the test case I implemented (and
> which reflects a scenario I am confronted with in my workplace) the branches
> have a base directory that is removed in the client view mapping.
> As such, we will have a situation where depotPath is //depot/branch1/ while
> runninng "p4 where" will result in //depot/branch1/base/. To overcome this I
> used find() instead of a direct comparison. Now that I think about that, I could
> probably have used the simpler `if depotPath in entry["depotFile"]`...

Hmph, is this find() under discussion the string.find() that finds a
substring?  You are doing >=0 comparison here, but with your example
that entry["depotFile"] may have "base/" appended to what you
expect, the result of running string.find() must yield "0", i.e. no
extra prefix string, no?  I kind of find it hard to believe that it
is OK to have any extra prefix is fine ...

>> The example in the Perforce man page for 'p4 where' would trigger this
>> for example:
>>
>> http://www.perforce.com/perforce/r14.2/manuals/cmdref/p4_where.html
>>
>> -//a/b/file.txt //client/a/b/file.txt //home/user/root/a/b/file.txt
>> //a/b/file.txt //client/b/file.txt /home/user/root/b/file.txt
>
> These are examples where a simple comparison as was implemented would work.

... so is this "find()" an attempt to catch prefix like "-"?  Even
if it that were the reason why you do not limit the acceptable
return value from find() to zero, it feels a bit too loose to allow
anything if the only thing you want to allow is a single "-" prefix.

Can you explain this a bit better?  I cannot quite tell what is
going on from what was written in the log message.

>> As an experiment, I hacked git-p4 to always use p4Where rather than
>> getClientRoot(), which I would have thought ought to work, but while
>> most of the tests passed, Pete's client-spec torture tests failed.
>
> That was exactly my first approach and got to the same conclusion. I would have
> investigated it further but since I haven't had much free time to invest in
> solving this problem I decided to implement an intermediary solution that would
> not introduce any regressions.

Thanks.

Re: [PATCH 0/2] git-p4: Improve client path detection

[Vitor Antunes]

Hi Junio,

Junio C Hamano <gitster@pobox.com> wrote on Sun, 12 Apr 2015 20:40:58 -0700
> Vitor Antunes <vitor.hda@gmail.com> writes:
>> Luke Diamand <luke@diamand.org> wrote on Sun, 05 Apr 2015 20:27:11 +0100
>>> Vitor, one thing I wondered about with this part of the change:
>>>
>>> -            if entry["depotFile"] == depotPath:
>>> +            if entry["depotFile"].find(depotPath) >= 0:
>>>
>>> Does this mean that if 'p4 where' produces multiple lines of output that
>>> this will get confused, as it's just going to search for an instance of
>>> depotPath.
>>
>> The reason why I introduced that was because in the test case I implemented (and
>> which reflects a scenario I am confronted with in my workplace) the branches
>> have a base directory that is removed in the client view mapping.
>> As such, we will have a situation where depotPath is //depot/branch1/ while
>> runninng "p4 where" will result in //depot/branch1/base/. To overcome this I
>> used find() instead of a direct comparison. Now that I think about that, I could
>> probably have used the simpler `if depotPath in entry["depotFile"]`...
>
> Hmph, is this find() under discussion the string.find() that finds a
> substring?  You are doing >=0 comparison here, but with your example
> that entry["depotFile"] may have "base/" appended to what you
> expect, the result of running string.find() must yield "0", i.e. no
> extra prefix string, no?  I kind of find it hard to believe that it
> is OK to have any extra prefix is fine ...

As usual, you're correct about your assumption. I should in fact be
using "== 0" because what I really want is to guarantee that the path
_starts_ with //depot/branch1.

>>> The example in the Perforce man page for 'p4 where' would trigger this
>>> for example:
>>>
>>> http://www.perforce.com/perforce/r14.2/manuals/cmdref/p4_where.html
>>>
>>> -//a/b/file.txt //client/a/b/file.txt //home/user/root/a/b/file.txt
>>> //a/b/file.txt //client/b/file.txt /home/user/root/b/file.txt
>>
>> These are examples where a simple comparison as was implemented would work.
>
> ... so is this "find()" an attempt to catch prefix like "-"?  Even
> if it that were the reason why you do not limit the acceptable
> return value from find() to zero, it feels a bit too loose to allow
> anything if the only thing you want to allow is a single "-" prefix.

Again, it was just a bad coding from my part.

> Can you explain this a bit better?  I cannot quite tell what is
> going on from what was written in the log message.

I've temporarily modified the script to print out the output of "p4
where", for future reference:

[{'clientFile': '//client/branch1/...',           'code': 'stat',              'depotFile': '//depot/branch1/base/...',           'path': '/path/to/git/t/trash directory.t9801-git-p4-branch/cli/branch1/...'},
 {'clientFile': '//client/branch1/sub_file1',     'code': 'stat', 'unmap': '', 'depotFile': '//depot/branch1/base/sub_file1',     'path': '/path/to/git/t/trash directory.t9801-git-p4-branch/cli/branch1/sub_file1'},
 {'clientFile': '//client/branch1/dir/sub_file1', 'code': 'stat', 'unmap': '', 'depotFile': '//depot/branch1/base/dir/sub_file1', 'path': '/path/to/git/t/trash directory.t9801-git-p4-branch/cli/branch1/dir/sub_file1'},
 {'clientFile': '//client/branch1/sub_file1',     'code': 'stat',              'depotFile': '//depot/branch1/base/dir/sub_file1', 'path': '/path/to/git/t/trash directory.t9801-git-p4-branch/cli/branch1/sub_file1'}]

Note that this is from a modified test case. As you can see, there are
no paths starting with "-", instead there a new attribute called "unmap"
that implements that description.

In the latest version of this update I'm searching for a path starting
with "//depot/branch1" and ending in "/...". This is a much more robust
solution, so I am really grateful for your review.

>>> As an experiment, I hacked git-p4 to always use p4Where rather than
>>> getClientRoot(), which I would have thought ought to work, but while
>>> most of the tests passed, Pete's client-spec torture tests failed.
>>
>> That was exactly my first approach and got to the same conclusion. I would have
>> investigated it further but since I haven't had much free time to invest in
>> solving this problem I decided to implement an intermediary solution that would
>> not introduce any regressions.

Since I'm looking at this more carefully now, I'll also try to see if I
am able to make p4 where work even when not using branch detection.

> Thanks.

No, thank _you_!

[PATCH] git-p4: Improve client path detection when branches are used

[Vitor Antunes]

This patch makes the client path detection more robust by limiting the valid
results from p4 where. The test case is also made more complex, to guarantee
that such client views are supported.

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
---
 git-p4.py                |    4 +++-
 t/t9801-git-p4-branch.sh |   12 ++++++++++--
 2 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 262a95b..28d0d90 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -507,7 +507,9 @@ def p4Where(depotPath):
     output = None
     for entry in outputList:
         if "depotFile" in entry:
-            if entry["depotFile"].find(depotPath) >= 0:
+            # Search for the base client side depot path, as long as it starts with the branch's P4 path.
+            # The base path always ends with "/...".
+            if entry["depotFile"].find(depotPath) == 0 and entry["depotFile"][-4:] == "/...":
                 output = entry
                 break
         elif "data" in entry:
diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 4fe4e18..0aafd03 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -512,23 +512,28 @@ test_expect_success 'restart p4d' '
 #
 # 1: //depot/branch1/base/file1
 #    //depot/branch1/base/file2
+#    //depot/branch1/base/dir/sub_file1
 # 2: integrate //depot/branch1/base/... -> //depot/branch2/base/...
 # 3: //depot/branch1/base/file3
 # 4: //depot/branch1/base/file2 (edit)
 # 5: integrate //depot/branch1/base/... -> //depot/branch3/base/...
 #
-# Note: the client view remove the "base" folder from the workspace
+# Note: the client view removes the "base" folder from the workspace
+#       and moves sub_file1 one level up.
 test_expect_success 'add simple p4 branches with common base folder on each branch' '
 	(
 		cd "$cli" &&
 		client_view "//depot/branch1/base/... //client/branch1/..." \
+			    "//depot/branch1/base/dir/sub_file1 //client/branch1/sub_file1" \
 			    "//depot/branch2/base/... //client/branch2/..." \
 			    "//depot/branch3/base/... //client/branch3/..." &&
 		mkdir -p branch1 &&
 		cd branch1 &&
 		echo file1 >file1 &&
 		echo file2 >file2 &&
-		p4 add file1 file2 &&
+		mkdir dir &&
+		echo sub_file1 >sub_file1 &&
+		p4 add file1 file2 sub_file1 &&
 		p4 submit -d "Create branch1" &&
 		p4 integrate //depot/branch1/base/... //depot/branch2/base/... &&
 		p4 submit -d "Integrate branch2 from branch1" &&
@@ -561,16 +566,19 @@ test_expect_success 'git p4 clone simple branches with base folder on server sid
 		test -f file1 &&
 		test -f file2 &&
 		test -f file3 &&
+		test -f sub_file1 &&
 		grep update file2 &&
 		git reset --hard p4/depot/branch2 &&
 		test -f file1 &&
 		test -f file2 &&
 		test ! -f file3 &&
+		test -f sub_file1 &&
 		! grep update file2 &&
 		git reset --hard p4/depot/branch3 &&
 		test -f file1 &&
 		test -f file2 &&
 		test -f file3 &&
+		test -f sub_file1 &&
 		grep update file2 &&
 		cd "$cli" &&
 		cd branch1 &&
-- 
1.7.10.4

Re: [PATCH] git-p4: Improve client path detection when branches are used

[Junio C Hamano]

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

> This patch makes the client path detection more robust by limiting the valid
> results from p4 where. The test case is also made more complex, to guarantee
> that such client views are supported.
>
> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
> ---

Was this designed to be squashed into the previous 2/2 patch?  I
do not think either 1/2 or 2/2 is in 'next' yet, and if this was
to correct mistakes in the 2/2 that was posted earlier, it would
be nicer to have a replacement patch with corrected log message.

Thanks.


>  git-p4.py                |    4 +++-
>  t/t9801-git-p4-branch.sh |   12 ++++++++++--
>  2 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/git-p4.py b/git-p4.py
> index 262a95b..28d0d90 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -507,7 +507,9 @@ def p4Where(depotPath):
>      output = None
>      for entry in outputList:
>          if "depotFile" in entry:
> -            if entry["depotFile"].find(depotPath) >= 0:
> +            # Search for the base client side depot path, as long as it starts with the branch's P4 path.
> +            # The base path always ends with "/...".
> +            if entry["depotFile"].find(depotPath) == 0 and entry["depotFile"][-4:] == "/...":
>                  output = entry
>                  break
>          elif "data" in entry:
> diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
> index 4fe4e18..0aafd03 100755
> --- a/t/t9801-git-p4-branch.sh
> +++ b/t/t9801-git-p4-branch.sh
> @@ -512,23 +512,28 @@ test_expect_success 'restart p4d' '
>  #
>  # 1: //depot/branch1/base/file1
>  #    //depot/branch1/base/file2
> +#    //depot/branch1/base/dir/sub_file1
>  # 2: integrate //depot/branch1/base/... -> //depot/branch2/base/...
>  # 3: //depot/branch1/base/file3
>  # 4: //depot/branch1/base/file2 (edit)
>  # 5: integrate //depot/branch1/base/... -> //depot/branch3/base/...
>  #
> -# Note: the client view remove the "base" folder from the workspace
> +# Note: the client view removes the "base" folder from the workspace
> +#       and moves sub_file1 one level up.
>  test_expect_success 'add simple p4 branches with common base folder on each branch' '
>  	(
>  		cd "$cli" &&
>  		client_view "//depot/branch1/base/... //client/branch1/..." \
> +			    "//depot/branch1/base/dir/sub_file1 //client/branch1/sub_file1" \
>  			    "//depot/branch2/base/... //client/branch2/..." \
>  			    "//depot/branch3/base/... //client/branch3/..." &&
>  		mkdir -p branch1 &&
>  		cd branch1 &&
>  		echo file1 >file1 &&
>  		echo file2 >file2 &&
> -		p4 add file1 file2 &&
> +		mkdir dir &&
> +		echo sub_file1 >sub_file1 &&
> +		p4 add file1 file2 sub_file1 &&
>  		p4 submit -d "Create branch1" &&
>  		p4 integrate //depot/branch1/base/... //depot/branch2/base/... &&
>  		p4 submit -d "Integrate branch2 from branch1" &&
> @@ -561,16 +566,19 @@ test_expect_success 'git p4 clone simple branches with base folder on server sid
>  		test -f file1 &&
>  		test -f file2 &&
>  		test -f file3 &&
> +		test -f sub_file1 &&
>  		grep update file2 &&
>  		git reset --hard p4/depot/branch2 &&
>  		test -f file1 &&
>  		test -f file2 &&
>  		test ! -f file3 &&
> +		test -f sub_file1 &&
>  		! grep update file2 &&
>  		git reset --hard p4/depot/branch3 &&
>  		test -f file1 &&
>  		test -f file2 &&
>  		test -f file3 &&
> +		test -f sub_file1 &&
>  		grep update file2 &&
>  		cd "$cli" &&
>  		cd branch1 &&

Re: [PATCH] git-p4: Improve client path detection when branches are used

[Vitor Antunes]

Junio C Hamano <gitster@pobox.com> wrote on Sat, 18 Apr 2015 17:58:11 -0700
> Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> This patch makes the client path detection more robust by limiting the valid
>> results from p4 where. The test case is also made more complex, to guarantee
>> that such client views are supported.
>>
>> Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
>> ---
>
>Was this designed to be squashed into the previous 2/2 patch?  I
>do not think either 1/2 or 2/2 is in 'next' yet, and if this was
>to correct mistakes in the 2/2 that was posted earlier, it would
>be nicer to have a replacement patch with corrected log message.
>
>Thanks.

Hope I got everything right this time :)

Thanks for your help,
Vitor

Re: [PATCH] git-p4: Improve client path detection when branches are used

[Junio C Hamano]

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

> Hope I got everything right this time :)
>
> Thanks for your help,

Thanks.