[PATCH 0/5] not making corruption worse

This is a grab bag of fixes related to performing destructive operations
in a repository with minor corruption. Of course we hope never to see
corruption in the first place, but I think if we do see it, we should
err on the side of not making things worse. IOW, it is better to abort
and say "fix this before pruning" than it is to just start deleting
objects.

The issue that spurred this is that I noticed recent versions of git
will omit funny-named refs from iteration. This comes from d0f810f
(refs.c: allow listing and deleting badly named refs, 2014-09-03), in
v2.2.0.  That's probably a good idea in general, but for things like
"prune" we need to be more careful (and we were, prior to that commit).

Similarly, if you have a ref whose tip object is missing, we tend to
just ignore it in our traversals, and it presents a similar problem for
pruning. I didn't trace it back, but I think this problem is much older.

The general strategy for these is to use for_each_rawref traversals in
these situations. That doesn't cover _every_ possible scenario. For
example, you could do:

  git clone --no-local repo.git backup.git &&
  rm -rf repo.git

and you might be disappointed if "backup.git" omitted some broken refs
(upload-pack will simply skip the broken refs in its advertisement).  We
could tighten this, but then it becomes hard to access slightly broken
repositories (e.g., you might prefer to clone what you can, and not have
git die() when it tries to serve the breakage). Patch 2 provides a
tweakable safety valve for this.

And not strictly related to the above, but in the same ballpark, is the
issue with packed-refs that I noted here:

  http://thread.gmane.org/gmane.comp.version-control.git/256051/focus=256896

where we can drop broken refs from the packed-refs file during the
deletion of an unrelated ref.

The patches are:

  [1/5]: t5312: test object deletion code paths in a corrupted repository
  [2/5]: refs: introduce a "ref paranoia" flag
  [3/5]: prune: turn on ref_paranoia flag
  [4/5]: repack: turn on "ref paranoia" when doing a destructive repack
  [5/5]: refs.c: drop curate_packed_refs

-Peff

When we are doing a destructive operation like "git prune",
we want to be extra careful that the set of reachable tips
we compute is valid. If there is any corruption or oddity,
we are better off aborting the operation and letting the
user figure things out rather than plowing ahead and
possibly deleting some data that cannot be recovered.

The tests here include:

  1. Pruning objects mentioned only be refs with invalid
     names. This used to abort prior to d0f810f (refs.c:
     allow listing and deleting badly named refs,
     2014-09-03), but since then we silently ignore the tip.

     Likewise, we test repacking that can drop objects
     (either "-ad", which drops anything unreachable,
     or "-Ad --unpack-unreachable=<time>", which tries to
     optimize out a loose object write that would be
     directly pruned).

  2. Pruning objects when some refs point to missing
     objects. We don't know whether any dangling objects
     would have been reachable from the missing objects. We
     are better to keep them around, as they are better than
     nothing for helping the user recover history.

  3. Packed refs that point to missing objects can sometimes
     be dropped. By itself, this is more of an annoyance
     (you do not have the object anyway; even if you can
     recover it from elsewhere, all you are losing is a
     placeholder for your state at the time of corruption).
     But coupled with (2), if we drop the ref and then go
     on to prune, we may lose unrecoverable objects.

Note that we use test_might_fail for some of the operations.
In some cases, it would be appropriate to abort the
operation, and in others, it might be acceptable to continue
but taking the information into account. The tests don't
care either way, and check only for data loss.

Signed-off-by: Jeff King <redacted>
---
 t/t5312-prune-corruption.sh | 104 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 104 insertions(+)
 create mode 100755 t/t5312-prune-corruption.sh

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
new file mode 100755
index 0000000..167031e
--- /dev/null
+++ b/t/t5312-prune-corruption.sh

@@ -0,0 +1,104 @@
+#!/bin/sh
+
+test_description='
+Test pruning of repositories with minor corruptions. The goal
+here is that we should always be erring on the side of safety. So
+if we see, for example, a ref with a bogus name, it is OK either to
+bail out or to proceed using it as a reachable tip, but it is _not_
+OK to proceed as if it did not exist. Otherwise we might silently
+delete objects that cannot be recovered.
+'
+. ./test-lib.sh
+
+test_expect_success 'disable reflogs' '
+	git config core.logallrefupdates false &&
+	rm -rf .git/logs
+'
+
+test_expect_success 'create history reachable only from a bogus-named ref' '
+	test_tick && git commit --allow-empty -m master &&
+	base=$(git rev-parse HEAD) &&
+	test_tick && git commit --allow-empty -m bogus &&
+	bogus=$(git rev-parse HEAD) &&
+	git cat-file commit $bogus >saved &&
+	echo $bogus >.git/refs/heads/bogus:name &&
+	git reset --hard HEAD^
+'
+
+test_expect_failure 'pruning does not drop bogus object' '
+	test_when_finished "git hash-object -w -t commit saved" &&
+	test_might_fail git prune --expire=now &&
+	verbose git cat-file -e $bogus
+'
+
+test_expect_success 'put bogus object into pack' '
+	git tag reachable $bogus &&
+	git repack -ad &&
+	git tag -d reachable &&
+	verbose git cat-file -e $bogus
+'
+
+test_expect_failure 'destructive repack keeps packed object' '
+	test_might_fail git repack -Ad --unpack-unreachable=now &&
+	verbose git cat-file -e $bogus &&
+	test_might_fail git repack -ad &&
+	verbose git cat-file -e $bogus
+'
+
+# subsequent tests will have different corruptions
+test_expect_success 'clean up bogus ref' '
+	rm .git/refs/heads/bogus:name
+'
+
+test_expect_success 'create history with missing tip commit' '
+	test_tick && git commit --allow-empty -m one &&
+	recoverable=$(git rev-parse HEAD) &&
+	git cat-file commit $recoverable >saved &&
+	test_tick && git commit --allow-empty -m two &&
+	missing=$(git rev-parse HEAD) &&
+	# point HEAD elsewhere
+	git checkout $base &&
+	rm .git/objects/$(echo $missing | sed "s,..,&/,") &&
+	test_must_fail git cat-file -e $missing
+'
+
+test_expect_failure 'pruning with a corrupted tip does not drop history' '
+	test_when_finished "git hash-object -w -t commit saved" &&
+	test_might_fail git prune --expire=now &&
+	verbose git cat-file -e $recoverable
+'
+
+test_expect_success 'pack-refs does not silently delete broken loose ref' '
+	git pack-refs --all --prune &&
+	echo $missing >expect &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+# we do not want to count on running pack-refs to
+# actually pack it, as it is perfectly reasonable to
+# skip processing a broken ref
+test_expect_success 'create packed-refs file with broken ref' '
+	rm -f .git/refs/heads/master &&
+	cat >.git/packed-refs <<-EOF
+	$missing refs/heads/master
+	$recoverable refs/heads/other
+	EOF
+	echo $missing >expect &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pack-refs does not silently delete broken packed ref' '
+	git pack-refs --all --prune &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'pack-refs does not drop broken refs during deletion' '
+	git update-ref -d refs/heads/other &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
2.3.3.520.g3cfbb5d

Most operations that iterate over refs are happy to ignore
broken cruft. However, some operations should be performed
with knowledge of these broken refs, because it is better
for the operation to choke on a missing object than it is to
silently pretend that the ref did not exist (e.g., if we are
computing the set of reachable tips in order to prune
objects).

These processes could just call for_each_rawref, except that
ref iteration is often hidden behind other interfaces. For
instance, for a destructive "repack -ad", we would have to
inform "pack-objects" that we are destructive, and then it
would in turn have to tell the revision code that our
"--all" should include broken refs.

It's much simpler to just set a global for "dangerous"
operations that includes broken refs in all iterations.

Signed-off-by: Jeff King <redacted>
---
I waffled on documenting this for end users. I'm not sure if people
would ever want to actually use the environment variable themselves. The
best hypothetical I could come up with was the:

  git clone --no-local repo.git backup.git &&
  rm -rf repo.git

scenario.

 Documentation/git.txt | 11 +++++++++++
 cache.h               |  8 ++++++++
 environment.c         |  1 +
 refs.c                |  5 +++++
 4 files changed, 25 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index af30620..8da85a6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -1026,6 +1026,17 @@ GIT_ICASE_PATHSPECS::
 	variable when it is invoked as the top level command by the
 	end user, to be recorded in the body of the reflog.
 
+`GIT_REF_PARANOIA`::
+	If set to `1`, include broken or badly named refs when iterating
+	over lists of refs. In a normal, non-corrupted repository, this
+	does nothing. However, enabling it may help git to detect and
+	abort some operations in the presence of broken refs. Git sets
+	this variable automatically when performing destructive
+	operations like linkgit:git-prune[1]. You should not need to set
+	it yourself unless you want to be paranoid about making sure
+	an operation has touched every ref (e.g., because you are
+	cloning a repository to make a backup).
+
 
 Discussion[[Discussion]]
 ------------------------

diff --git a/cache.h b/cache.h
index 761c570..162ea6f 100644
--- a/cache.h
+++ b/cache.h

@@ -614,6 +614,14 @@ extern int protect_hfs;
 extern int protect_ntfs;
 
 /*
+ * Include broken refs in all ref iterations, which will
+ * generally choke dangerous operations rather than letting
+ * them silently proceed without taking the broken ref into
+ * account.
+ */
+extern int ref_paranoia;
+
+/*
  * The character that begins a commented line in user-editable file
  * that is subject to stripspace.
  */

diff --git a/environment.c b/environment.c
index 1ade5c9..a40044c 100644
--- a/environment.c
+++ b/environment.c

@@ -24,6 +24,7 @@ int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
+int ref_paranoia = -1;
 int repository_format_version;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;

diff --git a/refs.c b/refs.c
index e23542b..7f0e7be 100644
--- a/refs.c
+++ b/refs.c

@@ -1934,6 +1934,11 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
 	data.fn = fn;
 	data.cb_data = cb_data;
 
+	if (ref_paranoia < 0)
+		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
+	if (ref_paranoia)
+		data.flags |= DO_FOR_EACH_INCLUDE_BROKEN;
+
 	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 

-- 
2.3.3.520.g3cfbb5d

Prune should know about broken objects at the tips of refs,
so that we can feed them to our traversal rather than
ignoring them. It's better for us to abort the operation on
the broken object than it is to start deleting objects with
an incomplete view of the reachability namespace.

Note that for missing objects, aborting is the best we can
do. For a badly-named ref, we technically could use its sha1
as a reachability tip. However, the iteration code just
feeds us a null sha1, so there would be a reasonable amount
of code involved to pass down our wishes. It's not really
worth trying to do better, because this is a case that
should happen extremely rarely, and the message we provide:

  fatal: unable to parse object: refs/heads/bogus:name

is probably enough to point the user in the right direction.

Signed-off-by: Jeff King <redacted>
---
Note that we should already be aborting for non-tip objects. I guess we
could test that explicitly, too, but I didn't here.

 builtin/prune.c             | 1 +
 t/t5312-prune-corruption.sh | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 04d3b12..17094ad 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c

@@ -115,6 +115,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	expire = ULONG_MAX;
 	save_commit_buffer = 0;
 	check_replace_refs = 0;
+	ref_paranoia = 1;
 	init_revisions(&revs, prefix);
 
 	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 167031e..cccab58 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -25,7 +25,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' '
 	git reset --hard HEAD^
 '
 
-test_expect_failure 'pruning does not drop bogus object' '
+test_expect_success 'pruning does not drop bogus object' '
 	test_when_finished "git hash-object -w -t commit saved" &&
 	test_might_fail git prune --expire=now &&
 	verbose git cat-file -e $bogus

@@ -62,7 +62,7 @@ test_expect_success 'create history with missing tip commit' '
 	test_must_fail git cat-file -e $missing
 '
 
-test_expect_failure 'pruning with a corrupted tip does not drop history' '
+test_expect_success 'pruning with a corrupted tip does not drop history' '
 	test_when_finished "git hash-object -w -t commit saved" &&
 	test_might_fail git prune --expire=now &&
 	verbose git cat-file -e $recoverable

-- 
2.3.3.520.g3cfbb5d

If we are repacking with "-ad", we will drop any unreachable
objects. Likewise, using "-Ad --unpack-unreachable=<time>"
will drop any old, unreachable objects. In these cases, we
want to make sure the reachability we compute with "--all"
is complete. We can do this by passing GIT_REF_PARANOIA=1 in
the environment to pack-objects.

Note that "-Ad" is safe already, because it only loosens
unreachable objects. It is up to "git prune" to avoid
deleting them.

Signed-off-by: Jeff King <redacted>
---
 builtin/repack.c            | 8 ++++++--
 t/t5312-prune-corruption.sh | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 28fbc70..f2edeb0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -228,13 +228,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		get_non_kept_pack_filenames(&existing_packs);
 
 		if (existing_packs.nr && delete_redundant) {
-			if (unpack_unreachable)
+			if (unpack_unreachable) {
 				argv_array_pushf(&cmd.args,
 						"--unpack-unreachable=%s",
 						unpack_unreachable);
-			else if (pack_everything & LOOSEN_UNREACHABLE)
+				argv_array_push(&cmd.env_array, "GIT_REF_PARANOIA=1");
+			} else if (pack_everything & LOOSEN_UNREACHABLE) {
 				argv_array_push(&cmd.args,
 						"--unpack-unreachable");
+			} else {
+				argv_array_push(&cmd.env_array, "GIT_REF_PARANOIA=1");
+			}
 		}
 	} else {
 		argv_array_push(&cmd.args, "--unpacked");

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index cccab58..e3e9994 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -38,7 +38,7 @@ test_expect_success 'put bogus object into pack' '
 	verbose git cat-file -e $bogus
 '
 
-test_expect_failure 'destructive repack keeps packed object' '
+test_expect_success 'destructive repack keeps packed object' '
 	test_might_fail git repack -Ad --unpack-unreachable=now &&
 	verbose git cat-file -e $bogus &&
 	test_might_fail git repack -ad &&

-- 
2.3.3.520.g3cfbb5d

When we delete a ref, we have to rewrite the entire
packed-refs file. We take this opportunity to "curate" the
packed-refs file and drop any entries that are crufty or
broken.

Dropping broken entries (e.g., with bogus names, or ones
that point to missing objects) is actively a bad idea, as it
means that we lose any notion that the data was there in the
first place. Aside from the general hackiness that we might
lose any information about ref "foo" while deleting an
unrelated ref "bar", this may seriously hamper any attempts
by the user at recovering from the corruption in "foo".

They will lose the sha1 and name of "foo"; the exact pointer
may still be useful even if they recover missing objects
from a different copy of the repository. But worse, once the
ref is gone, there is no trace of the corruption. A
follow-up "git prune" may delete objects, even though it
would otherwise bail when seeing corruption.

We could just drop the "broken" bits from
curate_packed_refs, and continue to drop the "crufty" bits:
refs whose loose counterpart exists in the filesystem. This
is not wrong to do, and it does have the advantage that we
may write out a slightly smaller packed-refs file. But it
has two disadvantages:

  1. It is a potential source of races or mistakes with
     respect to these refs that are otherwise unrelated to
     the operation. To my knowledge, there aren't any active
     problems in this area, but it seems like an unnecessary
     risk.

  2. We have to spend time looking up the matching loose
     refs for every item in the packed-refs file. If you
     have a large number of packed refs that do not change,
     that outweights the benefit from writing out a smaller
     packed-refs file (it doesn't get smaller, and you do a
     bunch of directory traversal to find that out).

Signed-off-by: Jeff King <redacted>
---
I'll admit my argument against curate_packed_refs is a bit hand-wavy. I
won't be _too_ sad if somebody insists on cutting this back to just
keeping "broken" refs around, and still curating the "crufty" ones.

 refs.c                      | 67 +--------------------------------------------
 t/t5312-prune-corruption.sh |  2 +-
 2 files changed, 2 insertions(+), 67 deletions(-)

diff --git a/refs.c b/refs.c
index 7f0e7be..47e4e53 100644
--- a/refs.c
+++ b/refs.c

@@ -2621,68 +2621,10 @@ int pack_refs(unsigned int flags)
 	return 0;
 }
 
-/*
- * If entry is no longer needed in packed-refs, add it to the string
- * list pointed to by cb_data.  Reasons for deleting entries:
- *
- * - Entry is broken.
- * - Entry is overridden by a loose ref.
- * - Entry does not point at a valid object.
- *
- * In the first and third cases, also emit an error message because these
- * are indications of repository corruption.
- */
-static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
-{
-	struct string_list *refs_to_delete = cb_data;
-
-	if (entry->flag & REF_ISBROKEN) {
-		/* This shouldn't happen to packed refs. */
-		error("%s is broken!", entry->name);
-		string_list_append(refs_to_delete, entry->name);
-		return 0;
-	}
-	if (!has_sha1_file(entry->u.value.sha1)) {
-		unsigned char sha1[20];
-		int flags;
-
-		if (read_ref_full(entry->name, 0, sha1, &flags))
-			/* We should at least have found the packed ref. */
-			die("Internal error");
-		if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) {
-			/*
-			 * This packed reference is overridden by a
-			 * loose reference, so it is OK that its value
-			 * is no longer valid; for example, it might
-			 * refer to an object that has been garbage
-			 * collected.  For this purpose we don't even
-			 * care whether the loose reference itself is
-			 * invalid, broken, symbolic, etc.  Silently
-			 * remove the packed reference.
-			 */
-			string_list_append(refs_to_delete, entry->name);
-			return 0;
-		}
-		/*
-		 * There is no overriding loose reference, so the fact
-		 * that this reference doesn't refer to a valid object
-		 * indicates some kind of repository corruption.
-		 * Report the problem, then omit the reference from
-		 * the output.
-		 */
-		error("%s does not point to a valid object!", entry->name);
-		string_list_append(refs_to_delete, entry->name);
-		return 0;
-	}
-
-	return 0;
-}
-
 int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 {
 	struct ref_dir *packed;
-	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
-	struct string_list_item *refname, *ref_to_delete;
+	struct string_list_item *refname;
 	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);

@@ -2718,13 +2660,6 @@ int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 		return 0;
 	}
 
-	/* Remove any other accumulated cruft */
-	do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);
-	for_each_string_list_item(ref_to_delete, &refs_to_delete) {
-		if (remove_entry(packed, ref_to_delete->string) == -1)
-			die("internal error");
-	}
-
 	/* Write what remains */
 	ret = commit_packed_refs();
 	if (ret)

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index e3e9994..8b54d16 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -95,7 +95,7 @@ test_expect_success 'pack-refs does not silently delete broken packed ref' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'pack-refs does not drop broken refs during deletion' '
+test_expect_success 'pack-refs does not drop broken refs during deletion' '
 	git update-ref -d refs/heads/other &&
 	git rev-parse refs/heads/master >actual &&
 	test_cmp expect actual

-- 
2.3.3.520.g3cfbb5d

On Tue, Mar 17, 2015 at 03:27:50AM -0400, Jeff King wrote:

The general strategy for these is to use for_each_rawref traversals in
these situations. That doesn't cover _every_ possible scenario. For
example, you could do:

  git clone --no-local repo.git backup.git &&
  rm -rf repo.git

and you might be disappointed if "backup.git" omitted some broken refs
(upload-pack will simply skip the broken refs in its advertisement).  We
could tighten this, but then it becomes hard to access slightly broken
repositories (e.g., you might prefer to clone what you can, and not have
git die() when it tries to serve the breakage). Patch 2 provides a
tweakable safety valve for this.

One thing I thought about while working on this was whether we should
just make _all_ ref iterations for_each_rawref. The benefit to not doing
so in the hypothetical above is that you might be able to clone "foo"
even if "bar" is broken.

But it strikes me as weird that we consider the _tips_ of history to be
special for ignoring breakage. If the tip of "bar" is broken, we omit
it. But if the tip is fine, and there's breakage three commits down in
the history, then doing a clone is going to fail horribly, as
pack-objects realizes it can't generate the pack. So in practice, I'm
not sure how much you're buying with the "don't mention broken refs"
code.

OTOH, there are probably _some_ situations that can be recovered with
the current code that could not otherwise. For example, in the current
code, I can still fetch "foo" even if "bar" is broken 3 commits down.
Whereas if the tip is broken, there's a reasonable chance that
"upload-pack" would just barf and I could fetch nothing.

So I stuck to the status quo in most cases, and only turned on the more
aggressive behavior for destructive operations (and people who want to
go wild can set GIT_REF_PARANOIA=1 for their every day operations if
they want to).

-Peff

Am 17.03.2015 um 08:28 schrieb Jeff King:

+test_expect_success 'create history reachable only from a bogus-named ref' '
+	test_tick && git commit --allow-empty -m master &&
+	base=$(git rev-parse HEAD) &&
+	test_tick && git commit --allow-empty -m bogus &&
+	bogus=$(git rev-parse HEAD) &&
+	git cat-file commit $bogus >saved &&
+	echo $bogus >.git/refs/heads/bogus:name &&

This causes headaches on Windows: It creates an empty file, named 
"bogus", with all the data diverted to the alternate data stream named 
"name". Needless to say that this...

+test_expect_success 'clean up bogus ref' '
+	rm .git/refs/heads/bogus:name
+'

does not remove the file "bogus", but only the alternate data stream (if 
at all---I forgot to check). How about .git/refs/heads/bogus..nam.e?

-- Hannes

On Tue, Mar 17, 2015 at 07:34:02PM +0100, Johannes Sixt wrote:

Am 17.03.2015 um 08:28 schrieb Jeff King:

quoted

+test_expect_success 'create history reachable only from a bogus-named ref' '
+	test_tick && git commit --allow-empty -m master &&
+	base=$(git rev-parse HEAD) &&
+	test_tick && git commit --allow-empty -m bogus &&
+	bogus=$(git rev-parse HEAD) &&
+	git cat-file commit $bogus >saved &&
+	echo $bogus >.git/refs/heads/bogus:name &&

This causes headaches on Windows: It creates an empty file, named "bogus",
with all the data diverted to the alternate data stream named "name".
Needless to say that this...

Ah, yes. Windows. Our usual workaround would be to put it straight into
packed-refs, but in this case, the test really does need the badly named
ref in the file system. But...

quoted

+test_expect_success 'clean up bogus ref' '
+	rm .git/refs/heads/bogus:name
+'

does not remove the file "bogus", but only the alternate data stream (if at
all---I forgot to check). How about .git/refs/heads/bogus..nam.e?

Yes, that works. The colon is what originally brought my attention to
this case, but anything that fails git-check-ref-format is fine. I've
squashed this in:

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 167031e..1001a69 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -21,7 +21,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' '
 	test_tick && git commit --allow-empty -m bogus &&
 	bogus=$(git rev-parse HEAD) &&
 	git cat-file commit $bogus >saved &&
-	echo $bogus >.git/refs/heads/bogus:name &&
+	echo $bogus >.git/refs/heads/bogus..name &&
 	git reset --hard HEAD^
 '
 

@@ -47,7 +47,7 @@ test_expect_failure 'destructive repack keeps packed object' '
 
 # subsequent tests will have different corruptions
 test_expect_success 'clean up bogus ref' '
-	rm .git/refs/heads/bogus:name
+	rm .git/refs/heads/bogus..name
 '
 
 test_expect_success 'create history with missing tip commit' '

I assumed the final "." in your example wasn't significant (it is not to
git), but let me know if I've run afoul of another weird restriction. :)

Thanks.

-Peff

Am 17.03.2015 um 19:55 schrieb Jeff King:

+	echo $bogus >.git/refs/heads/bogus..name &&
...
I assumed the final "." in your example wasn't significant (it is not to
git), but let me know if I've run afoul of another weird restriction. :)

It was actually deliberate (with intents too complicated to explain), 
but it turns out not to be required. Your updated test case is good.

-- Hannes

On Tue, Mar 17, 2015 at 3:28 AM, Jeff King [off-list ref] wrote:

Broken &&-chain.

+       $missing refs/heads/master
+       $recoverable refs/heads/other
+       EOF
+       echo $missing >expect &&
+       git rev-parse refs/heads/master >actual &&
+       test_cmp expect actual
+'

On Tue, Mar 17, 2015 at 3:31 AM, Jeff King [off-list ref] wrote:

When we delete a ref, we have to rewrite the entire
packed-refs file. We take this opportunity to "curate" the
packed-refs file and drop any entries that are crufty or
broken.

Dropping broken entries (e.g., with bogus names, or ones
that point to missing objects) is actively a bad idea, as it
means that we lose any notion that the data was there in the
first place. Aside from the general hackiness that we might
lose any information about ref "foo" while deleting an
unrelated ref "bar", this may seriously hamper any attempts
by the user at recovering from the corruption in "foo".

They will lose the sha1 and name of "foo"; the exact pointer
may still be useful even if they recover missing objects
from a different copy of the repository. But worse, once the
ref is gone, there is no trace of the corruption. A
follow-up "git prune" may delete objects, even though it
would otherwise bail when seeing corruption.

We could just drop the "broken" bits from
curate_packed_refs, and continue to drop the "crufty" bits:
refs whose loose counterpart exists in the filesystem. This
is not wrong to do, and it does have the advantage that we
may write out a slightly smaller packed-refs file. But it
has two disadvantages:

  1. It is a potential source of races or mistakes with
     respect to these refs that are otherwise unrelated to
     the operation. To my knowledge, there aren't any active
     problems in this area, but it seems like an unnecessary
     risk.

  2. We have to spend time looking up the matching loose
     refs for every item in the packed-refs file. If you
     have a large number of packed refs that do not change,
     that outweights the benefit from writing out a smaller

s/outweights/outweighs/

     packed-refs file (it doesn't get smaller, and you do a
     bunch of directory traversal to find that out).

Signed-off-by: Jeff King <redacted>

On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote:

quoted

--- /dev/null
+++ b/t/t5312-prune-corruption.sh

@@ -0,0 +1,104 @@
+# we do not want to count on running pack-refs to
+# actually pack it, as it is perfectly reasonable to
+# skip processing a broken ref
+test_expect_success 'create packed-refs file with broken ref' '
+       rm -f .git/refs/heads/master &&
+       cat >.git/packed-refs <<-EOF

Broken &&-chain.

Thanks. I notice that a large number of broken &&-chains are on
here-docs. I really wish you could put the && on the "EOF" line at the
end of the here-doc. I understand _why_ that this not the case, but
mentally it is where I want to type it, and I obviously sometimes fail
to go back and fix it. I don't think there's a better solution in POSIX
sh, though.

-Peff

On Thu, Mar 19, 2015 at 9:32 PM, Jeff King [off-list ref] wrote:

On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote:

quoted

quoted

--- /dev/null
+++ b/t/t5312-prune-corruption.sh

@@ -0,0 +1,104 @@
+# we do not want to count on running pack-refs to
+# actually pack it, as it is perfectly reasonable to
+# skip processing a broken ref
+test_expect_success 'create packed-refs file with broken ref' '
+       rm -f .git/refs/heads/master &&
+       cat >.git/packed-refs <<-EOF

Broken &&-chain.

Thanks. I notice that a large number of broken &&-chains are on
here-docs. I really wish you could put the && on the "EOF" line at the
end of the here-doc. I understand _why_ that this not the case, but
mentally it is where I want to type it, and I obviously sometimes fail
to go back and fix it. I don't think there's a better solution in POSIX
sh, though.

I wonder if test-lint could be enhanced to detect this sort of problem?

On Thu, Mar 19, 2015 at 09:37:12PM -0400, Eric Sunshine wrote:

quoted

Thanks. I notice that a large number of broken &&-chains are on
here-docs. I really wish you could put the && on the "EOF" line at the
end of the here-doc. I understand _why_ that this not the case, but
mentally it is where I want to type it, and I obviously sometimes fail
to go back and fix it. I don't think there's a better solution in POSIX
sh, though.

I wonder if test-lint could be enhanced to detect this sort of problem?

That would be nice, but it's complicated. A naive:

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc..3a6d8d8 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl

@@ -22,6 +22,7 @@ while (<>) {
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
 	/\btest\s+[^=]*==/ and err '"test a == b" is not portable (please use =)';
 	/\bexport\s+[A-Za-z0-9_]*=/ and err '"export FOO=bar" is not portable (please use FOO=bar && export FOO)';
+	/ <<-?.?EOF(.*)/ && $1 !~ /&&/ and err 'here-doc with broken &&-chain';
 	# this resets our $. for each file
 	close ARGV if eof;
 }

yields quite a few false positives, because of course we don't know
which are meant to be at the end of the chain and which are not. And
finding that out is tough. We'd have to actually parse to the end of
the here-doc ourselves, then see if it was the end of the test_expect
block.

I think it would be simpler to ask the shell to check this for us, like:

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -524,6 +524,21 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
+
+	if test -n "$GIT_TEST_CHAIN_LINT"; then
+		# 117 is unlikely to match the exit code of
+		# another part of the chain
+		test_eval_ "(exit 117) && $1"
+		if test "$?" != 117; then
+			# all bets are off for continuing with other tests;
+			# we expected none of the rest of the test commands to
+			# run, but at least some did. Who knows what weird
+			# state we're in? Just bail, and the user can diagnose
+			# by running in --verbose mode
+			error "bug in the test script: broken &&-chain"
+		fi
+	fi
+
 	setup_malloc_check
 	test_eval_ "$1"
 	eval_ret=$?

This turns up an appalling number of failures, but AFAICT they are all
"real" in the sense that the &&-chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.

I seem to recall people looked at doing this sort of lint a while ago,
but we never ended up committing anything. I wonder if it was because of
all of these "false positives".

diff --git a/t/t3010-ls-files-killed-modified.sh b/t/t3010-ls-files-killed-modified.sh
index 6d3b828..62fce10 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh

@@ -62,7 +62,7 @@ test_expect_success 'git update-index --add to add various paths.' '
 			cd submod$i && git commit --allow-empty -m "empty $i"
 		) || break
 	done &&
-	git update-index --add submod[12]
+	git update-index --add submod[12] &&
 	(
 		cd submod1 &&
 		git commit --allow-empty -m "empty 1 (updated)"

@@ -99,12 +99,12 @@ test_expect_success 'git ls-files -k to show killed files.' '
 '
 
 test_expect_success 'git ls-files -k output (w/o icase)' '
-	git ls-files -k >.output
+	git ls-files -k >.output &&
 	test_cmp .expected .output
 '
 
 test_expect_success 'git ls-files -k output (w/ icase)' '
-	git -c core.ignorecase=true ls-files -k >.output
+	git -c core.ignorecase=true ls-files -k >.output &&
 	test_cmp .expected .output
 '
 

-Peff

[+cc Jonathan, whose patch I apparently subconsciously copied]

On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:

This turns out to be rather annoying to grep for in the list archives,
but I found at least one discussion:

  http://article.gmane.org/gmane.comp.version-control.git/235913

I don't know why we didn't follow it up then. Perhaps because the patch
there (which is rather similar to what I have above) was not
conditional, so whole chunks of the test suite needed fixing. There are
enough problems that we would probably want to do this conditionally,
fix them over time, and then finally flip the feature on by default.

-Peff

On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:

quoted

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -524,6 +524,21 @@ test_eval_ () {
 test_run_ () {
 	test_cleanup=:
 	expecting_failure=$2
+
+	if test -n "$GIT_TEST_CHAIN_LINT"; then
+		# 117 is unlikely to match the exit code of
+		# another part of the chain
+		test_eval_ "(exit 117) && $1"
+		if test "$?" != 117; then
+			# all bets are off for continuing with other tests;
+			# we expected none of the rest of the test commands to
+			# run, but at least some did. Who knows what weird
+			# state we're in? Just bail, and the user can diagnose
+			# by running in --verbose mode
+			error "bug in the test script: broken &&-chain"
+		fi
+	fi
+
 	setup_malloc_check
 	test_eval_ "$1"
 	eval_ret=$?

This turns up an appalling number of failures, but AFAICT they are all
"real" in the sense that the &&-chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.

FWIW, I have spent about a few hours wading through the errors, and am
about 75% done. There are definitely some broken chains that were
causing test results to be ignored (as opposed to just minor setup steps
that we would not expect to fail). In most cases, the tests do passed. I
have a few that I still need to examine more closely, but there may be
some where there are actual test failures (but it's possible that I just
screwed it up while fixing the &&-chaining).

I hope to post something tonight, but I wanted to drop a note on the off
chance that you were actively looking at it at the same time.

-Peff

On Fri, Mar 20, 2015 at 1:10 AM, Jeff King [off-list ref] wrote:

On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:

quoted

quoted

diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -524,6 +524,21 @@ test_eval_ () {
 test_run_ () {
+   if test -n "$GIT_TEST_CHAIN_LINT"; then
+           # 117 is unlikely to match the exit code of
+           # another part of the chain
+           test_eval_ "(exit 117) && $1"
+           if test "$?" != 117; then
+                   # all bets are off for continuing with other tests;
+                   # we expected none of the rest of the test commands to
+                   # run, but at least some did. Who knows what weird
+                   # state we're in? Just bail, and the user can diagnose
+                   # by running in --verbose mode
+                   error "bug in the test script: broken &&-chain"
+           fi
+   fi

Clever (Jonathan's too); much nicer than trying to special case only here-doc.

quoted

quoted

This turns up an appalling number of failures, but AFAICT they are all
"real" in the sense that the &&-chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.

FWIW, I have spent about a few hours wading through the errors, and am
about 75% done. There are definitely some broken chains that were
causing test results to be ignored (as opposed to just minor setup steps
that we would not expect to fail). In most cases, the tests do passed. I
have a few that I still need to examine more closely, but there may be
some where there are actual test failures (but it's possible that I just
screwed it up while fixing the &&-chaining).

I hope to post something tonight, but I wanted to drop a note on the off
chance that you were actively looking at it at the same time.

Thanks for working on this. It looks like this technique should be a
valuable addition to test-lint. (I had intended, but haven't yet found
time to dig into it, so I'm happy to hear of your progress.)

When we delete a ref, we have to rewrite the entire
packed-refs file. We take this opportunity to "curate" the
packed-refs file and drop any entries that are crufty or
broken.

Dropping broken entries (e.g., with bogus names, or ones
that point to missing objects) is actively a bad idea, as it
means that we lose any notion that the data was there in the
first place. Aside from the general hackiness that we might
lose any information about ref "foo" while deleting an
unrelated ref "bar", this may seriously hamper any attempts
by the user at recovering from the corruption in "foo".

They will lose the sha1 and name of "foo"; the exact pointer
may still be useful even if they recover missing objects
from a different copy of the repository. But worse, once the
ref is gone, there is no trace of the corruption. A
follow-up "git prune" may delete objects, even though it
would otherwise bail when seeing corruption.

We could just drop the "broken" bits from
curate_packed_refs, and continue to drop the "crufty" bits:
refs whose loose counterpart exists in the filesystem. This
is not wrong to do, and it does have the advantage that we
may write out a slightly smaller packed-refs file. But it
has two disadvantages:

  1. It is a potential source of races or mistakes with
     respect to these refs that are otherwise unrelated to
     the operation. To my knowledge, there aren't any active
     problems in this area, but it seems like an unnecessary
     risk.

  2. We have to spend time looking up the matching loose
     refs for every item in the packed-refs file. If you
     have a large number of packed refs that do not change,
     that outweighs the benefit from writing out a smaller
     packed-refs file (it doesn't get smaller, and you do a
     bunch of directory traversal to find that out).

Signed-off-by: Jeff King <redacted>
---
 refs.c                      | 67 +--------------------------------------------
 t/t5312-prune-corruption.sh |  2 +-
 2 files changed, 2 insertions(+), 67 deletions(-)

diff --git a/refs.c b/refs.c
index 7f0e7be..47e4e53 100644
--- a/refs.c
+++ b/refs.c

@@ -2621,68 +2621,10 @@ int pack_refs(unsigned int flags)
 	return 0;
 }
 
-/*
- * If entry is no longer needed in packed-refs, add it to the string
- * list pointed to by cb_data.  Reasons for deleting entries:
- *
- * - Entry is broken.
- * - Entry is overridden by a loose ref.
- * - Entry does not point at a valid object.
- *
- * In the first and third cases, also emit an error message because these
- * are indications of repository corruption.
- */
-static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data)
-{
-	struct string_list *refs_to_delete = cb_data;
-
-	if (entry->flag & REF_ISBROKEN) {
-		/* This shouldn't happen to packed refs. */
-		error("%s is broken!", entry->name);
-		string_list_append(refs_to_delete, entry->name);
-		return 0;
-	}
-	if (!has_sha1_file(entry->u.value.sha1)) {
-		unsigned char sha1[20];
-		int flags;
-
-		if (read_ref_full(entry->name, 0, sha1, &flags))
-			/* We should at least have found the packed ref. */
-			die("Internal error");
-		if ((flags & REF_ISSYMREF) || !(flags & REF_ISPACKED)) {
-			/*
-			 * This packed reference is overridden by a
-			 * loose reference, so it is OK that its value
-			 * is no longer valid; for example, it might
-			 * refer to an object that has been garbage
-			 * collected.  For this purpose we don't even
-			 * care whether the loose reference itself is
-			 * invalid, broken, symbolic, etc.  Silently
-			 * remove the packed reference.
-			 */
-			string_list_append(refs_to_delete, entry->name);
-			return 0;
-		}
-		/*
-		 * There is no overriding loose reference, so the fact
-		 * that this reference doesn't refer to a valid object
-		 * indicates some kind of repository corruption.
-		 * Report the problem, then omit the reference from
-		 * the output.
-		 */
-		error("%s does not point to a valid object!", entry->name);
-		string_list_append(refs_to_delete, entry->name);
-		return 0;
-	}
-
-	return 0;
-}
-
 int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 {
 	struct ref_dir *packed;
-	struct string_list refs_to_delete = STRING_LIST_INIT_DUP;
-	struct string_list_item *refname, *ref_to_delete;
+	struct string_list_item *refname;
 	int ret, needs_repacking = 0, removed = 0;
 
 	assert(err);

@@ -2718,13 +2660,6 @@ int repack_without_refs(struct string_list *refnames, struct strbuf *err)
 		return 0;
 	}
 
-	/* Remove any other accumulated cruft */
-	do_for_each_entry_in_dir(packed, 0, curate_packed_ref_fn, &refs_to_delete);
-	for_each_string_list_item(ref_to_delete, &refs_to_delete) {
-		if (remove_entry(packed, ref_to_delete->string) == -1)
-			die("internal error");
-	}
-
 	/* Write what remains */
 	ret = commit_packed_refs();
 	if (ret)

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index e8d04ef..8e98b44 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -105,7 +105,7 @@ test_expect_success 'pack-refs does not silently delete broken packed ref' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'pack-refs does not drop broken refs during deletion' '
+test_expect_success 'pack-refs does not drop broken refs during deletion' '
 	git update-ref -d refs/heads/other &&
 	git rev-parse refs/heads/master >actual &&
 	test_cmp expect actual

-- 
2.3.3.520.g3cfbb5d

If we are repacking with "-ad", we will drop any unreachable
objects. Likewise, using "-Ad --unpack-unreachable=<time>"
will drop any old, unreachable objects. In these cases, we
want to make sure the reachability we compute with "--all"
is complete. We can do this by passing GIT_REF_PARANOIA=1 in
the environment to pack-objects.

Note that "-Ad" is safe already, because it only loosens
unreachable objects. It is up to "git prune" to avoid
deleting them.

Signed-off-by: Jeff King <redacted>
---
 builtin/repack.c            | 8 ++++++--
 t/t5312-prune-corruption.sh | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 28fbc70..f2edeb0 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c

@@ -228,13 +228,17 @@ int cmd_repack(int argc, const char **argv, const char *prefix)
 		get_non_kept_pack_filenames(&existing_packs);
 
 		if (existing_packs.nr && delete_redundant) {
-			if (unpack_unreachable)
+			if (unpack_unreachable) {
 				argv_array_pushf(&cmd.args,
 						"--unpack-unreachable=%s",
 						unpack_unreachable);
-			else if (pack_everything & LOOSEN_UNREACHABLE)
+				argv_array_push(&cmd.env_array, "GIT_REF_PARANOIA=1");
+			} else if (pack_everything & LOOSEN_UNREACHABLE) {
 				argv_array_push(&cmd.args,
 						"--unpack-unreachable");
+			} else {
+				argv_array_push(&cmd.env_array, "GIT_REF_PARANOIA=1");
+			}
 		}
 	} else {
 		argv_array_push(&cmd.args, "--unpacked");

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 5ffb817..e8d04ef 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -38,7 +38,7 @@ test_expect_success 'put bogus object into pack' '
 	verbose git cat-file -e $bogus
 '
 
-test_expect_failure 'destructive repack keeps packed object' '
+test_expect_success 'destructive repack keeps packed object' '
 	test_might_fail git repack -Ad --unpack-unreachable=now &&
 	verbose git cat-file -e $bogus &&
 	test_might_fail git repack -ad &&

-- 
2.3.3.520.g3cfbb5d

Prune should know about broken objects at the tips of refs,
so that we can feed them to our traversal rather than
ignoring them. It's better for us to abort the operation on
the broken object than it is to start deleting objects with
an incomplete view of the reachability namespace.

Note that for missing objects, aborting is the best we can
do. For a badly-named ref, we technically could use its sha1
as a reachability tip. However, the iteration code just
feeds us a null sha1, so there would be a reasonable amount
of code involved to pass down our wishes. It's not really
worth trying to do better, because this is a case that
should happen extremely rarely, and the message we provide:

  fatal: unable to parse object: refs/heads/bogus:name

is probably enough to point the user in the right direction.

Signed-off-by: Jeff King <redacted>
---
 builtin/prune.c             | 1 +
 t/t5312-prune-corruption.sh | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/prune.c b/builtin/prune.c
index 04d3b12..17094ad 100644
--- a/builtin/prune.c
+++ b/builtin/prune.c

@@ -115,6 +115,7 @@ int cmd_prune(int argc, const char **argv, const char *prefix)
 	expire = ULONG_MAX;
 	save_commit_buffer = 0;
 	check_replace_refs = 0;
+	ref_paranoia = 1;
 	init_revisions(&revs, prefix);
 
 	argc = parse_options(argc, argv, prefix, options, prune_usage, 0);

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 496a9f5..5ffb817 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh

@@ -25,7 +25,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' '
 	git reset --hard HEAD^
 '
 
-test_expect_failure 'pruning does not drop bogus object' '
+test_expect_success 'pruning does not drop bogus object' '
 	test_when_finished "git hash-object -w -t commit saved" &&
 	test_might_fail git prune --expire=now &&
 	verbose git cat-file -e $bogus

@@ -72,7 +72,7 @@ test_expect_success 'create history with missing tip commit' '
 	test_must_fail git cat-file -e $missing
 '
 
-test_expect_failure 'pruning with a corrupted tip does not drop history' '
+test_expect_success 'pruning with a corrupted tip does not drop history' '
 	test_when_finished "git hash-object -w -t commit saved" &&
 	test_might_fail git prune --expire=now &&
 	verbose git cat-file -e $recoverable

-- 
2.3.3.520.g3cfbb5d

Most operations that iterate over refs are happy to ignore
broken cruft. However, some operations should be performed
with knowledge of these broken refs, because it is better
for the operation to choke on a missing object than it is to
silently pretend that the ref did not exist (e.g., if we are
computing the set of reachable tips in order to prune
objects).

These processes could just call for_each_rawref, except that
ref iteration is often hidden behind other interfaces. For
instance, for a destructive "repack -ad", we would have to
inform "pack-objects" that we are destructive, and then it
would in turn have to tell the revision code that our
"--all" should include broken refs.

It's much simpler to just set a global for "dangerous"
operations that includes broken refs in all iterations.

Signed-off-by: Jeff King <redacted>
---
 Documentation/git.txt | 11 +++++++++++
 cache.h               |  8 ++++++++
 environment.c         |  1 +
 refs.c                |  5 +++++
 4 files changed, 25 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index af30620..8da85a6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt

@@ -1026,6 +1026,17 @@ GIT_ICASE_PATHSPECS::
 	variable when it is invoked as the top level command by the
 	end user, to be recorded in the body of the reflog.
 
+`GIT_REF_PARANOIA`::
+	If set to `1`, include broken or badly named refs when iterating
+	over lists of refs. In a normal, non-corrupted repository, this
+	does nothing. However, enabling it may help git to detect and
+	abort some operations in the presence of broken refs. Git sets
+	this variable automatically when performing destructive
+	operations like linkgit:git-prune[1]. You should not need to set
+	it yourself unless you want to be paranoid about making sure
+	an operation has touched every ref (e.g., because you are
+	cloning a repository to make a backup).
+
 
 Discussion[[Discussion]]
 ------------------------

diff --git a/cache.h b/cache.h
index 761c570..162ea6f 100644
--- a/cache.h
+++ b/cache.h

@@ -614,6 +614,14 @@ extern int protect_hfs;
 extern int protect_ntfs;
 
 /*
+ * Include broken refs in all ref iterations, which will
+ * generally choke dangerous operations rather than letting
+ * them silently proceed without taking the broken ref into
+ * account.
+ */
+extern int ref_paranoia;
+
+/*
  * The character that begins a commented line in user-editable file
  * that is subject to stripspace.
  */

diff --git a/environment.c b/environment.c
index 1ade5c9..a40044c 100644
--- a/environment.c
+++ b/environment.c

@@ -24,6 +24,7 @@ int is_bare_repository_cfg = -1; /* unspecified */
 int log_all_ref_updates = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
+int ref_paranoia = -1;
 int repository_format_version;
 const char *git_commit_encoding;
 const char *git_log_output_encoding;

diff --git a/refs.c b/refs.c
index e23542b..7f0e7be 100644
--- a/refs.c
+++ b/refs.c

@@ -1934,6 +1934,11 @@ static int do_for_each_ref(struct ref_cache *refs, const char *base,
 	data.fn = fn;
 	data.cb_data = cb_data;
 
+	if (ref_paranoia < 0)
+		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
+	if (ref_paranoia)
+		data.flags |= DO_FOR_EACH_INCLUDE_BROKEN;
+
 	return do_for_each_entry(refs, base, do_one_ref, &data);
 }
 

-- 
2.3.3.520.g3cfbb5d

When we are doing a destructive operation like "git prune",
we want to be extra careful that the set of reachable tips
we compute is valid. If there is any corruption or oddity,
we are better off aborting the operation and letting the
user figure things out rather than plowing ahead and
possibly deleting some data that cannot be recovered.

The tests here include:

  1. Pruning objects mentioned only be refs with invalid
     names. This used to abort prior to d0f810f (refs.c:
     allow listing and deleting badly named refs,
     2014-09-03), but since then we silently ignore the tip.

     Likewise, we test repacking that can drop objects
     (either "-ad", which drops anything unreachable,
     or "-Ad --unpack-unreachable=<time>", which tries to
     optimize out a loose object write that would be
     directly pruned).

  2. Pruning objects when some refs point to missing
     objects. We don't know whether any dangling objects
     would have been reachable from the missing objects. We
     are better to keep them around, as they are better than
     nothing for helping the user recover history.

  3. Packed refs that point to missing objects can sometimes
     be dropped. By itself, this is more of an annoyance
     (you do not have the object anyway; even if you can
     recover it from elsewhere, all you are losing is a
     placeholder for your state at the time of corruption).
     But coupled with (2), if we drop the ref and then go
     on to prune, we may lose unrecoverable objects.

Note that we use test_might_fail for some of the operations.
In some cases, it would be appropriate to abort the
operation, and in others, it might be acceptable to continue
but taking the information into account. The tests don't
care either way, and check only for data loss.

Signed-off-by: Jeff King <redacted>
---
 t/t5312-prune-corruption.sh | 114 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 114 insertions(+)
 create mode 100755 t/t5312-prune-corruption.sh

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
new file mode 100755
index 0000000..496a9f5
--- /dev/null
+++ b/t/t5312-prune-corruption.sh

@@ -0,0 +1,114 @@
+#!/bin/sh
+
+test_description='
+Test pruning of repositories with minor corruptions. The goal
+here is that we should always be erring on the side of safety. So
+if we see, for example, a ref with a bogus name, it is OK either to
+bail out or to proceed using it as a reachable tip, but it is _not_
+OK to proceed as if it did not exist. Otherwise we might silently
+delete objects that cannot be recovered.
+'
+. ./test-lib.sh
+
+test_expect_success 'disable reflogs' '
+	git config core.logallrefupdates false &&
+	rm -rf .git/logs
+'
+
+test_expect_success 'create history reachable only from a bogus-named ref' '
+	test_tick && git commit --allow-empty -m master &&
+	base=$(git rev-parse HEAD) &&
+	test_tick && git commit --allow-empty -m bogus &&
+	bogus=$(git rev-parse HEAD) &&
+	git cat-file commit $bogus >saved &&
+	echo $bogus >.git/refs/heads/bogus..name &&
+	git reset --hard HEAD^
+'
+
+test_expect_failure 'pruning does not drop bogus object' '
+	test_when_finished "git hash-object -w -t commit saved" &&
+	test_might_fail git prune --expire=now &&
+	verbose git cat-file -e $bogus
+'
+
+test_expect_success 'put bogus object into pack' '
+	git tag reachable $bogus &&
+	git repack -ad &&
+	git tag -d reachable &&
+	verbose git cat-file -e $bogus
+'
+
+test_expect_failure 'destructive repack keeps packed object' '
+	test_might_fail git repack -Ad --unpack-unreachable=now &&
+	verbose git cat-file -e $bogus &&
+	test_might_fail git repack -ad &&
+	verbose git cat-file -e $bogus
+'
+
+# subsequent tests will have different corruptions
+test_expect_success 'clean up bogus ref' '
+	rm .git/refs/heads/bogus..name
+'
+
+# We create two new objects here, "one" and "two". Our
+# master branch points to "two", which is deleted,
+# corrupting the repository. But we'd like to make sure
+# that the otherwise unreachable "one" is not pruned
+# (since it is the user's best bet for recovering
+# from the corruption).
+#
+# Note that we also point HEAD somewhere besides "two",
+# as we want to make sure we test the case where we
+# pick up the reference to "two" by iterating the refs,
+# not by resolving HEAD.
+test_expect_success 'create history with missing tip commit' '
+	test_tick && git commit --allow-empty -m one &&
+	recoverable=$(git rev-parse HEAD) &&
+	git cat-file commit $recoverable >saved &&
+	test_tick && git commit --allow-empty -m two &&
+	missing=$(git rev-parse HEAD) &&
+	git checkout --detach $base &&
+	rm .git/objects/$(echo $missing | sed "s,..,&/,") &&
+	test_must_fail git cat-file -e $missing
+'
+
+test_expect_failure 'pruning with a corrupted tip does not drop history' '
+	test_when_finished "git hash-object -w -t commit saved" &&
+	test_might_fail git prune --expire=now &&
+	verbose git cat-file -e $recoverable
+'
+
+test_expect_success 'pack-refs does not silently delete broken loose ref' '
+	git pack-refs --all --prune &&
+	echo $missing >expect &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+# we do not want to count on running pack-refs to
+# actually pack it, as it is perfectly reasonable to
+# skip processing a broken ref
+test_expect_success 'create packed-refs file with broken ref' '
+	rm -f .git/refs/heads/master &&
+	cat >.git/packed-refs <<-EOF &&
+	$missing refs/heads/master
+	$recoverable refs/heads/other
+	EOF
+	echo $missing >expect &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'pack-refs does not silently delete broken packed ref' '
+	git pack-refs --all --prune &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'pack-refs does not drop broken refs during deletion' '
+	git update-ref -d refs/heads/other &&
+	git rev-parse refs/heads/master >actual &&
+	test_cmp expect actual
+'
+
+test_done

-- 
2.3.3.520.g3cfbb5d

This is a re-roll of the series to make "git prune" in a corrupted
repository safer.

There are only minor tweaks from v1, but I think all of the raised
issues were addressed (there was discussion on some other points, but I
think they are OK as-is; more discussion is of course welcome).

The changes from v1 are:

  - use "bogus..name" as a bad refname instead of "bogus:name", as the
    latter has problems on Windows

  - fix broken &&-chains in tests

  - better commenting of missing-commit setup in t5312

  - typo-fixes in commit messages

Patches:

  [1/5]: t5312: test object deletion code paths in a corrupted repository
  [2/5]: refs: introduce a "ref paranoia" flag
  [3/5]: prune: turn on ref_paranoia flag
  [4/5]: repack: turn on "ref paranoia" when doing a destructive repack
  [5/5]: refs.c: drop curate_packed_refs

-Peff