Enumerate refspecs

[Yogu]

The Remote.RefSpecs property allows to enumerate over all refspecs defined for a specific remote.

The intention of this extension actually is to fetch git notes. As libgit2 does not allow to fetch a specified refspec, you have to add it to the configuration. But libgit2 does not support config --add either, so I decided to choose the proper way.

This pull request has been shrunken to only include read access. Write access will be discussed and implementet later (see this branch for the version containing updating features discussed here).

What do you think of it? I am eager for any feedback / suggestions.

[carlosmn]

As libgit2 does not allow to fetch a specified refspec,

This is wrong. With git_remote_clear_refspecs() + git_remote_add_fetch() you can ask it to fetch whatever you wish.

. But libgit2 does not support config --add either,

It does. git_remote_set_multivar() with a match of ^$.

Unless I'm missing some functions, this forces you to store every refspec in the configuration. This is bad, as it stops you from using a specific refspec for one fetch.

[Yogu]

This is wrong. With git_remote_clear_refspecs() + git_remote_add_fetch() you can ask it to fetch whatever you wish.

Ah, now I see.

Unless I'm missing some functions, this forces you to store every refspec in the configuration. This is bad, as it stops you from using a specific refspec for one fetch.

I think both methods (saving or temporary editing) would be useful. I guess we could simply omit the save calls and provide a Save() method on the Remote. Then we can use the modified Remote object as argument for Network.Fetch, but thenNetwork.Remotes['origin']` would still return the unmodified remote (as expected).

I see one issue: We would have to keep the RemoteSafeHandle from the point where it is changed, and we would not know when to release it. Or could we release it when the repository gets disposed? Otherwise, the Remote class would have to be declared as IDisposable.

But libgit2 does not support config --add either,

It does. git_remote_set_multivar() with a match of ^$.

I missed that one. But it is not implemented in libgit2sharp yet, is it?

[carlosmn]

Yes, we should provide the user a way to explicitly save the current configuration, but there are many situations where you want to modify the refspecs or other attributes only in-memory.

The issues with something like Network.Remotes["origin"] is that it's not explicit whether you get a cached version or not. IMO it should always return a brand new object.

You'd release the git_remote pointer the same as the other ones: provide the user a way to do it by hand IDisposable, as it's a memory resource, but let the GC do its job too.

I don't know whether mutlivars are exposed in libgit2sharp. They're a right PITA either way.

Btw, with libgit2/libgit2#1914 I'm hoping to move away from this kind of direct modification and simply provide a way to set the refspecs you want (with add_fetch/add_push as convenience for appending) and then iterate over the ones that do exist.

[Yogu]

IMO it should always return a brand new object.

That's my opinion as well, and just as it happens, it is currently implemented that way.

Maybe we should get rid of the RefSpec class, and just split push/fetch refspec collections up. How about this API?

Remote:    
    void ReplaceFetchRefSpecs(IEnumerable<string> refSpecs)
    void ReplacePushRefSpecs(IEnumerable<string> refSpecs)
    RefSpecCollection FetchRefSpecs
    RefSpecCollection PushRefSpecs
    Save()

RefSpecCollection:
    string this[index]
    IEnumerator<string> GetEnumerator()
    Clear()
    Add(string refSpec)

[dahlbyk]

The issues with something like Network.Remotes["origin"] is that it's not explicit whether you get a cached version or not. IMO it should always return a brand new object.

Current convention is that 1) a new instance is always returned, and 2) that instance should be immutable. See use of RemoteUpdater, for example.

[Yogu]

Current convention is that 1) a new instance is always returned, and 2) that instance should be immutable. See use of RemoteUpdater, for example.

If we actually use the RemoteUpdater to update refspecs, one might assume you could do this:

Remote origin = Network.Remotes["origin"];
Remote one = Network.Remotes.Update(origin, (r) => r.ReplaceFetchRefSpecs(specsOne));
Remote twoA = Network.Remotes.Update(first, (r) => r.FetchRefSpecs.Add(specsTwoA));
Remote twoB = Network.Remotes.Update(first, (r) => r.FetchRefSpecs.Add(specsTwoB));

and then have three remotes, where twoA is specsOne plus specsTwoA, and twoB is specsOne plus specsTwoB.

But as I see, this is not possible to implement with the current libgit2 api, because there is no way to clone a remote that does not exist in the config file. We can only derive remotes from actual config remotes.

(even if we did not provide the Add methods, the same issue would arise with SetFetchRefSpecs set by one and SetPushRefSpecs set by the twos.

Maybe we should provide Remotes and DerivedRemotes, where Remote has a method Derive that returns a mutable DerivedRemote?

[nulltoken]

Maybe we should provide Remotes and DerivedRemotes, where Remote has a method Derive that returns a mutable DerivedRemote?

@Yogu I'd prefer to not cope with mutable objects.

As we all make take some time to reach to an agreement regarding the design of the API to update the refspecs, how about reducing the scope of this PR? Ideally this PR would focus on the much more consensual reading aspects (loading, enumerator, indexer, ...) and could be merged reasonably quickly.

Then, once this is merged, a new one would be submitted to bring back the updating features on the table.

[nulltoken]

Please use Ensure to guard against potential invalid parameters

[nulltoken]

@Yogu Thanks a lot for this proposal. I quite like what I read!

[Yogu]

I will remove the modifying aspects of this pull requests, but I want to get this issue written down:

My implementation is currently inconsistent regarding the use of the RefSpec class. While you get RefSpec instances with the Enumerator / Indexer, you need to provide a string to the Add methods. This is because there is no API function to construct a refspec from its parts. I think this one would be better:

    Remote.Update(r => r.FetchRefSpecs.Add(new RefSpec("refs/heads/*", "refs/remotes/origin/*", RefSpecModifiers.Force)));

Or, if you really like to,

    Remote.Update(r => r.FetchRefSpecs.Add(RefSpec.Parse("+refs/heads/*:refs/remotes/origin/*")));

where RefSpec.Parse should throw a FormatException if the string is not a vaild refspec.

Having a way to parse refspecs, we could also construct RefSpec instances from git_remote_fetch_refspecs.

@carlosmn: Are there plans to provide parse/create refspec API methods? If not, should we implement it in libgit2sharp?

Alternatively to all this, we could stay with Add(string refspec) and git_remote_get_refspec.

[carlosmn]

I'm on the fence on exposing git_refspec as a user-serviceable type, as a refspec is only really useful when it's associated with a remote (fully-specified refspecs you can do something with, but stuff like master:foo-master needs a connected remote). If we were to expose that, then it should be at the libgit2 layer, with the bindings using that code.

The enumeration is useful for refspecs that have been dwim'ed already, as a bunch of them won't be able to match anything after you've connected to the remote. When used in git-core, these refspecs are either used in a connected remote or a fully-specified one.

The way I envision the refspec setting/update function is of the Func<IEnumerable<string>, IEnumerable<string> type, where add_fetch is short for (old) => old.Concat(new[] {str}) and set_fetch_refspecs is (_) => new_refspecs. This is a very annoying thing to implement in C, so I expose a get and set as separate methods.

[nulltoken]

Aren't we missing something like this here?

Assert.Equal(1, count);

[Yogu]

Oh right, I forgot about than one when I removed the Count property.

[nulltoken]

@Yogu The PR doesn't appear to merge cleanly. Can you rebase it on top of vNext, please?

[Yogu]

Rebased without conflicts. Looks fine, the tests still pass.

[nulltoken]

🎉

Thanks!