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

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

[Derrick Stolee via 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/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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Derrick Stolee via GitGitGadget]

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 &&
 
 		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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Taylor Blau]

On Tue, Jun 14, 2022 at 09:27:48PM +0000, Derrick Stolee via GitGitGadget wrote:
> 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

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

[Derrick Stolee]

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:

>> -		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.
 
> 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

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

[Eric Sunshine]

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.

> 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.

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

[Derrick Stolee]

On 6/15/2022 7:18 PM, Eric Sunshine wrote:
> 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.
 
>> 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

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

[Derrick Stolee via 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/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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Derrick Stolee via GitGitGadget]

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..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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Derrick Stolee via GitGitGadget]

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(-)

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

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

[Junio C Hamano]

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

> 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.