On April 22, 2015 9:47:42 PM GMT+01:00, Luke Diamand <l...@diamand.org> wrote: >On 22/04/15 18:11, Junio C Hamano wrote: >> Vitor Antunes <vitor....@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
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. -- 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
