Duplicate Filepaths in a commit causes failure when performing Diff between commit tree and its parent

[nulltoken]

Duplicate FilePaths in a commit causes failure when trying to perform Diff between commit tree and its parent
This happens in TreeChanges.cs class in AddChanges method, as Dictionary prevents addition of same key.

[ethomson]

A commit can't have duplicate file paths. Do you have repro steps?

[SinghVarun]

Under: https://android.googlesource.com/platform/ndk/
Commit Id - 6b9c9d8
if you do git show 6b9c9d8 --stat
and look for file "/arch-arm/usr/include/asm-generic/bitops/ffz.h" there are others as well.

[nulltoken]

Looks a lot like #196

@ethomson There was a attempt at fixing it in #201, which could be revived. But the API breaking change is less than nice. However, maybe would it be the good time to make this kind of decision 😉

[nulltoken]

Actually, I think I've been able to repro this by tweeking an actual test.

diff --git a/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs b/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs
index 99603b6..51fa4e0 100644
--- a/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs
+++ b/LibGit2Sharp.Tests/DiffTreeToTreeFixture.cs
@@ -911,7 +911,7 @@ public void CanHandleTwoTreeEntryChangesWithTheSamePath()

             using (var repo = new Repository(repoPath))
             {
-                Blob mainContent = OdbHelper.CreateBlob(repo, "awesome content\n");
+                Blob mainContent = OdbHelper.CreateBlob(repo, "awesome content\n" + new string('b', 4096));
                 Blob linkContent = OdbHelper.CreateBlob(repo, "../../objc/Nu.h");

                 string path = string.Format("include{0}Nu{0}Nu.h", Path.DirectorySeparatorChar);
@@ -930,7 +930,7 @@ public void CanHandleTwoTreeEntryChangesWithTheSamePath()
                 var changes = repo.Diff.Compare<TreeChanges>(treeOld, treeNew,
                     compareOptions: new CompareOptions
                     {
-                        Similarity = SimilarityOptions.None,
+                        Similarity = SimilarityOptions.Default,
                     });

                 /*

Applying the patch above generates the following

System.ArgumentExceptionAn item with the same key has already been added.
   at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
   at LibGit2Sharp.TreeChanges.AddFileChange(GitDiffDelta delta) in TreeChanges.cs: line 67
   at LibGit2Sharp.TreeChanges.FileCallback(GitDiffDelta delta, Single progress, IntPtr payload) in TreeChanges.cs: line 58
   at LibGit2Sharp.Core.NativeMethods.git_diff_foreach(DiffSafeHandle diff, git_diff_file_cb fileCallback, git_diff_hunk_cb hunkCallback, git_diff_line_cb lineCallback, IntPtr payload)
   at LibGit2Sharp.Core.Proxy.git_diff_foreach(DiffSafeHandle diff, git_diff_file_cb fileCallback, git_diff_hunk_cb hunkCallback, git_diff_line_cb lineCallback) in Proxy.cs: line 718
   at LibGit2Sharp.TreeChanges..ctor(DiffSafeHandle diff) in TreeChanges.cs: line 53
   at LibGit2Sharp.Diff.<.cctor>b__1a(DiffSafeHandle diff) in Diff.cs: line 96
   at LibGit2Sharp.Diff.Compare(Tree oldTree, Tree newTree, IEnumerable`1 paths, ExplicitPathsOptions explicitPathsOptions, CompareOptions compareOptions) in Diff.cs: line 158
   at LibGit2Sharp.Tests.DiffTreeToTreeFixture.CanHandleTwoTreeEntryChangesWithTheSamePath() in DiffTreeToTreeFixture.cs: line 930

[nulltoken]

@SinghVarun Can you please confirm the use case and the call stack?

[nulltoken]

FWIW, this test (ported from #201) passes

[Fact]
public void CanHandleTwoStatusEntryChangesWithTheSamePath()
{
    var path = InitNewRepository();

    using (Repository repo = new Repository(path))
    {
        Blob mainContent = OdbHelper.CreateBlob(repo, "awesome content\n");
        Blob linkContent = OdbHelper.CreateBlob(repo, "../../objc/Nu.h");

        const string filePath = "include/Nu/Nu.h";

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

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

        Commit commit = repo.ObjectDatabase.CreateCommit(Constants.Signature, Constants.Signature, "A symlink", tree, Enumerable.Empty<Commit>(), false);
        repo.Refs.UpdateTarget("HEAD", commit.Id.Sha);
        repo.Reset(ResetMode.Mixed);

        string parentPath = Path.Combine(repo.Info.WorkingDirectory, "include/Nu");

        Touch(parentPath, "Nu.h", "awesome content\n");

        RepositoryStatus status = repo.RetrieveStatus();

        Assert.Equal(2, status.Count());
        Assert.Equal(Path.Combine("include", "Nu", "Nu.h"), status.Modified.Single().FilePath);
        Assert.Equal(Path.Combine("objc", "Nu.h"), status.Missing.Single().FilePath);
    }
}

[nulltoken]

In order to move forward on this topic, I've added the hopefully failing tests. Let's see how the CI servers think of them.

[nulltoken]

@ethomson What should we assert there?

[nulltoken]

@SinghVarun @ethomson I've pushed a proposal. A breaking one.

Thoughts?

[nulltoken]

This result seems to not match libgit2 output. I may surely have messed my syntax somewhere.

@carlosmn @ethomson Any idea?

[nulltoken]

@carlosmn @ethomson Does this seem legit to you to get different outcomes depending on the platform?

[ethomson]

Probably not. Which one is the expected output?

[nulltoken]

Dunno. They both seem to be valid but expressed in different ways.

[nulltoken]

I'm going to move this test its strange output to a different PR in order keep this one scoped.

[nulltoken]

@SinghVarun Any feedback?

[SinghVarun]

@nulltoken Sorry for delay in response. Was testing this fix on multiple repository. Things look great.
Thanks all, once again for doing the quick fix.

[nulltoken]

Published as pre-release NuGet package LibGit2Sharp.0.22.0-pre20150324065655