Re: [PATCH 0/2] git-p4: Improve client path detection
Hi Junio,
Junio C Hamano gits...@pobox.com wrote on Sun, 12 Apr 2015 20:40:58 -0700
Vitor Antunes vitor@gmail.com writes:
Luke Diamand l...@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_!
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-p4: Improve client path detection
Vitor Antunes vitor@gmail.com writes: Luke Diamand l...@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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-p4: Improve client path detection
Luke Diamand l...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/2] git-p4: Improve client path detection
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(-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
