Avoid importing invalid and duplicate patch names

This aims to be a comprehensive repair of issues with `stg import` allowing
invalid and duplicate patch names.

The new Patches.make_name() method creates a unique and valid patch name
based on its knowledge of existing patch names. Since StGit patch names are
used as Git refs, make_name() also attempts to enforce Git's rules for ref
naming. Generated patch names are checked with `git check-ref-format`
before being returned.

Repairs #64

Signed-off-by: Peter Grayson <[email protected]>

Author: jpgrayson

stgit/commands/imprt.py

@@ -18,7 +18,6 @@
 from stgit.lib.transaction import StackTransaction, TransactionHalted
 from stgit.out import out
 from stgit.run import Run
-from stgit.utils import make_patch_name
 
 __copyright__ = """
 Copyright (C) 2005, Catalin Marinas <[email protected]>
@@ -149,17 +148,6 @@
 directory = DirectoryHasRepository()
 
 
-def __strip_patch_name(name):
-    stripped = re.sub('^[0-9]+-(.*)$', r'\g<1>', name)
-    stripped = re.sub(r'^(.*)\.(diff|patch)$', r'\g<1>', stripped)
-    return stripped
-
-
-def __replace_slashes_with_dashes(name):
-    stripped = name.replace('/', '-')
-    return stripped
-
-
 def __create_patch(
     filename, message, author_name, author_email, author_date, diff, options
 ):
@@ -168,29 +156,31 @@ def __create_patch(
 
     if options.name:
         name = options.name
-        if not stack.patches.is_name_valid(name):
-            raise CmdException('Invalid patch name: %s' % name)
     elif filename:
         name = os.path.basename(filename)
     else:
         name = ''
-    if options.stripname:
-        name = __strip_patch_name(name)
 
-    if not name:
-        if options.ignore or options.replace:
+    if options.stripname:
+        # Removing leading numbers and trailing extension
+        name = re.sub(
+            r'''^
+                (?:[0-9]+-)?           # Optional leading patch number
+                (.*?)                  # Patch name group (non-greedy)
+                (?:\.(?:diff|patch))?  # Optional .diff or .patch extension
+                $
+            ''',
+            r'\g<1>',
+            name,
+            flags=re.VERBOSE,
+        )
 
-            def unacceptable_name(name):
-                return False
+    need_unique = not (options.ignore or options.replace)
 
-        else:
-            unacceptable_name = stack.patches.exists
-        name = make_patch_name(message, unacceptable_name)
+    if name:
+        name = stack.patches.make_name(name, unique=need_unique, lower=False)
     else:
-        # fix possible invalid characters in the patch name
-        name = re.sub(r'[^\w.]+', '-', name).strip('-')
-
-    assert stack.patches.is_name_valid(name)
+        name = stack.patches.make_name(message, unique=need_unique, lower=True)
 
     if options.ignore and name in stack.patchorder.applied:
         out.info('Ignoring already applied patch "%s"' % name)
@@ -266,28 +256,30 @@ def __get_handle_and_name(filename):
     return (open(filename, 'rb'), filename)
 
 
-def __import_file(filename, options, patch=None):
+def __import_file(filename, options):
     """Import a patch from a file or standard input"""
-    pname = None
     if filename:
-        (f, pname) = __get_handle_and_name(filename)
+        f, filename = __get_handle_and_name(filename)
     else:
         f = os.fdopen(sys.__stdin__.fileno(), 'rb')
 
-    if patch:
-        pname = patch
-    elif not pname:
-        pname = filename
-
-    (message, author_name, author_email, author_date, diff) = parse_patch(
-        f.read(), contains_diff=True
-    )
+    patch_data = f.read()
 
     if filename:
         f.close()
 
+    message, author_name, author_email, author_date, diff = parse_patch(
+        patch_data, contains_diff=True
+    )
+
     __create_patch(
-        pname, message, author_name, author_email, author_date, diff, options
+        filename,
+        message,
+        author_name,
+        author_email,
+        author_date,
+        diff,
+        options,
     )
 
 
@@ -327,9 +319,8 @@ def __import_series(filename, options):
         else:
             options.strip = 1
         patchfile = os.path.join(patchdir, patch)
-        patch = __replace_slashes_with_dashes(patch)
 
-        __import_file(patchfile, options, patch)
+        __import_file(patchfile, options)
 
     if filename:
         f.close()

stgit/config.py

@@ -31,6 +31,7 @@
     ('stgit.autoimerge', ['no']),
     ('stgit.fetchcmd', ['git fetch']),
     ('stgit.keepoptimized', ['no']),
+    ('stgit.namelength', ['30']),
     ('stgit.pager', ['less']),
     ('stgit.pick.expose-format', ['format:%B%n(imported from commit %H)']),
     ('stgit.pull-policy', ['pull']),

stgit/lib/stack.py

@@ -1,4 +1,5 @@
 """A Python class hierarchy wrapping the StGit on-disk metadata."""
+import re
 
 from stgit.config import config
 from stgit.exception import StackException
@@ -167,6 +168,81 @@ def new(self, name, commit, msg):
         self._patches[name] = p
         return p
 
+    def make_name(self, raw, unique=True, lower=True):
+        """Make a unique and valid patch name from provided raw name.
+
+        The raw name may come from a filename, commit message, or email subject line.
+
+        The generated patch name will meet the rules of `git check-ref-format` along
+        with some additional StGit patch name rules.
+
+        """
+        default_name = 'patch'
+
+        for line in raw.split('\n'):
+            if line:
+                break
+
+        if not line:
+            line = default_name
+
+        if lower:
+            line = line.lower()
+
+        parts = []
+        for part in line.split('/'):
+            # fmt: off
+            part = re.sub(r'\.lock$', '', part)    # Disallowed in Git refs
+            part = re.sub(r'^\.+|\.+$', '', part)  # Cannot start or end with '.'
+            part = re.sub(r'\.+', '.', part)       # No consecutive '.'
+            part = re.sub(r'[^\w.]+', '-', part)   # Non-word and whitespace to dashes
+            part = re.sub(r'-+', '-', part)        # Squash consecutive dashes
+            part = re.sub(r'^-+|-+$', '', part)    # Remove leading and trailing dashes
+            # fmt: on
+            if part:
+                parts.append(part)
+
+        long_name = '/'.join(parts)
+
+        # TODO: slashes could be allowed in the future.
+        long_name = long_name.replace('/', '-')
+
+        if not long_name:
+            long_name = default_name
+
+        assert self.is_name_valid(long_name)
+
+        name_len = config.getint('stgit.namelength')
+
+        words = long_name.split('-')
+        short_name = words[0]
+        for word in words[1:]:
+            new_name = '%s-%s' % (short_name, word)
+            if name_len <= 0 or len(new_name) <= name_len:
+                short_name = new_name
+            else:
+                break
+        assert self.is_name_valid(short_name)
+
+        if not unique:
+            return short_name
+
+        unique_name = short_name
+        while self.exists(unique_name):
+            m = re.match(r'(.*?)(-)?(\d+)$', unique_name)
+            if m:
+                base, sep, n_str = m.groups()
+                n = int(n_str) + 1
+                if sep:
+                    unique_name = '%s%s%d' % (base, sep, n)
+                else:
+                    unique_name = '%s%d' % (base, n)
+            else:
+                unique_name = '%s-1' % unique_name
+
+        assert self.is_name_valid(unique_name)
+        return unique_name
+
 
 class Stack(Branch):
     """Represents a StGit stack.

stgit/utils.py

@@ -144,8 +144,6 @@ def patch_name_from_msg(msg):
         return None
 
     name_len = config.getint('stgit.namelength')
-    if not name_len:
-        name_len = 30
 
     subject_line = msg.split('\n', 1)[0].lstrip().lower()
     words = re.sub(r'(?u)[\W]+', ' ', subject_line).split()

t/t1800-import.sh

@@ -31,6 +31,16 @@ test_expect_success \
     stg delete ..
     '
 
+test_expect_success \
+    'Attempt import same patch twice' \
+    '
+    stg import "$TEST_DIRECTORY"/t1800/git-diff &&
+    stg pop &&
+    stg import "$TEST_DIRECTORY"/t1800/git-diff &&
+    test "$(echo $(stg series --noprefix))" = "git-diff-1 git-diff" &&
+    stg delete ..
+    '
+
 test_expect_success \
     'Apply a patch and edit message' \
     '
@@ -227,7 +237,7 @@ test_expect_success \
     '
     cat some.patch |
     stg import --ignore --author "Some Author <[email protected]>" &&
-    stg top | grep -e "unknown" &&
+    stg top | grep -e "patch" &&
     stg show | grep -e "Author: Some Author <[email protected]>" &&
     stg delete --top
     '

t/t1801-import-email.sh

@@ -105,6 +105,14 @@ test_expect_success \
     stg delete ..
     '
 
+test_expect_success \
+    'Import patches from mbox with duplicate subjects' \
+    '
+    stg import -M "$TEST_DIRECTORY"/t1801/email-mbox-same-subject &&
+    test "$(echo $(stg series --noprefix --applied))" = "my-patch my-patch-1 my-patch-2" &&
+    stg delete ..
+    '
+
 test_expect_success \
     'Apply several patches from an mbox file from stdin' \
     '

t/t1801/email-mbox-same-subject

@@ -0,0 +1,102 @@
+From nobody Sat Nov 11 12:45:27 2006
+From: Inge =?utf-8?q?Str=C3=B6m?= <[email protected]>
+Subject: My Patch
+To: Upstream <[email protected]>
+Date: Sat, 11 Nov 2006 12:45:27 +0100
+Message-ID: <20061111114527.31778.12942.stgit@localhost>
+User-Agent: StGIT/0.11
+MIME-Version: 1.0
+Content-Type: text/plain; charset="utf-8"
+Content-Transfer-Encoding: 8bit
+Status: RO
+Content-Length: 304
+Lines: 19
+
+Signed-off-by: Inge Ström <[email protected]>
+---
+
+ foo.txt |    2 +-
+ 1 files changed, 1 insertions(+), 1 deletions(-)
+
+diff --git a/foo.txt b/foo.txt
+index ad01662..91527b1 100644
+--- a/foo.txt
++++ b/foo.txt
+@@ -7,7 +7,7 @@ dobidum
+ dobodam
+ dobodim
+ dobodum
+-dibedam
++dibedad
+ dibedim
+ dibedum
+ dibidam
+
+From nobody Sat Nov 11 12:45:27 2006
+From: Inge =?utf-8?q?Str=C3=B6m?= <[email protected]>
+Subject: My Patch
+To: Upstream <[email protected]>
+Date: Sat, 11 Nov 2006 12:45:27 +0100
+Message-ID: <20061111114527.31778.92851.stgit@localhost>
+In-Reply-To: <20061111114527.31778.12942.stgit@localhost>
+References: <20061111114527.31778.12942.stgit@localhost>
+User-Agent: StGIT/0.11
+MIME-Version: 1.0
+Content-Type: text/plain; charset="utf-8"
+Content-Transfer-Encoding: 8bit
+Status: RO
+Content-Length: 296
+Lines: 18
+
+Signed-off-by: Inge Ström <[email protected]>
+---
+
+ foo.txt |    1 +
+ 1 files changed, 1 insertions(+), 0 deletions(-)
+
+diff --git a/foo.txt b/foo.txt
+index 91527b1..79922d7 100644
+--- a/foo.txt
++++ b/foo.txt
+@@ -18,6 +18,7 @@ dibodim
+ dibodum
+ dabedam
+ dabedim
++dibedam
+ dabedum
+ dabidam
+ dabidim
+
+From nobody Sat Nov 11 12:45:27 2006
+From: Inge =?utf-8?q?Str=C3=B6m?= <[email protected]>
+Subject: My Patch
+To: Upstream <[email protected]>
+Date: Sat, 11 Nov 2006 12:45:27 +0100
+Message-ID: <20061111114527.31778.45876.stgit@localhost>
+In-Reply-To: <20061111114527.31778.12942.stgit@localhost>
+References: <20061111114527.31778.12942.stgit@localhost>
+User-Agent: StGIT/0.11
+MIME-Version: 1.0
+Content-Type: text/plain; charset="utf-8"
+Content-Transfer-Encoding: 8bit
+Status: RO
+Content-Length: 278
+Lines: 16
+
+Signed-off-by: Inge Ström <[email protected]>
+---
+
+ foo.txt |    1 -
+ 1 files changed, 0 insertions(+), 1 deletions(-)
+
+diff --git a/foo.txt b/foo.txt
+index 79922d7..6f978b4 100644
+--- a/foo.txt
++++ b/foo.txt
+@@ -24,5 +24,4 @@ dabidam
+ dabidim
+ dabidum
+ dabodam
+-dabodim
+ dabodum
+