-
Notifications
You must be signed in to change notification settings - Fork 71
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
Add a flag to fix past merges. #89
base: master
Are you sure you want to change the base?
Conversation
This is a simple implementation where a commit chain A -> {B, C} -> merge commit -> D is just treated as A -> B -> C -> D for the purposes of absorb.
instinctively i think this is unsound. what if the merge is nontrivial and contains conflict resolutions? modifying B/C (the parents of the merge) might invalidate the resolution or introduce new conflicts. have you tested such a case? |
Those sound like reasonable concerns. Here's an algorithm that should address that. If you think it sounds reasonable I might implement it, but it does seem like a lot of work. Given a tree:
We would treat it internally as:
We would only absorb if:
This would mean that:
|
i'm going to be honest with you: convincing me that the described algorithm is correct will be as hard, possibly harder, than actually implementing it. which is why i didn't even attempt to support merges in the current design. can you demonstrate with a worked example? like, patches with line offsets.
i'm not sure i follow this sentence. if both A and B fail to commute, they have a common ancestor (base). can you rigorously define "most common"? |
Oh, sorry, most common child, not most common ancestor. Let's only work with a single file, since we only need a single file Base:
A:
B:
merged:
I'll give you a more concrete algorithm with a slight modification - some pseudocode for the algorithm in something between rust and python
In all following examples, we will be writing Examples:
Hunk containing changes between
Hunk containing changes between
|
hmmm, okay. this looks like something i could see myself accepting. i won't take this pr as-is, but you're welcome to start hacking on the algorithm (or throw your hands up in the air and say "nah it's too much work", in which case i'll close this pr and move the description to a new issue). i think the only detail that's missing from the pseudocode is that all merge parents could return the same result (eg |
Maybe this is answered in the pseudo-code, but I'm curious exactly how you'll decide the order of the two commits that are being merged. Naively, I think the commit with the older commit date could go first. I think commit date is better than author date because it's a more likely indication on conflict resolution. A commit that was authored 12 months ago and committed 12 months ago is more likely to have conflicts if applied on top of a commit made 2 days ago (regardless of author date), compared to applying a commit from 2 days ago on top of a commit made 12 months ago. Given this, we now have a reasonable heuristic for choosing which commit goes first. The second commit is then just the diff of A and the merge commit. |
@tummychow Good catch, I hadn't considered that. Yes, it should be as simple as @devinrhode2 I believe you're misunderstanding something, but it's hard to say exactly what without an example of what you think would happen. If it helps clarify anything, we specifically do not perform any merges, and absorb will never create a fixup unless the following |
This is a simple implementation where a commit chain A -> {B, C} -> merge commit -> D is just treated as A -> B -> C -> D for the purposes of absorb.