Re: [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format"
From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:42:33
Possibly related (same subject, not in this thread)
- 2016-06-15 · [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format" · Linus Torvalds <torvalds@osdl.org>
- 2016-06-15 · Re: [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format" · Johannes Schindelin <hidden>
- 2016-06-15 · Re: [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format" · Linus Torvalds <torvalds@osdl.org>
- 2016-06-15 · Re: [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format" · Johannes Schindelin <hidden>
- 2016-06-15 · Re: [PATCH 2/3] sha1_file: add the ability to parse objects in "pack file format" · Linus Torvalds <torvalds@osdl.org>
Hi, On Tue, 11 Jul 2006, Linus Torvalds wrote:
On Tue, 11 Jul 2006, Junio C Hamano wrote:quoted
I do not like to rely too heavily on what zlib compression's beginning of stream looks like.Well, I normally would agree with you if it was a "oh, all our zlib objects seem to start with 0x78" thing, but after having dug into both the zlib standard (which is actually an RFC, not just some random thing), and looked at the sources, it's definitely the case that the "0x78" byte isn't just an implementation detail. It's actually really part of the specs, and not just happenstance.
Good.
quoted hunk ↗ jump to hunk
--- a/sha1_file.c +++ b/sha1_file.c@@ -684,26 +684,74 @@ static void *map_sha1_file_internal(cons return map; } -static int unpack_sha1_header(z_stream *stream, void *map, unsigned long mapsize, void *buffer, unsigned long size) +static int unpack_sha1_header(z_stream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz) { + unsigned char c; + unsigned int word, bits; + unsigned long size; + static const char *typename[8] = { + NULL, /* OBJ_EXT */ + "commit", "tree", "blob", "tag", + NULL, NULL, NULL + };
I completely forgot to mention that type_names[] is already declared in object.h. Obviously, it is not really important, but maybe it would be better to obey the DRY principle (think addition of "bind" object type).
+ while (!(c & 0x80)) {
+ if (bits >= 8*sizeof(long))Another nit: while it is safe to assume that sizeof(long) == sizeof(unsigned long), it was nevertheless a little confusing to yours truly (especially since you changed it since your last patch).
static void *unpack_sha1_rest(z_stream *stream, void *buffer, unsigned long size)
{
int bytes = strlen(buffer) + 1;
unsigned char *buf = xmalloc(1+size);
+ unsigned long n;
- memcpy(buf, (char *) buffer + bytes, stream->total_out - bytes);
- bytes = stream->total_out - bytes;
+ n = stream->total_out - bytes;
+ if (n > size)
+ n = size;Just out of curiosity: when can this happen? I mean, there is no error or something which could tell the caller that not the whole object was copied.
memset(&stream, 0, sizeof(stream)); deflateInit(&stream, zlib_compression_level); - size = deflateBound(&stream, len+hdrlen); + size = 8 + deflateBound(&stream, len+hdrlen);
Again, I had to think why this is correct. I think it should be something like 2 + sizeof(unsigned long) * 8 / 7, but I did not think all that hard. It looks good to me, but I did not really test... Ciao, Dscho