8000 Split Patch and TreeChanges by yorah · Pull Request #512 · libgit2/libgit2sharp · GitHub
[go: up one dir, main page]
Skip to content

Split Patch and TreeChanges #512

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 15, 2013
Merged

Split Patch and TreeChanges #512

merged 1 commit into from
Oct 15, 2013

Conversation

yorah
Copy link
Contributor
@yorah yorah commented Sep 16, 2013

Linked to #483.

The idea is to remove the generation of the patch from the TreeChanges class, and in the process to create repo.Diff.GeneratePatch() (naming advices welcome!) methods to fullfill the need of patch generation.

I'm not very happy with the current complexity of the diff code, and am still trying to find a better way to do it, but I thought it would be interesting to have some early feedback.

@nulltoken nulltoken mentioned this pull request Sep 16, 2013
Closed
@dahlbyk
Copy link
Member
dahlbyk commented Sep 16, 2013

IMO, this seem like a lot of additional complexity to avoid what is essentially a two- or three-option flag (None | CountLines | GeneratePatch). Supporting a FileDiffMode (?) property on CompareOptions seems like it would be trivial (glad to have a go, if you'd like).

@nulltoken
Copy link
Member

@dahlbyk I'd rather not expose properties which would be null or not, depending on input flags.

@Spiralis
Copy link

Happy to see that this is being addressed and changed 👍

Having said that, that is a lot of changes. I hope that they don't change too much on apis and that they test-suite is good, hence verifying that all previous test-cases still pass as well as having added new cases to verify the new ways to use them.

Following @dahlbyks suggestion and taking into @nulltokens comment about null properties: What if the diff-object was an abstract with different inheriting types? Then, dependent on what type of diff one wanted, the diff object was instantiated accordingly and would then have only the relevant properties?

But, for all I know this works the way it is made now. Until I get the chance to test and use this in real world I cannot really argue for or against.

@dahlbyk
Copy link
Member
dahlbyk commented Sep 17, 2013

I'd rather not expose properties which would be null or not, depending on input flags.

Throw InvalidOperationException suggesting a change to CompareOptions? Default to generate the full patch with all properties populated, but allow the caller to opt out of information they don't need? Lazily evaluate Patch?

@Spiralis I do think there could be a common base class to share between TreeChanges and Patch; that base class could probably be TreeChanges, now that I look closer.

8000
@nulltoken
Copy link
Member

@dahlbyk I hear what you say about the code complexity. And I agree from the perspective of a contributor to the library.

From a user perspective, making properties return null or throwing depending on input flags isn't the best experience.

@yorah
Copy link
Contributor Author
yorah commented Sep 17, 2013

@Spiralis I do think there could be a common base class to share between TreeChanges and Patch; that base class could probably be TreeChanges, now that I look closer.

I had the same feeling, but so I will take a look at it. I also have an idea that I think would make the API easier to discover, while fitting with some of @dahlbyk expectations. I will have a go at it sometime today, stay tuned!

@yorah
Copy link
Contributor Author
yorah commented Sep 17, 2013

New version:

  • Removed GeneratePatch() in favor of a generic version of Compare<T>(). This leads to clearner code for the caller (tests), and the changes to Diff are now minimal, which makes me think this is the right approach (when compared to previous version at least).
  • Comments are not up to date
  • Some tests missing (e.g.: exception thrown when calling Compare<string>()
  • Patch still doesn't inherit from TreeChanges, because I ran into some issues with the fact that Patch is an IEnumerable<FileChanges> whereas TreeChanges is an IEnumerable<TreeEntryChanges>. I guess there is a way to make it work, but the resulting code would be quite complex and hard to follow. @dahlbyk with the new API proposal, do we still need it in your opinion?

This leads to a breaking changes in the API (removed properties from TreeChanges), but I don't see a cool way around it.

@jamill
Copy link
Member
jamill commented Sep 18, 2013

I haven't had a chance to look closely at this - but another behavior that might be desirable in the future is the ability to optionally perform rename detection as part of the Compare method. While that wouldn't be tackled with this issue, it might be nice if the API patterns were flexible enough to accommodate th 8000 is in the future without too much disruption...

@dahlbyk
Copy link
Member
dahlbyk commented Sep 18, 2013

@jamill see #278

@yorah
Copy link
Contributor Author
yorah commented Sep 18, 2013

Comments and tests up to date!

@yorah
Copy link
Contributor Author
yorah commented Oct 7, 2013

FileChanges has been removed, in favor of ContentChanges. It makes sense, as now we have:

  • ContentChanges: holds the diff between two blobs
  • TreeEntryChanges: holds the changes to the metadata (changed mode, oid, ...)

The Compare() methods have been made generic:

  • Pass it a TreeChanges, and you get the minimum needed to see which files have been changed, created, ...
  • Pass it a Patch, and you get the whole patch (the per-file diff can be accessed through the indexer)

Ready for review.

@nulltoken
Copy link
Member

👍 for me me

Anyone?

@yorah
Split Patch and TreeChanges generation
11f4cb8 
Remove Patch from TreeChanges
Make repo.Diff.Compare() generic
@nulltoken nulltoken merged commit 11f4cb8 into libgit2:vNext Oct 15, 2013
@nulltoken
Copy link
Member

❤️ 👍 🎱 ✨

@yorah yorah deleted the split-patch-and-diff branch October 15, 2013 13:46
@Spiralis
Copy link

This looks like a really good solution. Thanks. 👍 😃. I'll update my app to use the latest version instead of my own fork. I closed #483 as a result of this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.15.0
Development

Successfully merging this pull request may close these issues.
5 participants
@yorah @dahlbyk @nulltoken @Spiralis @jamill
0