Skip to content

Skip building diff patch while doing stage/reset#481

Merged
nulltoken merged 1 commit into
libgit2:vNextfrom
freevoid:skip_building_patch_for_staging
Aug 7, 2013
Merged

Skip building diff patch while doing stage/reset#481
nulltoken merged 1 commit into
libgit2:vNextfrom
freevoid:skip_building_patch_for_staging

Conversation

@freevoid

@freevoid freevoid commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

TreeChanges constructor always builds diff string in TreeChanges.fullPatchBuilder (and additionally adds diff string in per-file TreeChanges.changes dictionary).

This behavior leads to wasting time and memory in Index.Stage and Index.Reset methods. In our particular case, it fails with System.OutOfMemoryException in attempt to build full patch for a big initial commit.

This change adds a flag to CompareOptions to be able to skip building patch where it is not required.

Comment thread LibGit2Sharp/CompareOptions.cs Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As the fact we rely on diffing for staging/unstaging is an implementation detail, could you please make this internal rather than public?

@nulltoken

Copy link
Copy Markdown
Member

Indeed, that makes much sense. Thanks a lot for pointing at this flaw and for the very nice PR!

Once the little requested change is made, I'll be more than happy to merge it.

@freevoid

freevoid commented Aug 7, 2013

Copy link
Copy Markdown
Contributor Author

Thanks, this change definitely helps in our scenario with big files and/or big number of changes. It should also help to improve general performance of Index.Stage/Index.Unstage.

@nulltoken

Copy link
Copy Markdown
Member

One last nitpick: Could you please squash those two commits together? 🙏

@freevoid

freevoid commented Aug 7, 2013

Copy link
Copy Markdown
Contributor Author

Sure, sorry for that.

@nulltoken
nulltoken merged commit bcc25ce into libgit2:vNext Aug 7, 2013
@nulltoken

Copy link
Copy Markdown
Member

And... it's been merged!

Thanks a lot!

✨ ✨ ✨ ✨ ✨ ✨ ✨ ✨ ✨

@Aimeast

Aimeast commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

Nice, I am waiting this opinion.

@nulltoken

Copy link
Copy Markdown
Member

Nice, I am waiting this opinion.

@Aimeast I also like this commit very much! It's just been merged.

However, I'm not very sure what you're waiting for? This opinion?

@Aimeast

Aimeast commented Aug 7, 2013

Copy link
Copy Markdown
Contributor

@nulltoken CompareOptions.SkipPatchBuilding

@nulltoken

Copy link
Copy Markdown
Member

@Aimeast Although not public, the option now exists in vNext. Thanks to @freevoid's contribution, Stage, Unstage and other diff based operations should transparently benefit from a slight speed bump.

It's also completely possible I didn't understand your concern/request. If that's the case, please open a separate issue/feature request as this one has been merged.

@freevoid
freevoid deleted the skip_building_patch_for_staging branch August 8, 2013 05:58
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.

6 participants