Skip to content

Avoid asking about the same conflict twice #133

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Avoid asking about the same conflict twice #133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
17c75d6
test_rerere: Skip already tested uncached case
anordal May 14, 2023
290c0bd
test_rerere: Reduce repetition by parameterizing
anordal May 14, 2023
07d1e2d
test_rerere: Also check commit content
anordal May 14, 2023
aef0a11
test_rerere: Compare hunks as strings
anordal May 14, 2023
f6a8a57
test_rerere: Test conflicting state between commits instead of after
anordal May 14, 2023
4ffe13f
test_rerere: Use an example that reorders to the same state
anordal May 14, 2023
f6bf9eb
Commit reordering: Avoid asking about the same conflict twice
anordal May 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions gitrevise/merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 []

Expand Down
8 changes: 6 additions & 2 deletions gitrevise/odb.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
18 changes: 15 additions & 3 deletions gitrevise/todo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

@krobelus krobelus Jun 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as an optimization we could add

if deja_vu:
	applied_old_commits.clear()
	applied_new_commits.clear()

Copy link
Contributor Author

@anordal anordal Jun 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I considered that. But should I, though? I figured I might as well optimized for size.

Copy link
Contributor

@krobelus krobelus Jun 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are more scenarios where we could skip a redundant conflict resolution.
Say for example we start with this todo list

A
unrelated
B

and edit it to

B'
A'
unrelated

applying B without A requires manual resolution but neither interacts with "unrelated".
Maybe we can resolve this mechanically by doing conceptually

B' # manual resolution
revert B'
A
B
unrelated

and then squashing the middle three.
Of course a proper solution would operate on the content-level, not on commits. I haven't done the math.. I wonder if anyone else is doing this

Copy link
Contributor Author

@anordal anordal Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Clever! But I think it would take some headscratching to do generally, and we haven't heard from @mystor yet. I don't know if it's better to keep it simple in this PR in hope of getting it somewhere. It's good enough for me, because when you know the limitations, you can usually get around it by reordering such "unrelated" commits separately.

But yeah, I think a set-difference could be used to figure out the commits and anti-commits needed in order to apply a commit to the same tree it came from and then reverse the damage afterwards. Sounds almost like a sound theory of patches!

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:
Expand Down
2 changes: 1 addition & 1 deletion gitrevise/tui.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Loading