Move Repository.Reset(paths) into Index

[bording]

Here is what I have so far. It's just about finished, but I had some questions, so I figured I'd go ahead and open the PR for changes I made for #805.

The two things I have left to do are add messages to the Obsolete attributes, and update the relevant tests so they aren't getting deprecation warnings. However, I wasn't sure which methods I should be using for the messages and the tests. Should they be pointing to the new Repository.ResetPaths() methods or to Index.Reset() instead?

To add the ResetPaths() methods I had to update the IRepository interface also. While doing this, I did start to wonder about how useful these new methods actually were vs. just calling Index.Reset() directly. Are these really needed?

Something else that I noticed while working on this is that the version of the method that takes a committish string has a couple of things I wanted to comment on.

• First, it's using optional parameters, and I know Remove default parameters from public API #952 is talking about removing them from the public API surface. I figured that was out of scope for this PR, so I left them as is.
• Secondly, when this method was in RepositoryExtensions it was using a private LookUpCommit() method. I changed this to use the internal Repository.LookupCommit() instead after verifying that it appeared to do the same thing.
• Lastly, why does this version of the method have its own IsBare check vs. relying on the one that's already in the Reset() it calls? Unless looking up the commit from the committish is really expensive, it seems like there isn't a good reason for the duplicate check.

[nulltoken]

First, it's using optional parameters, and I know #952 is talking about removing them from the public API surface. I figured that was out of scope for this PR, so I left them as is.

👍

To add the ResetPaths() methods I had to update the IRepository interface also. While doing this, I did start to wonder about how useful these new methods actually were vs. just calling Index.Reset() directly. Are these really needed?

That makes a lot of sense. Let's drop them from IRepository and Repository. Going one step further, as the Index is now a lower level kind of Api, I think, we should rename those Index.Reset() methods as additional overloads of Index.Replace().

[nulltoken]

Let's drop this overload. Defaulting to HEAD in the Index type makes little sense, now. The user will have to pass in a previously selected commit.

[nulltoken]

This should also resolve the duplicate call to IsBare 😉

[bording]

Ok, I'll go ahead and dump the Repository.ResetPaths() stuff and change the Index.Reset() methods to additional Index.Replace() overloads.

One potential issue I see with dumping the committish overload is that since it's the one with all the optional parameters, that is the overload that most of the tests are currently using. There are a lot of calls to Repository.Reset() with no arguments in the tests.

[bording]

One thing I noticed while changing the Reset() methods to Replace() overloads, the existing Replace() doesn't have the IsBare check like the old Reset() does.

I couldn't see a reason why one of them would need the check and the other wouldn't, but maybe I've overlooked something?

[nulltoken]

I couldn't see a reason why one of them would need the check and the other wouldn't, but maybe I've overlooked something?

You're right. It's indeed missing. Could you please add it in a separate commit?

[nulltoken]

There are a lot of calls to Repository.Reset() with no arguments in the tests.

@bording I must be missing something obvious. I only find 7 calls to it in the tests. Are we talking about the same method?

[bording]

That's the one I as talking about. I suppose "lots" might have been overstating things a bit, but those calls would all need to be changed to work with one of the other overloads. I guess I was just wanting to make sure you were ok with all of those calls needing to be changed before actually going through with removing the method.

[nulltoken]

I suppose "lots" might have been overstating things a bit

😄

I guess I was just wanting to make sure you were ok with all of those calls needing to be changed before actually going through with removing the method.

I think repo.Reset(repo.Head.Tip) would actually be a beneficial change (in the sense that it may be more explicit to the reader with regards to what is actually done).

[bording]

Getting close to having everything done!

As part of removing the committish overload, I ended up having to add a bit more logic back into the deprecated RepositoryExtensions version to make sure the tests using that one passed.

I then went through and got the tests working without any warnings, though in the process I ended up with a test that is failing. ResetANewlyInitializedNonBareRepositoryThrows() isn't seeing the expected UnbornBranchException because the call to Replace() has a null argument now.

I haven't had time yet to think about the best way to resolve that, so I figured I'd go ahead and get what I had pushed up. Would it make sense to add an IsHeadUnborn check too?

I still need to add the IsBare (and anything else that gets added to resolve the above failing test) to the other Replace() overload.

I also noticed that there is some inconsistency in the xml doc comments (Index vs. staging area). Worth taking the time to clean that up?

[nulltoken]

As part of removing the committish overload, I ended up having to add a bit more logic back into
the deprecated RepositoryExtensions version to make sure the tests using that one passed.

👍

I then went through and got the tests working without any warnings, though in the process I ended
up with a test that is failing. ResetANewlyInitializedNonBareRepositoryThrows() isn't seeing the
expected UnbornBranchException because the call to Replace() has a null argument now.

Just thinking out loud. Would it make sense to actually just drop this test as there wouldn't be any way to call Replace() without a proper Commit now?

Would it make sense to add an IsHeadUnborn check too?

I'm not sure. I don't see any issue with letting a user create an unborn branch and resetting the index from an existing commit. That may actually be a way to restart a "fresh" branch and dropping the commit history. Would that make sense?

I also noticed that there is some inconsistency in the xml doc comments (Index vs. staging area).
Worth taking the time to clean that up?

That would be super nice of you!

I still need to add the IsBare (and anything else that gets added to resolve the above failing test)
to the other Replace() overload.

You're doing a fantastic job ✨ Thanks a lot ‼️

[bording]

I'm not sure. I don't see any issue with letting a user create an unborn branch and resetting the index >from an existing commit. That may actually be a way to restart a "fresh" branch and dropping the >commit history. Would that make sense?

That sounds reasonable enough to me. I can't see that being used terribly often, but it seems like allowing it should be ok. It appears to be possible from the command line!

I'll go ahead and drop that test then.

[bording]

So it turns out the existing Replace() didn't need to have an IsBare check added after all because the Repository.Index property getter already has the check built in! 🎉

I updated the xml docs, but let me know if there is anything in there you'd prefer that I hadn't changed, and I'll revert it. I did end up going a bit beyond just changing 'staging area' to 'Index'.

Otherwise I think this is good to go now. Let me know and I'll squash it down and rebase to current vNext!

[nulltoken]

Otherwise I think this is good to go now. Let me know and I'll squash it down and rebase to current vNext!

👍

[bording]

Squashed and rebased!

[nulltoken]

....and merged! 💖

[nulltoken]

Published in prerelease NuGet package LibGit2Sharp.0.22.0-pre20150220065928.nupkg