# [PATCH 2/3] pack-objects: use fixup_pack_header_footer()'s validation mode 16 messages from 2008-08-29 to 2008-08-31. Participants: Nicolas Pitre, Shawn O. Pearce, Junio C Hamano. Thread: https://gitlist.dev/t/15270 ## Nicolas Pitre, 2008-08-29 02:05 Subject: [PATCH 0/3] don't let disk corruptions escape pack SHA1 checksum Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.10.0808282142490.1624%40xanadu.home In-Reply-To: ``` On Wed, 27 Aug 2008, Nicolas Pitre wrote: > On Wed, 27 Aug 2008, Linus Torvalds wrote: > > > However, we do have this odd case of fixing up the pack after-the-fact > > when we receive it from somebody else (because we get a thin pack and > > don't know how many objects the final result will have). And that case > > seems to be not as safe, because it > > > > - re-reads the file to recompute the SHA1 > > > > This is understandable, and it's fairly ok, but it does mean that there > > is a bigger chance of the SHA1 matching if something has corrupted the > > file in the meantime! > > I think that can be fixed. When reading the file back, it is possible > to compute 2 sha1s: one to compare with the recieved one using original > pack header, and the second which would be the final one. FRom a > certain offset, new objects were added, so that first sha1 is validated > against the received one and reset, and at the end, it should correspond > to the sha1 of added objects that we should compute when writing them. So here is 3 patches for this in C git (Shawn already did it for jgit). There are 3 spots where this needs to be plugged: in pack-objects, index-pack and fast-import. I did the first two. I don't think this is that pertinent in fast-import because: - we want fast-import to be fast - after fast-import is done, a full repack is typically done which would revalidate everything (maybe fast-import should use index v2 with its per object CRC32 though) But maybe I'm wrong about that. In any case, I am leaving the fast-import to someone more knowledgeable about its code than I do. Nicolas Pitre (3): improve reliability of fixup_pack_header_footer() pack-objects: use fixup_pack_header_footer()'s validation mode index-pack: use fixup_pack_header_footer()'s validation mode Nicolas ``` ## Nicolas Pitre, 2008-08-29 02:07 Subject: [PATCH 1/3] improve reliability of fixup_pack_header_footer() Message-ID: <1219975624-7653-1-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1219975624-7653-1-git-send-email-nico%40cam.org In-Reply-To: ``` Currently, this function has the potential to read corrupted pack data from disk and give it a valid SHA1 checksum. Let's add the ability to validate SHA1 checksum of existing data along the way, including before and after any arbitrary point in the pack. Signed-off-by: Nicolas Pitre --- builtin-pack-objects.c | 3 +- fast-import.c | 3 +- index-pack.c | 3 +- pack-write.c | 78 ++++++++++++++++++++++++++++++++++++++++------- pack.h | 2 +- 5 files changed, 73 insertions(+), 16 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index ef3befe..ec80f14 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -498,7 +498,8 @@ static void write_pack_file(void) sha1close(f, sha1, CSUM_FSYNC); } else { int fd = sha1close(f, NULL, 0); - fixup_pack_header_footer(fd, sha1, pack_tmp_name, nr_written); + fixup_pack_header_footer(fd, sha1, pack_tmp_name, + nr_written, NULL, 0); close(fd); } diff --git a/fast-import.c b/fast-import.c index 7089e6f..d85b3a5 100644 --- a/fast-import.c +++ b/fast-import.c @@ -951,7 +951,8 @@ static void end_packfile(void) close_pack_windows(pack_data); fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1, - pack_data->pack_name, object_count); + pack_data->pack_name, object_count, + NULL, 0); close(pack_data->pack_fd); idx_name = keep_pack(create_index()); diff --git a/index-pack.c b/index-pack.c index 728af7d..411b80d 100644 --- a/index-pack.c +++ b/index-pack.c @@ -982,7 +982,8 @@ int main(int argc, char **argv) nr_objects - nr_objects_initial); stop_progress_msg(&progress, msg); fixup_pack_header_footer(output_fd, sha1, - curr_pack, nr_objects); + curr_pack, nr_objects, + NULL, 0); } if (nr_deltas != nr_resolved_deltas) die("pack has %d unresolved deltas", diff --git a/pack-write.c b/pack-write.c index ddcfd37..0c0abce 100644 --- a/pack-write.c +++ b/pack-write.c @@ -144,41 +144,95 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects, return index_name; } +/* + * Update pack header with object_count and compute new SHA1 for pack data + * associated to pack_fd, and write that SHA1 at the end. That new SHA1 + * is also returned in new_pack_sha1. + * + * If partial_pack_sha1 is non null, then the SHA1 of the existing pack + * (without the header update) is computed and validated against the one + * provided in partial_pack_sha1. The validation is performed at + * partial_pack_offset bytes in the pack file, or at the end of the pack + * file if partial_pack_offset is zero. Also, when partial_pack_offset is + * non zero, the SHA1 of the remaining data (i.e. from partial_pack_offset + * to the end) is returned in partial_pack_sha1. + * + * Note that new_pack_sha1 is updated last, so both new_pack_sha1 and + * partial_pack_sha1 can refer to the same buffer if the caller is not + * interested in the resulting SHA1 of pack data above partial_pack_offset. + */ void fixup_pack_header_footer(int pack_fd, - unsigned char *pack_file_sha1, + unsigned char *new_pack_sha1, const char *pack_name, - uint32_t object_count) + uint32_t object_count, + unsigned char *partial_pack_sha1, + off_t partial_pack_offset) { static const int buf_sz = 128 * 1024; - SHA_CTX c; + SHA_CTX old_sha1_ctx, new_sha1_ctx; struct pack_header hdr; char *buf; + SHA1_Init(&old_sha1_ctx); + SHA1_Init(&new_sha1_ctx); + + if (partial_pack_sha1 && !partial_pack_offset) { + partial_pack_offset = lseek(pack_fd, 0, SEEK_CUR); + if (partial_pack_offset == (off_t)-1) + die("Can't get size of %s: %s", pack_name, strerror(errno)); + } + if (lseek(pack_fd, 0, SEEK_SET) != 0) - die("Failed seeking to start: %s", strerror(errno)); + die("Failed seeking to start of %s: %s", pack_name, strerror(errno)); if (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr)) die("Unable to reread header of %s: %s", pack_name, strerror(errno)); if (lseek(pack_fd, 0, SEEK_SET) != 0) - die("Failed seeking to start: %s", strerror(errno)); + die("Failed seeking to start of %s: %s", pack_name, strerror(errno)); + SHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr)); hdr.hdr_entries = htonl(object_count); + SHA1_Update(&new_sha1_ctx, &hdr, sizeof(hdr)); write_or_die(pack_fd, &hdr, sizeof(hdr)); - - SHA1_Init(&c); - SHA1_Update(&c, &hdr, sizeof(hdr)); + partial_pack_offset -= sizeof(hdr); buf = xmalloc(buf_sz); for (;;) { - ssize_t n = xread(pack_fd, buf, buf_sz); + ssize_t m, n; + m = (partial_pack_sha1 && partial_pack_offset < buf_sz) ? + partial_pack_offset : buf_sz; + n = xread(pack_fd, buf, m); if (!n) break; if (n < 0) die("Failed to checksum %s: %s", pack_name, strerror(errno)); - SHA1_Update(&c, buf, n); + SHA1_Update(&new_sha1_ctx, buf, n); + + if (!partial_pack_sha1) + continue; + + SHA1_Update(&old_sha1_ctx, buf, n); + partial_pack_offset -= n; + if (partial_pack_offset == 0) { + unsigned char sha1[20]; + SHA1_Final(sha1, &old_sha1_ctx); + if (hashcmp(sha1, partial_pack_sha1) != 0) + die("Unexpected checksum for %s " + "(disk corruption?)", pack_name); + /* + * Now let's compute the SHA1 of the remainder of the + * pack, which also means making partial_pack_offset + * big enough not to matter anymore. + */ + SHA1_Init(&old_sha1_ctx); + partial_pack_offset = ~partial_pack_offset; + partial_pack_offset -= MSB(partial_pack_offset, 1); + } } free(buf); - SHA1_Final(pack_file_sha1, &c); - write_or_die(pack_fd, pack_file_sha1, 20); + if (partial_pack_sha1) + SHA1_Final(partial_pack_sha1, &old_sha1_ctx); + SHA1_Final(new_pack_sha1, &new_sha1_ctx); + write_or_die(pack_fd, new_pack_sha1, 20); fsync_or_die(pack_fd, pack_name); } diff --git a/pack.h b/pack.h index 76e6aa2..a883334 100644 --- a/pack.h +++ b/pack.h @@ -58,7 +58,7 @@ struct pack_idx_entry { extern char *write_idx_file(char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1); extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr); extern int verify_pack(struct packed_git *); -extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t); +extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t); extern char *index_pack_lockfile(int fd); #define PH_ERROR_EOF (-1) -- 1.6.0.1.174.g97d7e.dirty ``` ## Nicolas Pitre, 2008-08-29 02:07 Subject: [PATCH 2/3] pack-objects: use fixup_pack_header_footer()'s validation mode Message-ID: <1219975624-7653-2-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1219975624-7653-2-git-send-email-nico%40cam.org In-Reply-To: <1219975624-7653-1-git-send-email-nico@cam.org> ``` When limiting the pack size, a new header has to be written to the pack and a new SHA1 computed. Make sure that the SHA1 of what is being read back matches the SHA1 of what was written. Signed-off-by: Nicolas Pitre --- builtin-pack-objects.c | 4 ++-- csum-file.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index ec80f14..487943e 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -497,9 +497,9 @@ static void write_pack_file(void) } else if (nr_written == nr_remaining) { sha1close(f, sha1, CSUM_FSYNC); } else { - int fd = sha1close(f, NULL, 0); + int fd = sha1close(f, sha1, 0); fixup_pack_header_footer(fd, sha1, pack_tmp_name, - nr_written, NULL, 0); + nr_written, sha1, 0); close(fd); } diff --git a/csum-file.c b/csum-file.c index ace64f1..2838954 100644 --- a/csum-file.c +++ b/csum-file.c @@ -42,11 +42,11 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags) sha1flush(f, offset); f->offset = 0; } + SHA1_Final(f->buffer, &f->ctx); + if (result) + hashcpy(result, f->buffer); if (flags & (CSUM_CLOSE | CSUM_FSYNC)) { /* write checksum and close fd */ - SHA1_Final(f->buffer, &f->ctx); - if (result) - hashcpy(result, f->buffer); sha1flush(f, 20); if (flags & CSUM_FSYNC) fsync_or_die(f->fd, f->name); -- 1.6.0.1.174.g97d7e.dirty ``` ## Nicolas Pitre, 2008-08-29 02:07 Subject: [PATCH 3/3] index-pack: use fixup_pack_header_footer()'s validation mode Message-ID: <1219975624-7653-3-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1219975624-7653-3-git-send-email-nico%40cam.org In-Reply-To: <1219975624-7653-2-git-send-email-nico@cam.org> ``` When completing a thin pack, a new header has to be written to the pack and a new SHA1 computed. Make sure that the SHA1 of what is being read back matches the SHA1 of what was written for both: the original pack and the appended objects. To do so, a couple write_or_die() calls were converted to sha1write() which has the advantage of doing some buffering as well as handling SHA1 and CRC32 checksum already. Signed-off-by: Nicolas Pitre --- index-pack.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-) diff --git a/index-pack.c b/index-pack.c index 411b80d..a6e91fe 100644 --- a/index-pack.c +++ b/index-pack.c @@ -654,7 +654,7 @@ static void parse_pack_objects(unsigned char *sha1) } } -static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_crc) +static int write_compressed(struct sha1file *f, void *in, unsigned int size) { z_stream stream; unsigned long maxsize; @@ -674,13 +674,12 @@ static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_c deflateEnd(&stream); size = stream.total_out; - write_or_die(fd, out, size); - *obj_crc = crc32(*obj_crc, out, size); + sha1write(f, out, size); free(out); return size; } -static struct object_entry *append_obj_to_pack( +static struct object_entry *append_obj_to_pack(struct sha1file *f, const unsigned char *sha1, void *buf, unsigned long size, enum object_type type) { @@ -696,15 +695,15 @@ static struct object_entry *append_obj_to_pack( s >>= 7; } header[n++] = c; - write_or_die(output_fd, header, n); - obj[0].idx.crc32 = crc32(0, Z_NULL, 0); - obj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n); + crc32_begin(f); + sha1write(f, header, n); obj[0].size = size; obj[0].hdr_size = n; obj[0].type = type; obj[0].real_type = type; obj[1].idx.offset = obj[0].idx.offset + n; - obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32); + obj[1].idx.offset += write_compressed(f, buf, size); + obj[0].idx.crc32 = crc32_end(f); hashcpy(obj->idx.sha1, sha1); return obj; } @@ -716,7 +715,7 @@ static int delta_pos_compare(const void *_a, const void *_b) return a->obj_no - b->obj_no; } -static void fix_unresolved_deltas(int nr_unresolved) +static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved) { struct delta_entry **sorted_by_pos; int i, n = 0; @@ -754,8 +753,8 @@ static void fix_unresolved_deltas(int nr_unresolved) if (check_sha1_signature(d->base.sha1, base_obj.data, base_obj.size, typename(type))) die("local object %s is corrupt", sha1_to_hex(d->base.sha1)); - base_obj.obj = append_obj_to_pack(d->base.sha1, base_obj.data, - base_obj.size, type); + base_obj.obj = append_obj_to_pack(f, d->base.sha1, + base_obj.data, base_obj.size, type); link_base_data(NULL, &base_obj); find_delta_children(&d->base, &first, &last); @@ -875,7 +874,7 @@ int main(int argc, char **argv) const char *keep_name = NULL, *keep_msg = NULL; char *index_name_buf = NULL, *keep_name_buf = NULL; struct pack_idx_entry **idx_objects; - unsigned char sha1[20]; + unsigned char pack_sha1[20]; int nongit = 0; setup_git_directory_gently(&nongit); @@ -962,13 +961,15 @@ int main(int argc, char **argv) parse_pack_header(); objects = xmalloc((nr_objects + 1) * sizeof(struct object_entry)); deltas = xmalloc(nr_objects * sizeof(struct delta_entry)); - parse_pack_objects(sha1); + parse_pack_objects(pack_sha1); if (nr_deltas == nr_resolved_deltas) { stop_progress(&progress); /* Flush remaining pack final 20-byte SHA1. */ flush(); } else { if (fix_thin_pack) { + struct sha1file *f; + unsigned char read_sha1[20], tail_sha1[20]; char msg[48]; int nr_unresolved = nr_deltas - nr_resolved_deltas; int nr_objects_initial = nr_objects; @@ -977,13 +978,19 @@ int main(int argc, char **argv) objects = xrealloc(objects, (nr_objects + nr_unresolved + 1) * sizeof(*objects)); - fix_unresolved_deltas(nr_unresolved); + f = sha1fd(output_fd, curr_pack); + fix_unresolved_deltas(f, nr_unresolved); sprintf(msg, "completed with %d local objects", nr_objects - nr_objects_initial); stop_progress_msg(&progress, msg); - fixup_pack_header_footer(output_fd, sha1, + sha1close(f, tail_sha1, 0); + hashcpy(read_sha1, pack_sha1); + fixup_pack_header_footer(output_fd, pack_sha1, curr_pack, nr_objects, - NULL, 0); + read_sha1, consumed_bytes-20); + if (hashcmp(read_sha1, tail_sha1) != 0) + die("Unexpected tail checksum for %s " + "(disk corruption?)", curr_pack); } if (nr_deltas != nr_resolved_deltas) die("pack has %d unresolved deltas", @@ -996,13 +1003,13 @@ int main(int argc, char **argv) idx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *)); for (i = 0; i < nr_objects; i++) idx_objects[i] = &objects[i].idx; - curr_index = write_idx_file(index_name, idx_objects, nr_objects, sha1); + curr_index = write_idx_file(index_name, idx_objects, nr_objects, pack_sha1); free(idx_objects); final(pack_name, curr_pack, index_name, curr_index, keep_name, keep_msg, - sha1); + pack_sha1); free(objects); free(index_name_buf); free(keep_name_buf); -- 1.6.0.1.174.g97d7e.dirty ``` ## Shawn O. Pearce, 2008-08-29 04:44 Subject: Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer() Message-ID: <20080829044459.GA28492@spearce.org> URL: https://gitlist.dev/e/20080829044459.GA28492%40spearce.org In-Reply-To: <1219975624-7653-1-git-send-email-nico@cam.org> ``` Nicolas Pitre wrote: > Currently, this function has the potential to read corrupted pack data > from disk and give it a valid SHA1 checksum. Let's add the ability to > validate SHA1 checksum of existing data along the way, including before > and after any arbitrary point in the pack. I found your implementation in fixup_pack_header to be very contorted. Did you read the JGit patch I posted? I think its implementation is more elegant and easier to follow. Oh, and its BSD licensed... so easy for a GPLv2 project to borrow. ;-) > void fixup_pack_header_footer(int pack_fd, ... > + if (partial_pack_sha1 && !partial_pack_offset) { > + partial_pack_offset = lseek(pack_fd, 0, SEEK_CUR); > + if (partial_pack_offset == (off_t)-1) Eh? I find this to be nonsense. If the caller gave us a SHA-1 but wants us to do fixup then they have increased the size of the pack. Which means they must pass us the original length. Computing it here is relying on the caller leaving the file pointer positioned at the old end. Who the heck does that after fixing a thin pack? > buf = xmalloc(buf_sz); > for (;;) { > - ssize_t n = xread(pack_fd, buf, buf_sz); > + ssize_t m, n; > + m = (partial_pack_sha1 && partial_pack_offset < buf_sz) ? > + partial_pack_offset : buf_sz; > + n = xread(pack_fd, buf, m); Why not read the full buf_sz per round and then use only part of the buffer in SHA1_Update(&old_sha1_ctx)? It took me a while to figure out what the heck you were trying to measure here. It also would be a few less CPU instructions if you always just read buf_sz and leave the min test to after the "if (!partial_pack_sha1)" below. > + if (!partial_pack_sha1) > + continue; > + > + SHA1_Update(&old_sha1_ctx, buf, n); > + partial_pack_offset -= n; > + if (partial_pack_offset == 0) { > + unsigned char sha1[20]; > + SHA1_Final(sha1, &old_sha1_ctx); > + if (hashcmp(sha1, partial_pack_sha1) != 0) > + die("Unexpected checksum for %s " > + "(disk corruption?)", pack_name); > + /* > + * Now let's compute the SHA1 of the remainder of the > + * pack, which also means making partial_pack_offset > + * big enough not to matter anymore. > + */ > + SHA1_Init(&old_sha1_ctx); > + partial_pack_offset = ~partial_pack_offset; > + partial_pack_offset -= MSB(partial_pack_offset, 1); That's freaking brilliant. And something I missed in JGit. The way its implemented is downright difficult to follow though. I'll admit it took me a good 10 minutes to understand what you were doing here, and why. -- Shawn. ``` ## Shawn O. Pearce, 2008-08-29 04:55 Subject: Re: [PATCH 0/3] don't let disk corruptions escape pack SHA1 checksum Message-ID: <20080829045535.GB28492@spearce.org> URL: https://gitlist.dev/e/20080829045535.GB28492%40spearce.org In-Reply-To: ``` Nicolas Pitre wrote: > > So here is 3 patches for this in C git (Shawn already did it for jgit). > > There are 3 spots where this needs to be plugged: in pack-objects, > index-pack and fast-import. I did the first two. > > I don't think this is that pertinent in fast-import because: > > - we want fast-import to be fast > > - after fast-import is done, a full repack is typically done which > would revalidate everything (maybe fast-import should use index v2 > with its per object CRC32 though) > > But maybe I'm wrong about that. In any case, I am leaving the > fast-import to someone more knowledgeable about its code than I do. fast-import is the backend to a number of incremental import tools. They stream data in from foreign VCS systems (notably Perforce) and write into a live repository. That repository may not get repacked for days/weeks. We should treat it just like index-pack. So I think we should plug it. The cost to keep a running SHA-1 for what we have written is pretty low. I'd rather trade off a minute or so on an hour long import for data safety than have a chance for data corruption going unnoticed by the end user. And now that index v2 is available, yea, fast-import should write its index in that format so that a later repack can safely delta reuse if it (by rare chance) decides the current packed form is the best representation. This is especially true with "git gc --auto" triggering a repack every so often due to the incremental import packs I just metioned above. I've just been too busy with other things to add index v2 support. -- Shawn. ``` ## Nicolas Pitre, 2008-08-29 13:08 Subject: Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer() Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.10.0808290844200.1624%40xanadu.home In-Reply-To: <20080829044459.GA28492@spearce.org> ``` On Thu, 28 Aug 2008, Shawn O. Pearce wrote: > Nicolas Pitre wrote: > > Currently, this function has the potential to read corrupted pack data > > from disk and give it a valid SHA1 checksum. Let's add the ability to > > validate SHA1 checksum of existing data along the way, including before > > and after any arbitrary point in the pack. > > I found your implementation in fixup_pack_header to be very > contorted. Did you read the JGit patch I posted? I think its > implementation is more elegant and easier to follow. Oh, and its > BSD licensed... so easy for a GPLv2 project to borrow. ;-) Well, actually, I don't find the JGit implementation easier to follow at all. Of course I wrote the C version while you wrote the Java version. Maybe without our respective bias then things are just fine in both cases. > > void fixup_pack_header_footer(int pack_fd, > ... > > + if (partial_pack_sha1 && !partial_pack_offset) { > > + partial_pack_offset = lseek(pack_fd, 0, SEEK_CUR); > > + if (partial_pack_offset == (off_t)-1) > > Eh? > > I find this to be nonsense. If the caller gave us a SHA-1 but > wants us to do fixup then they have increased the size of the pack. > Which means they must pass us the original length. Computing it > here is relying on the caller leaving the file pointer positioned > at the old end. Who the heck does that after fixing a thin pack? This is not for thin packs but for split packs in repack-objects. And yeah, the caller has the offset information already in that case too, so this could be removed. It just felt more generic that way originally. > > buf = xmalloc(buf_sz); > > for (;;) { > > - ssize_t n = xread(pack_fd, buf, buf_sz); > > + ssize_t m, n; > > + m = (partial_pack_sha1 && partial_pack_offset < buf_sz) ? > > + partial_pack_offset : buf_sz; > > + n = xread(pack_fd, buf, m); > > Why not read the full buf_sz per round and then use only part of > the buffer in SHA1_Update(&old_sha1_ctx)? It took me a while to > figure out what the heck you were trying to measure here. It also > would be a few less CPU instructions if you always just read buf_sz > and leave the min test to after the "if (!partial_pack_sha1)" below. I doubt it. That would only move the offset test from before the read to after it, and actually complicate things for restarting the checksumming after the special offset was crossed. And we're talking about a comparison with a conditional move instructions only. And another thing I had in store (but for which you _again_ beat me to :-) ) is to realign data reads onto filesystem blocks. > > + if (!partial_pack_sha1) > > + continue; > > + > > + SHA1_Update(&old_sha1_ctx, buf, n); > > + partial_pack_offset -= n; > > + if (partial_pack_offset == 0) { > > + unsigned char sha1[20]; > > + SHA1_Final(sha1, &old_sha1_ctx); > > + if (hashcmp(sha1, partial_pack_sha1) != 0) > > + die("Unexpected checksum for %s " > > + "(disk corruption?)", pack_name); > > + /* > > + * Now let's compute the SHA1 of the remainder of the > > + * pack, which also means making partial_pack_offset > > + * big enough not to matter anymore. > > + */ > > + SHA1_Init(&old_sha1_ctx); > > + partial_pack_offset = ~partial_pack_offset; > > + partial_pack_offset -= MSB(partial_pack_offset, 1); > > That's freaking brilliant. And something I missed in JGit. > > The way its implemented is downright difficult to follow though. > I'll admit it took me a good 10 minutes to understand what you were > doing here, and why. Again maybe that's just because you're just too biased by your own implementation. I don't consider this code particularly obscur. Nicolas ``` ## Shawn O. Pearce, 2008-08-29 14:30 Subject: Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer() Message-ID: <20080829143023.GA7403@spearce.org> URL: https://gitlist.dev/e/20080829143023.GA7403%40spearce.org In-Reply-To: ``` Nicolas Pitre wrote: > On Thu, 28 Aug 2008, Shawn O. Pearce wrote: > > > > I found your implementation in fixup_pack_header to be very > > contorted. Did you read the JGit patch I posted? > > Well, actually, I don't find the JGit implementation easier to follow at > all. Of course I wrote the C version while you wrote the Java version. > Maybe without our respective bias then things are just fine in both > cases. I probably should have gone and edited my eariler comments after I reached the end of the patch and the light dawned about why you are summing the tail. Most of what I found to be complex in your code was just to handle that boundary condition at partial_pack_offset and restart the checksum for the tail. But I honestly cannot see an easier way to write that, so what you have is just fine. > > > void fixup_pack_header_footer(int pack_fd, > > ... > > > + if (partial_pack_sha1 && !partial_pack_offset) { > > > + partial_pack_offset = lseek(pack_fd, 0, SEEK_CUR); > > > + if (partial_pack_offset == (off_t)-1) > > > > Eh? > > > > I find this to be nonsense. > > This is not for thin packs but for split packs in repack-objects. And > yeah, the caller has the offset information already in that case too, so > this could be removed. It just felt more generic that way originally. Oh, yea, that makes sense. It still seems like playing with fire. I'd rather the caller pass in the proper offset than rely on it being the current position of the fd. Especially if the caller does actually have it available. If you change anything, I'd like to see this lseek(SEEK_CUR) go away. > And another thing I had in store (but for which you _again_ beat me to :-) ) > is to realign data reads onto filesystem blocks. That _really_ made the JGit code ugly. But I think its worth it. I also want to try and buffer the whole object appending we do during fixThinPack(), as right now we write the object header in one write and then compressed data bursts in the others. Moving it to at least write a full 4k at a time should remove about 2 write calls per object. > > That's freaking brilliant. And something I missed in JGit. > > The way its implemented is downright difficult to follow though. > > I'll admit it took me a good 10 minutes to understand what you were > > doing here, and why. > > Again maybe that's just because you're just too biased by your own > implementation. I don't consider this code particularly obscur. My own code in JGit got "obscure" when I added this check too. I take back the remark above. -- Shawn. ``` ## Nicolas Pitre, 2008-08-29 20:07 Subject: [PATCH 0/5] pack header rewriting improvements Message-ID: <1220040482-10108-1-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1220040482-10108-1-git-send-email-nico%40cam.org In-Reply-To: <20080829143023.GA7403@spearce.org> ``` This replaces my earlier 3-patch series called "don't let disk corruptions escape pack SHA1 checksum" with this set. Same purpose but better! Once again I'm leaving fast-import out. I'm sure Shawn will figure it out much faster than I could. Nicolas ``` ## Nicolas Pitre, 2008-08-29 20:07 Subject: [PATCH 1/5] pack-objects: improve returned information from write_one() Message-ID: <1220040482-10108-2-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1220040482-10108-2-git-send-email-nico%40cam.org In-Reply-To: <1220040482-10108-1-git-send-email-nico@cam.org> ``` This function returns 0 when the current object couldn't be written due to the pack size limit, otherwise the current offset in the pack. There is a problem with this approach however, since current object could be a delta and its delta base might just have been written in the same write_one() call, but those successfully written objects are not accounted in the offset variable tracked by the caller. Currently this is not an issue but a subsequent patch will need this. Signed-off-by: Nicolas Pitre --- builtin-pack-objects.c | 29 ++++++++++++----------------- 1 files changed, 12 insertions(+), 17 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index ef3befe..a615fcc 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -410,25 +410,22 @@ static unsigned long write_object(struct sha1file *f, return hdrlen + datalen; } -static off_t write_one(struct sha1file *f, +static int write_one(struct sha1file *f, struct object_entry *e, - off_t offset) + off_t *offset) { unsigned long size; /* offset is non zero if object is written already. */ if (e->idx.offset || e->preferred_base) - return offset; + return 1; /* if we are deltified, write out base object first. */ - if (e->delta) { - offset = write_one(f, e->delta, offset); - if (!offset) - return 0; - } + if (e->delta && !write_one(f, e->delta, offset)) + return 0; - e->idx.offset = offset; - size = write_object(f, e, offset); + e->idx.offset = *offset; + size = write_object(f, e, *offset); if (!size) { e->idx.offset = 0; return 0; @@ -436,9 +433,10 @@ static off_t write_one(struct sha1file *f, written_list[nr_written++] = &e->idx; /* make sure off_t is sufficiently large not to wrap */ - if (offset > offset + size) + if (*offset > *offset + size) die("pack too large for current definition of off_t"); - return offset + size; + *offset += size; + return 1; } /* forward declaration for write_pack_file */ @@ -448,7 +446,7 @@ static void write_pack_file(void) { uint32_t i = 0, j; struct sha1file *f; - off_t offset, offset_one, last_obj_offset = 0; + off_t offset; struct pack_header hdr; uint32_t nr_remaining = nr_result; time_t last_mtime = 0; @@ -480,11 +478,8 @@ static void write_pack_file(void) offset = sizeof(hdr); nr_written = 0; for (; i < nr_objects; i++) { - last_obj_offset = offset; - offset_one = write_one(f, objects + i, offset); - if (!offset_one) + if (!write_one(f, objects + i, &offset)) break; - offset = offset_one; display_progress(progress_state, written); } -- 1.6.0.1.212.g35f9f ``` ## Nicolas Pitre, 2008-08-29 20:07 Subject: [PATCH 2/5] improve reliability of fixup_pack_header_footer() Message-ID: <1220040482-10108-3-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1220040482-10108-3-git-send-email-nico%40cam.org In-Reply-To: <1220040482-10108-2-git-send-email-nico@cam.org> ``` Currently, this function has the potential to read corrupted pack data from disk and give it a valid SHA1 checksum. Let's add the ability to validate SHA1 checksum of existing data along the way, including before and after any arbitrary point in the pack. Signed-off-by: Nicolas Pitre --- builtin-pack-objects.c | 3 +- fast-import.c | 3 +- index-pack.c | 3 +- pack-write.c | 71 +++++++++++++++++++++++++++++++++++++++-------- pack.h | 2 +- 5 files changed, 66 insertions(+), 16 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index a615fcc..79417cb 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -493,7 +493,8 @@ static void write_pack_file(void) sha1close(f, sha1, CSUM_FSYNC); } else { int fd = sha1close(f, NULL, 0); - fixup_pack_header_footer(fd, sha1, pack_tmp_name, nr_written); + fixup_pack_header_footer(fd, sha1, pack_tmp_name, + nr_written, NULL, 0); close(fd); } diff --git a/fast-import.c b/fast-import.c index 7089e6f..d85b3a5 100644 --- a/fast-import.c +++ b/fast-import.c @@ -951,7 +951,8 @@ static void end_packfile(void) close_pack_windows(pack_data); fixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1, - pack_data->pack_name, object_count); + pack_data->pack_name, object_count, + NULL, 0); close(pack_data->pack_fd); idx_name = keep_pack(create_index()); diff --git a/index-pack.c b/index-pack.c index 728af7d..411b80d 100644 --- a/index-pack.c +++ b/index-pack.c @@ -982,7 +982,8 @@ int main(int argc, char **argv) nr_objects - nr_objects_initial); stop_progress_msg(&progress, msg); fixup_pack_header_footer(output_fd, sha1, - curr_pack, nr_objects); + curr_pack, nr_objects, + NULL, 0); } if (nr_deltas != nr_resolved_deltas) die("pack has %d unresolved deltas", diff --git a/pack-write.c b/pack-write.c index ddcfd37..f9776c5 100644 --- a/pack-write.c +++ b/pack-write.c @@ -144,41 +144,88 @@ char *write_idx_file(char *index_name, struct pack_idx_entry **objects, return index_name; } +/* + * Update pack header with object_count and compute new SHA1 for pack data + * associated to pack_fd, and write that SHA1 at the end. That new SHA1 + * is also returned in new_pack_sha1. + * + * If partial_pack_sha1 is non null, then the SHA1 of the existing pack + * (without the header update) is computed and validated against the + * one provided in partial_pack_sha1. The validation is performed at + * partial_pack_offset bytes in the pack file. The SHA1 of the remaining + * data (i.e. from partial_pack_offset to the end) is then computed and + * returned in partial_pack_sha1. + * + * Note that new_pack_sha1 is updated last, so both new_pack_sha1 and + * partial_pack_sha1 can refer to the same buffer if the caller is not + * interested in the resulting SHA1 of pack data above partial_pack_offset. + */ void fixup_pack_header_footer(int pack_fd, - unsigned char *pack_file_sha1, + unsigned char *new_pack_sha1, const char *pack_name, - uint32_t object_count) + uint32_t object_count, + unsigned char *partial_pack_sha1, + off_t partial_pack_offset) { static const int buf_sz = 128 * 1024; - SHA_CTX c; + SHA_CTX old_sha1_ctx, new_sha1_ctx; struct pack_header hdr; char *buf; + SHA1_Init(&old_sha1_ctx); + SHA1_Init(&new_sha1_ctx); + if (lseek(pack_fd, 0, SEEK_SET) != 0) - die("Failed seeking to start: %s", strerror(errno)); + die("Failed seeking to start of %s: %s", pack_name, strerror(errno)); if (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr)) die("Unable to reread header of %s: %s", pack_name, strerror(errno)); if (lseek(pack_fd, 0, SEEK_SET) != 0) - die("Failed seeking to start: %s", strerror(errno)); + die("Failed seeking to start of %s: %s", pack_name, strerror(errno)); + SHA1_Update(&old_sha1_ctx, &hdr, sizeof(hdr)); hdr.hdr_entries = htonl(object_count); + SHA1_Update(&new_sha1_ctx, &hdr, sizeof(hdr)); write_or_die(pack_fd, &hdr, sizeof(hdr)); - - SHA1_Init(&c); - SHA1_Update(&c, &hdr, sizeof(hdr)); + partial_pack_offset -= sizeof(hdr); buf = xmalloc(buf_sz); for (;;) { - ssize_t n = xread(pack_fd, buf, buf_sz); + ssize_t m, n; + m = (partial_pack_sha1 && partial_pack_offset < buf_sz) ? + partial_pack_offset : buf_sz; + n = xread(pack_fd, buf, m); if (!n) break; if (n < 0) die("Failed to checksum %s: %s", pack_name, strerror(errno)); - SHA1_Update(&c, buf, n); + SHA1_Update(&new_sha1_ctx, buf, n); + + if (!partial_pack_sha1) + continue; + + SHA1_Update(&old_sha1_ctx, buf, n); + partial_pack_offset -= n; + if (partial_pack_offset == 0) { + unsigned char sha1[20]; + SHA1_Final(sha1, &old_sha1_ctx); + if (hashcmp(sha1, partial_pack_sha1) != 0) + die("Unexpected checksum for %s " + "(disk corruption?)", pack_name); + /* + * Now let's compute the SHA1 of the remainder of the + * pack, which also means making partial_pack_offset + * big enough not to matter anymore. + */ + SHA1_Init(&old_sha1_ctx); + partial_pack_offset = ~partial_pack_offset; + partial_pack_offset -= MSB(partial_pack_offset, 1); + } } free(buf); - SHA1_Final(pack_file_sha1, &c); - write_or_die(pack_fd, pack_file_sha1, 20); + if (partial_pack_sha1) + SHA1_Final(partial_pack_sha1, &old_sha1_ctx); + SHA1_Final(new_pack_sha1, &new_sha1_ctx); + write_or_die(pack_fd, new_pack_sha1, 20); fsync_or_die(pack_fd, pack_name); } diff --git a/pack.h b/pack.h index 76e6aa2..a883334 100644 --- a/pack.h +++ b/pack.h @@ -58,7 +58,7 @@ struct pack_idx_entry { extern char *write_idx_file(char *index_name, struct pack_idx_entry **objects, int nr_objects, unsigned char *sha1); extern int check_pack_crc(struct packed_git *p, struct pack_window **w_curs, off_t offset, off_t len, unsigned int nr); extern int verify_pack(struct packed_git *); -extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t); +extern void fixup_pack_header_footer(int, unsigned char *, const char *, uint32_t, unsigned char *, off_t); extern char *index_pack_lockfile(int fd); #define PH_ERROR_EOF (-1) -- 1.6.0.1.212.g35f9f ``` ## Nicolas Pitre, 2008-08-29 20:08 Subject: [PATCH 3/5] pack-objects: use fixup_pack_header_footer()'s validation mode Message-ID: <1220040482-10108-4-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1220040482-10108-4-git-send-email-nico%40cam.org In-Reply-To: <1220040482-10108-3-git-send-email-nico@cam.org> ``` When limiting the pack size, a new header has to be written to the pack and a new SHA1 computed. Make sure that the SHA1 of what is being read back matches the SHA1 of what was written. Signed-off-by: Nicolas Pitre --- builtin-pack-objects.c | 4 ++-- csum-file.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c index 79417cb..ba2cf00 100644 --- a/builtin-pack-objects.c +++ b/builtin-pack-objects.c @@ -492,9 +492,9 @@ static void write_pack_file(void) } else if (nr_written == nr_remaining) { sha1close(f, sha1, CSUM_FSYNC); } else { - int fd = sha1close(f, NULL, 0); + int fd = sha1close(f, sha1, 0); fixup_pack_header_footer(fd, sha1, pack_tmp_name, - nr_written, NULL, 0); + nr_written, sha1, offset); close(fd); } diff --git a/csum-file.c b/csum-file.c index ace64f1..2838954 100644 --- a/csum-file.c +++ b/csum-file.c @@ -42,11 +42,11 @@ int sha1close(struct sha1file *f, unsigned char *result, unsigned int flags) sha1flush(f, offset); f->offset = 0; } + SHA1_Final(f->buffer, &f->ctx); + if (result) + hashcpy(result, f->buffer); if (flags & (CSUM_CLOSE | CSUM_FSYNC)) { /* write checksum and close fd */ - SHA1_Final(f->buffer, &f->ctx); - if (result) - hashcpy(result, f->buffer); sha1flush(f, 20); if (flags & CSUM_FSYNC) fsync_or_die(f->fd, f->name); -- 1.6.0.1.212.g35f9f ``` ## Nicolas Pitre, 2008-08-29 20:08 Subject: [PATCH 4/5] index-pack: use fixup_pack_header_footer()'s validation mode Message-ID: <1220040482-10108-5-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1220040482-10108-5-git-send-email-nico%40cam.org In-Reply-To: <1220040482-10108-4-git-send-email-nico@cam.org> ``` When completing a thin pack, a new header has to be written to the pack and a new SHA1 computed. Make sure that the SHA1 of what is being read back matches the SHA1 of what was written for both: the original pack and the appended objects. To do so, a couple write_or_die() calls were converted to sha1write() which has the advantage of doing some buffering as well as handling SHA1 and CRC32 checksum already. Signed-off-by: Nicolas Pitre --- index-pack.c | 43 +++++++++++++++++++++++++------------------ 1 files changed, 25 insertions(+), 18 deletions(-) diff --git a/index-pack.c b/index-pack.c index 411b80d..a6e91fe 100644 --- a/index-pack.c +++ b/index-pack.c @@ -654,7 +654,7 @@ static void parse_pack_objects(unsigned char *sha1) } } -static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_crc) +static int write_compressed(struct sha1file *f, void *in, unsigned int size) { z_stream stream; unsigned long maxsize; @@ -674,13 +674,12 @@ static int write_compressed(int fd, void *in, unsigned int size, uint32_t *obj_c deflateEnd(&stream); size = stream.total_out; - write_or_die(fd, out, size); - *obj_crc = crc32(*obj_crc, out, size); + sha1write(f, out, size); free(out); return size; } -static struct object_entry *append_obj_to_pack( +static struct object_entry *append_obj_to_pack(struct sha1file *f, const unsigned char *sha1, void *buf, unsigned long size, enum object_type type) { @@ -696,15 +695,15 @@ static struct object_entry *append_obj_to_pack( s >>= 7; } header[n++] = c; - write_or_die(output_fd, header, n); - obj[0].idx.crc32 = crc32(0, Z_NULL, 0); - obj[0].idx.crc32 = crc32(obj[0].idx.crc32, header, n); + crc32_begin(f); + sha1write(f, header, n); obj[0].size = size; obj[0].hdr_size = n; obj[0].type = type; obj[0].real_type = type; obj[1].idx.offset = obj[0].idx.offset + n; - obj[1].idx.offset += write_compressed(output_fd, buf, size, &obj[0].idx.crc32); + obj[1].idx.offset += write_compressed(f, buf, size); + obj[0].idx.crc32 = crc32_end(f); hashcpy(obj->idx.sha1, sha1); return obj; } @@ -716,7 +715,7 @@ static int delta_pos_compare(const void *_a, const void *_b) return a->obj_no - b->obj_no; } -static void fix_unresolved_deltas(int nr_unresolved) +static void fix_unresolved_deltas(struct sha1file *f, int nr_unresolved) { struct delta_entry **sorted_by_pos; int i, n = 0; @@ -754,8 +753,8 @@ static void fix_unresolved_deltas(int nr_unresolved) if (check_sha1_signature(d->base.sha1, base_obj.data, base_obj.size, typename(type))) die("local object %s is corrupt", sha1_to_hex(d->base.sha1)); - base_obj.obj = append_obj_to_pack(d->base.sha1, base_obj.data, - base_obj.size, type); + base_obj.obj = append_obj_to_pack(f, d->base.sha1, + base_obj.data, base_obj.size, type); link_base_data(NULL, &base_obj); find_delta_children(&d->base, &first, &last); @@ -875,7 +874,7 @@ int main(int argc, char **argv) const char *keep_name = NULL, *keep_msg = NULL; char *index_name_buf = NULL, *keep_name_buf = NULL; struct pack_idx_entry **idx_objects; - unsigned char sha1[20]; + unsigned char pack_sha1[20]; int nongit = 0; setup_git_directory_gently(&nongit); @@ -962,13 +961,15 @@ int main(int argc, char **argv) parse_pack_header(); objects = xmalloc((nr_objects + 1) * sizeof(struct object_entry)); deltas = xmalloc(nr_objects * sizeof(struct delta_entry)); - parse_pack_objects(sha1); + parse_pack_objects(pack_sha1); if (nr_deltas == nr_resolved_deltas) { stop_progress(&progress); /* Flush remaining pack final 20-byte SHA1. */ flush(); } else { if (fix_thin_pack) { + struct sha1file *f; + unsigned char read_sha1[20], tail_sha1[20]; char msg[48]; int nr_unresolved = nr_deltas - nr_resolved_deltas; int nr_objects_initial = nr_objects; @@ -977,13 +978,19 @@ int main(int argc, char **argv) objects = xrealloc(objects, (nr_objects + nr_unresolved + 1) * sizeof(*objects)); - fix_unresolved_deltas(nr_unresolved); + f = sha1fd(output_fd, curr_pack); + fix_unresolved_deltas(f, nr_unresolved); sprintf(msg, "completed with %d local objects", nr_objects - nr_objects_initial); stop_progress_msg(&progress, msg); - fixup_pack_header_footer(output_fd, sha1, + sha1close(f, tail_sha1, 0); + hashcpy(read_sha1, pack_sha1); + fixup_pack_header_footer(output_fd, pack_sha1, curr_pack, nr_objects, - NULL, 0); + read_sha1, consumed_bytes-20); + if (hashcmp(read_sha1, tail_sha1) != 0) + die("Unexpected tail checksum for %s " + "(disk corruption?)", curr_pack); } if (nr_deltas != nr_resolved_deltas) die("pack has %d unresolved deltas", @@ -996,13 +1003,13 @@ int main(int argc, char **argv) idx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *)); for (i = 0; i < nr_objects; i++) idx_objects[i] = &objects[i].idx; - curr_index = write_idx_file(index_name, idx_objects, nr_objects, sha1); + curr_index = write_idx_file(index_name, idx_objects, nr_objects, pack_sha1); free(idx_objects); final(pack_name, curr_pack, index_name, curr_index, keep_name, keep_msg, - sha1); + pack_sha1); free(objects); free(index_name_buf); free(keep_name_buf); -- 1.6.0.1.212.g35f9f ``` ## Nicolas Pitre, 2008-08-29 20:08 Subject: [PATCH 5/5] fixup_pack_header_footer(): use nicely aligned buffer sizes Message-ID: <1220040482-10108-6-git-send-email-nico@cam.org> URL: https://gitlist.dev/e/1220040482-10108-6-git-send-email-nico%40cam.org In-Reply-To: <1220040482-10108-5-git-send-email-nico@cam.org> ``` It should be more efficient to use nicely aligned buffer sizes, either for filesystem operations or SHA1 checksums. Also, using a relatively small nominal size might allow for the data to remain in L1 cache between both SHA1_Update() calls. Signed-off-by: Nicolas Pitre --- pack-write.c | 11 ++++++++--- 1 files changed, 8 insertions(+), 3 deletions(-) diff --git a/pack-write.c b/pack-write.c index f9776c5..939ed56 100644 --- a/pack-write.c +++ b/pack-write.c @@ -167,7 +167,7 @@ void fixup_pack_header_footer(int pack_fd, unsigned char *partial_pack_sha1, off_t partial_pack_offset) { - static const int buf_sz = 128 * 1024; + int aligned_sz, buf_sz = 8 * 1024; SHA_CTX old_sha1_ctx, new_sha1_ctx; struct pack_header hdr; char *buf; @@ -188,10 +188,11 @@ void fixup_pack_header_footer(int pack_fd, partial_pack_offset -= sizeof(hdr); buf = xmalloc(buf_sz); + aligned_sz = buf_sz - sizeof(hdr); for (;;) { ssize_t m, n; - m = (partial_pack_sha1 && partial_pack_offset < buf_sz) ? - partial_pack_offset : buf_sz; + m = (partial_pack_sha1 && partial_pack_offset < aligned_sz) ? + partial_pack_offset : aligned_sz; n = xread(pack_fd, buf, m); if (!n) break; @@ -199,6 +200,10 @@ void fixup_pack_header_footer(int pack_fd, die("Failed to checksum %s: %s", pack_name, strerror(errno)); SHA1_Update(&new_sha1_ctx, buf, n); + aligned_sz -= n; + if (!aligned_sz) + aligned_sz = buf_sz; + if (!partial_pack_sha1) continue; -- 1.6.0.1.212.g35f9f ``` ## Nicolas Pitre, 2008-08-29 20:14 Subject: Re: [PATCH 1/3] improve reliability of fixup_pack_header_footer() Message-ID: URL: https://gitlist.dev/e/alpine.LFD.1.10.0808291609030.1624%40xanadu.home In-Reply-To: <20080829143023.GA7403@spearce.org> ``` On Fri, 29 Aug 2008, Shawn O. Pearce wrote: > Oh, yea, that makes sense. It still seems like playing with fire. > > I'd rather the caller pass in the proper offset than rely on it > being the current position of the fd. Especially if the caller > does actually have it available. > > If you change anything, I'd like to see this lseek(SEEK_CUR) go away. Done. I struggled a bit since it simply didn't work initially -- see the first patch in the updated series. > > And another thing I had in store (but for which you _again_ beat me to :-) ) > > is to realign data reads onto filesystem blocks. > > That _really_ made the JGit code ugly. But I think its worth it. See my version. I think it is reasonably clear. > I also want to try and buffer the whole object appending we do > during fixThinPack(), as right now we write the object header in > one write and then compressed data bursts in the others. Moving it > to at least write a full 4k at a time should remove about 2 write > calls per object. Yep. In the C git case, moving to sha1write() added that buffering for free. Nicolas ``` ## Junio C Hamano, 2008-08-31 07:10 Subject: Re: [PATCH 5/5] fixup_pack_header_footer(): use nicely aligned buffer sizes Message-ID: <7v7i9xd5hu.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v7i9xd5hu.fsf%40gitster.siamese.dyndns.org In-Reply-To: <1220040482-10108-6-git-send-email-nico@cam.org> ``` Thanks -- it was a very good read. I wish every series I receive is clearly done like this one. Queued for 'next'. ```