Merge lp:~jelmer/brz-git/indexlocking into lp:brz-git

[Jelmer Vernooij]

Use proper index locking.

Use proper index locking.

[Jelmer Vernooij (jelmer)]

Rubberstamp! Proposer approves of own proposal.

review: Approve

=== modified file 'dir.py'
--- dir.py	2018-04-02 14:50:42 +0000
+++ dir.py	2018-04-02 21:59:37 +0000
@@ -537,14 +537,9 @@
         if not self._git.bare:
             from dulwich.errors import NoIndexPresent
             repo = self.find_repository()
-            try:
-                index = repo._git.open_index()
-            except NoIndexPresent:
-                pass
-            else:
-                from .workingtree import GitWorkingTree
-                branch = self.open_branch(ref=b'HEAD', nascent_ok=True)
-                return GitWorkingTree(self, repo, branch, index)
+            from .workingtree import GitWorkingTree
+            branch = self.open_branch(ref=b'HEAD', nascent_ok=True)
+            return GitWorkingTree(self, repo, branch)
         loc = urlutils.unescape_for_display(self.root_transport.base, 'ascii')
         raise bzr_errors.NoWorkingTree(loc)
 
@@ -584,15 +579,13 @@
         accelerator_tree=None, hardlink=False):
         if self._git.bare:
             raise bzr_errors.UnsupportedOperation(self.create_workingtree, self)
-        from dulwich.index import build_index_from_tree
         if from_branch is None:
             from_branch = self.open_branch(nascent_ok=True)
         if revision_id is None:
             revision_id = from_branch.last_revision()
         repo = self.find_repository()
         from .workingtree import GitWorkingTree
-        index = self._git.open_index()
-        wt = GitWorkingTree(self, repo, from_branch, index)
+        wt = GitWorkingTree(self, repo, from_branch)
         wt.set_last_revision(revision_id)
         wt._build_checkout_with_index()
         return wt

=== modified file 'tests/test_workingtree.py'
--- tests/test_workingtree.py	2018-03-26 00:30:51 +0000
+++ tests/test_workingtree.py	2018-04-02 21:59:37 +0000
@@ -52,6 +52,7 @@
         self.tree.add(['conflicted'])
         with self.tree.lock_tree_write():
             self.tree.index['conflicted'] = self.tree.index['conflicted'][:9] + (FLAG_STAGEMASK, )
+            self.tree._index_dirty = True
         conflicts = self.tree.conflicts()
         self.assertEqual(1, len(conflicts))
 
@@ -76,9 +77,10 @@
             tree_id = store[self.wt.branch.repository._git.head()].tree
         except KeyError:
             tree_id = None
-        changes, extras = changes_between_git_tree_and_working_copy(
-            store, tree_id, self.wt, want_unversioned=want_unversioned)
-        self.assertEqual(expected_changes, list(changes))
+        with self.wt.lock_read():
+            changes, extras = changes_between_git_tree_and_working_copy(
+                store, tree_id, self.wt, want_unversioned=want_unversioned)
+            self.assertEqual(expected_changes, list(changes))
         if expected_extras is None:
             expected_extras = set()
         self.assertEqual(set(expected_extras), set(extras))

=== modified file 'tree.py'
--- tree.py	2018-03-28 22:45:24 +0000
+++ tree.py	2018-04-02 21:59:37 +0000
@@ -741,6 +741,7 @@
         self._lock_mode = None
         self._lock_count = 0
         self._versioned_dirs = None
+        self._index_dirty = False
 
     def is_versioned(self, path):
         with self.lock_read():
@@ -813,6 +814,10 @@
                 raise errors.InvalidNormalization(path)
             self._index_add_entry(path, kind)
 
+    def _index_del_entry(self, path):
+        del self.index[path]
+        self._index_dirty = True
+
     def _index_add_entry(self, path, kind, flags=0):
         if not isinstance(path, basestring):
             raise TypeError(path)
@@ -852,6 +857,7 @@
             # TODO(jelmer): Why do we need to do this?
             trace.mutter('ignoring path with invalid newline in it: %r', path)
             return
+        self._index_dirty = True
         self.index[encoded_path] = index_entry_from_stat(
             stat_val, blob.id, flags)
         if self._versioned_dirs is not None:
@@ -948,13 +954,13 @@
         encoded_path = path.encode("utf-8")
         count = 0
         try:
-            del self.index[encoded_path]
+            self._index_del_entry(encoded_path)
         except KeyError:
             # A directory, perhaps?
             for p in list(self.index):
                 if p.startswith(encoded_path+b"/"):
                     count += 1
-                    del self.index[p]
+                    self._index_del_entry(p)
         else:
             count = 1
         self._versioned_dirs = None
@@ -975,7 +981,7 @@
         # TODO(jelmer): This shouldn't be called, it's inventory specific.
         for (old_path, new_path, file_id, ie) in delta:
             if old_path is not None and old_path.encode('utf-8') in self.index:
-                del self.index[old_path.encode('utf-8')]
+                self._index_del_entry(old_path.encode('utf-8'))
                 self._versioned_dirs = None
             if new_path is not None and ie.kind != 'directory':
                 self._index_add_entry(new_path, ie.kind)
@@ -1059,15 +1065,16 @@
                     raise
             if kind != 'directory':
                 try:
-                    del self.index[from_path]
+                    self._index_del_entry(from_path)
                 except KeyError:
                     pass
                 self._index_add_entry(to_rel, kind)
             else:
                 todo = [p for p in self.index if p.startswith(from_path+'/')]
                 for p in todo:
+                    self._index_dirty = True
                     self.index[posixpath.join(to_path, posixpath.relpath(p, from_path))] = self.index[p]
-                    del self.index[p]
+                    self._index_del_entry(p)
 
             self._versioned_dirs = None
             self.flush()

=== modified file 'workingtree.py'
--- workingtree.py	2018-03-27 03:09:48 +0000
+++ workingtree.py	2018-04-02 21:59:37 +0000
@@ -29,8 +29,10 @@
 from dulwich.ignore import (
     IgnoreFilterManager,
     )
+from dulwich.file import GitFile, FileLocked
 from dulwich.index import (
     Index,
+    SHA1Writer,
     build_index_from_tree,
     changes_from_tree,
     cleanup_mode,
@@ -41,6 +43,7 @@
     blob_from_path_and_stat,
     FLAG_STAGEMASK,
     validate_path,
+    write_index_dict,
     )
 from dulwich.object_store import (
     tree_lookup_path,
@@ -105,7 +108,7 @@
 class GitWorkingTree(MutableGitIndexTree,workingtree.WorkingTree):
     """A Git working tree."""
 
-    def __init__(self, controldir, repo, branch, index):
+    def __init__(self, controldir, repo, branch):
         MutableGitIndexTree.__init__(self)
         basedir = controldir.root_transport.local_abspath('.')
         self.basedir = osutils.realpath(basedir)
@@ -116,7 +119,8 @@
         self._branch = branch
         self._transport = controldir.transport
         self._format = GitWorkingTreeFormat()
-        self.index = index
+        self.index = None
+        self._index_file = None
         self.views = self._make_views()
         self._rules_searcher = None
         self._detect_case_handling()
@@ -128,6 +132,10 @@
     def supports_rename_tracking(self):
         return False
 
+    def _read_index(self):
+        self.index = Index(self.control_transport.local_abspath('index'))
+        self._index_dirty = False
+
     def lock_read(self):
         """Lock the repository for read operations.
 
@@ -136,7 +144,7 @@
         if not self._lock_mode:
             self._lock_mode = 'r'
             self._lock_count = 1
-            self.index.read()
+            self._read_index()
         else:
             self._lock_count += 1
         self.branch.lock_read()
@@ -147,7 +155,11 @@
         if not self._lock_mode:
             self._lock_mode = 'w'
             self._lock_count = 1
-            self.index.read()
+            try:
+                self._index_file = GitFile(self.control_transport.local_abspath('index'), 'wb')
+            except FileLocked:
+                raise errors.LockContention('index')
+            self._read_index()
         elif self._lock_mode == 'r':
             raise errors.ReadOnlyError(self)
         else:
@@ -177,6 +189,13 @@
     def get_physical_lock_status(self):
         return False
 
+    def break_lock(self):
+        try:
+            self.control_transport.delete('index.lock')
+        except errors.NoSuchFile:
+            pass
+        self.branch.break_lock()
+
     @only_raises(errors.LockNotHeld, errors.LockBroken)
     def unlock(self):
         if not self._lock_count:
@@ -186,7 +205,17 @@
             self._lock_count -= 1
             if self._lock_count > 0:
                 return
+            if self._index_file is not None:
+                if self._index_dirty:
+                    self._flush(self._index_file)
+                    self._index_file.close()
+                else:
+                    # Somebody else already wrote the index file
+                    # by calling .flush()
+                    self._index_file.abort()
+                self._index_file = None
             self._lock_mode = None
+            self.index = None
         finally:
             self.branch.unlock()
 
@@ -390,7 +419,6 @@
                 if message is not None:
                     trace.note(message)
             self._versioned_dirs = None
-            self.flush()
 
     def smart_add(self, file_list, recurse=True, action=None, save=True):
         if not file_list:
@@ -480,8 +508,6 @@
                         if save:
                             self._index_add_entry(subp, kind)
                         added.append(subp)
-            if added and save:
-                self.flush()
             return added, ignored
 
     def has_filename(self, filename):
@@ -525,10 +551,23 @@
                     yield p
 
     def flush(self):
-        # TODO: Maybe this should only write on dirty ?
         if self._lock_mode != 'w':
             raise errors.NotWriteLocked(self)
-        self.index.write()
+        # TODO(jelmer): This shouldn't be writing in-place, but index.lock is
+        # already in use and GitFile doesn't allow overriding the lock file name :(
+        f = open(self.control_transport.local_abspath('index'), 'wb')
+        # Note that _flush will close the file
+        self._flush(f)
+
+    def _flush(self, f):
+        try:
+            shaf = SHA1Writer(f)
+            write_index_dict(shaf, self.index)
+            shaf.close()
+        except:
+            f.abort()
+            raise
+        self._index_dirty = False
 
     def has_or_had_id(self, file_id):
         if self.has_id(file_id):
@@ -818,11 +857,11 @@
         with self.lock_tree_write():
             for path in self.index:
                 self._set_conflicted(path, path in by_path)
-            self.flush()
 
     def _set_conflicted(self, path, conflicted):
         trace.mutter('change conflict: %r -> %r', path, conflicted)
         value = self.index[path]
+        self._index_dirty = True
         if conflicted:
             self.index[path] = (value[:9] + (value[9] | FLAG_STAGEMASK, ))
         else:
@@ -838,7 +877,6 @@
                         raise errors.UnsupportedOperation(self.add_conflicts, self)
                 else:
                     raise errors.UnsupportedOperation(self.add_conflicts, self)
-            self.flush()
 
     def walkdirs(self, prefix=""):
         """Walk the directories of this tree.
@@ -1003,14 +1041,13 @@
         for (old_path, new_path, file_id, ie) in changes:
             if old_path is not None:
                 try:
-                    del self.index[old_path.encode('utf-8')]
+                    self._index_del_entry(old_path.encode('utf-8'))
                 except KeyError:
                     pass
                 else:
                     self._versioned_dirs = None
             if new_path is not None and ie.kind != 'directory':
                 self._index_add_entry(new_path, ie.kind)
-        self.flush()
 
     def annotate_iter(self, path, file_id=None,
                       default_revision=_mod_revision.CURRENT_REVISION):
@@ -1087,6 +1124,7 @@
             if revision_ids is not None:
                 self.set_parent_ids(revision_ids)
             self.index.clear()
+            self._index_dirty = True
             if self.branch.head is not None:
                 for entry in self.store.iter_tree_contents(self.store[self.branch.head].tree):
                     if not validate_path(entry.path):
@@ -1105,7 +1143,6 @@
                                   0, 0, len(obj.as_raw_string()), 0,
                                   0, 0))
                     self.index[entry.path] = index_entry_from_stat(st, entry.sha, 0)
-            self.flush()
 
     def pull(self, source, overwrite=False, stop_revision=None,
              change_reporter=None, possible_transports=None, local=False,
@@ -1161,13 +1198,11 @@
         """See WorkingTreeFormat.initialize()."""
         if not isinstance(a_controldir, LocalGitDir):
             raise errors.IncompatibleFormat(self, a_controldir)
-        index = Index(a_controldir.root_transport.local_abspath(".git/index"))
-        index.write()
         branch = a_controldir.open_branch(nascent_ok=True)
         if revision_id is not None:
             branch.set_last_revision(revision_id)
         wt = GitWorkingTree(
-                a_controldir, a_controldir.open_repository(), branch, index)
+                a_controldir, a_controldir.open_repository(), branch)
         for hook in MutableTree.hooks['post_build_tree']:
             hook(wt)
         return wt