Skip to content

Prevent the git_trace callback from being garbage collected #917

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
Jan 20, 2015
Merged

Prevent the git_trace callback from being garbage collected #917

merged 1 commit into from
Jan 20, 2015

Conversation

jeffhostetler
Copy link
Contributor

Here is a better fix for the problem with the git_trace_cb being GC'd.

Thanks to @jamill for the suggestion that the call in GlobalSettings to Proxy.git_trace_set() might be secretly creating a temporary delegate for the GitTraceHandler method.

@Therzok
Copy link
Member

Therzok commented Jan 19, 2015

This is way better, thanks!

@nulltoken
Copy link
Member

@jeffhostetler Neat!

Few things though:

  • It looks like the identity you used to craft your commit isn't known from GitHub. As such, there's no link between the commit and your profile. You might be willing to fix that.
  • The commit message actually relates to a discussion that happened in Wrap git_trace_set to hold a reference to trace method to prevent GC. #915. Could you please reword it so that it makes sense by itself? Something as simple as "Prevent the git_trace callback from being garbage collected" would do the trick.

@jeffhostetler jeffhostetler changed the title A better fix for GC problem with git_trace_set(). Prevent the git_trace callback from being garbage collected Jan 19, 2015
@jeffhostetler
Copy link
Contributor Author

Sorry, I pushed that from my office machine. I've added that email address to my account.

I've rebased and reworded the commit as suggested. Thanks for the feedback.

@nulltoken nulltoken added this to the v0.21 milestone Jan 19, 2015
jamill
jamill reviewed Jan 19, 2015
View reviewed changes
LibGit2Sharp/Core/Proxy.cs
///
/// Since the given callback will be passed into and retained by C code,
/// it is very important that you pass an actual delegate here (and don't
/// the compiler create/cast a temporary one for you). Furthermore, you
Copy link
Member

Choose a reason for hiding this comment

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

and don't [let] the compiler...?

@nulltoken
Copy link
Member

@jeffhostetler Thanks a lot ✨

As you're at ease with rebasing, could you please rebase onto the latest vNext once @jamill's comment is fixed?

@jeffhostetler
Copy link
Contributor Author

Fixed. Thanks.

@nulltoken
Copy link
Member

Awesome. Just a minor warning from the build log:

"C:\projects\libgit2sharp\LibGit2Sharp.sln" (default target) (1) ->
"C:\projects\libgit2sharp\LibGit2Sharp\LibGit2Sharp.csproj" (default target) (2) ->
(CoreCompile target) -> 
  Core\Proxy.cs(2973,9): warning CS1570: XML comment on 'LibGit2Sharp.Core.Proxy.git_trace_set(LibGit2Sharp.LogLevel, LibGit2Sharp.Core.NativeMethods.git_trace_cb)' has badly formed XML -- 'End tag 'member' does not match the start tag 'summary'.' [C:\projects\libgit2sharp\LibGit2Sharp\LibGit2Sharp.csproj]

@jeffhostetler
Copy link
Contributor Author

Sorry about that. Got it.

nulltoken added a commit that referenced this pull request Jan 20, 2015
@nulltoken
Merge pull request #917 from jeffhostetler/jeffhostetler/git_trace_re…
78e57eb 
…ference2

Prevent the git_trace callback from being garbage collected
@nulltoken nulltoken merged commit 78e57eb into libgit2:vNext Jan 20, 2015
@nulltoken
Copy link
Member

✨ ✨ ✨ ✨ ✨ ✨ ‼️

@jeffhostetler jeffhostetler deleted the jeffhostetler/git_trace_reference2 branch January 20, 2015 14:25
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.21
Development

Successfully merging this pull request may close these issues.
4 participants
@jeffhostetler @Therzok @nulltoken @jamill