diff options
author | Victoria Dye <vdye@github.com> | 2022-03-17 18:55:35 +0300 |
---|---|---|
committer | Junio C Hamano <gitster@pobox.com> | 2022-03-17 19:13:21 +0300 |
commit | bfc763df773ccfc10eb38a24caa111d0fcbc5493 (patch) | |
tree | cd9fbc41128666f189d0262edc1ddd2f98dd56f6 /t | |
parent | c3a9cecc7f5ea8b36d6943b868122ca0bfbc0f9c (diff) |
unpack-trees: increment cache_bottom for sparse directories
Correct tracking of the 'cache_bottom' for cases where sparse directories
are present in the index.
BACKGROUND
----------
The 'unpack_trees_options.cache_bottom' is a variable that tracks the
in-progress "bottom" of the cache as 'unpack_trees()' iterates through the
contents of the index. Most importantly, this value informs the sequential
return values of 'next_cache_entry()' which, in the "diff cache" usage of
'unpack_callback()', are either unpacked as-is or are passed into the diff
machinery.
The 'cache_bottom' is intended to track the position of the first entry in
the index that has not yet been diffed or unpacked. It is advanced in two
main ways: either it is incremented when an index entry is marked as "used"
(in 'mark_ce_used()'), indicating that it was unpacked or diffed, or when a
directory is unpacked, in which case it is increased by an amount equaling
the number of index entries inside that tree.
In 17a1bb570b (unpack-trees: preserve cache_bottom, 2021-07-14), it was
identified that sparse directories posed a problem to the above
'cache_bottom' advancement logic - because a sparse directory was both an
index entry that could be "used" and a directory that can be unpacked, the
'cache_bottom' would be incremented too many times. To solve this problem,
the 'mark_ce_used()' advancement of 'cache_bottom' was skipped for sparse
directories.
INCORRECT CACHE_BOTTOM TRACKING
-------------------------------
Skipping the 'cache_bottom' advancement for sparse directories in
'mark_ce_used()' breaks down in two cases:
1. When the 'unpack_trees()' operation is *not* a "cache diff" (because the
directory contents-based incrementing of 'cache_bottom' does not happen).
2. When a cache diff is performed with a pathspec (because
'unpack_index_entry()' will unpack a sparse directory not matched by the
pathspec without performing the directory contents-based increment).
The former luckily does not appear to affect 'git' behavior, likely because
'cache_bottom' is largely unused (non-"cache diff" 'unpack_trees()' uses
'find_index_entry()' - rather than 'next_cache_entry()' - to find the index
entries to unpack).
The latter, however, causes 'cache_bottom' to "lag behind" its intended
position by an amount equal to the number of sparse directories unpacked so
far with 'unpack_index_entry()'. If a repository is structured such that any
sparse directories are ordered lexicographically *after* any
pathspec-matching directories, though, this issue won't present any adverse
behavior.
This was the case with the 't1092-sparse-checkout-compatibility.sh' tests
before the addition of the 'before/' sparse directory (ordered *before* the
in-cone 'deep/' directory), therefore sidestepping the issue. Once the
'before/' directory was added, though, 'cache_bottom' began to lag behind
its intended position, causing 'next_cache_entry()' to return index entries
it had already processed and, ultimately, an incorrect diff.
CORRECTING CACHE_BOTTOM
-----------------------
The problems observed in 't1092' come from 'cache_bottom' lagging behind in
cases where the cache tree-based advancement doesn't occur. To solve this,
then, the fix in 17a1bb570b is "reversed"; rather than skipping
'cache_bottom' advancement in 'mark_ce_used()', we skip the directory
contents-based advancement for sparse directories. Now, every index entry
can be accounted for in 'cache_bottom':
* if you're working with a single index entry, 'cache_bottom' is incremented
in 'mark_ce_used()'
* if you're working with a directory that contains index entries (but is not
one itself), 'cache_bottom' is incremented by the number of entries in
that directory.
Finally, change the 'test_expect_failure' tests in 't1092' failing due to
this bug back to 'test_expect_success'.
Signed-off-by: Victoria Dye <vdye@github.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Diffstat (limited to 't')
-rwxr-xr-x | t/t1092-sparse-checkout-compatibility.sh | 6 |
1 files changed, 3 insertions, 3 deletions
diff --git a/t/t1092-sparse-checkout-compatibility.sh b/t/t1092-sparse-checkout-compatibility.sh index dcd7061fb3..236ab53028 100755 --- a/t/t1092-sparse-checkout-compatibility.sh +++ b/t/t1092-sparse-checkout-compatibility.sh @@ -340,7 +340,7 @@ test_expect_success 'deep changes during checkout' ' test_all_match git checkout base ' -test_expect_failure 'add outside sparse cone' ' +test_expect_success 'add outside sparse cone' ' init_repos && run_on_sparse mkdir folder1 && @@ -382,7 +382,7 @@ test_expect_success 'commit including unstaged changes' ' test_all_match git status --porcelain=v2 ' -test_expect_failure 'status/add: outside sparse cone' ' +test_expect_success 'status/add: outside sparse cone' ' init_repos && # folder1 is at HEAD, but outside the sparse cone @@ -593,7 +593,7 @@ test_expect_success 'checkout and reset (keep)' ' test_all_match test_must_fail git reset --keep deepest ' -test_expect_failure 'reset with pathspecs inside sparse definition' ' +test_expect_success 'reset with pathspecs inside sparse definition' ' init_repos && write_script edit-contents <<-\EOF && |