Re: [PATCH] fsck: return non-zero status on missing ref tips
On 09/12/2014 06:58 AM, Junio C Hamano wrote: On Thu, Sep 11, 2014 at 9:29 PM, Jeff King p...@peff.net wrote: [+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. I don't understand your argument. First, you would not just lose the SHA-1 of the object. You would also lose the name of the reference that was previously pointing at it. Second, the discarded information *is* useful. The more information you have, the more likely you can restore it and/or diagnose the original cause of the corruption. Third, even if the discarded information were not useful, the fact that *information has gone missing* is of overwhelming importance, and that fact would be forgotten as soon as the warning message scrolls off of your terminal. The reference deletion that triggered the warning might even have been done in the background by some other process (e.g., a GUI) and the output discarded or shunted into some debug window that the user would have no reason to look at. So I agree with Peff that it would be prudent to preserve the corrupt reference at least until the next git fsck, which (a) is run by the user specifically to look for corruption, and (b) can return an error result to make the failure obvious. The only thing that is unclear to me is whether the user would be able to get rid of the broken reference once it is discovered (short of opening packed-refs in an editor). Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: return non-zero status on missing ref tips
On 09/12/2014 06:29 AM, Jeff King wrote: [+cc mhagger for packed-refs wisdom] On Thu, Sep 11, 2014 at 11:38:30PM -0400, Jeff King wrote: 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 was confused for a while by your observation, because the curate function has if (read_ref_full(entry-name, sha1, 0, flags)) /* We should at least have found the packed ref. */ die(Internal error); , which looks like more than emit an error message and continue. But in fact the flow never gets this far, because iterating without DO_FOR_EACH_INCLUDE_BROKEN doesn't just skip references for which REF_ISBROKEN is set, but also (do to a test in do_one_ref()) references for which ref_resolves_to_object() fails. The ultimate source of my confusion is that the word BROKEN has two different meanings in the two constants' names. [...] 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? Something inside me screams out in horror that we would pass up an opportunity to delete unneeded cruft from the packed-refs file. But I can't think of a rational reason to disagree with you, so as far as I'm concerned your suggestion seems OK. Michael -- Michael Haggerty mhag...@alum.mit.edu -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fsck: return non-zero status on missing ref tips
On Tue, Sep 09, 2014 at 03:03:33PM -0700, Junio C Hamano wrote:
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 gits...@pobox.com
---
* 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.
Here's another structural case that we should catch but do not:
-- 8 --
Subject: fsck: return non-zero status on missing ref tips
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.
In addition to adding tests for this case, we add tests for
all types of missing-object links (all of which worked, but
which we were not testing).
Signed-off-by: Jeff King p...@peff.net
---
Just below here we check that refs/heads/* points only to commit
objects. That's also sort-of-structural, but is pretty easy to recover
from without data loss, so I don't think it is as obvious a candidate
for a non-zero exit.
builtin/fsck.c | 3 ++-
t/t1450-fsck.sh | 56
2 files changed, 58 insertions(+), 1 deletion(-)
diff --git a/builtin/fsck.c b/builtin/fsck.c
index 29de901..0928a98 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -489,6 +489,7 @@ static int fsck_handle_ref(const char *refname, const
unsigned char *sha1, int f
obj = parse_object(sha1);
if (!obj) {
error(%s: invalid sha1 pointer %s, refname,
sha1_to_hex(sha1));
+ errors_found |= ERROR_REACHABLE;
/* We'll continue with the rest despite the error.. */
return 0;
}
@@ -505,7 +506,7 @@ static void get_default_heads(void)
{
if (head_points_at !is_null_sha1(head_sha1))
fsck_handle_ref(HEAD, head_sha1, 0, NULL);
- for_each_ref(fsck_handle_ref, NULL);
+ for_each_rawref(fsck_handle_ref, NULL);
if (include_reflogs)
for_each_reflog(fsck_handle_reflog, NULL);
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 0de755c..b52397a 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -302,4 +302,60 @@ test_expect_success 'fsck notices .git in trees' '
)
'
+# create a static test repo which is broken by omitting
+# one particular object ($1, which is looked up via rev-parse
+# in the new repository).
+create_repo_missing () {
+ rm -rf missing
+ git init missing
+ (
+ cd missing
+ git commit -m one --allow-empty
+ mkdir subdir
+ echo content subdir/file
+ git add subdir/file
+ git commit -m two
+ unrelated=$(echo unrelated | git hash-object --stdin -w)
+ git tag -m foo tag $unrelated
+ sha1=$(git rev-parse --verify $1)
+ path=$(echo $sha1 | sed 's|..|/|')
+ rm .git/objects/$path
+ )
+}
+
+test_expect_success 'fsck notices missing blob' '
+ create_repo_missing HEAD:subdir/file
+ test_must_fail git -C missing fsck
+'
+
+test_expect_success 'fsck notices missing subtree' '
+ create_repo_missing HEAD:subdir
+ test_must_fail git -C missing fsck
+'
+
+test_expect_success 'fsck notices missing root tree' '
+ create_repo_missing HEAD^{tree}
+ test_must_fail git -C missing fsck
+'
+
+test_expect_success 'fsck notices missing parent' '
+ create_repo_missing HEAD^
+ test_must_fail git -C missing fsck
+'
+
+test_expect_success 'fsck notices missing tagged object' '
+ create_repo_missing tag^{blob}
+ test_must_fail git -C missing fsck
+'
+
+test_expect_success 'fsck notices ref pointing to missing commit' '
+ create_repo_missing HEAD
+ test_must_fail git -C missing fsck
+'
+
+test_expect_success 'fsck notices ref pointing to missing tag' '
+ create_repo_missing tag
+ test_must_fail git -C missing fsck
+'
+
test_done
--
2.1.0.373.g91ca799
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: return non-zero status on missing ref tips
[+cc mhagger for packed-refs wisdom]
On Thu, Sep 11, 2014 at 11:38:30PM -0400, Jeff King wrote:
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.
This test (on top of the patch I just sent) demonstrates:
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index b52397a..b0f4545 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -317,6 +317,7 @@ create_repo_missing () {
git commit -m two
unrelated=$(echo unrelated | git hash-object --stdin -w)
git tag -m foo tag $unrelated
+ git pack-refs --all --prune
sha1=$(git rev-parse --verify $1)
path=$(echo $sha1 | sed 's|..|/|')
rm .git/objects/$path
@@ -358,4 +359,10 @@ test_expect_success 'fsck notices ref pointing to missing
tag' '
test_must_fail git -C missing fsck
'
+test_expect_success 'ref deletion does not eat broken refs' '
+ create_repo_missing HEAD
+ git -C missing update-ref -d refs/tags/tag
+ test_must_fail git -C missing fsck
+'
+
test_done
The fsck will succeed even though master should be broken, because
we appear to have no refs at all.
The fault is in curate_packed_ref_fn, which drops crufty from
packed-refs as we repack. That in turn is representing an older:
if (!ref_resolves_to_object(entry))
return 0; /* Skip broken refs */
which comes from 624cac3 (refs: change the internal reference-iteration
API, 2013-04-22). But that was just maintaining the existing behavior,
which was using a do_for_each_ref iteration without INCLUDE_BROKEN. So I
think this problem has a similar root, though the fix is now slightly
different.
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?
diff --git a/refs.c b/refs.c
index a7853cc..83c2bf7 100644
--- a/refs.c
+++ b/refs.c
@@ -2484,70 +2484,11 @@ int pack_refs(unsigned int flags)
}
/*
- * 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, sha1, 0, flags))
- /* We should at least have found the packed ref. */
- die(Internal error);
- if ((flags REF_ISSYMREF) || !(flags
Re: [PATCH] fsck: return non-zero status on missing ref tips
On Fri, Sep 12, 2014 at 12:29:39AM -0400, Jeff King wrote: Dropping curate_packed_ref_fn (as below) fixes the test above. And miraculously does not even seem to conflict with ref patches in pu. :) Of course I spoke too soon. The patch I sent is actually based on pu. It is easy to make the equivalent change in either master or pu (they are both just deletions of the same blocks), but the code mutated a little in between, and there are purely textual conflicts going from one to the other. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: return non-zero status on missing ref tips
On Thu, Sep 11, 2014 at 9:29 PM, Jeff King p...@peff.net wrote: [+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. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fsck: return non-zero status on missing ref tips
On Thu, Sep 11, 2014 at 09:58:45PM -0700, Junio C Hamano wrote: On Thu, Sep 11, 2014 at 9:29 PM, Jeff King p...@peff.net wrote: [+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. Because your repository is corrupted and broken, and we then forget that fact. I.e., it is not a random 20-byte object name. It is the thing that your branch is pointing at, and that is valuable in itself. If you can restore the object (e.g., from another repository), you need to know which object to restore. But I also think corrupting a repository and not noticing is quite bad in itself. 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. Yes, but after you get the objects from the other person, what do you set your ref to? If I know my tip was at commit X, I can get any set of objects from another untrusted person that includes X, verify what they sent me cryptographically, and restore my tip to X. If I do not know X, they can send me any random set of objects. I can verify that the sha1s are OK and the graph is complete, but I cannot verify that the contents are sane. I am effectively just picking their master as my new starting point, and trusting that it has not been tampered with. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
