Re: [PATCH 5/5] support writing uncompressed loose object

From: Shawn O. Pearce <spearce@spearce.org>
Date: Dec 2, 2008, 16:07 UTC
Message-ID: <20081202160706.GN23984@spearce.org>
In-Reply-To: <4934975E.2010601@gmail.com>

Liu Yubao <yubao.liu@gmail.com> wrote:

> 
> Signed-off-by: Liu Yubao <yubao.liu@gmail.com>

IMHO, this needs more description in the commit message.

Show 10 quoted lines

> diff --git a/sha1_file.c b/sha1_file.c
> index 05a9fa3..053b564 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2328,7 +2328,7 @@ static int create_tmpfile(char *buffer, size_t bufsiz, const char *filename)
>  }
>  
>  static int write_loose_object(const unsigned char *sha1, char *hdr, int hdrlen,
> -			      void *buf, unsigned long len, time_t mtime)
> +			      void *buf, unsigned long len, time_t mtime, int dont_deflate)

Passing this as an argument is pointless. It should be a repository wide configuration option in core, so you can declare it a static and allow git_config to populate it. Defaulting to 1 (no compression) like you do elsewhere in the patch isn't good.

I'm still against this file format change. The series itself isn't that bad, and the buffer overflow catch in parse_sha1_header() may be something worthwhile fixing. But I'm still not sold that introducing a new loose object format is worth it.

I'd rather use a binary header encoding like the new-style/in-pack format rather than the older style text headers. Its faster to parse for one thing.

Your changes in the reading code cause a copy of the buffer we mmap()'d. That sort of ruins your argument that this change is worthwhile because concurrent processes on the same host can mmap the same buffer and save memory. We're still copying the buffer anyway. I probably should have commented on that in patch 4/5, but I just realized it, so I'm saying it here.

-- 
Shawn.