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

Shawn O. Pearce wrote:

Liu Yubao [off-list ref] wrote:

quoted

Signed-off-by: Liu Yubao <redacted>

IMHO, this needs more description in the commit message.

quoted

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.

Aha, sorry again, I sent the patch series as separate topics by mistake.

I considered adding a configuration variable, the patch series are sent
just to see whether the idea is worth.

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.

The key point I suggest is to use *uncompressed* loose object, I didn't
change the format of uncompressed loose object because I don't want
to distract your attention and keep the patches small.

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.

Yes, I mentioned it in the cover letter(sigh, sorry!)

I didn't use the mapped buffer directly because other functions required
a null terminated buffer to parse data part of loose object. It can be
fixed but I don't want to make the patches too big.

The two big pros of uncompressed loose object are:

*) avoid compressing and uncompressing loose objects    (I have implemented it)
*) use memory mapped loose object directly              (I havn't implemented it)


Thank you for reviewing my patches, seems the idea to use uncompressed loose
object isn't attractive enough, I will keep the patches locally.


Best regards,

Liu Yubao