Use Remote.Add and let libgit2 handle default refspec

[tclem]

I like Add better than Createa as it matches Git's language and I don't feel like you are creating something in the traditional sense. The remote already exists, you are just adding it to this particular repository.

This also makes sure that we don't have defaults in C# code. Whenever possible we should defer for libgit2 for these kinds of things.

[nulltoken]

This also makes sure that we don't have defaults in C# code. Whenever possible we should defer for libgit2 for these kinds of things.

I do agree. This was more a reminder/temporary code as the current default in libgit2 doesn't seem to be the correct one (cf The RefSpec). There's even a pending related PR in the libgit2 project. Could you please split this into a separate commit?

It matches Git language [...] I don't feel like you are creating something in the traditional sense. The remote already exists, you are just adding it to this particular repository [...] I like Add better than Create

It's funny that you mention that, because I was also having a similar kind of thought few days ago.

We're dealing with different kind of objects: immutable GitObjects, mutable ones (references, remotes, ...) or hybrid ones (tags, notes). Should we use different verbs when generating mutable objects and immutable ones? I eventually closed this train of thought with something like this "An immutable object is an object that cannot be modified, so the term that one uses to generate it is irrelevant"

When @yorah was implementing the Notes API, we debated a bit about Notes.Add() vs Notes.Create(). I eventually selected Create() because it was matching the other collections. And I was preferring to err on the side of consistency.

I'm ok to change the semantic of the API, from Create/Delete to a more collection oriented Add/Remove but

• I'd prefer this to be a global change impacting all the collections
• It'd be awesome to make it a two steps operation (first adding the new methods and decorating the old ones with [Obsolete]. Then, once the next version is released, definitively drop the decorated methods). cf @dahlbyk's 186ca17 and 783948e

[tclem]

Good catch @nulltoken! I reverted to the default refspec coming from C#, but made a note that it should be removed when that PR goes through.

[tclem]

As far as Add vs Create goes I think there is a place for both. Where things are truly semantically the same as a collection then Add/Remove is the way to go. I would argue, however, that things like commits should probably use the Create verb. I'm not sure I'd want to blank rename all Create/Delete to Add/Remove.

[nulltoken]

I'm not sure I'd want to blank rename all Create/Delete to Add/Remove.

I'd really like the API to be "predictable". So, if only RemoteCollection benefits from an Add verb, I'd prefer to stick to Create. From a complete opposite standpoint, if all but CommitCollection are renamed, that would feel unbalanced too.

/cc @dahlbyk @xpaulbettsx @haacked @yorah How do you feel about this? Should we stick with Create? Should we change some of those Create to Add?

[haacked]

For collections, I prefer Add because you are still adding the item to the collection. In most cases, "creation" happens elsewhere.

Also, if you ever decide to implement IList, you'd now have an Add method and a Create method. ;)

[davidfowl]

+1 for Add. Create feels weird on a collection.

[dahlbyk]

I agree that Add is better than Create for a collection. This makes sense for branches, tags, remotes and notes but not for commits.

Now that I think about it CommitCollection.Create() doesn't really fit anyway - creating a commit (without the ObjectDatabase) is a function of a repository's HEAD and index, not its current collection of commits. I propose CommitCollection.Create() be moved/renamed as Repository.Commit(), replacing the extension method.

Related: should CommitCollection and related interfaces be renamed, to avoid the suggestion that it might have/need an Add behavior?

[tclem]

I really like what @dahlbyk is suggesting here. We do in fact have Commit() as an extension method of the Repository.

I'm +1 on renaming CommitCollection as well.

[haacked]

Rename them to what?

[dahlbyk]

CommitLog?

[dahlbyk]

Needed material for a Git presentation, so...

https://github.com/dahlbyk/libgit2sharp/commits/CreateToAdd

[dahlbyk]

(Last commit in that branch is meant to be pulled in after a release with old methods/interfaces marked Obsolete)

[nulltoken]

@dahlbyk Nicey :) Some few comments

• Please add some explicit messages in the [Obsolete] attribute directing the user to the method he should rather use
• Could you also deal with Delete() methods from [Branch|Tag|Reference|Note]Collection and rename them to Remove()? However, let's keep Configuration.Delete() for now.

[dahlbyk]

Split out PR for Add/Remove change. Once that's merged, this change gets even easier!

[tclem]

Looks like @dahlbyk's got this sorted!

[nulltoken]

Let's keep it open until the default refspec is actually handled by libgit2 :)

[tclem]

Ok @nulltoken, in that case I'll make this PR actually use the libgit2 defaults :) We should be good to go with this soon as libgit2/libgit2#737 has been merged.

[nulltoken]

I'm a bit reluctant to bind against the latest tip of libgit2 yet, as a slight side effect prevents git_index_add() from correctly handling file modes on Windows and makes one of the test fail.

[tclem]

@nulltoken, doesn't change what this PR is trying to do though, so just let it sit until we feel that the tip is stable again.

[nulltoken]

Deal ;)