convert: fix the handling of empty changlist descriptions in P4

Empty changelist descriptions are valid in Perforce. If we encounter one of
them we are currently running into an IndexError. In case of empty commit
messages set the commit message to **empty changelist description**, which
follows Perforce terminology.

hgext/convert/p4.py

@@ -1,368 +1,373 b''
 # Perforce source for convert extension.
 #
 # Copyright 2009, Frank Kingswood <[e-mail]>
 #
 # This software may be used and distributed according to the terms of the
 # GNU General Public License version 2 or any later version.
 from __future__ import absolute_import
 
 import marshal
 import re
 
 from mercurial.i18n import _
 from mercurial import (
     error,
     util,
 )
 
 from . import common
 
 def loaditer(f):
     "Yield the dictionary objects generated by p4"
     try:
         while True:
             d = marshal.load(f)
             if not d:
                 break
             yield d
     except EOFError:
         pass
 
 def decodefilename(filename):
     """Perforce escapes special characters @, #, *, or %
     with %40, %23, %2A, or %25 respectively
 
     >>> decodefilename('portable-net45%252Bnetcore45%252Bwp8%252BMonoAndroid')
     'portable-net45%2Bnetcore45%2Bwp8%2BMonoAndroid'
     >>> decodefilename('//Depot/Directory/%2525/%2523/%23%40.%2A')
     '//Depot/Directory/%25/%23/#@.*'
     """
     replacements = [('%2A', '*'), ('%23', '#'), ('%40', '@'), ('%25', '%')]
     for k, v in replacements:
         filename = filename.replace(k, v)
     return filename
 
 class p4_source(common.converter_source):
     def __init__(self, ui, path, revs=None):
         # avoid import cycle
         from . import convcmd
 
         super(p4_source, self).__init__(ui, path, revs=revs)
 
         if "/" in path and not path.startswith('//'):
             raise common.NoRepo(_('%s does not look like a P4 repository') %
                                 path)
 
         common.checktool('p4', abort=False)
 
         self.revmap = {}
         self.encoding = self.ui.config('convert', 'p4.encoding',
                                        default=convcmd.orig_encoding)
         self.re_type = re.compile(
             "([a-z]+)?(text|binary|symlink|apple|resource|unicode|utf\d+)"
             "(\+\w+)?$")
         self.re_keywords = re.compile(
             r"\$(Id|Header|Date|DateTime|Change|File|Revision|Author)"
             r":[^$\n]*\$")
         self.re_keywords_old = re.compile("\$(Id|Header):[^$\n]*\$")
 
         if revs and len(revs) > 1:
             raise error.Abort(_("p4 source does not support specifying "
                                "multiple revisions"))
 
     def setrevmap(self, revmap):
         """Sets the parsed revmap dictionary.
 
         Revmap stores mappings from a source revision to a target revision.
         It is set in convertcmd.convert and provided by the user as a file
         on the commandline.
 
         Revisions in the map are considered beeing present in the
         repository and ignored during _parse(). This allows for incremental
         imports if a revmap is provided.
         """
         self.revmap = revmap
 
     def _parse_view(self, path):
         "Read changes affecting the path"
         cmd = 'p4 -G changes -s submitted %s' % util.shellquote(path)
         stdout = util.popen(cmd, mode='rb')
         p4changes = {}
         for d in loaditer(stdout):
             c = d.get("change", None)
             if c:
                 p4changes[c] = True
         return p4changes
 
     def _parse(self, ui, path):
         "Prepare list of P4 filenames and revisions to import"
         p4changes = {}
         changeset = {}
         files_map = {}
         copies_map = {}
         localname = {}
         depotname = {}
         heads = []
 
         ui.status(_('reading p4 views\n'))
 
         # read client spec or view
         if "/" in path:
             p4changes.update(self._parse_view(path))
             if path.startswith("//") and path.endswith("/..."):
                 views = {path[:-3]:""}
             else:
                 views = {"//": ""}
         else:
             cmd = 'p4 -G client -o %s' % util.shellquote(path)
             clientspec = marshal.load(util.popen(cmd, mode='rb'))
 
             views = {}
             for client in clientspec:
                 if client.startswith("View"):
                     sview, cview = clientspec[client].split()
                     p4changes.update(self._parse_view(sview))
                     if sview.endswith("...") and cview.endswith("..."):
                         sview = sview[:-3]
                         cview = cview[:-3]
                     cview = cview[2:]
                     cview = cview[cview.find("/") + 1:]
                     views[sview] = cview
 
         # list of changes that affect our source files
         p4changes = p4changes.keys()
         p4changes.sort(key=int)
 
         # list with depot pathnames, longest first
         vieworder = views.keys()
         vieworder.sort(key=len, reverse=True)
 
         # handle revision limiting
         startrev = self.ui.config('convert', 'p4.startrev', default=0)
 
         # now read the full changelists to get the list of file revisions
         ui.status(_('collecting p4 changelists\n'))
         lastid = None
         for change in p4changes:
             if startrev and int(change) < int(startrev):
                 continue
             if self.revs and int(change) > int(self.revs[0]):
                 continue
             if change in self.revmap:
                 # Ignore already present revisions, but set the parent pointer.
                 lastid = change
                 continue
 
             if lastid:
                 parents = [lastid]
             else:
                 parents = []
 
             d = self._fetch_revision(change)
             c = self._construct_commit(d, parents)
 
-            shortdesc = c.desc.splitlines(True)[0].rstrip('\r\n')
+            descarr = c.desc.splitlines(True)
+            if len(descarr) > 0:
+                shortdesc = descarr[0].rstrip('\r\n')
+            else:
+                shortdesc = '**empty changelist description**'
+
             t = '%s %s' % (c.rev, repr(shortdesc)[1:-1])
             ui.status(util.ellipsis(t, 80) + '\n')
 
             files = []
             copies = {}
             copiedfiles = []
             i = 0
             while ("depotFile%d" % i) in d and ("rev%d" % i) in d:
                 oldname = d["depotFile%d" % i]
                 filename = None
                 for v in vieworder:
                     if oldname.lower().startswith(v.lower()):
                         filename = decodefilename(views[v] + oldname[len(v):])
                         break
                 if filename:
                     files.append((filename, d["rev%d" % i]))
                     depotname[filename] = oldname
                     if (d.get("action%d" % i) == "move/add"):
                         copiedfiles.append(filename)
                     localname[oldname] = filename
                 i += 1
 
             # Collect information about copied files
             for filename in copiedfiles:
                 oldname = depotname[filename]
 
                 flcmd = 'p4 -G filelog %s' \
                       % util.shellquote(oldname)
                 flstdout = util.popen(flcmd, mode='rb')
 
                 copiedfilename = None
                 for d in loaditer(flstdout):
                     copiedoldname = None
 
                     i = 0
                     while ("change%d" % i) in d:
                         if (d["change%d" % i] == change and
                             d["action%d" % i] == "move/add"):
                             j = 0
                             while ("file%d,%d" % (i, j)) in d:
                                 if d["how%d,%d" % (i, j)] == "moved from":
                                     copiedoldname = d["file%d,%d" % (i, j)]
                                     break
                                 j += 1
                         i += 1
 
                     if copiedoldname and copiedoldname in localname:
                         copiedfilename = localname[copiedoldname]
                         break
 
                 if copiedfilename:
                     copies[filename] = copiedfilename
                 else:
                     ui.warn(_("cannot find source for copied file: %s@%s\n")
                             % (filename, change))
 
             changeset[change] = c
             files_map[change] = files
             copies_map[change] = copies
             lastid = change
 
         if lastid and len(changeset) > 0:
             heads = [lastid]
 
         return {
             'changeset': changeset,
             'files': files_map,
             'copies': copies_map,
             'heads': heads,
             'depotname': depotname,
         }
 
     @util.propertycache
     def _parse_once(self):
         return self._parse(self.ui, self.path)
 
     @util.propertycache
     def copies(self):
         return self._parse_once['copies']
 
     @util.propertycache
     def files(self):
         return self._parse_once['files']
 
     @util.propertycache
     def changeset(self):
         return self._parse_once['changeset']
 
     @util.propertycache
     def heads(self):
         return self._parse_once['heads']
 
     @util.propertycache
     def depotname(self):
         return self._parse_once['depotname']
 
     def getheads(self):
         return self.heads
 
     def getfile(self, name, rev):
         cmd = 'p4 -G print %s' \
             % util.shellquote("%s#%s" % (self.depotname[name], rev))
 
         lasterror = None
         while True:
             stdout = util.popen(cmd, mode='rb')
 
             mode = None
             contents = []
             keywords = None
 
             for d in loaditer(stdout):
                 code = d["code"]
                 data = d.get("data")
 
                 if code == "error":
                     # if this is the first time error happened
                     # re-attempt getting the file
                     if not lasterror:
                         lasterror = IOError(d["generic"], data)
                         # this will exit inner-most for-loop
                         break
                     else:
                         raise lasterror
 
                 elif code == "stat":
                     action = d.get("action")
                     if action in ["purge", "delete", "move/delete"]:
                         return None, None
                     p4type = self.re_type.match(d["type"])
                     if p4type:
                         mode = ""
                         flags = ((p4type.group(1) or "")
                                + (p4type.group(3) or ""))
                         if "x" in flags:
                             mode = "x"
                         if p4type.group(2) == "symlink":
                             mode = "l"
                         if "ko" in flags:
                             keywords = self.re_keywords_old
                         elif "k" in flags:
                             keywords = self.re_keywords
 
                 elif code == "text" or code == "binary":
                     contents.append(data)
 
                 lasterror = None
 
             if not lasterror:
                 break
 
         if mode is None:
             return None, None
 
         contents = ''.join(contents)
 
         if keywords:
             contents = keywords.sub("$\\1$", contents)
         if mode == "l" and contents.endswith("\n"):
             contents = contents[:-1]
 
         return contents, mode
 
     def getchanges(self, rev, full):
         if full:
             raise error.Abort(_("convert from p4 does not support --full"))
         return self.files[rev], self.copies[rev], set()
 
     def _construct_commit(self, obj, parents=None):
         """
         Constructs a common.commit object from an unmarshalled
         `p4 describe` output
         """
         desc = self.recode(obj.get("desc", ""))
         date = (int(obj["time"]), 0)     # timezone not set
         if parents is None:
             parents = []
 
         return common.commit(author=self.recode(obj["user"]),
             date=util.datestr(date, '%Y-%m-%d %H:%M:%S %1%2'),
             parents=parents, desc=desc, branch=None, rev=obj['change'],
             extra={"p4": obj['change'], "convert_revision": obj['change']})
 
     def _fetch_revision(self, rev):
         """Return an output of `p4 describe` including author, commit date as
         a dictionary."""
         cmd = "p4 -G describe -s %s" % rev
         stdout = util.popen(cmd, mode='rb')
         return marshal.load(stdout)
 
     def getcommit(self, rev):
         if rev in self.changeset:
             return self.changeset[rev]
         elif rev in self.revmap:
             d = self._fetch_revision(rev)
             return self._construct_commit(d, parents=None)
         raise error.Abort(
             _("cannot find %s in the revmap or parsed changesets") % rev)
 
     def gettags(self):
         return {}
 
     def getchangedfiles(self, rev, i):
         return sorted([x[0] for x in self.files[rev]])

tests/test-convert-p4.t

@@ -1,145 +1,163 b''
 #require p4
 
   $ echo "[extensions]" >> $HGRCPATH
   $ echo "convert = " >> $HGRCPATH
 
 create p4 depot
   $ P4ROOT=`pwd`/depot; export P4ROOT
   $ P4AUDIT=$P4ROOT/audit; export P4AUDIT
   $ P4JOURNAL=$P4ROOT/journal; export P4JOURNAL
   $ P4LOG=$P4ROOT/log; export P4LOG
   $ P4PORT=localhost:$HGPORT; export P4PORT
   $ P4DEBUG=1; export P4DEBUG
 
 start the p4 server
   $ [ ! -d $P4ROOT ] && mkdir $P4ROOT
   $ p4d -f -J off >$P4ROOT/stdout 2>$P4ROOT/stderr &
   $ echo $! >> $DAEMON_PIDS
   $ trap "echo stopping the p4 server ; p4 admin stop" EXIT
 
   $ # wait for the server to initialize
   $ while ! p4 ; do
   >    sleep 1
   > done >/dev/null 2>/dev/null
 
 create a client spec
   $ P4CLIENT=hg-p4-import; export P4CLIENT
   $ DEPOTPATH=//depot/test-mercurial-import/...
   $ p4 client -o | sed '/^View:/,$ d' >p4client
   $ echo View: >>p4client
   $ echo " $DEPOTPATH //$P4CLIENT/..." >>p4client
   $ p4 client -i <p4client
   Client hg-p4-import saved.
 
 populate the depot
   $ echo a > a
   $ mkdir b
   $ echo c > b/c
   $ p4 add a b/c
   //depot/test-mercurial-import/a#1 - opened for add
   //depot/test-mercurial-import/b/c#1 - opened for add
   $ p4 submit -d initial
   Submitting change 1.
   Locking 2 files ...
   add //depot/test-mercurial-import/a#1
   add //depot/test-mercurial-import/b/c#1
   Change 1 submitted.
 
 change some files
   $ p4 edit a
   //depot/test-mercurial-import/a#1 - opened for edit
   $ echo aa >> a
   $ p4 submit -d "change a"
   Submitting change 2.
   Locking 1 files ...
   edit //depot/test-mercurial-import/a#2
   Change 2 submitted.
 
   $ p4 edit b/c
   //depot/test-mercurial-import/b/c#1 - opened for edit
   $ echo cc >> b/c
   $ p4 submit -d "change b/c"
   Submitting change 3.
   Locking 1 files ...
   edit //depot/test-mercurial-import/b/c#2
   Change 3 submitted.
 
 convert
   $ hg convert -s p4 $DEPOTPATH dst
   initializing destination dst repository
   scanning source...
   reading p4 views
   collecting p4 changelists
   1 initial
   2 change a
   3 change b/c
   sorting...
   converting...
   2 initial
   1 change a
   0 change b/c
   $ hg -R dst log --template 'rev={rev} desc="{desc}" tags="{tags}" files="{files}"\n'
   rev=2 desc="change b/c" tags="tip" files="b/c"
   rev=1 desc="change a" tags="" files="a"
   rev=0 desc="initial" tags="" files="a b/c"
 
 change some files
   $ p4 edit a b/c
   //depot/test-mercurial-import/a#2 - opened for edit
   //depot/test-mercurial-import/b/c#2 - opened for edit
   $ echo aaa >> a
   $ echo ccc >> b/c
   $ p4 submit -d "change a b/c"
   Submitting change 4.
   Locking 2 files ...
   edit //depot/test-mercurial-import/a#3
   edit //depot/test-mercurial-import/b/c#3
   Change 4 submitted.
 
 convert again
   $ hg convert -s p4 $DEPOTPATH dst
   scanning source...
   reading p4 views
   collecting p4 changelists
   4 change a b/c
   sorting...
   converting...
   0 change a b/c
   $ hg -R dst log --template 'rev={rev} desc="{desc}" tags="{tags}" files="{files}"\n'
   rev=3 desc="change a b/c" tags="tip" files="a b/c"
   rev=2 desc="change b/c" tags="" files="b/c"
   rev=1 desc="change a" tags="" files="a"
   rev=0 desc="initial" tags="" files="a b/c"
 
 interesting names
   $ echo dddd > "d d"
   $ mkdir " e"
   $ echo fff >" e/ f"
   $ p4 add "d d" " e/ f"
   //depot/test-mercurial-import/d d#1 - opened for add
   //depot/test-mercurial-import/ e/ f#1 - opened for add
   $ p4 submit -d "add d e f"
   Submitting change 5.
   Locking 2 files ...
   add //depot/test-mercurial-import/ e/ f#1
   add //depot/test-mercurial-import/d d#1
   Change 5 submitted.
 
 convert again
   $ hg convert -s p4 $DEPOTPATH dst
   scanning source...
   reading p4 views
   collecting p4 changelists
   5 add d e f
   sorting...
   converting...
   0 add d e f
   $ hg -R dst log --template 'rev={rev} desc="{desc}" tags="{tags}" files="{files}"\n'
   rev=4 desc="add d e f" tags="tip" files=" e/ f d d"
   rev=3 desc="change a b/c" tags="" files="a b/c"
   rev=2 desc="change b/c" tags="" files="b/c"
   rev=1 desc="change a" tags="" files="a"
   rev=0 desc="initial" tags="" files="a b/c"
 
+empty commit message
+  $ p4 edit a
+  //depot/test-mercurial-import/a#3 - opened for edit
+  $ echo aaaaa >> a
+  $ p4 submit -d ""
+  Submitting change 6.
+  Locking 1 files ...
+  edit //depot/test-mercurial-import/a#4
+  Change 6 submitted.
+  $ hg convert -s p4 $DEPOTPATH dst
+  scanning source...
+  reading p4 views
+  collecting p4 changelists
+  6 **empty changelist description**
+  sorting...
+  converting...
+  0 
+
 exit trap:
   stopping the p4 server