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

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

[Vitor Antunes]

The updates introduced in the third revision of these two patches consist only
on updates to the commit messages to better clarify what they implement.

Vitor Antunes (2):
  t9801: check git-p4's branch detection with client spec enabled
  git-p4: improve client path detection when branches are used

 git-p4.py                |   13 ++++--
 t/t9801-git-p4-branch.sh |  106 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 115 insertions(+), 4 deletions(-)

-- 
1.7.10.4

[PATCH V3 1/2] t9801: check git-p4's branch detection with client spec enabled

[Vitor Antunes]

Add failing scenario when branch detection (--detect-branches) is
enabled together with use client spec (--use-client-spec). In this
specific scenario git-p4 will break when the Perforce client view
removes part of the depot path, as in the following example:

  //depot/branch1/base/... //client/branch1/...

The test case also includes an extra sub-file mapping to enforce
robustness check of git-p4's client view support:

  //depot/branch1/base/dir/sub_file1 //client/branch1/sub_file1

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

diff --git a/t/t9801-git-p4-branch.sh b/t/t9801-git-p4-branch.sh
index 2bf142d..36a7f51 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -504,6 +504,112 @@ 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
+#    //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 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 &&
+		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" &&
+		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 &&
+		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 &&
+		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 V3 2/2] git-p4: improve client path detection when branches are used

[Vitor Antunes]

Perforce allows client side file/directory remapping through
the use of the client view definition that is part of the
user's client spec.

To support this functionality while branch detection is
enabled it is important to determine the branch location in
the workspace such that the correct files are patched before
Perforce submission. Perforce provides a command that
facilitates this process: p4 where.

This patch does two things to fix improve file location
detection when git-p4 has branch detection and use of client
spec enabled:

 1. Enable usage of "p4 where" when Perforce branches exist
    in the git repository, even when client specification is
    used. This makes use of the already existing function
    p4Where.

 2. Allow identifying partial matches of the branch's depot
    path while processing the output of "p4 where". For
    robustness, paths will only match if ending in "/...".

Signed-off-by: Vitor Antunes <vitor.hda@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-p4.py                |   13 +++++++++----
 t/t9801-git-p4-branch.sh |    2 +-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 549022e..34e4fdd 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -502,12 +502,14 @@ 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:
+            # 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:
@@ -1627,7 +1629,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 36a7f51..0aafd03 100755
--- a/t/t9801-git-p4-branch.sh
+++ b/t/t9801-git-p4-branch.sh
@@ -593,7 +593,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

Re: [PATCH V3 0/2] git-p4: improve client path detection when branches are used

[Junio C Hamano]

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

> The updates introduced in the third revision of these two patches consist only
> on updates to the commit messages to better clarify what they implement.
>
> Vitor Antunes (2):
>   t9801: check git-p4's branch detection with client spec enabled
>   git-p4: improve client path detection when branches are used
>
>  git-p4.py                |   13 ++++--
>  t/t9801-git-p4-branch.sh |  106 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 115 insertions(+), 4 deletions(-)

Thanks; will re-queue.  Luke, could you comment?

Re: [PATCH V3 0/2] git-p4: improve client path detection when branches are used

[Luke Diamand]

On 22/04/15 18:11, Junio C Hamano wrote:
> Vitor Antunes <vitor.hda@gmail.com> writes:
>
>> The updates introduced in the third revision of these two patches consist only
>> on updates to the commit messages to better clarify what they implement.
>>
>> Vitor Antunes (2):
>>    t9801: check git-p4's branch detection with client spec enabled
>>    git-p4: improve client path detection when branches are used
>>
>>   git-p4.py                |   13 ++++--
>>   t/t9801-git-p4-branch.sh |  106 ++++++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 115 insertions(+), 4 deletions(-)
>
> Thanks; will re-queue.  Luke, could you comment?

First off: kudos to Vitor for daring to enter this particular dragon's 
den. The combination of branch-detection and use-client-spec isn't so 
bad, but throwing in the handling of excluding bits of the tree via the 
P4 client spec (like, who would even do that?) makes it into a real mind 
twister!

I've held off commenting as I don't feel I know the branch detection 
code as well as I would like. The change though seems a lot more robust 
now that the search is anchored. Having a test case is always good!

However, playing around with this (incredibly complex and obscure) 
scenario, I'm not yet sure about it.

I created a depot that had //depot/main and //depot/branch, and a branch 
mapping between the two. I cloned that in git using --use-client-spec 
and --branch-detect, and all was well.

I then modified my client spec to exclude //depot/main/excluded, and 
then started adding files in git to the 'excluded' directory. When I 
submit them, I get:

$ echo hello >excluded/f1.c
$ echo hello >f2.c
$ git add excluded/f1.c f2.c
$ git commit -m 'Partially excluded'
$ git-p4.py submit
DEBUG: self.useClientSpec = True
Perforce checkout for depot path //depot/main/ located at 
/home/lgd/p4-hacking/cli/main/
Synchronizing p4 checkout...
... - file(s) up-to-date.
Applying 51f187b Excluded added from git
excluded/c - file(s) not in client view.
excluded/c - file(s) not opened on this client.
Could not determine file type for excluded/c (result: '')

When I reverted this change, it failed differently, and appeared to be 
extremely confused in the way that I think Vitor originally describes, 
getting hopelessly baffled by the client spec layout.

It's entirely possibly I've messed up my manual testing though. I need 
to go and have a very strong cup of tea before I can look at this again.

Thanks!
Luke

Re: [PATCH V3 0/2] git-p4: improve client path detection when branches are used

[Vitor Antunes]

On April 22, 2015 9:47:42 PM GMT+01:00, Luke Diamand <luke@diamand.org> wrote:
>On 22/04/15 18:11, Junio C Hamano wrote:
>> Vitor Antunes <vitor.hda@gmail.com> writes:
>>
>>> The updates introduced in the third revision of these two patches
>consist only
>>> on updates to the commit messages to better clarify what they
>implement.
>>>
>>> Vitor Antunes (2):
>>>    t9801: check git-p4's branch detection with client spec enabled
>>>    git-p4: improve client path detection when branches are used
>>>
>>>   git-p4.py                |   13 ++++--
>>>   t/t9801-git-p4-branch.sh |  106
>++++++++++++++++++++++++++++++++++++++++++++++
>>>   2 files changed, 115 insertions(+), 4 deletions(-)
>>
>> Thanks; will re-queue.  Luke, could you comment?
>
>First off: kudos to Vitor for daring to enter this particular dragon's 
>den. The combination of branch-detection and use-client-spec isn't so 
>bad, but throwing in the handling of excluding bits of the tree via the
>
>P4 client spec (like, who would even do that?) makes it into a real
>mind 
>twister!
>
>I've held off commenting as I don't feel I know the branch detection 
>code as well as I would like. The change though seems a lot more robust
>
>now that the search is anchored. Having a test case is always good!
>
>However, playing around with this (incredibly complex and obscure) 
>scenario, I'm not yet sure about it.
>
>I created a depot that had //depot/main and //depot/branch, and a
>branch 
>mapping between the two. I cloned that in git using --use-client-spec 
>and --branch-detect, and all was well.
>
>I then modified my client spec to exclude //depot/main/excluded, and 
>then started adding files in git to the 'excluded' directory. When I 
>submit them, I get:
>
>$ echo hello >excluded/f1.c
>$ echo hello >f2.c
>$ git add excluded/f1.c f2.c
>$ git commit -m 'Partially excluded'
>$ git-p4.py submit
>DEBUG: self.useClientSpec = True
>Perforce checkout for depot path //depot/main/ located at 
>/home/lgd/p4-hacking/cli/main/
>Synchronizing p4 checkout...
>... - file(s) up-to-date.
>Applying 51f187b Excluded added from git
>excluded/c - file(s) not in client view.
>excluded/c - file(s) not opened on this client.
>Could not determine file type for excluded/c (result: '')
>
>When I reverted this change, it failed differently, and appeared to be 
>extremely confused in the way that I think Vitor originally describes, 
>getting hopelessly baffled by the client spec layout.
>
>It's entirely possibly I've messed up my manual testing though. I need 
>to go and have a very strong cup of tea before I can look at this
>again.
>
>Thanks!
>Luke

That was a good combination to test. In fact, I am using such
a client spec at my work place to exclude the import from
Perforce of a folder that only contains binary files, but I never
even considered to add files to that folder from git!
Although I do agree that git-p4 should be able to exit sanely
in this scenario, it is also my opinion that this is a different
scenario from the one I'm tryig to fix in this set of patches and
that it should not be enough to stop this merge.

I will take this scenario into consideration, create a new test
case and finally fix git-p4 to exit sanely in such a scenario.
This new test will also be able to show that folder exclusion
is working perfectly during import, which is important to
guarantee that that functionality is not broken in future.

BTW, no kudos is necessary because I've already walked this
path before :) I've introduced branchList and improved how
git-p4 looks for the original changelist used to create the new
branch in Perforce side. If I remember correctly, many of the
test cases in 9801 file were also created by me before Pete
started splitting the git-p4 test file into topics.

Re: [PATCH V3 0/2] git-p4: improve client path detection when branches are used

[Luke Diamand]

On 23 April 2015 at 09:37, Vitor Antunes <vitor.hda@gmail.com> wrote:

> That was a good combination to test. In fact, I am using such
> a client spec at my work place to exclude the import from
> Perforce of a folder that only contains binary files, but I never
> even considered to add files to that folder from git!
> Although I do agree that git-p4 should be able to exit sanely
> in this scenario, it is also my opinion that this is a different
> scenario from the one I'm tryig to fix in this set of patches and
> that it should not be enough to stop this merge.
>
> I will take this scenario into consideration, create a new test
> case and finally fix git-p4 to exit sanely in such a scenario.
> This new test will also be able to show that folder exclusion
> is working perfectly during import, which is important to
> guarantee that that functionality is not broken in future.
>
> BTW, no kudos is necessary because I've already walked this
> path before :) I've introduced branchList and improved how
> git-p4 looks for the original changelist used to create the new
> branch in Perforce side. If I remember correctly, many of the
> test cases in 9801 file were also created by me before Pete
> started splitting the git-p4 test file into topics.
>

Yes, I think you're right that this is a different, albeit related
problem. It was broken before, and it's still broken. So I'm happy
with this change - Ack.

Teaching git-p4 to handle this corner case would be a good thing to do
as a separate followup if you're OK to do that.

Thanks!
Luke