Bind the methods for refspecs

[carlosmn]

Refspecs are more than just a few fields. They contain the logic to transform reference names forward and backward. This is something we need when we want to figure out which refspecs go where and figure out e.g. if we're pushing on to a branch which has moved since we last fetched.

This means we need to keep the refspec's memory around, which means we need to keep the remote around. There's maybe a better way to handle us now keeping the memory around than freeing in the destructor (does making Repository disposable break compat?) but it's about freeing memory, so it's not a bad place.

[ethomson]

(does making Repository disposable break compat?)

Repository is IDisposable already. Did you mean Remote?

[carlosmn]

Yes, I did mean Remote. It's a new thing that's disposable, so existing code wouldn't know to handle it already.

[ethomson]

I'm in favor of us having more bindings directly to libgit2 and have them be IDisposable. Best to do this before the mythical move towards 1.0.

/cc @whoisj

[carlosmn]

Merged manually into master.

[niik]

I'm in favor of us having more bindings directly to libgit2 and have them be IDisposable

@carlosmn @ethomson I too, am very much in favor of having more IDisposable. But in this case I must say that the API is far from ideal for disposable resources. I just went through all the places in GitHub Desktop where Remotes needed disposing and some of them weren't pretty.

I realize that we're not talking about straight up leaks here but it's common practice to eagerly dispose of any Disposable even though the destructor of the safe handle would do it for you eventually.

Here's some examples from our codebase that I encountered

- RemoteName = libgit2Branch.Remote != null ? libgit2Branch.Remote.Name : null;
+ using (var remote = libgit2Branch.Remote)
+ {
+     RemoteName = remote != null ? remote.Name : null;
+ }

-                        var remote = branch.TrackedBranch;
-                        if (remote != null && remote.Remote.Name != remoteName)
+                        var trackedBranch = branch.TrackedBranch;
+
+                        if (trackedBranch != null)
                         {
-                            branch = null;
+                            using (var remote = trackedBranch.Remote)
+                            {
+                                if (remote.Name != remoteName)
+                                {
+                                    branch = null;
+                                }
+                            }

-                if (!Repository.Network.Remotes.Any())
+                if (!Repository.HasAnyRemotes())

+        public static bool HasAnyRemotes(this IRepository repository)
+        {
+            using (var remote = repository.Network.Remotes.FirstOrDefault())
+            {
+                return remote != null;
+            }
+        }

-                bool hasUpstreamRemote = Repository.Network.Remotes
-                    .Any(x => String.Equals(x.Name, remoteName, StringComparison.OrdinalIgnoreCase));
+                bool hasUpstreamRemote = false;
+                
+                foreach(var remote in Repository.Network.Remotes)
+                {
+                    using(remote)
+                    {
+                        if(String.Equals(remote.Name, remoteName, StringComparison.OrdinalIgnoreCase)) 
+                        {
+                            hasUpstreamRemote = true;
+                            break;
+                        }
+                    }
+                }

Not saying that there's not better ways of doing any of these but I wouldn't expect something coming out of either an enumerator or an indexed property to be disposable is all.

Since it looks like we're eagerly loading everything except the refspecs and seeing as there's usually only one refspec per remote I don't see the big benefit of this approach over copying the data (I can't believe I'm saying this).

[carlosmn]

The advantage here is being able to use the refspecs. They're not just some string, you sometimes actually want to apply them to strings you have. And they're owned by the remote, so you need to keep it around. I'd be in favour of making the Remote load memory less eagerly, this was a first pass to enable some functionality I needed.

[carlosmn]

Looking at your diffs, it looks like what we need is a way to return the name of the remote (i.e. a managed String) in question rather than an instantiated Remote. Even inside the bindings, for some network operations all we want out of the Remote we pass in is the name.