Re: [PATCH] Unify write_index_file functions

From: Nicolas Pitre <nico@cam.org>
Date: Jun 1, 2007, 20:54 UTC
Message-ID: <alpine.LFD.0.99.0706011638250.12885@xanadu.home>
In-Reply-To: <20070601194856.66DFB4D7206@potomac.gnat.com>

On Fri, 1 Jun 2007, Geert Bosch wrote:

Show 5 quoted lines

> This patch creates a new pack-idx.c file containing a unified version of
> the write_index_file functions in builtin-pack-objects.c and index-pack.c.
> As the name "index" is overloaded in git, move in the direction
> of using "idx" and "pack idx" when refering to the pack index.
> There should be no change in functionality.

I intended to do exactly that (I even mentioned it in 81a216a5d6) but I'm glad you beat me to it.

A few comments.

Please use pack-write.c rather than a new file. This pack-write.c was created exactly to gather common pack writing tasks.

Please also consider removing the pack index writing code from fast-import.c as well.

Show 8 quoted lines

> @@ -24,9 +24,10 @@ git-pack-objects [{ -q | --progress | --all-progress }] [--max-pack-size=N] \n\
>  
>  struct object_entry {
>  	unsigned char sha1[20];
> -	uint32_t crc32;		/* crc of raw pack data for this object */
>  	off_t offset;		/* offset into the final pack file */
>  	unsigned long size;	/* uncompressed size */
> +	uint32_t crc32;		/* crc of raw pack data for this object */

Don't do this. The crc32 field was carefully placed so the offset field is 64-bit aligned with no need for any padding.

In fact, those 3 fields should probably be defined in a structure of their own rather than hoping that no one will fail to change the ordering in all places.

Other than that it looks pretty good.

Nicolas

Nicolas