{"thread":{"id":"38829","subject":"[PATCH 0/5] not making corruption worse","startedAt":"2015-03-17T07:27:51Z","lastAt":"2015-03-20T17:59:45Z","messageCount":35,"participants":["Jeff King","Johannes Sixt","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"257837","messageId":"20150317072750.GA22155@peff.net","threadId":"38829","inReplyTo":null,"subject":"[PATCH 0/5] not making corruption worse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:27:51Z","receivedAt":"2015-03-17T07:27:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This is a grab bag of fixes related to performing destructive operations\nin a repository with minor corruption. Of course we hope never to see\ncorruption in the first place, but I think if we do see it, we should\nerr on the side of not making things worse. IOW, it is better to abort\nand say \"fix this before pruning\" than it is to just start deleting\nobjects.\n\nThe issue that spurred this is that I noticed recent versions of git\nwill omit funny-named refs from iteration. This comes from d0f810f\n(refs.c: allow listing and deleting badly named refs, 2014-09-03), in\nv2.2.0.  That's probably a good idea in general, but for things like\n\"prune\" we need to be more careful (and we were, prior to that commit).\n\nSimilarly, if you have a ref whose tip object is missing, we tend to\njust ignore it in our traversals, and it presents a similar problem for\npruning. I didn't trace it back, but I think this problem is much older.\n\nThe general strategy for these is to use for_each_rawref traversals in\nthese situations. That doesn't cover _every_ possible scenario. For\nexample, you could do:\n\n  git clone --no-local repo.git backup.git &&\n  rm -rf repo.git\n\nand you might be disappointed if \"backup.git\" omitted some broken refs\n(upload-pack will simply skip the broken refs in its advertisement).  We\ncould tighten this, but then it becomes hard to access slightly broken\nrepositories (e.g., you might prefer to clone what you can, and not have\ngit die() when it tries to serve the breakage). Patch 2 provides a\ntweakable safety valve for this.\n\nAnd not strictly related to the above, but in the same ballpark, is the\nissue with packed-refs that I noted here:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/256051/focus=256896\n\nwhere we can drop broken refs from the packed-refs file during the\ndeletion of an unrelated ref.\n\nThe patches are:\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":"257838","messageId":"20150317072844.GA25191@peff.net","threadId":"38829","inReplyTo":"20150317072750.GA22155@peff.net","subject":"[PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:28:45Z","receivedAt":"2015-03-17T07:28:45Z","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 | 104 ++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 104 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..167031e\n--- /dev/null\n+++ b/t/t5312-prune-corruption.sh\n@@ -0,0 +1,104 @@\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+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+\t# point HEAD elsewhere\n+\tgit checkout $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":"257839","messageId":"20150317072957.GB25191@peff.net","threadId":"38829","inReplyTo":"20150317072750.GA22155@peff.net","subject":"[PATCH 2/5] refs: introduce a \"ref paranoia\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:29:57Z","receivedAt":"2015-03-17T07:29:57Z","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---\nI waffled on documenting this for end users. I'm not sure if people\nwould ever want to actually use the environment variable themselves. The\nbest hypothetical I could come up with was the:\n\n  git clone --no-local repo.git backup.git &&\n  rm -rf repo.git\n\nscenario.\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":"257840","messageId":"20150317073052.GC25191@peff.net","threadId":"38829","inReplyTo":"20150317072750.GA22155@peff.net","subject":"[PATCH 3/5] prune: turn on ref_paranoia flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:30:52Z","receivedAt":"2015-03-17T07:30:52Z","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---\nNote that we should already be aborting for non-tip objects. I guess we\ncould test that explicitly, too, but I didn't here.\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 167031e..cccab58 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@@ -62,7 +62,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":"257841","messageId":"20150317073102.GD25191@peff.net","threadId":"38829","inReplyTo":"20150317072750.GA22155@peff.net","subject":"[PATCH 4/5] repack: turn on \"ref paranoia\" when doing a destructive repack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:31:02Z","receivedAt":"2015-03-17T07:31:02Z","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 cccab58..e3e9994 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":"257842","messageId":"20150317073157.GE25191@peff.net","threadId":"38829","inReplyTo":"20150317072750.GA22155@peff.net","subject":"[PATCH 5/5] refs.c: drop curate_packed_refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:31:57Z","receivedAt":"2015-03-17T07:31:57Z","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 outweights 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---\nI'll admit my argument against curate_packed_refs is a bit hand-wavy. I\nwon't be _too_ sad if somebody insists on cutting this back to just\nkeeping \"broken\" refs around, and still curating the \"crufty\" ones.\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 e3e9994..8b54d16 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -95,7 +95,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"},{"id":"257843","messageId":"20150317073730.GA25267@peff.net","threadId":"38829","inReplyTo":"20150317072750.GA22155@peff.net","subject":"Re: [PATCH 0/5] not making corruption worse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T07:37:31Z","receivedAt":"2015-03-17T07:37:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 17, 2015 at 03:27:50AM -0400, Jeff King wrote:\n\n> The general strategy for these is to use for_each_rawref traversals in\n> these situations. That doesn't cover _every_ possible scenario. For\n> example, you could do:\n> \n>   git clone --no-local repo.git backup.git &&\n>   rm -rf repo.git\n> \n> and you might be disappointed if \"backup.git\" omitted some broken refs\n> (upload-pack will simply skip the broken refs in its advertisement).  We\n> could tighten this, but then it becomes hard to access slightly broken\n> repositories (e.g., you might prefer to clone what you can, and not have\n> git die() when it tries to serve the breakage). Patch 2 provides a\n> tweakable safety valve for this.\n\nOne thing I thought about while working on this was whether we should\njust make _all_ ref iterations for_each_rawref. The benefit to not doing\nso in the hypothetical above is that you might be able to clone \"foo\"\neven if \"bar\" is broken.\n\nBut it strikes me as weird that we consider the _tips_ of history to be\nspecial for ignoring breakage. If the tip of \"bar\" is broken, we omit\nit. But if the tip is fine, and there's breakage three commits down in\nthe history, then doing a clone is going to fail horribly, as\npack-objects realizes it can't generate the pack. So in practice, I'm\nnot sure how much you're buying with the \"don't mention broken refs\"\ncode.\n\nOTOH, there are probably _some_ situations that can be recovered with\nthe current code that could not otherwise. For example, in the current\ncode, I can still fetch \"foo\" even if \"bar\" is broken 3 commits down.\nWhereas if the tip is broken, there's a reasonable chance that\n\"upload-pack\" would just barf and I could fetch nothing.\n\nSo I stuck to the status quo in most cases, and only turned on the more\naggressive behavior for destructive operations (and people who want to\ngo wild can set GIT_REF_PARANOIA=1 for their every day operations if\nthey want to).\n\n-Peff\n"},{"id":"257875","messageId":"5508739A.5030608@kdbg.org","threadId":"38829","inReplyTo":"20150317072844.GA25191@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-03-17T18:34:02Z","receivedAt":"2015-03-17T18:34:02Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.03.2015 um 08:28 schrieb Jeff King:\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\nThis causes headaches on Windows: It creates an empty file, named \n\"bogus\", with all the data diverted to the alternate data stream named \n\"name\". Needless to say that this...\n\n> +test_expect_success 'clean up bogus ref' '\n> +\trm .git/refs/heads/bogus:name\n> +'\n\ndoes not remove the file \"bogus\", but only the alternate data stream (if \nat all---I forgot to check). How about .git/refs/heads/bogus..nam.e?\n\n-- Hannes\n"},{"id":"257878","messageId":"20150317185550.GA14550@peff.net","threadId":"38829","inReplyTo":"5508739A.5030608@kdbg.org","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-17T18:55:50Z","receivedAt":"2015-03-17T18:55:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 17, 2015 at 07:34:02PM +0100, Johannes Sixt wrote:\n\n> Am 17.03.2015 um 08:28 schrieb Jeff King:\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> \n> This causes headaches on Windows: It creates an empty file, named \"bogus\",\n> with all the data diverted to the alternate data stream named \"name\".\n> Needless to say that this...\n\nAh, yes. Windows. Our usual workaround would be to put it straight into\npacked-refs, but in this case, the test really does need the badly named\nref in the file system. But...\n\n> >+test_expect_success 'clean up bogus ref' '\n> >+\trm .git/refs/heads/bogus:name\n> >+'\n> \n> does not remove the file \"bogus\", but only the alternate data stream (if at\n> all---I forgot to check). How about .git/refs/heads/bogus..nam.e?\n\nYes, that works. The colon is what originally brought my attention to\nthis case, but anything that fails git-check-ref-format is fine. I've\nsquashed this in:\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 167031e..1001a69 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -21,7 +21,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' '\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+\techo $bogus >.git/refs/heads/bogus..name &&\n \tgit reset --hard HEAD^\n '\n \n@@ -47,7 +47,7 @@ test_expect_failure 'destructive repack keeps packed object' '\n \n # subsequent tests will have different corruptions\n test_expect_success 'clean up bogus ref' '\n-\trm .git/refs/heads/bogus:name\n+\trm .git/refs/heads/bogus..name\n '\n \n test_expect_success 'create history with missing tip commit' '\n\n\nI assumed the final \".\" in your example wasn't significant (it is not to\ngit), but let me know if I've run afoul of another weird restriction. :)\n\nThanks.\n\n-Peff\n"},{"id":"257898","messageId":"xmqqh9tjme0l.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150317073730.GA25267@peff.net","subject":"Re: [PATCH 0/5] not making corruption worse","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-17T22:54:02Z","receivedAt":"2015-03-17T22:54:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> But it strikes me as weird that we consider the _tips_ of history to be\n> special for ignoring breakage. If the tip of \"bar\" is broken, we omit\n> it. But if the tip is fine, and there's breakage three commits down in\n> the history, then doing a clone is going to fail horribly, as\n> pack-objects realizes it can't generate the pack. So in practice, I'm\n> not sure how much you're buying with the \"don't mention broken refs\"\n> code.\n\nI think this is a trade-off between strictness and convenience.  Is\nit preferrable that every time you try to clone a repository you get\nreminded that one of its refs point at a bogus object and you\ninstead have to do \"git fetch $there\" with a refspec that excludes\nthe broken one, or is it OK to allow clones and fetches silently\nsucceed as if nothing is broken?\n\nIf the breakage in the reachability chain is somewhere that affects\na branch that is actively in use by the project, with or without\nhiding a broken tip, you will be hit by object transfer failure and\nyou need to really go in there and fix things anyway.  If it is just\na no-longer-used experimental branch that lost necessary objects,\nit may be more convenient if the system automatically ignored it.\n\nIn some parts of the system there is a movement to make this trade\noff tweakable (hint: what happened to the knobs to fsck that allow\ncertain kinds of broken objects in the object store?  did the topic\ngo anywhere?). This one so far lacked such a knob to tweak, and I\nview your paranoia bit as such a knob.\n"},{"id":"257937","messageId":"20150318102108.GA27249@peff.net","threadId":"38829","inReplyTo":"xmqqh9tjme0l.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 0/5] not making corruption worse","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-18T10:21:08Z","receivedAt":"2015-03-18T10:21:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 17, 2015 at 03:54:02PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > But it strikes me as weird that we consider the _tips_ of history to be\n> > special for ignoring breakage. If the tip of \"bar\" is broken, we omit\n> > it. But if the tip is fine, and there's breakage three commits down in\n> > the history, then doing a clone is going to fail horribly, as\n> > pack-objects realizes it can't generate the pack. So in practice, I'm\n> > not sure how much you're buying with the \"don't mention broken refs\"\n> > code.\n> \n> I think this is a trade-off between strictness and convenience.  Is\n> it preferrable that every time you try to clone a repository you get\n> reminded that one of its refs point at a bogus object and you\n> instead have to do \"git fetch $there\" with a refspec that excludes\n> the broken one, or is it OK to allow clones and fetches silently\n> succeed as if nothing is broken?\n\nI think the real issue is that we do not know on the server side what\nthe client wants. Is it \"tell me the refs, so I can grab just the one I\nneed, and I don't care about the broken ones\"? Or is it \"I want\neverything you have, and tell me if you can't serve it\"?  You want\nstrictness in the latter case, but not in the former. But if we were to\nerr on the side of strictness, you could not do the former at all\n(because upload-pack would barf before the client even has a chance to\nsay anything).\n\nI'm not sure if anyone will actually find GIT_REF_PARANOIA useful for\nsomething like that or not. As an environment variable, it may impact a\nfilesystem-local clone, but it would not travel across a TCP connection.\nAnd doing so is tough, because the ref advertisement happens before the\nclient speaks.\n\nIf we ever have a client-speaks-first protocol, one extension could\nallow the client to flip the paranoia switch on the server. But my main\ngoal here was really just making \"prune\" safer, so I'm happy enough with\nwhat this series does, for now.\n\n> In some parts of the system there is a movement to make this trade\n> off tweakable (hint: what happened to the knobs to fsck that allow\n> certain kinds of broken objects in the object store?  did the topic\n> go anywhere?). This one so far lacked such a knob to tweak, and I\n> view your paranoia bit as such a knob.\n\nI think I promised several times to review that topic and never got\naround to it. Which makes me a bad person. It is still on my todo list.\n\n-Peff\n"},{"id":"257977","messageId":"5509E339.2030900@kdbg.org","threadId":"38829","inReplyTo":"20150317185550.GA14550@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-03-18T20:42:33Z","receivedAt":"2015-03-18T20:42:33Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.03.2015 um 19:55 schrieb Jeff King:\n> +\techo $bogus >.git/refs/heads/bogus..name &&\n>...\n> I assumed the final \".\" in your example wasn't significant (it is not to\n> git), but let me know if I've run afoul of another weird restriction. :)\n\nIt was actually deliberate (with intents too complicated to explain), \nbut it turns out not to be required. Your updated test case is good.\n\n-- Hannes\n"},{"id":"258044","messageId":"xmqqfv90iwjj.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150317072844.GA25191@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T20:04:16Z","receivedAt":"2015-03-19T20:04:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\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> +\t# point HEAD elsewhere\n> +\tgit checkout $base &&\n\nCould you spell this as \"$base^0\" (or \"--detach\") to clarify the\nintention?  I have been scraching my head for a few minutes just\nnow, trying to figure out what you are doing here.  I _think_ you\nwanted master to point at the missing \"two\" and wanted to make sure\nall other refs (including HEAD) to point away from it.\n\nMental note: At this point, the history looks like\n\n    base   one    two\n    o------o------o\n     \\\n      o bogus\n\nand because the reference to two is still there but two itself is\nmissing, pruning may well end up losing one, because the reference\nto it is only through master pointing at two.\n\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\nMental note: OK, this demonstrates that the missing two makes us\nlose the only reference to one (aka $recoverable in saved).\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\nI do not know offhand if the lack of the pack-refs feature header\nmatters here; I assume it does not?\n\nA safer check may be to pack and then make it missing, I guess, but\nI do not know if the difference matters.\n"},{"id":"258045","messageId":"xmqqbnjoiw4m.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150317072957.GB25191@peff.net","subject":"Re: [PATCH 2/5] refs: introduce a \"ref paranoia\" flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T20:13:13Z","receivedAt":"2015-03-19T20:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/refs.c b/refs.c\n> index 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\nI am not a big fan of proliferation of interfaces based on\nenvironment variables, but hopefully this is isolated enough to\nbecome an issue in the future.\n\n> +\n>  \treturn do_for_each_entry(refs, base, do_one_ref, &data);\n>  }\n"},{"id":"258053","messageId":"20150319205147.GA7775@peff.net","threadId":"38829","inReplyTo":"xmqqfv90iwjj.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T20:51:47Z","receivedAt":"2015-03-19T20:51:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 01:04:16PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \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> > +\t# point HEAD elsewhere\n> > +\tgit checkout $base &&\n> \n> Could you spell this as \"$base^0\" (or \"--detach\") to clarify the\n> intention?  I have been scraching my head for a few minutes just\n> now, trying to figure out what you are doing here.  I _think_ you\n> wanted master to point at the missing \"two\" and wanted to make sure\n> all other refs (including HEAD) to point away from it.\n\nYes, exactly. I've squashed in your suggestion and added a comment\nexplaining it:\n\ndiff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\nindex 1001a69..1cdbd9f 100755\n--- a/t/t5312-prune-corruption.sh\n+++ b/t/t5312-prune-corruption.sh\n@@ -50,14 +50,24 @@ 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-\t# point HEAD elsewhere\n-\tgit checkout $base &&\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> > +# 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> \n> I do not know offhand if the lack of the pack-refs feature header\n> matters here; I assume it does not?\n\nIt doesn't matter. We also do similarly gross things in other\ncorruption-related tests, but I suspect if you git-blamed them all you\nwould find that I am responsible. :)\n\n> A safer check may be to pack and then make it missing, I guess, but\n> I do not know if the difference matters.\n\nYeah, I considered that. The trouble is that we are relying on the\nearlier setup that made the object go missing. We cannot pack the refs\nin the setup step, because the earlier tests are checking the loose-ref\nbehavior. So we would have to actually restore the object, pack, and\nthen re-delete it.\n\nAnother option would be to restructure the whole test script to perform\neach individual corruption in its own sub-repo. I thought that would end\nup making things harder to understand due to the extra setup\nboilerplate, but it would make the tests less fragile with respect to\neach other (e.g., see the \"clean up bogus ref\" step which exists only to\nclean up our earlier corruption that could influence later tests).\n\n-Peff\n"},{"id":"258054","messageId":"20150319210019.GB7775@peff.net","threadId":"38829","inReplyTo":"xmqqbnjoiw4m.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/5] refs: introduce a \"ref paranoia\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T21:00:19Z","receivedAt":"2015-03-19T21:00:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 01:13:13PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > diff --git a/refs.c b/refs.c\n> > index 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> I am not a big fan of proliferation of interfaces based on\n> environment variables, but hopefully this is isolated enough to\n> become an issue in the future.\n\nI'm not sure which part you don't like.\n\nWe do have to have this variable cross some process boundaries. Only\n\"repack\" knows whether to turn on paranoia, but \"pack-objects\" is the\none that must act on it. For that case, we could add a \"--ref-paranoia\"\nflag to pack-objects.\n\nBut there are also other cases where the _user_ might want to tell us to\nbe paranoid (e.g., the upload-pack example I gave earlier). Adding\n--paranoia options to every command that might iterate, along with\nsupport for other programs that call them to pass the option through,\nseems like a large amount of maintenance burden for little benefit.\n\nWe could add a git-global \"git --ref-paranoia <cmd>\" option and leave\nthe environment variable as an implementation detail. That makes it hard\nto turn on for server-side operations, though. git-daemon runs\ngit-upload-pack without room for options, though I suppose you could run\n\"git --ref-paranoia daemon\". Smart-http is harder. I'm not sure Apache\nlets you add random arguments to CGI programs.\n\nOr is there something else I'm missing?\n\n-Peff\n"},{"id":"258059","messageId":"xmqqvbhwheb6.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150319205147.GA7775@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T21:23:25Z","receivedAt":"2015-03-19T21:23:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> A safer check may be to pack and then make it missing, I guess, but\n>> I do not know if the difference matters.\n>\n> Yeah, I considered that. The trouble is that we are relying on the\n> earlier setup that made the object go missing. We cannot pack the refs\n> in the setup step, because the earlier tests are checking the loose-ref\n> behavior. So we would have to actually restore the object, pack, and\n> then re-delete it.\n\nYes, \"restore pack redelete\" was what I had in mind when I wondered\nsuch a sequence of extra steps is worth and the difference between\nsuch an approach and an approach to use a hand-crafted packed-refs\nfile matters.\n"},{"id":"258062","messageId":"xmqqpp84hdxg.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150319210019.GB7775@peff.net","subject":"Re: [PATCH 2/5] refs: introduce a \"ref paranoia\" flag","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T21:31:39Z","receivedAt":"2015-03-19T21:31:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\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>> I am not a big fan of proliferation of interfaces based on\n>> environment variables, but hopefully this is isolated enough to\n>> become an issue in the future.\n>\n> I'm not sure which part you don't like.\n> \n> We do have to have this variable cross some process boundaries. Only\n> \"repack\" knows whether to turn on paranoia, but \"pack-objects\" is the\n> one that must act on it.\n>\n> Or is there something else I'm missing?\n\nIn general, I do not like the pattern of program A setting an\nenvironment only because it wants to tell program B it spawns\nsomething, because we cannot tell program B that the environment\nshould be dropped when it calls something else (e.g. user defined\nhooks, merge drivers, textconvs, etc.) to prevent end user\ninvocation of Git commands from honoring it.  Setting GIT_DIR or\nGIT_WORK_TREE and having to know when to drop them is not very\npleasant, for example.\n\nI think the use of this pattern is OK in this codepath in which\nrepack calls pack-objects, and I think I can be persuaded to buy an\nargument that there is no harm, or it may even be a good thing, to\nrun such an end-user program under paranoia mode, if pack-objects\nwants to spawn one.\n"},{"id":"258063","messageId":"20150319214707.GA8680@peff.net","threadId":"38829","inReplyTo":"xmqqvbhwheb6.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T21:47:07Z","receivedAt":"2015-03-19T21:47:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 02:23:25PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> >> A safer check may be to pack and then make it missing, I guess, but\n> >> I do not know if the difference matters.\n> >\n> > Yeah, I considered that. The trouble is that we are relying on the\n> > earlier setup that made the object go missing. We cannot pack the refs\n> > in the setup step, because the earlier tests are checking the loose-ref\n> > behavior. So we would have to actually restore the object, pack, and\n> > then re-delete it.\n> \n> Yes, \"restore pack redelete\" was what I had in mind when I wondered\n> such a sequence of extra steps is worth and the difference between\n> such an approach and an approach to use a hand-crafted packed-refs\n> file matters.\n\nI took a look at this. It turns out to be rather annoying, because we\ncan't just restore $missing. The earlier tests may have deleted other\nrandom objects (like $recoverable) depending on whether or not they\nactually failed.\n\nSo I'm inclined to leave it (we do confirm with the rev-parse call at\nthe end of the setup that our packed-refs file is working) unless you\nfeel strongly. If you do, I'd rather go the route of sticking each\ncorruption in its own separate sub-repo.\n\n-Peff\n"},{"id":"258064","messageId":"xmqqlhishd3i.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150319214707.GA8680@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-19T21:49:37Z","receivedAt":"2015-03-19T21:49:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So I'm inclined to leave it (we do confirm with the rev-parse call at\n> the end of the setup that our packed-refs file is working) unless you\n> feel strongly. If you do, I'd rather go the route of sticking each\n> corruption in its own separate sub-repo.\n\nNo, I don't feel strongly either way---otherwise I wouldn't be\nwondering if it makes a difference, but explaining why hand-crafting\nis a bad idea (or the other way around).\n\nThanks.\n"},{"id":"258065","messageId":"20150319215114.GB8680@peff.net","threadId":"38829","inReplyTo":"xmqqpp84hdxg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/5] refs: introduce a \"ref paranoia\" flag","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T21:51:14Z","receivedAt":"2015-03-19T21:51:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 02:31:39PM -0700, Junio C Hamano wrote:\n\n> > We do have to have this variable cross some process boundaries. Only\n> > \"repack\" knows whether to turn on paranoia, but \"pack-objects\" is the\n> > one that must act on it.\n> >\n> > Or is there something else I'm missing?\n> \n> In general, I do not like the pattern of program A setting an\n> environment only because it wants to tell program B it spawns\n> something, because we cannot tell program B that the environment\n> should be dropped when it calls something else (e.g. user defined\n> hooks, merge drivers, textconvs, etc.) to prevent end user\n> invocation of Git commands from honoring it.  Setting GIT_DIR or\n> GIT_WORK_TREE and having to know when to drop them is not very\n> pleasant, for example.\n> \n> I think the use of this pattern is OK in this codepath in which\n> repack calls pack-objects, and I think I can be persuaded to buy an\n> argument that there is no harm, or it may even be a good thing, to\n> run such an end-user program under paranoia mode, if pack-objects\n> wants to spawn one.\n\nAh, I see. Yeah, I consider that to be a _feature_ for REF_PARANOIA\nhere. If you are running receive-pack under REF_PARANOIA, for example,\nyou would want your pre-receive hooks to use the same rules as the rest\nof receive-pack.\n\nIf there is a misfeature, it is that we turn on REF_PARANOIA\nautomatically behind the user's back in some cases, which could surprise\nthem if we call through to custom code. But as you note, I think this\ncode path is OK, because we don't spawn anything else from pack-objects\n(and if we did, arguably it is OK because our caller told us we are\ndoing something dangerous; but we would have to evaluate that\ncase-by-case, I would think).\n\n-Peff\n"},{"id":"258066","messageId":"20150319215200.GC8680@peff.net","threadId":"38829","inReplyTo":"xmqqlhishd3i.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-19T21:52:00Z","receivedAt":"2015-03-19T21:52:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 02:49:37PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > So I'm inclined to leave it (we do confirm with the rev-parse call at\n> > the end of the setup that our packed-refs file is working) unless you\n> > feel strongly. If you do, I'd rather go the route of sticking each\n> > corruption in its own separate sub-repo.\n> \n> No, I don't feel strongly either way---otherwise I wouldn't be\n> wondering if it makes a difference, but explaining why hand-crafting\n> is a bad idea (or the other way around).\n\nAnd here I thought you were just being polite. ;)\n\n-Peff\n"},{"id":"258078","messageId":"CAPig+cTfqWr9un=4+QGs0jcUaV9U=z5Xyg3-r-D6dABGXUkmzg@mail.gmail.com","threadId":"38829","inReplyTo":"20150317072844.GA25191@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-20T01:16:52Z","receivedAt":"2015-03-20T01:16:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 17, 2015 at 3:28 AM, Jeff King <peff@peff.net> wrote:\n> When we are doing a destructive operation like \"git prune\",\n> we want to be extra careful that the set of reachable tips\n> we compute is valid. If there is any corruption or oddity,\n> we are better off aborting the operation and letting the\n> user figure things out rather than plowing ahead and\n> possibly deleting some data that cannot be recovered.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh\n> new file mode 100755\n> index 0000000..167031e\n> --- /dev/null\n> +++ b/t/t5312-prune-corruption.sh\n> @@ -0,0 +1,104 @@\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> +       rm -f .git/refs/heads/master &&\n> +       cat >.git/packed-refs <<-EOF\n\nBroken &&-chain.\n\n> +       $missing refs/heads/master\n> +       $recoverable refs/heads/other\n> +       EOF\n> +       echo $missing >expect &&\n> +       git rev-parse refs/heads/master >actual &&\n> +       test_cmp expect actual\n> +'\n"},{"id":"258079","messageId":"CAPig+cSBnLbZVikuZDy4cN5hV1rMs4cmLVqd-k+9Qi9e-9t+kQ@mail.gmail.com","threadId":"38829","inReplyTo":"20150317073157.GE25191@peff.net","subject":"Re: [PATCH 5/5] refs.c: drop curate_packed_refs","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-20T01:27:02Z","receivedAt":"2015-03-20T01:27:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Mar 17, 2015 at 3:31 AM, Jeff King <peff@peff.net> wrote:\n> When we delete a ref, we have to rewrite the entire\n> packed-refs file. We take this opportunity to \"curate\" the\n> packed-refs file and drop any entries that are crufty or\n> broken.\n>\n> Dropping broken entries (e.g., with bogus names, or ones\n> that point to missing objects) is actively a bad idea, as it\n> means that we lose any notion that the data was there in the\n> first place. Aside from the general hackiness that we might\n> lose any information about ref \"foo\" while deleting an\n> unrelated ref \"bar\", this may seriously hamper any attempts\n> by the user at recovering from the corruption in \"foo\".\n>\n> They will lose the sha1 and name of \"foo\"; the exact pointer\n> may still be useful even if they recover missing objects\n> from a different copy of the repository. But worse, once the\n> ref is gone, there is no trace of the corruption. A\n> follow-up \"git prune\" may delete objects, even though it\n> would otherwise bail when seeing corruption.\n>\n> We could just drop the \"broken\" bits from\n> curate_packed_refs, and continue to drop the \"crufty\" bits:\n> refs whose loose counterpart exists in the filesystem. This\n> is not wrong to do, and it does have the advantage that we\n> may write out a slightly smaller packed-refs file. But it\n> has 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 outweights the benefit from writing out a smaller\n\ns/outweights/outweighs/\n\n>      packed-refs file (it doesn't get smaller, and you do a\n>      bunch of directory traversal to find that out).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"},{"id":"258080","messageId":"20150320013217.GA15302@peff.net","threadId":"38829","inReplyTo":"CAPig+cTfqWr9un=4+QGs0jcUaV9U=z5Xyg3-r-D6dABGXUkmzg@mail.gmail.com","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T01:32:17Z","receivedAt":"2015-03-20T01:32:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote:\n\n> > --- /dev/null\n> > +++ b/t/t5312-prune-corruption.sh\n> > @@ -0,0 +1,104 @@\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> > +       rm -f .git/refs/heads/master &&\n> > +       cat >.git/packed-refs <<-EOF\n> \n> Broken &&-chain.\n\nThanks. I notice that a large number of broken &&-chains are on\nhere-docs. I really wish you could put the && on the \"EOF\" line at the\nend of the here-doc. I understand _why_ that this not the case, but\nmentally it is where I want to type it, and I obviously sometimes fail\nto go back and fix it. I don't think there's a better solution in POSIX\nsh, though.\n\n-Peff\n"},{"id":"258081","messageId":"CAPig+cQKhBw8air5y3NJaP5Rx9pXVaz9Lmvhe20AimW7pCsWBw@mail.gmail.com","threadId":"38829","inReplyTo":"20150320013217.GA15302@peff.net","subject":"Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-20T01:37:12Z","receivedAt":"2015-03-20T01:37:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 19, 2015 at 9:32 PM, Jeff King <peff@peff.net> wrote:\n> On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote:\n>\n>> > --- /dev/null\n>> > +++ b/t/t5312-prune-corruption.sh\n>> > @@ -0,0 +1,104 @@\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>> > +       rm -f .git/refs/heads/master &&\n>> > +       cat >.git/packed-refs <<-EOF\n>>\n>> Broken &&-chain.\n>\n> Thanks. I notice that a large number of broken &&-chains are on\n> here-docs. I really wish you could put the && on the \"EOF\" line at the\n> end of the here-doc. I understand _why_ that this not the case, but\n> mentally it is where I want to type it, and I obviously sometimes fail\n> to go back and fix it. I don't think there's a better solution in POSIX\n> sh, though.\n\nI wonder if test-lint could be enhanced to detect this sort of problem?\n"},{"id":"258082","messageId":"20150320020851.GC15302@peff.net","threadId":"38829","inReplyTo":"CAPig+cQKhBw8air5y3NJaP5Rx9pXVaz9Lmvhe20AimW7pCsWBw@mail.gmail.com","subject":"test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T02:08:51Z","receivedAt":"2015-03-20T02:08:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 09:37:12PM -0400, Eric Sunshine wrote:\n\n> > Thanks. I notice that a large number of broken &&-chains are on\n> > here-docs. I really wish you could put the && on the \"EOF\" line at the\n> > end of the here-doc. I understand _why_ that this not the case, but\n> > mentally it is where I want to type it, and I obviously sometimes fail\n> > to go back and fix it. I don't think there's a better solution in POSIX\n> > sh, though.\n> \n> I wonder if test-lint could be enhanced to detect this sort of problem?\n\nThat would be nice, but it's complicated. A naive:\n\ndiff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl\nindex b170cbc..3a6d8d8 100755\n--- a/t/check-non-portable-shell.pl\n+++ b/t/check-non-portable-shell.pl\n@@ -22,6 +22,7 @@ while (<>) {\n \t/^\\s*[^#]\\s*which\\s/ and err 'which is not portable (please use type)';\n \t/\\btest\\s+[^=]*==/ and err '\"test a == b\" is not portable (please use =)';\n \t/\\bexport\\s+[A-Za-z0-9_]*=/ and err '\"export FOO=bar\" is not portable (please use FOO=bar && export FOO)';\n+\t/ <<-?.?EOF(.*)/ && $1 !~ /&&/ and err 'here-doc with broken &&-chain';\n \t# this resets our $. for each file\n \tclose ARGV if eof;\n }\n\nyields quite a few false positives, because of course we don't know\nwhich are meant to be at the end of the chain and which are not. And\nfinding that out is tough. We'd have to actually parse to the end of\nthe here-doc ourselves, then see if it was the end of the test_expect\nblock.\n\nI think it would be simpler to ask the shell to check this for us, like:\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex c096778..02a03d5 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -524,6 +524,21 @@ test_eval_ () {\n test_run_ () {\n \ttest_cleanup=:\n \texpecting_failure=$2\n+\n+\tif test -n \"$GIT_TEST_CHAIN_LINT\"; then\n+\t\t# 117 is unlikely to match the exit code of\n+\t\t# another part of the chain\n+\t\ttest_eval_ \"(exit 117) && $1\"\n+\t\tif test \"$?\" != 117; then\n+\t\t\t# all bets are off for continuing with other tests;\n+\t\t\t# we expected none of the rest of the test commands to\n+\t\t\t# run, but at least some did. Who knows what weird\n+\t\t\t# state we're in? Just bail, and the user can diagnose\n+\t\t\t# by running in --verbose mode\n+\t\t\terror \"bug in the test script: broken &&-chain\"\n+\t\tfi\n+\tfi\n+\n \tsetup_malloc_check\n \ttest_eval_ \"$1\"\n \teval_ret=$?\n\nThis turns up an appalling number of failures, but AFAICT they are all\n\"real\" in the sense that the &&-chains are broken. In some cases these\nare real, but in others the tests are of an older style where they did\nnot expect some early commands to fail (and we would catch their bogus\noutput if they did). E.g., in the patch below, I think the first one is\na real potential bug, and the other two are mostly noise. I do not mind\nsetting a rule and fixing all of them, though.\n\nI seem to recall people looked at doing this sort of lint a while ago,\nbut we never ended up committing anything. I wonder if it was because of\nall of these \"false positives\".\n\ndiff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh\nindex 6d3b828..62fce10 100755\n--- a/t/t3010-ls-files-killed-modified.sh\n+++ b/t/t3010-ls-files-killed-modified.sh\n@@ -62,7 +62,7 @@ test_expect_success 'git update-index --add to add various paths.' '\n \t\t\tcd submod$i && git commit --allow-empty -m \"empty $i\"\n \t\t) || break\n \tdone &&\n-\tgit update-index --add submod[12]\n+\tgit update-index --add submod[12] &&\n \t(\n \t\tcd submod1 &&\n \t\tgit commit --allow-empty -m \"empty 1 (updated)\"\n@@ -99,12 +99,12 @@ test_expect_success 'git ls-files -k to show killed files.' '\n '\n \n test_expect_success 'git ls-files -k output (w/o icase)' '\n-\tgit ls-files -k >.output\n+\tgit ls-files -k >.output &&\n \ttest_cmp .expected .output\n '\n \n test_expect_success 'git ls-files -k output (w/ icase)' '\n-\tgit -c core.ignorecase=true ls-files -k >.output\n+\tgit -c core.ignorecase=true ls-files -k >.output &&\n \ttest_cmp .expected .output\n '\n \n\n-Peff\n"},{"id":"258083","messageId":"20150320022532.GA5502@peff.net","threadId":"38829","inReplyTo":"20150320020851.GC15302@peff.net","subject":"Re: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T02:25:32Z","receivedAt":"2015-03-20T02:25:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Jonathan, whose patch I apparently subconsciously copied]\n\nOn Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index c096778..02a03d5 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -524,6 +524,21 @@ test_eval_ () {\n>  test_run_ () {\n>  \ttest_cleanup=:\n>  \texpecting_failure=$2\n> +\n> +\tif test -n \"$GIT_TEST_CHAIN_LINT\"; then\n> +\t\t# 117 is unlikely to match the exit code of\n> +\t\t# another part of the chain\n> +\t\ttest_eval_ \"(exit 117) && $1\"\n> +\t\tif test \"$?\" != 117; then\n> +\t\t\t# all bets are off for continuing with other tests;\n> +\t\t\t# we expected none of the rest of the test commands to\n> +\t\t\t# run, but at least some did. Who knows what weird\n> +\t\t\t# state we're in? Just bail, and the user can diagnose\n> +\t\t\t# by running in --verbose mode\n> +\t\t\terror \"bug in the test script: broken &&-chain\"\n> +\t\tfi\n> +\tfi\n> +\n>  \tsetup_malloc_check\n>  \ttest_eval_ \"$1\"\n>  \teval_ret=$?\n> \n> This turns up an appalling number of failures, but AFAICT they are all\n> \"real\" in the sense that the &&-chains are broken. In some cases these\n> are real, but in others the tests are of an older style where they did\n> not expect some early commands to fail (and we would catch their bogus\n> output if they did). E.g., in the patch below, I think the first one is\n> a real potential bug, and the other two are mostly noise. I do not mind\n> setting a rule and fixing all of them, though.\n> \n> I seem to recall people looked at doing this sort of lint a while ago,\n> but we never ended up committing anything. I wonder if it was because of\n> all of these \"false positives\".\n\nThis turns out to be rather annoying to grep for in the list archives,\nbut I found at least one discussion:\n\n  http://article.gmane.org/gmane.comp.version-control.git/235913\n\nI don't know why we didn't follow it up then. Perhaps because the patch\nthere (which is rather similar to what I have above) was not\nconditional, so whole chunks of the test suite needed fixing. There are\nenough problems that we would probably want to do this conditionally,\nfix them over time, and then finally flip the feature on by default.\n\n-Peff\n"},{"id":"258086","messageId":"20150320051051.GA30803@peff.net","threadId":"38829","inReplyTo":"20150320022532.GA5502@peff.net","subject":"Re: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T05:10:52Z","receivedAt":"2015-03-20T05:10:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:\n\n> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n> > index c096778..02a03d5 100644\n> > --- a/t/test-lib.sh\n> > +++ b/t/test-lib.sh\n> > @@ -524,6 +524,21 @@ test_eval_ () {\n> >  test_run_ () {\n> >  \ttest_cleanup=:\n> >  \texpecting_failure=$2\n> > +\n> > +\tif test -n \"$GIT_TEST_CHAIN_LINT\"; then\n> > +\t\t# 117 is unlikely to match the exit code of\n> > +\t\t# another part of the chain\n> > +\t\ttest_eval_ \"(exit 117) && $1\"\n> > +\t\tif test \"$?\" != 117; then\n> > +\t\t\t# all bets are off for continuing with other tests;\n> > +\t\t\t# we expected none of the rest of the test commands to\n> > +\t\t\t# run, but at least some did. Who knows what weird\n> > +\t\t\t# state we're in? Just bail, and the user can diagnose\n> > +\t\t\t# by running in --verbose mode\n> > +\t\t\terror \"bug in the test script: broken &&-chain\"\n> > +\t\tfi\n> > +\tfi\n> > +\n> >  \tsetup_malloc_check\n> >  \ttest_eval_ \"$1\"\n> >  \teval_ret=$?\n> > \n> > This turns up an appalling number of failures, but AFAICT they are all\n> > \"real\" in the sense that the &&-chains are broken. In some cases these\n> > are real, but in others the tests are of an older style where they did\n> > not expect some early commands to fail (and we would catch their bogus\n> > output if they did). E.g., in the patch below, I think the first one is\n> > a real potential bug, and the other two are mostly noise. I do not mind\n> > setting a rule and fixing all of them, though.\n\nFWIW, I have spent about a few hours wading through the errors, and am\nabout 75% done. There are definitely some broken chains that were\ncausing test results to be ignored (as opposed to just minor setup steps\nthat we would not expect to fail). In most cases, the tests do passed. I\nhave a few that I still need to examine more closely, but there may be\nsome where there are actual test failures (but it's possible that I just\nscrewed it up while fixing the &&-chaining).\n\nI hope to post something tonight, but I wanted to drop a note on the off\nchance that you were actively looking at it at the same time.\n\n-Peff\n"},{"id":"258087","messageId":"xmqqd244go0h.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150320022532.GA5502@peff.net","subject":"Re: test &&-chain lint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-20T06:51:26Z","receivedAt":"2015-03-20T06:51:26Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> [+cc Jonathan, whose patch I apparently subconsciously copied]\n>\n> On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:\n>\n>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> index c096778..02a03d5 100644\n>> --- a/t/test-lib.sh\n>> +++ b/t/test-lib.sh\n>> @@ -524,6 +524,21 @@ test_eval_ () {\n>>  test_run_ () {\n>>  \ttest_cleanup=:\n>>  \texpecting_failure=$2\n>> +\n>> +\tif test -n \"$GIT_TEST_CHAIN_LINT\"; then\n>> +\t\t# 117 is unlikely to match the exit code of\n>> +\t\t# another part of the chain\n>> +\t\ttest_eval_ \"(exit 117) && $1\"\n>> +\t\tif test \"$?\" != 117; then\n>> +\t\t\t# all bets are off for continuing with other tests;\n>> +\t\t\t# we expected none of the rest of the test commands to\n>> +\t\t\t# run, but at least some did. Who knows what weird\n>> +\t\t\t# state we're in? Just bail, and the user can diagnose\n>> +\t\t\t# by running in --verbose mode\n>> +\t\t\terror \"bug in the test script: broken &&-chain\"\n>> +\t\tfi\n>> +\tfi\n>> +\n>>  \tsetup_malloc_check\n>>  \ttest_eval_ \"$1\"\n>>  \teval_ret=$?\n>> \n>> This turns up an appalling number of failures, but AFAICT they are all\n>> \"real\" in the sense that the &&-chains are broken. In some cases these\n>> are real, but in others the tests are of an older style where they did\n>> not expect some early commands to fail (and we would catch their bogus\n>> output if they did). E.g., in the patch below, I think the first one is\n>> a real potential bug, and the other two are mostly noise. I do not mind\n>> setting a rule and fixing all of them, though.\n>> \n>> I seem to recall people looked at doing this sort of lint a while ago,\n>> but we never ended up committing anything. I wonder if it was because of\n>> all of these \"false positives\".\n>\n> This turns out to be rather annoying to grep for in the list archives,\n> but I found at least one discussion:\n>\n>   http://article.gmane.org/gmane.comp.version-control.git/235913\n>\n> I don't know why we didn't follow it up then. Perhaps because the patch\n> there (which is rather similar to what I have above) was not\n> conditional, so whole chunks of the test suite needed fixing. There are\n> enough problems that we would probably want to do this conditionally,\n> fix them over time, and then finally flip the feature on by default.\n\nHmmm, they do look similar and unfamiliar ;-)  It happened while I\nwas offline, it seems.\n\nThanks for working on this---I think test-chain-lint should become\none of the pre-acceptance test on my end when it gets applied.\n"},{"id":"258090","messageId":"CAPig+cQ29Ab7-2RWB6HXUk=oikhpF=1J7qnLcw6kqvTR7p_tow@mail.gmail.com","threadId":"38829","inReplyTo":"20150320051051.GA30803@peff.net","subject":"Re: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2015-03-20T07:18:23Z","receivedAt":"2015-03-20T07:18:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Mar 20, 2015 at 1:10 AM, Jeff King <peff@peff.net> wrote:\n> On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:\n>> > diff --git a/t/test-lib.sh b/t/test-lib.sh\n>> > index c096778..02a03d5 100644\n>> > --- a/t/test-lib.sh\n>> > +++ b/t/test-lib.sh\n>> > @@ -524,6 +524,21 @@ test_eval_ () {\n>> >  test_run_ () {\n>> > +   if test -n \"$GIT_TEST_CHAIN_LINT\"; then\n>> > +           # 117 is unlikely to match the exit code of\n>> > +           # another part of the chain\n>> > +           test_eval_ \"(exit 117) && $1\"\n>> > +           if test \"$?\" != 117; then\n>> > +                   # all bets are off for continuing with other tests;\n>> > +                   # we expected none of the rest of the test commands to\n>> > +                   # run, but at least some did. Who knows what weird\n>> > +                   # state we're in? Just bail, and the user can diagnose\n>> > +                   # by running in --verbose mode\n>> > +                   error \"bug in the test script: broken &&-chain\"\n>> > +           fi\n>> > +   fi\n\nClever (Jonathan's too); much nicer than trying to special case only here-doc.\n\n>> > This turns up an appalling number of failures, but AFAICT they are all\n>> > \"real\" in the sense that the &&-chains are broken. In some cases these\n>> > are real, but in others the tests are of an older style where they did\n>> > not expect some early commands to fail (and we would catch their bogus\n>> > output if they did). E.g., in the patch below, I think the first one is\n>> > a real potential bug, and the other two are mostly noise. I do not mind\n>> > setting a rule and fixing all of them, though.\n>\n> FWIW, I have spent about a few hours wading through the errors, and am\n> about 75% done. There are definitely some broken chains that were\n> causing test results to be ignored (as opposed to just minor setup steps\n> that we would not expect to fail). In most cases, the tests do passed. I\n> have a few that I still need to examine more closely, but there may be\n> some where there are actual test failures (but it's possible that I just\n> screwed it up while fixing the &&-chaining).\n>\n> I hope to post something tonight, but I wanted to drop a note on the off\n> chance that you were actively looking at it at the same time.\n\nThanks for working on this. It looks like this technique should be a\nvaluable addition to test-lint. (I had intended, but haven't yet found\ntime to dig into it, so I'm happy to hear of your progress.)\n"},{"id":"258134","messageId":"xmqq384zha6s.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"xmqqd244go0h.fsf@gitster.dls.corp.google.com","subject":"Re: test &&-chain lint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-20T17:04:43Z","receivedAt":"2015-03-20T17:04:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Jeff King <peff@peff.net> writes:\n>\n>> [+cc Jonathan, whose patch I apparently subconsciously copied]\n>>\n>> On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:\n>>\n>>> diff --git a/t/test-lib.sh b/t/test-lib.sh\n>>> index c096778..02a03d5 100644\n>>> --- a/t/test-lib.sh\n>>> +++ b/t/test-lib.sh\n>>> @@ -524,6 +524,21 @@ test_eval_ () {\n>>>  test_run_ () {\n>>>  \ttest_cleanup=:\n>>>  \texpecting_failure=$2\n>>> +\n>>> +\tif test -n \"$GIT_TEST_CHAIN_LINT\"; then\n>>> +\t\t# 117 is unlikely to match the exit code of\n>>> +\t\t# another part of the chain\n>>> +\t\ttest_eval_ \"(exit 117) && $1\"\n>>> +\t\tif test \"$?\" != 117; then\n>>> +\t\t\t# all bets are off for continuing with other tests;\n>>> +\t\t\t# we expected none of the rest of the test commands to\n>>> +\t\t\t# run, but at least some did. Who knows what weird\n>>> +\t\t\t# state we're in? Just bail, and the user can diagnose\n>>> +\t\t\t# by running in --verbose mode\n>>> +\t\t\terror \"bug in the test script: broken &&-chain\"\n>>> +\t\tfi\n>>> +\tfi\n>> ...\n> Hmmm, they do look similar and unfamiliar ;-)  It happened while I\n> was offline, it seems.\n\nOne case where this might misdetect a good test would be this one:\n\n    test_expect_success 'either succeed or fail with status 1' '\n\tgit subcmd || case \"$?\" in 1) : happy ;; *) false failure ;; esac\n    '\n\nwhich would turn into\n\n\t(exit 117) && git subcmd || case ...\n\nand fail to set $? to 117, triggering a false positive.\n\nI do not have a good solution fo that, though.  Obviously, turning\nthe check into\n\n\t(exit 117) && {\n        \t$1\n\t}\n\nmisses the entire point of the chain-lint.\n\nI wonder if another valid way to make it harder for us to commit\n\"broken && chain\" errors in our test may be to make it not an error\nin the first place.  Do we know how buggy various implementations of\nshells are with respect to their handling of \"set -e\"?\n\nWe know that chaining commands with && is much less likely to be\nbroken in various reimplementation of bourne shells, and that is\nthe primary reason we stick to this style in our tests, but if\neverybody implements \"set -e\" reliably and consistently, flipping\nthat bit in test_eval_ and removing the need to &&-cascade the\ncommands might not be such a bad idea.\n\nJust thinking aloud...\n"},{"id":"258137","messageId":"20150320172406.GA15172@peff.net","threadId":"38829","inReplyTo":"xmqq384zha6s.fsf@gitster.dls.corp.google.com","subject":"Re: test &&-chain lint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T17:24:07Z","receivedAt":"2015-03-20T17:24:07Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2015 at 10:04:43AM -0700, Junio C Hamano wrote:\n\n> One case where this might misdetect a good test would be this one:\n> \n>     test_expect_success 'either succeed or fail with status 1' '\n> \tgit subcmd || case \"$?\" in 1) : happy ;; *) false failure ;; esac\n>     '\n\nYes. Any use of \"||\" is going to cause problems. See the conversions in\nthe series I posted a few hours ago. The solution is to adapt the test\nstyle to encase any \"||\" inside a block. I don't like having to modify\nthe tests to match our lint check, but it is a fairly rare case, and the\nmodification is not complex. And in fact I think it often highlights to\nthe reader that something funny is going on; a snippet like the one\nabove is OK to run at the start of a test, but:\n\n  foo &&\n  bar ||\n  case \"$?\"\n   ...\n\nis potentially dangerous. You do not know if you are checking the $? of\n\"foo\" or \"bar\" in that case statement.\n\nYour case above is actually better spelled as test_expect_code, but\nthere are more complex one-off cases that I solved using a {} block.\n\n> I wonder if another valid way to make it harder for us to commit\n> \"broken && chain\" errors in our test may be to make it not an error\n> in the first place.  Do we know how buggy various implementations of\n> shells are with respect to their handling of \"set -e\"?\n\nCertainly that is a thought that occurred to me while writing the\nseries. At GitHub, we have a shell-based test harness for internal\nprojects that is somewhat like Git's t/test-lib.sh, but uses \"-e\" to\ncatch errors.\n\nIt's mostly OK, but there are some surprising bits. For instance, try\nthis sequence of snippets:\n\n     set -e\n     false\n     echo should not reach\n\nThat should print nothing. So far so good. How about in a subshell:\n\n     set -e\n     (\n       false\n       echo 1\n     )\n     echo 2\n\nThat also prints nothing. Good. But \"set -e\" is suppressed in a\nconditional or in the presence of logic operators like \"||\".  So you'd\nexpect this:\n\n     set -e\n     (\n       false\n       echo 1\n     ) || {\n         echo outcome=$?\n         false\n     }\n     echo 2\n\nto break out of the subshell, print the failed outcome, and then exit.\nBut it doesn't. It prints \"1\" and \"2\". The suppression of \"set -e\"\ncontinues inside the subshell, even though the inner commands are not\npart of their own conditionals.\n\nOK, so when we open a subshell we have to re-iterate our desire for \"set\n-e\":\n\n    set -e\n    (\n      set -e\n      false\n      echo 1\n    ) || {\n        echo outcome=$?\n\tfalse\n    }\n    echo 2\n\nNope, does nothing, at least in bash and dash. The ||-operator\nsuppression is clearly not \"turn off set -e\", but \"globally make -e\nuseless inside here\".\n\nSo you end up having to use manual exits to jump out of the subshell,\nlike:\n\n     set -e\n     (\n       false || exit 1\n       echo 1\n     ) || {\n         echo outcome=$?\n         false\n     }\n     echo 2\n\n\nThat snippet produces \"outcome=1\", which is what I expect.\n\nOf course most tests are not so complicated to have subshells with\nconditional operators on the side. But I did not just type out that\nexample right now from scratch. I cut-and-pasted it from a real-world\ncommit message.\n\nSo I dunno. I think \"set -e\" is kind of a dangerous lure. It works so\nwell _most_ of the time that you start to rely on it, but it really does\nhave some funny corner cases (even on modern shells, and for all I know,\nthe behavior above is mandated by POSIX).\n\n-Peff\n"},{"id":"258138","messageId":"xmqqoannfu84.fsf@gitster.dls.corp.google.com","threadId":"38829","inReplyTo":"20150320172406.GA15172@peff.net","subject":"Re: test &&-chain lint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-03-20T17:34:51Z","receivedAt":"2015-03-20T17:34:51Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Your case above is actually better spelled as test_expect_code, but\n> there are more complex one-off cases that I solved using a {} block.\n\nJust for the record, test_expect_code expects only one possible good\nexit status and it does not allow us to say \"0 is OK and 1 is also\nOK, everything else is bad\", so it is not quite appropriate there.\n\n> ...\n> So I dunno. I think \"set -e\" is kind of a dangerous lure.\n\nYes, I think we should stay away from it.  &&-chaining is simpler to\nsee what is going on, even though it is a bit more to type.\n\nThanks.\n"},{"id":"258145","messageId":"20150320175945.GA11844@peff.net","threadId":"38829","inReplyTo":"xmqqoannfu84.fsf@gitster.dls.corp.google.com","subject":"Re: test &&-chain lint","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-03-20T17:59:45Z","receivedAt":"2015-03-20T17:59:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 20, 2015 at 10:34:51AM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Your case above is actually better spelled as test_expect_code, but\n> > there are more complex one-off cases that I solved using a {} block.\n> \n> Just for the record, test_expect_code expects only one possible good\n> exit status and it does not allow us to say \"0 is OK and 1 is also\n> OK, everything else is bad\", so it is not quite appropriate there.\n\nOh, sorry, I misread your test as expecting 1, but it is actually\nexpecting 0 or 1. Yeah, I agree this is the exact sort of case covered\nin my patch 11/25 (and there are only 4 instances in the whole test\nsuite).\n\n-Peff\n"}]}