{"thread":{"id":"38870","subject":"[PATCH v2 5/5] refs.c: drop curate_packed_refs","startedAt":"2015-03-20T18:42:16Z","lastAt":"2015-03-20T18:43:17Z","messageCount":6,"participants":["Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"258159","messageId":"20150320184215.GA26368@peff.net","threadId":"38870","inReplyTo":"20150317072750.GA22155@peff.net","subject":"[PATCH v2 0/5] not making corruption worse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T18:42:16Z","receivedAt":"2015-03-20T18:42:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a re-roll of the series to make \"git prune\" in a corrupted\nrepository safer.\n\nThere are only minor tweaks from v1, but I think all of the raised\nissues were addressed (there was discussion on some other points, but I\nthink they are OK as-is; more discussion is of course welcome).\n\nThe changes from v1 are:\n\n  - use \"bogus..name\" as a bad refname instead of \"bogus:name\", as the\n    latter has problems on Windows\n\n  - fix broken &&-chains in tests\n\n  - better commenting of missing-commit setup in t5312\n\n  - typo-fixes in commit messages\n\nPatches:\n\n  [1/5]: t5312: test object deletion code paths in a corrupted repository\n  [2/5]: refs: introduce a \"ref paranoia\" flag\n  [3/5]: prune: turn on ref_paranoia flag\n  [4/5]: repack: turn on \"ref paranoia\" when doing a destructive repack\n  [5/5]: refs.c: drop curate_packed_refs\n\n-Peff\n"},{"id":"258158","messageId":"20150320184302.GA23849@peff.net","threadId":"38870","inReplyTo":"20150320184215.GA26368@peff.net","subject":"[PATCH v2 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T18:43:02Z","receivedAt":"2015-03-20T18:43:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we are doing a destructive operation like \"git prune\",\nwe want to be extra careful that the set of reachable tips\nwe compute is valid. If there is any corruption or oddity,\nwe are better off aborting the operation and letting the\nuser figure things out rather than plowing ahead and\npossibly deleting some data that cannot be recovered.\n\nThe tests here include:\n\n  1. Pruning objects mentioned only be refs with invalid\n     names. This used to abort prior to d0f810f (refs.c:\n     allow listing and deleting badly named refs,\n     2014-09-03), but since then we silently ignore the tip.\n\n     Likewise, we test repacking that can drop objects\n     (either \"-ad\", which drops anything unreachable,\n     or \"-Ad --unpack-unreachable=<time>\", which tries to\n     optimize out a loose object write that would be\n     directly pruned).\n\n  2. Pruning objects when some refs point to missing\n     objects. We don't know whether any dangling objects\n     would have been reachable from the missing objects. We\n     are better to keep them around, as they are better than\n     nothing for helping the user recover history.\n\n  3. Packed refs that point to missing objects can sometimes\n     be dropped. By itself, this is more of an annoyance\n     (you do not have the object anyway; even if you can\n     recover it from elsewhere, all you are losing is a\n     placeholder for your state at the time of corruption).\n     But coupled with (2), if we drop the ref and then go\n     on to prune, we may lose unrecoverable objects.\n\nNote that we use test_might_fail for some of the operations.\nIn some cases, it would be appropriate to abort the\noperation, and in others, it might be acceptable to continue\nbut taking the information into account. The tests don't\ncare either way, and check only for data loss.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5312-prune-corruption.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 114 insertions(+)\n create mode 100755 t/t5312-prune-corruption.sh\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nnew file mode 100755\nindex 0000000..496a9f5\n--- /dev/null\n+++ b/t/t5312-prune-corruption.sh\n@@ -0,0 +1,114 @@\n+#!/bin/sh\n+\n+test_description='\n+Test pruning of repositories with minor corruptions. The goal\n+here is that we should always be erring on the side of safety. So\n+if we see, for example, a ref with a bogus name, it is OK either to\n+bail out or to proceed using it as a reachable tip, but it is _not_\n+OK to proceed as if it did not exist. Otherwise we might silently\n+delete objects that cannot be recovered.\n+'\n+. ./test-lib.sh\n+\n+test_expect_success 'disable reflogs' '\n+\tgit config core.logallrefupdates false &&\n+\trm -rf .git/logs\n+'\n+\n+test_expect_success 'create history reachable only from a bogus-named ref' '\n+\ttest_tick && git commit --allow-empty -m master &&\n+\tbase=$(git rev-parse HEAD) &&\n+\ttest_tick && git commit --allow-empty -m bogus &&\n+\tbogus=$(git rev-parse HEAD) &&\n+\tgit cat-file commit $bogus >saved &&\n+\techo $bogus >.git/refs/heads/bogus..name &&\n+\tgit reset --hard HEAD^\n+'\n+\n+test_expect_failure 'pruning does not drop bogus object' '\n+\ttest_when_finished \"git hash-object -w -t commit saved\" &&\n+\ttest_might_fail git prune --expire=now &&\n+\tverbose git cat-file -e $bogus\n+'\n+\n+test_expect_success 'put bogus object into pack' '\n+\tgit tag reachable $bogus &&\n+\tgit repack -ad &&\n+\tgit tag -d reachable &&\n+\tverbose git cat-file -e $bogus\n+'\n+\n+test_expect_failure 'destructive repack keeps packed object' '\n+\ttest_might_fail git repack -Ad --unpack-unreachable=now &&\n+\tverbose git cat-file -e $bogus &&\n+\ttest_might_fail git repack -ad &&\n+\tverbose git cat-file -e $bogus\n+'\n+\n+# subsequent tests will have different corruptions\n+test_expect_success 'clean up bogus ref' '\n+\trm .git/refs/heads/bogus..name\n+'\n+\n+# We create two new objects here, \"one\" and \"two\". Our\n+# master branch points to \"two\", which is deleted,\n+# corrupting the repository. But we'd like to make sure\n+# that the otherwise unreachable \"one\" is not pruned\n+# (since it is the user's best bet for recovering\n+# from the corruption).\n+#\n+# Note that we also point HEAD somewhere besides \"two\",\n+# as we want to make sure we test the case where we\n+# pick up the reference to \"two\" by iterating the refs,\n+# not by resolving HEAD.\n+test_expect_success 'create history with missing tip commit' '\n+\ttest_tick && git commit --allow-empty -m one &&\n+\trecoverable=$(git rev-parse HEAD) &&\n+\tgit cat-file commit $recoverable >saved &&\n+\ttest_tick && git commit --allow-empty -m two &&\n+\tmissing=$(git rev-parse HEAD) &&\n+\tgit checkout --detach $base &&\n+\trm .git/objects/$(echo $missing | sed \"s,..,&/,\") &&\n+\ttest_must_fail git cat-file -e $missing\n+'\n+\n+test_expect_failure 'pruning with a corrupted tip does not drop history' '\n+\ttest_when_finished \"git hash-object -w -t commit saved\" &&\n+\ttest_might_fail git prune --expire=now &&\n+\tverbose git cat-file -e $recoverable\n+'\n+\n+test_expect_success 'pack-refs does not silently delete broken loose ref' '\n+\tgit pack-refs --all --prune &&\n+\techo $missing >expect &&\n+\tgit rev-parse refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+# we do not want to count on running pack-refs to\n+# actually pack it, as it is perfectly reasonable to\n+# skip processing a broken ref\n+test_expect_success 'create packed-refs file with broken ref' '\n+\trm -f .git/refs/heads/master &&\n+\tcat >.git/packed-refs <<-EOF &&\n+\t$missing refs/heads/master\n+\t$recoverable refs/heads/other\n+\tEOF\n+\techo $missing >expect &&\n+\tgit rev-parse refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'pack-refs does not silently delete broken packed ref' '\n+\tgit pack-refs --all --prune &&\n+\tgit rev-parse refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'pack-refs does not drop broken refs during deletion' '\n+\tgit update-ref -d refs/heads/other &&\n+\tgit rev-parse refs/heads/master >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258157","messageId":"20150320184305.GB23849@peff.net","threadId":"38870","inReplyTo":"20150320184215.GA26368@peff.net","subject":"[PATCH v2 2/5] refs: introduce a \"ref paranoia\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T18:43:06Z","receivedAt":"2015-03-20T18:43:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Most operations that iterate over refs are happy to ignore\nbroken cruft. However, some operations should be performed\nwith knowledge of these broken refs, because it is better\nfor the operation to choke on a missing object than it is to\nsilently pretend that the ref did not exist (e.g., if we are\ncomputing the set of reachable tips in order to prune\nobjects).\n\nThese processes could just call for_each_rawref, except that\nref iteration is often hidden behind other interfaces. For\ninstance, for a destructive \"repack -ad\", we would have to\ninform \"pack-objects\" that we are destructive, and then it\nwould in turn have to tell the revision code that our\n\"--all\" should include broken refs.\n\nIt's much simpler to just set a global for \"dangerous\"\noperations that includes broken refs in all iterations.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Documentation/git.txt | 11 +++++++++++\n cache.h               |  8 ++++++++\n environment.c         |  1 +\n refs.c                |  5 +++++\n 4 files changed, 25 insertions(+)\n\ndiff --git a/Documentation/git.txt b/Documentation/git.txt\nindex af30620..8da85a6 100644\n--- a/Documentation/git.txt\n+++ b/Documentation/git.txt\n@@ -1026,6 +1026,17 @@ GIT_ICASE_PATHSPECS::\n \tvariable when it is invoked as the top level command by the\n \tend user, to be recorded in the body of the reflog.\n \n+`GIT_REF_PARANOIA`::\n+\tIf set to `1`, include broken or badly named refs when iterating\n+\tover lists of refs. In a normal, non-corrupted repository, this\n+\tdoes nothing. However, enabling it may help git to detect and\n+\tabort some operations in the presence of broken refs. Git sets\n+\tthis variable automatically when performing destructive\n+\toperations like linkgit:git-prune[1]. You should not need to set\n+\tit yourself unless you want to be paranoid about making sure\n+\tan operation has touched every ref (e.g., because you are\n+\tcloning a repository to make a backup).\n+\n \n Discussion[[Discussion]]\n ------------------------\ndiff --git a/cache.h b/cache.h\nindex 761c570..162ea6f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -614,6 +614,14 @@ extern int protect_hfs;\n extern int protect_ntfs;\n \n /*\n+ * Include broken refs in all ref iterations, which will\n+ * generally choke dangerous operations rather than letting\n+ * them silently proceed without taking the broken ref into\n+ * account.\n+ */\n+extern int ref_paranoia;\n+\n+/*\n  * The character that begins a commented line in user-editable file\n  * that is subject to stripspace.\n  */\ndiff --git a/environment.c b/environment.c\nindex 1ade5c9..a40044c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -24,6 +24,7 @@ int is_bare_repository_cfg = -1; /* unspecified */\n int log_all_ref_updates = -1; /* unspecified */\n int warn_ambiguous_refs = 1;\n int warn_on_object_refname_ambiguity = 1;\n+int ref_paranoia = -1;\n int repository_format_version;\n const char *git_commit_encoding;\n const char *git_log_output_encoding;\ndiff --git a/refs.c b/refs.c\nindex e23542b..7f0e7be 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1934,6 +1934,11 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,\n \tdata.fn = fn;\n \tdata.cb_data = cb_data;\n \n+\tif (ref_paranoia < 0)\n+\t\tref_paranoia = git_env_bool(\"GIT_REF_PARANOIA\", 0);\n+\tif (ref_paranoia)\n+\t\tdata.flags |= DO_FOR_EACH_INCLUDE_BROKEN;\n+\n \treturn do_for_each_entry(refs, base, do_one_ref, &data);\n }\n \n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258156","messageId":"20150320184309.GC23849@peff.net","threadId":"38870","inReplyTo":"20150320184215.GA26368@peff.net","subject":"[PATCH v2 3/5] prune: turn on ref_paranoia flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T18:43:09Z","receivedAt":"2015-03-20T18:43:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prune should know about broken objects at the tips of refs,\nso that we can feed them to our traversal rather than\nignoring them. It's better for us to abort the operation on\nthe broken object than it is to start deleting objects with\nan incomplete view of the reachability namespace.\n\nNote that for missing objects, aborting is the best we can\ndo. For a badly-named ref, we technically could use its sha1\nas a reachability tip. However, the iteration code just\nfeeds us a null sha1, so there would be a reasonable amount\nof code involved to pass down our wishes. It's not really\nworth trying to do better, because this is a case that\nshould happen extremely rarely, and the message we provide:\n\n  fatal: unable to parse object: refs/heads/bogus:name\n\nis probably enough to point the user in the right direction.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/prune.c             | 1 +\n t/t5312-prune-corruption.sh | 4 ++--\n 2 files changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/prune.c b/builtin/prune.c\nindex 04d3b12..17094ad 100644\n--- a/builtin/prune.c\n+++ b/builtin/prune.c\n@@ -115,6 +115,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)\n \texpire = ULONG_MAX;\n \tsave_commit_buffer = 0;\n \tcheck_replace_refs = 0;\n+\tref_paranoia = 1;\n \tinit_revisions(&revs, prefix);\n \n \targc = parse_options(argc, argv, prefix, options, prune_usage, 0);\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 496a9f5..5ffb817 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -25,7 +25,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' '\n \tgit reset --hard HEAD^\n '\n \n-test_expect_failure 'pruning does not drop bogus object' '\n+test_expect_success 'pruning does not drop bogus object' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n \ttest_might_fail git prune --expire=now &&\n \tverbose git cat-file -e $bogus\n@@ -72,7 +72,7 @@ test_expect_success 'create history with missing tip commit' '\n \ttest_must_fail git cat-file -e $missing\n '\n \n-test_expect_failure 'pruning with a corrupted tip does not drop history' '\n+test_expect_success 'pruning with a corrupted tip does not drop history' '\n \ttest_when_finished \"git hash-object -w -t commit saved\" &&\n \ttest_might_fail git prune --expire=now &&\n \tverbose git cat-file -e $recoverable\n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258155","messageId":"20150320184313.GD23849@peff.net","threadId":"38870","inReplyTo":"20150320184215.GA26368@peff.net","subject":"[PATCH v2 4/5] repack: turn on \"ref paranoia\" when doing a destructive repack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T18:43:13Z","receivedAt":"2015-03-20T18:43:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we are repacking with \"-ad\", we will drop any unreachable\nobjects. Likewise, using \"-Ad --unpack-unreachable=<time>\"\nwill drop any old, unreachable objects. In these cases, we\nwant to make sure the reachability we compute with \"--all\"\nis complete. We can do this by passing GIT_REF_PARANOIA=1 in\nthe environment to pack-objects.\n\nNote that \"-Ad\" is safe already, because it only loosens\nunreachable objects. It is up to \"git prune\" to avoid\ndeleting them.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/repack.c            | 8 ++++++--\n t/t5312-prune-corruption.sh | 2 +-\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/repack.c b/builtin/repack.c\nindex 28fbc70..f2edeb0 100644\n--- a/builtin/repack.c\n+++ b/builtin/repack.c\n@@ -228,13 +228,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)\n \t\tget_non_kept_pack_filenames(&existing_packs);\n \n \t\tif (existing_packs.nr && delete_redundant) {\n-\t\t\tif (unpack_unreachable)\n+\t\t\tif (unpack_unreachable) {\n \t\t\t\targv_array_pushf(&cmd.args,\n \t\t\t\t\t\t\"--unpack-unreachable=%s\",\n \t\t\t\t\t\tunpack_unreachable);\n-\t\t\telse if (pack_everything & LOOSEN_UNREACHABLE)\n+\t\t\t\targv_array_push(&cmd.env_array, \"GIT_REF_PARANOIA=1\");\n+\t\t\t} else if (pack_everything & LOOSEN_UNREACHABLE) {\n \t\t\t\targv_array_push(&cmd.args,\n \t\t\t\t\t\t\"--unpack-unreachable\");\n+\t\t\t} else {\n+\t\t\t\targv_array_push(&cmd.env_array, \"GIT_REF_PARANOIA=1\");\n+\t\t\t}\n \t\t}\n \t} else {\n \t\targv_array_push(&cmd.args, \"--unpacked\");\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 5ffb817..e8d04ef 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -38,7 +38,7 @@ test_expect_success 'put bogus object into pack' '\n \tverbose git cat-file -e $bogus\n '\n \n-test_expect_failure 'destructive repack keeps packed object' '\n+test_expect_success 'destructive repack keeps packed object' '\n \ttest_might_fail git repack -Ad --unpack-unreachable=now &&\n \tverbose git cat-file -e $bogus &&\n \ttest_might_fail git repack -ad &&\n-- \n2.3.3.520.g3cfbb5d\n"},{"id":"258154","messageId":"20150320184317.GE23849@peff.net","threadId":"38870","inReplyTo":"20150320184215.GA26368@peff.net","subject":"[PATCH v2 5/5] refs.c: drop curate_packed_refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T18:43:17Z","receivedAt":"2015-03-20T18:43:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we delete a ref, we have to rewrite the entire\npacked-refs file. We take this opportunity to \"curate\" the\npacked-refs file and drop any entries that are crufty or\nbroken.\n\nDropping broken entries (e.g., with bogus names, or ones\nthat point to missing objects) is actively a bad idea, as it\nmeans that we lose any notion that the data was there in the\nfirst place. Aside from the general hackiness that we might\nlose any information about ref \"foo\" while deleting an\nunrelated ref \"bar\", this may seriously hamper any attempts\nby the user at recovering from the corruption in \"foo\".\n\nThey will lose the sha1 and name of \"foo\"; the exact pointer\nmay still be useful even if they recover missing objects\nfrom a different copy of the repository. But worse, once the\nref is gone, there is no trace of the corruption. A\nfollow-up \"git prune\" may delete objects, even though it\nwould otherwise bail when seeing corruption.\n\nWe could just drop the \"broken\" bits from\ncurate_packed_refs, and continue to drop the \"crufty\" bits:\nrefs whose loose counterpart exists in the filesystem. This\nis not wrong to do, and it does have the advantage that we\nmay write out a slightly smaller packed-refs file. But it\nhas two disadvantages:\n\n  1. It is a potential source of races or mistakes with\n     respect to these refs that are otherwise unrelated to\n     the operation. To my knowledge, there aren't any active\n     problems in this area, but it seems like an unnecessary\n     risk.\n\n  2. We have to spend time looking up the matching loose\n     refs for every item in the packed-refs file. If you\n     have a large number of packed refs that do not change,\n     that outweighs the benefit from writing out a smaller\n     packed-refs file (it doesn't get smaller, and you do a\n     bunch of directory traversal to find that out).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c                      | 67 +--------------------------------------------\n t/t5312-prune-corruption.sh |  2 +-\n 2 files changed, 2 insertions(+), 67 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 7f0e7be..47e4e53 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2621,68 +2621,10 @@ int pack_refs(unsigned int flags)\n \treturn 0;\n }\n \n-/*\n- * If entry is no longer needed in packed-refs, add it to the string\n- * list pointed to by cb_data.  Reasons for deleting entries:\n- *\n- * - Entry is broken.\n- * - Entry is overridden by a loose ref.\n- * - Entry does not point at a valid object.\n- *\n- * In the first and third cases, also emit an error message because these\n- * are indications of repository corruption.\n- */\n-static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)\n-{\n-\tstruct string_list *refs_to_delete = cb_data;\n-\n-\tif (entry->flag & REF_ISBROKEN) {\n-\t\t/* This shouldn't happen to packed refs. */\n-\t\terror(\"%s is broken!\", entry->name);\n-\t\tstring_list_append(refs_to_delete, entry->name);\n-\t\treturn 0;\n-\t}\n-\tif (!has_sha1_file(entry->u.value.sha1)) {\n-\t\tunsigned char sha1[20];\n-\t\tint flags;\n-\n-\t\tif (read_ref_full(entry->name, 0, sha1, &flags))\n-\t\t\t/* We should at least have found the packed ref. */\n-\t\t\tdie(\"Internal error\");\n-\t\tif ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) {\n-\t\t\t/*\n-\t\t\t * This packed reference is overridden by a\n-\t\t\t * loose reference, so it is OK that its value\n-\t\t\t * is no longer valid; for example, it might\n-\t\t\t * refer to an object that has been garbage\n-\t\t\t * collected.  For this purpose we don't even\n-\t\t\t * care whether the loose reference itself is\n-\t\t\t * invalid, broken, symbolic, etc.  Silently\n-\t\t\t * remove the packed reference.\n-\t\t\t */\n-\t\t\tstring_list_append(refs_to_delete, entry->name);\n-\t\t\treturn 0;\n-\t\t}\n-\t\t/*\n-\t\t * There is no overriding loose reference, so the fact\n-\t\t * that this reference doesn't refer to a valid object\n-\t\t * indicates some kind of repository corruption.\n-\t\t * Report the problem, then omit the reference from\n-\t\t * the output.\n-\t\t */\n-\t\terror(\"%s does not point to a valid object!\", entry->name);\n-\t\tstring_list_append(refs_to_delete, entry->name);\n-\t\treturn 0;\n-\t}\n-\n-\treturn 0;\n-}\n-\n int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n {\n \tstruct ref_dir *packed;\n-\tstruct string_list refs_to_delete = STRING_LIST_INIT_DUP;\n-\tstruct string_list_item *refname, *ref_to_delete;\n+\tstruct string_list_item *refname;\n \tint ret, needs_repacking = 0, removed = 0;\n \n \tassert(err);\n@@ -2718,13 +2660,6 @@ int repack_without_refs(struct string_list *refnames, struct strbuf *err)\n \t\treturn 0;\n \t}\n \n-\t/* Remove any other accumulated cruft */\n-\tdo_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);\n-\tfor_each_string_list_item(ref_to_delete, &refs_to_delete) {\n-\t\tif (remove_entry(packed, ref_to_delete->string) == -1)\n-\t\t\tdie(\"internal error\");\n-\t}\n-\n \t/* Write what remains */\n \tret = commit_packed_refs();\n \tif (ret)\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex e8d04ef..8e98b44 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -105,7 +105,7 @@ test_expect_success 'pack-refs does not silently delete broken packed ref' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'pack-refs does not drop broken refs during deletion' '\n+test_expect_success 'pack-refs does not drop broken refs during deletion' '\n \tgit update-ref -d refs/heads/other &&\n \tgit rev-parse refs/heads/master >actual &&\n \ttest_cmp expect actual\n-- \n2.3.3.520.g3cfbb5d\n"}]}