Fix issues caused by batch file staging/discard changes

[lapfelix]

To address #638

I'm still not sure about removing GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH as a checkout option since it's an option that makes sense considering we don't use the matching features. But there's clearly an issue with it when we're doing a checkout with multiple paths.

I'll keep testing this branch over the next few days.

Also includes the original changes from #629

[lapfelix]

@lucasderraugh I'm pretty confident in these changes. Especially with this last commit where we correctly escape the paths.

I tried without escaping the paths (and without the GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH parameter) and if I had a file named Test *.txt which I discarded changes from, it would recognize a pattern and also discard all changes from files named Test 1.txt, Test 2.txt, Test 3.txt, etc. Escaping the file paths we pass on to libgit2 fixes this.

[lucasderraugh]

@lapfelix Sorry I forgot to push my revert of your original changes to remote. You can easily fix this branch by reverting my revert. Sorry for the headache.

[lapfelix]

@lucasderraugh No worries! It's easy to fix with GitUp 😄

[lucasderraugh]

So it was ultimately a bug in libgit2 where disabling matching just doesn't work correctly? It does feel like we're working around the issue here right? Maybe I should try that merge into our version of libgit2 to see if that resolves the original issue. I'm a little hesitant to work around the pattern matching aspect.

[lapfelix]

Yes it's definitely a workaround. I could try and see if I can write a unit test for our branch of libgit2 that reproduces the issue, which I could then test on a recent release of libgit2 to see if they've fixed it (or if the issue is specific to our branch).

[lucasderraugh]

@lapfelix If you have the time to test that out, it would be incredibly helpful. I probably won't get around to it until this coming weekend either way so if you have time before then, be my guest 😄

[lapfelix]

Ugh I can't reproduce the issue in libgit2. I wrote a test that did what I did to reproduce the issue which was to creates multiple files, commits them, makes more changes to every file I created, then I would delete 3 files then perform a checkout on these 3 files (which is what GitUpKit does). And it works fine in our fork and in the main repo.

[lucasderraugh]

Hmm, well that's odd. Maybe we can work the other way where we eliminate some factors on the GitUp app and see if the CLI git reflects it correctly without trying to get updates in the UI. Perhaps something else is effecting this? I'll try to get through some of these PRs this weekend.

[lapfelix]

I dug deeper and I don't think it's about the UI. I can really see that it happens in the checkout libgit2 function and that its new iterator is returning all of the files that start with the shared prefix. So in my case I was discarding the changes to aaa1, aaa2 and aaa3 (but not aaa4 and aaa5). Libgit2 figured out the common prefix was aaa. It created an "old iterator" correctly, but the "new_iterator" (specifically in git_diff__from_iterators) had the prefix set but nothing in its pathlist so it just iterates on every file that starts with aaa.

Here the oitem (which stands for "old item". The variable names are hard to understand for someone not familiar at all with the codebase) returned by the old iterator has a NULL path and the nitem (new item) has its path set to SceneDelegate copy 14.swift which is the equivalent of aaa4 in my example scenario above (I didn't discard this file but it starts with the same prefix as the files I did select which is SceneDelegate copy 1 and in my case all of the files that start with that prefix get removed). Which shows that the problem really comes from the new_iterator passed to git_diff__from_iterators being wrong.

And to probably prove my theory, when I return false in iterator_pathlist_next_is(iterator, path) when the iterator's pathlist is empty, like this:

..the checkout works fine and only deletes the right files. It's not meant to be a fix at all though since it (predictably) breaks a ton of other stuff.

Still looks like a bug to me and the solution of removing GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH and properlly escaping the paths still seems like the right one to me. I'll see if I can dig deeper to maybe open an issue on the libgit2 repo.

[lapfelix]

Oh hey I reproduced the issue in a libgit2 test! I was using the wrong method to reproduce it. Everything is fine with git_checkout_tree but I can reproduce the bug with git_checkout_index!

So it really is a libgit2 bug. And I can reproduce the bug with my test in our libgit2 fork and in the main libgit2 repo (on the master branch).

I'll clean up my test and make it as simple as possible and I'll post it here and open an issue on the libgit2 repo.

[lucasderraugh]

Awesome work 👏. I’ll try taking the change you proposed here when I go through PRs tomorrow. Let me know when you create a PR/Issue on libgit2.

[lapfelix]

I just opened the libgit2 issue: libgit2/libgit2#5377

Also I noticed that I can reproduce it in the current shipping version of GitUp (1.1.1, not 1.1.2). If your files are named:
test*
test1
test2
test3

and you have changes on all 4 and you discard the changes made to test*, the changes for all of the files that start with test will be discarded too. It was more obvious with my batch changes because libgit2 would figure out a common prefix for all of the selected files and then include all of the other files with the same prefix (that last part is the bug).

And all of this made me realize that I don't think we need to delete files before doing the checkout operation. Pretty sure that's already done in the libgit2 checkout.

[ethomson]

Oh hey I reproduced the issue in a libgit2 test! I was using the wrong method to reproduce it. Everything is fine with git_checkout_tree but I can reproduce the bug with git_checkout_index!

Yes, oops. We simply ignored the pathspec entirely in git_checkout_index when GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH was specified. 😰

Sorry about this - indeed, re-enabling pathspec matching should be a suitable workaround if you escape any fnmatch wildcards - but it will also be a little bit slower (by virtue of needing to do wildcard matching instead of literal matching).

Thanks much for all the helpful reproduction steps and the investigation!

[lapfelix]

Works fine after cherry picking the changes from libgit2/libgit2#5378 (with GIT_CHECKOUT_DISABLE_PATHSPEC_MATCH and without escaping the paths) 🎉

So either we only cherry pick the changes or we update GitUp's libgit2 fork.

[lucasderraugh]

Thanks @lapfelix! Tomorrow morning I'm going to try merging original libgit2 into our fork and see how awful the changes will be for that. Currently that PR on libgit2 is failing so I would want to make sure there aren't other issues that would arise from this change. Assuming the resolution of the PR is that some other tests are just wrong and the change is actually correct, we may just cherry pick the changes across.

[lucasderraugh]

May have taken 2+ years...but let's merge it now that we're finally onto 1.4.4 for libgit2.

[lapfelix]

yyyeEEEESSSSSSS thank you @lucasderraugh