Update working directory on Checkout

[jamill]

This is an initial implementation of updating the working directory as part of Checkout (using git_checkout_head)

I am labeling this as a work in progress because I do not include any progress reporting (until the progress callbacks are in libgit2 and make it to LibGit2Sharp).

As there is not support for merge in libgit2, I only allow checking out the working directory if there are no changes in the current branch (staged, unstages, or untracked) by default. I do support taking a Force option, but it behaves slightly differently from git checkout -f on the command line (see below).

CheckoutOptions

I expose a bit flag enum CheckoutOptions on the Checkout() API. Currently, this only exposes a single flag (Force) - but once more checkout behavior is supported in libgit2 we can add extra flags here (Merge, AutoMerge, etc). One thing I am not sure of is how closely this particular API should match options exposed on the Git command line vs the flexibility exposed by libgit2.

As it is, CheckoutOptions.Force does not completly mimic git checkout -f branch, as git_checkout_head currently resets the whole working directory, while git checkout -f leaves untracked files. There is a small discussion on libgit2 checkout options at libgit2/libgit2#986

Any feedback is appreciated. I will close Issue #227, and continue the discussion on this thread.

[ben]

I like it. 😸

Have you thought about progress reporting? Libgit2 is moving to just callbacks for this; how do you envision the user passing in a delegate? (Obviously you wouldn't be able to write this until that PR is merged, but it bears thinking about.)

I ask because I've heard some good arguments against optional method parameters. What do you think about the proposal over on #223 for exposing operations with lots of optional parameters as separate classes?

[nulltoken]

Hmmm. Something may be fishy here. Checking a tag out should detach the HEAD.

[jamill]

Thanks - fixed this test.

[nulltoken]

@jamill Thanks a lot for contributing this. This is very promising!

while git checkout -f leaves untracked files. There is a small discussion on libgit2 checkout options at libgit2/libgit2#986

Have you tried the following?:

• Instead of checking the dirtiness of RepositoryStatus, making sure it only contains untracked entries
• Unsetting the CheckoutStrategy.GIT_CHECKOUT_REMOVE_UNTRACKED flag

[jamill]

Have you tried the following?:
•Instead of checking the dirtiness of RepositoryStatus, making sure it only contains untracked entries
•Unsetting the CheckoutStrategy.GIT_CHECKOUT_REMOVE_UNTRACKED flag

I have - However, unsetting this flag also results in files that exist in the current branch but that are not present in the checkout target not being removed. That is, if a.txt exist in branchA but not in branchB, then a.txt will not get removed when switching from branchA to branchB if this flag is not set.

[jamill]

@ben @nulltoken Thanks for the review!

Have you thought about progress reporting? Libgit2 is moving to just callbacks for this; how do you envision the user passing in a delegate? (Obviously you wouldn't be able to write this until that PR is merged, but it bears thinking about.)

Yes. I wouldn't expect it to be very disruptive to the current API. I was thinking that I would add another parameter for the delegate. I think it also wouldn't be too bad to not use optional parameters here, and just have a single overload:

Branch.cs:

public virtual void Checkout(CheckoutOptions checkoutOptions);
public virtual void Checkout(CheckoutOptions checkoutOptions, CheckoutProgressHandler progress);

Repository.cs:

public virtual void Checkout(string commitishOrBranchSpec, CheckoutOptions checkoutOptions);
public virtual void Checkout(string commitishOrBranchSpec, CheckoutOptions checkoutOptions, CheckoutProgressHandler progress);

In this case, CheckoutOptions is always a required parameter, but I don't think it adds to much complexity to the simple case (i.e. passing CheckoutOptions.None)...

[nulltoken]

I have - However, unsetting this flag also results in files that exist in the current branch but that are not present in the checkout target not being removed. That is, if a.txt exist in branchA but not in branchB, then a.txt will not get removed when switching from branchA to branchB if this flag is not set.

Dammit You're right!

This would require a ton of failing test to be built, but I think I might solve this with a diff involving the old Tree, the Index (updated from the target Tree) and the workdir.

@arrbee Have you got some code up you sleeve or can I give a try at this one?

[arrbee]

@nulltoken Interesting. This does seem like a design flaw in the checkout code. Simply doing a git_diff_workdir_to_index does not have enough information to distinguish between completely untracked files and untracked files that were in the old tree. To make matters worse, in the current design by the time we are in git_checkout_index() it is already a bit too late to recover the information.

I don't have any code sitting around -- if you want to look at it, that sounds great! I'm guessing you'll need to rearrange things so that both git_checkout_index and git_checkout_tree call a shared function, instead of just having the tree function call the index one. That should allow you to preserve the extra information to do the right thing...

[nulltoken]

I'm guessing you'll need to rearrange things

@arrbee Indeed, I was under the impression that this would require some interesting refactoring ;-)

[dahlbyk]

public virtual void Checkout(CheckoutOptions checkoutOptions, CheckoutProgressHandler progress);

👍 if you make that onProgress for consistency with Fetch().

[jamill]

if you make that onProgress for consistency with Fetch().

One weird thing is that this is not the same callback as the onProgress callback in Fetch(). This callback will match the callback for indexer progress. I will make sure to have this parameter name match the one we add for Fetch() when they are both available. onIndexerProgress.

I have updated the PR and moved the Checkout tests into their own fixture.

[jamill]

I have updated the PR to react to the newly exposed callbacks for checkout. Here is output from an example program calling checkout on a repository cloned from http://github.com/libgit2/TestGitRepository (from checkout master from first-merge):

(0/7),
(1/7), a/a1
(2/7), a/a1.txt
(3/7), a/a2.txt
(4/7), b/b1.txt
(5/7), b/b2.txt
(6/7), c/c1.txt
(7/7), c/c2.txt
(7/7),

With the following Checkout progress handler:

private static void CheckoutProgressHandler(string path, int completedSteps, int totalSteps)
{
      Console.WriteLine("({0}/{1}), {2}", completedSteps, totalSteps, path);
}

@ben are the duplicate 7/7 steps expected? the path string in this case in null (also null for the 0/7 step), maybe LibGit2Sharp should not call the progress callback if the path string is null?

[ben]

@jamill It's a small fencepost bug in libgit2. The initial (0/7), (null) is so that an good count of the steps can be performed. The final output was intended to ensure that 100% was reached, but it looks unnecessary now. I'm pushing a fix, so that nit will go away when we next update the native binaries.

[jamill]

hmm.. not sure about the build errors:

/home/travis/builds/libgit2/libgit2sharp/CI-build.msbuild: error:
LibGit2Sharp.Tests.RemoteFixture.CanFetchIntoAnEmptyRepository(url: "http://github.com/libgit2/TestGitRepository"): System.ArgumentNullException : Argument cannot be null.
Parameter name: src

and:

/home/travis/builds/libgit2/libgit2sharp/CI-build.msbuild: error :
at (wrapper managed-to-native) System.Runtime.InteropServices.Marshal:PtrToStructure (intptr,System.Type)
at LibGit2Sharp.Core.Handles.GitErrorSafeHandle.MarshalAsGitError () [0x00000] in :0
at LibGit2Sharp.Core.Ensure.Success (Int32 result, Boolean allowPositiveResult) [0x00000] in :0
at LibGit2Sharp.Core.Proxy.git_remote_connect (LibGit2Sharp.Core.Handles.RemoteSafeHandle remote, GitDirection direction) [0x00000] in :0
at LibGit2Sharp.Remote.Fetch (LibGit2Sharp.FetchProgress progress, TagFetchMode tagFetchMode, LibGit2Sharp.Handlers.ProgressHandler onProgress, LibGit2Sharp.Handlers.CompletionHandler onCompletion, LibGit2Sharp.Handlers.UpdateTipsHandler onUpdateTips) [0x00000] in :0
at LibGit2Sharp.Tests.RemoteFixture.CanFetchIntoAnEmptyRepository (System.String url) [0x00000] in
at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in :0

I do not see them locally, and this is not really a code path that I modified. Maybe an interaction with a transient network connection issue during fetch that is not handled appropriately (guessing)?

[nulltoken]

@jamill I just also hit the same error through the TeamCity CI server against 46548c5 (cf. log)

System.ArgumentNullException : Argument cannot be null.
Parameter name: src
  at (wrapper managed-to-native) System.Runtime.InteropServices.Marshal:PtrToStructure (intptr,System.Type)
  at LibGit2Sharp.Core.Handles.GitErrorSafeHandle.MarshalAsGitError () [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Core.Ensure.Success (Int32 result, Boolean allowPositiveResult) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Core.Proxy.git_remote_connect (LibGit2Sharp.Core.Handles.RemoteSafeHandle remote, GitDirection direction) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Remote.Fetch (LibGit2Sharp.FetchProgress progress, TagFetchMode tagFetchMode, LibGit2Sharp.Handlers.ProgressHandler onProgress, LibGit2Sharp.Handlers.CompletionHandler onCompletion, LibGit2Sharp.Handlers.UpdateTipsHandler onUpdateTips) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Tests.RemoteFixture.CanFetchAllTagsIntoAnEmptyRepository (System.String url) [0x00000] in <filename unknown>:0 
  at (wrapper managed-to-native) System.Reflection.MonoMethod:InternalInvoke (System.Reflection.MonoMethod,object,object[],System.Exception&)
  at System.Reflection.MonoMethod.Invoke (System.Object obj, BindingFlags invokeAttr, System.Reflection.Binder binder, System.Object[] parameters, System.Globalization.CultureInfo culture) [0x00000] in <filename unknown>:0 
« Hide stacktrace

[jamill]

@jamill I just also hit the same error through the TeamCity CI server against 46548c5 (cf. log)

I would have to investigate this some more... I suspect we are hitting some other intermittent error, and then when LibGit2Sharp calls to get the last error, the error has been cleared (or not set?)...

@nulltoken : I know there are some changes going on in libgit2 regarding checkout flags (libgit2/libgit2#1016). As it appears that the main change(s) will be the flags that are exposed (and we might have to switch which checkout_* method is called), what are your thoughts on bringing in this PR sooner? As I don't believe the basic flow of the checkout would change, and there are other PR's that are waiting on this on (#223), would it be reasonable to go ahead and bring this in? Unless there are other outstanding issues to address with this PR...

[nulltoken]

@jamill Dammit! I was under the impression that you were waiting after the checkout fix. Sorry for this.

I've slightly rewritten the history (cf. https://github.com/nulltoken/libgit2sharp/commits/checkout_workdir_rebased) to get rid of the review commits and get a coherent commit timeline.

The fixup commit is made of some proposals I'd like you to review. If you agree with them, I'll sqash them into your last commit.

I haven't had the time to perform a full review yet. I'll tackle this within the following hours.

[jamill]

@jamill Dammit! I was under the impression that you were waiting after the checkout fix. Sorry for this.

Not a problem - I was waiting to complete the Fetch update more than anything. Incorporating this PR would be helpful for me, as long as it makes sense (with the understanding that there will be reaction to updates in libgit2). If this is a burden on you, we can wait, but I wanted to at least get your thoughts on this.

The fixup commit is made of some proposals I'd like you to review. If you agree with them, I'll sqash them into your last commit.

👍

[nulltoken]

As it is, CheckoutOptions.Force does not completly mimic git checkout -f branch, as git_checkout_head currently resets the whole working directory, while git checkout -f leaves untracked files.

@jamill This looks pretty nice and ready tio be merged. One last nitpick: could you please make sure that we do not assert that an untracked file is being overwritten in the tests? This way, we'll avoid having passing tests today that will fail when libgit2/libgit2#1016 land.

[jamill]

@nulltoken Thank you for the review! There are not currently any tests that assert that untracked files are being overwritten. All of the tests either deal with:

• Branches without untracked files.
• Verifying that we complain when attempting to checkout a branch when there are existing untracked files.
• Verifying that staged changes are overwritten when forcefully checking out.

I did update one test (CanForcefullyCheckoutWithStagedChanges) to clarify that it only deals with staged changes. Should I squash the commits?

[nulltoken]

I did update one test (CanForcefullyCheckoutWithStagedChanges) to clarify that it only deals with staged changes.

Perfect! 👍

Should I squash the commits?

I'd only squash jamill/libgit2sharp@63fbb4c into jamill/libgit2sharp@c3d5d1d

[jamill]

I hit an issue with the checkout tests when run against the 64-bit version and the 3.5 CLR that I am working on tracking down.

[jamill]

Fixed the issue with the tests failing with the 64-bit 2.0 xunit test runner. The issue was with the checkout progress callback containing a custom marshaler - it appears that this causes some issues. I think I have seen similar issues discussed here in the past. The fix is to have the callback method take a pointer to the string, and then manually marshal the string.

[nulltoken]

Awesome job, and .... merged in vNext!

💖