Skip to content

More remote rename fixes #2407

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

Merged
merged 6 commits into from
Jun 8, 2014
Merged

More remote rename fixes #2407

merged 6 commits into from
Jun 8, 2014

Conversation

carlosmn
Copy link
Member

@carlosmn carlosmn commented Jun 6, 2014

Remove a bunch of code and conditionals we just don't need and return the problems to the user as a list of strings instead of using a callback, which has no business being there.

From the discussion in libgit2/libgit2sharp#741

carlosmn added 2 commits June 6, 2014 15:54
We don't allow renames of anonymous remotes, so there's no need to
handle them.

A remote is always associated with a repository, so there's no need to
check for that.
We must make sure that the name pointer remains valid, so make sure to
allocate the new one before freeing the old one and swap them so the
user never sees an invalid pointer.
@carlosmn
Copy link
Member Author

carlosmn commented Jun 6, 2014

Hold off on this for a bit, I forgot that we also fail to retarget symrefs.

@vmg
Copy link
Member

vmg commented Jun 6, 2014

Really like this callback removal. I really don't wanna put pressure on you, but I promised @arrbee I'd tag a release candidate this weekend, so if you could wrap this up, it'd be niiiiice. :p

carlosmn added 3 commits June 6, 2014 21:43
There is no reason why we need to use a callback here. A string array
fits better with the usage, as this is not an event and we don't need
anything from the user.
A symref inside the namespace gets renamed, we should make it point to
the target's new name.

This is for the origin/HEAD -> origin/master type of situations.
@carlosmn
Copy link
Member Author

carlosmn commented Jun 6, 2014

This should be it as far as rename stuff goes. I'd also like to fix the remote-freeing we do on remove. Would you prefer a different pr for that or should I just push it here?

This was a bad idea. Don't free except in the free function.
@nulltoken
Copy link
Member

PUSH. IT. HERE. 😉

@carlosmn
Copy link
Member Author

carlosmn commented Jun 6, 2014

Yeah, I just went ahead and did that.

@nulltoken
Copy link
Member

Yeah, I just went ahead and did that.

Move fast and break things the API

@carlosmn
Copy link
Member Author

carlosmn commented Jun 6, 2014

We've never made a release with remote deletion, so this is one place where I didn't break it ;)

vmg pushed a commit that referenced this pull request Jun 8, 2014
@vmg vmg merged commit ce5e661 into development Jun 8, 2014
@carlosmn carlosmn deleted the cmn/remote-rename-more branch June 10, 2014 13:01
phatblat pushed a commit to phatblat/libgit2 that referenced this pull request Sep 13, 2014
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