Straighten api

[nulltoken]

Pushing things left and right....

[travisbot]

This pull request passes (merged bd715e13 into 1e86842).

[travisbot]

This pull request passes (merged 05c3a297 into 1e86842).

[nulltoken]

@dahlbyk Thanks a lot for the review! You may have commented directly in the commits rather than in the diff of the pull request. This explains why my answers are not gathered in this thread.

The two comments below require some more specific explanation

"Doesn't it feel strange that this delegates to an extension method?" and "Exposing repo internally here strikes me as quite odd. If an extension is sufficiently complex that it needs innards, it seems to me that it shouldn't be an extension."

From the standpoint of the LibGit2Sharp project, I see string based methods as "less" core. Turning them into extension methods has the side benefit to also push them farther in the intellisense context menu.
I see extension methods as non essential ones. They're shortcuts.

The fact that some core methods delegate to an extension method is often the result of some effort in order to reduce the code duplication. I agree this is misleading and could benefit from some additional work :)

I agree that some LibGit2Sharp string based extensions methods are very "thick" and bear far too much "logic" for a binding. When they're bug free and proved useful enough, I'll convert them into C code and open a PR against libgit2.

[dahlbyk]

You may have commented directly in the commits rather than in the diff of the pull request.

I did indeed... sometimes it's harder to see in the overall diff how we got to where we ended up.

The fact that some core methods delegate to an extension method is often the result of some effort in order to reduce the code duplication.

I rarely disagree with reducing code duplication, and certainly not here.

I guess my disagreement would come from the blanket idea that using a string instead of an object necessarily makes it less deserving of a spot in the core API. If anything, I might argue the opposite - deleting/moving/etc by string is the more natural operation (after all, user input almost always comes in strings), and accepting a full object if we have one is a useful shortcut.

[nulltoken]

...how we got to where we ended up.

I agree with this. I also like a clean history :)

I guess my disagreement would come from the blanket idea that using a string instead of an object necessarily makes it less deserving of a spot in the core API.

Sorry, I wasn't very clear. I was not trying to be dogmatic. Far from it. Let me rephrase this in order to try to better express my intent:

I'd prefer to drive the consumer toward using the GitObject and ObjectId based methods because they are more straightforward and, generally speaking, less "magic" than the string based methods.

Some of those string based methods might even be too "magic" for the average user and more error prone (think Refs.Add(string, string) or Refs.UpdateTarget(string, string) for instance). If you also take into account the subtilties of the revparse syntax, you'd better be prepared to spend some quality time answering questions on StackOverflow.

Moreover, as they're a bit less straightforward (or a bit more complex), they might be a little bit less efficient than ObjectId based methods.

In less words, I generally see LibGit2Sharp string based methods as:

• More complex to grasp BUT barely documented
• More powerful BUT less efficient

Of course, I have no plan to deprecate them. However, I think they require some "above than average" knowledge to be properly used, though.