v3, 2 partst9801: check git-p4's branch detection with client spec enabled

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(-)

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(+)

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(-)

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

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

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

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

On April 22, 2015 9:47:42 PM GMT+01:00, Luke Diamand <luke@diamand.org> 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.

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

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