Re: [PATCH 1/9] close another possibility for propagating pack corruption

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 31, 2008, 08:46 UTC
Message-ID: <7vod11884o.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1225321372-6570-2-git-send-email-nico@cam.org>

Nicolas Pitre <nico@cam.org> writes:

Show 5 quoted lines

> Abstract
> --------
>
> With index v2 we have a per object CRC to allow quick and safe reuse of
> pack data when repacking.  this, however, doesn't currently prevent a

s/this/This/;

Show 8 quoted lines

> stealth corruption from being propagated into a new pack when _not_
> reusing pack data as demonstrated by the modification to t5302 included
> here.
>
> The Context
> -----------
>
> The Git database is all checksumed with SHA1 hashes.  Any kind of

s/checksumed/checksummed/;

Show 6 quoted lines

> ...
> What about the streamed pack case?  Well, any client receiving a pack
> must always consider that pack as untrusty and perform full validation
> anyway, hence no such stealth corruption could be propagated to remote
> repositoryes already.  It is therefore worthless doing local validation
> in that case.

Very nicely done. I've never seen a commit message that needs its own Abstract ;-)

Show 10 quoted lines

> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
> index 59c30d1..0366277 100644
> --- a/builtin-pack-objects.c
> +++ b/builtin-pack-objects.c
> @@ -1689,6 +1689,8 @@ static int add_ref_tag(const char *path, const unsigned char *sha1, int flag, vo
>  	return 0;
>  }
>  
> +extern int do_check_packed_object_crc;
> +

This ought to belong to cache.h or some other header file. Perhaps you did this to avoid unnecessary recompilation (we've discussed this at GitTogether)?

Show 9 quoted lines

> @@ -1697,6 +1699,16 @@ static void prepare_pack(int window, int depth)
>  
>  	get_object_details();
>  
> +	/*
> +	 * If we're locally repacking then we need to be doubly careful
> +	 * from now on in order to make sure no stealth corruption gets
> +	 * propagated to the new pack.  Clients receiving streamed packs
> +	 * should validate everything they get anyway so no need to incure

s/incure/incur/;