Skip to content

make proxy_stream_close close target stream even on errors#4905

Merged
ethomson merged 1 commit into
libgit2:masterfrom
palmin:proxy_stream_close
Dec 5, 2018
Merged

make proxy_stream_close close target stream even on errors#4905
ethomson merged 1 commit into
libgit2:masterfrom
palmin:proxy_stream_close

Conversation

@palmin
Copy link
Copy Markdown
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
Copy Markdown
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 palmin force-pushed the proxy_stream_close branch from 15999c6 to f4835e4 Compare December 4, 2018 20:52
@palmin
Copy link
Copy Markdown
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
Copy Markdown
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.

3 participants