Skip to content

Tests for case sensitive conflicts in the index #345

Closed
wants to merge 1 commit into from

6 participants

@ethomson
libgit2 member

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
libgit2 member
dahlbyk commented Feb 20, 2013

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
libgit2 member

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.

@dahlbyk dahlbyk referenced this pull request Feb 20, 2013
Closed

Ignore Case via Repository.PathComparer #344

2 of 3 tasks complete
@nulltoken
libgit2 member

@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
libgit2 member
arrbee commented Feb 21, 2013

@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
libgit2 member
dahlbyk commented Apr 25, 2013

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

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

@nulltoken
libgit2 member
@yorah
yorah commented Apr 26, 2013

@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
libgit2 member

Issue created in libgit2 -> libgit2/libgit2#2534

@carlosmn
libgit2 member

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
libgit2 member

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.

@carlosmn carlosmn closed this Apr 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.