Re: [PATCH] make pack-objects a bit more resilient to repo corruption

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