{"thread":{"id":"40797","subject":"[PATCH 0/3] Add cleanup for garbage .bitmap files","startedAt":"2015-11-14T00:10:50Z","lastAt":"2016-01-19T18:36:03Z","messageCount":33,"participants":["Doug Kelly","Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"273306","messageId":"1447459853-28838-1-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":null,"subject":"[PATCH 0/3] Add cleanup for garbage .bitmap files","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:10:50Z","receivedAt":"2015-11-14T00:10:50Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Following Peff and Junio's comments when adding support for cleaning garbage\n.idx files left in the pack directory, this patch introduces the ability to\ndetect garbage .bitmap files. Additionally, .keep files are still reported,\nbut no action is taken to clean them.\n\nThis includes some refactor around count-objects' report_pack_garbage handler,\nto make it more extensible when adding understanding for different file types.\nTesting shows this working, but it may be a section to provide extra scrutiny\nto.\n"},{"id":"273309","messageId":"1447459853-28838-2-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1447459853-28838-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:10:51Z","receivedAt":"2015-11-14T00:10:51Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":".bitmap and .keep files without .idx/.pack don't make much sense, so\nmake sure these are reported as garbage as well. At the same time,\nrefactoring report_garbage to handle extra bits.\n\nSigned-off-by: Doug Kelly \n---\n builtin/count-objects.c | 16 ++++++----------\n cache.h | 4 +++-\n sha1_file.c | 17 +++++++++++++++--\n t/t5304-prune.sh | 2 ++\n 4 files changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..1637037 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -17,19 +17,15 @@ static off_t loose_size;\n \n static const char *bits_to_msg(unsigned seen_bits)\n {\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\treturn \"no corresponding .idx or .pack\";\n-\tcase PACKDIR_FILE_GARBAGE:\n+\tif (seen_bits == PACKDIR_FILE_GARBAGE)\n \t\treturn \"garbage found\";\n-\tcase PACKDIR_FILE_PACK:\n+\telse if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ ~PACKDIR_FILE_IDX)\n \t\treturn \"no corresponding .idx\";\n-\tcase PACKDIR_FILE_IDX:\n+\telse if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ ~PACKDIR_FILE_PACK)\n \t\treturn \"no corresponding .pack\";\n-\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n-\tdefault:\n-\t\treturn NULL;\n-\t}\n+\telse if (seen_bits == 0 || seen_bits ^ ~(PACKDIR_FILE_IDX|PACKDIR_FILE_PACK))\n+\t\treturn \"no corresponding .idx or .pack\";\n+\treturn NULL;\n }\n \n static void real_report_garbage(unsigned seen_bits, const char *path)\ndiff --git a/cache.h b/cache.h\nindex 736abc0..5b9d791 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_\n /* A hook to report invalid files in pack directory */\n #define PACKDIR_FILE_PACK 1\n #define PACKDIR_FILE_IDX 2\n-#define PACKDIR_FILE_GARBAGE 4\n+#define PACKDIR_FILE_BITMAP 4\n+#define PACKDIR_FILE_KEEP 8\n+#define PACKDIR_FILE_GARBAGE 16\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3d56746..5f939e4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,\n \tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n \t\treturn;\n \n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))\n+\t\treturn;\n+\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))\n+\t\treturn;\n+\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))\n+\t\treturn;\n+\n \tfor (; first < last; first++)\n \t\treport_garbage(seen_bits, list->items[first].string);\n }\n@@ -1256,9 +1265,13 @@ static void report_pack_garbage(struct string_list *list)\n \t\t\tfirst = i;\n \t\t}\n \t\tif (!strcmp(path + baselen, \"pack\"))\n-\t\t\tseen_bits |= 1;\n+\t\t\tseen_bits |= PACKDIR_FILE_PACK;\n \t\telse if (!strcmp(path + baselen, \"idx\"))\n-\t\t\tseen_bits |= 2;\n+\t\t\tseen_bits |= PACKDIR_FILE_IDX;\n+\t\telse if (!strcmp(path + baselen, \"bitmap\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_BITMAP;\n+\t\telse if (!strcmp(path + baselen, \"keep\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_KEEP;\n \t}\n \treport_helper(list, seen_bits, first, list->nr);\n }\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex def203c..1ea8279 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n+warning: no corresponding .pack: .git/objects/pack/fake2.idx\n+warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"273308","messageId":"1447459853-28838-3-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1447459853-28838-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 2/3] t5304: Add test for .bitmap garbage files","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:10:52Z","receivedAt":"2015-11-14T00:10:52Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"When checking for pack garbage, .bitmap files are now detected as\ngarbage when not associated with another .pack/.idx file.\n\nSigned-off-by: Doug Kelly \n---\n t/t5304-prune.sh | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 1ea8279..4fa6e7a 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '\n \t: >.git/objects/pack/fake.idx &&\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake3.idx &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"index file .git/objects/pack/fake.idx is too small\" stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar\n warning: garbage found: .git/objects/pack/foo\n warning: garbage found: .git/objects/pack/foo.bar\n warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n warning: no corresponding .pack: .git/objects/pack/fake3.idx\n+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake5.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake6.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.keep\n EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'clean pack garbage with gc' '\n+test_expect_failure 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake2.idx &&\n \t: >.git/objects/pack/fake3.keep &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit gc &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n \tcat >expected <<\\EOF &&\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n-warning: no corresponding .pack: .git/objects/pack/fake2.idx\n-warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"273307","messageId":"1447459853-28838-4-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1447459853-28838-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 3/3] gc: Clean garbage .bitmap files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:10:53Z","receivedAt":"2015-11-14T00:10:53Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Similar to cleaning up excess .idx files, clean any garbage .bitmap\nfiles that are not otherwise associated with any .idx/.pack files.\n\nSigned-off-by: Doug Kelly \n---\n builtin/gc.c | 12 ++++++++++--\n t/t5304-prune.sh | 2 +-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c583aad..7ddf071 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,8 +58,16 @@ static void clean_pack_garbage(void)\n \n static void report_pack_garbage(unsigned seen_bits, const char *path)\n {\n-\tif (seen_bits == PACKDIR_FILE_IDX)\n-\t\tstring_list_append(&pack_garbage, path);\n+\tif (seen_bits & PACKDIR_FILE_IDX ||\n+\t seen_bits & PACKDIR_FILE_BITMAP) {\n+\t\tconst char *dot = strrchr(path, '.');\n+\t\tif (dot) {\n+\t\t\tint baselen = dot - path + 1;\n+\t\t\tif (!strcmp(path+baselen, \"idx\") ||\n+\t\t\t\t!strcmp(path+baselen, \"bitmap\"))\n+\t\t\t\tstring_list_append(&pack_garbage, path);\n+\t\t}\n+\t}\n }\n \n static void git_config_date_string(const char *key, const char **output)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 4fa6e7a..9f9f263 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -257,7 +257,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'clean pack garbage with gc' '\n+test_expect_success 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n-- \n2.6.1\n"},{"id":"273311","messageId":"CAGZ79kYPv2OLzMX6t9=mejes9F8CzxAJiERs8GGxDnaAG8Q64g@mail.gmail.com","threadId":"40797","inReplyTo":"1447459853-28838-2-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-14T00:43:41Z","receivedAt":"2015-11-14T00:43:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> + else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ ~PACKDIR_FILE_IDX)\n\nas just talked about: did you mention && !(seen_bits & FILE_IDX)\n>\n> + if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))\n> + return;\n> +\n> + if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))\n> + return;\n> +\n> + if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))\n> + return;\n\nI wonder if this should be rewritten as\n if (seen_bits & FILE_PACK && seen_bits & FILE_IDX\n && (seen_bits & FILE_KEEP || seen_bits & BITMAP))\n return;\n\nto dense it a bit. ;)\n"},{"id":"273313","messageId":"1447461987-35450-1-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"CAGZ79kYPv2OLzMX6t9=mejes9F8CzxAJiERs8GGxDnaAG8Q64g@mail.gmail.com","subject":"[PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:46:25Z","receivedAt":"2015-11-14T00:46:25Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":".bitmap and .keep files without .idx/.pack don't make much sense, so\nmake sure these are reported as garbage as well. At the same time,\nrefactoring report_garbage to handle extra bits.\n\nSigned-off-by: Doug Kelly \n---\n builtin/count-objects.c | 16 ++++++----------\n cache.h | 4 +++-\n sha1_file.c | 17 +++++++++++++++--\n t/t5304-prune.sh | 2 ++\n 4 files changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..1637037 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -17,19 +17,15 @@ static off_t loose_size;\n \n static const char *bits_to_msg(unsigned seen_bits)\n {\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\treturn \"no corresponding .idx or .pack\";\n-\tcase PACKDIR_FILE_GARBAGE:\n+\tif (seen_bits == PACKDIR_FILE_GARBAGE)\n \t\treturn \"garbage found\";\n-\tcase PACKDIR_FILE_PACK:\n+\telse if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ ~PACKDIR_FILE_IDX)\n \t\treturn \"no corresponding .idx\";\n-\tcase PACKDIR_FILE_IDX:\n+\telse if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ ~PACKDIR_FILE_PACK)\n \t\treturn \"no corresponding .pack\";\n-\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n-\tdefault:\n-\t\treturn NULL;\n-\t}\n+\telse if (seen_bits == 0 || seen_bits ^ ~(PACKDIR_FILE_IDX|PACKDIR_FILE_PACK))\n+\t\treturn \"no corresponding .idx or .pack\";\n+\treturn NULL;\n }\n \n static void real_report_garbage(unsigned seen_bits, const char *path)\ndiff --git a/cache.h b/cache.h\nindex 736abc0..5b9d791 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_\n /* A hook to report invalid files in pack directory */\n #define PACKDIR_FILE_PACK 1\n #define PACKDIR_FILE_IDX 2\n-#define PACKDIR_FILE_GARBAGE 4\n+#define PACKDIR_FILE_BITMAP 4\n+#define PACKDIR_FILE_KEEP 8\n+#define PACKDIR_FILE_GARBAGE 16\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3d56746..5f939e4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,\n \tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n \t\treturn;\n \n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))\n+\t\treturn;\n+\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))\n+\t\treturn;\n+\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))\n+\t\treturn;\n+\n \tfor (; first < last; first++)\n \t\treport_garbage(seen_bits, list->items[first].string);\n }\n@@ -1256,9 +1265,13 @@ static void report_pack_garbage(struct string_list *list)\n \t\t\tfirst = i;\n \t\t}\n \t\tif (!strcmp(path + baselen, \"pack\"))\n-\t\t\tseen_bits |= 1;\n+\t\t\tseen_bits |= PACKDIR_FILE_PACK;\n \t\telse if (!strcmp(path + baselen, \"idx\"))\n-\t\t\tseen_bits |= 2;\n+\t\t\tseen_bits |= PACKDIR_FILE_IDX;\n+\t\telse if (!strcmp(path + baselen, \"bitmap\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_BITMAP;\n+\t\telse if (!strcmp(path + baselen, \"keep\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_KEEP;\n \t}\n \treport_helper(list, seen_bits, first, list->nr);\n }\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex def203c..1ea8279 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n+warning: no corresponding .pack: .git/objects/pack/fake2.idx\n+warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"273312","messageId":"1447461987-35450-2-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1447461987-35450-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 2/3] t5304: Add test for .bitmap garbage files","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:46:26Z","receivedAt":"2015-11-14T00:46:26Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"When checking for pack garbage, .bitmap files are now detected as\ngarbage when not associated with another .pack/.idx file.\n\nSigned-off-by: Doug Kelly \n---\n t/t5304-prune.sh | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 1ea8279..4fa6e7a 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '\n \t: >.git/objects/pack/fake.idx &&\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake3.idx &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"index file .git/objects/pack/fake.idx is too small\" stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar\n warning: garbage found: .git/objects/pack/foo\n warning: garbage found: .git/objects/pack/foo.bar\n warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n warning: no corresponding .pack: .git/objects/pack/fake3.idx\n+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake5.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake6.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.keep\n EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'clean pack garbage with gc' '\n+test_expect_failure 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake2.idx &&\n \t: >.git/objects/pack/fake3.keep &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit gc &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n \tcat >expected <<\\EOF &&\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n-warning: no corresponding .pack: .git/objects/pack/fake2.idx\n-warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"273314","messageId":"1447461987-35450-3-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1447461987-35450-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 3/3] gc: Clean garbage .bitmap files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:46:27Z","receivedAt":"2015-11-14T00:46:27Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Similar to cleaning up excess .idx files, clean any garbage .bitmap\nfiles that are not otherwise associated with any .idx/.pack files.\n\nSigned-off-by: Doug Kelly \n---\n builtin/gc.c | 12 ++++++++++--\n t/t5304-prune.sh | 2 +-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c583aad..7ddf071 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,8 +58,16 @@ static void clean_pack_garbage(void)\n \n static void report_pack_garbage(unsigned seen_bits, const char *path)\n {\n-\tif (seen_bits == PACKDIR_FILE_IDX)\n-\t\tstring_list_append(&pack_garbage, path);\n+\tif (seen_bits & PACKDIR_FILE_IDX ||\n+\t seen_bits & PACKDIR_FILE_BITMAP) {\n+\t\tconst char *dot = strrchr(path, '.');\n+\t\tif (dot) {\n+\t\t\tint baselen = dot - path + 1;\n+\t\t\tif (!strcmp(path+baselen, \"idx\") ||\n+\t\t\t\t!strcmp(path+baselen, \"bitmap\"))\n+\t\t\t\tstring_list_append(&pack_garbage, path);\n+\t\t}\n+\t}\n }\n \n static void git_config_date_string(const char *key, const char **output)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 4fa6e7a..9f9f263 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -257,7 +257,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'clean pack garbage with gc' '\n+test_expect_success 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n-- \n2.6.1\n"},{"id":"273315","messageId":"CAEtYS8T9kZo6J3ZTQn210xRFPvNVew3oqV3fWMXf2CdKh4we-Q@mail.gmail.com","threadId":"40797","inReplyTo":"CAGZ79kYPv2OLzMX6t9=mejes9F8CzxAJiERs8GGxDnaAG8Q64g@mail.gmail.com","subject":"Re: [PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-14T00:47:25Z","receivedAt":"2015-11-14T00:47:25Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Yes, without a doubt. I think I'm blaming this one on being late on a\nFriday afternoon, and really not thinking out the logic clearly. :)\n\nOn Fri, Nov 13, 2015 at 4:43 PM, Stefan Beller wrote:\n>> + else if (seen_bits & PACKDIR_FILE_PACK && seen_bits ^ ~PACKDIR_FILE_IDX)\n>\n> as just talked about: did you mention && !(seen_bits & FILE_IDX)\n>>\n>> + if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))\n>> + return;\n>> +\n>> + if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))\n>> + return;\n>> +\n>> + if (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))\n>> + return;\n>\n> I wonder if this should be rewritten as\n> if (seen_bits & FILE_PACK && seen_bits & FILE_IDX\n> && (seen_bits & FILE_KEEP || seen_bits & BITMAP))\n> return;\n>\n> to dense it a bit. ;)\n"},{"id":"273316","messageId":"CAGZ79kaHCC=M6g7gahLsX0vfyQ=fOOU3xJbtMQPOe3dtByKRMw@mail.gmail.com","threadId":"40797","inReplyTo":"1447459853-28838-3-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 2/3] t5304: Add test for .bitmap garbage files","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-14T00:47:55Z","receivedAt":"2015-11-14T00:47:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 13, 2015 at 4:10 PM, Doug Kelly wrote:\n> When checking for pack garbage, .bitmap files are now detected as\n> garbage when not associated with another .pack/.idx file.\n>\n> Signed-off-by: Doug Kelly \n> ---\n> t/t5304-prune.sh | 24 +++++++++++++++++++++---\n> 1 file changed, 21 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index 1ea8279..4fa6e7a 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '\n> : >.git/objects/pack/fake.idx &&\n> : >.git/objects/pack/fake2.keep &&\n> : >.git/objects/pack/fake3.idx &&\n> + : >.git/objects/pack/fake4.bitmap &&\n> + : >.git/objects/pack/fake5.bitmap &&\n> + : >.git/objects/pack/fake5.idx &&\n> + : >.git/objects/pack/fake6.keep &&\n> + : >.git/objects/pack/fake6.bitmap &&\n> + : >.git/objects/pack/fake6.idx &&\n> git count-objects -v 2>stderr &&\n> grep \"index file .git/objects/pack/fake.idx is too small\" stderr &&\n> grep \"^warning:\" stderr | sort >actual &&\n> @@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar\n> warning: garbage found: .git/objects/pack/foo\n> warning: garbage found: .git/objects/pack/foo.bar\n> warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n> +warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap\n\nDo we want to split that up further, into\n\n no corresponding .idx and .pack:...\n\nto tell that actually both files are missing and we know it?\n\n> warning: no corresponding .idx: .git/objects/pack/foo.keep\n> warning: no corresponding .idx: .git/objects/pack/foo.pack\n> warning: no corresponding .pack: .git/objects/pack/fake3.idx\n> +warning: no corresponding .pack: .git/objects/pack/fake5.bitmap\n> +warning: no corresponding .pack: .git/objects/pack/fake5.idx\n\nWondering if we can condense this into one message (because only\none pack is missing).\n\n> +warning: no corresponding .pack: .git/objects/pack/fake6.bitmap\n> +warning: no corresponding .pack: .git/objects/pack/fake6.idx\n> +warning: no corresponding .pack: .git/objects/pack/fake6.keep\n\nsame here.\n no corresponding .pack: .git/objects/pack/fake6.{keep,idx, bitmap}\nwould look nice and be shell compatible. (rm on that multi path just works,\nin case you expect the pack to be gone)\n"},{"id":"273750","messageId":"CAGZ79kaCNT06mAGQbHNgZmdBQUyxGFTFA2Y2FXvG2UG+P7s2kg@mail.gmail.com","threadId":"40797","inReplyTo":"1447461987-35450-1-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-11-25T18:43:27Z","receivedAt":"2015-11-25T18:43:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Nov 13, 2015 at 4:46 PM, Doug Kelly wrote:\n> return \"no corresponding .idx\";\n> - case PACKDIR_FILE_IDX:\n> + else if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ ~PACKDIR_FILE_PACK)\n\nDid you intend to use\n (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))\nhere?\n\nI was just looking at the state in peff/pu and it still has the xor\nvariant, which exposes more\nthan just the selected bit to the decision IIRC.\n"},{"id":"273781","messageId":"1448518529-2659-1-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"CAGZ79kaCNT06mAGQbHNgZmdBQUyxGFTFA2Y2FXvG2UG+P7s2kg@mail.gmail.com","subject":"[PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-26T06:15:29Z","receivedAt":"2015-11-26T06:15:29Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":".bitmap and .keep files without .idx/.pack don't make much sense, so\nmake sure these are reported as garbage as well. At the same time,\nrefactoring report_garbage to handle extra bits.\n\nSigned-off-by: Doug Kelly \n---\n builtin/count-objects.c | 16 ++++++----------\n cache.h | 4 +++-\n sha1_file.c | 17 +++++++++++++++--\n t/t5304-prune.sh | 2 ++\n 4 files changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..5197b57 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -17,19 +17,15 @@ static off_t loose_size;\n \n static const char *bits_to_msg(unsigned seen_bits)\n {\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\treturn \"no corresponding .idx or .pack\";\n-\tcase PACKDIR_FILE_GARBAGE:\n+\tif (seen_bits == PACKDIR_FILE_GARBAGE)\n \t\treturn \"garbage found\";\n-\tcase PACKDIR_FILE_PACK:\n+\telse if (seen_bits & PACKDIR_FILE_PACK && !(seen_bits & PACKDIR_FILE_IDX))\n \t\treturn \"no corresponding .idx\";\n-\tcase PACKDIR_FILE_IDX:\n+\telse if (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))\n \t\treturn \"no corresponding .pack\";\n-\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n-\tdefault:\n-\t\treturn NULL;\n-\t}\n+\telse if (seen_bits == 0 || !(seen_bits & (PACKDIR_FILE_IDX|PACKDIR_FILE_PACK)))\n+\t\treturn \"no corresponding .idx or .pack\";\n+\treturn NULL;\n }\n \n static void real_report_garbage(unsigned seen_bits, const char *path)\ndiff --git a/cache.h b/cache.h\nindex 736abc0..5b9d791 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_\n /* A hook to report invalid files in pack directory */\n #define PACKDIR_FILE_PACK 1\n #define PACKDIR_FILE_IDX 2\n-#define PACKDIR_FILE_GARBAGE 4\n+#define PACKDIR_FILE_BITMAP 4\n+#define PACKDIR_FILE_KEEP 8\n+#define PACKDIR_FILE_GARBAGE 16\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3d56746..5f939e4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,\n \tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n \t\treturn;\n \n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))\n+\t\treturn;\n+\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))\n+\t\treturn;\n+\n+\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))\n+\t\treturn;\n+\n \tfor (; first < last; first++)\n \t\treport_garbage(seen_bits, list->items[first].string);\n }\n@@ -1256,9 +1265,13 @@ static void report_pack_garbage(struct string_list *list)\n \t\t\tfirst = i;\n \t\t}\n \t\tif (!strcmp(path + baselen, \"pack\"))\n-\t\t\tseen_bits |= 1;\n+\t\t\tseen_bits |= PACKDIR_FILE_PACK;\n \t\telse if (!strcmp(path + baselen, \"idx\"))\n-\t\t\tseen_bits |= 2;\n+\t\t\tseen_bits |= PACKDIR_FILE_IDX;\n+\t\telse if (!strcmp(path + baselen, \"bitmap\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_BITMAP;\n+\t\telse if (!strcmp(path + baselen, \"keep\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_KEEP;\n \t}\n \treport_helper(list, seen_bits, first, list->nr);\n }\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex def203c..1ea8279 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n+warning: no corresponding .pack: .git/objects/pack/fake2.idx\n+warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"273782","messageId":"CAEtYS8RHk8dbXs2jBRaCDkOHNEEFHWOxCAMFHY9+wJhWSSFpYQ@mail.gmail.com","threadId":"40797","inReplyTo":"CAGZ79kaCNT06mAGQbHNgZmdBQUyxGFTFA2Y2FXvG2UG+P7s2kg@mail.gmail.com","subject":"Re: [PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-11-26T06:18:41Z","receivedAt":"2015-11-26T06:18:41Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Apparently, I fixed this and forgot to re-run format-patch, so I sent\nout the same patch the second time... My fault on that one. I've at\nleast checked what I sent this time around, and it seems to match\nwhat's in my current tree. :) The second and third patches should be\nunmodified.\n\nThanks for catching that, Stefan!\n\nOn Wed, Nov 25, 2015 at 12:43 PM, Stefan Beller wrote:\n> On Fri, Nov 13, 2015 at 4:46 PM, Doug Kelly wrote:\n>> return \"no corresponding .idx\";\n>> - case PACKDIR_FILE_IDX:\n>> + else if (seen_bits & PACKDIR_FILE_IDX && seen_bits ^ ~PACKDIR_FILE_PACK)\n>\n> Did you intend to use\n> (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))\n> here?\n>\n> I was just looking at the state in peff/pu and it still has the xor\n> variant, which exposes more\n> than just the selected bit to the decision IIRC.\n"},{"id":"274520","messageId":"20151215230957.GA30353@sigill.intra.peff.net","threadId":"40797","inReplyTo":"1448518529-2659-1-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-15T23:09:57Z","receivedAt":"2015-12-15T23:09:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 26, 2015 at 12:15:29AM -0600, Doug Kelly wrote:\n\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index ba92919..5197b57 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -17,19 +17,15 @@ static off_t loose_size;\n> \n> static const char *bits_to_msg(unsigned seen_bits)\n> {\n> -\tswitch (seen_bits) {\n> -\tcase 0:\n> -\t\treturn \"no corresponding .idx or .pack\";\n> -\tcase PACKDIR_FILE_GARBAGE:\n> +\tif (seen_bits == PACKDIR_FILE_GARBAGE)\n> \t\treturn \"garbage found\";\n\nIt seems weird to use \"==\" on a bitfield. I think it is the case now\nthat we would never see GARBAGE alongside anything else, but I wonder if\nwe should future-proof that as:\n\n if (seen_bits & PACKDIR_FILE_GARBAGE)\n\nSpecifically, I am wondering what would happen if we had \"foo.pack\" and\n\"foo.bogus\", where we do not know about the latter at all.\n\n> -\tcase PACKDIR_FILE_PACK:\n> +\telse if (seen_bits & PACKDIR_FILE_PACK && !(seen_bits & PACKDIR_FILE_IDX))\n> \t\treturn \"no corresponding .idx\";\n> -\tcase PACKDIR_FILE_IDX:\n> +\telse if (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))\n> \t\treturn \"no corresponding .pack\";\n> -\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n> -\tdefault:\n> -\t\treturn NULL;\n> -\t}\n> +\telse if (seen_bits == 0 || !(seen_bits & (PACKDIR_FILE_IDX|PACKDIR_FILE_PACK)))\n> +\t\treturn \"no corresponding .idx or .pack\";\n> +\treturn NULL;\n\nThis bottom conditional is interesting. I understand the second half: we\nsaw something pack-like, but there is not matching .idx or .pack at all\n(if we saw one but not the other, we would have caught it above).\n\nBut when will we get an empty seen_bits? What did we see that triggered\nthis function, but didn't trigger a bit (even GARBAGE)?\n\nI don't mind if the answer is \"nothing, this is future-proofing\", but am\nmostly curious.\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 3d56746..5f939e4 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,\n> \tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n> \t\treturn;\n> \n> +\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP))\n> +\t\treturn;\n> +\n> +\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_KEEP))\n> +\t\treturn;\n> +\n> +\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX|PACKDIR_FILE_BITMAP|PACKDIR_FILE_KEEP))\n> +\t\treturn;\n> +\n\nIt seems like we're enumerating a lot of cases here that will explode if\nwe get even one more file type (e.g., we add \"pack-XXX.foo\" in the\nfuture). If I understand this function correctly, we're just trying to\nget rid of \"boring\" cases that do not need to be reported.\n\nIsn't any case that has both a pack and an idx boring (no matter if it\nhas a .bitmap or .keep)?\n\nIOW, can these four conditionals just become:\n\n unsigned pack_with_idx = PACKDIR_FILE_PACK | PACKDIR_FILE_IDX;\n\n if ((seen_bits & pack_with_idx) == pack_with_idx)\n\treturn;\n\n?\n\n-Peff\n"},{"id":"274522","messageId":"20151215232313.GB30353@sigill.intra.peff.net","threadId":"40797","inReplyTo":"1447461987-35450-3-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 3/3] gc: Clean garbage .bitmap files from pack dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-15T23:23:14Z","receivedAt":"2015-12-15T23:23:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 13, 2015 at 04:46:27PM -0800, Doug Kelly wrote:\n\n> Similar to cleaning up excess .idx files, clean any garbage .bitmap\n> files that are not otherwise associated with any .idx/.pack files.\n> \n> Signed-off-by: Doug Kelly \n> ---\n> builtin/gc.c | 12 ++++++++++--\n> t/t5304-prune.sh | 2 +-\n> 2 files changed, 11 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c583aad..7ddf071 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -58,8 +58,16 @@ static void clean_pack_garbage(void)\n> \n> static void report_pack_garbage(unsigned seen_bits, const char *path)\n> {\n> -\tif (seen_bits == PACKDIR_FILE_IDX)\n> -\t\tstring_list_append(&pack_garbage, path);\n> +\tif (seen_bits & PACKDIR_FILE_IDX ||\n> +\t seen_bits & PACKDIR_FILE_BITMAP) {\n\nSo here we're relying on report_helper to have culled the boring cases,\nright? (Sorry if that is totally obvious; I'm mostly just thinking out\nloud). That makes sense, then.\n\n> +\t\tconst char *dot = strrchr(path, '.');\n> +\t\tif (dot) {\n> +\t\t\tint baselen = dot - path + 1;\n> +\t\t\tif (!strcmp(path+baselen, \"idx\") ||\n> +\t\t\t\t!strcmp(path+baselen, \"bitmap\"))\n> +\t\t\t\tstring_list_append(&pack_garbage, path);\n> +\t\t}\n> +\t}\n\nI was confused at first why we couldn't just pass \"path\" here. But it's\nbecause we will get a garbage report for each related file, and we want\nto keep some of them (like .keep). Which I guess makes sense.\n\nI wonder if this would be simpler to read as just:\n\n if (ends_with(path, \".idx\") ||\n ends_with(path, \".bitmap\"))\n string_list_append(&pack_garbage, path);\n\nTechnically it is less efficient because we will compute strlen(path)\ntwice, but that seems like premature optimization (not to mention that\nends_with is an inline, so a good compiler can probably optimize out the\nsecond call anyway).\n\n> -test_expect_failure 'clean pack garbage with gc' '\n> +test_expect_success 'clean pack garbage with gc' '\n> \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n> \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n> \t: >.git/objects/pack/foo.keep &&\n\nShould we be checking at the end of this test that \"*.keep\" didn't get\nblown away? It might be nice to just test_cmp the results of \"ls\" on the\npack directory to confirm exactly what got deleted and what didn't.\n\n-Peff\n"},{"id":"274523","messageId":"20151215232534.GA30998@sigill.intra.peff.net","threadId":"40797","inReplyTo":"20151215230957.GA30353@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-15T23:25:35Z","receivedAt":"2015-12-15T23:25:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 15, 2015 at 06:09:57PM -0500, Jeff King wrote:\n\n> > @@ -1225,6 +1225,15 @@ static void report_helper(const struct string_list *list,\n> [...]\n> If I understand this function correctly, we're just trying to\n> get rid of \"boring\" cases that do not need to be reported.\n\nBTW, I wondered if this should perhaps just be calling bits_to_msg() and\nseeing if it returns NULL. It seems like the logic for which cases are\n\"interesting\" ends up duplicated. But maybe I am missing something.\n\n-Peff\n"},{"id":"274735","messageId":"1450483600-64091-1-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"20151215232534.GA30998@sigill.intra.peff.net","subject":"[PATCH v3 0/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-12-19T00:06:37Z","receivedAt":"2015-12-19T00:06:37Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Corrects the issues found in review by Peff, including simplifying\nthe logic in report_helper(). bits_to_msg() would've been an alternate\nsolution to that change, however it'll get called by\nreal_report_garbage(), so there's no need to call it twice, especially\nwhen the check we need within report_helper().\n\nI think checking for seen_bits == 0 would be future-proofing should we\narrive at a file bit not otherwise match it (i.e. file.foo and\nfile.bar, but nothing else would cause seen_bits to be zero, but if\nthat's the case, we wouldn't have PACKDIR_FILE_IDX or\nPACKDIR_FILE_PACK set, either, and the second half would also match.\n\nDoug Kelly (3):\n prepare_packed_git(): find more garbage\n t5304: Add test for .bitmap garbage files\n gc: Clean garbage .bitmap files from pack dir\n\n builtin/count-objects.c | 16 ++++++----------\n builtin/gc.c | 12 ++++++++++--\n cache.h | 4 +++-\n sha1_file.c | 12 +++++++++---\n t/t5304-prune.sh | 20 ++++++++++++++++++++\n 5 files changed, 48 insertions(+), 16 deletions(-)\n\n-- \n2.6.1\n"},{"id":"274733","messageId":"1450483600-64091-2-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1450483600-64091-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 1/3] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-12-19T00:06:38Z","receivedAt":"2015-12-19T00:06:38Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":".bitmap and .keep files without .idx/.pack don't make much sense, so\nmake sure these are reported as garbage as well. At the same time,\nrefactoring report_garbage to handle extra bits.\n\nSigned-off-by: Doug Kelly \n---\n builtin/count-objects.c | 16 ++++++----------\n cache.h | 4 +++-\n sha1_file.c | 12 +++++++++---\n t/t5304-prune.sh | 2 ++\n 4 files changed, 20 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..ed103ae 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -17,19 +17,15 @@ static off_t loose_size;\n \n static const char *bits_to_msg(unsigned seen_bits)\n {\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\treturn \"no corresponding .idx or .pack\";\n-\tcase PACKDIR_FILE_GARBAGE:\n+\tif (seen_bits & PACKDIR_FILE_GARBAGE)\n \t\treturn \"garbage found\";\n-\tcase PACKDIR_FILE_PACK:\n+\telse if (seen_bits & PACKDIR_FILE_PACK && !(seen_bits & PACKDIR_FILE_IDX))\n \t\treturn \"no corresponding .idx\";\n-\tcase PACKDIR_FILE_IDX:\n+\telse if (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))\n \t\treturn \"no corresponding .pack\";\n-\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n-\tdefault:\n-\t\treturn NULL;\n-\t}\n+\telse if (!(seen_bits & (PACKDIR_FILE_IDX|PACKDIR_FILE_PACK)))\n+\t\treturn \"no corresponding .idx or .pack\";\n+\treturn NULL;\n }\n \n static void real_report_garbage(unsigned seen_bits, const char *path)\ndiff --git a/cache.h b/cache.h\nindex 736abc0..5b9d791 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1292,7 +1292,9 @@ extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_\n /* A hook to report invalid files in pack directory */\n #define PACKDIR_FILE_PACK 1\n #define PACKDIR_FILE_IDX 2\n-#define PACKDIR_FILE_GARBAGE 4\n+#define PACKDIR_FILE_BITMAP 4\n+#define PACKDIR_FILE_KEEP 8\n+#define PACKDIR_FILE_GARBAGE 16\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 3d56746..3524274 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1222,7 +1222,9 @@ void (*report_garbage)(unsigned seen_bits, const char *path);\n static void report_helper(const struct string_list *list,\n \t\t\t int seen_bits, int first, int last)\n {\n-\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n+\tstatic const int pack_and_index = PACKDIR_FILE_PACK|PACKDIR_FILE_IDX;\n+\n+\tif ((seen_bits & pack_and_index) == pack_and_index)\n \t\treturn;\n \n \tfor (; first < last; first++)\n@@ -1256,9 +1258,13 @@ static void report_pack_garbage(struct string_list *list)\n \t\t\tfirst = i;\n \t\t}\n \t\tif (!strcmp(path + baselen, \"pack\"))\n-\t\t\tseen_bits |= 1;\n+\t\t\tseen_bits |= PACKDIR_FILE_PACK;\n \t\telse if (!strcmp(path + baselen, \"idx\"))\n-\t\t\tseen_bits |= 2;\n+\t\t\tseen_bits |= PACKDIR_FILE_IDX;\n+\t\telse if (!strcmp(path + baselen, \"bitmap\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_BITMAP;\n+\t\telse if (!strcmp(path + baselen, \"keep\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_KEEP;\n \t}\n \treport_helper(list, seen_bits, first, list->nr);\n }\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex def203c..1ea8279 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n+warning: no corresponding .pack: .git/objects/pack/fake2.idx\n+warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"274736","messageId":"1450483600-64091-3-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1450483600-64091-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 2/3] t5304: Add test for .bitmap garbage files","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-12-19T00:06:39Z","receivedAt":"2015-12-19T00:06:39Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"When checking for pack garbage, .bitmap files are now detected as\ngarbage when not associated with another .pack/.idx file.\n\nSigned-off-by: Doug Kelly \n---\n t/t5304-prune.sh | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 1ea8279..4fa6e7a 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '\n \t: >.git/objects/pack/fake.idx &&\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake3.idx &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"index file .git/objects/pack/fake.idx is too small\" stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar\n warning: garbage found: .git/objects/pack/foo\n warning: garbage found: .git/objects/pack/foo.bar\n warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n warning: no corresponding .pack: .git/objects/pack/fake3.idx\n+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake5.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake6.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.keep\n EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'clean pack garbage with gc' '\n+test_expect_failure 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake2.idx &&\n \t: >.git/objects/pack/fake3.keep &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit gc &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n \tcat >expected <<\\EOF &&\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n-warning: no corresponding .pack: .git/objects/pack/fake2.idx\n-warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"274734","messageId":"1450483600-64091-4-git-send-email-dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1450483600-64091-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH 3/3] gc: Clean garbage .bitmap files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2015-12-19T00:06:40Z","receivedAt":"2015-12-19T00:06:40Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Similar to cleaning up excess .idx files, clean any garbage .bitmap\nfiles that are not otherwise associated with any .idx/.pack files.\n\nSigned-off-by: Doug Kelly \n---\n builtin/gc.c | 12 ++++++++++--\n t/t5304-prune.sh | 2 +-\n 2 files changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c583aad..7ddf071 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,8 +58,16 @@ static void clean_pack_garbage(void)\n \n static void report_pack_garbage(unsigned seen_bits, const char *path)\n {\n-\tif (seen_bits == PACKDIR_FILE_IDX)\n-\t\tstring_list_append(&pack_garbage, path);\n+\tif (seen_bits & PACKDIR_FILE_IDX ||\n+\t seen_bits & PACKDIR_FILE_BITMAP) {\n+\t\tconst char *dot = strrchr(path, '.');\n+\t\tif (dot) {\n+\t\t\tint baselen = dot - path + 1;\n+\t\t\tif (!strcmp(path+baselen, \"idx\") ||\n+\t\t\t\t!strcmp(path+baselen, \"bitmap\"))\n+\t\t\t\tstring_list_append(&pack_garbage, path);\n+\t\t}\n+\t}\n }\n \n static void git_config_date_string(const char *key, const char **output)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 4fa6e7a..9f9f263 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -257,7 +257,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'clean pack garbage with gc' '\n+test_expect_success 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n-- \n2.6.1\n"},{"id":"274739","messageId":"20151219020123.GA31782@sigill.intra.peff.net","threadId":"40797","inReplyTo":"1450483600-64091-4-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH 3/3] gc: Clean garbage .bitmap files from pack dir","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-19T02:01:23Z","receivedAt":"2015-12-19T02:01:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 18, 2015 at 06:06:40PM -0600, Doug Kelly wrote:\n\n> Similar to cleaning up excess .idx files, clean any garbage .bitmap\n> files that are not otherwise associated with any .idx/.pack files.\n> \n> Signed-off-by: Doug Kelly \n> ---\n> builtin/gc.c | 12 ++++++++++--\n> t/t5304-prune.sh | 2 +-\n> 2 files changed, 11 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/gc.c b/builtin/gc.c\n> index c583aad..7ddf071 100644\n> --- a/builtin/gc.c\n> +++ b/builtin/gc.c\n> @@ -58,8 +58,16 @@ static void clean_pack_garbage(void)\n> \n> static void report_pack_garbage(unsigned seen_bits, const char *path)\n> {\n> -\tif (seen_bits == PACKDIR_FILE_IDX)\n> -\t\tstring_list_append(&pack_garbage, path);\n> +\tif (seen_bits & PACKDIR_FILE_IDX ||\n> +\t seen_bits & PACKDIR_FILE_BITMAP) {\n> +\t\tconst char *dot = strrchr(path, '.');\n> +\t\tif (dot) {\n> +\t\t\tint baselen = dot - path + 1;\n> +\t\t\tif (!strcmp(path+baselen, \"idx\") ||\n> +\t\t\t\t!strcmp(path+baselen, \"bitmap\"))\n> +\t\t\t\tstring_list_append(&pack_garbage, path);\n> +\t\t}\n> +\t}\n> }\n\nHmm. Thinking on this further, do we actually need to check seen_bits\nhere at all?\n\nThe original was trying to ask \"is this a .idx file\" by checking\nseen_bits. That was actually broken by the first patch in this series\nfor some cases, as we might send more bits. E.g., if you have \"foo.idx\"\nand \"foo.pack\", this function will get called twice (once per file), but\nwith seen_bits set to IDX|BITMAP for both cases. And we would not match\nthe \"==\" above, and would therefore fail to trigger.\n\nThat case is re-fixed by this patch, which is good. But I think\nseen_bits is not really telling us anything at this point. We know it's\na garbage case, or else report_helper wouldn't have passed it along to\nus. But we care only about the extension in the path, which is what\ndistinguishes each individual call to this function.\n\nSo we can just check that. I also think the logic may be clearer if we\nhandle each extension exhaustively, like:\n\n /* We know these are useless without the matching .pack */\n if (ends_with(path, \".bitmap\") || ends_with(path, \".idx\")) {\n string_list_append(&pack_garbage, path);\n\t return;\n }\n\n /*\n * A pack without other files cannot be used, but should be saved,\n * as this is a recoverable situation (we may even see it racily\n * as new packs come into existence).\n */\n if (ends_with(path, \".pack\"))\n\t return;\n\n /*\n * A .keep file is useless without the matching pack, but it\n * _could_ contain information generated by the user. Let's keep it.\n * In the future, we may expand this to look for obvious leftover\n * receive-pack locks and drop them.\n */\n if (ends_with(path, \".keep\"))\n return;\n\n /*\n * A totally unrelated garbage file should be kept, to err\n * on the conservative side.\n */\n if (seen_bits & PACKDIR_FILE_GARBAGE)\n\treturn;\n\n /*\n * We have a file type that the garbage-reporting functions\n * know about but we don't. This function needs updating.\n */\n die(\"BUG: report_pack_garbage confused\");\n\n> -test_expect_failure 'clean pack garbage with gc' '\n> +test_expect_success 'clean pack garbage with gc' '\n> \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n> \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n> \t: >.git/objects/pack/foo.keep &&\n\nAnd I think here we should make sure that we are covering the above\nsituations (and especially that we are keeping files that should be\nkept).\n\n-Peff\n"},{"id":"274740","messageId":"20151219020247.GA3098@sigill.intra.peff.net","threadId":"40797","inReplyTo":"1450483600-64091-1-git-send-email-dougk.ff7@gmail.com","subject":"Re: [PATCH v3 0/3] prepare_packed_git(): find more garbage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-19T02:02:47Z","receivedAt":"2015-12-19T02:02:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 18, 2015 at 06:06:37PM -0600, Doug Kelly wrote:\n\n> Corrects the issues found in review by Peff, including simplifying\n> the logic in report_helper(). bits_to_msg() would've been an alternate\n> solution to that change, however it'll get called by\n> real_report_garbage(), so there's no need to call it twice, especially\n> when the check we need within report_helper().\n\nOK. The new logic in 1/3 looks fine to me.\n\n> I think checking for seen_bits == 0 would be future-proofing should we\n> arrive at a file bit not otherwise match it (i.e. file.foo and\n> file.bar, but nothing else would cause seen_bits to be zero, but if\n> that's the case, we wouldn't have PACKDIR_FILE_IDX or\n> PACKDIR_FILE_PACK set, either, and the second half would also match.\n\nYeah, I think this is sound.\n\nI left a few comments on 3/3. I don't think it's _wrong_, but I think we\ncan be a bit more thorough (and IMHO, a little more maintainable, but\nothers might disagree).\n\n-Peff\n"},{"id":"274741","messageId":"20151219020324.GA3118@sigill.intra.peff.net","threadId":"40797","inReplyTo":"20151219020247.GA3098@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/3] prepare_packed_git(): find more garbage","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-12-19T02:03:25Z","receivedAt":"2015-12-19T02:03:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 18, 2015 at 09:02:47PM -0500, Jeff King wrote:\n\n> I left a few comments on 3/3. I don't think it's _wrong_, but I think we\n> can be a bit more thorough (and IMHO, a little more maintainable, but\n> others might disagree).\n\nOh, and I forgot to say thank you for working on this. :)\n\n-Peff\n"},{"id":"275657","messageId":"CAGZ79kZ=OSGVu5w7ZjZHhUKggSStp2ihV5iW2oawTYLG5htj7Q@mail.gmail.com","threadId":"40797","inReplyTo":"20151219020324.GA3118@sigill.intra.peff.net","subject":"Re: [PATCH v3 0/3] prepare_packed_git(): find more garbage","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-01-11T16:35:00Z","receivedAt":"2016-01-11T16:35:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 18, 2015 at 6:03 PM, Jeff King wrote:\n> On Fri, Dec 18, 2015 at 09:02:47PM -0500, Jeff King wrote:\n>\n>> I left a few comments on 3/3. I don't think it's _wrong_, but I think we\n>> can be a bit more thorough (and IMHO, a little more maintainable, but\n>> others might disagree).\n>\n> Oh, and I forgot to say thank you for working on this. :)\n\nThanks for working on this from me, too!\n[PATCH 1/3] prepare_packed_git(): find more garbage looks good to me.\n\n\n>\n> -Peff\n"},{"id":"275931","messageId":"cover.1452704305.git.dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"1450483600-64091-1-git-send-email-dougk.ff7@gmail.com","subject":"[PATCH v4 0/4] gc: Clean garbage .bitmap files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T17:07:08Z","receivedAt":"2016-01-13T17:07:08Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Updated version to prepare_packed_git based on additional feedback,\nincluding Peff's idea to explicitly call out each file we want to\nhandle in gc. Also adds a test case in t5304 to ensure files we\nexplicitly don't wnat to touch are not modified.\n\nDoug Kelly (4):\n prepare_packed_git(): find more garbage\n t5304: Add test for .bitmap garbage files\n t5304: Ensure wanted files are not deleted\n gc: Clean garbage .bitmap files from pack dir\n\n builtin/count-objects.c | 16 ++++++----------\n builtin/gc.c | 35 ++++++++++++++++++++++++++++++++++-\n cache.h | 4 +++-\n sha1_file.c | 12 +++++++++---\n t/t5304-prune.sh | 37 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 89 insertions(+), 15 deletions(-)\n\n-- \n2.6.1\n"},{"id":"275929","messageId":"bb5104d63e7095ae96fad8461bb6f904b800e168.1452704305.git.dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"cover.1452704305.git.dougk.ff7@gmail.com","subject":"[PATCH 1/4] prepare_packed_git(): find more garbage","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T17:07:09Z","receivedAt":"2016-01-13T17:07:09Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":".bitmap and .keep files without .idx/.pack don't make much sense, so\nmake sure these are reported as garbage as well. At the same time,\nrefactoring report_garbage to handle extra bits.\n\nSigned-off-by: Doug Kelly \n---\n builtin/count-objects.c | 16 ++++++----------\n cache.h | 4 +++-\n sha1_file.c | 12 +++++++++---\n t/t5304-prune.sh | 2 ++\n 4 files changed, 20 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..ed103ae 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -17,19 +17,15 @@ static off_t loose_size;\n \n static const char *bits_to_msg(unsigned seen_bits)\n {\n-\tswitch (seen_bits) {\n-\tcase 0:\n-\t\treturn \"no corresponding .idx or .pack\";\n-\tcase PACKDIR_FILE_GARBAGE:\n+\tif (seen_bits & PACKDIR_FILE_GARBAGE)\n \t\treturn \"garbage found\";\n-\tcase PACKDIR_FILE_PACK:\n+\telse if (seen_bits & PACKDIR_FILE_PACK && !(seen_bits & PACKDIR_FILE_IDX))\n \t\treturn \"no corresponding .idx\";\n-\tcase PACKDIR_FILE_IDX:\n+\telse if (seen_bits & PACKDIR_FILE_IDX && !(seen_bits & PACKDIR_FILE_PACK))\n \t\treturn \"no corresponding .pack\";\n-\tcase PACKDIR_FILE_PACK|PACKDIR_FILE_IDX:\n-\tdefault:\n-\t\treturn NULL;\n-\t}\n+\telse if (!(seen_bits & (PACKDIR_FILE_IDX|PACKDIR_FILE_PACK)))\n+\t\treturn \"no corresponding .idx or .pack\";\n+\treturn NULL;\n }\n \n static void real_report_garbage(unsigned seen_bits, const char *path)\ndiff --git a/cache.h b/cache.h\nindex dfc459c..aee1d51 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1317,7 +1317,9 @@ extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_\n /* A hook to report invalid files in pack directory */\n #define PACKDIR_FILE_PACK 1\n #define PACKDIR_FILE_IDX 2\n-#define PACKDIR_FILE_GARBAGE 4\n+#define PACKDIR_FILE_BITMAP 4\n+#define PACKDIR_FILE_KEEP 8\n+#define PACKDIR_FILE_GARBAGE 16\n extern void (*report_garbage)(unsigned seen_bits, const char *path);\n \n extern void prepare_packed_git(void);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 73ccd49..b21b2ba 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1223,7 +1223,9 @@ void (*report_garbage)(unsigned seen_bits, const char *path);\n static void report_helper(const struct string_list *list,\n \t\t\t int seen_bits, int first, int last)\n {\n-\tif (seen_bits == (PACKDIR_FILE_PACK|PACKDIR_FILE_IDX))\n+\tstatic const int pack_and_index = PACKDIR_FILE_PACK|PACKDIR_FILE_IDX;\n+\n+\tif ((seen_bits & pack_and_index) == pack_and_index)\n \t\treturn;\n \n \tfor (; first < last; first++)\n@@ -1257,9 +1259,13 @@ static void report_pack_garbage(struct string_list *list)\n \t\t\tfirst = i;\n \t\t}\n \t\tif (!strcmp(path + baselen, \"pack\"))\n-\t\t\tseen_bits |= 1;\n+\t\t\tseen_bits |= PACKDIR_FILE_PACK;\n \t\telse if (!strcmp(path + baselen, \"idx\"))\n-\t\t\tseen_bits |= 2;\n+\t\t\tseen_bits |= PACKDIR_FILE_IDX;\n+\t\telse if (!strcmp(path + baselen, \"bitmap\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_BITMAP;\n+\t\telse if (!strcmp(path + baselen, \"keep\"))\n+\t\t\tseen_bits |= PACKDIR_FILE_KEEP;\n \t}\n \treport_helper(list, seen_bits, first, list->nr);\n }\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex def203c..1ea8279 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -261,6 +261,8 @@ test_expect_success 'clean pack garbage with gc' '\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n+warning: no corresponding .pack: .git/objects/pack/fake2.idx\n+warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"275930","messageId":"543776a2a3e5d9f920280d06363a85f30a279d94.1452704305.git.dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"cover.1452704305.git.dougk.ff7@gmail.com","subject":"[PATCH 2/4] t5304: Add test for .bitmap garbage files","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T17:07:10Z","receivedAt":"2016-01-13T17:07:10Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"When checking for pack garbage, .bitmap files are now detected as\ngarbage when not associated with another .pack/.idx file.\n\nSigned-off-by: Doug Kelly \n---\n t/t5304-prune.sh | 24 +++++++++++++++++++++---\n 1 file changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 1ea8279..4fa6e7a 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -230,6 +230,12 @@ test_expect_success 'garbage report in count-objects -v' '\n \t: >.git/objects/pack/fake.idx &&\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake3.idx &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"index file .git/objects/pack/fake.idx is too small\" stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n@@ -238,14 +244,20 @@ warning: garbage found: .git/objects/pack/fake.bar\n warning: garbage found: .git/objects/pack/foo\n warning: garbage found: .git/objects/pack/foo.bar\n warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake4.bitmap\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n warning: no corresponding .pack: .git/objects/pack/fake3.idx\n+warning: no corresponding .pack: .git/objects/pack/fake5.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake5.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.bitmap\n+warning: no corresponding .pack: .git/objects/pack/fake6.idx\n+warning: no corresponding .pack: .git/objects/pack/fake6.keep\n EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_success 'clean pack garbage with gc' '\n+test_expect_failure 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n@@ -254,15 +266,21 @@ test_expect_success 'clean pack garbage with gc' '\n \t: >.git/objects/pack/fake2.keep &&\n \t: >.git/objects/pack/fake2.idx &&\n \t: >.git/objects/pack/fake3.keep &&\n+\t: >.git/objects/pack/fake4.bitmap &&\n+\t: >.git/objects/pack/fake5.bitmap &&\n+\t: >.git/objects/pack/fake5.idx &&\n+\t: >.git/objects/pack/fake6.keep &&\n+\t: >.git/objects/pack/fake6.bitmap &&\n+\t: >.git/objects/pack/fake6.idx &&\n \tgit gc &&\n \tgit count-objects -v 2>stderr &&\n \tgrep \"^warning:\" stderr | sort >actual &&\n \tcat >expected <<\\EOF &&\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake2.keep\n warning: no corresponding .idx or .pack: .git/objects/pack/fake3.keep\n+warning: no corresponding .idx or .pack: .git/objects/pack/fake6.keep\n warning: no corresponding .idx: .git/objects/pack/foo.keep\n warning: no corresponding .idx: .git/objects/pack/foo.pack\n-warning: no corresponding .pack: .git/objects/pack/fake2.idx\n-warning: no corresponding .pack: .git/objects/pack/fake2.keep\n EOF\n \ttest_cmp expected actual\n '\n-- \n2.6.1\n"},{"id":"275927","messageId":"670a9d9268beb0d70fb877a7c62d769062babba9.1452704305.git.dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"cover.1452704305.git.dougk.ff7@gmail.com","subject":"[PATCH 3/4] t5304: Ensure wanted files are not deleted","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T17:07:11Z","receivedAt":"2016-01-13T17:07:11Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Explicitly test for and ensure files that may be wanted are not\ndeleted during a gc operation. These include .pack without .idx\n(which may be in-flight), garbage in the directory, and .keep files\nthe user created.\n\nSigned-off-by: Doug Kelly \n---\n t/t5304-prune.sh | 17 +++++++++++++++++\n 1 file changed, 17 insertions(+)\n\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex 4fa6e7a..f7c380c 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -285,6 +285,23 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'ensure unknown garbage kept with gc' '\n+\ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n+\ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n+\t: >.git/objects/pack/foo.keep &&\n+\t: >.git/objects/pack/fake.pack &&\n+\t: >.git/objects/pack/fake2.foo &&\n+\tgit gc &&\n+\tgit count-objects -v 2>stderr &&\n+\tgrep \"^warning:\" stderr | sort >actual &&\n+\tcat >expected <<\\EOF &&\n+warning: garbage found: .git/objects/pack/fake2.foo\n+warning: no corresponding .idx or .pack: .git/objects/pack/foo.keep\n+warning: no corresponding .idx: .git/objects/pack/fake.pack\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_expect_success 'prune .git/shallow' '\n \tSHA1=`echo hi|git commit-tree HEAD^{tree}` &&\n \techo $SHA1 >.git/shallow &&\n-- \n2.6.1\n"},{"id":"275928","messageId":"8277e5f62991fce9524ea3020636fc8dafe926b4.1452704305.git.dougk.ff7@gmail.com","threadId":"40797","inReplyTo":"cover.1452704305.git.dougk.ff7@gmail.com","subject":"[PATCH 4/4] gc: Clean garbage .bitmap files from pack dir","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-13T17:07:12Z","receivedAt":"2016-01-13T17:07:12Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"Similar to cleaning up excess .idx files, clean any garbage .bitmap\nfiles that are not otherwise associated with any .idx/.pack files.\n\nSigned-off-by: Doug Kelly \nSuggested-by: Jeff King \n---\n builtin/gc.c | 35 ++++++++++++++++++++++++++++++++++-\n t/t5304-prune.sh | 2 +-\n 2 files changed, 35 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex c583aad..79e9886 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -58,8 +58,41 @@ static void clean_pack_garbage(void)\n \n static void report_pack_garbage(unsigned seen_bits, const char *path)\n {\n-\tif (seen_bits == PACKDIR_FILE_IDX)\n+\t/* We know these are useless without the matching .pack */\n+\tif (ends_with(path, \".bitmap\") || ends_with(path, \".idx\")) {\n \t\tstring_list_append(&pack_garbage, path);\n+\t\treturn;\n+\t}\n+\n+\t/*\n+\t * A pack without other files cannot be used, but should be saved,\n+\t * as this is a recoverable situation (we may even see it racily\n+\t * as new packs come into existence).\n+\t */\n+\tif (ends_with(path, \".pack\"))\n+\t\treturn;\n+\n+\t/*\n+\t * A .keep file is useless without the matching pack, but it\n+\t * _could_ contain information generated by the user. Let's keep it.\n+\t * In the future, we may expand this to look for obvious leftover\n+\t * receive-pack locks and drop them.\n+\t */\n+\tif (ends_with(path, \".keep\"))\n+\t\treturn;\n+\n+\t/*\n+\t * A totally unrelated garbage file should be kept, to err\n+\t * on the conservative side.\n+\t */\n+\tif (seen_bits & PACKDIR_FILE_GARBAGE)\n+\t\treturn;\n+\n+\t/*\n+\t * We have a file type that the garbage-reporting functions\n+\t * know about but we don't. This function needs updating.\n+\t */\n+\tdie(\"BUG: report_pack_garbage confused\");\n }\n \n static void git_config_date_string(const char *key, const char **output)\ndiff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\nindex f7c380c..cbcc0c0 100755\n--- a/t/t5304-prune.sh\n+++ b/t/t5304-prune.sh\n@@ -257,7 +257,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'clean pack garbage with gc' '\n+test_expect_success 'clean pack garbage with gc' '\n \ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n \ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n \t: >.git/objects/pack/foo.keep &&\n-- \n2.6.1\n"},{"id":"275973","messageId":"xmqqr3hlmcsr.fsf@gitster.mtv.corp.google.com","threadId":"40797","inReplyTo":"543776a2a3e5d9f920280d06363a85f30a279d94.1452704305.git.dougk.ff7@gmail.com","subject":"Re: [PATCH 2/4] t5304: Add test for .bitmap garbage files","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-13T20:42:28Z","receivedAt":"2016-01-13T20:42:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> When checking for pack garbage, .bitmap files are now detected as\n> garbage when not associated with another .pack/.idx file.\n\nProbably the above would read better with s/are now/should now be/,\nas the _expect_failure this step introduces will be corrected with \nPatch 4/4.\n\nAlso I'd suggest s/another/any/.\n"},{"id":"275974","messageId":"xmqqmvs9mc6h.fsf@gitster.mtv.corp.google.com","threadId":"40797","inReplyTo":"670a9d9268beb0d70fb877a7c62d769062babba9.1452704305.git.dougk.ff7@gmail.com","subject":"Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-13T20:55:50Z","receivedAt":"2016-01-13T20:55:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> Subject: Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted\n\nI'd suggest s/wanted/non-garbage/.\n\n> Explicitly test for and ensure files that may be wanted are not\n> deleted during a gc operation. These include .pack without .idx\n> (which may be in-flight), garbage in the directory, and .keep files\n> the user created.\n\n\"garbage in the directory\" is not well defined. \"files in the\ndirectory that clearly are not related to packing\" is probably what\nyou meant, but the definition of \"related to packing\" is still\nfuzzy. Please clarify.\n\nThe following is me thinking aloud about things that you would need\nto think about while attempting to clarify this.\n\nWhat should the code do if we find\n\n pack-b0a9d62a7471e58832a575a78d57f8fb26822125.frotz\n\nin $GIT_OBJECT_DIRECTORY/pack/ directory? Is it a \"garbage in the\ndirectory\"? The filename looks so similar to the usual things with\nknow suffixes .pack, .idx, .bitmap, and .keep, that we may want to\nconsider that it might be another file related to the packing left\nby a future version of Git and if we do not see corresponding .pack\nwe would want to remove it? Or do we want to do something else?\n\nWhat should \"gc\" do if we find\n\n pack-frotz.idx\n\nwithout corresponding \".pack\"? Wouldn't it be safer to consider it\na garbage unrelated to packing (because regular packing would have\ngiven it with 40-hex name, not \"frotz\") and leave it undeleted?\n\nThanks.\n\n> Signed-off-by: Doug Kelly \n> ---\n> t/t5304-prune.sh | 17 +++++++++++++++++\n> 1 file changed, 17 insertions(+)\n>\n> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n> index 4fa6e7a..f7c380c 100755\n> --- a/t/t5304-prune.sh\n> +++ b/t/t5304-prune.sh\n> @@ -285,6 +285,23 @@ EOF\n> \ttest_cmp expected actual\n> '\n> \n> +test_expect_success 'ensure unknown garbage kept with gc' '\n> +\ttest_when_finished \"rm -f .git/objects/pack/fake*\" &&\n> +\ttest_when_finished \"rm -f .git/objects/pack/foo*\" &&\n> +\t: >.git/objects/pack/foo.keep &&\n> +\t: >.git/objects/pack/fake.pack &&\n> +\t: >.git/objects/pack/fake2.foo &&\n> +\tgit gc &&\n> +\tgit count-objects -v 2>stderr &&\n> +\tgrep \"^warning:\" stderr | sort >actual &&\n> +\tcat >expected <<\\EOF &&\n> +warning: garbage found: .git/objects/pack/fake2.foo\n> +warning: no corresponding .idx or .pack: .git/objects/pack/foo.keep\n> +warning: no corresponding .idx: .git/objects/pack/fake.pack\n> +EOF\n> +\ttest_cmp expected actual\n> +'\n> +\n> test_expect_success 'prune .git/shallow' '\n> \tSHA1=`echo hi|git commit-tree HEAD^{tree}` &&\n> \techo $SHA1 >.git/shallow &&\n"},{"id":"276279","messageId":"CAEtYS8TfcJBbO_QJGH-Z9a3AHLdsO+H+k_fAS2EtJOH8bVwFEg@mail.gmail.com","threadId":"40797","inReplyTo":"xmqqmvs9mc6h.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted","fromName":"Doug Kelly","fromEmail":"dougk.ff7@gmail.com","sentAt":"2016-01-18T16:54:50Z","receivedAt":"2016-01-18T16:54:50Z","isPatch":true,"sender":{"key":"dougk.ff7@gmail.com","avatar":"https://avatars.githubusercontent.com/u/93357?v=4"},"body":"On Wed, Jan 13, 2016 at 2:55 PM, Junio C Hamano wrote:\n> Doug Kelly writes:\n>\n>> Subject: Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted\n>\n> I'd suggest s/wanted/non-garbage/.\n>\n\nI'm okay with this.\n\n>> Explicitly test for and ensure files that may be wanted are not\n>> deleted during a gc operation. These include .pack without .idx\n>> (which may be in-flight), garbage in the directory, and .keep files\n>> the user created.\n>\n> \"garbage in the directory\" is not well defined. \"files in the\n> directory that clearly are not related to packing\" is probably what\n> you meant, but the definition of \"related to packing\" is still\n> fuzzy. Please clarify.\n\nThis is probably a good point. Perhaps a better way to think about it\nwould be by rewording the paragraph to something like this:\n\nExplicitly test for and ensure files that may either be desired by the user\nor are possibly not garbage are not deleted during a gc operation.\nThese include .pack files missing a corresponding .idx file (possibly due\nto it being in-flight), .keep files created by the user, and other\nunknown garbage in the pack directory. These files will still be identified\nby \"git count-objects -v\", but should not be removed automatically by\ngc. Only files we are absolutely sure are unnecessary will be removed\nas a part of the gc process.\n\n>\n> The following is me thinking aloud about things that you would need\n> to think about while attempting to clarify this.\n>\n> What should the code do if we find\n>\n> pack-b0a9d62a7471e58832a575a78d57f8fb26822125.frotz\n>\n> in $GIT_OBJECT_DIRECTORY/pack/ directory? Is it a \"garbage in the\n> directory\"? The filename looks so similar to the usual things with\n> know suffixes .pack, .idx, .bitmap, and .keep, that we may want to\n> consider that it might be another file related to the packing left\n> by a future version of Git and if we do not see corresponding .pack\n> we would want to remove it? Or do we want to do something else?\n>\n> What should \"gc\" do if we find\n>\n> pack-frotz.idx\n>\n> without corresponding \".pack\"? Wouldn't it be safer to consider it\n> a garbage unrelated to packing (because regular packing would have\n> given it with 40-hex name, not \"frotz\") and leave it undeleted?\n>\n\nI think the above paragraph helps explain what we're doing and why.\nIn your examples, a somewhat valid looking pack file with an unknown\nextension may be flagged as \"garbage,\" but should not be deleted\nduring the gc. Similarly, we decided that an .idx file with no\ncorresponding .pack was safe to delete (since the pack is written before\nidx, and the initial performance problem was related to scanning a large\nnumber of idx files).\n\nI'm not saying there's nothing to be said for the difference in the base\nfilename without extension. Currently, the logic to remove pack garbage\ndoesn't look at that, though: it only considers the extension, and what\nrelated files are found in the directory. Whether this is good or bad, I'm\nnot sure. It certainly does what I need at fairly low risk, though.\n\nDoes this help clarify the situation more?\n\n> Thanks.\n>\n>> Signed-off-by: Doug Kelly \n>> ---\n>> t/t5304-prune.sh | 17 +++++++++++++++++\n>> 1 file changed, 17 insertions(+)\n>>\n>> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh\n>> index 4fa6e7a..f7c380c 100755\n>> --- a/t/t5304-prune.sh\n>> +++ b/t/t5304-prune.sh\n>> @@ -285,6 +285,23 @@ EOF\n>> test_cmp expected actual\n>> '\n>>\n>> +test_expect_success 'ensure unknown garbage kept with gc' '\n>> + test_when_finished \"rm -f .git/objects/pack/fake*\" &&\n>> + test_when_finished \"rm -f .git/objects/pack/foo*\" &&\n>> + : >.git/objects/pack/foo.keep &&\n>> + : >.git/objects/pack/fake.pack &&\n>> + : >.git/objects/pack/fake2.foo &&\n>> + git gc &&\n>> + git count-objects -v 2>stderr &&\n>> + grep \"^warning:\" stderr | sort >actual &&\n>> + cat >expected <<\\EOF &&\n>> +warning: garbage found: .git/objects/pack/fake2.foo\n>> +warning: no corresponding .idx or .pack: .git/objects/pack/foo.keep\n>> +warning: no corresponding .idx: .git/objects/pack/fake.pack\n>> +EOF\n>> + test_cmp expected actual\n>> +'\n>> +\n>> test_expect_success 'prune .git/shallow' '\n>> SHA1=`echo hi|git commit-tree HEAD^{tree}` &&\n>> echo $SHA1 >.git/shallow &&\n"},{"id":"276354","messageId":"xmqq60ypbeng.fsf@gitster.mtv.corp.google.com","threadId":"40797","inReplyTo":"CAEtYS8TfcJBbO_QJGH-Z9a3AHLdsO+H+k_fAS2EtJOH8bVwFEg@mail.gmail.com","subject":"Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T18:36:03Z","receivedAt":"2016-01-19T18:36:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Doug Kelly writes:\n\n> On Wed, Jan 13, 2016 at 2:55 PM, Junio C Hamano wrote:\n>> Doug Kelly writes:\n>>\n>>> Subject: Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted\n>>\n>> I'd suggest s/wanted/non-garbage/.\n>>\n>\n> I'm okay with this.\n>\n>>> Explicitly test for and ensure files that may be wanted are not\n>>> deleted during a gc operation. These include .pack without .idx\n>>> (which may be in-flight), garbage in the directory, and .keep files\n>>> the user created.\n>>\n>> \"garbage in the directory\" is not well defined. \"files in the\n>> directory that clearly are not related to packing\" is probably what\n>> you meant, but the definition of \"related to packing\" is still\n>> fuzzy. Please clarify.\n>\n> This is probably a good point. Perhaps a better way to think about it\n> would be by rewording the paragraph to something like this:\n>\n> Explicitly test for and ensure files that may either be desired by the user\n> or are possibly not garbage are not deleted during a gc operation.\n> These include .pack files missing a corresponding .idx file (possibly due\n> to it being in-flight), .keep files created by the user, and other\n> unknown garbage in the pack directory. These files will still be identified\n> by \"git count-objects -v\", but should not be removed automatically by\n> gc. Only files we are absolutely sure are unnecessary will be removed\n> as a part of the gc process.\n\nThat, especially \"other _unknown_ garbage\", looks much better than\nthe original.\n\n> I'm not saying there's nothing to be said for the difference in the base\n> filename without extension. Currently, the logic to remove pack garbage\n> doesn't look at that, though: it only considers the extension, and what\n> related files are found in the directory. Whether this is good or bad, I'm\n> not sure. It certainly does what I need at fairly low risk, though.\n>\n> Does this help clarify the situation more?\n\nI was shooting for making you _think_ exactly about what you wrote\nin the above paragraph, i.e. what the current logic does and if it\nis sensible, is overly pessimistic for some files, and/or is risky\nfor some other files and if so in what way, as that would help you\nrecord the thinking behind the different treatment for files based\non various file extentions clearly in the log message.\n\nThanks.\n"}]}