From: Nicolas Pitre <nico@fluxnic.net> Date: 2016-06-15 22:49:50
Right now, packing valid objects could fail when creating a thin pack
simply because a pack edge object used as a preferred base is corrupted.
Since preferred base objects are not strictly needed to produce a valid
pack, let's not consider the inability to read them as a fatal error.
Delta compression may well be attempted against other objects in the
search window.
Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---
I wrote this patch while helping Uwe with his repo corruption. While
his problem turned out to be different from the one this patch is
addressing (see previous patch I posted) I think that since I did the
patch already then this wouldn't hurt to have this one merged too.
From: Jeff King <hidden> Date: 2016-06-15 22:49:51
On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
- if (!src->data)
+ if (!src->data) {
+ if (src_entry->preferred_base) {
+ /*
+ * Those objects are not included in the
+ * resulting pack. Be resilient and ignore
+ * them if they can't be read, in case the
+ * pack could be created nevertheless.
+ */
+ return 0;
+ }
die("object %s cannot be read",
sha1_to_hex(src_entry->idx.sha1));
+ }
By converting this die() into a silent return, are we losing a place
where git might previously have alerted a user to corruption? In this
case, we can continue the operation without the object, but if we have
detected corruption, letting the user know as soon as possible is
probably a good idea.
In other words, should this instead be:
warning("unable to read preferred base object: %s", ...);
return 0;
Or will some other part of the code already complained to stderr?
-Peff
On Fri, 2010-10-22 at 10:46 -0400, Jeff King wrote:
On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
quoted
- if (!src->data)
+ if (!src->data) {
+ if (src_entry->preferred_base) {
+ /*
+ * Those objects are not included in the
+ * resulting pack. Be resilient and ignore
+ * them if they can't be read, in case the
+ * pack could be created nevertheless.
+ */
+ return 0;
+ }
die("object %s cannot be read",
sha1_to_hex(src_entry->idx.sha1));
+ }
By converting this die() into a silent return, are we losing a place
where git might previously have alerted a user to corruption? In this
case, we can continue the operation without the object, but if we have
detected corruption, letting the user know as soon as possible is
probably a good idea.
In other words, should this instead be:
warning("unable to read preferred base object: %s", ...);
return 0;
Or will some other part of the code already complained to stderr?
-Peff
Agreed. If it broke we should probably tell the user--even if we can't
do much useful about it other than attempt to recover by continuing.
--
-Drew Northup N1XIM
AKA RvnPhnx on OPN
________________________________________________
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59
From: Nicolas Pitre <nico@fluxnic.net> Date: 2016-06-15 22:49:51
On Fri, 22 Oct 2010, Jeff King wrote:
On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
quoted
- if (!src->data)
+ if (!src->data) {
+ if (src_entry->preferred_base) {
+ /*
+ * Those objects are not included in the
+ * resulting pack. Be resilient and ignore
+ * them if they can't be read, in case the
+ * pack could be created nevertheless.
+ */
+ return 0;
+ }
die("object %s cannot be read",
sha1_to_hex(src_entry->idx.sha1));
+ }
By converting this die() into a silent return, are we losing a place
where git might previously have alerted a user to corruption? In this
case, we can continue the operation without the object, but if we have
detected corruption, letting the user know as soon as possible is
probably a good idea.
In other words, should this instead be:
warning("unable to read preferred base object: %s", ...);
return 0;
Well, this get called repeatedly, being within the inner part of the
delta search loop. So you might get that warning as many times as the
delta window which is not that nice. If anything a static flag to
display the warning only once would be needed. But you're pretty likely
to have met that warning/error already from other operations, which is
why I didn't bother.
Or will some other part of the code already complained to stderr?
Some other part is likely to already have complained, through
check_object() -> sha1_object_info(). But not necessarily in all cases.
Nicolas
From: Nicolas Pitre <nico@fluxnic.net> Date: 2016-06-15 22:49:51
On Fri, 22 Oct 2010, Drew Northup wrote:
On Fri, 2010-10-22 at 10:46 -0400, Jeff King wrote:
quoted
On Fri, Oct 22, 2010 at 12:53:32AM -0400, Nicolas Pitre wrote:
quoted
- if (!src->data)
+ if (!src->data) {
+ if (src_entry->preferred_base) {
+ /*
+ * Those objects are not included in the
+ * resulting pack. Be resilient and ignore
+ * them if they can't be read, in case the
+ * pack could be created nevertheless.
+ */
+ return 0;
+ }
die("object %s cannot be read",
sha1_to_hex(src_entry->idx.sha1));
+ }
By converting this die() into a silent return, are we losing a place
where git might previously have alerted a user to corruption? In this
case, we can continue the operation without the object, but if we have
detected corruption, letting the user know as soon as possible is
probably a good idea.
In other words, should this instead be:
warning("unable to read preferred base object: %s", ...);
return 0;
Or will some other part of the code already complained to stderr?
-Peff
Agreed. If it broke we should probably tell the user--even if we can't
do much useful about it other than attempt to recover by continuing.
Please don't misinterpret this case. As far as this change is
concerned, nothing is actually "broken". The operation _will_ still
succeed. The repository may be broken, but in this case we can do
without the broken object. In those cases where the object is really
needed the original die() is still in place.
Nicolas
From: Nicolas Pitre <nico@fluxnic.net> Date: 2016-06-15 22:49:51
Right now, packing valid objects could fail when creating a thin pack
simply because a pack edge object used as a preferred base is corrupted.
Since preferred base objects are not strictly needed to produce a valid
pack, let's not consider the inability to read them as a fatal error.
Delta compression may well be attempted against other objects in the
search window. To avoid warning storms (we are in the inner loop of
the delta search window) a warning is emitted only on the first
occurrence.
Signed-off-by: Nicolas Pitre <nico@fluxnic.net>
---
On Fri, 22 Oct 2010, Nicolas Pitre wrote:
On Fri, 22 Oct 2010, Jeff King wrote:
quoted
By converting this die() into a silent return, are we losing a place
where git might previously have alerted a user to corruption? In this
case, we can continue the operation without the object, but if we have
detected corruption, letting the user know as soon as possible is
probably a good idea.
In other words, should this instead be:
warning("unable to read preferred base object: %s", ...);
return 0;
Well, this get called repeatedly, being within the inner part of the
delta search loop. So you might get that warning as many times as the
delta window which is not that nice. If anything a static flag to
display the warning only once would be needed. But you're pretty likely
to have met that warning/error already from other operations, which is
why I didn't bother.
quoted
Or will some other part of the code already complained to stderr?
Some other part is likely to already have complained, through
check_object() -> sha1_object_info(). But not necessarily in all cases.
OK... After further analysis, it seems that the cases when problem
objects are already warned about through sha1_object_info(), those
objects will never end up in the delta search window, as their type ends
up being a negative error code. We already support that possibility on
purpose even.
So let's add a warning for when check_object() was able to bypass
the more expensive sha1_object_info() call and therefore object
corruptions remain undetected until that point.
From: Nicolas Pitre <nico@fluxnic.net> Date: 2016-06-15 22:49:51
On Fri, 22 Oct 2010, Sverre Rabbelier wrote:
Heya,
On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre [off-list ref] wrote:
quoted
+ static int warned = 0;
+ if (!warned++)
+ warning("object %s cannot be read",
+ sha1_to_hex(src_entry->idx.sha1));
How does this handle multiple missing objects? Will it only warn for
the first one?
Yes, only the first one, so you have a bone to chase if that ever
happens to you. And that's good enough IMHO. Trying to warn for every
missing object would require extra storage per object to remember if any
particular object was warned for already, which is I think overkill for
an extremely unlikely event. Comprehensive reporting is the job of
fsck.
Nicolas
On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre [off-list ref] wrote:
quoted
+ static int warned = 0;
+ if (!warned++)
+ warning("object %s cannot be read",
+ sha1_to_hex(src_entry->idx.sha1));
How does this handle multiple missing objects? Will it only warn for
the first one?
Yes, only the first one, so you have a bone to chase if that ever
happens to you. And that's good enough IMHO. Trying to warn for every
missing object would require extra storage per object to remember if any
particular object was warned for already, which is I think overkill for
an extremely unlikely event. Comprehensive reporting is the job of
fsck.
Maybe add a ", run git fsck" to the message. Will still comfortably fit a line.
-Geert
From: Nicolas Pitre <nico@fluxnic.net> Date: 2016-06-15 22:49:51
On Sat, 23 Oct 2010, Geert Bosch wrote:
On Oct 22, 2010, at 17:19, Nicolas Pitre wrote:
quoted
quoted
On Fri, Oct 22, 2010 at 13:26, Nicolas Pitre [off-list ref] wrote:
quoted
+ static int warned = 0;
+ if (!warned++)
+ warning("object %s cannot be read",
+ sha1_to_hex(src_entry->idx.sha1));
How does this handle multiple missing objects? Will it only warn for
the first one?
Yes, only the first one, so you have a bone to chase if that ever
happens to you. And that's good enough IMHO. Trying to warn for every
missing object would require extra storage per object to remember if any
particular object was warned for already, which is I think overkill for
an extremely unlikely event. Comprehensive reporting is the job of
fsck.
Maybe add a ", run git fsck" to the message. Will still comfortably fit a line.
Maybe if this message ever gets printed often enough. Let's see if
someone will even report it before 2012, and be clueless about it. And
to be consistent, you'd have to do the same throughout the code where
this could be relevant.
Furthermore, if someone really need the additional clue, then I'm afraid
that the current fsck output won't help at all except to confuse that
person even more.
Better for people to ask for help on this list when things break due to
corruptions if they can't figure it out on their own.
Nicolas