Re: [PATCH v3 1/2] cache-tree: invalidate i-t-a paths after generating trees

From: Nguyen Thai Ngoc Duy <pclouds@gmail.com>
Date: Dec 15, 2012, 02:52 UTC
Message-ID: <CACsJy8BcLmGmxBUq3+4_Go4v1-HOFDOj9wi8A_-fbk-on4MX3A@mail.gmail.com>
In-Reply-To: <7v4njq1ze7.fsf@alter.siamese.dyndns.org>

On Thu, Dec 13, 2012 at 9:04 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>> @@ -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...

Show 21 quoted lines

>
>> +                     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

Show 10 quoted lines

> -               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