Re: [PATCH/RFC] Allow writing loose objects that are corrupted in a pack file

From: Shawn O. Pearce <spearce@spearce.org>
Date: Dec 9, 2008, 16:24 UTC
Message-ID: <20081209162402.GP31551@spearce.org>
In-Reply-To: <20081209093627.77039a1f@perceptron>

Jan Krüger <jk@jk.gs> wrote:

> For fixing a corrupted repository by using backup copies of individual
> files, allow write_sha1_file() to write loose files even if the object
> already exists in a pack file, but only if the existing entry is marked
> as corrupted.

Huh. So I'm digging around sha1_file.c and I'm not yet sure why your patch makes a difference.

has_sha1_file() calls find_pack_entry() to determine which pack has the object, and at what offset (if found). It doesn't care about the offset, but it does care about the successful match.

find_pack_entry() already considers the bad_object_sha1 for each pack, before it even tries the binary search within the index. So if the entry was known to be bad has_sha1_file() will return 0, unless the object is loose.

Where this breaks down is if the object is being created, its very likely we didn't attempt to read it in this process. The bad_object_sha1 table is transient and populated only when unpacking an object entry fails. So for example during a merge if a tree was stored in a pack and is corrupt and the merge result produces that same tree object we won't write it out with write_sha1_file() because it exists in a pack, but since we never read it we also don't know the pack entry is corrupt.

Its horribly inefficient to read every object before we write it back out. The best thing to do when faced with corruption is to stop and repack, overlaying the object database from a known good copy of the repository so pack-objects can use the good copy when a corrupt object is identified.

So I agree with you that changing this in write_sha1_file() is a bad idea for the normal good cases, but I also don't see how this patch changes anything at all... the code path you introduced is already implemented.

Show 26 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index 6c0e251..17085cc 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2373,14 +2373,17 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha
>  	char hdr[32];
>  	int hdrlen;
>  
> -	/* Normally if we have it in the pack then we do not bother writing
> -	 * it out into .git/objects/??/?{38} file.
> -	 */
>  	write_sha1_file_prepare(buf, len, type, sha1, hdr, &hdrlen);
>  	if (returnsha1)
>  		hashcpy(returnsha1, sha1);
> -	if (has_sha1_file(sha1))
> -		return 0;
> +	/* Normally if we have it in the pack then we do not bother writing
> +	 * it out into .git/objects/??/?{38} file. We do, though, if there
> +	 * is no chance that we have an uncorrupted version of the object.
> +	 */
> +	if (has_sha1_file(sha1)) {
> +		if (has_loose_object(sha1) || !has_packed_and_bad(sha1))
> +			return 0;
> +	}
>  	return write_loose_object(sha1, hdr, hdrlen, buf, len, 0);
>  }

-- 
Shawn.