[PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[Sam Hocevar]

When submitting from a repository that was cloned using a client spec,
use the full list of paths when ruling out files that are outside the
view.  This fixes a bug where only files pertaining to the first path
would be included in the p4 submit.

Signed-off-by: Sam Hocevar <sam@hocevar.net>
---
 git-p4.py | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index a79b6d8..210f100 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1253,6 +1253,8 @@ class P4Submit(Command, P4UserMap):
            Remove lines in the Files section that show changes to files
            outside the depot path we're committing into."""
 
+        [upstream, settings] = findUpstreamBranchPoint()
+
         template = ""
         inFilesSection = False
         for line in p4_read_pipe_lines(['change', '-o']):
@@ -1265,8 +1267,13 @@ class P4Submit(Command, P4UserMap):
                     lastTab = path.rfind("\t")
                     if lastTab != -1:
                         path = path[:lastTab]
-                        if not p4PathStartsWith(path, self.depotPath):
-                            continue
+                        if settings.has_key('depot-paths'):
+                            if not [p for p in settings['depot-paths']
+                                    if p4PathStartsWith(path, p)]:
+                                continue
+                        else:
+                            if not p4PathStartsWith(path, self.depotPath):
+                                continue
                 else:
                     inFilesSection = False
             else:
-- 
2.6.2

Re: [PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[Lars Schneider]

Thanks for the patch! Do you see a way to demonstrate the bug in a test case similar to t9821 [1]?

Cheers,
Lars

[1] https://github.com/git/git/blob/master/t/t9821-git-p4-path-variations.sh

> On 05 Dec 2015, at 12:22, Sam Hocevar <sam@hocevar.net> wrote:
> 
> When submitting from a repository that was cloned using a client spec,
> use the full list of paths when ruling out files that are outside the
> view.  This fixes a bug where only files pertaining to the first path
> would be included in the p4 submit.
> 
> Signed-off-by: Sam Hocevar <sam@hocevar.net>
> ---
> git-p4.py | 11 +++++++++--
> 1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index a79b6d8..210f100 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -1253,6 +1253,8 @@ class P4Submit(Command, P4UserMap):
>            Remove lines in the Files section that show changes to files
>            outside the depot path we're committing into."""
> 
> +        [upstream, settings] = findUpstreamBranchPoint()
> +
>         template = ""
>         inFilesSection = False
>         for line in p4_read_pipe_lines(['change', '-o']):
> @@ -1265,8 +1267,13 @@ class P4Submit(Command, P4UserMap):
>                     lastTab = path.rfind("\t")
>                     if lastTab != -1:
>                         path = path[:lastTab]
> -                        if not p4PathStartsWith(path, self.depotPath):
> -                            continue
> +                        if settings.has_key('depot-paths'):
> +                            if not [p for p in settings['depot-paths']
> +                                    if p4PathStartsWith(path, p)]:
> +                                continue
> +                        else:
> +                            if not p4PathStartsWith(path, self.depotPath):
> +                                continue
>                 else:
>                     inFilesSection = False
>             else:
> -- 
> 2.6.2

Re: [PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[Sam Hocevar]

On Sun, Dec 06, 2015, Lars Schneider wrote:
> Thanks for the patch! Do you see a way to demonstrate the bug in a test case similar to t9821 [1]?

   Not yet, I'm afraid. It's proving trickier than expected because for
now I can only reproduce the bug when the view uses multiples depots
(solution #2 on http://answers.perforce.com/articles/KB/2437), and
unfortunately the test case system in Git was designed for a single
depot.

   Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to
support multiple depots be acceptable and/or welcome? I prefer to ask
before I dig into the task.

Regards,
-- 
Sam.

Re: [PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[Lars Schneider]

On 07 Dec 2015, at 19:51, Sam Hocevar <sam@hocevar.net> wrote:

> On Sun, Dec 06, 2015, Lars Schneider wrote:
>> Thanks for the patch! Do you see a way to demonstrate the bug in a test case similar to t9821 [1]?
> 
>   Not yet, I'm afraid. It's proving trickier than expected because for
> now I can only reproduce the bug when the view uses multiples depots
> (solution #2 on http://answers.perforce.com/articles/KB/2437), and
> unfortunately the test case system in Git was designed for a single
> depot.
> 
>   Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to
> support multiple depots be acceptable and/or welcome? I prefer to ask
> before I dig into the task.

Can you outline your idea a bit? Are you aware of the following way to define client specs: [1] ? Would that help?
I haven't used multiple depots, yet. Therefore please bare with me :-)

Thanks,
Lars

[1] https://github.com/git/git/blob/362d2fc2f8ab9ee22072f76fb36ec16918511944/t/t9821-git-p4-path-variations.sh#L109-L111

Re: [PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[Sam Hocevar]

On Tue, Dec 08, 2015, Lars Schneider wrote:

> >   Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to
> > support multiple depots be acceptable and/or welcome? I prefer to ask
> > before I dig into the task.
> 
> Can you outline your idea a bit? Are you aware of the following way to define client specs: [1] ? Would that help?

   That's the idea, but the bug occurs when the client view looks like this:

     //depot/... //client/dir1/...
     //depot2/... //client/dir2/...

   And is then cloned with (it is not legal in Perforce to specify //...
directly to grab both depots at once):

     git p4 clone --use-client-spec //depot/... //depot2/...

   Then when a file is modified in dir2/, git p4 submit does not elect it
for the changelist. A file in dir1/ will work fine.

   Unfortunately the current test suite assumes everything is under
//depot/ so in order to write a test for this situation there are a few
things to change in lib-git-p4.sh.

Regards,
-- 
Sam.

Re: [PATCH 1/2] git-p4: support multiple depot paths in p4 submit

[Luke Diamand]

On 8 December 2015 at 11:41, Sam Hocevar <sam@hocevar.net> wrote:
> On Tue, Dec 08, 2015, Lars Schneider wrote:
>
>> >   Would a refactor of lib-git-p4.sh (and probably all git-p4 tests) to
>> > support multiple depots be acceptable and/or welcome? I prefer to ask
>> > before I dig into the task.
>>
>> Can you outline your idea a bit? Are you aware of the following way to define client specs: [1] ? Would that help?
>
>    That's the idea, but the bug occurs when the client view looks like this:
>
>      //depot/... //client/dir1/...
>      //depot2/... //client/dir2/...
>
>    And is then cloned with (it is not legal in Perforce to specify //...
> directly to grab both depots at once):
>
>      git p4 clone --use-client-spec //depot/... //depot2/...
>
>    Then when a file is modified in dir2/, git p4 submit does not elect it
> for the changelist. A file in dir1/ will work fine.
>
>    Unfortunately the current test suite assumes everything is under
> //depot/ so in order to write a test for this situation there are a few
> things to change in lib-git-p4.sh.

I think the existing structure ought to mostly work, but it might need
a bit of tweaking.

You would need to create a new depot, but you can do that in your test script.

And you would need a client spec that pointed at this depot, but again
you can do that in your script with the client_view shell function.

I've not tried it myself though, so maybe it's harder than that.

Luke


>
> Regards,
> --
> Sam.