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

From: Nicolas Pitre <nico@fluxnic.net>
Date: Oct 22, 2010, 18:42 UTC
Message-ID: <alpine.LFD.2.00.1010221427390.2764@xanadu.home>
In-Reply-To: <20101022144600.GA5554@sigill.intra.peff.net>

On Fri, 22 Oct 2010, Jeff King wrote:

Show 27 quoted lines

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

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