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 /unpack-trees.c | |
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 'unpack-trees.c')
-rw-r--r-- | unpack-trees.c | 16 |
1 files changed, 8 insertions, 8 deletions
diff --git a/unpack-trees.c b/unpack-trees.c index 2763a029a1..b82c1a9705 100644 --- a/unpack-trees.c +++ b/unpack-trees.c @@ -595,13 +595,6 @@ static void mark_ce_used(struct cache_entry *ce, struct unpack_trees_options *o) { ce->ce_flags |= CE_UNPACKED; - /* - * If this is a sparse directory, don't advance cache_bottom. - * That will be advanced later using the cache-tree data. - */ - if (S_ISSPARSEDIR(ce->ce_mode)) - return; - if (o->cache_bottom < o->src_index->cache_nr && o->src_index->cache[o->cache_bottom] == ce) { int bottom = o->cache_bottom; @@ -1478,7 +1471,14 @@ static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, str * it does not do any look-ahead, so this is safe. */ if (matches) { - o->cache_bottom += matches; + /* + * Only increment the cache_bottom if the + * directory isn't a sparse directory index + * entry (if it is, it was already incremented) + * in 'mark_ce_used()' + */ + if (!src[0] || !S_ISSPARSEDIR(src[0]->ce_mode)) + o->cache_bottom += matches; return mask; } } |