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

[Nicolas Pitre <nico@fluxnic.net>]

On Fri, 22 Oct 2010, Jeff King wrote:

> 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