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

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.