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

make proxy_stream_close close target stream even on errors #4905

Merged
merged 1 commit into from
Dec 5, 2018

Conversation

palmin
Copy link
Contributor

@palmin palmin commented Dec 4, 2018

When a git_filter_apply_fn callback returns a error while smudging proxy_stream_close
ends up returning without closing the stream. This is turn makes blob_content_to_file
crash as it asserts the stream being closed whether there are errors or not.

Closing the target stream on error fixes this problem, with the downside being that
error strings set by proxy_stream->target->close() will override ones set by git_filter_apply_fn
callback.

@ethomson
Copy link
Member

ethomson commented Dec 4, 2018

Closing the target stream on error fixes this problem, with the downside being that
error strings set by proxy_stream->target->close() will override ones set by git_filter_apply_fn
callback.

Yeah, I think that we should keep the original error message(s). You can giterr_save and giterr_restore to avoid this problem.

When git_filter_apply_fn callback returns a error while smudging proxy_stream_close
ends up returning without closing the stream. This is turn makes blob_content_to_file
crash as it asserts the stream being closed whether there are errors or not.

Closing the target stream on error fixes this problem.
@palmin
Copy link
Contributor Author

palmin commented Dec 4, 2018

It's ready for another look.

@ethomson ethomson merged commit 8092c43 into libgit2:master Dec 5, 2018
@ethomson
Copy link
Member

ethomson commented Dec 5, 2018

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.

None yet

3 participants