I fear there's neither an easy nor a definitive answer to this question. Below are some random thoughts about it:

• From a strict code style perspective, I agree with you that making Init() and Clone() return a repository would be more elegant when one is willing to start using the repository in the very next millisecond.
• Usually Init() and Clone() are both called once in the lifetime of a repository.
• Multiplying the number of methods returning a IDisposable repository raises the probability of someone forgetting to Dispose() it.
• I'm not sure that a GUI client would benefit from it. Instead, there are chances that cloning a repository would result in a call to Clone(), then another call to the Repository constructor.

As I wrote earlier, I have no firm position and I'm open to discussion on this subject.

/cc @tclem, @xpaulbettsx, @henon, @dahlbyk

I don't quite understand your last point. What's the difference between a GUI and a non-GUI client?

I do plan to write a Web client using the library. But I still don't know exactly how I'm going to use it, meaning the exact sequence of operations. I however, discovered a (minor) friction while writing a test, and a (minor) surprise point, so, my suggestion is based solely on listening to the tests.

In my mind, it'd be better to return the repo object. Can you ever imagine an app that wouldnt immediately new up the repo?

I however, discovered a (minor) friction while writing a test, and a (minor) surprise point, so, my suggestion is based solely on listening to the tests.

This is why I suggested you to open a request :)

I don't quite understand your last point. What's the difference between a GUI and a non-GUI client?

Sorry, I was unclear. I was opposing a unit test to a full blown desktop client.

When running a cloning test, the code is focused on asserting the result of the cloning. This is the main goal of the test. Returning a Repository instead of a path would allow to reduce the number of lines and start asserting "sooner".

However, when using a desktop client, I'd like not to be blocked while cloning a repository, especially a large one. I may even want to clone several ones at a time, in parallel (with a progress bar for each or a notification system (ding "Linux" successfully cloned). While my repos are being cloned, I might be editing the Markdown of a Readme file from another repo. In this perspective, chances are than the browsing the commits of one the cloned repositories will be the result of an explicit action of mine and not an automatic reaction of the application upon a successful clone. I wouldn't start "using" it immediately.

In my mind, it'd be better to return the repo object.

@xpaulbettsx That's a +1, then ;) IIRC @tclem was also in favor of this approach.

@uluhonolulu scratch my last comment. We're talking about syntactic sugar here, not its impact on a software architecture.

@uluhonolulu @xpaulbettsx I've given some more thought to this matter.

Nice thing about being in v0.x.x is that API is considered unstable, isn't it? ;) So, let's try this.

I remember talking about this a while back. I actually don't care either way. So far when I've used Init() I've always turned around and immediately created a repository object. I'm game to change the API and see how if feels.

I try to avoid returning an IDisposable unless there's no reason to call the method without using its return value. In this case, I could see calling Init() or Clone() without needing a Repository instance, but maybe I'm in the minority.

One alternative is an optional callback: dahlbyk/libgit2sharp@cd96c50

@nulltoken Now I see your point.

Still, your notification system will probably need more info than just the path to the cloned repo => it'll still have to create a new Repository instance in order to Ding(repo.Name);.

If someone forgets to Dispose of Repository.Init(), then he will surely forget to dispose of new Repository(), no?

Still, your notification system will probably need more info than just the path to the cloned repo => it'll still have to create a new Repository instance in order to Ding(repo.Name);

@uluhonolulu This is what the *"I've given some more thought to this matter."` part was about. :)

If someone forgets to Dispose of Repository.Init(), then he will surely forget to dispose of new Repository(), no?

You may be right. My goal, by not multiplying the number of method returning an IDisposable, was to reduce the surface of exposure to this risk. The only way I see to mitigate this would be to create some code samples and tutorials (cf. RavenDB) in order to raise the probability of having good code being copied/pasted.

So, in order to make everybody happy, I'd create two overloads: one that returns a Repository instance, the other is with a callback like @dahlbyk suggested.

As I was the one who initiated this issue, I suggest I do the pull request.

@nulltoken I volunteer to contribute to the Samples section when the API is stabilized. Until then, I hope folks will look at our tests for the samples.

One alternative is an optional callback: dahlbyk/libgit2sharp@cd96c50

@dahlbyk Wow! You just blew my mind. I really like this functional approach. This would lead to some JQuery-like code/syntax. We could even ditch the public constructor.

Repository.Open(path, repositoryOptions, repo => {
    repo.Stage("a.xtx);
    repo.Commit(...);
});

The only downside I see is that some desktop GUI clients might require to store the repo variable in order not to rebuild it each time the user click on a commit headline. Relying on this Action<> code pattern would dispose the repo instance at the end of the code block. Unless I'm wrong, the stored copy of the repo would then become useless. Too bad as I really like the code style :)

I could see calling Init() or Clone() without needing a Repository instance, but maybe I'm in the minority.

Would you have some use cases in mind?

I'm game to change the API and see how it feels.

@tclem ACK. If we go this way, I sincerely hope we won't have to rollback. Even if semver is on our side, switching back and forth would be... hmm....how to say this politely... well... less than pleasant :p

As I was the one who initiated this issue, I suggest I do the pull request.

Awesome!

I'd create two overloads: one that returns a Repository instance, the other is with a callback like @dahlbyk suggested.

@uluhonolulu Could we wait for @dahlbyk 's feedback on my comment? ^^ Beside this, I'm really not in favor of proposing two different ways of opening/initializing/cloning a repo. As those are two really different approaches, I'd really prefer to select one and stick with it. Less doc to write, less explanation to give, easier to support...

I volunteer to contribute to the Samples section when the API is stabilized.

Beware, I'll remind you of this ;p

I could see calling Init() or Clone() without needing a Repository instance, but maybe I'm in the minority.

Would you have some use cases in mind?

The first example that jumps to mind is the GUI scenario. "New Repository" dialog opens, user types path, repo gets created... It seems unlikely to me that rendering the new repo is now that dialog's responsibility. More likely you would just return "success" and move on to an existing repo viewer that would likely need to create its own Repository instance to do its thing.

Unless I'm wrong, the stored copy of the repo would then become useless.

Correct.

I think it makes sense for there to be two ways we expose Repository:

1. Callbacks in places where it makes sense, or use of the Repository seems optional. In these cases, we "own" the instance and take care of Dispose().
2. Through the constructor, with clear documentation that since the developer is creating this instance she is responsible for owning its lifestyle.

Since the approach might feel foreign to some .NET devs, we could emphasize the former in documentation/examples, but provide the latter (always with using()) to handle scenarios where the dev desires a long-running instance. To further emphasize its IDisposable-ness, instead of Repository.Open(..., callback) we might consider Repository.Use(..., callback).

One other thought: documentation needs to be clear that the callback is the usable scope both for the Repository instance and anything created from it (Branch, etc). I guarantee someone will try this:

Branch head;
Repository.Clone(..., repo => { head = repo.Head; });
// head is now invalid, but available

That being said, it might be handy to provide two overloads, void Repository.Init(..., Action<Repository> action) and TResult Repository.Clone<TResult>(..., Func<Repository, TResult> selector). The caller just has to be careful with selector to only return a result that does not depend on the now-disposed Repository.

With all respect, now I see that the callback version gives much more possibilities to shoot yourself in a leg.

No more so than this common antipattern:

Branch GetHead(string path) {
    using(var repo = new Repository(path)) {
        return repo.Head;
    }
}

Ultimately working with stuff that depends on an IDisposable is hard to do right - regardless of what's decided here, there needs to be guidance. I took an interest in my former life as a SharePoint dev: http://solutionizing.net/tag/dispose/

@dahlbyk @uluhonolulu @tclem @xpaulbettsx Although, I still feel a bit uneasy adding some more IDisposable to deal with let's give this a try and change the Init() and (soon to be released wink @uluhonolulu wink) Clone() signature so that they return a Repository instance.

In order to provide (aside from the unit tests) some additional guidance, the xml documentation should be updated as well in order to emphasise that the returned type should be disposed, either by a call to the Dispose() method or through recommended usage of the using statement.

@uluhonolulu If you're still up to implement this, could you please change the Clone() signature as part of #65? However, I'd prefer to deal with the update of Init() and the impact on the unit tests in a separate PR.

Thank you all, guys, for your feedback! 👍

Guys, sorry to dig up this thread, but I'd like you to peek at #453.

This new PR directly impacts back on this issue.