Vitor Antunes [off-list ref] writes:
Luke Diamand [off-list ref] wrote on Sun, 05 Apr 2015 20:27:11 +0100
quoted
On 28/03/15 12:28, Vitor Antunes wrote:
quoted
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 ...
quoted
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.
quoted
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.
Hi Junio,
Junio C Hamano [off-list ref] wrote on Sun, 12 Apr 2015 20:40:58 -0700
Vitor Antunes [off-list ref] writes:
quoted
Luke Diamand [off-list ref] wrote on Sun, 05 Apr 2015 20:27:11 +0100
quoted
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.
quoted
quoted
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.
quoted
quoted
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_!
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 <redacted>
---
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