[PATCH 1/3] git-p4: Fix an obvious typo

[PATCH 1/3] git-p4: Fix an obvious typo

[Tommy Thorn]

The regexp "$," can't match anything. Clearly not intended.

This was introduced in ce6f33c8 which is quite a while ago.

Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>
---
 contrib/fast-import/git-p4 |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index c80a6da..553e237 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -1670,7 +1670,7 @@ class P4Clone(P4Sync):
         depotPath = args[0]
         depotDir = re.sub("(@[^@]*)$", "", depotPath)
         depotDir = re.sub("(#[^#]*)$", "", depotDir)
-        depotDir = re.sub(r"\.\.\.$,", "", depotDir)
+        depotDir = re.sub(r"\.\.\.$", "", depotDir)
         depotDir = re.sub(r"/$", "", depotDir)
         return os.path.split(depotDir)[1]
 
-- 
1.5.4.rc5.17.g22b645

[PATCH 2/3] git-p4: support exclude paths

[Tommy Thorn]

Teach git-p4 about the -/ option which adds depot paths to the exclude
list, used when cloning. The option is chosen such that the natural
Perforce syntax works, eg:

  git p4 clone //branch/path/... -//branch/path/{large,old}/...

Trailing ... on exclude paths are optional.

This is a generalization of a change by Dmitry Kakurin (thanks).

Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>
---
 contrib/fast-import/git-p4 |   26 ++++++++++++++++++++++----
 1 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 553e237..2340876 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -876,18 +876,25 @@ class P4Sync(Command):
         self.keepRepoPath = False
         self.depotPaths = None
         self.p4BranchesInGit = []
+        self.cloneExclude = []
 
         if gitConfig("git-p4.syncFromOrigin") == "false":
             self.syncWithOrigin = False
 
     def extractFilesFromCommit(self, commit):
+        self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
+                             for path in self.cloneExclude]
         files = []
         fnum = 0
         while commit.has_key("depotFile%s" % fnum):
             path =  commit["depotFile%s" % fnum]
 
-            found = [p for p in self.depotPaths
-                     if path.startswith (p)]
+            if [p for p in self.cloneExclude
+                if path.startswith (p)]:
+                found = False
+            else:
+                found = [p for p in self.depotPaths
+                         if path.startswith (p)]
             if not found:
                 fnum = fnum + 1
                 continue
@@ -1658,13 +1665,23 @@ class P4Clone(P4Sync):
         P4Sync.__init__(self)
         self.description = "Creates a new git repository and imports from Perforce into it"
         self.usage = "usage: %prog [options] //depot/path[@revRange]"
-        self.options.append(
+        self.options += [
             optparse.make_option("--destination", dest="cloneDestination",
                                  action='store', default=None,
-                                 help="where to leave result of the clone"))
+                                 help="where to leave result of the clone"),
+            optparse.make_option("-/", dest="cloneExclude",
+                                 action="append", type="string",
+                                 help="exclude depot path")
+        ]
         self.cloneDestination = None
         self.needsGit = False
 
+    # This is required for the "append" cloneExclude action
+    def ensure_value(self, attr, value):
+        if not hasattr(self, attr) or getattr(self, attr) is None:
+            setattr(self, attr, value)
+        return getattr(self, attr)
+
     def defaultDestination(self, args):
         ## TODO: use common prefix of args?
         depotPath = args[0]
@@ -1688,6 +1705,7 @@ class P4Clone(P4Sync):
             self.cloneDestination = depotPaths[-1]
             depotPaths = depotPaths[:-1]
 
+        self.cloneExclude = ["/"+p for p in self.cloneExclude]
         for p in depotPaths:
             if not p.startswith("//"):
                 return False
-- 
1.5.4.rc5.17.g22b645

[PATCH 3/3] git-p4: no longer keep all file contents while cloning

[Tommy Thorn]

- Factor out the pipe creation part of p4CmdList() as p4CmdListPipe()

- Factor readP4Files() into openP4Files() and readP4File() and
  changed P4Sync to use this.

The upshot is that git-p4 now read only one file at a time and
immediately pass it on to fast-import. This massively reduces the
memory requirement of git-p4.

Note, git-p4 still reads in the whole file in memory -- this can and
should be fixed in future.

Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>
---
 contrib/fast-import/git-p4 |  102 ++++++++++++++++++++++++++++++-------------
 1 files changed, 71 insertions(+), 31 deletions(-)

diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
index 2340876..78a5d02 100755
--- a/contrib/fast-import/git-p4
+++ b/contrib/fast-import/git-p4
@@ -144,7 +144,8 @@ def isModeExec(mode):
 def isModeExecChanged(src_mode, dst_mode):
     return isModeExec(src_mode) != isModeExec(dst_mode)
 
-def p4CmdList(cmd, stdin=None, stdin_mode='w+b'):
+# p4CmdListPipe returns a pipe delivering the result of the p4 command
+def p4CmdListPipe(cmd, stdin=None, stdin_mode='w+b'):
     cmd = "p4 -G %s" % cmd
     if verbose:
         sys.stderr.write("Opening pipe: %s\n" % cmd)
@@ -162,7 +163,11 @@ def p4CmdList(cmd, stdin=None, stdin_mode='w+b'):
     p4 = subprocess.Popen(cmd, shell=True,
                           stdin=stdin_file,
                           stdout=subprocess.PIPE)
+    return p4
 
+# p4CmdList returns the stdout result of the p4 command
+def p4CmdList(cmd, stdin=None, stdin_mode='w+b'):
+    p4 = p4CmdListPipe(cmd, stdin, stdin_mode)
     result = []
     try:
         while True:
@@ -950,42 +955,62 @@ class P4Sync(Command):
         return branches
 
     ## Should move this out, doesn't use SELF.
-    def readP4Files(self, files):
+    def openP4Files(self, files):
         files = [f for f in files
                  if f['action'] != 'delete']
 
         if not files:
             return
 
-        filedata = p4CmdList('-x - print',
-                             stdin='\n'.join(['%s#%s' % (f['path'], f['rev'])
-                                              for f in files]),
-                             stdin_mode='w+')
-        if "p4ExitCode" in filedata[0]:
-            die("Problems executing p4. Error: [%d]."
-                % (filedata[0]['p4ExitCode']));
-
-        j = 0;
-        contents = {}
-        while j < len(filedata):
-            stat = filedata[j]
-            j += 1
-            text = ''
-            while j < len(filedata) and filedata[j]['code'] in ('text',
-                                                                'binary'):
-                text += filedata[j]['data']
-                j += 1
+        p4 = p4CmdListPipe('-x - print',
+                           stdin='\n'.join(['%s#%s' % (f['path'], f['rev'])
+                                            for f in files]),
+                           stdin_mode='w+')
 
+        self.curDepotFile = None
+        return p4
 
-            if not stat.has_key('depotFile'):
-                sys.stderr.write("p4 print fails with: %s\n" % repr(stat))
-                continue
+    # Uisng the pipe handle provided by openP4File, read in a file and
+    # return the pair of (depot file name, contents)
+    def readP4File(self, p4):
+        text = ''
 
-            contents[stat['depotFile']] = text
+        try:
+            while True:
+                entry = marshal.load(p4.stdout)
+
+                if entry['code'] in ('text', 'binary'):
+                    text += entry['data']
+                elif entry['code'] == 'stat':
+                    # We are done with the previous file
+                    if self.curDepotFile is not None:
+                        if not entry.has_key('depotFile'):
+                            sys.stderr.write("p4 print fails with: %s\n" % repr(entry))
+                            self.curDepotFile = None
+                            continue
+
+                        depotFile = self.curDepotFile
+                        self.curDepotFile = entry['depotFile']
+                        if verbose: sys.stderr.write('Read %s\n' % depotFile)
+                        return (depotFile, text)
+
+                    text = ''
+                    self.curDepotFile = entry['depotFile']
+                else:
+                    sys.stderr.write("p4 print returned unexpected code: %s\n" % entry['code'])
+
+        except EOFError:
+            pass
+
+        exitCode = p4.wait()
+        if exitCode != 0:
+            die("Problems executing p4. Error: [%d]." % exitCode);
+
+        depotFile = self.curDepotFile
+        self.curDepotFile = None
+        if verbose: sys.stderr.write('Read %s\n' % depotFile)
+        return (depotFile, text)
 
-        for f in files:
-            assert not f.has_key('data')
-            f['data'] = contents[f['path']]
 
     def commit(self, details, files, branch, branchPrefixes, parent = ""):
         epoch = details["time"]
@@ -996,6 +1021,10 @@ class P4Sync(Command):
 
         # start with reading files; if that fails, we should not
         # create a commit.
+
+        # XXX No, reading all file contents into memory is to tall a
+        # price to pay. Right now if an error is found, it will abort,
+        # leaving cruft for git gc to prune.
         new_files = []
         for f in files:
             if [p for p in branchPrefixes if f['path'].startswith(p)]:
@@ -1003,7 +1032,6 @@ class P4Sync(Command):
             else:
                 sys.stderr.write("Ignoring file outside of prefix: %s\n" % path)
         files = new_files
-        self.readP4Files(files)
 
 
 
@@ -1034,17 +1062,26 @@ class P4Sync(Command):
                 print "parent %s" % parent
             self.gitStream.write("from %s\n" % parent)
 
-        for file in files:
+        # Create a depotFileName -> file mapping
+        revFile = {}
+        for f in files:
+            revFile[f['path']] = f
+
+        p4 = self.openP4Files(files)
+        (depotFile, data) = self.readP4File(p4)
+
+        while depotFile is not None:
+
+            file = revFile[depotFile]
             if file["type"] == "apple":
                 print "\nfile %s is a strange apple file that forks. Ignoring!" % file['path']
+                (depotFile, data) = self.readP4File(p4)
                 continue
 
             relPath = self.stripRepoPath(file['path'], branchPrefixes)
             if file["action"] == "delete":
                 self.gitStream.write("D %s\n" % relPath)
             else:
-                data = file['data']
-
                 mode = "644"
                 if isP4Exec(file["type"]):
                     mode = "755"
@@ -1061,6 +1098,9 @@ class P4Sync(Command):
                 self.gitStream.write(data)
                 self.gitStream.write("\n")
 
+            (depotFile, data) = self.readP4File(p4)
+
+
         self.gitStream.write("\n")
 
         change = int(details["change"])
-- 
1.5.4.rc5.17.g22b645

Re: [PATCH 2/3] git-p4: support exclude paths

[Simon Hausmann]

[-- Attachment #1: Type: text/plain, Size: 3529 bytes --]

On Sunday 03 February 2008 10:21:05 Tommy Thorn wrote:
> Teach git-p4 about the -/ option which adds depot paths to the exclude
> list, used when cloning. The option is chosen such that the natural
> Perforce syntax works, eg:
>
>   git p4 clone //branch/path/... -//branch/path/{large,old}/...
>
> Trailing ... on exclude paths are optional.
>
> This is a generalization of a change by Dmitry Kakurin (thanks).
>
> Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>

Acked-By: Simon Hausmann <simon@lst.de>

I like it, Perforce'ish syntax. (Not that I like p4 though ;)


Simon

> ---
>  contrib/fast-import/git-p4 |   26 ++++++++++++++++++++++----
>  1 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 553e237..2340876 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -876,18 +876,25 @@ class P4Sync(Command):
>          self.keepRepoPath = False
>          self.depotPaths = None
>          self.p4BranchesInGit = []
> +        self.cloneExclude = []
>
>          if gitConfig("git-p4.syncFromOrigin") == "false":
>              self.syncWithOrigin = False
>
>      def extractFilesFromCommit(self, commit):
> +        self.cloneExclude = [re.sub(r"\.\.\.$", "", path)
> +                             for path in self.cloneExclude]
>          files = []
>          fnum = 0
>          while commit.has_key("depotFile%s" % fnum):
>              path =  commit["depotFile%s" % fnum]
>
> -            found = [p for p in self.depotPaths
> -                     if path.startswith (p)]
> +            if [p for p in self.cloneExclude
> +                if path.startswith (p)]:
> +                found = False
> +            else:
> +                found = [p for p in self.depotPaths
> +                         if path.startswith (p)]
>              if not found:
>                  fnum = fnum + 1
>                  continue
> @@ -1658,13 +1665,23 @@ class P4Clone(P4Sync):
>          P4Sync.__init__(self)
>          self.description = "Creates a new git repository and imports from
> Perforce into it" self.usage = "usage: %prog [options]
> //depot/path[@revRange]" -        self.options.append(
> +        self.options += [
>              optparse.make_option("--destination", dest="cloneDestination",
>                                   action='store', default=None,
> -                                 help="where to leave result of the
> clone")) +                                 help="where to leave result of
> the clone"), +            optparse.make_option("-/", dest="cloneExclude",
> +                                 action="append", type="string",
> +                                 help="exclude depot path")
> +        ]
>          self.cloneDestination = None
>          self.needsGit = False
>
> +    # This is required for the "append" cloneExclude action
> +    def ensure_value(self, attr, value):
> +        if not hasattr(self, attr) or getattr(self, attr) is None:
> +            setattr(self, attr, value)
> +        return getattr(self, attr)
> +
>      def defaultDestination(self, args):
>          ## TODO: use common prefix of args?
>          depotPath = args[0]
> @@ -1688,6 +1705,7 @@ class P4Clone(P4Sync):
>              self.cloneDestination = depotPaths[-1]
>              depotPaths = depotPaths[:-1]
>
> +        self.cloneExclude = ["/"+p for p in self.cloneExclude]
>          for p in depotPaths:
>              if not p.startswith("//"):
>                  return False

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 2/3] git-p4: support exclude paths

[Tommy Thorn]

Simon Hausmann wrote:
> On Sunday 03 February 2008 10:21:05 Tommy Thorn wrote:
>   
>> Teach git-p4 about the -/ option which adds depot paths to the exclude
>> list, used when cloning. The option is chosen such that the natural
>> Perforce syntax works, eg:
>>
>>   git p4 clone //branch/path/... -//branch/path/{large,old}/...
>>
>> Trailing ... on exclude paths are optional.
>>
>> This is a generalization of a change by Dmitry Kakurin (thanks).
>>
>> Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>
>>     
>
> Acked-By: Simon Hausmann <simon@lst.de>
>
> I like it, Perforce'ish syntax. (Not that I like p4 though ;)
>   
Thank you.

I would appear that I have some mail server problems, so apologies if 
you get multiple copies.

Also, this is the first Python hacking I've tried, so it's likely that 
my changes needs improvement.

With these two patches, I can now use git-p4. However in a perfect 
world, git-p4 would:

- include support everything that a (ugly) Perforce client can do, 
including naming individual files
  and remapping things around (a sick feature that never should be used 
IMO), and

- not consume memory proportional to the imported files.

The former would require pervasive changes and likely break some 
assumptions currently made.

The latter is easy enough.


Regards
Tommy

Re: [PATCH 2/3] git-p4: support exclude paths

[Tommy Thorn]

On Sunday 03 February 2008 10:21:05 I wrote:
> Teach git-p4 about the -/ option which adds depot paths to the exclude
> list, used when cloning. The option is chosen such that the natural
> Perforce syntax works, eg:
>
>   git p4 clone //branch/path/... -//branch/path/{large,old}/...
>
> Trailing ... on exclude paths are optional.
>
> This is a generalization of a change by Dmitry Kakurin (thanks).
>
> Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>

.. to which Simon replied:
> Acked-By: Simon Hausmann <simon@lst.de>
>
> I like it, Perforce'ish syntax. (Not that I like p4 though ;)

Alas, this change needs more work - the exclude paths needs to 
be maintained in the commit messages as otherwise we pull in new
files in the excluded path. I haven't done this yet.

However, the other patch (git-p4: no longer keep all file contents while cloning)
is IMO critical. You simply cannot clone a non-trivial Perforce repository 
without it. Why is this being ignored? Are there no users of git-p4?


Tommy

**

Re: [PATCH 2/3] git-p4: support exclude paths

[Simon Hausmann]

[-- Attachment #1: Type: text/plain, Size: 1434 bytes --]

On Tuesday 12 February 2008 16:53:38 Tommy Thorn wrote:
> On Sunday 03 February 2008 10:21:05 I wrote:
> > Teach git-p4 about the -/ option which adds depot paths to the exclude
> > list, used when cloning. The option is chosen such that the natural
> > Perforce syntax works, eg:
> >
> >   git p4 clone //branch/path/... -//branch/path/{large,old}/...
> >
> > Trailing ... on exclude paths are optional.
> >
> > This is a generalization of a change by Dmitry Kakurin (thanks).
> >
> > Signed-off-by: Tommy Thorn <tommy-git@thorn.ws>
>
> .. to which Simon replied:
> > Acked-By: Simon Hausmann <simon@lst.de>
> >
> > I like it, Perforce'ish syntax. (Not that I like p4 though ;)
>
> Alas, this change needs more work - the exclude paths needs to
> be maintained in the commit messages as otherwise we pull in new
> files in the excluded path. I haven't done this yet.
>
> However, the other patch (git-p4: no longer keep all file contents while
> cloning) is IMO critical. You simply cannot clone a non-trivial Perforce
> repository without it. Why is this being ignored? Are there no users of
> git-p4?

Sorry for the delay, my real life has kept be busy :)

I now looked at the patch and I'm all in favour of applying it. However it 
doesn't seem to apply against the current git-p4. Can you re-send the patch 
to me?

Thanks,
Simon

P.S.: I have applied the other patch (exclude path support)

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 189 bytes --]