Commit reordering: Avoid asking about the same conflict twice

It is fair to assume that the user never wants or expects
the sum of changes to be different after reordering them.
That would be an impure reordering,
which is not what the user is asking for.
Even if the conflicts are resolved by the user or git-rerere,
it is fair to assume such a result to be a failure.

This commit simply always upholds this invariant
for the practical benefit of not bothering the user
about a conflict with a known resolution.

I can hear critics say that this is an assumption,
and it's safer not to assume. The answer to that is plainly no:
There is only one correct solution to the second conflict given the first.
Not just in theory, but when you do this exercise day in and day out
and see that the second conflict is always a rearrangement of the first,
you want to spend your time on making the first one right.

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],
+    tree_to_keep: Optional[Tree] = None,
+) -> 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 tree_to_keep is not None:
+        tree = tree_to_keep
+    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,16 @@ def summary(self) -> str:
         )
         return " ".join(summary_paragraph.splitlines())
 
-    def rebase(self, parent: Optional[Commit]) -> Commit:
+    def rebase(
+        self,
+        parent: Optional[Commit],
+        tree_to_keep: Optional[Tree] = None,
+    ) -> 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, tree_to_keep)
 
     def update(
         self,

gitrevise/todo.py

@@ -244,11 +244,23 @@ def edit_todos(
 
 def apply_todos(
     current: Optional[Commit],
-    todos: List[Step],
+    todos_original: List[Step],
+    todos_edited: 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_state, step in zip(todos_original, todos_edited):
+        # Avoid making the user resolve the same conflict twice:
+        # When reordering commits, the final state is known.
+        applied_old_commits.add(known_state.commit.oid)
+        applied_new_commits.add(step.commit.oid)
+        deja_vu = applied_new_commits == applied_old_commits
+        tree_to_keep = known_state.commit.tree() if deja_vu else None
+
+        rebased = step.commit.rebase(current, tree_to_keep).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)

tests/test_rerere.py

@@ -37,12 +37,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}")
@@ -51,18 +49,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()
 
@@ -96,12 +92,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}")
 
@@ -124,15 +118,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 uncommitted_changes(repo) == ""
 
 
 def test_replay_resolution_recorded_by_git(repo: Repository) -> None:
@@ -154,25 +142,27 @@ 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.
+    # Test reusing the same recorded resolutions with git-revise.
+    # Except the last, that is: The total diff, hence the final state,
+    # of any reordering is a holy invariant.
     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]>