[PATCH v4 0/4] nd/invalidate-i-t-a-cache-tree

[PATCH v4 0/4] nd/invalidate-i-t-a-cache-tree

[Nguyễn Thái Ngọc Duy]

This version also fixes the CE_REMOVE bug I mentioned. As a side
effect, the bug fix makes the i-t-a fix cleaner.

Nguyễn Thái Ngọc Duy (4):
  cache-tree: remove dead i-t-a code in verify_cache()
  cache-tree: replace "for" loops in update_one with "while" loops
  cache-tree: fix writing cache-tree when CE_REMOVE is present
  cache-tree: invalidate i-t-a paths after generating trees

 cache-tree.c          | 61 +++++++++++++++++++++++++++++++++++++--------------
 cache-tree.h          |  1 +
 t/t2203-add-intent.sh | 20 +++++++++++++++++
 3 files changed, 65 insertions(+), 17 deletions(-)

-- 
1.8.0.rc2.23.g1fb49df

[PATCH v4 1/4] cache-tree: remove dead i-t-a code in verify_cache()

[Nguyễn Thái Ngọc Duy]

This code is added in 331fcb5 (git add --intent-to-add: do not let an
empty blob be committed by accident - 2008-11-28) to forbid committing
when i-t-a entries are present. When we allow that, we forgot to
remove this.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 28ed657..e2beab5 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -166,12 +166,8 @@ static int verify_cache(struct cache_entry **cache,
 				fprintf(stderr, "...\n");
 				break;
 			}
-			if (ce_stage(ce))
-				fprintf(stderr, "%s: unmerged (%s)\n",
-					ce->name, sha1_to_hex(ce->sha1));
-			else
-				fprintf(stderr, "%s: not added yet\n",
-					ce->name);
+			fprintf(stderr, "%s: unmerged (%s)\n",
+				ce->name, sha1_to_hex(ce->sha1));
 		}
 	}
 	if (funny)
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v4 2/4] cache-tree: replace "for" loops in update_one with "while" loops

[Nguyễn Thái Ngọc Duy]

The loops in update_one can be increased in two different ways: step
by one for files and by <n> for directories. "for" loop is not
suitable for this as it always steps by one and special handling is
required for directories. Replace them with "while" loops for clarity.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index e2beab5..44eed28 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -259,7 +259,8 @@ static int update_one(struct cache_tree *it,
 	/*
 	 * Find the subtrees and update them.
 	 */
-	for (i = 0; i < entries; i++) {
+	i = 0;
+	while (i < entries) {
 		struct cache_entry *ce = cache[i];
 		struct cache_tree_sub *sub;
 		const char *path, *slash;
@@ -271,8 +272,10 @@ static int update_one(struct cache_tree *it,
 			break; /* at the end of this level */
 
 		slash = strchr(path + baselen, '/');
-		if (!slash)
+		if (!slash) {
+			i++;
 			continue;
+		}
 		/*
 		 * a/bbb/c (base = a/, slash = /c)
 		 * ==>
@@ -289,7 +292,7 @@ static int update_one(struct cache_tree *it,
 				    flags);
 		if (subcnt < 0)
 			return subcnt;
-		i += subcnt - 1;
+		i += subcnt;
 		sub->used = 1;
 	}
 
@@ -300,7 +303,8 @@ static int update_one(struct cache_tree *it,
 	 */
 	strbuf_init(&buffer, 8192);
 
-	for (i = 0; i < entries; i++) {
+	i = 0;
+	while (i < entries) {
 		struct cache_entry *ce = cache[i];
 		struct cache_tree_sub *sub;
 		const char *path, *slash;
@@ -320,7 +324,7 @@ 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 += sub->cache_tree->entry_count;
 			sha1 = sub->cache_tree->sha1;
 			mode = S_IFDIR;
 		}
@@ -328,6 +332,7 @@ static int update_one(struct cache_tree *it,
 			sha1 = ce->sha1;
 			mode = ce->ce_mode;
 			entlen = pathlen - baselen;
+			i++;
 		}
 		if (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {
 			strbuf_release(&buffer);
-- 
1.8.0.rc2.23.g1fb49df

[PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present

[Nguyễn Thái Ngọc Duy]

entry_count is used in update_one() for two purposes:

1. to skip through the number of processed entries in in-memory index
2. to record the number of entries this cache-tree covers on disk

Unfortunately when CE_REMOVE is present these numbers are not the same
because CE_REMOVE entries are automatically removed before writing to
disk but entry_count is not adjusted and still counts CE_REMOVE
entries.

Separate the two use cases into two different variables. #1 is taken
care by the new field count in struct cache_tree_sub and entry_count
is prepared for #2.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c | 30 +++++++++++++++++++++++-------
 cache-tree.h |  1 +
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 44eed28..2c10b2e 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -238,6 +238,7 @@ static int update_one(struct cache_tree *it,
 		      int entries,
 		      const char *base,
 		      int baselen,
+		      int *skip_count,
 		      int flags)
 {
 	struct strbuf buffer;
@@ -245,6 +246,8 @@ static int update_one(struct cache_tree *it,
 	int dryrun = flags & WRITE_TREE_DRY_RUN;
 	int i;
 
+	*skip_count = 0;
+
 	if (0 <= it->entry_count && has_sha1_file(it->sha1))
 		return it->entry_count;
 
@@ -264,7 +267,7 @@ static int update_one(struct cache_tree *it,
 		struct cache_entry *ce = cache[i];
 		struct cache_tree_sub *sub;
 		const char *path, *slash;
-		int pathlen, sublen, subcnt;
+		int pathlen, sublen, subcnt, subskip;
 
 		path = ce->name;
 		pathlen = ce_namelen(ce);
@@ -289,10 +292,13 @@ static int update_one(struct cache_tree *it,
 				    cache + i, entries - i,
 				    path,
 				    baselen + sublen + 1,
+				    &subskip,
 				    flags);
 		if (subcnt < 0)
 			return subcnt;
 		i += subcnt;
+		sub->count = subcnt; /* to be used in the next loop */
+		*skip_count += subskip;
 		sub->used = 1;
 	}
 
@@ -324,7 +330,7 @@ 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;
+			i += sub->count;
 			sha1 = sub->cache_tree->sha1;
 			mode = S_IFDIR;
 		}
@@ -340,8 +346,18 @@ 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) {
+			*skip_count = *skip_count + 1;
+			continue;
+		}
+
+		if (ce->ce_flags & CE_INTENT_TO_ADD)
+			continue;
 
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
@@ -361,7 +377,7 @@ static int update_one(struct cache_tree *it,
 	}
 
 	strbuf_release(&buffer);
-	it->entry_count = i;
+	it->entry_count = i - *skip_count;
 #if DEBUG
 	fprintf(stderr, "cache-tree update-one (%d ent, %d subtree) %s\n",
 		it->entry_count, it->subtree_nr,
@@ -375,11 +391,11 @@ int cache_tree_update(struct cache_tree *it,
 		      int entries,
 		      int flags)
 {
-	int i;
+	int i, skip;
 	i = verify_cache(cache, entries, flags);
 	if (i)
 		return i;
-	i = update_one(it, cache, entries, "", 0, flags);
+	i = update_one(it, cache, entries, "", 0, &skip, flags);
 	if (i < 0)
 		return i;
 	return 0;
diff --git a/cache-tree.h b/cache-tree.h
index d8cb2e9..55d0f59 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -7,6 +7,7 @@
 struct cache_tree;
 struct cache_tree_sub {
 	struct cache_tree *cache_tree;
+	int count;		/* internally used by update_one() */
 	int namelen;
 	int used;
 	char name[FLEX_ARRAY];
-- 
1.8.0.rc2.23.g1fb49df

Re: [PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> entry_count is used in update_one() for two purposes:
>
> 1. to skip through the number of processed entries in in-memory index
> 2. to record the number of entries this cache-tree covers on disk
>
> Unfortunately when CE_REMOVE is present these numbers are not the same
> because CE_REMOVE entries are automatically removed before writing to
> disk but entry_count is not adjusted and still counts CE_REMOVE
> entries.

Nicely explained.  I wonder if we can also add a piece of test to
the patch 4/4 to demonstrate the issue with CE_REMOVE entries,
though.

Thanks.

Re: [PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present

[Nguyen Thai Ngoc Duy]

On Sun, Dec 16, 2012 at 2:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> entry_count is used in update_one() for two purposes:
>>
>> 1. to skip through the number of processed entries in in-memory index
>> 2. to record the number of entries this cache-tree covers on disk
>>
>> Unfortunately when CE_REMOVE is present these numbers are not the same
>> because CE_REMOVE entries are automatically removed before writing to
>> disk but entry_count is not adjusted and still counts CE_REMOVE
>> entries.
>
> Nicely explained.  I wonder if we can also add a piece of test to
> the patch 4/4 to demonstrate the issue with CE_REMOVE entries,
> though.

A hand crafted one, maybe. I did not attempt to recreate it with git
commands (and I don't think we update cache-tree after unpack_trees).
So I wrote something like this instead:

int main(int ac, char **av)
{
	unsigned char sha1[20];
	setup_git_directory();
	read_cache();
	active_cache[1]->ce_flags |= CE_REMOVE;
	write_cache_as_tree(sha1, 0, NULL);
	return 0;
}

I can polish it a bit and write new tests based on it and
test-dump-cache-tree if you want.
-- 
Duy

Re: [PATCH v4 3/4] cache-tree: fix writing cache-tree when CE_REMOVE is present

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:

>On Sun, Dec 16, 2012 at 2:20 PM, Junio C Hamano <gitster@pobox.com>
>wrote:
>> Nicely explained.  I wonder if we can also add a piece of test to
>> the patch 4/4 to demonstrate the issue with CE_REMOVE entries,
>> though.
>
>A hand crafted one, maybe. I did not attempt to recreate it with git
>commands (and I don't think we update cache-tree after unpack_trees).

Yeah, that's what I thought. No need to bother creating a bug that won't appear in the real life :-) 

Tusks for sanity checking.
-- 
Pardon terseness, typo and HTML from a tablet.

[PATCH v4 4/4] cache-tree: invalidate i-t-a paths after generating trees

[Nguyễn Thái Ngọc Duy]

Intent-to-add entries used to forbid writing trees so it was not a
problem. After commit 3f6d56d (commit: ignore intent-to-add entries
instead of refusing - 2012-02-07), we can generate trees from an index
with i-t-a entries.

However, the commit forgets to invalidate all paths leading to i-t-a
entries. With fully valid cache-tree (e.g. after commit or
write-tree), diff operations may prefer cache-tree to index and not
see i-t-a entries in the index, because cache-tree does not have them.

Reported-by: Jonathon Mah <me@JonathonMah.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 cache-tree.c          | 14 ++++++++++++--
 t/t2203-add-intent.sh | 20 ++++++++++++++++++++
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 2c10b2e..37e4d00 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -244,6 +244,7 @@ static int update_one(struct cache_tree *it,
 	struct strbuf buffer;
 	int missing_ok = flags & WRITE_TREE_MISSING_OK;
 	int dryrun = flags & WRITE_TREE_DRY_RUN;
+	int to_invalidate = 0;
 	int i;
 
 	*skip_count = 0;
@@ -333,6 +334,8 @@ static int update_one(struct cache_tree *it,
 			i += sub->count;
 			sha1 = sub->cache_tree->sha1;
 			mode = S_IFDIR;
+			if (sub->cache_tree->entry_count < 0)
+				to_invalidate = 1;
 		}
 		else {
 			sha1 = ce->sha1;
@@ -356,8 +359,15 @@ static int update_one(struct cache_tree *it,
 			continue;
 		}
 
-		if (ce->ce_flags & CE_INTENT_TO_ADD)
+		/*
+		 * 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;
+		}
 
 		strbuf_grow(&buffer, entlen + 100);
 		strbuf_addf(&buffer, "%o %.*s%c", mode, entlen, path + baselen, '\0');
@@ -377,7 +387,7 @@ static int update_one(struct cache_tree *it,
 	}
 
 	strbuf_release(&buffer);
-	it->entry_count = i - *skip_count;
+	it->entry_count = to_invalidate ? -1 : i - *skip_count;
 #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
 
-- 
1.8.0.rc2.23.g1fb49df