Skip to content
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.

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

Avoid asking about the same conflict twice #133

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

Conversation

anordal
Copy link
Contributor

@anordal anordal commented May 14, 2023

Implements #132.

This PR also contains some test prework that can go in first if wanted.

@anordal anordal force-pushed the avoid-asking-about-the-same-conflict-twice branch from 11ad0b0 to 1859fd4 Compare May 14, 2023 22:16
tests/test_rerere.py Outdated Show resolved Hide resolved
gitrevise/todo.py Outdated Show resolved Hide resolved
gitrevise/todo.py Outdated Show resolved Hide resolved
gitrevise/todo.py Outdated Show resolved Hide resolved
@anordal anordal force-pushed the avoid-asking-about-the-same-conflict-twice branch 2 times, most recently from 8a942dd to c467c1f Compare May 14, 2023 22:42
gitrevise/merge.py Outdated Show resolved Hide resolved
The uncached case is effectively also tested
at the beginning of the function.
Reduce repetition by testing this part first.
Apologies for the pytest magic.
This makes changing the tests less repetitive.
Just for good measure, since there is now one place to add it.
@anordal
Copy link
Contributor Author

anordal commented May 29, 2023

(Let me squash my fixups so far. State before: 4da9a43)

@anordal anordal force-pushed the avoid-asking-about-the-same-conflict-twice branch from 4da9a43 to e22fad5 Compare May 29, 2023 19:13
@krobelus
Copy link
Contributor

I think it's fine to always squash fixups eagerly. People who have remembered a previous state can use git range-diff

@anordal
Copy link
Contributor Author

anordal commented Jun 8, 2023

Ready for more feedback. I would think this is ready to take if anyone wants it.

I've switched to using this branch for my own use.

# 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

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

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

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

@krobelus
Copy link
Contributor

looks great, thanks! I'm using it as well, it's very handy

More readable on failure than bytes.
Move this test aspect out of the way, so the end state can be kept.

Prework for mystor#132
Change the example so that leftover_index becomes empty.

To make leftover_index empty, i.e. keep the final state unchanged
as a sum of both changes, it makes more conceptual sense
when one commit does not effectively revert the other.

That should also be more representative of real commits that can
sensibly be reordered.

Prework for mystor#132
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 mystor#132
tests/test_rerere.py Outdated Show resolved Hide resolved
@anordal anordal force-pushed the avoid-asking-about-the-same-conflict-twice branch from e22fad5 to f6bf9eb Compare July 28, 2023 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants