Shawn O. Pearce wrote: > Liu Yubao <yubao.liu@xxxxxxxxx> wrote: >> Signed-off-by: Liu Yubao <yubao.liu@xxxxxxxxx> > > IMHO, this needs more description in the commit message. > >> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
