git fsck exit code?

It looks like git fsck exits with 0 status even if there are some errors. The only case where there's a non-zero exit code is if verify_pack reports errors -- but not e.g. fsck_object_dir.

Is that really the intended behavior? I think it would be nice to at least support --exit-code (but probably a sensible exit code ought to be the default).

Quoted 3 lines · David Turner

It looks like git fsck exits with 0 status even if there are some errors. The only case where there's a non-zero exit code is if verify_pack reports errors -- but not e.g. fsck_object_dir.

It will also bail non-zero with _certain_ tree errors that cause git to die() rather than fscking more completely.

Quoted 6 lines · Jeff King

Quoted 3 lines · David Turner

It looks like git fsck exits with 0 status even if there are some errors. The only case where there's a non-zero exit code is if verify_pack reports errors -- but not e.g. fsck_object_dir.

Quoted 2 lines

It will also bail non-zero with _certain_ tree errors that cause git to die() rather than fscking more completely.

Even if git does not die, whenever it says broken link, missing object, or object corrupt, we set errors_found and that variable affects the exit status of fsck. What does "some errors" exactly mean in the original report? Dangling objects are *not* errors and should not cause fsck to report an error with its exit status.

Quoted 5 lines

Even if git does not die, whenever it says broken link, missing object, or object corrupt, we set errors_found and that variable affects the exit status of fsck. What does "some errors" exactly mean in the original report? Dangling objects are *not* errors and should not cause fsck to report an error with its exit status.

error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains duplicate file entries

(at least -- there might be more, but that's the one that bit me)

Quoted 6 lines · David Turner

Quoted 5 lines

Even if git does not die, whenever it says broken link, missing object, or object corrupt, we set errors_found and that variable affects the exit status of fsck. What does "some errors" exactly mean in the original report? Dangling objects are *not* errors and should not cause fsck to report an error with its exit status.

Quoted 4 lines

error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains duplicate file entries

(at least -- there might be more, but that's the one that bit me)

I think that we just don't set "errors_found" in fsck_obj (nor do we in fsck_obj_buffer, but in that case its caller is verify-pack, which propagates the return code). Maybe (completely untested):

builtin/fsck.c +2 -1

index d42a27d..29de901 100644
@@ -388,7 +388,8 @@ static void fsck_sha1_list(void)
 		unsigned char *sha1 = entry->sha1;
 
 		sha1_list.entry[i] = NULL;
-		fsck_sha1(sha1);
+		if (fsck_sha1(sha1))
+			errors_found |= ERROR_OBJECT;
 		free(entry);
 	}
 	sha1_list.nr = 0;

From: Jeff King <[email protected]>
Date: Fri, 29 Aug 2014 16:31:46 -0400

Upon finding a corrupt loose object, we forgot to note the error to signal it with the exit status of the entire process.

[jc: adjusted t1450 and added another test]

Signed-off-by Junio C Hamano <[email protected]>

---

* I think your fix is a right one that catches all the "we can parse minimally for the purpose of 'struct object' class system, but the object is semantically broken" cases, as fsck_obj() is where such a validation should all happen.

I can haz a sign off? Thanks.

Quoted 6 lines

Upon finding a corrupt loose object, we forgot to note the error to signal it with the exit status of the entire process.

[jc: adjusted t1450 and added another test]

Quoted 7 lines

Signed-off-by Junio C Hamano <[email protected]>

---

* I think your fix is a right one that catches all the "we can parse minimally for the purpose of 'struct object' class system, but the object is semantically broken" cases, as fsck_obj() is where such a validation should all happen.

Fsck tries hard to detect missing objects, and will complain (and exit non-zero) about any inter-object links that are missing. However, it will not exit non-zero for any missing ref tips, meaning that a severely broken repository may still pass "git fsck && echo ok".

The problem is that we use for_each_ref to iterate over the ref tips, which hides broken tips. It does at least print an error from the refs.c code, but fsck does not ever see the ref and cannot note the problem in its exit code. We can solve this by using for_each_rawref and noting the error ourselves.

Dropping curate_packed_ref_fn (as below) fixes the test above. And miraculously does not even seem to conflict with ref patches in pu. :)

[+cc mhagger for packed-refs wisdom]

If we only have a packed copy of "refs/heads/master" and it is broken, then deleting any _other_ unrelated ref will cause refs/heads/master to be dropped from the packed-refs file entirely. We get an error message, but that's easy to miss, and the pointer to master's sha1 is lost forever.

Quoted 7 lines · Jeff King

[+cc mhagger for packed-refs wisdom]

If we only have a packed copy of "refs/heads/master" and it is broken, then deleting any _other_ unrelated ref will cause refs/heads/master to be dropped from the packed-refs file entirely. We get an error message, but that's easy to miss, and the pointer to master's sha1 is lost forever.

Hmph, and the significance of losing a random 20-byte object name that is useless in your repository is? You could of course ask around other repositories (i.e. your origin, others that fork from the same origin, etc.), and having the name might make it easier to locate the exact object.

But in such a case, either they have it at the tip (in which case you can just fetch the branch you lost), or they have it reachable from one of their tips of branches you had shown interest in (that is why you had that lost object in the first place). Either way, you would be running "git fetch" or asking them to send "git bundle" output to be unbundled at your end, and the way you ask would be by refname, not the object name, so I am not sure if the loss is that grave.

Quoted 7 lines · Jeff King

[+cc mhagger for packed-refs wisdom]

If we only have a packed copy of "refs/heads/master" and it is broken, then deleting any _other_ unrelated ref will cause refs/heads/master to be dropped from the packed-refs file entirely. We get an error message, but that's easy to miss, and the pointer to master's sha1 is lost forever.

Hmph, and the significance of losing a random 20-byte object name that is useless in your repository is? You could of course ask around other repositories (i.e. your origin, others that fork from the same origin, etc.), and having the name might make it easier to locate the exact object.

But in such a case, either they have it at the tip (in which case you can just fetch the branch you lost), or they have it reachable from one of their tips of branches you had shown interest in (that is why you had that lost object in the first place). Either way, you would be running "git fetch" or asking them to send "git bundle" output to be unbundled at your end, and the way you ask would be by refname, not the object name, so I am not sure if the loss is that grave.

Perhaps I am missing something, of course, though.

[+cc mhagger for packed-refs wisdom]

Quoted 11 lines · Jeff King

Fsck tries hard to detect missing objects, and will complain (and exit non-zero) about any inter-object links that are missing. However, it will not exit non-zero for any missing ref tips, meaning that a severely broken repository may still pass "git fsck && echo ok".

The problem is that we use for_each_ref to iterate over the ref tips, which hides broken tips. It does at least print an error from the refs.c code, but fsck does not ever see the ref and cannot note the problem in its exit code. We can solve this by using for_each_rawref and noting the error ourselves.

There's a possibly related problem with packed-refs that I noticed while looking at this.

When we call pack-refs, it will refuse to pack any broken loose refs, and leave them loose. Which is sane. But when we delete a ref, we need to rewrite the packed-refs file, and we omit any broken packed refs. We wouldn't have written a broken entry, but we may get broken later (i.e., the tip object may go missing after the packed-refs file is written).

If we only have a packed copy of "refs/heads/master" and it is broken, then deleting any _other_ unrelated ref will cause refs/heads/master to be dropped from the packed-refs file entirely. We get an error message, but that's easy to miss, and the pointer to master's sha1 is lost forever.

[...]
I am tempted to say that we do not need to do curate_each_ref_fn at all. Any entry with a broken sha1 is either:

1. A truly broken ref, in which case we should make sure to keep it (i.e., it is not cruft at all).

2. A crufty entry that has been replaced by a loose reference that has not yet been packed. Such a crufty entry may point to broken objects, and that is OK.

In case 2, we _could_ delete the cruft. But I do not think we need to. The loose ref will take precedence to anybody who actually does a ref lookup, so the cruft is not hurting anybody.

Dropping curate_packed_ref_fn (as below) fixes the test above. And miraculously does not even seem to conflict with ref patches in pu. :)

Am I missing any case that it is actually helping?

From: Jeff King <[email protected]>
Date: Fri, 29 Aug 2014 16:31:46 -0400

Upon finding a corrupt loose object, we forgot to note the error to signal it with the exit status of the entire process.

[jc: adjusted t1450 and added another test]

By the way, Jonathan, with dbedf8bf (t1450 (fsck): remove dangling objects, 2010-09-06) you added a 'test_might_fail git fsck' to the 1450 test that catches an object corruption. Do you remember if there was some flakiness in this test that necessitated it, or is it merely "I think this should fail, but it does not, and we may fix it some day but I am not doing that in this patch?"

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · Jeff King

Quoted 3 lines · David Turner

It looks like git fsck exits with 0 status even if there are some errors. The only case where there's a non-zero exit code is if verify_pack reports errors -- but not e.g. fsck_object_dir.

It will also bail non-zero with _certain_ tree errors that cause git to die() rather than fscking more completely.

Quoted 5 lines

Even if git does not die, whenever it says broken link, missing object, or object corrupt, we set errors_found and that variable affects the exit status of fsck. What does "some errors" exactly mean in the original report? Dangling objects are *not* errors and should not cause fsck to report an error with its exit status.

error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains duplicate file entries

Quoted 2 lines · David Turner

error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains duplicate file entries

I don't think git fsck should return !0 in this case. Yes, it's an inconsistency in the repo, but it's sometimes due to erroneous conversions from another SCM or some other (non-standard) implementation of the git client. I've seen things like this (and other inconsistencies in repos, like wrong date formats, non-standard Author fields, unsorted trees, zero-padded file modes and so on), and the thing is, owners of public repos with these errors tend to avoid fixing it because it changes the commit SHAs. If git fsck starts to return !0 on these errors, it will always return error on that repo, which in practise means that the error code is rendered useless. IMHO git fsck should only return !0 on errors that can be fixed without changing the commit history, for example missing or invalid objects.

Quoted 10 lines · David Turner

Quoted 9 lines · Junio C Hamano

Quoted 7 lines · Jeff King

Quoted 3 lines · David Turner

It looks like git fsck exits with 0 status even if there are some errors. The only case where there's a non-zero exit code is if verify_pack reports errors -- but not e.g. fsck_object_dir.

It will also bail non-zero with _certain_ tree errors that cause git to die() rather than fscking more completely.

Quoted 8 lines

Quoted 5 lines

Even if git does not die, whenever it says broken link, missing object, or object corrupt, we set errors_found and that variable affects the exit status of fsck. What does "some errors" exactly mean in the original report? Dangling objects are *not* errors and should not cause fsck to report an error with its exit status.

error in tree 9f50addba2b4e9e928d9c6a7056bdf71b36fba90: contains duplicate file entries

I don't think git fsck should return !0 in this case. Yes, it's an inconsistency in the repo, but it's sometimes due to erroneous conversions from another SCM or some other (non-standard) implementation of the git client. I've seen things like this (and other inconsistencies in repos, like wrong date formats, non-standard Author fields, unsorted trees, zero-padded file modes and so on), and the thing is, owners of public repos with these errors tend to avoid fixing it because it changes the commit SHAs. If git fsck starts to return !0 on these errors, it will always return error on that repo, which in practise means that the error code is rendered useless. IMHO git fsck should only return !0 on errors that can be fixed without changing the commit history, for example missing or invalid objects.

Quoted 4 lines

I don't think git fsck should return !0 in this case. Yes, it's an inconsistency in the repo, but it's sometimes due to erroneous conversions from another SCM or some other (non-standard) implementation of the git client. I've seen things like this (and other inconsistencies

Quoted 8 lines

in repos, like wrong date formats, non-standard Author fields, unsorted trees, zero-padded file modes and so on), and the thing is, owners of public repos with these errors tend to avoid fixing it because it changes the commit SHAs. If git fsck starts to return !0 on these errors, it will always return error on that repo, which in practise means that the error code is rendered useless. IMHO git fsck should only return !0 on errors that can be fixed without changing the commit history, for example missing or invalid objects.

We could have one exit code for errors which can be fixed without rewriting history, and another for errors that can't. Or different command-line arguments to suppress errors of this sort.

In my case, I actually could fix the issue, because it was in a newly-created branch; I just rewrote the script that created the branch to be a little smarter.