Re: [PATCH v2] cache-tree: invalidate i-t-a paths after generating trees
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 10, 2012, 06:50 UTC
- Message-ID
- <7v7goqcsdy.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1354939803-8466-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
Show 24 quoted lines
> diff --git a/cache-tree.c b/cache-tree.c
> index 28ed657..989a7ff 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -248,6 +248,7 @@ static int update_one(struct cache_tree *it,
> int missing_ok = flags & WRITE_TREE_MISSING_OK;
> int dryrun = flags & WRITE_TREE_DRY_RUN;
> int i;
> + int to_invalidate = 0;
>
> if (0 <= it->entry_count && has_sha1_file(it->sha1))
> return it->entry_count;
> @@ -324,7 +325,13 @@ 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" */
> + if (sub->cache_tree->entry_count < 0) {
> + i -= sub->cache_tree->entry_count;
> + to_invalidate = 1;
> + }
> + else
> + i += sub->cache_tree->entry_count;Hrm. update_one() is prepared to see a cache-tree whose entry count is zero (see the context lines in the previous hunk) and the invariant for the rest of the code is "if 0 <= entry_count, the cached tree is valid; invalid cache-tree has -1 in entry_count. More importantly, entry_count negated does not in general express how many entries there are in the subtree and does not tell us how many index entries to skip.
Show 23 quoted lines
> @@ -339,8 +346,23 @@ static int update_one(struct cache_tree *it,
> mode, sha1_to_hex(sha1), entlen+baselen, path);
> }
>
> - 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;
> +
> + /*
> + * CE_INTENT_TO_ADD entries exist on on-disk index but
> + * they are not part of generated trees. Invalidate up
> + * to root to force cache-tree users to read elsewhere.
> + */
> + if (ce->ce_flags & CE_INTENT_TO_ADD) {
> + to_invalidate = 1;
> + continue;
> + }Thanks for documenting these.
Show 6 quoted lines
> @@ -360,7 +382,7 @@ static int update_one(struct cache_tree *it, > } > > strbuf_release(&buffer); > - it->entry_count = i; > + it->entry_count = to_invalidate ? -i : i;
See above. I am not fundamentally opposed to a change to redefine entry_count so that it always maintains how many index entries the subtree covers, even for invalidated subtree, but I do not think this patch alone is sufficient to maintain such invariant.
Show 32 quoted lines
> #if DEBUG > fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n", > it->entry_count, it->subtree_nr, > diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh > index ec35409..2a4a749 100755 > --- a/t/t2203-add-intent.sh > +++ b/t/t2203-add-intent.sh > @@ -62,5 +62,25 @@ test_expect_success 'can "commit -a" with an i-t-a entry' ' > git commit -a -m all > ' > > +test_expect_success 'cache-tree invalidates i-t-a paths' ' > + git reset --hard && > + mkdir dir && > + : >dir/foo && > + git add dir/foo && > + git commit -m foo && > + > + : >dir/bar && > + git add -N dir/bar && > + git diff --cached --name-only >actual && > + echo dir/bar >expect && > + test_cmp expect actual && > + > + git write-tree >/dev/null && > + > + git diff --cached --name-only >actual && > + echo dir/bar >expect && > + test_cmp expect actual > +' > + > test_done