From 969ea59f67b17fdd3d89b8f9841e029235be8982 Mon Sep 17 00:00:00 2001 From: Jason Mansour Date: Mon, 11 Apr 2022 22:03:57 +0200 Subject: [PATCH 1/5] Deal with modify/delete conflicts --- nbgitpuller/pull.py | 71 +++++++++++++++++++++++++++++++---------- tests/test_gitpuller.py | 45 ++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 16 deletions(-) diff --git a/nbgitpuller/pull.py b/nbgitpuller/pull.py index 900dc0d6..c489c141 100644 --- a/nbgitpuller/pull.py +++ b/nbgitpuller/pull.py @@ -45,6 +45,10 @@ def flush(): raise subprocess.CalledProcessError(ret, cmd) +class ModifyDeleteConflict(Exception): + pass + + class GitPuller(Configurable): depth = Integer( config=True, @@ -249,6 +253,48 @@ 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 the branch from origin into the current branch. If there is a + conflict that needs resolution, a ModifyDeleteConflict exception is raised. + """ + try: + # Not using execute_cmd, because we need to get the output + # when there is an error + lines = subprocess.check_output([ + 'git', + '-c', 'user.email=nbgitpuller@nbgitpuller.link', + '-c', 'user.name=nbgitpuller', + 'merge', + '-Xours', 'origin/{}'.format(self.branch_name) + ], + cwd=self.repo_dir, + stderr=subprocess.STDOUT, + env={**os.environ, 'LANG': 'C'}).decode().splitlines() + for line in lines: + yield line + + except subprocess.CalledProcessError as exc: + for line in exc.output.decode().splitlines(): + yield line + + if exc.output.decode().startswith("CONFLICT (modify/delete)"): + raise ModifyDeleteConflict() from exc + raise + + def commit_all(self): + """ + Creates a new commit with all the changes + """ + 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) + def update(self): """ Do the pulling if necessary @@ -281,24 +327,17 @@ def update(self): # 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) + try: + yield from self.merge() + except ModifyDeleteConflict: + 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 main(): diff --git a/tests/test_gitpuller.py b/tests/test_gitpuller.py index cdc00735..4ab35b29 100644 --- a/tests/test_gitpuller.py +++ b/tests/test_gitpuller.py @@ -421,6 +421,51 @@ 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_delete_locally_and_remotely(): """ Test that sync works after deleting a file locally and remotely From 9349e3cb5e352ba33dfd6e3b9b3f3cbcbf300c93 Mon Sep 17 00:00:00 2001 From: Jason Mansour Date: Tue, 19 Apr 2022 14:56:41 +0200 Subject: [PATCH 2/5] Add test: handle conflict when not in first line --- tests/test_gitpuller.py | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/test_gitpuller.py b/tests/test_gitpuller.py index 4ab35b29..b526796c 100644 --- a/tests/test_gitpuller.py +++ b/tests/test_gitpuller.py @@ -466,6 +466,40 @@ def test_diverged_reverse(): 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 From 1c5fcbb6b8ab962900cc199c80b1b6101d1ca990 Mon Sep 17 00:00:00 2001 From: Jason Mansour Date: Tue, 19 Apr 2022 14:57:41 +0200 Subject: [PATCH 3/5] Handle modify/delete conflict if there is other output --- nbgitpuller/pull.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/nbgitpuller/pull.py b/nbgitpuller/pull.py index c489c141..068b8d18 100644 --- a/nbgitpuller/pull.py +++ b/nbgitpuller/pull.py @@ -275,11 +275,13 @@ def merge(self): yield line except subprocess.CalledProcessError as exc: - for line in exc.output.decode().splitlines(): + lines = exc.output.decode().splitlines() + for line in lines: yield line - if exc.output.decode().startswith("CONFLICT (modify/delete)"): - raise ModifyDeleteConflict() from exc + for line in lines: + if line.startswith("CONFLICT (modify/delete)"): + raise ModifyDeleteConflict() from exc raise def commit_all(self): From 599b7a408e6310e2bd701419ed1bed538206bc5f Mon Sep 17 00:00:00 2001 From: Jason Mansour Date: Tue, 19 Apr 2022 14:58:04 +0200 Subject: [PATCH 4/5] Add newlines to git output --- nbgitpuller/pull.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/nbgitpuller/pull.py b/nbgitpuller/pull.py index 068b8d18..904ed52c 100644 --- a/nbgitpuller/pull.py +++ b/nbgitpuller/pull.py @@ -272,12 +272,12 @@ def merge(self): stderr=subprocess.STDOUT, env={**os.environ, 'LANG': 'C'}).decode().splitlines() for line in lines: - yield line + yield line + '\n' except subprocess.CalledProcessError as exc: lines = exc.output.decode().splitlines() for line in lines: - yield line + yield line + '\n' for line in lines: if line.startswith("CONFLICT (modify/delete)"): From 67dfbd5df55fb0f8dd4cf87750b93eead0f9c7d2 Mon Sep 17 00:00:00 2001 From: YuviPanda Date: Mon, 13 Jun 2022 07:21:41 +0530 Subject: [PATCH 5/5] Don't use Exceptions for flow control Instead, move the commit step into the merge function, as it is not called anywhere else --- nbgitpuller/pull.py | 74 +++++++++++++++++++++------------------------ 1 file changed, 34 insertions(+), 40 deletions(-) diff --git a/nbgitpuller/pull.py b/nbgitpuller/pull.py index 904ed52c..048cab8c 100644 --- a/nbgitpuller/pull.py +++ b/nbgitpuller/pull.py @@ -45,10 +45,6 @@ def flush(): raise subprocess.CalledProcessError(ret, cmd) -class ModifyDeleteConflict(Exception): - pass - - class GitPuller(Configurable): depth = Integer( config=True, @@ -255,45 +251,57 @@ def rename_local_untracked(self): def merge(self): """ - Merges the branch from origin into the current branch. If there is a - conflict that needs resolution, a ModifyDeleteConflict exception is raised. + 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: - # Not using execute_cmd, because we need to get the output - # when there is an error - lines = subprocess.check_output([ + 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, - stderr=subprocess.STDOUT, - env={**os.environ, 'LANG': 'C'}).decode().splitlines() - for line in lines: - yield line + '\n' - - except subprocess.CalledProcessError as exc: - lines = exc.output.decode().splitlines() - for line in lines: - yield line + '\n' - - for line in lines: + cwd=self.repo_dir): + yield line + # Detect conflict caused by one branch if line.startswith("CONFLICT (modify/delete)"): - raise ModifyDeleteConflict() from exc - raise + 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 the changes + 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) @@ -319,27 +327,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 self.commit_all() - + # Merge master into local! yield from self.ensure_lock() - try: - yield from self.merge() - except ModifyDeleteConflict: - 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() + yield from self.merge() def main():