From c467c1fbed35f6caeccffc5bc27c4d9b1d410dc4 Mon Sep 17 00:00:00 2001 From: Andreas Nordal Date: Wed, 10 May 2023 00:59:24 +0200 Subject: [PATCH] Commit reordering: Avoid asking about the same conflict twice Implements https://github.com/mystor/git-revise/issues/132 --- gitrevise/merge.py | 23 +++++++++++++++-------- gitrevise/odb.py | 4 ++-- gitrevise/todo.py | 16 ++++++++++++++-- gitrevise/tui.py | 8 ++++---- tests/test_rerere.py | 35 +++++++++++++---------------------- 5 files changed, 48 insertions(+), 38 deletions(-) diff --git a/gitrevise/merge.py b/gitrevise/merge.py index 0a27beb..4534c99 100644 --- a/gitrevise/merge.py +++ b/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 [] diff --git a/gitrevise/odb.py b/gitrevise/odb.py index 641ad70..07e18fd 100644 --- a/gitrevise/odb.py +++ b/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, diff --git a/gitrevise/todo.py b/gitrevise/todo.py index 08b312c..6fc5faf 100644 --- a/gitrevise/todo.py +++ b/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: diff --git a/gitrevise/tui.py b/gitrevise/tui.py index 4c24cd9..ef73360 100644 --- a/gitrevise/tui.py +++ b/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) diff --git a/tests/test_rerere.py b/tests/test_rerere.py index 350fcc8..4a49957 100644 --- a/tests/test_rerere.py +++ b/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,9 +49,9 @@ 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: @@ -61,8 +59,6 @@ def test_reuse_recorded_resolution( 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 leftover_index(repo.git("diff", "-U0", "HEAD")) == "" def test_replay_resolution_recorded_by_git(repo: Repository) -> None: @@ -154,25 +142,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 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