Re: [PATCH] unpack_entry: do not die when we fail to apply a delta

[Nicolas Pitre <nico@fluxnic.net>]

On Thu, 13 Jun 2013, Jeff King wrote:

> When we try to load an object from disk and fail, our
> general strategy is to see if we can get it from somewhere
> else (e.g., a loose object). That lets users fix corruption
> problems by copying known-good versions of objects into the
> object database.
> 
> We already handle the case where we were not able to read
> the delta from disk. However, when we find that the delta we
> read does not apply, we simply die.  This case is harder to
> trigger, as corruption in the delta data itself would
> trigger a crc error from zlib.  However, a corruption that
> pointed us at the wrong delta base might cause it.
> 
> We can do the same "fail and try to find the object
> elsewhere" trick instead of dying. This not only gives us a
> chance to recover, but also puts us on code paths that will
> alert the user to the problem (with the current message,
> they do not even know which sha1 caused the problem).
> 
> Signed-off-by: Jeff King <peff@peff.net>

That makes sense.

Could you produce a test case to go along with this change?

> ---
> I needed this earlier today to recover from a corrupted packfile (I
> fortunately had an older version of the repo in backups). Still tracking
> down the exact nature of the corruption.
> 
>  sha1_file.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/sha1_file.c b/sha1_file.c
> index 5c08701..d458708 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2135,8 +2135,17 @@ void *unpack_entry(struct packed_git *p, off_t obj_offset,
>  		data = patch_delta(base, base_size,
>  				   delta_data, delta_size,
>  				   &size);
> +
> +		/*
> +		 * We could not apply the delta; warn the user, but keep going.
> +		 * Our failure will be noticed either in the next iteration of
> +		 * the loop, or if this is the final delta, in the caller when
> +		 * we return NULL. Those code paths will take care of making
> +		 * a more explicit warning and retrying with another copy of
> +		 * the object.
> +		 */
>  		if (!data)
> -			die("failed to apply delta");
> +			error("failed to apply delta");
>  
>  		free(delta_data);
>  	}
> -- 
> 1.8.3.rc2.14.g7eee6b3
>