-
Notifications
You must be signed in to change notification settings - Fork 31
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
base: main
Are you sure you want to change the base?
Avoid asking about the same conflict twice #133
Conversation
11ad0b0
to
1859fd4
Compare
8a942dd
to
c467c1f
Compare
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.
(Let me squash my fixups so far. State before: 4da9a43) |
4da9a43
to
e22fad5
Compare
I think it's fine to always squash fixups eagerly. People who have remembered a previous state can use |
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 |
There was a problem hiding this comment.
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()
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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
e22fad5
to
f6bf9eb
Compare
Implements #132.
This PR also contains some test prework that can go in first if wanted.