do not allow multiple files with the same UID

Prevent someone from adding multiple files with the same UID in the same
commit. Ignore any existing commits with the problem as long as there
was a later commit to delete the files.

Closes-Bug: #1688042
Change-Id: Id62361f3aba195417b369293e411c36172d27229
Signed-off-by: Doug Hellmann <doug@doughellmann.com>

releasenotes/notes/avoid-clashing-uids-e84ffe8132ce996d.yaml

@@ -0,0 +1,8 @@
+---
+fixes:
+  - |
+    Fix a problem caused by failing to process multiple files with the
+    same UID portion of the filename. Ignore existing cases as long as
+    there is a corrective patch to remove them. Prevent new cases from
+    being introduced. See https://bugs.launchpad.net/reno/+bug/1688042
+    for details.

reno/scanner.py

@@ -150,6 +150,11 @@ class _ChangeAggregator(object):
     _delete_op = set([diff_tree.CHANGE_DELETE])
     _add_op = set([diff_tree.CHANGE_ADD])
 
+    def __init__(self):
+        # Track UIDs that had a duplication issue but have been
+        # deleted so we know not to throw an error for them.
+        self._deleted_bad_uids = set()
+
     def aggregate_changes(self, walk_entry, changes):
         sha = walk_entry.commit.id
         by_uid = collections.defaultdict(list)
@@ -216,15 +221,19 @@ class _ChangeAggregator(object):
                         (uid, diff_tree.CHANGE_DELETE, c[1], sha)
                         for c in changes
                     )
+                    self._deleted_bad_uids.add(uid)
                 elif types == self._add_op:
                     # There were multiple files in one commit using the
                     # same UID but different slugs. Warn the user about
                     # this case and then ignore the files. We allow delete
                     # (see above) to ensure they can be cleaned up.
                     msg = ('%s: found several files in one commit (%s)'
-                           ' with the same UID, ignoring them: %s' %
+                           ' with the same UID: %s' %
                            (uid, sha, [c[1] for c in changes]))
-                    LOG.warning(msg)
+                    if uid not in self._deleted_bad_uids:
+                        raise ValueError(msg)
+                    else:
+                        LOG.warning(msg)
                 else:
                     raise ValueError('Unrecognized changes: {!r}'.format(
                         changes))

reno/tests/test_scanner.py

@@ -1798,14 +1798,16 @@ class AggregateChangesTest(Base):
             results,
         )
 
-    def test_add_multiple(self):
+    def test_add_multiple_after_delete(self):
         # Adding multiple files in one commit using the same UID but
-        # different slug causes the files to be ignored.
+        # different slug after we have seen a delete for the same UID
+        # causes the files to be ignored.
         entry = mock.Mock()
         n = self.get_note_num()
+        uid = '%016x' % n
         changes = []
         for i in range(2):
-            name = 'prefix/add%d-%016x.yaml' % (i, n)
+            name = 'prefix/add%d-%s.yaml' % (i, uid)
             entry.commit.id = 'commit-id'
             changes.append(
                 diff_tree.TreeChange(
@@ -1818,9 +1820,49 @@ class AggregateChangesTest(Base):
                     )
                 )
             )
+        # Set up the aggregator as though it had already seen a delete
+        # operation. Since the scan happens in reverse chronological
+        # order, the delete would have happened after the add, and we
+        # can ignore the files because the error has been corrected in
+        # a later patch.
+        self.aggregator._deleted_bad_uids.add(uid)
         results = list(self.aggregator.aggregate_changes(entry, changes))
         self.assertEqual([], results)
 
+    def test_add_multiple_without_delete(self):
+        # Adding multiple files in one commit using the same UID but
+        # different slug without a delete operation causes an
+        # exception.
+        entry = mock.Mock()
+        n = self.get_note_num()
+        uid = '%016x' % n
+        changes = []
+        for i in range(2):
+            name = 'prefix/add%d-%s.yaml' % (i, uid)
+            entry.commit.id = 'commit-id'
+            changes.append(
+                diff_tree.TreeChange(
+                    type=diff_tree.CHANGE_ADD,
+                    old=objects.TreeEntry(path=None, mode=None, sha=None),
+                    new=objects.TreeEntry(
+                        path=name.encode('utf-8'),
+                        mode='0222',
+                        sha='not-a-hash',
+                    )
+                )
+            )
+
+        # aggregate_changes() is a generator, so we have to wrap it in
+        # list() to process the data, so we need a little temporary
+        # function to do that and pass to assertRaises().
+        def get_results():
+            return list(self.aggregator.aggregate_changes(entry, changes))
+
+        self.assertRaises(
+            ValueError,
+            get_results,
+        )
+
     def test_delete(self):
         entry = mock.Mock()
         n = self.get_note_num()