Merge pull request #269 from jdmansour/handle-modify-delete-conflicts

Deal with modify/delete conflicts

nbgitpuller/pull.py

@@ -244,6 +244,62 @@ def rename_local_untracked(self):
                 os.rename(f, new_file_name)
                 yield 'Renamed {} to {} to avoid conflict with upstream'.format(f, new_file_name)
 
+    def merge(self):
+        """
+        Merges branch from origin into current branch, resolving conflicts when possible.
+
+        Resolves conflicts in two ways:
+
+        - Passes `-Xours` to git, setting merge-strategy to preserve changes made by the
+          user whererver possible
+        - Detect (modify/delete) conflicts, where the user has locally modified something
+          that was deleted upstream. We just keep the local file.
+        """
+        modify_delete_conflict = False
+        try:
+            for line in execute_cmd([
+                'git',
+                '-c', 'user.email=nbgitpuller@nbgitpuller.link',
+                '-c', 'user.name=nbgitpuller',
+                'merge',
+                '-Xours', 'origin/{}'.format(self.branch_name)
+            ],
+            cwd=self.repo_dir):
+                yield line
+                # Detect conflict caused by one branch
+                if line.startswith("CONFLICT (modify/delete)"):
+                    modify_delete_conflict = True
+        except subprocess.CalledProcessError:
+            if not modify_delete_conflict:
+                raise
+
+        if modify_delete_conflict:
+            yield "Caught modify/delete conflict, trying to resolve"
+            # If a file was deleted on one branch, and modified on another,
+            # we just keep the modified file.  This is done by `git add`ing it.
+            yield from self.commit_all()
+
+    def commit_all(self):
+        """
+        Creates a new commit with all current changes
+        """
+        yield from execute_cmd([
+            'git',
+            # We explicitly set user info of the commits we are making, to keep that separate from
+            # whatever author info is set in system / repo config by the user. We pass '-c' to git
+            # itself (rather than to 'git commit') to temporarily set config variables. This is
+            # better than passing --author, since git treats author separately from committer.
+            '-c', 'user.email=nbgitpuller@nbgitpuller.link',
+            '-c', 'user.name=nbgitpuller',
+            'commit',
+            '-am', 'Automatic commit by nbgitpuller',
+            # We allow empty commits. On NFS (at least), sometimes repo_is_dirty returns a false
+            # positive, returning True even when there are no local changes (git diff-files seems to return
+            # bogus output?). While ideally that would not happen, allowing empty commits keeps us
+            # resilient to that issue.
+            '--allow-empty'
+        ], cwd=self.repo_dir)
+
     def update(self):
         """
         Do the pulling if necessary
@@ -266,34 +322,13 @@ def update(self):
         yield from execute_cmd(['git', 'reset', '--mixed'], cwd=self.repo_dir)
 
         # If there are local changes, make a commit so we can do merges when pulling
-        # We also allow empty commits. On NFS (at least), sometimes repo_is_dirty returns a false
-        # positive, returning True even when there are no local changes (git diff-files seems to return
-        # bogus output?). While ideally that would not happen, allowing empty commits keeps us
-        # resilient to that issue.
-        # We explicitly set user info of the commits we are making, to keep that separate from
-        # whatever author info is set in system / repo config by the user. We pass '-c' to git
-        # itself (rather than to 'git commit') to temporarily set config variables. This is
-        # better than passing --author, since git treats author separately from committer.
         if self.repo_is_dirty():
             yield from self.ensure_lock()
-            yield from execute_cmd([
-                'git',
-                '-c', 'user.email=nbgitpuller@nbgitpuller.link',
-                '-c', 'user.name=nbgitpuller',
-                'commit',
-                '-am', 'Automatic commit by nbgitpuller',
-                '--allow-empty'
-            ], cwd=self.repo_dir)
+            yield from self.commit_all()
 
         # Merge master into local!
         yield from self.ensure_lock()
-        yield from execute_cmd([
-            'git',
-            '-c', 'user.email=nbgitpuller@nbgitpuller.link',
-            '-c', 'user.name=nbgitpuller',
-            'merge',
-            '-Xours', 'origin/{}'.format(self.branch_name)
-        ], cwd=self.repo_dir)
+        yield from self.merge()
 
 
 def main():

tests/test_gitpuller.py

@@ -355,6 +355,85 @@ def test_delete_remotely_modify_locally():
             assert puller.read_file('README.md') == 'HELLO'
 
 
+def test_diverged():
+    """
+    Test deleting a file upstream, and editing it locally.  This time we
+    commit to create diverged brances.
+    """
+    with Remote() as remote, Pusher(remote) as pusher:
+        pusher.push_file('README.md', 'new')
+
+        with Puller(remote) as puller:
+            # Delete the file remotely
+            pusher.git('rm', 'README.md')
+            pusher.git('commit', '-m', 'Deleted file')
+            pusher.git('push', '-u', 'origin', 'master')
+
+            # Edit locally
+            puller.write_file('README.md', 'conflict')
+            puller.git('add', 'README.md')
+            puller.git('commit', '-m', 'Make conflicting change')
+
+            # The local change should be kept
+            puller.pull_all()
+            assert puller.read_file('README.md') == 'conflict'
+
+
+def test_diverged_reverse():
+    """
+    Test deleting a file locally, and editing it upstream.  We commit the changes
+    to create diverged branches.  Like `test_diverged`, but flipped. 
+    """
+    with Remote() as remote, Pusher(remote) as pusher:
+        pusher.push_file('README.md', 'new')
+
+        with Puller(remote) as puller:
+            # Delete the file locally
+            puller.git('rm', 'README.md')
+            puller.git('commit', '-m', 'Deleted file')
+
+            # Edit the file remotely
+            pusher.push_file('README.md', 'conflicting change')
+
+            # Pulling should get the latest version of the file
+            puller.pull_all()
+            assert(puller.read_file('README.md') == 'conflicting change')
+
+
+def test_diverged_multiple():
+    """
+    Test deleting a file upstream, and editing it locally.  We commit the changes
+    to create diverged branches.
+
+    Use two files, so git merge doesn't mention the conflict in the first line.
+
+        puller: Auto-merging AFILE.txt
+        puller: CONFLICT (modify/delete): BFILE.txt deleted in origin/master and modified in HEAD.  Version HEAD of BFILE.txt left in tree.
+        puller: Automatic merge failed; fix conflicts and then commit the result.
+
+    """
+    with Remote() as remote, Pusher(remote) as pusher:
+        pusher.push_file('AFILE.txt', 'new')
+        pusher.push_file('BFILE.txt', 'new')
+
+        with Puller(remote) as puller:
+            # Remote changes - BFILE.txt deleted    
+            pusher.write_file('AFILE.txt', 'changed remotely')
+            pusher.git('add', 'AFILE.txt')
+            pusher.git('rm', 'BFILE.txt')
+            pusher.git('commit', '-m', 'Remote changes')
+            pusher.git('push', '-u', 'origin', 'master')
+
+            # Local changes - BFILE.txt edited
+            puller.write_file('AFILE.txt', 'edited')
+            puller.write_file('BFILE.txt', 'edited')
+            puller.git('commit', '-am', 'Make conflicting change')
+
+            puller.pull_all()
+            assert puller.read_file('AFILE.txt') == 'edited'
+            assert puller.read_file('BFILE.txt') == 'edited'
+
+
 def test_delete_locally_and_remotely():
     """
     Test that sync works after deleting a file locally and remotely