On Thu, Dec 13, 2012 at 9:04 AM, Junio C Hamano <gits...@pobox.com> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to