Tree to Tree diffing issue

[nulltoken]

Repository -> git://github.com/timburks/nu.git

Parent -> b46ed8403aba281d47619f71dd46fefa08e5c809
Commit -> d7a206818e70d753845fff30264fbae4a69ad7a0

Diffing those two commits creates two entries with the same name
See programming-nu/nu@d7a2068

=== Stack Trace ===
Error: An item with the same key has already been added.
    at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
    at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
    at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
    at LibGit2Sharp.TreeChanges.AddFileChange(GitDiffDelta delta) in 
C:\data\workspaces\git\libgit2sharp\LibGit2Sharp\TreeChanges.cs:line 98
    at LibGit2Sharp.TreeChanges.PrintCallBack(IntPtr data, GitDiffDelta delta, 
GitDiffRange range, GitDiffLineOrigin lineorigin, IntPtr content, UInt32 contentlen)
aces\git\libgit2sharp\LibGit2Sharp\TreeChanges.cs:line 53
    at LibGit2Sharp.Core.NativeMethods.git_diff_print_patch(DiffListSafeHandle diff, 
IntPtr data, git_diff_data_fn printCallback)
    at LibGit2Sharp.TreeChanges..ctor(DiffListSafeHandle diff) in 
C:\data\workspaces\git\libgit2sharp\LibGit2Sharp\TreeChanges.cs:line 41
    at LibGit2Sharp.Diff.Compare(Tree oldTree, Tree newTree) in 
C:\data\workspaces\git\libgit2sharp\LibGit2Sharp\Diff.cs:line 34

[nulltoken]

/cc @carlosmn

[arrbee]

Here is the relevant libgit2 code:

https://github.com/libgit2/libgit2/blob/development/src/diff.c#L481-483

When the type of a file changes (e.g. symlink -> regular file) then two entries will be created for the path, a deletion and a addition.

[nulltoken]

@arrbee Thanks a lot for this pointer.

When one gives a little thought about it, it indeed makes sense.

I'll recreate a test case as part of the fix to make sure this is covered.

[nulltoken]

@carlosmn I've got a slight issue

The two following tests pass in my native Win7/x86 environment against the latest tip of vNext. According to my understanding of your issue, they should fail :-/

The first one attempts at recreating the issue.
The second one was created out of despair and runs against a fresh clone of nu.git.

[Fact]
public void CanHandleTwoTreeEntryChangesWithTheSamePath()
{
    SelfCleaningDirectory scd = BuildSelfCleaningDirectory();

    using (Repository repo = Repository.Init(scd.DirectoryPath))
    {
        Blob mainContent = CreateBlob(repo, "awesome content\n");
        Blob linkContent = CreateBlob(repo, "../../objc/Nu.h");

        var tdOld = new TreeDefinition()
            .Add("include/Nu/Nu.h", linkContent, Mode.SymbolicLink)
            .Add("objc/Nu.h", mainContent, Mode.NonExecutableFile);

        Tree treeOld = repo.ObjectDatabase.CreateTree(tdOld);

        var tdNew = new TreeDefinition()
            .Add("include/Nu/Nu.h", mainContent, Mode.NonExecutableFile);

        Tree treeNew = repo.ObjectDatabase.CreateTree(tdNew);

        TreeChanges changes = repo.Diff.Compare(treeOld, treeNew);
        Assert.NotNull(changes);
    }
}

private static Blob CreateBlob(Repository repo, string content)
{
    using (var stream = new MemoryStream(Encoding.UTF8.GetBytes(content)))
    using (var binReader = new BinaryReader(stream))
    {
        return repo.ObjectDatabase.CreateBlob(binReader);
    }
}

[Fact]
public void Issue_196()
{
    using (var repo = new Repository(@"D:\Temp\sss\nu"))
    {
        var treeOld = repo.Lookup<Commit>("b46ed84").Tree;
        var treeNew = repo.Lookup<Commit>("d7a2068").Tree;

        TreeChanges changes = repo.Diff.Compare(treeOld, treeNew);
        Assert.NotNull(changes);
    }
}

You'll have to apply the following patch to allow the creation of a symlink:

diff --git a/LibGit2Sharp/TreeDefinition.cs b/LibGit2Sharp/TreeDefinition.cs
index 8a725a5..7da0eea 100644
--- a/LibGit2Sharp/TreeDefinition.cs
+++ b/LibGit2Sharp/TreeDefinition.cs
@@ -136,7 +136,7 @@ public TreeDefinition Add(string targetTreeEntryPath, Blob blob, Mode mode)
         {
             Ensure.ArgumentNotNull(blob, "blob");
             Ensure.ArgumentConformsTo(mode,
-                                      m => m.HasAny(new[] { Mode.ExecutableFile, Mode.NonExecutableFile, Mode.NonExecutableGroupWritableFile }), "mode");
+                                      m => m.HasAny(new[] { Mode.ExecutableFile, Mode.NonExecutableFile, Mode.NonExecutableGroupWritableFile, Mode.SymbolicLink }), "mode");

             TreeEntryDefinition ted = TreeEntryDefinition.From(blob, mode);

This libgit2 code https://github.com/libgit2/libgit2/blob/development/src/diff.c#L459-462 might explain why the tests pass on my computer. However, the stack trace you've provided looks like it's coming from a Win environment, too...

• Are you running the latest vNext? (Repository.Version outputs 0.9.5-unknown-1d94a7d (x86) on my box)
• Can you share some detail about the hosting platform where the exception was raised?
• Are you able to make the tests fail?

[carlosmn]

I't not based on vNext but v9.5.0 (because the modified library). I haven't had time to test it yet, though it looks like it should show the issue.

The issue did happen on a Windows system, so it's not libgit2 working around the issue. I'll investigate today.

[carlosmn]

Works for me. I.e. it fails. This is with your first two tests, so it looks like it's able to recreate the situation on Linux/Mono.

/home/carlos/git/libgit2sharp/CI-build.msbuild (default targets) ->
(Test target) ->

    /home/carlos/git/libgit2sharp/CI-build.msbuild: error : LibGit2Sharp.Tests.DiffTreeToTreeFixture.CanHandleTwoTreeEntryChangesWithTheSamePath: System.ArgumentException : An element with the same key already exists in the dictionary.
    /home/carlos/git/libgit2sharp/CI-build.msbuild: error :   at System.Collections.Generic.Dictionary`2[LibGit2Sharp.Core.FilePath,LibGit2Sharp.TreeEntryChanges].Add (LibGit2Sharp.Core.FilePath key, LibGit2Sharp.TreeEntryChanges value) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.TreeChanges.AddFileChange (LibGit2Sharp.Core.GitDiffDelta delta) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.TreeChanges.PrintCallBack (IntPtr data, LibGit2Sharp.Core.GitDiffDelta delta, LibGit2Sharp.Core.GitDiffRange range, GitDiffLineOrigin lineorigin, IntPtr content, UInt32 contentlen) [0x00000] in <filename unknown>:0 
  at (wrapper native-to-managed) LibGit2Sharp.TreeChanges:PrintCallBack (intptr,LibGit2Sharp.Core.GitDiffDelta,LibGit2Sharp.Core.GitDiffRange,LibGit2Sharp.Core.GitDiffLineOrigin,intptr,uint)
  at (wrapper managed-to-native) LibGit2Sharp.Core.NativeMethods:git_diff_print_patch (LibGit2Sharp.Core.Handles.DiffListSafeHandle,intptr,LibGit2Sharp.Core.NativeMethods/git_diff_data_fn)
  at LibGit2Sharp.TreeChanges..ctor (LibGit2Sharp.Core.Handles.DiffListSafeHandle diff) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Diff.Compare (LibGit2Sharp.Tree oldTree, LibGit2Sharp.Tree newTree, IEnumerable`1 paths) [0x00000] in <filename unknown>:0 
  at LibGit2Sharp.Tests.DiffTreeToTreeFixture.CanHandleTwoTreeEntryChangesWithTheSamePath () [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 

[nulltoken]

it's able to recreate the situation on Linux/Mono

That's a plus!

Have you had the chance to run it on Windows as well?

[carlosmn]

I takes more willpower than I had to start up the Windows VM, but I have to double-check whether something is a mono bug, so I might do it later.

[carlosmn]

Just tried on Windows and I can't reproduce it. I'll have to ask for more information.

[nulltoken]

@carlosmn I've started to work on a fix

@carlosmn @arrbee I have a slight doubt, as the Status API is diff based, could it be possible that another LibGit2Sharp bug might be hiding in this area as well? Maybe with the symlink in the Tree and a regular file in the workdir... What do you think?

[carlosmn]

That situation sounds like it should also have two distinct changes for the one file, so it should also be affected.

[nulltoken]

@carlosmn I've worked on a first fix https://github.com/nulltoken/libgit2sharp/tree/fix/issue-196

I'm not especially happy with the code, but that should do it for now. This will require some refactoring later, though as it's barely readable now.

Could you please fill in the gaps regarding assertions on Linux and make sure the code actually works?

[dahlbyk]

Not sure where we left off on this, but https://github.com/dahlbyk/libgit2sharp/compare/fix/issue196 has been rebased on latest.

It includes a standalone commit (dahlbyk@71eb036) with build.libgit2sharp.x64.cmd which might be worth cherry-picking into vNext.

[nulltoken]

It includes a standalone commit (dahlbyk/libgit2sharp@71eb036) with build.libgit2sharp.x64.cmd which might be worth cherry-picking into vNext.

I can't exactly remember why we would need this. Isn't the 32bits version of msbuild able to correctly produce an AnyCPU build?

[dahlbyk]

I don't know that there's any difference in the actual build, but tests run in 32-bit by default:

Test:
  xUnit.net MSBuild runner (32-bit .NET 4.0.30319.18010)
  xunit.dll:     Version 1.9.0.1566
  Test assembly: C:\Dev\GitHub\libgit2sharp\LibGit2Sharp.Tests\bin\Release\LibGit2Sharp.Tests.dll

With build.libgit2sharp.x64.cmd:

Test:
  xUnit.net MSBuild runner (64-bit .NET 4.0.30319.18010)
  xunit.dll:     Version 1.9.0.1566

[nulltoken]

@dahlbyk I wonder if we could tweak the build.libgit2sharp.cmd batch in order to make less "32-bits only"

From what I've read, %PROGRAMFILES(x86)% is only defined on x64 platforms.

Could you see if something like this would work on your computer, please?

SETLOCAL

SET BASEDIR=%~dp0
SET FrameworkVersion=v4.0.30319
SET Suffix=

IF DEFINED %PROGRAMFILES(x86)% SET Suffix=64

SET FrameworkDir=%SystemRoot%\Microsoft.NET\Framework%Suffix%
SET CommitSha=%~1

"%FrameworkDir%\%FrameworkVersion%\msbuild.exe" "%BASEDIR%CI-build.msbuild" /property:CommitSha=%CommitSha%

ENDLOCAL

EXIT /B %ERRORLEVEL%

[nulltoken]

@dahlbyk Forget about my last proposal. It might work, but wouldn't help ;-)

/cc @jamill @ben

[nulltoken]

We might need to run those tests in 32 and 64 bits mode, thus against the clr2.0 and clr4.0.

How about

• Adding the xUnit.console.*.exe runner binaries
• Replacing the *.cmd batch with a .ps1 script which would run msbuild (by specifying the Build target) then run a combination of tests depending on passed parameters

[dahlbyk]

Doing more than this at the moment really feels like YAGNI to me. We already have both architectures tested with CI, so this .cmd is really only useful if you're trying to debug something that works in x86 but not in x64.

[nulltoken]

Doing more than this at the moment really feels like YAGNI to me. We already have both architectures tested with CI, so this .cmd is really only useful if you're trying to debug something that works in x86 but not in x64.

Fair enough. I've cherry-picked your commit and will open an issue.

[nulltoken]

Not sure where we left off on this, but https://github.com/dahlbyk/libgit2sharp/compare/fix/issue196 has been rebased on latest.

See libgit2/libgit2#878.

I've slightly rewrote the tree-to-tree test in C# (cf. nulltoken/topic/type-change). However, I'm not really satisfied with it. I really don't like returning a IEnumerable from the TreeChanges indexer. I'd like to see if passing GIT_DIFF_INCLUDE_TYPECHANGE, and adding a new TypeChanged property could avoid this. While I'm at it, I may elaborate on @roend83's #278 and include rename detection (unless he's available so that we can collaborate).

The tree-to-workdir test still fails on Windows.

[roend83]

@nulltoken I'd be happy to help. Let me know what I can do.

[nulltoken]

@nulltoken I'd be happy to help. Let me know what I can do.

@roend83 Thanks a lot for this! It would be awesome if you could rebase #278 on top of vNext and cover it with some tests. Maybe one identifying the exact renamings and another one identifying the copying.

Would that fit you?

[roend83]

@nulltoken Yeah, I'll try to get to it tonight.

[nulltoken]

• topic/type-change has been updated and is ready for review
• Previous proposal has moved to topic/type-change-old

Ready for review.

[dahlbyk]

I like topic/type-change a lot better than the alternative.

Also, TreeChanges.DebuggerDisplay should totally use posh-git symbology for labeling added/modified/deleted. :)

[nulltoken]

@carlosmn This took some time. Sorry for that. It should now be fixed.

Also, TreeChanges.DebuggerDisplay should totally use posh-git symbology for labeling added/modified/deleted. :)

@dahlbyk Deal! Could you please send a PR covering this?