8000 Setting a remote named "origin" doesn't save it properly. by haacked · Pull Request #114 · libgit2/libgit2sharp · GitHub
[go: up one dir, main page]
Skip to content

Setting a remote named "origin" doesn't save it properly. #114

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
Closed
wants to merge 1 commit into from
Closed

Setting a remote named "origin" doesn't save it properly. #114

wants to merge 1 commit into from

Conversation

haacked
Copy link
Contributor
@haacked haacked commented Feb 24, 2012

Please take a look at the two unit tests I added. I think they should pass, but they fail. The problem seems to be that setting the origin url doesn't allow you to retrieve the setting you just saved.

If you create a new instance of repository pointing to the same directory, you can retrieve it via the Configuration property of Repository.

However, in all cases, repository.Remotes["origin"] returns null.

@nulltoken
Copy link
Member

@haacked Thanks for report. I've started to dig into this issue.

Currently, git.git produces the following changes to the CONFIG file when adding a remote:

$ cd /d/Temp
$ mkdir remote && cd remote
$ git init .
$ git remote add origin http://example.com
$ cat .git/config
[core]
        repositoryformatversion = 0
        filemode = false
        bare = false
        logallrefupdates = true
        symlinks = false
        ignorecase = true
        hideDotFiles = dotGitOnly
[remote "origin"]
        url = http://example.com
        fetch = +refs/heads/*:refs/remotes/origin/*

Both url and fetch configuration entries are created. Current implementation of libgit2 git_remote_load() cringes when no fetch exists.

Adding the following line to your tests would allow the correct retrieval of a non null remote:

repo.Config.Set("remote.origin.fetch", "+refs/heads/*:refs/remotes/origin/*");

However, the main problem is that LibGit2Sharp doesn't expose an API to properly create a remote and compelling the user to directly create configuration entries is a smelly/fragile workaround.

Thus the following proposal:

  • Bind against libgit2/libgit2@1db9d2c to benefit from @carlosmn's work on the remote API
  • Draft a LibGit2Sharp RemoteCollection API update proposal
  • Add tests
  • Kindly request for a review from you

How does it feel?

@carlosmn
Copy link
Member

Yeah, it looks like git_remote_load is too tailored to the stuff I was doing to test. git itself doesn't care that there's not refspec. I'll fix that in libgit2.

How should an API to list all remotes in a repo look like (presumably you'll want that for RemoteCollection)? Callback? A list? /cc @tanoku

@nulltoken
Copy link
Member

I'll fix that in libgit2.

@carlosmn Awesome!

How should an API to list all remotes in a repo look like (presumably you'll want that for RemoteCollection)?

That would be pretty nice indeed. How about reusing the git_strarray type ?

int git_remote_listall(git_strarray *array, git_repository *repo);

@nulltoken
Copy link
Member
8000

@haacked I've pushed a first batch of changes in my repo

The issue that you initially raised has been fixed by @carlosmn.

Some work still has to be done:

  • Make RemoteCollection implement IEnumerable<Remote>. This would require libgit2 to expose a API to list all remotes.
  • Make Remote expose a collection of (Fetch|Push)RefSpec
  • Update the Create method to accept more parameters
  • Add some XML documentation to RemoteCollection.Create()

@haacked
Copy link
Contributor Author
haacked commented Feb 24, 2012

Thanks all!

I like those ideas. I'd love to see RemoteCollection become a proper .NET collection, aka implement IEnumerable<Remote> or even derive from ReadOnlyCollection<Remote>.

@carlosmn carlosmn mentioned this pull request Feb 26, 2012
Merged
@nulltoken
Copy link
Member

I'd love to see RemoteCollection become a proper .NET collection

@haacked Looks like @carlosmn is making sure this happens sooner than later ;-)

@nulltoken
Copy link
Member

@haacked I'm closing this as the original issue is now fixed and pushed to vNext. Credits should go to @carlosmn :-)

The RemoteCollection type now implements IEnumerable<Remote> and exposes a Create() method

@nulltoken nulltoken closed this Feb 27, 2012
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.9.0
Development

Successfully merging this pull request may close these issues.
3 participants
@haacked @nulltoken @carlosmn
0