Extended RemoteUpdater to update refspecs

[Yogu]

Push/Pull Ref Specs can be updated via Remotes.Update(). The collections
can either be manipulated or replaced completely. Any changes are saved
immediately.

This is the place for a discussion about the API to manipulate push/fetch refspecs. In the end, there should be two options:

• Manipulate the git config, i.e. write the changes to disk
• Create a temporary remote based on an existing one, change some refspecs, and use that for a push/pull operation. (will be addressed in Teach repo.Network.Fetch() to accept an optional refspec #572)

The second option requires a lot of changes, because libgit2sharp currently creates a RemoteSafeHandle based on a remote name in many places (e.g. in Network.Fetch).

As the first option should be relatively simple to implement, I only addressed this one.

I extended the RemoteUpdater instead of RefSpecCollection because that way, the Remotes and its RefSpecs properties stay immutable.

[nulltoken]

The native setter methods are not implemented yet, because I do not know how to marshal an array as git_strarray. Any support would be greatly appreciated.

@Yogu GitStrArrayIn might be what you're looking for.

Few things to consider:

• Currently BuildFrom() only accepts a FilePath[]. One option could be to add an overload accepting a string[] (along with a bit of refactoring to avoid any code duplication). However, the FilePath type exposes an implicit string cast operator which might silently come into play and force the runtime to pick the wrong overload. This would require some further investigation.
• GitStrArrayIn allocates some memory. Please, ensure that it's properly disposed once you're done with it.

[Yogu]

This is ugly. Do you know how to ass a string array here? int[] works, but string[] triggers the following error:

An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type

[nulltoken]

This is ugly. Do you know how to ass a string array here? int[] works, but string[] triggers the following error:

An attribute argument must be a constant expression, typeof expression or array creation expression of an attribute parameter type

This is the closest hack I can think of. See also this StackOverflow question where a string array is being yield.

[dahlbyk]

If you try to specify a string array as the only argument it seems to get confused, but InlineData(new[] { "one", "two" }, new[] { "three", "four" }) works for me at least.

[Yogu]

@dahlbyk Great! I had only one parameter at first, it works fine with two.

[Yogu]

@nulltoken Thanks, GitStrArrayIn worked. IntelliSense tells me that the string overload is used.

Implemented setters, added tests, squashed and rebased.

[Yogu]

What do you think of it? Anything to improve? Ready to go? Once we've merged this, I can start on #572.

The Travis build has passed (just click on detalis); somehow GitHub did not update the status.

[nulltoken]

This is never used. I think we can safely drop it.

[nulltoken]

What do you think of it? Anything to improve? Ready to go?

I have thought about this for days, but I still don't really feel at ease with the full blown refspecs collection (Remove(), Enumerate(), Clear(), ...). Although your proposal is quite powerful, I'm afraid it might not be discoverable as it doesn't fit with the rest of the API.

I was rather thinking about a replace only setter (with [Push|Fetch]RefSpecs property being a IEnumerable<> rather than a ICollection<>), which would clear the entries when being passed null.

However, adding a single refspec would require a full list containing all the existing specs and the new one.

@dahlbyk @carlosmn @ben @ethomson @jamill @yorah Guys, what's your feedback about this?

[dahlbyk]

Rather than replace list, I would Clear() and AddRange().

[Yogu]

I think, this would decrease performance if you only want to replace the list, because it would have to retrieve the list first. And as I understand, just replacing is a common use case.

[dahlbyk]

We immediately Save(), which will evaluate the Lazy<> and enumerate newValues.

As I see it, the tradeoff is between allocating a new Lazy<>, a new List<>, and one more more new inner arrays (depending on size); or reusing all three (at worst allocating additional storage, though that would have been necessary anyway). Not to mention that letting list be readonly makes the internals rather cleaner.

[Yogu]

Ok, there would would be less overhead in the managed scope. However, we would save the call to git_remote_get_fetch_refspecs() which is a result of the original lazy evaluating. As the list would normally be small, and the remote is already loaded in memory, it is probably not a big deal. So I am not sure which of these alternatives would perform better.

I agree that your proposal would be more readable, though.

[nulltoken]

I tend to favor readability until concrete performance issues are raised 😉

[carlosmn]

I was rather thinking about a replace only setter (with [Push|Fetch]RefSpecs property being a IEnumerable<> rather than a ICollection<>), which would clear the entries when being passed null.

That sounds about right. There is no reason for the refspecs to behave like a collection, as they're something which 99% of the time you simply want to set (e.g. git push origin master:foo, git fetch origin refs/pull/1337/head). There are only really one place where you want to do anything else, and that is for fetch --tags, which is going to have that behaviour automatically anyway (it replaces currently) and for clone, which we also do inside the library (and expose the functions because why not).

[dahlbyk]

I think you want Concat here instead of Union.

[dahlbyk]

I'll support the "opposing side" - even if the common use case is just replacement (which is simply supported by this API), there are other common-enough use cases that treating it as a collection doesn't seem like a terrible idea (even if most of the collection API surface area is never used - such is life with .NET too-big list/collection APIs). Especially in the context of an "updater".

[nulltoken]

I'll support the "opposing side"

Ok. Let's go down this road.

@Yogu Thanks for this nice piece of work! Can you please update the code according to the review comments and rebase this on top of vNext so that we can merge this?

[Yogu]

@Yogu Thanks for this nice piece of work! Can you please update the code according to the review comments and rebase this on top of vNext so that we can merge this?

Done. Great do have this worked out, now I can use official versions in my software again... until the next new feature new is required ;)

[nulltoken]

I think that @dahlbyk was rather after something like

public virtual ICollection<string> FetchRefSpecs
{
   get { return fetchRefSpecs; }
   set { fetchRefSpecs.ReplaceAll(value); }
}

[dahlbyk]

I would!

[nulltoken]

Last nitpick: Can you please drop all the code beautification related changes?

[nulltoken]

@Yogu Thanks for this nice piece of work! Can you please update the code according to the review comments and rebase this on top of vNext so that we can merge this?

Done. Great do have this worked out, now I can use official versions in my software again

I'm eager to get this merged!

until the next new feature new is required ;)

And this one as well ;-)

[nulltoken]

An there maybe

public virtual IEnumerable<RefSpec> FetchRefSpecs
{
   get { return refSpecs.Where(r => r.Direction == RefSpecDirection.Fetch); }
}

[Yogu]

Last nitpick: Can you please drop all the code beautification related changes?

I hope I found them all. It was only Proxy.cs, right?

[nulltoken]

And that's a meeerrrgeee!

Thanks a lot for this 💎‼️