Re: [PATCH v2 2/2] cache-tree: do not generate empty trees as a result of all i-t-a subentries

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

The test would not let you tell between the two possible ways the
last step "git commit" fails.

Did it fail due to the protection this change adds (i.e. you should
be checking if "git write-tree" fails if that is the case we want to
cover), or did it fail because you recorded an empty tree as the
root commit without giving the --allow-empty option?

In any case, I am not sure about the logic in the comment, either.
"git commit --allow-empty" should be able to create a new commit
without any files in it, no?

On Wed, Jul 06, 2016 at 12:26:19PM -0700, Junio C Hamano wrote:

Nguyễn Thái Ngọc Duy  [off-list ref] writes:

quoted

@@ -426,6 +433,15 @@ int cache_tree_update(struct index_state *istate, int flags)
 	i = update_one(it, cache, entries, "", 0, &skip, flags);
 	if (i < 0)
 		return i;
+	/*
+	 * Top dir can become empty if all entries are i-t-a (even
+	 * from subdirs). Note that we do allow to create an empty
+	 * tree from an empty index. Only error when an empty tree is
+	 * a result of the i-t-a thing.
+	 */
+	if (it->entry_count < 0 &&
+	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
+		return error(_("cannot build a tree from just intent-to-add entries"));

The test would not let you tell between the two possible ways the
last step "git commit" fails.

Did it fail due to the protection this change adds (i.e. you should
be checking if "git write-tree" fails if that is the case we want to
cover), or did it fail because you recorded an empty tree as the
root commit without giving the --allow-empty option?

In any case, I am not sure about the logic in the comment, either.
"git commit --allow-empty" should be able to create a new commit
without any files in it, no?

You're right. If an empty index can produce an empty tree, then an
index full of i-t-a entries should also be able to produce an empty
tree.

git-commit not failing when --allow-empty is not given is another
(known) problem, where it miscounts the number of real index entries.
It's not right to "fix" it in here.

I'll deal with that separately. Let's focus on cache-tree only this
time. So how about this on top?

-- 8< --

diff --git a/cache-tree.c b/cache-tree.c
index 75e73d7..2d50640 100644
--- a/cache-tree.c
+++ b/cache-tree.c

@@ -433,15 +433,6 @@ int cache_tree_update(struct index_state *istate, int flags)
 	i = update_one(it, cache, entries, "", 0, &skip, flags);
 	if (i < 0)
 		return i;
-	/*
-	 * Top dir can become empty if all entries are i-t-a (even
-	 * from subdirs). Note that we do allow to create an empty
-	 * tree from an empty index. Only error when an empty tree is
-	 * a result of the i-t-a thing.
-	 */
-	if (it->entry_count < 0 &&
-	    !hashcmp(it->sha1, EMPTY_TREE_SHA1_BIN))
-		return error(_("cannot build a tree from just intent-to-add entries"));
 	istate->cache_changed |= CACHE_TREE_CHANGED;
 	return 0;
 }

diff --git a/t/t2203-add-intent.sh b/t/t2203-add-intent.sh
index a19f06b..1a01279 100755
--- a/t/t2203-add-intent.sh
+++ b/t/t2203-add-intent.sh

@@ -104,10 +104,17 @@ test_expect_success 'cache-tree does skip dir that becomes empty' '
 	git init ita-in-dir &&
 	(
 		cd ita-in-dir &&
+		echo ground >ground &&
+		git add ground &&
 		mkdir -p 1/2/3 &&
 		echo 4 >1/2/3/4 &&
 		git add -N 1/2/3/4 &&
-		test_must_fail git commit -m committed
+		git commit -m committed &&
+		git ls-tree -r HEAD >actual &&
+		cat >expected <<\EOF &&
+100644 blob b649b43b0708f5604ac912f5a15f7da2bad51a1b	ground
+EOF
+		test_cmp expected actual
 	)
 '
 
-- 8< --

quoted

+test_expect_success 'cache-tree does skip dir that becomes empty' '
+	rm -fr ita-in-dir &&
+	git init ita-in-dir &&
+	(
+		cd ita-in-dir &&
+		mkdir -p 1/2/3 &&
+		echo 4 >1/2/3/4 &&
+		git add -N 1/2/3/4 &&
+		test_must_fail git commit -m committed
+	)
+'
+
 test_done