patch, 5 partsnot making corruption worse

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

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

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

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

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

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

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

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

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

The patches are:

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

-Peff

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

The tests here include:

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

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

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

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

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

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

Am 17.03.2015 um 08:28 schrieb Jeff King:

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

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

-- Hannes

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

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

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:

Am 17.03.2015 um 19:55 schrieb Jeff King:

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

Jeff King <peff@peff.net> writes:

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.

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

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.

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

-Peff

On Tue, Mar 17, 2015 at 3:28 AM, Jeff King <peff@peff.net> wrote:

Broken &&-chain.

On Thu, Mar 19, 2015 at 09:16:52PM -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.

-Peff

On Thu, Mar 19, 2015 at 9:32 PM, Jeff King <peff@peff.net> wrote:

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

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

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

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

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

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

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

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

-Peff

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

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

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

-Peff

On Fri, Mar 20, 2015 at 1:10 AM, Jeff King <peff@peff.net> wrote:

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

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

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.

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

Jeff King <peff@peff.net> writes:

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.

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.

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

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

Jeff King <peff@peff.net> writes:

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.

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

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

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

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

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

On Tue, Mar 17, 2015 at 3:31 AM, Jeff King <peff@peff.net> wrote:

s/outweights/outweighs/

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

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

Jeff King <peff@peff.net> writes:

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.

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

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.

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