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

Add diff3 support #498

Merged
merged 3 commits into from
Oct 10, 2024
Merged

Add diff3 support #498

merged 3 commits into from
Oct 10, 2024

Conversation

wetneb
Copy link
Contributor

@wetneb wetneb commented Feb 14, 2024

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.

@slarse
Copy link
Collaborator

slarse commented Apr 6, 2024

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).

@wetneb
Copy link
Contributor Author

wetneb commented Apr 6, 2024

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.
This PR is therefore blocked by that bug, for which I have a pending fix: https://eclipse.gerrithub.io/c/eclipse-jgit/jgit/+/1177976.
Once this is merged and released, this PR can then go ahead.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2024

Codecov Report

Attention: Patch coverage is 87.37864% with 13 lines in your changes missing coverage. Please review.

Project coverage is 82.60%. Comparing base (ba76361) to head (6d9343a).
Report is 9 commits behind head on master.

Files Patch % Lines
...e/kth/spork/spoon/printer/PrinterPreprocessor.java 78.26% 2 Missing and 3 partials ⚠️
...rc/main/kotlin/se/kth/spork/util/LineBasedMerge.kt 57.14% 2 Missing and 1 partial ⚠️
...se/kth/spork/spoon/printer/SporkPrettyPrinter.java 90.47% 1 Missing and 1 partial ⚠️
src/main/java/se/kth/spork/cli/Cli.java 90.00% 1 Missing ⚠️
.../kth/spork/spoon/conflict/CommentContentHandler.kt 50.00% 0 Missing and 1 partial ⚠️
...kth/spork/spoon/pcsinterpreter/SporkTreeBuilder.kt 96.00% 0 Missing and 1 partial ⚠️
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.
📢 Have feedback on the report? Share it here.

@wetneb wetneb marked this pull request as ready for review June 16, 2024 19:36
Copy link
Contributor Author

@wetneb wetneb left a 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
Copy link
Contributor Author

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.

@wetneb wetneb changed the title Add scaffold and test cases for diff3 support Add diff3 support Jun 17, 2024
@monperrus
Copy link
Collaborator

@slarse ping?

@slarse
Copy link
Collaborator

slarse commented Oct 9, 2024

@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.

@monperrus
Copy link
Collaborator

@slarse I fully understand. thanks for all the effort you put in Spoon and Spork.

@monperrus monperrus merged commit 392e743 into ASSERT-KTH:master Oct 10, 2024
4 checks passed
@monperrus
Copy link
Collaborator

thanks a lot @wetneb, that's a super useful feature.

could you also document the new feature in the README? Thanks!

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.

4 participants