Commit leaves inverted index, and Reset jumps back too far

[AArnott]

Nerdbank.GitVersioning has tests that pass when run with libgit2sharp 0.22.0, but one test (GetVersionHeight_VersionJsonHasParsingErrorsInHistory) fails when run against 0.23.1.

Specifically, it appears that when I call

this.Repo.Index.Add(relativeFilePath);
this.Repo.Commit("some text");

This leaves the repo in an odd state. The commit is made, but then the inverse of that commit is staged to the index.

Later, when I call Reset to move the head and index back one step but without touching the work tree,

this.Repo.Reset(ResetMode.Mixed, this.Repo.Head.Commits.Skip(1).First());

This jumps HEAD all the way back to the start of the repo (2 steps instead of just 1).

[ethomson]

That's pretty strange... there really haven't been that many changes between 0.22 and 0.23. And as far as libgit2 goes, I can't think of any changes that would change the very basic add + commit scenarios. And having the index get reverted to HEAD~1 really doesn't make any sense.

Do you have any other unit tests that do an add and commit that are failing in a similar way?

[AArnott]

This is the only one that seems impacted. I suspect it's because it's the only test that uses the reset method. I plan to add a unit test to this repository in an attempt to Repro it.

[AArnott]

I have a repro in a simple unit test within this repo. I've pushed the commit that adds it on v0.22 where the test passes. If you merge in v0.23.1 the test fails, as advertised.

I used git bisect with the script below to determine that the commit that introduces the regression is 82d621c:

pushd "%~dp0"
git cherry-pick fix1412
git reset --mixed HEAD~1
C:\tools\NuGet.exe restore
msbuild /nologo /m /fl /v:minimal
.\packages\xunit.runner.console.2.0.0\tools\xunit.console.x86.exe .\LibGit2Sharp.Tests\bin\Debug\LibGit2Sharp.Tests.dll -method "LibGit2Sharp.Tests.CommitFixture.CommitLeavesIndexIntact"
git reset --hard
popd

[AArnott]

BTW, if as you look at the unit test you can think of a workaround I can use in the meantime, I'd be very grateful. I'm trying to adapt Nerdbank.GitVersioning to the portable version of libgit2sharp, but that requires that I take the update that is buggy, which actually won't matter at runtime since I don't author commits but for tests it messes me up.

[ethomson]

So: repo.Index.Add(...) will add a file to the index but it doesn't write the index. repo.Commands.Stage (or just repo.Stage in 0.22, deprecated in 0.23) will Add and Write the index.

So that change isn't actually being committed.

To be completely honest, I'm not entirely sure when this changed. I wouldn't have thought the libgit2 update would have changed things, but I'll take a closer look.

But as for workarounds, use Stage (or Add then Write) and that should be solid.

[ethomson]

Ah, it turns out that the Index methods used to update the index after operating, which meant that there was no way to operate on the index in-memory. So we split things up into Add (which is a more "plumbing" method) and Stage (which is a more "porcelain" command and does the save).

Looking back on #1291, it looks like we may have done a poor job of documenting this. Apologies for that.

[AArnott]

Thanks for the review, @ethomson. I tried swapping out all the Index commands for the Commands.Stage method but the bug still repros.

[ethomson]

That's odd, it does not for me. I added your test to 0.23.1 and it does fail. But after changing things to Stage, it passes.

When you say that the bug repros, are you saying that your LibGit2Sharp test fails, or that it fails within GitVersioning? (Or both.)

[AArnott]

So that change isn't actually being committed.

Yes it is. I can see in git.exe history that the commit is valid. It's only the index after the commit that's messed up.

[AArnott]

My LibGit2Sharp test certainly fails. I haven't tried changing everything in NB.GV yet.

Here is my commit that fixes the test to use Stage: 734fc4f. It's the tip of the fix1412 branch in my fork. Can you see what I did differently than you did?

[AArnott]

BTW, I really appreciate how easy it is to add a unit test to this library. That's the way to do it. :)

[carlosmn]

So that change isn't actually being committed.

Yes it is. I can see in git.exe history that the commit is valid. It's only the index after the commit that's messed up.

This is likely due to some sloppy object lifetime management. What the correct thing to do is a fight for a different day, but what is most likely happening here is that we're re-using the same git_index object attached to the repository. By using the Repository.Index getter, the Index.Add() acts on that instance. Then Repository.Commit() takes in that same object and writes it out to a tree, making the commit "valid", or at least the expected value.

Since this index state is never persisted to disk, git diff and friends show the old index. You would see the same thing if you re-read the index file yourself. There is no "reversed" index because it doesn't store changes, but represents a state. The index is a commit behind, so it looks like you undid your changes in the index (since it's the same state that you'd get if you did that).

The reset methods (beyond soft) do write out the given commit's tree to the index file, but we don't refresh automagically most of the time, so that might explain why you see some difference.

[AArnott]

Thanks, @carlosmn. I'll disregard the state of the index then. But when I leave git.exe out of it and just run the test, it ultimately fails because the reset-mixed command does something different still. Do we have any insight into that at this point?

[carlosmn]

My guess would be that Repository.Commits underspecifies the order it expects to see commits in. Either way, HEAD~1 is not the same as Repository.Commits.Skip(1) except for some pretty specific circumstances around the ordering of the output. If you want to specify "the parent of the current HEAD" then you should either do it by hand via a lookup and access to its first parent, or by doing the equivalent of rev-list rev-parse.

Can you check whether the output of this.Repo.Commits stays the same for the repository you're testing with?