Use Remote.Add and let libgit2 handle default refspec #164

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
5 participants
Member

tclem commented Jun 1, 2012

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.

Member

nulltoken commented Jun 3, 2012

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
Member

tclem commented Jun 4, 2012

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.

Member

tclem commented Jun 4, 2012

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.

Member

nulltoken commented Jun 4, 2012

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?

Contributor

Haacked commented Jun 4, 2012

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. ;)

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

Member

dahlbyk commented Jun 5, 2012

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?

Member

tclem commented Jun 5, 2012

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.

Contributor

Haacked commented Jun 6, 2012

Rename them to what?

Member

dahlbyk commented Jun 6, 2012

CommitLog?

Member

dahlbyk commented Jun 8, 2012

Needed material for a Git presentation, so...

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

Member

dahlbyk commented Jun 8, 2012

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

Member

nulltoken commented Jun 8, 2012

@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 dahlbyk referenced this pull request Jun 9, 2012

Closed

Create/Delete to Add/Remove #175

Member

dahlbyk commented Jun 9, 2012

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

Member

tclem commented Jun 15, 2012

Looks like @dahlbyk's got this sorted!

@tclem tclem closed this Jun 15, 2012

@nulltoken nulltoken reopened this Jun 16, 2012

Member

nulltoken commented Jun 16, 2012

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

Merge branch 'vNext' into use-libgit2-default-refspec
Conflicts:
	LibGit2Sharp/RemoteCollection.cs
Member

tclem commented Jun 18, 2012

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.

Member

nulltoken commented Jun 18, 2012

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.

Member

tclem commented Jun 18, 2012

@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.

Member

nulltoken commented Jun 18, 2012

Deal ;)

@tclem tclem closed this in 3916ea5 Jun 20, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment