{"thread":{"id":"8150","subject":"Re: Rebase max-pack-size?","startedAt":"2007-05-14T15:47:23Z","lastAt":"2007-05-23T07:23:10Z","messageCount":4,"participants":["Dana How","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"42135","messageId":"4648848B.1030304@gmail.com","threadId":"8150","inReplyTo":null,"subject":"Re: Rebase max-pack-size?","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-05-14T15:47:23Z","receivedAt":"2007-05-14T15:47:23Z","isPatch":false,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"\"Junio C Hamano\" <junkio@cox.net> writes:\n> \"Dana How\" <danahow@gmail.com> writes:\n> > Since max-pack-size has appeared, and disappeard, in \"pu\" twice\n> > and never made it into \"next\",  I'm guessing I should rebase/merge it on\n> > top of the other recent pack-objects work.  I may get to this tonight;\n> > perhaps I'll send the patchset in one email with multiple attachments?\n> I am still holding onto the series on its own topic branch, but\n> merging it to 'pu' and resolving conflicts seem pointless while\n> I am (and hopefully the rest of the list ought to be ;-)\n> primarily concentrating on last-minute bugfixes in preparation\n> for v1.5.2.\n> \n> A rebase to be queued in 'next' would be good, though.\n\nYes,  the conflicts were extensive.\nThe rebased patches are attached.\nLet me know if you want them re-sent inline and/or separately.\n\nThanks\n\nDana\n\n\n>From 781820f147bcb12dae576734585b27f42faca3ea Mon Sep 17 00:00:00 2001\nFrom: Dana L. How <danahow@gmail.com>\nDate: Sun, 13 May 2007 11:28:19 -0700\nSubject: [PATCH] Alter sha1close() 3rd argument to request flush only\n\nupdate=0 suppressed writing the final SHA-1 but was not used.\nNow final=0 suppresses SHA-1 finalization, SHA-1 writing,\nand closing -- in other words,  only flush the buffer.\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n csum-file.c |    8 +++++---\n 1 files changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/csum-file.c b/csum-file.c\nindex 7088f6e..5109342 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -29,18 +29,20 @@ static void sha1flush(struct sha1file *f, unsigned int count)\n \t}\n }\n \n-int sha1close(struct sha1file *f, unsigned char *result, int update)\n+int sha1close(struct sha1file *f, unsigned char *result, int final)\n {\n \tunsigned offset = f->offset;\n \tif (offset) {\n \t\tSHA1_Update(&f->ctx, f->buffer, offset);\n \t\tsha1flush(f, offset);\n+\t\tf->offset = 0;\n \t}\n+\tif (!final)\n+\t\treturn 0;\t/* only want to flush (no checksum write, no close) */\n \tSHA1_Final(f->buffer, &f->ctx);\n \tif (result)\n \t\thashcpy(result, f->buffer);\n-\tif (update)\n-\t\tsha1flush(f, 20);\n+\tsha1flush(f, 20);\n \tif (close(f->fd))\n \t\tdie(\"%s: sha1 file error on close (%s)\", f->name, strerror(errno));\n \tfree(f);\n-- \n1.5.2.rc3.726.g279d\n\n\n\n>From 0c75fd66aa6a758e245d85b9304e85a38e977f94 Mon Sep 17 00:00:00 2001\nFrom: Dana L. How <danahow@gmail.com>\nDate: Sun, 13 May 2007 11:34:56 -0700\nSubject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring\n\nAdd \"pack_size_limit\", the limit specified by --max-pack-size,\n\"written_list\", the list of objects written to the current pack,\nand \"nr_written\", the number of objects in written_list.\nPut \"base_name\" at file scope again and add forward declarations.\nMove write_index_file() call from cnd_pack_objects() to\nwrite_pack_file() since only the latter will know how\nmany times to call write_index_file().\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n builtin-pack-objects.c |   68 +++++++++++++++++++++++++----------------------\n 1 files changed, 36 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex d165f10..7c17ea0 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -52,7 +52,8 @@ struct object_entry {\n  * nice \"minimum seek\" order.\n  */\n static struct object_entry *objects;\n-static uint32_t nr_objects, nr_alloc, nr_result;\n+static struct object_entry **written_list;\n+static uint32_t nr_objects, nr_alloc, nr_result, nr_written;\n \n static int non_empty;\n static int no_reuse_delta, no_reuse_object;\n@@ -61,9 +62,11 @@ static int incremental;\n static int allow_ofs_delta;\n static const char *pack_tmp_name, *idx_tmp_name;\n static char tmpname[PATH_MAX];\n+static const char *base_name;\n static unsigned char pack_file_sha1[20];\n static int progress = 1;\n static int window = 10;\n+static uint32_t pack_size_limit;\n static int depth = 50;\n static int pack_to_stdout;\n static int num_preferred_base;\n@@ -501,7 +504,11 @@ static int open_object_dir_tmp(const char *path)\n     return mkstemp(tmpname);\n }\n \n-static off_t write_pack_file(void)\n+/* forward declarations for write_pack_file */\n+static void write_index_file(off_t last_obj_offset, unsigned char *sha1);\n+static int adjust_perm(const char *path, mode_t mode);\n+\n+static void write_pack_file(void)\n {\n \tuint32_t i;\n \tstruct sha1file *f;\n@@ -543,7 +550,32 @@ static off_t write_pack_file(void)\n \t\tdie(\"wrote %u objects while expecting %u\", written, nr_result);\n \tsha1close(f, pack_file_sha1, 1);\n \n-\treturn last_obj_offset;\n+\tif (!pack_to_stdout) {\n+\t\t\tunsigned char object_list_sha1[20];\n+\t\t\tmode_t mode = umask(0);\n+\n+\t\t\tumask(mode);\n+\t\t\tmode = 0444 & ~mode;\n+\n+\t\t\twrite_index_file(last_obj_offset, object_list_sha1);\n+\t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.pack\",\n+\t\t\t\t base_name, sha1_to_hex(object_list_sha1));\n+\t\t\tif (adjust_perm(pack_tmp_name, mode))\n+\t\t\t\tdie(\"unable to make temporary pack file readable: %s\",\n+\t\t\t\t    strerror(errno));\n+\t\t\tif (rename(pack_tmp_name, tmpname))\n+\t\t\t\tdie(\"unable to rename temporary pack file: %s\",\n+\t\t\t\t    strerror(errno));\n+\t\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.idx\",\n+\t\t\t\t base_name, sha1_to_hex(object_list_sha1));\n+\t\t\tif (adjust_perm(idx_tmp_name, mode))\n+\t\t\t\tdie(\"unable to make temporary index file readable: %s\",\n+\t\t\t\t    strerror(errno));\n+\t\t\tif (rename(idx_tmp_name, tmpname))\n+\t\t\t\tdie(\"unable to rename temporary index file: %s\",\n+\t\t\t\t    strerror(errno));\n+\t\t\tputs(sha1_to_hex(object_list_sha1));\n+\t}\n }\n \n static int sha1_sort(const void *_a, const void *_b)\n@@ -1537,8 +1569,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tint use_internal_rev_list = 0;\n \tint thin = 0;\n \tuint32_t i;\n-\toff_t last_obj_offset;\n-\tconst char *base_name = NULL;\n \tconst char **rp_av;\n \tint rp_ac_alloc = 64;\n \tint rp_ac;\n@@ -1707,33 +1737,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\tfprintf(stderr, \"Result has %u objects.\\n\", nr_result);\n \tif (nr_result)\n \t\tprepare_pack(window, depth);\n-\tlast_obj_offset = write_pack_file();\n-\tif (!pack_to_stdout) {\n-\t\tunsigned char object_list_sha1[20];\n-\t\tmode_t mode = umask(0);\n-\n-\t\tumask(mode);\n-\t\tmode = 0444 & ~mode;\n-\n-\t\twrite_index_file(last_obj_offset, object_list_sha1);\n-\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.pack\",\n-\t\t\t base_name, sha1_to_hex(object_list_sha1));\n-\t\tif (adjust_perm(pack_tmp_name, mode))\n-\t\t\tdie(\"unable to make temporary pack file readable: %s\",\n-\t\t\t    strerror(errno));\n-\t\tif (rename(pack_tmp_name, tmpname))\n-\t\t\tdie(\"unable to rename temporary pack file: %s\",\n-\t\t\t    strerror(errno));\n-\t\tsnprintf(tmpname, sizeof(tmpname), \"%s-%s.idx\",\n-\t\t\t base_name, sha1_to_hex(object_list_sha1));\n-\t\tif (adjust_perm(idx_tmp_name, mode))\n-\t\t\tdie(\"unable to make temporary index file readable: %s\",\n-\t\t\t    strerror(errno));\n-\t\tif (rename(idx_tmp_name, tmpname))\n-\t\t\tdie(\"unable to rename temporary index file: %s\",\n-\t\t\t    strerror(errno));\n-\t\tputs(sha1_to_hex(object_list_sha1));\n-\t}\n+\twrite_pack_file();\n \tif (progress)\n \t\tfprintf(stderr, \"Total %u (delta %u), reused %u (delta %u)\\n\",\n \t\t\twritten, written_delta, reused, reused_delta);\n-- \n1.5.2.rc3.726.g279d\n\n\n\n>From 40ac2f294ec68cae27f8a9262db4cab84f1d2257 Mon Sep 17 00:00:00 2001\nFrom: Dana L. How <danahow@gmail.com>\nDate: Sun, 13 May 2007 12:06:18 -0700\nSubject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit\n\nWith --max-pack-size,  generate the appropriate write limit\nfor each object and check against it before each group of writes.\nUpdate delta usability rules to handle base being in a previously-\nwritten pack.  Inline sha1write_compress() so we know the\nexact size of the written data when it needs to be compressed.\nDetect and return write \"failure\".\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n builtin-pack-objects.c |  131 ++++++++++++++++++++++++++++++++++++++----------\n 1 files changed, 104 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 7c17ea0..3023aac 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -352,16 +352,31 @@ static void copy_pack_data(struct sha1file *f,\n }\n \n static unsigned long write_object(struct sha1file *f,\n-\t\t\t\t  struct object_entry *entry)\n+\t\t\t\t  struct object_entry *entry,\n+\t\t\t\t  off_t write_offset)\n {\n \tunsigned long size;\n \tenum object_type type;\n \tvoid *buf;\n \tunsigned char header[10];\n+\tunsigned char dheader[10];\n \tunsigned hdrlen;\n \toff_t datalen;\n \tenum object_type obj_type;\n \tint to_reuse = 0;\n+\t/* write limit if limited packsize and not first object */\n+\tunsigned long limit = pack_size_limit && nr_written ?\n+\t\t\t\tpack_size_limit - write_offset : 0;\n+\t\t\t\t/* no if no delta */\n+\tint usable_delta =\t!entry->delta ? 0 :\n+\t\t\t\t/* yes if unlimited packfile */\n+\t\t\t\t!pack_size_limit ? 1 :\n+\t\t\t\t/* no if base written to previous pack */\n+\t\t\t\tentry->delta->offset == (off_t)-1 ? 0 :\n+\t\t\t\t/* otherwise double-check written to this\n+\t\t\t\t * pack,  like we do below\n+\t\t\t\t */\n+\t\t\t\tentry->delta->offset ? 1 : 0;\n \n \tif (!pack_to_stdout)\n \t\tcrc32_begin(f);\n@@ -372,7 +387,9 @@ static unsigned long write_object(struct sha1file *f,\n \telse if (!entry->in_pack)\n \t\tto_reuse = 0;\t/* can't reuse what we don't have */\n \telse if (obj_type == OBJ_REF_DELTA || obj_type == OBJ_OFS_DELTA)\n-\t\tto_reuse = 1;\t/* check_object() decided it for us */\n+\t\t\t\t/* check_object() decided it for us ... */\n+\t\tto_reuse = usable_delta;\n+\t\t\t\t/* ... but pack split may override that */\n \telse if (obj_type != entry->in_pack_type)\n \t\tto_reuse = 0;\t/* pack has delta which is unusable */\n \telse if (entry->delta)\n@@ -383,24 +400,48 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\t\t */\n \n \tif (!to_reuse) {\n+\t\tz_stream stream;\n+\t\tunsigned long maxsize;\n+\t\tvoid *out;\n \t\tbuf = read_sha1_file(entry->sha1, &type, &size);\n \t\tif (!buf)\n \t\t\tdie(\"unable to read %s\", sha1_to_hex(entry->sha1));\n \t\tif (size != entry->size)\n \t\t\tdie(\"object %s size inconsistency (%lu vs %lu)\",\n \t\t\t    sha1_to_hex(entry->sha1), size, entry->size);\n-\t\tif (entry->delta) {\n+\t\tif (usable_delta) {\n \t\t\tbuf = delta_against(buf, size, entry);\n \t\t\tsize = entry->delta_size;\n \t\t\tobj_type = (allow_ofs_delta && entry->delta->offset) ?\n \t\t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n+\t\t} else {\n+\t\t\t/*\n+\t\t\t * recover real object type in case\n+\t\t\t * check_object() wanted to re-use a delta,\n+\t\t\t * but we couldn't since base was in previous split pack\n+\t\t\t */\n+\t\t\tobj_type = type;\n \t\t}\n+\t\t/* compress the data to store and put compressed length in datalen */\n+\t\tmemset(&stream, 0, sizeof(stream));\n+\t\tdeflateInit(&stream, pack_compression_level);\n+\t\tmaxsize = deflateBound(&stream, size);\n+\t\tout = xmalloc(maxsize);\n+\t\t/* Compress it */\n+\t\tstream.next_in = buf;\n+\t\tstream.avail_in = size;\n+\t\tstream.next_out = out;\n+\t\tstream.avail_out = maxsize;\n+\t\twhile (deflate(&stream, Z_FINISH) == Z_OK)\n+\t\t\t/* nothing */;\n+\t\tdeflateEnd(&stream);\n+\t\tdatalen = stream.total_out;\n+\t\tdeflateEnd(&stream);\n \t\t/*\n \t\t * The object header is a byte of 'type' followed by zero or\n \t\t * more bytes of length.\n \t\t */\n \t\thdrlen = encode_header(obj_type, size, header);\n-\t\tsha1write(f, header, hdrlen);\n \n \t\tif (obj_type == OBJ_OFS_DELTA) {\n \t\t\t/*\n@@ -409,21 +450,41 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\t * base from this object's position in the pack.\n \t\t\t */\n \t\t\toff_t ofs = entry->offset - entry->delta->offset;\n-\t\t\tunsigned pos = sizeof(header) - 1;\n-\t\t\theader[pos] = ofs & 127;\n+\t\t\tunsigned pos = sizeof(dheader) - 1;\n+\t\t\tdheader[pos] = ofs & 127;\n \t\t\twhile (ofs >>= 7)\n-\t\t\t\theader[--pos] = 128 | (--ofs & 127);\n-\t\t\tsha1write(f, header + pos, sizeof(header) - pos);\n-\t\t\thdrlen += sizeof(header) - pos;\n+\t\t\t\tdheader[--pos] = 128 | (--ofs & 127);\n+\t\t\tif (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit) {\n+\t\t\t\tfree(out);\n+\t\t\t\tfree(buf);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\tsha1write(f, header, hdrlen);\n+\t\t\tsha1write(f, dheader + pos, sizeof(dheader) - pos);\n+\t\t\thdrlen += sizeof(dheader) - pos;\n \t\t} else if (obj_type == OBJ_REF_DELTA) {\n \t\t\t/*\n \t\t\t * Deltas with a base reference contain\n \t\t\t * an additional 20 bytes for the base sha1.\n \t\t\t */\n+\t\t\tif (limit && hdrlen + 20 + datalen + 20 >= limit) {\n+\t\t\t\tfree(out);\n+\t\t\t\tfree(buf);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\tsha1write(f, header, hdrlen);\n \t\t\tsha1write(f, entry->delta->sha1, 20);\n \t\t\thdrlen += 20;\n+\t\t} else {\n+\t\t\tif (limit && hdrlen + datalen + 20 >= limit) {\n+\t\t\t\tfree(out);\n+\t\t\t\tfree(buf);\n+\t\t\t\treturn 0;\n+\t\t\t}\n+\t\t\tsha1write(f, header, hdrlen);\n \t\t}\n-\t\tdatalen = sha1write_compressed(f, buf, size, pack_compression_level);\n+\t\tsha1write(f, out, datalen);\n+\t\tfree(out);\n \t\tfree(buf);\n \t}\n \telse {\n@@ -438,20 +499,6 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\treused_delta++;\n \t\t}\n \t\thdrlen = encode_header(obj_type, entry->size, header);\n-\t\tsha1write(f, header, hdrlen);\n-\t\tif (obj_type == OBJ_OFS_DELTA) {\n-\t\t\toff_t ofs = entry->offset - entry->delta->offset;\n-\t\t\tunsigned pos = sizeof(header) - 1;\n-\t\t\theader[pos] = ofs & 127;\n-\t\t\twhile (ofs >>= 7)\n-\t\t\t\theader[--pos] = 128 | (--ofs & 127);\n-\t\t\tsha1write(f, header + pos, sizeof(header) - pos);\n-\t\t\thdrlen += sizeof(header) - pos;\n-\t\t} else if (obj_type == OBJ_REF_DELTA) {\n-\t\t\tsha1write(f, entry->delta->sha1, 20);\n-\t\t\thdrlen += 20;\n-\t\t}\n-\n \t\toffset = entry->in_pack_offset;\n \t\trevidx = find_packed_object(p, offset);\n \t\tdatalen = revidx[1].offset - offset;\n@@ -460,6 +507,29 @@ static unsigned long write_object(struct sha1file *f,\n \t\t\tdie(\"bad packed object CRC for %s\", sha1_to_hex(entry->sha1));\n \t\toffset += entry->in_pack_header_size;\n \t\tdatalen -= entry->in_pack_header_size;\n+\t\tif (obj_type == OBJ_OFS_DELTA) {\n+\t\t\toff_t ofs = entry->offset - entry->delta->offset;\n+\t\t\tunsigned pos = sizeof(dheader) - 1;\n+\t\t\tdheader[pos] = ofs & 127;\n+\t\t\twhile (ofs >>= 7)\n+\t\t\t\tdheader[--pos] = 128 | (--ofs & 127);\n+\t\t\tif (limit && hdrlen + sizeof(dheader) - pos + datalen + 20 >= limit)\n+\t\t\t\treturn 0;\n+\t\t\tsha1write(f, header, hdrlen);\n+\t\t\tsha1write(f, dheader + pos, sizeof(dheader) - pos);\n+\t\t\thdrlen += sizeof(dheader) - pos;\n+\t\t} else if (obj_type == OBJ_REF_DELTA) {\n+\t\t\tif (limit && hdrlen + 20 + datalen + 20 >= limit)\n+\t\t\t\treturn 0;\n+\t\t\tsha1write(f, header, hdrlen);\n+\t\t\tsha1write(f, entry->delta->sha1, 20);\n+\t\t\thdrlen += 20;\n+\t\t} else {\n+\t\t\tif (limit && hdrlen + datalen + 20 >= limit)\n+\t\t\t\treturn 0;\n+\t\t\tsha1write(f, header, hdrlen);\n+\t\t}\n+\n \t\tif (!pack_to_stdout && p->index_version == 1 &&\n \t\t    check_pack_inflate(p, &w_curs, offset, datalen, entry->size))\n \t\t\tdie(\"corrupt packed object for %s\", sha1_to_hex(entry->sha1));\n@@ -467,7 +537,7 @@ static unsigned long write_object(struct sha1file *f,\n \t\tunuse_pack(&w_curs);\n \t\treused++;\n \t}\n-\tif (entry->delta)\n+\tif (usable_delta)\n \t\twritten_delta++;\n \twritten++;\n \tif (!pack_to_stdout)\n@@ -486,11 +556,18 @@ static off_t write_one(struct sha1file *f,\n \t\treturn offset;\n \n \t/* if we are deltified, write out base object first. */\n-\tif (e->delta)\n+\tif (e->delta) {\n \t\toffset = write_one(f, e->delta, offset);\n+\t\tif (!offset)\n+\t\t\treturn 0;\n+\t}\n \n \te->offset = offset;\n-\tsize = write_object(f, e);\n+\tsize = write_object(f, e, offset);\n+\tif (!size) {\n+\t\te->offset = 0;\n+\t\treturn 0;\n+\t}\n \n \t/* make sure off_t is sufficiently large not to wrap */\n \tif (offset > offset + size)\n-- \n1.5.2.rc3.726.g279d\n\n\n\n>From 9ec3af1f52f41d0737b6271fded5e94527eb0466 Mon Sep 17 00:00:00 2001\nFrom: Dana L. How <danahow@gmail.com>\nDate: Sun, 13 May 2007 12:09:16 -0700\nSubject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()\n\nRewrite write_pack_file() to break to a new packfile\nwhenever write_object/write_one request it,  and\ncorrect the header's object count in the previous packfile.\nChange write_index_file() to write an index\nfor just the objects in the most recent packfile.\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n builtin-pack-objects.c |  120 ++++++++++++++++++++++++++++-------------------\n 1 files changed, 71 insertions(+), 49 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 3023aac..ce9eb2d 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -568,6 +568,7 @@ static off_t write_one(struct sha1file *f,\n \t\te->offset = 0;\n \t\treturn 0;\n \t}\n+\twritten_list[nr_written++] = e;\n \n \t/* make sure off_t is sufficiently large not to wrap */\n \tif (offset > offset + size)\n@@ -587,47 +588,57 @@ static int adjust_perm(const char *path, mode_t mode);\n \n static void write_pack_file(void)\n {\n-\tuint32_t i;\n+\tuint32_t i = 0, j;\n \tstruct sha1file *f;\n-\toff_t offset, last_obj_offset = 0;\n+\toff_t offset, offset_one, last_obj_offset = 0;\n \tstruct pack_header hdr;\n-\tint do_progress = progress;\n-\n-\tif (pack_to_stdout) {\n-\t\tf = sha1fd(1, \"<stdout>\");\n-\t\tdo_progress >>= 1;\n-\t} else {\n-\t\tint fd = open_object_dir_tmp(\"tmp_pack_XXXXXX\");\n-\t\tif (fd < 0)\n-\t\t\tdie(\"unable to create %s: %s\\n\", tmpname, strerror(errno));\n-\t\tpack_tmp_name = xstrdup(tmpname);\n-\t\tf = sha1fd(fd, pack_tmp_name);\n-\t}\n+\tint do_progress = progress >> pack_to_stdout;\n+\tuint32_t nr_remaining = nr_result;\n \n \tif (do_progress)\n \t\tstart_progress(&progress_state, \"Writing %u objects...\", \"\", nr_result);\n+\twritten_list = xmalloc(nr_objects * sizeof(struct object_entry *));\n \n-\thdr.hdr_signature = htonl(PACK_SIGNATURE);\n-\thdr.hdr_version = htonl(PACK_VERSION);\n-\thdr.hdr_entries = htonl(nr_result);\n-\tsha1write(f, &hdr, sizeof(hdr));\n-\toffset = sizeof(hdr);\n-\tif (!nr_result)\n-\t\tgoto done;\n-\tfor (i = 0; i < nr_objects; i++) {\n-\t\tlast_obj_offset = offset;\n-\t\toffset = write_one(f, objects + i, offset);\n-\t\tif (do_progress)\n-\t\t\tdisplay_progress(&progress_state, written);\n-\t}\n-\tif (do_progress)\n-\t\tstop_progress(&progress_state);\n- done:\n-\tif (written != nr_result)\n-\t\tdie(\"wrote %u objects while expecting %u\", written, nr_result);\n-\tsha1close(f, pack_file_sha1, 1);\n+\tdo {\n+\t\tif (pack_to_stdout) {\n+\t\t\tf = sha1fd(1, \"<stdout>\");\n+\t\t} else {\n+\t\t\tint fd = open_object_dir_tmp(\"tmp_pack_XXXXXX\");\n+\t\t\tif (fd < 0)\n+\t\t\t\tdie(\"unable to create %s: %s\\n\", tmpname, strerror(errno));\n+\t\t\tpack_tmp_name = xstrdup(tmpname);\n+\t\t\tf = sha1fd(fd, pack_tmp_name);\n+\t\t}\n \n-\tif (!pack_to_stdout) {\n+\t\thdr.hdr_signature = htonl(PACK_SIGNATURE);\n+\t\thdr.hdr_version = htonl(PACK_VERSION);\n+\t\thdr.hdr_entries = htonl(nr_remaining);\n+\t\tsha1write(f, &hdr, sizeof(hdr));\n+\t\toffset = sizeof(hdr);\n+\t\tnr_written = 0;\n+\t\tfor (; i < nr_objects; i++) {\n+\t\t\tlast_obj_offset = offset;\n+\t\t\toffset_one = write_one(f, objects + i, offset);\n+\t\t\tif (!offset_one)\n+\t\t\t\tbreak;\n+\t\t\toffset = offset_one;\n+\t\t\tif (do_progress)\n+\t\t\t\tdisplay_progress(&progress_state, written);\n+\t\t}\n+\n+\t\t/*\n+\t\t * Did we write the wrong # entries in the header?\n+\t\t * If so, rewrite it like in fast-import\n+\t\t */\n+\t\tif (pack_to_stdout || nr_written == nr_remaining) {\n+\t\t\tsha1close(f, pack_file_sha1, 1);\n+\t\t} else {\n+\t\t\tsha1close(f, pack_file_sha1, 0);\n+\t\t\tfixup_pack_header_footer(f->fd, pack_file_sha1, pack_tmp_name, nr_written);\n+\t\t\tclose(f->fd);\n+\t\t}\n+\n+\t\tif (!pack_to_stdout) {\n \t\t\tunsigned char object_list_sha1[20];\n \t\t\tmode_t mode = umask(0);\n \n@@ -652,7 +663,26 @@ static void write_pack_file(void)\n \t\t\t\tdie(\"unable to rename temporary index file: %s\",\n \t\t\t\t    strerror(errno));\n \t\t\tputs(sha1_to_hex(object_list_sha1));\n+\t\t}\n+\n+\t\t/* mark written objects as written to previous pack */\n+\t\tfor (j = 0; j < nr_written; j++) {\n+\t\t\twritten_list[j]->offset = (off_t)-1;\n+\t\t}\n+\t\tnr_remaining -= nr_written;\n+\t} while (nr_remaining && i < nr_objects);\n+\n+\tfree(written_list);\n+\tif (do_progress)\n+\t\tstop_progress(&progress_state);\n+\tif (written != nr_result)\n+\t\tdie(\"wrote %u objects while expecting %u\", written, nr_result);\n+\tfor (j = 0; i < nr_objects; i++) {\n+\t\tstruct object_entry *e = objects + i;\n+\t\tj += !e->offset && !e->preferred_base;\n \t}\n+\tif (j)\n+\t\tdie(\"wrote %u objects as expected but %u unwritten\", written, j);\n }\n \n static int sha1_sort(const void *_a, const void *_b)\n@@ -679,18 +709,11 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)\n \tidx_tmp_name = xstrdup(tmpname);\n \tf = sha1fd(fd, idx_tmp_name);\n \n-\tif (nr_result) {\n-\t\tuint32_t j = 0;\n-\t\tsorted_by_sha =\n-\t\t\txcalloc(nr_result, sizeof(struct object_entry *));\n-\t\tfor (i = 0; i < nr_objects; i++)\n-\t\t\tif (!objects[i].preferred_base)\n-\t\t\t\tsorted_by_sha[j++] = objects + i;\n-\t\tif (j != nr_result)\n-\t\t\tdie(\"listed %u objects while expecting %u\", j, nr_result);\n-\t\tqsort(sorted_by_sha, nr_result, sizeof(*sorted_by_sha), sha1_sort);\n+\tif (nr_written) {\n+\t\tsorted_by_sha = written_list;\n+\t\tqsort(sorted_by_sha, nr_written, sizeof(*sorted_by_sha), sha1_sort);\n \t\tlist = sorted_by_sha;\n-\t\tlast = sorted_by_sha + nr_result;\n+\t\tlast = sorted_by_sha + nr_written;\n \t} else\n \t\tsorted_by_sha = list = last = NULL;\n \n@@ -728,7 +751,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)\n \n \t/* Write the actual SHA1 entries. */\n \tlist = sorted_by_sha;\n-\tfor (i = 0; i < nr_result; i++) {\n+\tfor (i = 0; i < nr_written; i++) {\n \t\tstruct object_entry *entry = *list++;\n \t\tif (index_version < 2) {\n \t\t\tuint32_t offset = htonl(entry->offset);\n@@ -743,7 +766,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)\n \n \t\t/* write the crc32 table */\n \t\tlist = sorted_by_sha;\n-\t\tfor (i = 0; i < nr_objects; i++) {\n+\t\tfor (i = 0; i < nr_written; i++) {\n \t\t\tstruct object_entry *entry = *list++;\n \t\t\tuint32_t crc32_val = htonl(entry->crc32);\n \t\t\tsha1write(f, &crc32_val, 4);\n@@ -751,7 +774,7 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)\n \n \t\t/* write the 32-bit offset table */\n \t\tlist = sorted_by_sha;\n-\t\tfor (i = 0; i < nr_objects; i++) {\n+\t\tfor (i = 0; i < nr_written; i++) {\n \t\t\tstruct object_entry *entry = *list++;\n \t\t\tuint32_t offset = (entry->offset <= index_off32_limit) ?\n \t\t\t\tentry->offset : (0x80000000 | nr_large_offset++);\n@@ -776,7 +799,6 @@ static void write_index_file(off_t last_obj_offset, unsigned char *sha1)\n \n \tsha1write(f, pack_file_sha1, 20);\n \tsha1close(f, NULL, 1);\n-\tfree(sorted_by_sha);\n \tSHA1_Final(sha1, &ctx);\n }\n \n-- \n1.5.2.rc3.726.g279d\n\n\n\n>From 279d1f9e0e7265c6ccf8759bbe8c5539bf3089fa Mon Sep 17 00:00:00 2001\nFrom: Dana L. How <danahow@gmail.com>\nDate: Sun, 13 May 2007 12:47:09 -0700\nSubject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature\n\nAdd --max-pack-size parsing and usage messages.\nUpgrade git-repack.sh to handle multiple packfile names,\nand build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.\nUpdate documentation.\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n Documentation/git-pack-objects.txt |    5 +++++\n Documentation/git-repack.txt       |    5 +++++\n builtin-pack-objects.c             |    9 ++++++++-\n git-repack.sh                      |   14 ++++++++------\n 4 files changed, 26 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 2531238..cfe127a 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -85,6 +85,11 @@ base-name::\n \ttimes to get to the necessary object.\n \tThe default value for --window is 10 and --depth is 50.\n \n+--max-pack-size=<n>::\n+\tMaximum size of each output packfile, expressed in MiB.\n+\tIf specified,  multiple packfiles may be created.\n+\tThe default is unlimited.\n+\n --incremental::\n \tThis flag causes an object already in a pack ignored\n \teven if it appears in the standard input.\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex cc3b0b2..2847c9b 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -65,6 +65,11 @@ OPTIONS\n \tto be applied that many times to get to the necessary object.\n \tThe default value for --window is 10 and --depth is 50.\n \n+--max-pack-size=<n>::\n+\tMaximum size of each output packfile, expressed in MiB.\n+\tIf specified,  multiple packfiles may be created.\n+\tThe default is unlimited.\n+\n \n Configuration\n -------------\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex ce9eb2d..930b57a 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -15,7 +15,7 @@\n #include \"progress.h\"\n \n static const char pack_usage[] = \"\\\n-git-pack-objects [{ -q | --progress | --all-progress }] \\n\\\n+git-pack-objects [{ -q | --progress | --all-progress }] [--max-pack-size=N] \\n\\\n \t[--local] [--incremental] [--window=N] [--depth=N] \\n\\\n \t[--no-reuse-delta] [--no-reuse-object] [--delta-base-offset] \\n\\\n \t[--non-empty] [--revs [--unpacked | --all]*] [--reflog] \\n\\\n@@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tpack_compression_level = level;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!prefixcmp(arg, \"--max-pack-size=\")) {\n+\t\t\tchar *end;\n+\t\t\tpack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;\n+\t\t\tif (!arg[16] || *end)\n+\t\t\t\tusage(pack_usage);\n+\t\t\tcontinue;\n+\t\t}\n \t\tif (!prefixcmp(arg, \"--window=\")) {\n \t\t\tchar *end;\n \t\t\twindow = strtoul(arg+9, &end, 0);\ndiff --git a/git-repack.sh b/git-repack.sh\nindex 8bf66a4..4ea6e5b 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -3,7 +3,7 @@\n # Copyright (c) 2005 Linus Torvalds\n #\n \n-USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--window=N] [--depth=N]'\n+USAGE='[-a] [-d] [-f] [-l] [-n] [-q] [--max-pack-size=N] [--window=N] [--depth=N]'\n SUBDIRECTORY_OK='Yes'\n . git-sh-setup\n \n@@ -18,6 +18,7 @@ do\n \t-q)\tquiet=-q ;;\n \t-f)\tno_reuse=--no-reuse-object ;;\n \t-l)\tlocal=--local ;;\n+\t--max-pack-size=*) extra=\"$extra $1\" ;;\n \t--window=*) extra=\"$extra $1\" ;;\n \t--depth=*) extra=\"$extra $1\" ;;\n \t*)\tusage ;;\n@@ -35,7 +36,7 @@ true)\n esac\n \n PACKDIR=\"$GIT_OBJECT_DIRECTORY/pack\"\n-PACKTMP=\"$GIT_DIR/.tmp-$$-pack\"\n+PACKTMP=\"$GIT_OBJECT_DIRECTORY/.tmp-$$-pack\"\n rm -f \"$PACKTMP\"-*\n trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n \n@@ -62,11 +63,12 @@ case \",$all_into_one,\" in\n esac\n \n args=\"$args $local $quiet $no_reuse$extra\"\n-name=$(git-pack-objects --non-empty --all --reflog $args </dev/null \"$PACKTMP\") ||\n+names=$(git-pack-objects --non-empty --all --reflog $args </dev/null \"$PACKTMP\") ||\n \texit 1\n-if [ -z \"$name\" ]; then\n+if [ -z \"$names\" ]; then\n \techo Nothing new to pack.\n-else\n+fi\n+for name in $names ; do\n \tchmod a-w \"$PACKTMP-$name.pack\"\n \tchmod a-w \"$PACKTMP-$name.idx\"\n \tif test \"$quiet\" != '-q'; then\n@@ -92,7 +94,7 @@ else\n \t\texit 1\n \t}\n \trm -f \"$PACKDIR/old-pack-$name.pack\" \"$PACKDIR/old-pack-$name.idx\"\n-fi\n+done\n \n if test \"$remove_redundant\" = t\n then\n-- \n1.5.2.rc3.726.g279d\n\n"},{"id":"43016","messageId":"7virak5cr5.fsf@assigned-by-dhcp.cox.net","threadId":"8150","inReplyTo":"4648848B.1030304@gmail.com","subject":"Re: Rebase max-pack-size?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-23T05:29:18Z","receivedAt":"2007-05-23T05:29:18Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dana How <danahow@gmail.com> writes:\n\n> Let me know if you want them re-sent inline and/or separately.\n\nRecently Linus was very unhappy on the kernel list when somebody\nforwarded bunch of patches in a single message, saying we do\nreviews one patch per message (and we do not do attachments).\n\nI haven't been very strict about that kind of formality, as I\nhaven't felt the need too much so far.  Let's see how well we\ncan do this with this message, which has 5 patches.\n\n----------------------------------------------------------------\n>>From 781820f147bcb12dae576734585b27f42faca3ea Mon Sep 17 00:00:00 2001\n> From: Dana L. How <danahow@gmail.com>\n> Date: Sun, 13 May 2007 11:28:19 -0700\n> Subject: [PATCH] Alter sha1close() 3rd argument to request flush only\n>\n> update=0 suppressed writing the final SHA-1 but was not used.\n> Now final=0 suppresses SHA-1 finalization, SHA-1 writing,\n> and closing -- in other words,  only flush the buffer.\n>\n> Signed-off-by: Dana L. How <danahow@gmail.com>\n\nWhat it does is understandable but it somehow feels funny that\n\"sha1close(file, hashresult, 0)\" does _not_ close it (and does\nnot hash either for obvious reasons ;-).  I would say we should\nlet it pass this round, but might want to separate the first\npart out into a separate \"update hash and flush\" function if we\nget more callers.\n\n----------------------------------------------------------------\n>>From 0c75fd66aa6a758e245d85b9304e85a38e977f94 Mon Sep 17 00:00:00 2001\n> From: Dana L. How <danahow@gmail.com>\n> Date: Sun, 13 May 2007 11:34:56 -0700\n> Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring\n>\n> Add \"pack_size_limit\", the limit specified by --max-pack-size,\n> \"written_list\", the list of objects written to the current pack,\n> and \"nr_written\", the number of objects in written_list.\n> Put \"base_name\" at file scope again and add forward declarations.\n> Move write_index_file() call from cnd_pack_objects() to\n> write_pack_file() since only the latter will know how\n> many times to call write_index_file().\n>\n> Signed-off-by: Dana L. How <danahow@gmail.com>\n\nI would have split this part a bit differently.\n\nThis is mostly about restructuring the code so that\nwrite_index_file() is called from write_pack_file(), which by\nitself is a very good change (but then we might have been better\noff passing basename as a parameter).\n\nYou are not using written_list nor limit yet but are introducing\nthem in this step, which feels not quite right.  I usually compile stuff\nwith -Werror, and if I ever have to bisect the series, this would bomb out\nfor these unused variables.  Not nice.\n\n----------------------------------------------------------------\n>>From 40ac2f294ec68cae27f8a9262db4cab84f1d2257 Mon Sep 17 00:00:00 2001\n> From: Dana L. How <danahow@gmail.com>\n> Date: Sun, 13 May 2007 12:06:18 -0700\n> Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit\n>\n> With --max-pack-size,  generate the appropriate write limit\n> for each object and check against it before each group of writes.\n> Update delta usability rules to handle base being in a previously-\n> written pack.  Inline sha1write_compress() so we know the\n> exact size of the written data when it needs to be compressed.\n> Detect and return write \"failure\".\n>\n> Signed-off-by: Dana L. How <danahow@gmail.com>\n> ---\n>  builtin-pack-objects.c |  131 ++++++++++++++++++++++++++++++++++++++----------\n>  1 files changed, 104 insertions(+), 27 deletions(-)\n\nAgain, this is split somewhat wrongly, as you do not have a way\nto set max size from the caller, but more importantly, even\nthough write_one and write_object knows to obey the limit\n(perhaps somebody bisecting this series later may set the limit\nunder the debugger, to work around the lack of option parser),\nwrite_pack_file() does not notice zero return from write_one();\nI would have added a check there to do:\n\n\tdie(\"sorry, limit reached and we do not have code to split the pack yet.\")\n\nwhich obviously can be updated in the next patch.\n\n----------------------------------------------------------------\n>>From 9ec3af1f52f41d0737b6271fded5e94527eb0466 Mon Sep 17 00:00:00 2001\n> From: Dana L. How <danahow@gmail.com>\n> Date: Sun, 13 May 2007 12:09:16 -0700\n> Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()\n>\n> Rewrite write_pack_file() to break to a new packfile\n> whenever write_object/write_one request it,  and\n> correct the header's object count in the previous packfile.\n> Change write_index_file() to write an index\n> for just the objects in the most recent packfile.\n>\n> Signed-off-by: Dana L. How <danahow@gmail.com>\n> ---\n>  builtin-pack-objects.c |  120 ++++++++++++++++++++++++++++-------------------\n>  1 files changed, 71 insertions(+), 49 deletions(-)\n>\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index 3023aac..ce9eb2d 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> ...\n> @@ -652,7 +663,26 @@ static void write_pack_file(void)\n>  \t\t\t\tdie(\"unable to rename temporary index file: %s\",\n>  \t\t\t\t    strerror(errno));\n>  \t\t\tputs(sha1_to_hex(object_list_sha1));\n> +\t\t}\n> +\n> +\t\t/* mark written objects as written to previous pack */\n> +\t\tfor (j = 0; j < nr_written; j++) {\n> +\t\t\twritten_list[j]->offset = (off_t)-1;\n> +\t\t}\n> +\t\tnr_remaining -= nr_written;\n> +\t} while (nr_remaining && i < nr_objects);\n> +\n> +\tfree(written_list);\n> +\tif (do_progress)\n> +\t\tstop_progress(&progress_state);\n> +\tif (written != nr_result)\n> +\t\tdie(\"wrote %u objects while expecting %u\", written, nr_result);\n> +\tfor (j = 0; i < nr_objects; i++) {\n> +\t\tstruct object_entry *e = objects + i;\n> +\t\tj += !e->offset && !e->preferred_base;\n>  \t}\n\nI am a bit confused by this loop.  Don't you have to start with i=0\nfor this check to be meaningful?\n\n> +\tif (j)\n> +\t\tdie(\"wrote %u objects as expected but %u unwritten\", written, j);\n>  }\n\n\n----------------------------------------------------------------\n>>From 279d1f9e0e7265c6ccf8759bbe8c5539bf3089fa Mon Sep 17 00:00:00 2001\n> From: Dana L. How <danahow@gmail.com>\n> Date: Sun, 13 May 2007 12:47:09 -0700\n> Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature\n>\n> Add --max-pack-size parsing and usage messages.\n> Upgrade git-repack.sh to handle multiple packfile names,\n> and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.\n> Update documentation.\n>\n> Signed-off-by: Dana L. How <danahow@gmail.com>\n> ---\n>  Documentation/git-pack-objects.txt |    5 +++++\n>  Documentation/git-repack.txt       |    5 +++++\n>  builtin-pack-objects.c             |    9 ++++++++-\n>  git-repack.sh                      |   14 ++++++++------\n>  4 files changed, 26 insertions(+), 7 deletions(-)\n> diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> index ce9eb2d..930b57a 100644\n> --- a/builtin-pack-objects.c\n> +++ b/builtin-pack-objects.c\n> ...\n> @@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n>  \t\t\tpack_compression_level = level;\n>  \t\t\tcontinue;\n>  \t\t}\n> +\t\tif (!prefixcmp(arg, \"--max-pack-size=\")) {\n> +\t\t\tchar *end;\n> +\t\t\tpack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;\n> +\t\t\tif (!arg[16] || *end)\n> +\t\t\t\tusage(pack_usage);\n> +\t\t\tcontinue;\n> +\t\t}\n>  \t\tif (!prefixcmp(arg, \"--window=\")) {\n>  \t\t\tchar *end;\n>  \t\t\twindow = strtoul(arg+9, &end, 0);\n\nHmmm.  I was almost going to suggest to have this spelled in\nbytes, with suffixes like k/m/g.  However, because wanting to\nlimit a pack under 1.4MB does not make much sense these days,\nand because having to spell \"up to 2GB\" as 2047 is not too much\ntrouble, I think this is Ok.\n\nShouldn't we have a safety to error out when --stdout and\n--max-pack-size are both given?  Currently it silently ignores\nthe limit, doesn't it?\n\n> diff --git a/git-repack.sh b/git-repack.sh\n> index 8bf66a4..4ea6e5b 100755\n> --- a/git-repack.sh\n> +++ b/git-repack.sh\n> ...\n> @@ -35,7 +36,7 @@ true)\n>  esac\n>  \n>  PACKDIR=\"$GIT_OBJECT_DIRECTORY/pack\"\n> -PACKTMP=\"$GIT_DIR/.tmp-$$-pack\"\n> +PACKTMP=\"$GIT_OBJECT_DIRECTORY/.tmp-$$-pack\"\n>  rm -f \"$PACKTMP\"-*\n>  trap 'rm -f \"$PACKTMP\"-*' 0 1 2 3 15\n>  \n\nAlthough this is a good change, this hunk does not belong to\nthis.\n\n----------------------------------------------------------------\n\nOverall everything looks good, except some minor details noted\nabove.  Separation of the commits into logical steps does not\nneed to be fixed up (they are already in 'next'), but follow-up\npatches might be needed.\n\nAnd I have to agree with Linus; responding this way was more\ncumbersome than it should have been.\n"},{"id":"43019","messageId":"56b7f5510705222333l6e285e67l1dc327322a2ab250@mail.gmail.com","threadId":"8150","inReplyTo":"7virak5cr5.fsf@assigned-by-dhcp.cox.net","subject":"Re: Rebase max-pack-size?","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-05-23T06:33:27Z","receivedAt":"2007-05-23T06:33:27Z","isPatch":false,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 5/22/07, Junio C Hamano <junkio@cox.net> wrote:\n> Dana How <danahow@gmail.com> writes:\n> ----------------------------------------------------------------\n> > Subject: [PATCH] Alter sha1close() 3rd argument to request flush only\n> >\n> > update=0 suppressed writing the final SHA-1 but was not used.\n> > Now final=0 suppresses SHA-1 finalization, SHA-1 writing,\n> > and closing -- in other words,  only flush the buffer.\n>\n> What it does is understandable but it somehow feels funny that\n> \"sha1close(file, hashresult, 0)\" does _not_ close it (and does\n> not hash either for obvious reasons ;-).  I would say we should\n> let it pass this round, but might want to separate the first\n> part out into a separate \"update hash and flush\" function if we\n> get more callers.\nOK,  see the list at the end.\n\n> ----------------------------------------------------------------\n> > Subject: [PATCH 1/4] git-repack --max-pack-size: new file statics and code restructuring\n> >\n> > Add \"pack_size_limit\", the limit specified by --max-pack-size,\n> > \"written_list\", the list of objects written to the current pack,\n> > and \"nr_written\", the number of objects in written_list.\n> > Put \"base_name\" at file scope again and add forward declarations.\n> > Move write_index_file() call from cnd_pack_objects() to\n> > write_pack_file() since only the latter will know how\n> > many times to call write_index_file().\n>\n> I would have split this part a bit differently.\n>\n> This is mostly about restructuring the code so that\n> write_index_file() is called from write_pack_file(), which by\n> itself is a very good change (but then we might have been better\n> off passing basename as a parameter).\nThat would have worked too.  I made base_name file scope\nb/c that's how it was before NP's immediatley previous patches.\n\n> You are not using written_list nor limit yet but are introducing\n> them in this step, which feels not quite right.  I usually compile stuff\n> with -Werror, and if I ever have to bisect the series, this would bomb\n> out for these unused variables.  Not nice.\nVery good point -- I did not think of that.\nI will make sure in the future\neach patch in a patchset has no warnings.\n\n> ----------------------------------------------------------------\n> > Subject: [PATCH 2/4] git-repack --max-pack-size: write_{object,one}() respect pack limit\n> >\n> > With --max-pack-size,  generate the appropriate write limit\n> > for each object and check against it before each group of writes.\n> > Update delta usability rules to handle base being in a previously-\n> > written pack.  Inline sha1write_compress() so we know the\n> > exact size of the written data when it needs to be compressed.\n> > Detect and return write \"failure\".\n>\n> Again, this is split somewhat wrongly, as you do not have a way\n> to set max size from the caller, but more importantly, even\n> though write_one and write_object knows to obey the limit\n> (perhaps somebody bisecting this series later may set the limit\n> under the debugger, to work around the lack of option parser),\n> write_pack_file() does not notice zero return from write_one();\n> I would have added a check there to do:\n>\n>         die(\"sorry, limit reached and we do not have code to split the pack yet.\")\n>\n> which obviously can be updated in the next patch.\nThis makes sense to me.  In a previous version of the patchset,\nI had some temporary code like you say (just some extra\narguments, not any checking or die() calls) which was\nimmediately changed/replaced in the next patch.\nShawn or Nicolas didn't like that,\nso I migrated to the split I used here.\n\nI didn't really intend bisecting this patchset to do\nanything useful *with respect to max-pack-size functionality*,\nbut bisecting it would be useful to detect if these patches\nhad broken something else.  But I can rethink that in\nthe future.\n\n> ----------------------------------------------------------------\n> > Subject: [PATCH 3/4] git-repack --max-pack-size: split packs as asked by write_{object,one}()\n> >\n> > Rewrite write_pack_file() to break to a new packfile\n> > whenever write_object/write_one request it,  and\n> > correct the header's object count in the previous packfile.\n> > Change write_index_file() to write an index\n> > for just the objects in the most recent packfile.\n> >\n> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> > --- a/builtin-pack-objects.c\n> > +++ b/builtin-pack-objects.c\n> > @@ -652,7 +663,26 @@ static void write_pack_file(void)\n> > +     } while (nr_remaining && i < nr_objects);\n> > +\n> > +     for (j = 0; i < nr_objects; i++) {\n> > +             struct object_entry *e = objects + i;\n> > +             j += !e->offset && !e->preferred_base;\n> > +     }\n> > +     if (j)\n> > +             die(\"wrote %u objects as expected but %u unwritten\", written, j);\n> I am a bit confused by this loop.  Don't you have to start with i=0\n> for this check to be meaningful?\nThe previous do-while loop whose last line is shown\ncould end with i < nr_objects due to nr_remaining becoming\n0.  This means the objects [i ... nr_objects) have not been\ninspected.  They should all be either already written\nor non-writable,  which is what the two terms in the && expression\nare testing.  See list at end.\n\n> ----------------------------------------------------------------\n> > Subject: [PATCH 4/4] git-repack --max-pack-size: add option parsing to enable feature\n> >\n> > Add --max-pack-size parsing and usage messages.\n> > Upgrade git-repack.sh to handle multiple packfile names,\n> > and build packfiles in GIT_OBJECT_DIRECTORY not GIT_DIR.\n> > Update documentation.\n> >\n> > diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\n> > --- a/builtin-pack-objects.c\n> > +++ b/builtin-pack-objects.c\n> > @@ -1713,6 +1713,13 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n> > +             if (!prefixcmp(arg, \"--max-pack-size=\")) {\n> > +                     char *end;\n> > +                     pack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;\n> > +                     if (!arg[16] || *end)\n> > +                             usage(pack_usage);\n> > +                     continue;\n> > +             }\n>\n> Hmmm.  I was almost going to suggest to have this spelled in\n> bytes, with suffixes like k/m/g.  However, because wanting to\n> limit a pack under 1.4MB does not make much sense these days,\n> and because having to spell \"up to 2GB\" as 2047 is not too much\n> trouble, I think this is Ok.\nI thought about suffixes too but I chose this way\nb/c that's how git fast-import does it.\n\n> Shouldn't we have a safety to error out when --stdout and\n> --max-pack-size are both given?  Currently it silently ignores\n> the limit, doesn't it?\nYes & yes.  Previously there was disagreement on this point.\nOne preference was to disallow the combination,\nanother was that it was useful and should be supported.\nSo I left the code in a state where a small follow-on patch\ncould make the decision.  The current behavior is not\nto disallow the combination,  and it is well-defined\n(you get a sequence of packs on stdout,  concatenated,\nwhose headers indicate how many objects there are\nin the current pack and all following).\n\nI do not think --stdout && --max-pack-size is currently\nuseful,  and a follow-on patch should complain.\nSee list at end.\n\n> > diff --git a/git-repack.sh b/git-repack.sh\n> > --- a/git-repack.sh\n> > +++ b/git-repack.sh\n> > @@ -35,7 +36,7 @@ true)\n> > -PACKTMP=\"$GIT_DIR/.tmp-$$-pack\"\n> > +PACKTMP=\"$GIT_OBJECT_DIRECTORY/.tmp-$$-pack\"\n>\n> Although this is a good change, this hunk does not belong to\n> this.\nOK.\nIf you decide not to keep this change here for historical clarity,\nI can add it back later.\n\n> Overall everything looks good, except some minor details noted\n> above.  Separation of the commits into logical steps does not\n> need to be fixed up (they are already in 'next'), but follow-up\n> patches might be needed.\nOK,  I will not edit and re-submit these patches.\nBut I will submit two follow-on patches:\n\nPatch 1:\n* Pull \"sha1flush()\" or similar out of sha1close() inside csum-file.c.\n  This will require some edits to callers.\n\nPatch 2:\n* Add a comment to \"confusing loop\" to explain what it's checking.\n* Complain about --stdout && --max-pack-size combination.\n\nThis will have to happen tomorrow.\n\n> And I have to agree with Linus; responding this way was more\n> cumbersome than it should have been.\nUnderstood.\n\nThanks,\n-- \nDana L. How  danahow@gmail.com  +1 650 804 5991 cell\n"},{"id":"43022","messageId":"7vodkc3swx.fsf@assigned-by-dhcp.cox.net","threadId":"8150","inReplyTo":"56b7f5510705222333l6e285e67l1dc327322a2ab250@mail.gmail.com","subject":"Re: Rebase max-pack-size?","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-23T07:23:10Z","receivedAt":"2007-05-23T07:23:10Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dana How\" <danahow@gmail.com> writes:\n\n> Patch 1:\n> * Pull \"sha1flush()\" or similar out of sha1close() inside csum-file.c.\n>  This will require some edits to callers.\n>\n> Patch 2:\n> * Add a comment to \"confusing loop\" to explain what it's checking.\n> * Complain about --stdout && --max-pack-size combination.\n>\n> This will have to happen tomorrow.\n>\n>> And I have to agree with Linus; responding this way was more\n>> cumbersome than it should have been.\n> Understood.\n>\n> Thanks,\n\nAlthough I mentioned it, as we have only one caller that does\nnot want to close the file, we probably do not need the first\npatch right now.\n\nAlso thanks for the clarification on the (nr_result != nr_objects)\nstuff -- it should have been obvious, as I was the guilty party\nwho originally introduced that condition (i.e. \"thin\" packs).\n"}]}