Patchwork convert: Fix the handling of empty changlist descriptions in P4

login
register
mail settings
Submitter David Soria Parra
Date March 22, 2017, 7:13 p.m.
Message ID <ff24a4168c2f05fd367b.1490210007@davidsp-mbp.dhcp.thefacebook.com>
Download mbox | patch
Permalink /patch/19596/
State Accepted
Headers show

Comments

David Soria Parra - March 22, 2017, 7:13 p.m.
# HG changeset patch
# User David Soria Parra <davidsp@fb.com>
# Date 1490209978 18000
#      Wed Mar 22 14:12:58 2017 -0500
# Node ID ff24a4168c2f05fd367b0b126e52559ef89af77f
# Parent  102f291807c92864a2231e5e925d6cd64783bb59
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.
Sean Farley - March 22, 2017, 8:22 p.m.
David Soria Parra <davidsp@fb.com> writes:

> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1490209978 18000
> #      Wed Mar 22 14:12:58 2017 -0500
> # Node ID ff24a4168c2f05fd367b0b126e52559ef89af77f
> # Parent  102f291807c92864a2231e5e925d6cd64783bb59
> convert: Fix the handling of empty changlist descriptions in P4

Looks like a good patch. Only minor nits in my review. 'F' in "Fix"
should be 'f'.

> 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.
>
> diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
> --- a/hgext/convert/p4.py
> +++ b/hgext/convert/p4.py
> @@ -161,7 +161,12 @@
>              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**'

We usually write this as:

shortdesc = '**empty changelist description**'
descarr = c.desc.splitlines(True)
if len(descarr) > 0:
    shortdesc = descarr[0].rstrip('\r\n')

But that's probably not worth sending again. More of an FYI.
Augie Fackler - March 23, 2017, 3:30 p.m.
On Wed, Mar 22, 2017 at 02:13:27PM -0500, David Soria Parra wrote:
> # HG changeset patch
> # User David Soria Parra <davidsp@fb.com>
> # Date 1490209978 18000
> #      Wed Mar 22 14:12:58 2017 -0500
> # Node ID ff24a4168c2f05fd367b0b126e52559ef89af77f
> # Parent  102f291807c92864a2231e5e925d6cd64783bb59
> convert: Fix the handling of empty changlist descriptions in P4

Queued, thanks.

>
> 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.
>
> diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
> --- a/hgext/convert/p4.py
> +++ b/hgext/convert/p4.py
> @@ -161,7 +161,12 @@
>              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')
>
> diff --git a/tests/test-convert-p4.t b/tests/test-convert-p4.t
> --- a/tests/test-convert-p4.t
> +++ b/tests/test-convert-p4.t
> @@ -141,5 +141,23 @@
>    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
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/hgext/convert/p4.py b/hgext/convert/p4.py
--- a/hgext/convert/p4.py
+++ b/hgext/convert/p4.py
@@ -161,7 +161,12 @@ 
             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')
 
diff --git a/tests/test-convert-p4.t b/tests/test-convert-p4.t
--- a/tests/test-convert-p4.t
+++ b/tests/test-convert-p4.t
@@ -141,5 +141,23 @@ 
   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