[Sugar-devel] [PATCH] Copying files multiple times results in bogus names #2060

[Aleksey Lim]

---
 .gitignore                  |    1 +
 src/jarabe/journal/model.py |   33 ++++++++++++++++-----
 tests/__main__.py           |    6 ++++
 tests/jarabe                |    1 +
 tests/journal_model.py      |   67 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 100 insertions(+), 8 deletions(-)
 create mode 100644 tests/__main__.py
 create mode 120000 tests/jarabe
 create mode 100644 tests/journal_model.py

diff --git a/.gitignore b/.gitignore
index 5da75ec..7bf998d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -13,6 +13,7 @@ Makefile.in
 .*.sw?
 *.service
 stamp-*
+.tmp
 
 # Absolute
 
diff --git a/src/jarabe/journal/model.py b/src/jarabe/journal/model.py
index ffc62e0..26afa25 100644
--- a/src/jarabe/journal/model.py
+++ b/src/jarabe/journal/model.py
@@ -487,7 +487,7 @@ def write(metadata, file_path='', update_mtime=True, transfer_ownership=True):
                              'removable devices')
 
         file_name = _get_file_name(metadata['title'], metadata['mime_type'])
-        file_name = _get_unique_file_name(metadata['mountpoint'], file_name)
+        file_name = get_unique_file_name(metadata['mountpoint'], file_name)
 
         destination_path = os.path.join(metadata['mountpoint'], file_name)
         shutil.copy(file_path, destination_path)
@@ -520,18 +520,35 @@ def _get_file_name(title, mime_type):
 
     return file_name
 
-def _get_unique_file_name(mount_point, file_name):
+
+_get_unique_file_name_re = re.compile('(.*)_([0-9]+)$')
+
+
+def get_unique_file_name(mount_point, file_name):
     if os.path.exists(os.path.join(mount_point, file_name)):
-        i = 1
-        while len(file_name) <= 255:
-            name, extension = os.path.splitext(file_name)
-            file_name = name + '_' + str(i) + extension
-            if not os.path.exists(os.path.join(mount_point, file_name)):
+        name, extension = os.path.splitext(file_name)
+
+        match = _get_unique_file_name_re.match(name)
+        if match is None:
+            number = 0
+        else:
+            name, number = match.groups()
+            number = int(number)
+
+        while True:
+            number += 1
+            new_file_name = '%s_%s%s' % (name, number, extension)
+
+            if len(new_file_name) > 255:
+                break
+
+            if not os.path.exists(os.path.join(mount_point, new_file_name)):
+                file_name = new_file_name
                 break
-            i += 1
 
     return file_name
 
+
 def is_editable(metadata):
     mountpoint = metadata.get('mountpoint', '/')
     return mountpoint == '/'
diff --git a/tests/__main__.py b/tests/__main__.py
new file mode 100644
index 0000000..3105c15
--- /dev/null
+++ b/tests/__main__.py
@@ -0,0 +1,6 @@
+import unittest
+
+from journal_model import *
+
+if __name__ == '__main__':
+    unittest.main()
diff --git a/tests/jarabe b/tests/jarabe
new file mode 120000
index 0000000..aae041a
--- /dev/null
+++ b/tests/jarabe
@@ -0,0 +1 @@
+../src/jarabe/
\ No newline at end of file
diff --git a/tests/journal_model.py b/tests/journal_model.py
new file mode 100644
index 0000000..02556e8
--- /dev/null
+++ b/tests/journal_model.py
@@ -0,0 +1,67 @@
+import os
+import re
+import shutil
+import unittest
+
+from jarabe.journal import model
+
+
+class TestJournalModel(unittest.TestCase):
+
+    def setUp(self):
+        shutil.rmtree('.tmp', ignore_errors=True)
+        os.makedirs('.tmp')
+
+    def test_get_unique_file_name(self):
+        touch('.tmp/foo.bar')
+
+        self.assertEqual(
+                'foo_bar',
+                model.get_unique_file_name('.tmp', 'foo_bar'))
+        self.assertEqual(
+                'foo_1.bar',
+                model.get_unique_file_name('.tmp', 'foo.bar'))
+        self.assertEqual(
+                'foo_1.bar',
+                model.get_unique_file_name('.tmp', 'foo_1.bar'))
+
+        touch('.tmp/foo_1.bar')
+        touch('.tmp/foo_2.bar')
+        touch('.tmp/foo_4.bar')
+
+        self.assertEqual(
+                'foo_3.bar',
+                model.get_unique_file_name('.tmp', 'foo.bar'))
+        self.assertEqual(
+                'foo_3.bar',
+                model.get_unique_file_name('.tmp', 'foo_1.bar'))
+        self.assertEqual(
+                'foo_3.bar',
+                model.get_unique_file_name('.tmp', 'foo_3.bar'))
+        self.assertEqual(
+                'foo_5.bar',
+                model.get_unique_file_name('.tmp', 'foo_4.bar'))
+
+        long_name = '_' * 254
+
+        touch('.tmp/' + long_name + '8')
+        self.assertEqual(
+                long_name + '9',
+                model.get_unique_file_name('.tmp', long_name + '8'))
+
+        touch('.tmp/' + long_name + '9')
+        self.assertEqual(
+                long_name + '9',
+                model.get_unique_file_name('.tmp', long_name + '9'))
+
+
+def touch(path):
+    if not os.path.exists(os.path.dirname(path)):
+        os.makedirs(os.path.dirname(path))
+
+    f = file(path, 'w')
+    f.close()
+
+
+if __name__ == '__main__':
+    unittest.main()
-- 
1.7.3.3