git/read-cache.c, branch v2.19.2

split-index: smudge and add racily clean cache entries to split index

2018-10-11T22:23:29Z

Ever since the split index feature was introduced [1], refreshing a split index is prone to a variant of the classic racy git problem. Consider the following sequence of commands updating the split index when the shared index contains a racily clean cache entry, i.e. an entry whose cached stat data matches with the corresponding file in the worktree and the cached mtime matches that of the index: echo "cached content" >file git update-index --split-index --add file echo "dirty worktree" >file # size stays the same! # ... wait ... git update-index --add other-file Normally, when a non-split index is updated, then do_write_index() (the function responsible for writing all kinds of indexes, "regular", split, and shared) recognizes racily clean cache entries, and writes them with smudged stat data, i.e. with file size set to 0. When subsequent git commands read the index, they will notice that the smudged stat data doesn't match with the file in the worktree, and then go on to check the file's content and notice its dirtiness. In the above example, however, in the second 'git update-index' prepare_to_write_split_index() decides which cache entries stored only in the shared index should be replaced in the new split index. Alas, this function never looks out for racily clean cache entries, and since the file's stat data in the worktree hasn't changed since the shared index was written, it won't be replaced in the new split index. Consequently, do_write_index() doesn't even get this racily clean cache entry, and can't smudge its stat data. Subsequent git commands will then see that the index has more recent mtime than the file and that the (not smudged) cached stat data still matches with the file in the worktree, and, ultimately, will erroneously consider the file clean. Modify prepare_to_write_split_index() to recognize racily clean cache entries, and mark them to be added to the split index. Note that there are two places where it should check raciness: first those cache entries that are only stored in the shared index, and then those that have been copied by unpack_trees() from the shared index while it constructed a new index. This way do_write_index() will get these racily clean cache entries as well, and will then write them with smudged stat data to the new split index. This change makes all tests in 't1701-racy-split-index.sh' pass, so flip the two 'test_expect_failure' tests to success. Also add the '#' (as in nr. of trial) to those tests' description that were omitted when the tests expected failure. Note that after this change if the index is split when it contains a racily clean cache entry, then a smudged cache entry will be written both to the new shared and to the new split indexes. This doesn't affect regular git commands: as far as they are concerned this is just an entry in the split index replacing an outdated entry in the shared index. It did affect a few tests in 't1700-split-index.sh', though, because they actually check which entries are stored in the split index; a previous patch in this series has already made the necessary adjustments in 't1700'. And racily clean cache entries and index splitting are rare enough to not worry about the resulting duplicated smudged cache entries, and the additional complexity required to prevent them is not worth it. Several tests failed occasionally when the test suite was run with 'GIT_TEST_SPLIT_INDEX=yes'. Here are those that I managed to trace back to this racy split index problem, starting with those failing more frequently, with a link to a failing Travis CI build job for each. The highlighted line [2] shows when the racy file was written, which is not always in the failing test but in a preceeding setup test. t3903-stash.sh: https://travis-ci.org/git/git/jobs/385542084#L5858 t4024-diff-optimize-common.sh: https://travis-ci.org/git/git/jobs/386531969#L3174 t4015-diff-whitespace.sh: https://travis-ci.org/git/git/jobs/360797600#L8215 t2200-add-update.sh: https://travis-ci.org/git/git/jobs/382543426#L3051 t0090-cache-tree.sh: https://travis-ci.org/git/git/jobs/416583010#L3679 There might be others, e.g. perhaps 't1000-read-tree-m-3way.sh' and others using 'lib-read-tree-m-3way.sh', but I couldn't confirm yet. [1] In the branch leading to the merge commit v2.1.0-rc0~45 (Merge branch 'nd/split-index', 2014-07-16). [2] Note that those highlighted lines are in the 'after failure' fold, and your browser might unhelpfully fold it up before you could take a good look. Signed-off-by: SZEDER Gábor Signed-off-by: Junio C Hamano

Merge branch 'nd/no-the-index'

2018-08-20T18:33:53Z

The more library-ish parts of the codebase learned to work on the in-core index-state instance that is passed in by their callers, instead of always working on the singleton "the_index" instance. * nd/no-the-index: (24 commits) blame.c: remove implicit dependency on the_index apply.c: remove implicit dependency on the_index apply.c: make init_apply_state() take a struct repository apply.c: pass struct apply_state to more functions resolve-undo.c: use the right index instead of the_index archive-*.c: use the right repository archive.c: avoid access to the_index grep: use the right index instead of the_index attr: remove index from git_attr_set_direction() entry.c: use the right index instead of the_index submodule.c: use the right index instead of the_index pathspec.c: use the right index instead of the_index unpack-trees: avoid the_index in verify_absent() unpack-trees: convert clear_ce_flags* to avoid the_index unpack-trees: don't shadow global var the_index unpack-trees: add a note about path invalidation unpack-trees: remove 'extern' on function declaration ls-files: correct index argument to get_convert_attr_ascii() preload-index.c: use the right index instead of the_index dir.c: remove an implicit dependency on the_index in pathspec code ...

Merge branch 'en/abort-df-conflict-fixes'

2018-08-17T20:09:57Z

"git merge --abort" etc. did not clean things up properly when there were conflicted entries in the index in certain order that are involved in D/F conflicts. This has been corrected. * en/abort-df-conflict-fixes: read-cache: fix directory/file conflict handling in read_index_unmerged() t1015: demonstrate directory/file conflict recovery failures

dir.c: remove an implicit dependency on the_index in pathspec code

2018-08-13T21:14:42Z

Make the match_patchspec API and friends take an index_state instead of assuming the_index in dir.c. All external call sites are converted blindly to keep the patch simple and retain current behavior. Individual call sites may receive further updates to use the right index instead of the_index. Signed-off-by: Nguyễn Thái Ngọc Duy Signed-off-by: Junio C Hamano

Merge branch 'en/dirty-merge-fixes'

2018-08-02T22:30:45Z

The recursive merge strategy did not properly ensure there was no change between HEAD and the index before performing its operation, which has been corrected. * en/dirty-merge-fixes: merge: fix misleading pre-merge check documentation merge-recursive: enforce rule that index matches head before merging t6044: add more testcases with staged changes before a merge is invoked merge-recursive: fix assumption that head tree being merged is HEAD merge-recursive: make sure when we say we abort that we actually abort t6044: add a testcase for index matching head, when head doesn't match HEAD t6044: verify that merges expected to abort actually abort index_has_changes(): avoid assuming operating on the_index read-cache.c: move index_has_changes() from merge.c

Merge branch 'jm/cache-entry-from-mem-pool'

2018-08-02T22:30:43Z

For a large tree, the index needs to hold many cache entries allocated on heap. These cache entries are now allocated out of a dedicated memory pool to amortize malloc(3) overhead. * jm/cache-entry-from-mem-pool: block alloc: add validations around cache_entry lifecyle block alloc: allocate cache entries from mem_pool mem-pool: fill out functionality mem-pool: add life cycle management functions mem-pool: only search head block for available space block alloc: add lifecycle APIs for cache_entry structs read-cache: teach make_cache_entry to take object_id read-cache: teach refresh_cache_entry to take istate

read-cache: fix directory/file conflict handling in read_index_unmerged()

2018-07-31T19:51:11Z

read_index_unmerged() has two intended purposes: * return 1 if there are any unmerged entries, 0 otherwise * drops any higher-stage entries down to stage #0 There are several callers of read_index_unmerged() that check the return value to see if it is non-zero, all of which then die() if that condition is met. For these callers, dropping higher-stage entries down to stage #0 is a waste of resources, and returning immediately on first unmerged entry would be better. But it's probably only a very minor difference and isn't the focus of this series. The remaining callers ignore the return value and call this function for the side effect of dropping higher-stage entries down to stage #0. As mentioned in commit e11d7b596970 ("'reset --merge': fix unmerged case", 2009-12-31), The _only_ reason we want to keep a previously unmerged entry in the index at stage #0 is so that we don't forget the fact that we have corresponding file in the work tree in order to be able to remove it when the tree we are resetting to does not have the path. In fact, prior to commit d1a43f2aa4bf ("reset --hard/read-tree --reset -u: remove unmerged new paths", 2008-10-15), read_index_unmerged() did just remove unmerged entries from the cache immediately but that had the unwanted effect of leaving around new untracked files in the tree from aborted merges. So, that's the intended purpose of this function. The problem is that when directory/files conflicts are present, trying to add the file to the index at stage 0 fails (because there is still a directory in the way), and the function returns early with a -1 return code to signify the error. As noted above, none of the callers who want the drop-to-stage-0 behavior check the return status, though, so this means all remaining unmerged entries remain in the index and the callers proceed assuming otherwise. Users then see errors of the form: error: 'DIR-OR-FILE' appears as both a file and as a directory error: DIR-OR-FILE: cannot drop to stage #0 and potentially also messages about other unmerged entries which came lexicographically later than whatever pathname was both a file and a directory. Google finds a few hits searching for those messages, suggesting there were probably a couple people who hit this besides me. Luckily, calling `git reset --hard` multiple times would workaround this bug. Since the whole purpose here is to just put the entry *temporarily* into the index so that any associated file in the working copy can be removed, we can just skip the DFCHECK and allow both the file and directory to appear in the index. The temporary simultaneous appearance of the directory and file entries in the index will be removed by the callers by calling unpack_trees(), which excludes these unmerged entries marked with CE_CONFLICTED flag from the resulting index, before they attempt to write the index anywhere. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

Merge branch 'sb/object-store-grafts'

2018-07-18T19:20:28Z

The conversion to pass "the_repository" and then "a_repository" throughout the object access API continues. * sb/object-store-grafts: commit: allow lookup_commit_graft to handle arbitrary repositories commit: allow prepare_commit_graft to handle arbitrary repositories shallow: migrate shallow information into the object parser path.c: migrate global git_path_* to take a repository argument cache: convert get_graft_file to handle arbitrary repositories commit: convert read_graft_file to handle arbitrary repositories commit: convert register_commit_graft to handle arbitrary repositories commit: convert commit_graft_pos() to handle arbitrary repositories shallow: add repository argument to is_repository_shallow shallow: add repository argument to check_shallow_file_for_update shallow: add repository argument to register_shallow shallow: add repository argument to set_alternate_shallow_file commit: add repository argument to lookup_commit_graft commit: add repository argument to prepare_commit_graft commit: add repository argument to read_graft_file commit: add repository argument to register_commit_graft commit: add repository argument to commit_graft_pos object: move grafts to object parser object-store: move object access functions to object-store.h

merge-recursive: fix assumption that head tree being merged is HEAD

2018-07-11T16:38:36Z

`git merge-recursive` does a three-way merge between user-specified trees base, head, and remote. Since the user is allowed to specify head, we can not necesarily assume that head == HEAD. Modify index_has_changes() to take an extra argument specifying the tree to compare against. If NULL, it will compare to HEAD. We then use this from merge-recursive to make sure we compare to the user-specified head. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano

index_has_changes(): avoid assuming operating on the_index

2018-07-03T20:13:18Z

Modify index_has_changes() to take a struct istate* instead of just operating on the_index. This is only a partial conversion, though, because we call do_diff_cache() which implicitly assumes work is to be done on the_index. Ongoing work is being done elsewhere to do the remainder of the conversion, and thus is not duplicated here. Instead, a simple check is put in place until that work is complete. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano