threads / patch / 58004

patch, 4 partsReactions to v2.37.0-rc0 test coverage report

Subject: [PATCH 0/4] Reactions to v2.37.0-rc0 test coverage report

## tl;dr

15 messages between Jun 14, 2022 and Jun 16, 2022. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Derrick Stolee via GitGitGadget· Jun 14, 2022, 21:27 UTC · lore

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/3d1c6dfd-1df6-3393-df5e-692719375772@github.com/

 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

Derrick Stolee via GitGitGadget· Jun 14, 2022, 21:27 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 1/4] t2107: test 'git update-index --verbose'

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t2106-update-index-assume-unchanged.sh |  2 +-
 t/t2107-update-index-basic.sh            | 31 +++++++++++++++++++-----
 2 files changed, 26 insertions(+), 7 deletions(-)

Show changes to 2 files +26 −7

t/t2106-update-index-assume-unchanged.sh, t/t2107-update-index-basic.sh

diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh
index d943ddf47e0..ad2692a2979 100755
--- a/t/t2106-update-index-assume-unchanged.sh
+++ b/t/t2106-update-index-assume-unchanged.sh
@@ -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
 '
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index a30b7ca6bc9..07e6de84e6d 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -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

Eric Sunshine· Jun 15, 2022, 23:18 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 1/4] t2107: test 'git update-index --verbose'

On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 14 quoted lines

> 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.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh
> @@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' '
>         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
>  '

If this test passes with or without the addition of `--verbose`, then adding `--verbose` unnecessarily only pollutes what is (presumably) the minimum code necessary to implement what the test is checking, and may confuse future readers into thinking that something subtle is going on.

Show 16 quoted lines

> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
> @@ -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
>  '

While I understand your desire to address a gap in the test coverage, I worry that this sort of change, which is orthogonal to the test's stated purpose, has the same downsides as mentioned above (i.e. polluting the minimum necessary code, and potentially confusing readers). Rather than piggybacking on existing tests, adding one or two new standalone tests dedicated to checking `--verbose` would be more palatable, more understandable, and be less likely to confuse future readers. The same comment applies to the remaining changes in this patch.

Derrick Stolee· Jun 16, 2022, 12:54 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/4] t2107: test 'git update-index --verbose'

On 6/15/2022 7:18 PM, Eric Sunshine wrote:

Show 22 quoted lines

> On Tue, Jun 14, 2022 at 5:36 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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.
>>
>> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
>> ---
>> diff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh
>> @@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' '
>>         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
>>  '
> 
> If this test passes with or without the addition of `--verbose`, then
> adding `--verbose` unnecessarily only pollutes what is (presumably)
> the minimum code necessary to implement what the test is checking, and
> may confuse future readers into thinking that something subtle is
> going on.

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

Show 26 quoted lines

>> diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
>> @@ -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
>>  '
> 
> While I understand your desire to address a gap in the test coverage,
> I worry that this sort of change, which is orthogonal to the test's
> stated purpose, has the same downsides as mentioned above (i.e.
> polluting the minimum necessary code, and potentially confusing
> readers). Rather than piggybacking on existing tests, adding one or
> two new standalone tests dedicated to checking `--verbose` would be
> more palatable, more understandable, and be less likely to confuse
> future readers. The same comment applies to the remaining changes in
> this patch.

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

Derrick Stolee via GitGitGadget· Jun 14, 2022, 21:27 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t5329-pack-objects-cruft.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to t/t5329-pack-objects-cruft.sh +3 −1

diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index b481224b93d..55bb9c0e7b5 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -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

Taylor Blau· Jun 14, 2022, 23:38 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'

On Tue, Jun 14, 2022 at 09:27:48PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 22 quoted lines

> From: Derrick Stolee <derrickstolee@github.com>
>
> 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.
>
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>  t/t5329-pack-objects-cruft.sh | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
> index b481224b93d..55bb9c0e7b5 100755
> --- a/t/t5329-pack-objects-cruft.sh
> +++ b/t/t5329-pack-objects-cruft.sh
> @@ -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 &&

These ("git repack --cruft -d" and "git gc --cruft") do the same thing, so this transformation makes sense.

It may be slightly clearer to refer to "objects" instead of "files", perhaps like:

    # Write a cruft pack containing all unreachable objects

and then replace:

> +		# Ignore the cruft pack and delete every unreachable object.
>  		git gc --cruft --prune=now &&

with:

    # Prune all unreachable objects from the cruft pack

But I don't think the current wording is a problem, either, so feel free to take or leave these suggestions.

Quoting from your original coverage report, this should take care of:

Taylor Blau 5b92477f builtin/gc.c: conditionally avoid pruning objects via loose builtin/gc.c 5b92477f 337) strvec_push(&repack, "--cruft"); 5b92477f 338) if (prune_expire) 5b92477f 339) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire);

so this patch looks good to me.

Thanks, Taylor

Derrick Stolee· Jun 15, 2022, 13:28 UTC · re: Taylor Blau · lore

Re: [PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'

On 6/14/22 7:38 PM, Taylor Blau wrote:

> On Tue, Jun 14, 2022 at 09:27:48PM +0000, Derrick Stolee via GitGitGadget wrote:

Show 23 quoted lines

>> -		git repack --cruft -d &&
>> +		# Write a cruft pack instead of deleting files.
>> +		git gc --cruft &&
> 
> These ("git repack --cruft -d" and "git gc --cruft") do the same thing,
> so this transformation makes sense.
> 
> It may be slightly clearer to refer to "objects" instead of "files",
> perhaps like:
> 
>     # Write a cruft pack containing all unreachable objects
> 
> and then replace:
> 
>> +		# Ignore the cruft pack and delete every unreachable object.
>>  		git gc --cruft --prune=now &&
> 
> with:
> 
>     # Prune all unreachable objects from the cruft pack
> 
> But I don't think the current wording is a problem, either, so feel free
> to take or leave these suggestions.

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

Show 6 quoted lines

> Quoting from your original coverage report, this should take care of:
> 
> Taylor Blau     5b92477f builtin/gc.c: conditionally avoid pruning objects via loose
> builtin/gc.c
> 5b92477f 337) strvec_push(&repack, "--cruft");
> 5b92477f 338) if (prune_expire)

These lines, yes.

> 5b92477f 339) strvec_pushf(&repack, "--cruft-expiration=%s", prune_expire);

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

Derrick Stolee via GitGitGadget· Jun 14, 2022, 21:27 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 3/4] pack-write: drop always-NULL parameter

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 pack-write.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Show changes to pack-write.c +6 −11

diff --git a/pack-write.c b/pack-write.c
index 23c0342018f..00787e306db 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -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

Derrick Stolee via GitGitGadget· Jun 14, 2022, 21:27 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH 4/4] cache-tree: remove cache_tree_find_path()

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 cache-tree.c | 27 ---------------------------
 cache-tree.h |  2 --
 2 files changed, 29 deletions(-)

Show changes to 2 files +0 −29

cache-tree.c, cache-tree.h

diff --git a/cache-tree.c b/cache-tree.c
index ff794d940fa..56db0b5026b 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -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
diff --git a/cache-tree.h b/cache-tree.h
index f75f8e74dcd..8efeccebfc9 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -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

Derrick Stolee via GitGitGadget· Jun 16, 2022, 13:13 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 0/4] 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/3d1c6dfd-1df6-3393-df5e-692719375772@github.com/

 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
      
          Signed-off-by: Derrick Stolee <derrickstolee@github.com>
      
     - ## 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

Derrick Stolee via GitGitGadget· Jun 16, 2022, 13:13 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 1/4] t2107: test 'git update-index --verbose'

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t2107-update-index-basic.sh | 31 +++++++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

Show changes to t/t2107-update-index-basic.sh +25 −6

diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index a30b7ca6bc9..07e6de84e6d 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -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

Derrick Stolee via GitGitGadget· Jun 16, 2022, 13:13 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 2/4] t5329: test 'git gc --cruft' without '--prune=now'

From: Derrick Stolee <derrickstolee@github.com>

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.

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 t/t5329-pack-objects-cruft.sh | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Show changes to t/t5329-pack-objects-cruft.sh +3 −1

diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh
index b481224b93d..8968f7a08d8 100755
--- a/t/t5329-pack-objects-cruft.sh
+++ b/t/t5329-pack-objects-cruft.sh
@@ -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

Derrick Stolee via GitGitGadget· Jun 16, 2022, 13:13 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 3/4] pack-write: drop always-NULL parameter

From: Derrick Stolee <derrickstolee@github.com>

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 pack-write.c | 17 ++++++-----------
 1 file changed, 6 insertions(+), 11 deletions(-)

Show changes to pack-write.c +6 −11

diff --git a/pack-write.c b/pack-write.c
index 23c0342018f..00787e306db 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -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

Derrick Stolee via GitGitGadget· Jun 16, 2022, 13:13 UTC · re: Derrick Stolee via GitGitGadget · lore

[PATCH v2 4/4] cache-tree: remove cache_tree_find_path()

From: Derrick Stolee <derrickstolee@github.com>

Signed-off-by: Derrick Stolee <derrickstolee@github.com>
---
 cache-tree.c | 27 ---------------------------
 cache-tree.h |  2 --
 2 files changed, 29 deletions(-)

Show changes to 2 files +0 −29

cache-tree.c, cache-tree.h

diff --git a/cache-tree.c b/cache-tree.c
index ff794d940fa..56db0b5026b 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -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
diff --git a/cache-tree.h b/cache-tree.h
index f75f8e74dcd..8efeccebfc9 100644
--- a/cache-tree.h
+++ b/cache-tree.h
@@ -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

Junio C Hamano· Jun 16, 2022, 19:13 UTC · re: Derrick Stolee via GitGitGadget · lore

Re: [PATCH v2 0/4] Reactions to v2.37.0-rc0 test coverage report

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 23 quoted lines

> 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/3d1c6dfd-1df6-3393-df5e-692719375772@github.com/
>
>  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.  Queued.