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

remote: don't free transport on disconnect #658

Merged
merged 1 commit into from
May 2, 2012

Conversation

schu
Copy link
Member

@schu schu commented May 1, 2012

Currently, git_remote_disconnect not only closes the connection but also
frees the underlying transport object, making it impossible to write
code like

// fetch stuff
git_remote_download()

// close connection
git_remote_disconnect()

// call user provided callback for each ref
git_remote_update_tips(remote, callback)

because remote->refs points to references owned by the transport object.
This means, we have an idling connection while running the callback for
each reference.

Instead, free the transport later with git_remote_free() or on explicit
call of git_remote_free_transport().

cc @carlosmn: Pretty much what we have talked about. Ack?

@travisbot
Copy link

This pull request passes (merged 3f3b8ac7 into 52877c8).

@carlosmn
Copy link
Member

carlosmn commented May 1, 2012

Yeah, pretty much. Though I don't see why git_remote_free_transport() would be something we'd need or want. Consider this:

// Download all of the things
git_remote_download();
// Open fds are bad
git_remote_disconnect();
// My memory is precious, let's free as much as we can
git_remote_free_transport();
// Now let's update the branches
git_remote_upate_tips(); // OH NOES, segfault!

And we're back to where we started.

@schu
Copy link
Member Author

schu commented May 1, 2012

Yeah, I was undecided. I thought, people might want to reuse the remote object (repeated fetch or something), but that's probably rather uncommon.. Will update.

@vmg
Copy link
Member

vmg commented May 1, 2012

I'm not seeing this. Why do we need an explicit git_remote_free_transport()? Is there any way to hide that complexity?

Currently, git_remote_disconnect not only closes the connection but also
frees the underlying transport object, making it impossible to write
code like

	// fetch stuff
	git_remote_download()

	// close connection
	git_remote_disconnect()

	// call user provided callback for each ref
	git_remote_update_tips(remote, callback)

because remote->refs points to references owned by the transport object.
This means, we have an idling connection while running the callback for
each reference.

Instead, allow immediate disconnect and free the transport later in
git_remote_free().
@carlosmn
Copy link
Member

carlosmn commented May 1, 2012

What we do need is to move freeing the transport to git_remote_free() instead of git_remote_disconect().

git_remote_free_transport() needs to die.

@vmg
Copy link
Member

vmg commented May 1, 2012

Sounds much more reasonable. Let's do it that way.

@travisbot
Copy link

This pull request passes (merged 42ea35c into 52877c8).

@schu
Copy link
Member Author

schu commented May 1, 2012

Done.

@carlosmn
Copy link
Member

carlosmn commented May 1, 2012

There we go, 👍

vmg added a commit that referenced this pull request May 2, 2012
remote: don't free transport on disconnect
@vmg vmg merged commit 5587a9c into libgit2:new-error-handling May 2, 2012
phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014
remote: don't free transport on disconnect
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.

4 participants