Reactions to v2.37.0-rc0 test coverage report

These patches add test coverage or simplify code based on discoveries in the test coverage report (specifically, some that I highlighted at [1]).

[1]
https://lore.kernel.org/git/[email protected]/

1. Add tests for 'git update-index --verbose'.
2. Add 'git gc --cruft' without '--prune=now' to test.
3. Drop an always-NULL parameter from an internal method.
4. Revert 080ab56a4 (cache-tree: implement cache_tree_find_path(),

    2022-05-23).

Any subset of these could be taken (or dropped), but I thought they would be worth considering.

Thanks, -Stolee

Derrick Stolee (4):
t2107: test 'git update-index --verbose'
t5329: test 'git gc --cruft' without '--prune=now'
pack-write: drop always-NULL parameter
cache-tree: remove cache_tree_find_path()

 cache-tree.c                             | 27 ---------------------
 cache-tree.h                             |  2 --
 pack-write.c                             | 17 +++++--------
 t/t2106-update-index-assume-unchanged.sh |  2 +-
 t/t2107-update-index-basic.sh            | 31 +++++++++++++++++++-----
 t/t5329-pack-objects-cruft.sh            |  4 ++-
 6 files changed, 35 insertions(+), 48 deletions(-)

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1261%2Fderrickstolee%2Ftest-coverage-response-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1261/derrickstolee/test-coverage-response-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1261

gitgitgadget

From: Derrick Stolee <[email protected]>

The '--verbose' option reports what is being added and removed from the index, but has not been tested up to this point. Augment the tests in t2107 to check the '--verbose' option in some scenarios.

---
 t/t2106-update-index-assume-unchanged.sh |  2 +-
 t/t2107-update-index-basic.sh            | 31 +++++++++++++++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

index d943ddf47e0..ad2692a2979 100755
@@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' '
 	git reset --hard &&
 	git checkout other &&
 	echo dirt >file &&
-	git update-index --assume-unchanged file &&
+	git update-index --verbose --assume-unchanged file &&
 	test_must_fail git checkout - 2>err &&
 	test_i18ngrep overwritten err
 '

index a30b7ca6bc9..07e6de84e6d 100755
@@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' '
 	echo content >file &&
 	git add file &&
 	git rev-parse :file >expect &&
-	test_must_fail git update-index --cacheinfo 100644 $ZERO_OID file &&
+	test_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out &&
 	git rev-parse :file >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	add '\''file'\''
+	EOF
+	test_cmp expect out
 '
 
 test_expect_success '--cacheinfo does not accept gitlink null sha1' '

@@ -59,9 +64,14 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '
 	git rev-parse :file >actual &&
 	test_cmp expect actual &&
 
-	git update-index --add --cacheinfo "100644,$(cat expect),elif" &&
+	git update-index --add --verbose --cacheinfo "100644,$(cat expect),elif" >out &&
 	git rev-parse :elif >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	add '\''elif'\''
+	EOF
+	test_cmp expect out
 '
 
 test_expect_success '.lock files cleaned up' '

@@ -74,7 +84,8 @@ test_expect_success '.lock files cleaned up' '
 	git config core.worktree ../../worktree &&
 	# --refresh triggers late setup_work_tree,
 	# active_cache_changed is zero, rollback_lock_file fails
-	git update-index --refresh &&
+	git update-index --refresh --verbose >out &&
+	test_must_be_empty out &&
 	! test -f .git/index.lock
 	)
 '

@@ -83,7 +94,15 @@ test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
 	>A &&
 	>B &&
 	git add A B &&
-	git update-index --chmod=+x A --chmod=-x B &&
+	git update-index --verbose --chmod=+x A --chmod=-x B >out &&
+	cat >expect <<-\EOF &&
+	add '\''A'\''
+	chmod +x '\''A'\''
+	add '\''B'\''
+	chmod -x '\''B'\''
+	EOF
+	test_cmp expect out &&
+
 	cat >expect <<-EOF &&
 	100755 $EMPTY_BLOB 0	A
 	100644 $EMPTY_BLOB 0	B

gitgitgadget

From: Derrick Stolee <[email protected]>

Replace a 'git repack --cruft -d' with the wrapper 'git gc --cruft' to exercise some logic in builtin/gc.c that adds the '--cruft' option to the underlying 'git repack' command.

---
 t/t5329-pack-objects-cruft.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index b481224b93d..55bb9c0e7b5 100755
@@ -451,11 +451,13 @@ test_expect_success 'expiring cruft objects with git gc' '
 		sort <reachable.raw >reachable &&
 		comm -13 reachable objects >unreachable &&
 
-		git repack --cruft -d &&
+		# Write a cruft pack instead of deleting files.
+		git gc --cruft &&
 
 		mtimes=$(ls .git/objects/pack/pack-*.mtimes) &&
 		test_path_is_file $mtimes &&
 
+		# Ignore the cruft pack and delete every unreachable object.
 		git gc --cruft --prune=now &&
 
 		git cat-file --batch-all-objects --batch-check="%(objectname)" >objects &&

gitgitgadget

From: Derrick Stolee <[email protected]>

write_mtimes_file() takes an mtimes parameter as its first option, but the only caller passes a NULL constant. Drop this parameter to simplify logic. This can be reverted if that parameter is needed in the future.

---
 pack-write.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

index 23c0342018f..00787e306db 100644
@@ -310,26 +310,21 @@ static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)
 	hashwrite(f, hash, the_hash_algo->rawsz);
 }
 
-static const char *write_mtimes_file(const char *mtimes_name,
-				     struct packing_data *to_pack,
+static const char *write_mtimes_file(struct packing_data *to_pack,
 				     struct pack_idx_entry **objects,
 				     uint32_t nr_objects,
 				     const unsigned char *hash)
 {
+	struct strbuf tmp_file = STRBUF_INIT;
+	const char *mtimes_name;
 	struct hashfile *f;
 	int fd;
 
 	if (!to_pack)
 		BUG("cannot call write_mtimes_file with NULL packing_data");
 
-	if (!mtimes_name) {
-		struct strbuf tmp_file = STRBUF_INIT;
-		fd = odb_mkstemp(&tmp_file, "pack/tmp_mtimes_XXXXXX");
-		mtimes_name = strbuf_detach(&tmp_file, NULL);
-	} else {
-		unlink(mtimes_name);
-		fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
-	}
+	fd = odb_mkstemp(&tmp_file, "pack/tmp_mtimes_XXXXXX");
+	mtimes_name = strbuf_detach(&tmp_file, NULL);
 	f = hashfd(fd, mtimes_name);
 
 	write_mtimes_header(f);

@@ -561,7 +556,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 				      pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
-		mtimes_tmp_name = write_mtimes_file(NULL, to_pack, written_list,
+		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
 						    nr_written,
 						    hash);
 	}

gitgitgadget

From: Derrick Stolee <[email protected]>

This reverts 080ab56a46 (cache-tree: implement cache_tree_find_path(), 2022-05-23). The cache_tree_find_path() method was never actually called in the topic that added it. I cannot find any reference to it in any of my forks, so this appears to not be needed at the moment.

---
 cache-tree.c | 27 ---------------------------
 cache-tree.h |  2 --
 2 files changed, 29 deletions(-)

index ff794d940fa..56db0b5026b 100644
@@ -101,33 +101,6 @@ struct cache_tree_sub *cache_tree_sub(struct cache_tree *it, const char *path)
 	return find_subtree(it, path, pathlen, 1);
 }
 
-struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path)
-{
-	const char *slash;
-	int namelen;
-	struct cache_tree_sub it_sub = {
-		.cache_tree = it,
-	};
-	struct cache_tree_sub *down = &it_sub;
-
-	while (down) {
-		slash = strchrnul(path, '/');
-		namelen = slash - path;
-		down->cache_tree->entry_count = -1;
-		if (!*slash) {
-			int pos;
-			pos = cache_tree_subtree_pos(down->cache_tree, path, namelen);
-			if (0 <= pos)
-				return down->cache_tree->down[pos]->cache_tree;
-			return NULL;
-		}
-		down = find_subtree(it, path, namelen, 0);
-		path = slash + 1;
-	}
-
-	return NULL;
-}
-
 static int do_invalidate_path(struct cache_tree *it, const char *path)
 {
 	/* a/b/c

index f75f8e74dcd..8efeccebfc9 100644
@@ -29,8 +29,6 @@ struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);
 
 int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);
 
-struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path);
-
 void cache_tree_write(struct strbuf *, struct cache_tree *root);
 struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
 

gitgitgadget

I like your suggestions. I'll update them if there are other reasons to do a v2.

These lines, yes.

This one requires "git gc --cruft --prune=<time>" where <time> is not "now". I didn't want to jump too much into the exact expire time as I feared that could cause some issues, but I suppose passing --prune="01-01-1980" would provide a non-zero expiration and then lead to us testing "git repack --cruft --cruft-expiration" as well. (It doesn't actually test the exact expiration time, but does test that the options are sent down the pipe.)

Thanks,
-Stolee

Thanks for pointing this out. I shouldn't have left this change in.

I understand that the test wants to test specific behavior, and that behavior is focused on certain inputs to 'git update-index', but I also think that the --verbose option presents _additional information_ about what is expected from these behaviors. It doesn't change the already-tested behavior, only enhances it.

If I separate things out and only had test for --verbose, I would need to replicate many of these behaviors just for that option, which would be wasteful.

In this particular case, I'm demonstrating that the --verbose mode still reports the file as added (because of the earlier 'git add file') even though the command as a whole failed due to an invalid OID.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

The '--verbose' option reports what is being added and removed from the index, but has not been tested up to this point. Augment the tests in t2107 to check the '--verbose' option in some scenarios.

---
 t/t2107-update-index-basic.sh | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

index a30b7ca6bc9..07e6de84e6d 100755
@@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' '
 	echo content >file &&
 	git add file &&
 	git rev-parse :file >expect &&
-	test_must_fail git update-index --cacheinfo 100644 $ZERO_OID file &&
+	test_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out &&
 	git rev-parse :file >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	add '\''file'\''
+	EOF
+	test_cmp expect out
 '
 
 test_expect_success '--cacheinfo does not accept gitlink null sha1' '

@@ -59,9 +64,14 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '
 	git rev-parse :file >actual &&
 	test_cmp expect actual &&
 
-	git update-index --add --cacheinfo "100644,$(cat expect),elif" &&
+	git update-index --add --verbose --cacheinfo "100644,$(cat expect),elif" >out &&
 	git rev-parse :elif >actual &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+
+	cat >expect <<-\EOF &&
+	add '\''elif'\''
+	EOF
+	test_cmp expect out
 '
 
 test_expect_success '.lock files cleaned up' '

@@ -74,7 +84,8 @@ test_expect_success '.lock files cleaned up' '
 	git config core.worktree ../../worktree &&
 	# --refresh triggers late setup_work_tree,
 	# active_cache_changed is zero, rollback_lock_file fails
-	git update-index --refresh &&
+	git update-index --refresh --verbose >out &&
+	test_must_be_empty out &&
 	! test -f .git/index.lock
 	)
 '

@@ -83,7 +94,15 @@ test_expect_success '--chmod=+x and chmod=-x in the same argument list' '
 	>A &&
 	>B &&
 	git add A B &&
-	git update-index --chmod=+x A --chmod=-x B &&
+	git update-index --verbose --chmod=+x A --chmod=-x B >out &&
+	cat >expect <<-\EOF &&
+	add '\''A'\''
+	chmod +x '\''A'\''
+	add '\''B'\''
+	chmod -x '\''B'\''
+	EOF
+	test_cmp expect out &&
+
 	cat >expect <<-EOF &&
 	100755 $EMPTY_BLOB 0	A
 	100644 $EMPTY_BLOB 0	B

gitgitgadget

These patches add test coverage or simplify code based on discoveries in the test coverage report (specifically, some that I highlighted at [1]).

[1]
https://lore.kernel.org/git/[email protected]/

1. Add tests for 'git update-index --verbose'.
2. Add 'git gc --cruft' without '--prune=now' to test.
3. Drop an always-NULL parameter from an internal method.
4. Revert 080ab56a4 (cache-tree: implement cache_tree_find_path(),

    2022-05-23).

Any subset of these could be taken (or dropped), but I thought they would be worth considering.

Updates in v2
=============

* A useless addition of --verbose was removed from a test in patch 1.
* Comments are updated in patch 2 based on Taylor's recommendations.
* Added --prune="01-01-1980" to exercise the --cruft-expiration logic underneath 'git gc --cruft'.

Thanks, -Stolee

Derrick Stolee (4):
t2107: test 'git update-index --verbose'
t5329: test 'git gc --cruft' without '--prune=now'
pack-write: drop always-NULL parameter
cache-tree: remove cache_tree_find_path()

 cache-tree.c                  | 27 ---------------------------
 cache-tree.h                  |  2 --
 pack-write.c                  | 17 ++++++-----------
 t/t2107-update-index-basic.sh | 31 +++++++++++++++++++++++++------
 t/t5329-pack-objects-cruft.sh |  4 +++-
 5 files changed, 34 insertions(+), 47 deletions(-)

base-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1261%2Fderrickstolee%2Ftest-coverage-response-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1261/derrickstolee/test-coverage-response-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1261

Range-diff vs v1:

1: c6803df1b6a ! 1: c22963b49e6 t2107: test 'git update-index --verbose'
@@ Commit message

- ## t/t2106-update-index-assume-unchanged.sh ##
-@@ t/t2106-update-index-assume-unchanged.sh: test_expect_success 'do not switch branches with dirty file' '
- git reset --hard &&
- git checkout other &&
- echo dirt >file &&
-- git update-index --assume-unchanged file &&
-+ git update-index --verbose --assume-unchanged file &&
- test_must_fail git checkout - 2>err &&
- test_i18ngrep overwritten err
- '
-
## t/t2107-update-index-basic.sh ##
@@ t/t2107-update-index-basic.sh: test_expect_success '--cacheinfo does not accept blob null sha1' '
echo content >file &&
2: f59a0c326a0 ! 2: ca5dbc29e40 t5329: test 'git gc --cruft' without '--prune=now'

     @@ t/t5329-pack-objects-cruft.sh: test_expect_success 'expiring cruft objects with
       		comm -13 reachable objects >unreachable &&
       
      -		git repack --cruft -d &&
     -+		# Write a cruft pack instead of deleting files.
     -+		git gc --cruft &&
     ++		# Write a cruft pack containing all unreachable objects.
     ++		git gc --cruft --prune="01-01-1980" &&
       
       		mtimes=$(ls .git/objects/pack/pack-*.mtimes) &&
       		test_path_is_file $mtimes &&
       
     -+		# Ignore the cruft pack and delete every unreachable object.
     ++		# Prune all unreachable objects from the cruft pack.
       		git gc --cruft --prune=now &&
       
       		git cat-file --batch-all-objects --batch-check="%(objectname)" >objects &&
 3:  b67e110bf60 = 3:  b956a371c70 pack-write: drop always-NULL parameter
 4:  81d8e2b5fdd = 4:  8486a1d6eca cache-tree: remove cache_tree_find_path()

gitgitgadget

From: Derrick Stolee <[email protected]>

Replace a 'git repack --cruft -d' with the wrapper 'git gc --cruft' to exercise some logic in builtin/gc.c that adds the '--cruft' option to the underlying 'git repack' command.

---
 t/t5329-pack-objects-cruft.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index b481224b93d..8968f7a08d8 100755
@@ -451,11 +451,13 @@ test_expect_success 'expiring cruft objects with git gc' '
 		sort <reachable.raw >reachable &&
 		comm -13 reachable objects >unreachable &&
 
-		git repack --cruft -d &&
+		# Write a cruft pack containing all unreachable objects.
+		git gc --cruft --prune="01-01-1980" &&
 
 		mtimes=$(ls .git/objects/pack/pack-*.mtimes) &&
 		test_path_is_file $mtimes &&
 
+		# Prune all unreachable objects from the cruft pack.
 		git gc --cruft --prune=now &&
 
 		git cat-file --batch-all-objects --batch-check="%(objectname)" >objects &&

gitgitgadget

From: Derrick Stolee <[email protected]>

write_mtimes_file() takes an mtimes parameter as its first option, but the only caller passes a NULL constant. Drop this parameter to simplify logic. This can be reverted if that parameter is needed in the future.

---
 pack-write.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

index 23c0342018f..00787e306db 100644
@@ -310,26 +310,21 @@ static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)
 	hashwrite(f, hash, the_hash_algo->rawsz);
 }
 
-static const char *write_mtimes_file(const char *mtimes_name,
-				     struct packing_data *to_pack,
+static const char *write_mtimes_file(struct packing_data *to_pack,
 				     struct pack_idx_entry **objects,
 				     uint32_t nr_objects,
 				     const unsigned char *hash)
 {
+	struct strbuf tmp_file = STRBUF_INIT;
+	const char *mtimes_name;
 	struct hashfile *f;
 	int fd;
 
 	if (!to_pack)
 		BUG("cannot call write_mtimes_file with NULL packing_data");
 
-	if (!mtimes_name) {
-		struct strbuf tmp_file = STRBUF_INIT;
-		fd = odb_mkstemp(&tmp_file, "pack/tmp_mtimes_XXXXXX");
-		mtimes_name = strbuf_detach(&tmp_file, NULL);
-	} else {
-		unlink(mtimes_name);
-		fd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
-	}
+	fd = odb_mkstemp(&tmp_file, "pack/tmp_mtimes_XXXXXX");
+	mtimes_name = strbuf_detach(&tmp_file, NULL);
 	f = hashfd(fd, mtimes_name);
 
 	write_mtimes_header(f);

@@ -561,7 +556,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,
 				      pack_idx_opts->flags);
 
 	if (pack_idx_opts->flags & WRITE_MTIMES) {
-		mtimes_tmp_name = write_mtimes_file(NULL, to_pack, written_list,
+		mtimes_tmp_name = write_mtimes_file(to_pack, written_list,
 						    nr_written,
 						    hash);
 	}

gitgitgadget

From: Derrick Stolee <[email protected]>

This reverts 080ab56a46 (cache-tree: implement cache_tree_find_path(), 2022-05-23). The cache_tree_find_path() method was never actually called in the topic that added it. I cannot find any reference to it in any of my forks, so this appears to not be needed at the moment.

---
 cache-tree.c | 27 ---------------------------
 cache-tree.h |  2 --
 2 files changed, 29 deletions(-)

index ff794d940fa..56db0b5026b 100644
@@ -101,33 +101,6 @@ struct cache_tree_sub *cache_tree_sub(struct cache_tree *it, const char *path)
 	return find_subtree(it, path, pathlen, 1);
 }
 
-struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path)
-{
-	const char *slash;
-	int namelen;
-	struct cache_tree_sub it_sub = {
-		.cache_tree = it,
-	};
-	struct cache_tree_sub *down = &it_sub;
-
-	while (down) {
-		slash = strchrnul(path, '/');
-		namelen = slash - path;
-		down->cache_tree->entry_count = -1;
-		if (!*slash) {
-			int pos;
-			pos = cache_tree_subtree_pos(down->cache_tree, path, namelen);
-			if (0 <= pos)
-				return down->cache_tree->down[pos]->cache_tree;
-			return NULL;
-		}
-		down = find_subtree(it, path, namelen, 0);
-		path = slash + 1;
-	}
-
-	return NULL;
-}
-
 static int do_invalidate_path(struct cache_tree *it, const char *path)
 {
 	/* a/b/c

index f75f8e74dcd..8efeccebfc9 100644
@@ -29,8 +29,6 @@ struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);
 
 int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);
 
-struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path);
-
 void cache_tree_write(struct strbuf *, struct cache_tree *root);
 struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);
 

gitgitgadget

Thanks. Queued.