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

[Nicolas Pitre <nico@fluxnic.net>]

On Fri, 22 Oct 2010, Drew Northup wrote:

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