{"thread":{"id":"58004","subject":"[PATCH 0/4] Reactions to v2.37.0-rc0 test coverage report","startedAt":"2022-06-14T21:27:58Z","lastAt":"2022-06-16T19:13:37Z","messageCount":15,"participants":["Derrick Stolee via GitGitGadget","Taylor Blau","Derrick Stolee","Eric Sunshine","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"457220","messageId":"pull.1261.git.1655242070.gitgitgadget@gmail.com","threadId":"58004","inReplyTo":null,"subject":"[PATCH 0/4] Reactions to v2.37.0-rc0 test coverage report","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T21:27:46Z","receivedAt":"2022-06-14T21:27:58Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"These patches add test coverage or simplify code based on discoveries in the\ntest coverage report (specifically, some that I highlighted at [1]).\n\n[1]\nhttps://lore.kernel.org/git/3d1c6dfd-1df6-3393-df5e-692719375772@github.com/\n\n 1. Add tests for 'git update-index --verbose'.\n 2. Add 'git gc --cruft' without '--prune=now' to test.\n 3. Drop an always-NULL parameter from an internal method.\n 4. Revert 080ab56a4 (cache-tree: implement cache_tree_find_path(),\n 2022-05-23).\n\nAny subset of these could be taken (or dropped), but I thought they would be\nworth considering.\n\nThanks, -Stolee\n\nDerrick Stolee (4):\n t2107: test 'git update-index --verbose'\n t5329: test 'git gc --cruft' without '--prune=now'\n pack-write: drop always-NULL parameter\n cache-tree: remove cache_tree_find_path()\n\n cache-tree.c | 27 ---------------------\n cache-tree.h | 2 --\n pack-write.c | 17 +++++--------\n t/t2106-update-index-assume-unchanged.sh | 2 +-\n t/t2107-update-index-basic.sh | 31 +++++++++++++++++++-----\n t/t5329-pack-objects-cruft.sh | 4 ++-\n 6 files changed, 35 insertions(+), 48 deletions(-)\n\n\nbase-commit: 8168d5e9c23ed44ae3d604f392320d66556453c9\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1261%2Fderrickstolee%2Ftest-coverage-response-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1261/derrickstolee/test-coverage-response-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1261\n-- \ngitgitgadget\n"},{"id":"457221","messageId":"c6803df1b6afead99a0a6a383ab9aa563920f464.1655242070.git.gitgitgadget@gmail.com","threadId":"58004","inReplyTo":"pull.1261.git.1655242070.gitgitgadget@gmail.com","subject":"[PATCH 1/4] t2107: test 'git update-index --verbose'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T21:27:47Z","receivedAt":"2022-06-14T21:28:10Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThe '--verbose' option reports what is being added and removed from the\nindex, but has not been tested up to this point. Augment the tests in\nt2107 to check the '--verbose' option in some scenarios.\n\nSigned-off-by: Derrick Stolee \n---\n t/t2106-update-index-assume-unchanged.sh | 2 +-\n t/t2107-update-index-basic.sh | 31 +++++++++++++++++++-----\n 2 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t2106-update-index-assume-unchanged.sh b/t/t2106-update-index-assume-unchanged.sh\nindex d943ddf47e0..ad2692a2979 100755\n--- a/t/t2106-update-index-assume-unchanged.sh\n+++ b/t/t2106-update-index-assume-unchanged.sh\n@@ -20,7 +20,7 @@ test_expect_success 'do not switch branches with dirty file' '\n \tgit reset --hard &&\n \tgit checkout other &&\n \techo dirt >file &&\n-\tgit update-index --assume-unchanged file &&\n+\tgit update-index --verbose --assume-unchanged file &&\n \ttest_must_fail git checkout - 2>err &&\n \ttest_i18ngrep overwritten err\n '\ndiff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh\nindex a30b7ca6bc9..07e6de84e6d 100755\n--- a/t/t2107-update-index-basic.sh\n+++ b/t/t2107-update-index-basic.sh\n@@ -36,9 +36,14 @@ test_expect_success '--cacheinfo does not accept blob null sha1' '\n \techo content >file &&\n \tgit add file &&\n \tgit rev-parse :file >expect &&\n-\ttest_must_fail git update-index --cacheinfo 100644 $ZERO_OID file &&\n+\ttest_must_fail git update-index --verbose --cacheinfo 100644 $ZERO_OID file >out &&\n \tgit rev-parse :file >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tadd '\\''file'\\''\n+\tEOF\n+\ttest_cmp expect out\n '\n \n test_expect_success '--cacheinfo does not accept gitlink null sha1' '\n@@ -59,9 +64,14 @@ test_expect_success '--cacheinfo mode,sha1,path (new syntax)' '\n \tgit rev-parse :file >actual &&\n \ttest_cmp expect actual &&\n \n-\tgit update-index --add --cacheinfo \"100644,$(cat expect),elif\" &&\n+\tgit update-index --add --verbose --cacheinfo \"100644,$(cat expect),elif\" >out &&\n \tgit rev-parse :elif >actual &&\n-\ttest_cmp expect actual\n+\ttest_cmp expect actual &&\n+\n+\tcat >expect <<-\\EOF &&\n+\tadd '\\''elif'\\''\n+\tEOF\n+\ttest_cmp expect out\n '\n \n test_expect_success '.lock files cleaned up' '\n@@ -74,7 +84,8 @@ test_expect_success '.lock files cleaned up' '\n \tgit config core.worktree ../../worktree &&\n \t# --refresh triggers late setup_work_tree,\n \t# active_cache_changed is zero, rollback_lock_file fails\n-\tgit update-index --refresh &&\n+\tgit update-index --refresh --verbose >out &&\n+\ttest_must_be_empty out &&\n \t! test -f .git/index.lock\n \t)\n '\n@@ -83,7 +94,15 @@ test_expect_success '--chmod=+x and chmod=-x in the same argument list' '\n \t>A &&\n \t>B &&\n \tgit add A B &&\n-\tgit update-index --chmod=+x A --chmod=-x B &&\n+\tgit update-index --verbose --chmod=+x A --chmod=-x B >out &&\n+\tcat >expect <<-\\EOF &&\n+\tadd '\\''A'\\''\n+\tchmod +x '\\''A'\\''\n+\tadd '\\''B'\\''\n+\tchmod -x '\\''B'\\''\n+\tEOF\n+\ttest_cmp expect out &&\n+\n \tcat >expect <<-EOF &&\n \t100755 $EMPTY_BLOB 0\tA\n \t100644 $EMPTY_BLOB 0\tB\n-- \ngitgitgadget\n\n"},{"id":"457222","messageId":"f59a0c326a01ab5f4e415a7ed03ff046b84b337b.1655242070.git.gitgitgadget@gmail.com","threadId":"58004","inReplyTo":"pull.1261.git.1655242070.gitgitgadget@gmail.com","subject":"[PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T21:27:48Z","receivedAt":"2022-06-14T21:28:11Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nReplace a 'git repack --cruft -d' with the wrapper 'git gc --cruft' to\nexercise some logic in builtin/gc.c that adds the '--cruft' option to\nthe underlying 'git repack' command.\n\nSigned-off-by: Derrick Stolee \n---\n t/t5329-pack-objects-cruft.sh | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\nindex b481224b93d..55bb9c0e7b5 100755\n--- a/t/t5329-pack-objects-cruft.sh\n+++ b/t/t5329-pack-objects-cruft.sh\n@@ -451,11 +451,13 @@ test_expect_success 'expiring cruft objects with git gc' '\n \t\tsort reachable &&\n \t\tcomm -13 reachable objects >unreachable &&\n \n-\t\tgit repack --cruft -d &&\n+\t\t# Write a cruft pack instead of deleting files.\n+\t\tgit gc --cruft &&\n \n \t\tmtimes=$(ls .git/objects/pack/pack-*.mtimes) &&\n \t\ttest_path_is_file $mtimes &&\n \n+\t\t# Ignore the cruft pack and delete every unreachable object.\n \t\tgit gc --cruft --prune=now &&\n \n \t\tgit cat-file --batch-all-objects --batch-check=\"%(objectname)\" >objects &&\n-- \ngitgitgadget\n\n"},{"id":"457223","messageId":"b67e110bf60e820874de94c64ee8c32d69413877.1655242070.git.gitgitgadget@gmail.com","threadId":"58004","inReplyTo":"pull.1261.git.1655242070.gitgitgadget@gmail.com","subject":"[PATCH 3/4] pack-write: drop always-NULL parameter","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T21:27:49Z","receivedAt":"2022-06-14T21:28:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nwrite_mtimes_file() takes an mtimes parameter as its first option, but\nthe only caller passes a NULL constant. Drop this parameter to simplify\nlogic. This can be reverted if that parameter is needed in the future.\n\nSigned-off-by: Derrick Stolee \n---\n pack-write.c | 17 ++++++-----------\n 1 file changed, 6 insertions(+), 11 deletions(-)\n\ndiff --git a/pack-write.c b/pack-write.c\nindex 23c0342018f..00787e306db 100644\n--- a/pack-write.c\n+++ b/pack-write.c\n@@ -310,26 +310,21 @@ static void write_mtimes_trailer(struct hashfile *f, const unsigned char *hash)\n \thashwrite(f, hash, the_hash_algo->rawsz);\n }\n \n-static const char *write_mtimes_file(const char *mtimes_name,\n-\t\t\t\t struct packing_data *to_pack,\n+static const char *write_mtimes_file(struct packing_data *to_pack,\n \t\t\t\t struct pack_idx_entry **objects,\n \t\t\t\t uint32_t nr_objects,\n \t\t\t\t const unsigned char *hash)\n {\n+\tstruct strbuf tmp_file = STRBUF_INIT;\n+\tconst char *mtimes_name;\n \tstruct hashfile *f;\n \tint fd;\n \n \tif (!to_pack)\n \t\tBUG(\"cannot call write_mtimes_file with NULL packing_data\");\n \n-\tif (!mtimes_name) {\n-\t\tstruct strbuf tmp_file = STRBUF_INIT;\n-\t\tfd = odb_mkstemp(&tmp_file, \"pack/tmp_mtimes_XXXXXX\");\n-\t\tmtimes_name = strbuf_detach(&tmp_file, NULL);\n-\t} else {\n-\t\tunlink(mtimes_name);\n-\t\tfd = xopen(mtimes_name, O_CREAT|O_EXCL|O_WRONLY, 0600);\n-\t}\n+\tfd = odb_mkstemp(&tmp_file, \"pack/tmp_mtimes_XXXXXX\");\n+\tmtimes_name = strbuf_detach(&tmp_file, NULL);\n \tf = hashfd(fd, mtimes_name);\n \n \twrite_mtimes_header(f);\n@@ -561,7 +556,7 @@ void stage_tmp_packfiles(struct strbuf *name_buffer,\n \t\t\t\t pack_idx_opts->flags);\n \n \tif (pack_idx_opts->flags & WRITE_MTIMES) {\n-\t\tmtimes_tmp_name = write_mtimes_file(NULL, to_pack, written_list,\n+\t\tmtimes_tmp_name = write_mtimes_file(to_pack, written_list,\n \t\t\t\t\t\t nr_written,\n \t\t\t\t\t\t hash);\n \t}\n-- \ngitgitgadget\n\n"},{"id":"457224","messageId":"81d8e2b5fddbc0943f6daa6d8bdb189bb98da57e.1655242070.git.gitgitgadget@gmail.com","threadId":"58004","inReplyTo":"pull.1261.git.1655242070.gitgitgadget@gmail.com","subject":"[PATCH 4/4] cache-tree: remove cache_tree_find_path()","fromName":"Derrick Stolee via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-06-14T21:27:50Z","receivedAt":"2022-06-14T21:28:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"From: Derrick Stolee \n\nThis reverts 080ab56a46 (cache-tree: implement cache_tree_find_path(),\n2022-05-23). The cache_tree_find_path() method was never actually called\nin the topic that added it. I cannot find any reference to it in any of\nmy forks, so this appears to not be needed at the moment.\n\nSigned-off-by: Derrick Stolee \n---\n cache-tree.c | 27 ---------------------------\n cache-tree.h | 2 --\n 2 files changed, 29 deletions(-)\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex ff794d940fa..56db0b5026b 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -101,33 +101,6 @@ struct cache_tree_sub *cache_tree_sub(struct cache_tree *it, const char *path)\n \treturn find_subtree(it, path, pathlen, 1);\n }\n \n-struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path)\n-{\n-\tconst char *slash;\n-\tint namelen;\n-\tstruct cache_tree_sub it_sub = {\n-\t\t.cache_tree = it,\n-\t};\n-\tstruct cache_tree_sub *down = &it_sub;\n-\n-\twhile (down) {\n-\t\tslash = strchrnul(path, '/');\n-\t\tnamelen = slash - path;\n-\t\tdown->cache_tree->entry_count = -1;\n-\t\tif (!*slash) {\n-\t\t\tint pos;\n-\t\t\tpos = cache_tree_subtree_pos(down->cache_tree, path, namelen);\n-\t\t\tif (0 <= pos)\n-\t\t\t\treturn down->cache_tree->down[pos]->cache_tree;\n-\t\t\treturn NULL;\n-\t\t}\n-\t\tdown = find_subtree(it, path, namelen, 0);\n-\t\tpath = slash + 1;\n-\t}\n-\n-\treturn NULL;\n-}\n-\n static int do_invalidate_path(struct cache_tree *it, const char *path)\n {\n \t/* a/b/c\ndiff --git a/cache-tree.h b/cache-tree.h\nindex f75f8e74dcd..8efeccebfc9 100644\n--- a/cache-tree.h\n+++ b/cache-tree.h\n@@ -29,8 +29,6 @@ struct cache_tree_sub *cache_tree_sub(struct cache_tree *, const char *);\n \n int cache_tree_subtree_pos(struct cache_tree *it, const char *path, int pathlen);\n \n-struct cache_tree *cache_tree_find_path(struct cache_tree *it, const char *path);\n-\n void cache_tree_write(struct strbuf *, struct cache_tree *root);\n struct cache_tree *cache_tree_read(const char *buffer, unsigned long size);\n \n-- \ngitgitgadget\n"},{"id":"457229","messageId":"YqkcEhQX4qm7fSer@nand.local","threadId":"58004","inReplyTo":"f59a0c326a01ab5f4e415a7ed03ff046b84b337b.1655242070.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-06-14T23:38:58Z","receivedAt":"2022-06-14T23:39:04Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Jun 14, 2022 at 09:27:48PM +0000, Derrick Stolee via GitGitGadget wrote:\n> From: Derrick Stolee \n>\n> Replace a 'git repack --cruft -d' with the wrapper 'git gc --cruft' to\n> exercise some logic in builtin/gc.c that adds the '--cruft' option to\n> the underlying 'git repack' command.\n>\n> Signed-off-by: Derrick Stolee \n> ---\n> t/t5329-pack-objects-cruft.sh | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t5329-pack-objects-cruft.sh b/t/t5329-pack-objects-cruft.sh\n> index b481224b93d..55bb9c0e7b5 100755\n> --- a/t/t5329-pack-objects-cruft.sh\n> +++ b/t/t5329-pack-objects-cruft.sh\n> @@ -451,11 +451,13 @@ test_expect_success 'expiring cruft objects with git gc' '\n> \t\tsort reachable &&\n> \t\tcomm -13 reachable objects >unreachable &&\n>\n> -\t\tgit repack --cruft -d &&\n> +\t\t# Write a cruft pack instead of deleting files.\n> +\t\tgit gc --cruft &&\n\nThese (\"git repack --cruft -d\" and \"git gc --cruft\") do the same thing,\nso this transformation makes sense.\n\nIt may be slightly clearer to refer to \"objects\" instead of \"files\",\nperhaps like:\n\n # Write a cruft pack containing all unreachable objects\n\nand then replace:\n\n> +\t\t# Ignore the cruft pack and delete every unreachable object.\n> \t\tgit gc --cruft --prune=now &&\n\nwith:\n\n # Prune all unreachable objects from the cruft pack\n\nBut I don't think the current wording is a problem, either, so feel free\nto take or leave these suggestions.\n\nQuoting from your original coverage report, this should take care of:\n\nTaylor Blau 5b92477f builtin/gc.c: conditionally avoid pruning objects via loose\nbuiltin/gc.c\n5b92477f 337) strvec_push(&repack, \"--cruft\");\n5b92477f 338) if (prune_expire)\n5b92477f 339) strvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n\nso this patch looks good to me.\n\nThanks,\nTaylor\n"},{"id":"457279","messageId":"5c05094a-5563-d55a-ea12-7dd74cd22fca@github.com","threadId":"58004","inReplyTo":"YqkcEhQX4qm7fSer@nand.local","subject":"Re: [PATCH 2/4] t5329: test 'git gc --cruft' without '--prune=now'","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-06-15T13:28:07Z","receivedAt":"2022-06-15T13:28:14Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 6/14/22 7:38 PM, Taylor Blau wrote:\n> On Tue, Jun 14, 2022 at 09:27:48PM +0000, Derrick Stolee via GitGitGadget wrote:\n\n>> -\t\tgit repack --cruft -d &&\n>> +\t\t# Write a cruft pack instead of deleting files.\n>> +\t\tgit gc --cruft &&\n> \n> These (\"git repack --cruft -d\" and \"git gc --cruft\") do the same thing,\n> so this transformation makes sense.\n> \n> It may be slightly clearer to refer to \"objects\" instead of \"files\",\n> perhaps like:\n> \n> # Write a cruft pack containing all unreachable objects\n> \n> and then replace:\n> \n>> +\t\t# Ignore the cruft pack and delete every unreachable object.\n>> \t\tgit gc --cruft --prune=now &&\n> \n> with:\n> \n> # Prune all unreachable objects from the cruft pack\n> \n> But I don't think the current wording is a problem, either, so feel free\n> to take or leave these suggestions.\n\nI like your suggestions. I'll update them if there are other reasons\nto do a v2.\n \n> Quoting from your original coverage report, this should take care of:\n> \n> Taylor Blau 5b92477f builtin/gc.c: conditionally avoid pruning objects via loose\n> builtin/gc.c\n> 5b92477f 337) strvec_push(&repack, \"--cruft\");\n> 5b92477f 338) if (prune_expire)\n\nThese lines, yes.\n\n> 5b92477f 339) strvec_pushf(&repack, \"--cruft-expiration=%s\", prune_expire);\n\nThis one requires \"git gc --cruft --prune=