Commit reordering: Avoid asking about the same conflict twice

Implements #132

Author: anordal

gitrevise/merge.py

@@ -29,7 +29,11 @@ class MergeConflict(Exception):
     pass
 
 
-def rebase(commit: Commit, new_parent: Optional[Commit]) -> Commit:
+def rebase(
+    commit: Commit,
+    new_parent: Optional[Commit],
+    known_state: Optional[Commit],
+) -> Commit:
     repo = commit.repo
 
     orig_parent = commit.parent() if not commit.is_root else None
@@ -43,13 +47,16 @@ def get_summary(cmt: Optional[Commit]) -> str:
     def get_tree(cmt: Optional[Commit]) -> Tree:
         return cmt.tree() if cmt is not None else Tree(repo, b"")
 
-    tree = merge_trees(
-        Path(),
-        (get_summary(new_parent), get_summary(orig_parent), get_summary(commit)),
-        get_tree(new_parent),
-        get_tree(orig_parent),
-        get_tree(commit),
-    )
+    if known_state is not None:
+        tree = get_tree(known_state)
+    else:
+        tree = merge_trees(
+            Path(),
+            (get_summary(new_parent), get_summary(orig_parent), get_summary(commit)),
+            get_tree(new_parent),
+            get_tree(orig_parent),
+            get_tree(commit),
+        )
 
     new_parents = [new_parent] if new_parent is not None else []
 

gitrevise/odb.py

@@ -616,12 +616,12 @@ def summary(self) -> str:
         )
         return " ".join(summary_paragraph.splitlines())
 
-    def rebase(self, parent: Optional[Commit]) -> Commit:
+    def rebase(self, parent: Optional[Commit], known_state: Optional[Commit]) -> Commit:
         """Create a new commit with the same changes, except with ``parent``
         as its parent. If ``parent`` is ``None``, this becomes a root commit."""
         from .merge import rebase  # pylint: disable=import-outside-toplevel
 
-        return rebase(self, parent)
+        return rebase(self, parent, known_state)
 
     def update(
         self,

gitrevise/todo.py

@@ -244,11 +244,23 @@ def edit_todos(
 
 def apply_todos(
     current: Optional[Commit],
+    original: List[Step],
     todos: List[Step],
     reauthor: bool = False,
 ) -> Commit:
-    for step in todos:
-        rebased = step.commit.rebase(current).update(message=step.message)
+    applied_old_commits = set()
+    applied_new_commits = set()
+
+    for known, step in zip(original, todos):
+        # Avoid making the user resolve the same conflict twice:
+        # When reordering commits, the final state is known.
+        applied_old_commits.add(known.commit.oid)
+        applied_new_commits.add(step.commit.oid)
+        is_known = applied_new_commits == applied_old_commits
+        known_state = known.commit if is_known else None
+
+        rebased = step.commit.rebase(current, known_state).update(message=step.message)
+
         if step.kind == StepKind.PICK:
             current = rebased
         elif step.kind == StepKind.FIXUP:

gitrevise/tui.py

@@ -138,7 +138,7 @@ def interactive(
 
     if todos != original:
         # Perform the todo list actions.
-        new_head = apply_todos(base, todos, reauthor=args.reauthor)
+        new_head = apply_todos(base, original, todos, reauthor=args.reauthor)
 
         # Update the value of HEAD to the new state.
         update_head(head, new_head, None)
@@ -183,8 +183,8 @@ def noninteractive(
     final = head.target.tree()
     if staged:
         print(f"Applying staged changes to '{args.target}'")
-        current = current.update(tree=staged.rebase(current).tree())
-        final = staged.rebase(head.target).tree()
+        current = current.update(tree=staged.rebase(current, known_state=None).tree())
+        final = staged.rebase(head.target, known_state=None).tree()
 
     # Update the commit message on the target commit if requested.
     if args.message:
@@ -217,7 +217,7 @@ def noninteractive(
         for commit in to_rebase:
             if repo.sign_commits != bool(commit.gpgsig):
                 commit = commit.update(recommit=True)
-            current = commit.rebase(current)
+            current = commit.rebase(current, known_state=None)
             print(f"{current.oid.short()} {current.summary()}")
 
         update_head(head, current, final)

tests/test_rerere.py

@@ -36,12 +36,10 @@ def test_reuse_recorded_resolution(
     history_with_two_conflicting_commits(auto_update=auto_update)
 
     # Uncached case: Record the user's resolution (in .git/rr-cache/*/preimage).
-    with editor_main(("-i", "HEAD~~"), input=b"y\n" * 4) as ed:
+    with editor_main(("-i", "HEAD~~"), input=b"y\n" * 2) as ed:
         flip_last_two_commits(repo, ed)
         with ed.next_file() as f:
             f.replace_dedent("spam\n")
-        with ed.next_file() as f:
-            f.replace_dedent("eggs spam\n")
 
     tree_after_resolving_conflicts = repo.get_commit("HEAD").tree()
     bash("git reset --hard HEAD@{1}")
@@ -50,18 +48,16 @@ def test_reuse_recorded_resolution(
     acceptance_input = None
     intermediate_state = "spam"
     if not auto_update:
-        acceptance_input = b"y\n" * 2
+        acceptance_input = b"y\n"
         if custom_resolution is not None:
-            acceptance_input = b"n\n" + b"y\n" * 4
+            acceptance_input = b"n\n" + b"y\n" * 2
             intermediate_state = custom_resolution
 
     with editor_main(("-i", "HEAD~~"), input=acceptance_input) as ed:
         flip_last_two_commits(repo, ed)
         if custom_resolution is not None:
             with ed.next_file() as f:
                 f.replace_dedent(custom_resolution + "\n")
-            with ed.next_file() as f:
-                f.replace_dedent("eggs spam\n")
 
     assert tree_after_resolving_conflicts == repo.get_commit("HEAD").tree()
 
@@ -95,12 +91,10 @@ def test_rerere_merge(repo: Repository) -> None:
     bash("git commit -am 'commit 2'")
 
     # Record a resolution for changing the order of two commits.
-    with editor_main(("-i", "HEAD~~"), input=b"y\ny\ny\ny\n") as ed:
+    with editor_main(("-i", "HEAD~~"), input=b"y\ny\n") as ed:
         flip_last_two_commits(repo, ed)
         with ed.next_file() as f:
             f.replace_dedent(b"resolved1\n" + 9 * b"x\n")
-        with ed.next_file() as f:
-            f.replace_dedent(b"resolved2\n" + 9 * b"x\n")
     # Go back to the old history so we can try replaying the resolution.
     bash("git reset --hard HEAD@{1}")
 
@@ -123,15 +117,9 @@ def test_rerere_merge(repo: Repository) -> None:
         """\
             @@ -1 +1 @@
             -resolved1
-            +resolved2"""
-    )
-    leftover_index = hunks(repo.git("diff", "-U0", "HEAD"))
-    assert leftover_index == dedent(
-        """\
-        @@ -1 +1 @@
-        -resolved2
-        +original2"""
+            +original2"""
     )
+    assert leftover_index(repo.git("diff", "-U0", "HEAD")) == ""
 
 
 def test_replay_resolution_recorded_by_git(repo: Repository) -> None:
@@ -151,25 +139,28 @@ def test_replay_resolution_recorded_by_git(repo: Repository) -> None:
         """
     )
 
-    # Now let's try to do the same thing with git-revise, reusing the recorded resolution.
+    # Git-revise may well reuse an intermediate state resolution,
+    # but actually knows that nobody expects the final state to change
+    # when just changing the order it's done: Whenever the final state
+    # is known, that pointless second conflict is bypassed.
     with editor_main(("-i", "HEAD~~")) as ed:
         flip_last_two_commits(repo, ed)
 
     assert repo.git("log", "-p", trim_newline=False).decode() == dedent(
         """\
-        commit 44fdce0cf7ae75ed5edac5f3defed83cddf3ec4a
+        commit 31aa1057aca9f039e997fff9396fb9656fb4c01c
         Author: Bash Author <[email protected]>
         Date:   Thu Jul 13 21:40:00 2017 -0500
 
             add eggs
 
         diff --git a/file b/file
-        index 5d0f8a8..cb90548 100644
+        index 5d0f8a8..2481b83 100644
         --- a/file
         +++ b/file
         @@ -1 +1 @@
         -intermediate state
-        +something completely different
+        +eggs spam
 
         commit 1fa5135a6cce1f63dc2f5584ee68e15a4de3a99c
         Author: Bash Author <[email protected]>