Tests for case sensitive conflicts in the index

[ethomson]

So after looking at #344 , I wanted to add a test case that had conflicts in the index that differed only in case.

core git will allow a merge of two branches that contain items differ in case, regardless of your platform or your core.ignorecase settings and we should, I would think, be able to handle those conflicts.

My thought was that these tests would simply pass against the current code base, but to my surprise, they do not...! I have not yet been able to debug this, but it appears that ignorecase is a deeper setting than I had imagined.

I did want to push up this test, though, to illustrate a problem that can arise and start some discussion.

/cc @nulltoken
/cc @dahlbyk

[dahlbyk]

Thanks for this - there's definitely something off here. I've extended your tests here: https://github.com/dahlbyk/libgit2sharp/compare/gh345

• With ignorecase = false, everything work as expected.
• With ignorecase = true, I get inconsistent results:
  • Lookup by path returns matching a/o/t regardless of case, equivalent to a/o/t for Differs-In-Case.txt.
  • Enumerating the index yields an identical result to ignorecase = false, but I have to proactively sort Index case-sensitively to get the groups of entries to line up.

[ethomson]

Yeah, that's what I was seeing, too. I should have written tests for this earlier, because this seems like madness, but I thought I knew how it worked.

[nulltoken]

@ethomson Here's a failing libgit2 test reproducing the issue

void test_index_conflicts__case_matters(void)
{
    git_index_entry *conflict_entry[3];
    git_oid oid;
    char *upper_case = "DIFFERS-IN-CASE.TXT";
    char *mixed_case = "Differs-In-Case.txt";

    git_index_entry ancestor_entry, our_entry, their_entry;

    memset(&ancestor_entry, 0x0, sizeof(git_index_entry));
    memset(&our_entry, 0x0, sizeof(git_index_entry));
    memset(&their_entry, 0x0, sizeof(git_index_entry));

    ancestor_entry.path = upper_case;
    ancestor_entry.flags |= (1 << GIT_IDXENTRY_STAGESHIFT);
    git_oid_fromstr(&ancestor_entry.oid, CONFLICTS_ONE_ANCESTOR_OID);

    our_entry.path = upper_case;
    ancestor_entry.flags |= (2 << GIT_IDXENTRY_STAGESHIFT);
    git_oid_fromstr(&our_entry.oid, CONFLICTS_ONE_OUR_OID);

    their_entry.path = upper_case;
    ancestor_entry.flags |= (3 << GIT_IDXENTRY_STAGESHIFT);
    git_oid_fromstr(&their_entry.oid, CONFLICTS_ONE_THEIR_OID);

    cl_git_pass(git_index_conflict_add(repo_index,
        &ancestor_entry, &our_entry, &their_entry));

    ancestor_entry.path = mixed_case;
    ancestor_entry.flags |= (1 << GIT_IDXENTRY_STAGESHIFT);
    git_oid_fromstr(&ancestor_entry.oid, CONFLICTS_TWO_ANCESTOR_OID);

    our_entry.path = mixed_case;
    ancestor_entry.flags |= (2 << GIT_IDXENTRY_STAGESHIFT);
    git_oid_fromstr(&our_entry.oid, CONFLICTS_TWO_OUR_OID);

    their_entry.path = mixed_case;
    ancestor_entry.flags |= (3 << GIT_IDXENTRY_STAGESHIFT);
    git_oid_fromstr(&their_entry.oid, CONFLICTS_TWO_THEIR_OID);

    cl_git_pass(git_index_conflict_add(repo_index,
        &ancestor_entry, &our_entry, &their_entry));

    cl_git_pass(git_index_conflict_get(&conflict_entry[0], &conflict_entry[1],
        &conflict_entry[2], repo_index, upper_case));

    cl_assert_equal_s(upper_case, conflict_entry[0]->path);
    cl_assert_equal_i(true,
        git_oid_streq(&conflict_entry[0]->oid, CONFLICTS_ONE_ANCESTOR_OID) == 0);

    cl_assert_equal_s(upper_case, conflict_entry[1]->path);
    cl_assert_equal_i(true,
        git_oid_streq(&conflict_entry[1]->oid, CONFLICTS_ONE_OUR_OID) == 0);

    cl_assert_equal_s(upper_case, conflict_entry[0]->path);
    cl_assert_equal_i(true,
        git_oid_streq(&conflict_entry[2]->oid, CONFLICTS_ONE_THEIR_OID) == 0);

    cl_git_pass(git_index_conflict_get(&conflict_entry[0], &conflict_entry[1],
        &conflict_entry[2], repo_index, mixed_case));

    cl_assert_equal_s(mixed_case, conflict_entry[0]->path);
    cl_assert_equal_i(true,
        git_oid_streq(&conflict_entry[0]->oid, CONFLICTS_TWO_ANCESTOR_OID) == 0);

    cl_assert_equal_s(mixed_case, conflict_entry[1]->path);
    cl_assert_equal_i(true,
        git_oid_streq(&conflict_entry[1]->oid, CONFLICTS_TWO_OUR_OID) == 0);

    cl_assert_equal_s(mixed_case, conflict_entry[0]->path);
    cl_assert_equal_i(true,
        git_oid_streq(&conflict_entry[2]->oid, CONFLICTS_TWO_THEIR_OID) == 0);
}

From what I've understood current behavior comes from index.c::250.

[arrbee]

@nulltoken This is great - i.e. having a failing libgit2 test. I have come to worry about our strategy of actually storing the index internally case insensitively (since the core git index never is). I'm interested in how this pans out and in helping if I can. We may be able to use the secondary index technique that I used to add case-insensitivity to tree iterators to support efficient index use on case insensitive platforms without altering the internal representation to be incompatible with git.

[dahlbyk]

Updated https://github.com/dahlbyk/libgit2sharp/compare/gh345

@arrbee Was a libgit2 issue ever created for this? I couldn't find one...

[nulltoken]

@dahlbyk, see @yorah's comment in libgit2/libgit2#1499

[yorah]

@dahlbyk @nulltoken @arrbee the C test is still failing (as it relies on the index capabilities, so libgit2/libgit2#1499 doesn't help here).

[nulltoken]

Issue created in libgit2 -> libgit2/libgit2#2534

[carlosmn]

The C version of the test has been merged, making adding conflicts align with adding hand-crafted index entries in that we take whatever string was passed in, potentially changing the existing case in case-insensitive indices.

Do we still want to do something here?

[carlosmn]

Since this changes so many files marked as binary and we did merge the C test, I'm going to close this as resolved and we'll let the library say what's right in this.