Skip to content

Add new functionaltiy for reversing diffs. #72

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

vslashg
Copy link

@vslashg vslashg commented Sep 20, 2024

ReverseFileDiff() and ReverseMultiFileDiff() accept a diff and return a diff which is the reverse operation -- that is, a diff that when applied would undo the edits of the original diff.

ReverseFileDiff() and ReverseMultiFileDiff() accept a diff and return
a diff which is the reverse operation -- that is, a diff that when
applied would undo the edits of the original diff.
@vslashg
Copy link
Author

vslashg commented Sep 20, 2024

We found ourselves needing a tool to reverse diffs, and wrote one in terms of sourcegraph/go-diff. Some colleagues suggested that this library might be generally useful, so I'm offering this as a contribution.

@vslashg vslashg closed this Sep 20, 2024
@vslashg
Copy link
Author

vslashg commented Sep 20, 2024

Sorry, I need to make a quick edit. I'll repost soon.

@vslashg
Copy link
Author

vslashg commented Sep 24, 2024

Sorry about the noise. This PR is ready for consideration.

@vslashg
Copy link
Author

vslashg commented Jan 13, 2025

I'm curious about the level of interest in future patches. For our local purposes, I'm adding semantic understanding of git annotations to our wrapper. Is it worth the effort to develop this with an eye towards upstreaming it? (I'll take away no hard feelings if the answer is "no", of course.)

@keegancsmith
Copy link
Member

Sorry about letting reviews on this slip. I'll take a look soon. We still internally heavily rely on this library. We are super open to contributions and improvements, but major refactors to the API are unlikely. Additionally it will likely be up to the will of the reviewer to decide if a feature is appropriate in this or in maybe a wrapper of this library :)

@keegancsmith keegancsmith self-requested a review January 16, 2025 13:30
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a rad change, well written and seems appropriate to me. Thanks so much!

One idea for testing, can you run the test on all patches we have in testdata and check that ReverseFileDiff(ReverseFileDiff(...)) gets back the same FileDiff?

Copy link
Member

@eseliger eseliger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me! Thanks for all the tests as well.

@keegancsmith did you hold back on merging this intentionally or can we accept this patch?

@keegancsmith
Copy link
Member

I would be very convinced this is good with my suggestion around tests.

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

3 participants