Skip to content

Index collision fixes#4818

Merged
pks-t merged 2 commits intolibgit2:masterfrom
pks-t:pks/index-collision
Nov 13, 2018
Merged

Index collision fixes#4818
pks-t merged 2 commits intolibgit2:masterfrom
pks-t:pks/index-collision

Conversation

@pks-t
Copy link
Copy Markdown
Member

@pks-t pks-t commented Sep 21, 2018

This fixes our code in the following two scenarios:

  • adding an index entry "a" when "a/b" exists
  • adding an index entry "a/b" when "a" exists

In both cases, git.git simply removes the obstructing index entry and replaces it with the new one. But we currently fail to do so correctly -- instead, we just remove the old one, error out and then leave the caller with an index that lost data. Not good. So this PR fixes our code to also correctly add the new index entries as expected.

As always, please have a look at the commit messages to get some more rationale about the changes.

Supersedes #4814, fixes #4077

@pks-t pks-t force-pushed the pks/index-collision branch from ee23f75 to 929da3b Compare October 5, 2018 18:25
@pks-t
Copy link
Copy Markdown
Member Author

pks-t commented Oct 5, 2018

Rebased and amended to fix two memory leaks in the testsuite

Comment thread src/index.c

if (!git_object__is_valid(INDEX_OWNER(index), &entry->id,
git_object__type_from_filemode(entry->mode)))
git_object__type_from_filemode(entry->mode))) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be aligned with the if (, no?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's part of the parameters of the git_object__is_valid, so it should be aligned with its opening parenthesis. At least that's what I thought our style was, feel free to correct me

Comment thread src/index.c Outdated
path_length = ((struct entry_internal *)entry)->pathlen;
index_entry_adjust_namemask(entry, path_length);
/* Make sure that the path length flag is correct */
index_entry_adjust_namemask(entry, ((struct entry_internal *)entry)->pathlen);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is personal preference, but the old version looks clearer to me as we're not doing the casting and dereferencing in the function call.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've dropped this commit. I prefer not having the variable, but taste is different and I don't care that much

@carlosmn
Copy link
Copy Markdown
Member

Looks reasonable; the behaviour of the "check" functions is indeed confusing.

pks-t added 2 commits October 19, 2018 12:28
The current error hanling of the function `index_insert` is currently
very fragile. Instead of erroring out in case an error has happened, it
will instead verify that no error has happened for each statement. This
makes adding new code to that function an adventurous task.

Improve the situation by converting the function to use our typical
`goto out` pattern.
When adding an index entry "a/b/c" while an index entry "a/b" already
exists, git will happily remove "a/b/c" and only add the new index
entry:

    $ git init test
    Initialized empty Git repository in /tmp/test.repo/test/.git/
    $ touch x
    $ git add x
    $ rm x
    $ mkdir x
    $ touch x/y
    $ git add x/y
    $ git status
    A x/y

The other way round, adding an index entry "a/b" with an entry "a/b/c"
already existing is equivalent, where git will remove "a/b/c" and add
"a/b".

In contrast, libgit2 will currently fail to add these properly and
instead complain about the entry appearing as both a file and a
directory. This is a programming error, though: our current code already
tries to detect and, in the case of `git_index_add`, to automatically
replace such index entries. Funnily enough, we already remove the
conflicting index entries, but instead of adding the new entry we then
bail out afterwards. This leaves callers with the worst of both worlds:
we both remove the old entry but fail to add the new one.

The root cause is weird semantics of the `has_file_name` and
`has_dir_name` functions. While these functions only sound like they are
responsible for detecting such conflicts, they will also already remove
them in case where its `ok_to_replace` parameter is set. But even if we
tell it to replace such entries, it will return an error code.

Fix the error by returning success in case where the entries have been
replaced. Fix an already existing test which tested for wrong behaviour.
Note that the test didn't notice that the resulting tree had no entries.
Thus it is fine to change existing behaviour here, as the previous
result could've let to silently loosing data. Also add a new test that
verifies behaviour in the reverse conflicting case.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Inserting a file into an index which overwrites a directory silently discards the file when a tree is produced.

2 participants