-
Notifications
You must be signed in to change notification settings - Fork 8
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 diff3 support #498
Add diff3 support #498
Conversation
Hi @wetneb, Sorry about the complete blackout in communications here, I've been busy with other things. I don't think there is any good reason to merge this before there's at least a POC of outputting the base revision for the structural merge, but I don't mind leaving the PR open for some time to see if anyone is interested in looking into it. On that note, I outlined some potential solutions to outputting the base revision in #496 (comment). |
Hi @slarse, I have a solution that is almost ready, but it surfaced a bug in jgit's own implementation of diff3: eclipse-jgit/jgit#38. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #498 +/- ##
============================================
- Coverage 82.70% 82.60% -0.10%
- Complexity 363 369 +6
============================================
Files 43 43
Lines 1769 1771 +2
Branches 303 313 +10
============================================
Hits 1463 1463
- Misses 180 182 +2
Partials 126 126 ☔ View full report in Codecov by Sentry. |
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.
Here is a working version, now that the fixed jgit has been released.
val otherPred = delta.getOtherPredecessors(currentPcs).firstOrNull() | ||
if (otherPred != null) { | ||
remainingInconsistencies.remove(otherPred) | ||
return nodes |
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.
This is quite a simplification from the original so I suspect I might have overseen something here. Curious to know what you think.
@slarse ping? |
@monperrus / @wetneb I don't think I will get around to integrating this. I've lost too much understanding of the internal workings of Spork over the years. I haven't written Java or Kotlin for years, either. I put a couple of hours going over this and trying to evaluate it, but I'm just not up for the task right now. Given how my open source commitment has diminished in the past years, I don't think I'll ever be. My career is taking me other places. @monperrus For the same reasons I stepped down from Spoon, I cannot put much time into maintaining Spork at this time. I can do small things, I can answer questions and provide what insights I remember, but for Spork to live on and keep growing, it needs a new maintainer. If you can staff one, feel free to treat the project as the sole property of the ASSERT group, I make no claims of ownership. But I feel I have done my part. @wetneb I'm truly sorry to have kept you waiting all this time just to get back to you with "I can't do it". It must be disappointing. Trying to integrate this change a few months back, failing, and then completely sticking my head in the sand was quite the wake-up call for me. I hope @monperrus can find someone to integrate this, otherwise you can always fork Spork (hah!) and carry on. |
@slarse I fully understand. thanks for all the effort you put in Spoon and Spork. |
thanks a lot @wetneb, that's a super useful feature. could you also document the new feature in the README? Thanks! |
For #496.
I have added test cases (failing) with some intuitive expectations of what I would find useful as a user. I haven't thought super hard about it so I expect some cases might need adjusting.
No idea if it can be implemented, but I guess if someone wants to have a go at this, those changes should be a good start and they would only need to figure out the clever part of the algorithm.