Re: [PATCH V3 0/2] git-p4: improve client path detection when branches are used
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
Re: [PATCH V3 0/2] git-p4: improve client path detection when branches are used
On 23 April 2015 at 09:37, Vitor Antunes vitor@gmail.com 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. 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 -- 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 V3 0/2] git-p4: improve client path detection when branches are used
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? -- 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 V3 0/2] git-p4: improve client path detection when branches are used
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 -- 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
