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

From: Luke Diamand <luke@diamand.org>
Date: Apr 22, 2015, 20:47 UTC
Message-ID: <553808EE.8020404@diamand.org>
In-Reply-To: <xmqqegncf5ps.fsf@gitster.dls.corp.google.com>

On 22/04/15 18:11, Junio C Hamano wrote:

Show 14 quoted lines

> 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