Re: [PATCH] unpack_entry: do not die when we fail to apply a delta
On Thu, 13 Jun 2013, Jeff King wrote:
Show 20 quoted lines
> 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?
Show 34 quoted lines
> ---
> 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
> Nicolas Pitre <nico@fluxnic.net>