On Thu, Dec 13, 2012 at 9:04 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>> @@ -324,7 +325,14 @@ static int update_one(struct cache_tree *it,
>> if (!sub)
>> die("cache-tree.c: '%.*s' in '%s' not found",
>> entlen, path + baselen, path);
>> - i += sub->cache_tree->entry_count - 1;
>> + i--; /* this entry is already counted in "sub" */
>
> Huh?
>
> The "-1" in the original is the bog-standard compensation for the
> for(;;i++) loop.
Exactly. It took me a while to figure out what " - 1" was for and I
wanted to avoid that for other developers. Only I worded it badly.
I'll replace the for loop with a while loop to make it clearer...
>
>> + if (sub->cache_tree->entry_count < 0) {
>> + i -= sub->cache_tree->entry_count;
>> + sub->cache_tree->entry_count = -1;
>> + to_invalidate = 1;
>> + }
>> + else
>> + i += sub->cache_tree->entry_count;
>
> While the rewritten version is not *wrong* per-se, I have a feeling
> that it may be much easier to read if written like this:
>
> if (sub->cache_tree_entry_count < 0) {
> to_invalidate = 1;
> to_skip = 0 - sub->cache_tree_entry_count;
> sub->cache_tree_entry_count = -1;
> } else {
> to_skip = sub->cache_tree_entry_count;
> }
> i += to_skip - 1;
>
..or this would be fine too. Which way to go?
A while we're still at the cache tree
> - if (ce->ce_flags & (CE_REMOVE | CE_INTENT_TO_ADD))
> - continue; /* entry being removed or placeholder */
> + /*
> + * CE_REMOVE entries are removed before the index is
> + * written to disk. Skip them to remain consistent
> + * with the future on-disk index.
> + */
> + if (ce->ce_flags & CE_REMOVE)
> + continue;
> +
A CE_REMOVE entry is removed later from the index, however it is still
counted in entry_count. entry_count serves two purposes: to skip the
number of processed entries in the in-memory index, and to record the
number of entries in the on-disk index. These numbers do not match
when CE_REMOVE is present. We have correct in-memory entry_count,
which means incorrect on-disk entry_count in this case.
I tested with an index that has a/b and a/c. The latter has CE_REMOVE.
After writing cache tree I get:
$ git ls-files --stage
100644 e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 0 a/b
$ ../test-dump-cache-tree
878e27c626266ac04087a203e4bdd396dcf74763 (2 entries, 1 subtrees)
878e27c626266ac04087a203e4bdd396dcf74763 #(ref) (1 entries, 1 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a a/ (2 entries, 0 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a #(ref) a/ (1 entries, 0 subtrees)
If I throw out that index, create a new one with a/b alone and write-tree, I get
$ ../test-dump-cache-tree
878e27c626266ac04087a203e4bdd396dcf74763 (1 entries, 1 subtrees)
4277b6e69d25e5efa77c455340557b384a4c018a a/ (1 entries, 0 subtrees)
Shall we fix this too? I'm thinking of adding "skip" argument to update_one and
i += sub->cache_tree->entry_count - 1;
would become
i += sub->cache_tree->entry_count + skip - 1;
and entry_count would always reflect on-disk value. This "skip" can be
reused for this i-t-a patch as well.
--
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html