[PATCH 0/5] not making corruption worse

[PATCH 0/5] not making corruption worse

[Jeff King]

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

[PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

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 <peff@peff.net>
---
 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

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Johannes Sixt]

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

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

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

> 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...

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...

> >+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

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Johannes Sixt]

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

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +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 &&

Could you spell this as "$base^0" (or "--detach") to clarify the
intention?  I have been scraching my head for a few minutes just
now, trying to figure out what you are doing here.  I _think_ you
wanted master to point at the missing "two" and wanted to make sure
all other refs (including HEAD) to point away from it.

Mental note: At this point, the history looks like

    base   one    two
    o------o------o
     \
      o bogus

and because the reference to two is still there but two itself is
missing, pruning may well end up losing one, because the reference
to it is only through master pointing at two.

> +	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
> +'

Mental note: OK, this demonstrates that the missing two makes us
lose the only reference to one (aka $recoverable in saved).

> +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

I do not know offhand if the lack of the pack-refs feature header
matters here; I assume it does not?

A safer check may be to pack and then make it missing, I guess, but
I do not know if the difference matters.

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

On Thu, Mar 19, 2015 at 01:04:16PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > +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 &&
> 
> Could you spell this as "$base^0" (or "--detach") to clarify the
> intention?  I have been scraching my head for a few minutes just
> now, trying to figure out what you are doing here.  I _think_ you
> wanted master to point at the missing "two" and wanted to make sure
> all other refs (including HEAD) to point away from it.

Yes, exactly. I've squashed in your suggestion and added a comment
explaining it:

diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh
index 1001a69..1cdbd9f 100755
--- a/t/t5312-prune-corruption.sh
+++ b/t/t5312-prune-corruption.sh
@@ -50,14 +50,24 @@ 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) &&
-	# point HEAD elsewhere
-	git checkout $base &&
+	git checkout --detach $base &&
 	rm .git/objects/$(echo $missing | sed "s,..,&/,") &&
 	test_must_fail git cat-file -e $missing
 '

> > +# 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
> 
> I do not know offhand if the lack of the pack-refs feature header
> matters here; I assume it does not?

It doesn't matter. We also do similarly gross things in other
corruption-related tests, but I suspect if you git-blamed them all you
would find that I am responsible. :)

> A safer check may be to pack and then make it missing, I guess, but
> I do not know if the difference matters.

Yeah, I considered that. The trouble is that we are relying on the
earlier setup that made the object go missing. We cannot pack the refs
in the setup step, because the earlier tests are checking the loose-ref
behavior. So we would have to actually restore the object, pack, and
then re-delete it.

Another option would be to restructure the whole test script to perform
each individual corruption in its own sub-repo. I thought that would end
up making things harder to understand due to the extra setup
boilerplate, but it would make the tests less fragile with respect to
each other (e.g., see the "clean up bogus ref" step which exists only to
clean up our earlier corruption that could influence later tests).

-Peff

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> A safer check may be to pack and then make it missing, I guess, but
>> I do not know if the difference matters.
>
> Yeah, I considered that. The trouble is that we are relying on the
> earlier setup that made the object go missing. We cannot pack the refs
> in the setup step, because the earlier tests are checking the loose-ref
> behavior. So we would have to actually restore the object, pack, and
> then re-delete it.

Yes, "restore pack redelete" was what I had in mind when I wondered
such a sequence of extra steps is worth and the difference between
such an approach and an approach to use a hand-crafted packed-refs
file matters.

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

On Thu, Mar 19, 2015 at 02:23:25PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> A safer check may be to pack and then make it missing, I guess, but
> >> I do not know if the difference matters.
> >
> > Yeah, I considered that. The trouble is that we are relying on the
> > earlier setup that made the object go missing. We cannot pack the refs
> > in the setup step, because the earlier tests are checking the loose-ref
> > behavior. So we would have to actually restore the object, pack, and
> > then re-delete it.
> 
> Yes, "restore pack redelete" was what I had in mind when I wondered
> such a sequence of extra steps is worth and the difference between
> such an approach and an approach to use a hand-crafted packed-refs
> file matters.

I took a look at this. It turns out to be rather annoying, because we
can't just restore $missing. The earlier tests may have deleted other
random objects (like $recoverable) depending on whether or not they
actually failed.

So I'm inclined to leave it (we do confirm with the rev-parse call at
the end of the setup that our packed-refs file is working) unless you
feel strongly. If you do, I'd rather go the route of sticking each
corruption in its own separate sub-repo.

-Peff

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So I'm inclined to leave it (we do confirm with the rev-parse call at
> the end of the setup that our packed-refs file is working) unless you
> feel strongly. If you do, I'd rather go the route of sticking each
> corruption in its own separate sub-repo.

No, I don't feel strongly either way---otherwise I wouldn't be
wondering if it makes a difference, but explaining why hand-crafting
is a bad idea (or the other way around).

Thanks.

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

On Thu, Mar 19, 2015 at 02:49:37PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > So I'm inclined to leave it (we do confirm with the rev-parse call at
> > the end of the setup that our packed-refs file is working) unless you
> > feel strongly. If you do, I'd rather go the route of sticking each
> > corruption in its own separate sub-repo.
> 
> No, I don't feel strongly either way---otherwise I wouldn't be
> wondering if it makes a difference, but explaining why hand-crafting
> is a bad idea (or the other way around).

And here I thought you were just being polite. ;)

-Peff

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Eric Sunshine]

On Tue, Mar 17, 2015 at 3:28 AM, Jeff King <peff@peff.net> wrote:
> 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.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> 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 @@
> +# 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.

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

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

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

> > --- /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

Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository

[Eric Sunshine]

On Thu, Mar 19, 2015 at 9:32 PM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote:
>
>> > --- /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?

test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)

[Jeff King]

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

> > 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

Re: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)

[Jeff King]

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

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

> 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".

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

Re: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)

[Jeff King]

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

> > 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

Re: test &&-chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)

[Eric Sunshine]

On Fri, Mar 20, 2015 at 1:10 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:
>> > 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.

>> > 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.)

Re: test &&-chain lint

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [+cc Jonathan, whose patch I apparently subconsciously copied]
>
> On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:
>
>> 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".
>
> 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.

Hmmm, they do look similar and unfamiliar ;-)  It happened while I
was offline, it seems.

Thanks for working on this---I think test-chain-lint should become
one of the pre-acceptance test on my end when it gets applied.

Re: test &&-chain lint

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> [+cc Jonathan, whose patch I apparently subconsciously copied]
>>
>> On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:
>>
>>> 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
>> ...
> Hmmm, they do look similar and unfamiliar ;-)  It happened while I
> was offline, it seems.

One case where this might misdetect a good test would be this one:

    test_expect_success 'either succeed or fail with status 1' '
	git subcmd || case "$?" in 1) : happy ;; *) false failure ;; esac
    '

which would turn into

	(exit 117) && git subcmd || case ...

and fail to set $? to 117, triggering a false positive.

I do not have a good solution fo that, though.  Obviously, turning
the check into

	(exit 117) && {
        	$1
	}

misses the entire point of the chain-lint.

I wonder if another valid way to make it harder for us to commit
"broken && chain" errors in our test may be to make it not an error
in the first place.  Do we know how buggy various implementations of
shells are with respect to their handling of "set -e"?

We know that chaining commands with && is much less likely to be
broken in various reimplementation of bourne shells, and that is
the primary reason we stick to this style in our tests, but if
everybody implements "set -e" reliably and consistently, flipping
that bit in test_eval_ and removing the need to &&-cascade the
commands might not be such a bad idea.

Just thinking aloud...

Re: test &&-chain lint

[Jeff King]

On Fri, Mar 20, 2015 at 10:04:43AM -0700, Junio C Hamano wrote:

> One case where this might misdetect a good test would be this one:
> 
>     test_expect_success 'either succeed or fail with status 1' '
> 	git subcmd || case "$?" in 1) : happy ;; *) false failure ;; esac
>     '

Yes. Any use of "||" is going to cause problems. See the conversions in
the series I posted a few hours ago. The solution is to adapt the test
style to encase any "||" inside a block. I don't like having to modify
the tests to match our lint check, but it is a fairly rare case, and the
modification is not complex. And in fact I think it often highlights to
the reader that something funny is going on; a snippet like the one
above is OK to run at the start of a test, but:

  foo &&
  bar ||
  case "$?"
   ...

is potentially dangerous. You do not know if you are checking the $? of
"foo" or "bar" in that case statement.

Your case above is actually better spelled as test_expect_code, but
there are more complex one-off cases that I solved using a {} block.

> I wonder if another valid way to make it harder for us to commit
> "broken && chain" errors in our test may be to make it not an error
> in the first place.  Do we know how buggy various implementations of
> shells are with respect to their handling of "set -e"?

Certainly that is a thought that occurred to me while writing the
series. At GitHub, we have a shell-based test harness for internal
projects that is somewhat like Git's t/test-lib.sh, but uses "-e" to
catch errors.

It's mostly OK, but there are some surprising bits. For instance, try
this sequence of snippets:

     set -e
     false
     echo should not reach

That should print nothing. So far so good. How about in a subshell:

     set -e
     (
       false
       echo 1
     )
     echo 2

That also prints nothing. Good. But "set -e" is suppressed in a
conditional or in the presence of logic operators like "||".  So you'd
expect this:

     set -e
     (
       false
       echo 1
     ) || {
         echo outcome=$?
         false
     }
     echo 2

to break out of the subshell, print the failed outcome, and then exit.
But it doesn't. It prints "1" and "2". The suppression of "set -e"
continues inside the subshell, even though the inner commands are not
part of their own conditionals.

OK, so when we open a subshell we have to re-iterate our desire for "set
-e":

    set -e
    (
      set -e
      false
      echo 1
    ) || {
        echo outcome=$?
	false
    }
    echo 2

Nope, does nothing, at least in bash and dash. The ||-operator
suppression is clearly not "turn off set -e", but "globally make -e
useless inside here".

So you end up having to use manual exits to jump out of the subshell,
like:

     set -e
     (
       false || exit 1
       echo 1
     ) || {
         echo outcome=$?
         false
     }
     echo 2


That snippet produces "outcome=1", which is what I expect.

Of course most tests are not so complicated to have subshells with
conditional operators on the side. But I did not just type out that
example right now from scratch. I cut-and-pasted it from a real-world
commit message.

So I dunno. I think "set -e" is kind of a dangerous lure. It works so
well _most_ of the time that you start to rely on it, but it really does
have some funny corner cases (even on modern shells, and for all I know,
the behavior above is mandated by POSIX).

-Peff

Re: test &&-chain lint

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Your case above is actually better spelled as test_expect_code, but
> there are more complex one-off cases that I solved using a {} block.

Just for the record, test_expect_code expects only one possible good
exit status and it does not allow us to say "0 is OK and 1 is also
OK, everything else is bad", so it is not quite appropriate there.

> ...
> So I dunno. I think "set -e" is kind of a dangerous lure.

Yes, I think we should stay away from it.  &&-chaining is simpler to
see what is going on, even though it is a bit more to type.

Thanks.

Re: test &&-chain lint

[Jeff King]

On Fri, Mar 20, 2015 at 10:34:51AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Your case above is actually better spelled as test_expect_code, but
> > there are more complex one-off cases that I solved using a {} block.
> 
> Just for the record, test_expect_code expects only one possible good
> exit status and it does not allow us to say "0 is OK and 1 is also
> OK, everything else is bad", so it is not quite appropriate there.

Oh, sorry, I misread your test as expecting 1, but it is actually
expecting 0 or 1. Yeah, I agree this is the exact sort of case covered
in my patch 11/25 (and there are only 4 instances in the whole test
suite).

-Peff

[PATCH 2/5] refs: introduce a "ref paranoia" flag

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH 2/5] refs: introduce a "ref paranoia" flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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;

I am not a big fan of proliferation of interfaces based on
environment variables, but hopefully this is isolated enough to
become an issue in the future.

> +
>  	return do_for_each_entry(refs, base, do_one_ref, &data);
>  }

Re: [PATCH 2/5] refs: introduce a "ref paranoia" flag

[Jeff King]

On Thu, Mar 19, 2015 at 01:13:13PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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;
> 
> I am not a big fan of proliferation of interfaces based on
> environment variables, but hopefully this is isolated enough to
> become an issue in the future.

I'm not sure which part you don't like.

We do have to have this variable cross some process boundaries. Only
"repack" knows whether to turn on paranoia, but "pack-objects" is the
one that must act on it. For that case, we could add a "--ref-paranoia"
flag to pack-objects.

But there are also other cases where the _user_ might want to tell us to
be paranoid (e.g., the upload-pack example I gave earlier). Adding
--paranoia options to every command that might iterate, along with
support for other programs that call them to pass the option through,
seems like a large amount of maintenance burden for little benefit.

We could add a git-global "git --ref-paranoia <cmd>" option and leave
the environment variable as an implementation detail. That makes it hard
to turn on for server-side operations, though. git-daemon runs
git-upload-pack without room for options, though I suppose you could run
"git --ref-paranoia daemon". Smart-http is harder. I'm not sure Apache
lets you add random arguments to CGI programs.

Or is there something else I'm missing?

-Peff

Re: [PATCH 2/5] refs: introduce a "ref paranoia" flag

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > +	if (ref_paranoia < 0)
>> > +		ref_paranoia = git_env_bool("GIT_REF_PARANOIA", 0);
>> > +	if (ref_paranoia)
>> > +		data.flags |= DO_FOR_EACH_INCLUDE_BROKEN;
>> 
>> I am not a big fan of proliferation of interfaces based on
>> environment variables, but hopefully this is isolated enough to
>> become an issue in the future.
>
> I'm not sure which part you don't like.
> 
> We do have to have this variable cross some process boundaries. Only
> "repack" knows whether to turn on paranoia, but "pack-objects" is the
> one that must act on it.
>
> Or is there something else I'm missing?

In general, I do not like the pattern of program A setting an
environment only because it wants to tell program B it spawns
something, because we cannot tell program B that the environment
should be dropped when it calls something else (e.g. user defined
hooks, merge drivers, textconvs, etc.) to prevent end user
invocation of Git commands from honoring it.  Setting GIT_DIR or
GIT_WORK_TREE and having to know when to drop them is not very
pleasant, for example.

I think the use of this pattern is OK in this codepath in which
repack calls pack-objects, and I think I can be persuaded to buy an
argument that there is no harm, or it may even be a good thing, to
run such an end-user program under paranoia mode, if pack-objects
wants to spawn one.

Re: [PATCH 2/5] refs: introduce a "ref paranoia" flag

[Jeff King]

On Thu, Mar 19, 2015 at 02:31:39PM -0700, Junio C Hamano wrote:

> > We do have to have this variable cross some process boundaries. Only
> > "repack" knows whether to turn on paranoia, but "pack-objects" is the
> > one that must act on it.
> >
> > Or is there something else I'm missing?
> 
> In general, I do not like the pattern of program A setting an
> environment only because it wants to tell program B it spawns
> something, because we cannot tell program B that the environment
> should be dropped when it calls something else (e.g. user defined
> hooks, merge drivers, textconvs, etc.) to prevent end user
> invocation of Git commands from honoring it.  Setting GIT_DIR or
> GIT_WORK_TREE and having to know when to drop them is not very
> pleasant, for example.
> 
> I think the use of this pattern is OK in this codepath in which
> repack calls pack-objects, and I think I can be persuaded to buy an
> argument that there is no harm, or it may even be a good thing, to
> run such an end-user program under paranoia mode, if pack-objects
> wants to spawn one.

Ah, I see. Yeah, I consider that to be a _feature_ for REF_PARANOIA
here. If you are running receive-pack under REF_PARANOIA, for example,
you would want your pre-receive hooks to use the same rules as the rest
of receive-pack.

If there is a misfeature, it is that we turn on REF_PARANOIA
automatically behind the user's back in some cases, which could surprise
them if we call through to custom code. But as you note, I think this
code path is OK, because we don't spawn anything else from pack-objects
(and if we did, arguably it is OK because our caller told us we are
doing something dangerous; but we would have to evaluate that
case-by-case, I would think).

-Peff

[PATCH 3/5] prune: turn on ref_paranoia flag

[Jeff King]

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 <peff@peff.net>
---
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

[PATCH 4/5] repack: turn on "ref paranoia" when doing a destructive repack

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH 5/5] refs.c: drop curate_packed_refs

[Jeff King]

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 <peff@peff.net>
---
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

Re: [PATCH 5/5] refs.c: drop curate_packed_refs

[Eric Sunshine]

On Tue, Mar 17, 2015 at 3:31 AM, Jeff King <peff@peff.net> 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 <peff@peff.net>

Re: [PATCH 0/5] not making corruption worse

[Jeff King]

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

Re: [PATCH 0/5] not making corruption worse

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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.

I think this is a trade-off between strictness and convenience.  Is
it preferrable that every time you try to clone a repository you get
reminded that one of its refs point at a bogus object and you
instead have to do "git fetch $there" with a refspec that excludes
the broken one, or is it OK to allow clones and fetches silently
succeed as if nothing is broken?

If the breakage in the reachability chain is somewhere that affects
a branch that is actively in use by the project, with or without
hiding a broken tip, you will be hit by object transfer failure and
you need to really go in there and fix things anyway.  If it is just
a no-longer-used experimental branch that lost necessary objects,
it may be more convenient if the system automatically ignored it.

In some parts of the system there is a movement to make this trade
off tweakable (hint: what happened to the knobs to fsck that allow
certain kinds of broken objects in the object store?  did the topic
go anywhere?). This one so far lacked such a knob to tweak, and I
view your paranoia bit as such a knob.

Re: [PATCH 0/5] not making corruption worse

[Jeff King]

On Tue, Mar 17, 2015 at 03:54:02PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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.
> 
> I think this is a trade-off between strictness and convenience.  Is
> it preferrable that every time you try to clone a repository you get
> reminded that one of its refs point at a bogus object and you
> instead have to do "git fetch $there" with a refspec that excludes
> the broken one, or is it OK to allow clones and fetches silently
> succeed as if nothing is broken?

I think the real issue is that we do not know on the server side what
the client wants. Is it "tell me the refs, so I can grab just the one I
need, and I don't care about the broken ones"? Or is it "I want
everything you have, and tell me if you can't serve it"?  You want
strictness in the latter case, but not in the former. But if we were to
err on the side of strictness, you could not do the former at all
(because upload-pack would barf before the client even has a chance to
say anything).

I'm not sure if anyone will actually find GIT_REF_PARANOIA useful for
something like that or not. As an environment variable, it may impact a
filesystem-local clone, but it would not travel across a TCP connection.
And doing so is tough, because the ref advertisement happens before the
client speaks.

If we ever have a client-speaks-first protocol, one extension could
allow the client to flip the paranoia switch on the server. But my main
goal here was really just making "prune" safer, so I'm happy enough with
what this series does, for now.

> In some parts of the system there is a movement to make this trade
> off tweakable (hint: what happened to the knobs to fsck that allow
> certain kinds of broken objects in the object store?  did the topic
> go anywhere?). This one so far lacked such a knob to tweak, and I
> view your paranoia bit as such a knob.

I think I promised several times to review that topic and never got
around to it. Which makes me a bad person. It is still on my todo list.

-Peff

[PATCH v2 0/5] not making corruption worse

[Jeff King]

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

[PATCH v2 1/5] t5312: test object deletion code paths in a corrupted repository

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH v2 2/5] refs: introduce a "ref paranoia" flag

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH v2 3/5] prune: turn on ref_paranoia flag

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH v2 4/5] repack: turn on "ref paranoia" when doing a destructive repack

[Jeff King]

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 <peff@peff.net>
---
 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

[PATCH v2 5/5] refs.c: drop curate_packed_refs

[Jeff King]

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 <peff@peff.net>
---
 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