{"thread":{"id":"7516","subject":"[PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","startedAt":"2007-04-04T20:40:42Z","lastAt":"2007-04-05T21:17:58Z","messageCount":12,"participants":["Dana How","Nicolas Pitre","Shawn O. Pearce","Linus Torvalds","Florian Weimer"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"38652","messageId":"56b7f5510704041340l5997b04dp853e8270a644ab28@mail.gmail.com","threadId":"7516","inReplyTo":null,"subject":"[PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-04T20:40:42Z","receivedAt":"2007-04-04T20:40:42Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"The motivations are to better support portable media,\nolder filesystems,  and larger repositories without\nawkward enormous packfiles.\n\nWhen --pack-limit[=N] is specified and --stdout is not,\nall bytes in the resulting packfile(s) appear at offsets\nless than N (which defaults to 1<<31).  The default\nguarantees mmap(2) on 32b systems never sees signed off_t's.\nThe object stream may be broken into multiple packfiles\nas a result,  each properly and conventionally built.\n\nWhen --stdout is also specified,  all objects in the\nresulting packfile(s) _start_ at offsets less than N.\nAll the packfiles appear concatenated on stdout,\nand each has its object count set to 0.  The behavior\nwithout --stdout cannot be duplicated here since\nlseek(2) is not generally possible on stdout.  The end\nof each pack in the concatenated whole can be detected by\nasking,  just before reading each object,  if the next\n20 bytes match the SHA-1 of what came before.\nA future change might insert a null-byte EOF marker\n(i.e. type=OBJ_NONE/length=0) before each pack's final SHA-1\nor before only the final pack's SHA-1.\n\nWhen --blob-limit=N is specified,  blobs whose uncompressed\nsize is greater than or equal to N are omitted from the pack(s).\nIf --pack-limit is specified, --blob-limit is not, and\n--stdout is not,  then --blob-limit defaults to 1/4\nof the --pack-limit.\n\nTo enable this,  csum-file.c now has the ability to rollback\na checksummed write (see sha1mark/sha1undo).\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n builtin-pack-objects.c   |  300 ++++++++++++++++++++++++++++++++++++++--------\n builtin-unpack-objects.c |    2 +-\n csum-file.c              |   41 +++++++\n csum-file.h              |    6 +\n git-repack.sh            |   12 ++-\n http-fetch.c             |    2 +-\n http-push.c              |    2 +-\n index-pack.c             |    2 +-\n sha1_file.c              |    2 +-\n 9 files changed, 307 insertions(+), 62 deletions(-)\n\n[Full patch attached since gmail insists on wordwrap and eating ws]\n\nThanks,\n-- \nDana L. How  danahow@gmail.com  +1 650 804 5991 cell\n\n\nFrom f9da90fb81a8ed3336829c824baf3fd5c08cf74b Mon Sep 17 00:00:00 2001\nFrom: Dana L. How <danahow@gmail.com>\nDate: Wed, 4 Apr 2007 13:15:45 -0700\nSubject: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size\n\nThe motivations are to better support portable media,\nolder filesystems,  and larger repositories without\nawkward enormous packfiles.\n\nWhen --pack-limit[=N] is specified and --stdout is not,\nall bytes in the resulting packfile(s) appear at offsets\nless than N (which defaults to 1<<31).  The default\nguarantees mmap(2) on 32b systems never sees signed off_t's.\nThe object stream may be broken into multiple packfiles\nas a result,  each properly and conventionally built.\n\nWhen --stdout is also specified,  all objects in the\nresulting packfile(s) _start_ at offsets less than N.\nAll the packfiles appear concatenated on stdout,\nand each has its object count set to 0.  The behavior\nwithout --stdout cannot be duplicated here since\nlseek(2) is not generally possible on stdout.  The end\nof each pack in the concatenated whole can be detected by\nasking,  just before reading each object,  if the next\n20 bytes match the SHA-1 of what came before.\nA future change might insert a null-byte EOF marker\n(i.e. type=OBJ_NONE/length=0) before each pack's final SHA-1\nor before only the final pack's SHA-1.\n\nWhen --blob-limit=N is specified,  blobs whose uncompressed\nsize is greater than or equal to N are omitted from the pack(s).\nIf --pack-limit is specified, --blob-limit is not, and\n--stdout is not,  then --blob-limit defaults to 1/4\nof the --pack-limit.\n\nTo enable this,  csum-file.c now has the ability to rollback\na checksummed write (see sha1mark/sha1undo).\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n builtin-pack-objects.c   |  300 ++++++++++++++++++++++++++++++++++++++--------\n builtin-unpack-objects.c |    2 +-\n csum-file.c              |   41 +++++++\n csum-file.h              |    6 +\n git-repack.sh            |   12 ++-\n http-fetch.c             |    2 +-\n http-push.c              |    2 +-\n index-pack.c             |    2 +-\n sha1_file.c              |    2 +-\n 9 files changed, 307 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex b5f9648..0a11113 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -14,8 +14,8 @@\n #include \"list-objects.h\"\n \n static const char pack_usage[] = \"\\\n-git-pack-objects [{ -q | --progress | --all-progress }] \\n\\\n-\t[--local] [--incremental] [--window=N] [--depth=N] \\n\\\n+git-pack-objects [{ -q | --progress | --all-progress }] [--pack-limit[=N]]\\n\\\n+\t[--blob-limit=N] [--local] [--incremental] [--window=N] [--depth=N]\\n\\\n \t[--no-reuse-delta] [--delta-base-offset] [--non-empty] \\n\\\n \t[--revs [--unpacked | --all]*] [--reflog] [--stdout | base-name] \\n\\\n \t[<ref-list | <object-list]\";\n@@ -29,6 +29,7 @@ struct object_entry {\n \tunsigned int depth;\t/* delta depth */\n \tunsigned int delta_limit;\t/* base adjustment for in-pack delta */\n \tunsigned int hash;\t/* name hint hash */\n+\tchar no_write;\t\t/* flag: written to previous pack OR too big */\n \tenum object_type type;\n \tenum object_type in_pack_type;\t/* could be delta */\n \tunsigned long delta_size;\t/* delta data size (uncompressed) */\n@@ -68,7 +69,10 @@ static int allow_ofs_delta;\n \n static struct object_entry **sorted_by_sha, **sorted_by_type;\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, nr_skipped = 0;\n+static uint32_t offset_limit = 0;\n+static uint32_t blob_limit = 0;\n static const char *base_name;\n static unsigned char pack_file_sha1[20];\n static int progress = 1;\n@@ -415,13 +419,17 @@ static off_t write_object(struct sha1file *f,\n \t}\n \n \tif (!to_reuse) {\n+\t\tint usable_delta =\t!entry->delta ? 0 :\n+\t\t\t\t\t!offset_limit ? 1 :\n+\t\t\t\t\tentry->delta->no_write ? 0 :\n+\t\t\t\t\tentry->delta->offset ? 1 : 0;\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@@ -503,62 +511,212 @@ static off_t write_one(struct sha1file *f,\n \t\t\t       struct object_entry *e,\n \t\t\t       off_t offset)\n {\n-\tif (e->offset || e->preferred_base)\n+\tstruct sha1posn posn;\n+\tuint32_t save_written = 0, save_written_delta = 0;\n+\tuint32_t save_reused = 0, save_reused_delta = 0;\n+\tif (e->offset || e->preferred_base || e->no_write)\n \t\t/* offset starts from header size and cannot be zero\n \t\t * if it is written already.\n \t\t */\n \t\treturn offset;\n \t/* if we are deltified, write out its 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 offset;\n+\t}\n+\tif (offset_limit) {\n+\t\tif ( !pack_to_stdout ) {\n+\t\t\t/* save state before write for possible later seekback */\n+\t\t\tsave_written = written, save_written_delta = written_delta;\n+\t\t\tsave_reused = reused, save_reused_delta = reused_delta;\n+\t\t\tsha1mark(f, &posn);\n+\t\t} else if ( (unsigned long)offset >= (unsigned long)offset_limit )\n+\t\t\t/*\n+\t\t\t * This ensures that no object's offset in the pack\n+\t\t\t * exceeds or is equal to the offset_limit.  It is\n+\t\t\t * a looser way of limiting packsize w/o seeking and\n+\t\t\t * is used in --stdout mode.\n+\t\t\t */\n+\t\t\treturn 0;\n+\t}\n \te->offset = offset;\n-\treturn offset + write_object(f, e);\n+\toffset += write_object(f, e);\n+\t/*\n+\t * This ensures that the packfile size never exceeds or matches\n+\t * the offset_limit if supplied.  The \"20\" is for the final SHA1.\n+\t * This limit isn't used with --stdout since it requires seeking.\n+\t */\n+\tif (offset_limit && !pack_to_stdout &&\n+\t    (unsigned long)offset >= (unsigned long)(offset_limit - 20)) {\n+\t\twritten = save_written, written_delta = save_written_delta;\n+\t\treused = save_reused, reused_delta = save_reused_delta;\n+\t\tsha1undo(f, &posn, offset, e->offset);\n+\t\te->offset = 0;\n+\t\treturn 0;\n+\t}\n+\t*written_list++ = e;\n+\treturn offset;\n+}\n+\n+/*\n+ * Move this, the version in fast-import.c,\n+ * and index_pack.c:readjust_pack_header_and_sha1 into sha1_file.c ?\n+ */\n+static void fixup_header_footer(int pack_fd, unsigned char *pack_file_sha1,\n+\t\t\t\tchar *pack_name, uint32_t object_count)\n+{\n+\tstatic const int buf_sz = 128 * 1024;\n+\tSHA_CTX c;\n+\tstruct pack_header hdr;\n+\tchar *buf;\n+\n+\tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n+\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\tif (read_in_full(pack_fd, &hdr, sizeof(hdr)) != sizeof(hdr))\n+\t\tdie(\"Unable to reread header of %s\", pack_name);\n+\tif (lseek(pack_fd, 0, SEEK_SET) != 0)\n+\t\tdie(\"Failed seeking to start: %s\", strerror(errno));\n+\thdr.hdr_entries = htonl(object_count);\n+\twrite_or_die(pack_fd, &hdr, sizeof(hdr));\n+\n+\tSHA1_Init(&c);\n+\tSHA1_Update(&c, &hdr, sizeof(hdr));\n+\n+\tbuf = xmalloc(buf_sz);\n+\tfor (;;) {\n+\t\tsize_t n = xread(pack_fd, buf, buf_sz);\n+\t\tif (!n)\n+\t\t\tbreak;\n+\t\tif (n < 0)\n+\t\t\tdie(\"Failed to checksum %s\", pack_name);\n+\t\tSHA1_Update(&c, buf, n);\n+\t}\n+\tfree(buf);\n+\n+\tSHA1_Final(pack_file_sha1, &c);\n+\twrite_or_die(pack_fd, pack_file_sha1, 20);\n+\tclose(pack_fd);\n }\n \n+typedef int (*entry_sort_t)(const struct object_entry *, const struct object_entry *);\n+\n+static entry_sort_t current_sort;\n+\n+/* forward declarations for write_pack_file */\n+/* (probably should move sorting stuff up here) */\n+static int sort_comparator(const void *_a, const void *_b);\n+static int sha1_sort(const struct object_entry *a, const struct object_entry *b);\n+static void write_index_file(void);\n+\n static void write_pack_file(void)\n {\n-\tuint32_t i;\n+\tuint32_t i, j;\n \tstruct sha1file *f;\n \toff_t offset;\n \tstruct pack_header hdr;\n \tunsigned last_percent = 999;\n-\tint do_progress = progress;\n+\tint do_progress = progress >> !base_name;\n+\tchar oldname[PATH_MAX];\n+\tint pack_fd;\n+\tstruct object_entry **list;\n+\tSHA_CTX ctx;\n+\tuint32_t nr_actual = nr_result - nr_skipped;\n \n-\tif (!base_name) {\n-\t\tf = sha1fd(1, \"<stdout>\");\n-\t\tdo_progress >>= 1;\n-\t}\n-\telse\n-\t\tf = sha1create(\"%s-%s.%s\", base_name,\n-\t\t\t       sha1_to_hex(object_list_sha1), \"pack\");\n \tif (do_progress)\n-\t\tfprintf(stderr, \"Writing %u objects.\\n\", nr_result);\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\toffset = write_one(f, objects + i, offset);\n-\t\tif (do_progress) {\n-\t\t\tunsigned percent = written * 100 / nr_result;\n-\t\t\tif (progress_update || percent != last_percent) {\n-\t\t\t\tfprintf(stderr, \"%4u%% (%u/%u) done\\r\",\n-\t\t\t\t\tpercent, written, nr_result);\n-\t\t\t\tprogress_update = 0;\n-\t\t\t\tlast_percent = percent;\n+\t\tfprintf(stderr, \"Writing %u objects.\\n\", nr_actual);\n+\twritten_list = list = xmalloc(nr_objects * sizeof(struct object_entry *));\n+\n+\tfor (i = 0; i < nr_objects;) {\n+\t\tif (!base_name) {\n+\t\t\tf = sha1fd(pack_fd = 1, \"<stdout>\");\n+\t\t}\n+\t\telse {\n+\t\t\tint len = snprintf(oldname, sizeof oldname, \"%s-XXXXXX\", base_name);\n+\t\t\tif (len >= PATH_MAX)\n+\t\t\t\tdie(\"excessive pathname length for initial packfile name\");\n+\t\t\tpack_fd = mkstemp(oldname);\n+\t\t\tif (pack_fd < 0)\n+\t\t\t\tdie(\"can't create %s: %s\", oldname, strerror(errno));\n+\t\t\tf = sha1fd(pack_fd, oldname);\n+\t\t}\n+\n+\t\thdr.hdr_signature = htonl(PACK_SIGNATURE);\n+\t\thdr.hdr_version = htonl(PACK_VERSION);\n+\t\thdr.hdr_entries = htonl(!base_name && offset_limit ? 0 : nr_actual);\n+\t\tsha1write(f, &hdr, sizeof(hdr));\n+\t\toffset = sizeof(hdr);\n+\t\tfor (; i < nr_objects; i++) {\n+\t\t\toff_t offset_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\tunsigned percent = written * 100 / nr_actual;\n+\t\t\t\tif (progress_update || percent != last_percent) {\n+\t\t\t\t\tfprintf(stderr, \"%4u%% (%u/%u) done\\r\",\n+\t\t\t\t\t\tpercent, written, nr_actual);\n+\t\t\t\t\tprogress_update = 0;\n+\t\t\t\t\tlast_percent = percent;\n+\t\t\t\t}\n \t\t\t}\n \t\t}\n+\t\tnr_written = written_list - list;\n+\t\twritten_list = list;\n+\n+\t\t/*\n+\t\t * Write terminator record here if desired:\n+\t\t * type=OBJ_NONE, len=0;  this is a zero byte.\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 (gackk).\n+\t\t */\n+\t\tif ( !base_name || nr_written == nr_actual ) {\n+\t\t\tsha1close(f, pack_file_sha1, 1);\n+\t\t} else {\n+\t\t\tsha1close(f, pack_file_sha1, -1);\n+\t\t\tfixup_header_footer(pack_fd, pack_file_sha1, oldname, nr_written);\n+\t\t}\n+\n+\t\t/*\n+\t\t * compute object_list_sha1 of sorted sha's we just wrote out;\n+\t\t * we also mark these objects as written\n+\t\t */\n+\t\tcurrent_sort = sha1_sort;\n+\t\tqsort(list, nr_written, sizeof(struct object_entry *), sort_comparator);\n+\t\tSHA1_Init(&ctx);\n+\t\tfor (j = 0; j < nr_written; j++) {\n+\t\t\tstruct object_entry *entry = *list++;\n+\t\t\tentry->no_write = 1;\n+\t\t\tSHA1_Update(&ctx, entry->sha1, 20);\n+\t\t}\n+\t\tSHA1_Final(object_list_sha1, &ctx);\n+\t\tlist = written_list;\n+\t\t/*\n+\t\t * now we can rename the pack correctly and write the index file\n+\t\t */\n+\t\tif (base_name) {\n+\t\t\tchar newname[PATH_MAX];\n+\t\t\tint len = snprintf(newname, sizeof newname, \"%s-%s.%s\",\n+\t\t\t\t\t\tbase_name, sha1_to_hex(object_list_sha1), \"pack\");\n+\t\t\tif (len >= PATH_MAX)\n+\t\t\t\tdie(\"excessive pathname length for final packfile name\");\n+\t\t\tif (rename(oldname, newname) < 0)\n+\t\t\t\tdie(\"could not rename the pack file\");\n+\t\t}\n+\t\tif (!pack_to_stdout) {\n+\t\t\twrite_index_file();\n+\t\t\tputs(sha1_to_hex(object_list_sha1));\n+\t\t}\n \t}\n-\tif (do_progress)\n+\n+\tfree(written_list);\n+\tif (nr_actual && do_progress)\n \t\tfputc('\\n', stderr);\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+\tif (written != nr_actual)\n+\t\tdie(\"wrote %u objects while expecting %u\", written, nr_actual);\n }\n \n static void write_index_file(void)\n@@ -566,8 +724,8 @@ static void write_index_file(void)\n \tuint32_t i;\n \tstruct sha1file *f = sha1create(\"%s-%s.%s\", base_name,\n \t\t\t\t\tsha1_to_hex(object_list_sha1), \"idx\");\n-\tstruct object_entry **list = sorted_by_sha;\n-\tstruct object_entry **last = list + nr_result;\n+\tstruct object_entry **list = written_list;\n+\tstruct object_entry **last = list + nr_written;\n \tuint32_t array[256];\n \n \t/*\n@@ -583,7 +741,7 @@ static void write_index_file(void)\n \t\t\t\tbreak;\n \t\t\tnext++;\n \t\t}\n-\t\tarray[i] = htonl(next - sorted_by_sha);\n+\t\tarray[i] = htonl(next - written_list);\n \t\tlist = next;\n \t}\n \tsha1write(f, array, 256 * 4);\n@@ -591,8 +749,8 @@ static void write_index_file(void)\n \t/*\n \t * Write the actual SHA1 entries..\n \t */\n-\tlist = sorted_by_sha;\n-\tfor (i = 0; i < nr_result; i++) {\n+\tlist = written_list;\n+\tfor (i = 0; i < nr_written; i++) {\n \t\tstruct object_entry *entry = *list++;\n \t\tuint32_t offset = htonl(entry->offset);\n \t\tsha1write(f, &offset, 4);\n@@ -1014,7 +1172,7 @@ static void check_object(struct object_entry *entry)\n \t\t\t\tofs = c & 127;\n \t\t\t\twhile (c & 128) {\n \t\t\t\t\tofs += 1;\n-\t\t\t\t\tif (!ofs || ofs & ~(~0UL >> 7))\n+\t\t\t\t\tif (!ofs || ofs & ~(~0UL >> 1))\n \t\t\t\t\t\tdie(\"delta base offset overflow in pack for %s\",\n \t\t\t\t\t\t    sha1_to_hex(entry->sha1));\n \t\t\t\t\tc = buf[used_0++];\n@@ -1058,6 +1216,7 @@ static void check_object(struct object_entry *entry)\n \t}\n \n \tentry->type = sha1_object_info(entry->sha1, &entry->size);\n+\tnr_skipped += entry->no_write = blob_limit && (unsigned long)entry->size >= blob_limit;\n \tif (entry->type < 0)\n \t\tdie(\"unable to get type of object %s\",\n \t\t    sha1_to_hex(entry->sha1));\n@@ -1103,10 +1262,6 @@ static void get_object_details(void)\n \t}\n }\n \n-typedef int (*entry_sort_t)(const struct object_entry *, const struct object_entry *);\n-\n-static entry_sort_t current_sort;\n-\n static int sort_comparator(const void *_a, const void *_b)\n {\n \tstruct object_entry *a = *(struct object_entry **)_a;\n@@ -1197,6 +1352,12 @@ static int try_delta(struct unpacked *trg, struct unpacked *src,\n \tif (trg_entry->type != src_entry->type)\n \t\treturn -1;\n \n+\t/* Don't try deltas involving excessively large objects when\n+\t * pack size is limited\n+\t */\n+\tif (trg_entry->no_write || src_entry->no_write)\n+\t\treturn -1;\n+\n \t/* We do not compute delta to *create* objects we are not\n \t * going to pack.\n \t */\n@@ -1538,6 +1699,7 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \tconst char **rp_av;\n \tint rp_ac_alloc = 64;\n \tint rp_ac;\n+\tint added = 0;\n \n \trp_av = xcalloc(rp_ac_alloc, sizeof(*rp_av));\n \n@@ -1566,6 +1728,24 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tincremental = 1;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!strcmp(\"--pack-limit\", arg)) {\n+\t\t\toffset_limit = 1UL << 31;\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!prefixcmp(arg, \"--pack-limit=\")) {\n+\t\t\tchar *end;\n+\t\t\toffset_limit = strtoul(arg+13, &end, 0);\n+\t\t\tif (!arg[13] || *end)\n+\t\t\t\tusage(pack_usage);\n+\t\t\tcontinue;\n+\t\t}\n+\t\tif (!prefixcmp(arg, \"--blob-limit=\")) {\n+\t\t\tchar *end;\n+\t\t\tblob_limit = strtoul(arg+13, &end, 0);\n+\t\t\tif (!arg[13] || *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@@ -1629,6 +1809,24 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tusage(pack_usage);\n \t}\n+\tif ( offset_limit && !blob_limit && !pack_to_stdout ) {\n+\t\t/* need to limit blob size when creating bounded packs on disk */\n+\t\tblob_limit = offset_limit >> 2;\n+\t\tadded |= 2;\n+\t}\n+\tif ( offset_limit && !no_reuse_delta ) {\n+\t\t/* didn't audit this case yet */\n+\t\tno_reuse_delta = 1;\n+\t\tadded |= 1;\n+\t}\n+\tif ( added ) {\n+\t\tfprintf(stderr, \"Added to command line:\");\n+\t\tif ( added & 1 )\n+\t\t\tfprintf(stderr, \" --no-reuse-delta\");\n+\t\tif ( added & 2 )\n+\t\t\tfprintf(stderr, \" --blob-limit=%u\", blob_limit);\n+\t\tfprintf(stderr, \"\\n\");\n+\t}\n \n \t/* Traditionally \"pack-objects [options] base extra\" failed;\n \t * we would however want to take refs parameter that would\n@@ -1695,10 +1893,6 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tprogress_update = 0;\n \t\t}\n \t\twrite_pack_file();\n-\t\tif (!pack_to_stdout) {\n-\t\t\twrite_index_file();\n-\t\t\tputs(sha1_to_hex(object_list_sha1));\n-\t\t}\n \t}\n \tif (progress)\n \t\tfprintf(stderr, \"Total %u (delta %u), reused %u (delta %u)\\n\",\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 3956c56..84a6c5c 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -209,7 +209,7 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\tbase_offset = c & 127;\n \t\twhile (c & 128) {\n \t\t\tbase_offset += 1;\n-\t\t\tif (!base_offset || base_offset & ~(~0UL >> 7))\n+\t\t\tif (!base_offset || base_offset & ~(~0UL >> 1))\n \t\t\t\tdie(\"offset value overflow for delta base object\");\n \t\t\tpack = fill(1);\n \t\t\tc = *pack;\ndiff --git a/csum-file.c b/csum-file.c\nindex b7174c6..e2bef75 100644\n--- a/csum-file.c\n+++ b/csum-file.c\n@@ -35,7 +35,10 @@ int sha1close(struct sha1file *f, unsigned char *result, int update)\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 (update < 0)\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@@ -69,6 +72,44 @@ int sha1write(struct sha1file *f, void *buf, unsigned int count)\n \treturn 0;\n }\n \n+/*\n+ * Save current position/state in output file\n+ * (needs to be fast -- no system calls!)\n+ */\n+void sha1mark(struct sha1file *f, struct sha1posn *p)\n+{\n+\tp->offset = f->offset;\n+\tp->ctx = f->ctx;\t/* larger than I'd like */\n+}\n+\n+/*\n+ * Restore previous position/state in output file\n+ * (can be slow)\n+ */\n+void sha1undo(struct sha1file *f, struct sha1posn *p, long new, long old)\n+{\n+\tif (new - old == (long)f->offset - (long)p->offset) {\n+\t\tf->ctx = p->ctx;\n+\t\tf->offset = p->offset;\n+\t\treturn;\n+\t}\n+\tif (lseek(f->fd, (off_t)old - (off_t)p->offset, SEEK_SET) == (off_t)-1)\n+\t\tdie(\"sha1 file '%s' undo seekback error (%s)\", f->name, strerror(errno));\n+\tf->ctx = p->ctx;\n+\twhile (p->offset) {\n+\t\tint ret = xread(f->fd, f->buffer, p->offset);\n+\t\tif (!ret)\n+\t\t\tdie(\"sha1 file '%s' undo readback error. No data\", f->name);\n+\t\tif (ret < 0)\n+\t\t\tdie(\"sha1 file '%s' undo readback error (%s)\", f->name, strerror(errno));\n+\t\tSHA1_Update(&f->ctx, f->buffer, ret);\n+\t\tp->offset -= ret;\n+\t}\n+\tif (ftruncate(f->fd, (off_t)old))\n+\t\tdie(\"sha1 file '%s' undo truncate error (%s)\", f->name, strerror(errno));\n+\tf->offset = 0;\n+}\n+\n struct sha1file *sha1create(const char *fmt, ...)\n {\n \tstruct sha1file *f;\ndiff --git a/csum-file.h b/csum-file.h\nindex 3ad1a99..780df17 100644\n--- a/csum-file.h\n+++ b/csum-file.h\n@@ -9,11 +9,17 @@ struct sha1file {\n \tchar name[PATH_MAX];\n \tunsigned char buffer[8192];\n };\n+struct sha1posn {\n+\tunsigned int offset;\n+\tSHA_CTX ctx;\n+};\n \n extern struct sha1file *sha1fd(int fd, const char *name);\n extern struct sha1file *sha1create(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n extern int sha1close(struct sha1file *, unsigned char *, int);\n extern int sha1write(struct sha1file *, void *, unsigned int);\n extern int sha1write_compressed(struct sha1file *, void *, unsigned int);\n+extern void sha1mark(struct sha1file *, struct sha1posn *);\n+extern void sha1undo(struct sha1file *, struct sha1posn *, long, long);\n \n #endif\ndiff --git a/git-repack.sh b/git-repack.sh\nindex ddfa8b4..0299ff1 100755\n--- a/git-repack.sh\n+++ b/git-repack.sh\n@@ -18,6 +18,9 @@ do\n \t-q)\tquiet=-q ;;\n \t-f)\tno_reuse_delta=--no-reuse-delta ;;\n \t-l)\tlocal=--local ;;\n+\t--pack-limit) extra=\"$extra $1\" ;;\n+\t--pack-limit=*) extra=\"$extra $1\" ;;\n+\t--blob-limit=*) extra=\"$extra $1\" ;;\n \t--window=*) extra=\"$extra $1\" ;;\n \t--depth=*) extra=\"$extra $1\" ;;\n \t*)\tusage ;;\n@@ -62,11 +65,12 @@ case \",$all_into_one,\" in\n esac\n \n args=\"$args $local $quiet $no_reuse_delta$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 +96,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\ndiff --git a/http-fetch.c b/http-fetch.c\nindex 557b403..09baedc 100644\n--- a/http-fetch.c\n+++ b/http-fetch.c\n@@ -198,7 +198,7 @@ static void start_object_request(struct object_request *obj_req)\n \t\tSHA1_Init(&obj_req->c);\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\n-\t\t\tlseek(obj_req->local, SEEK_SET, 0);\n+\t\t\tlseek(obj_req->local, 0, SEEK_SET);\n \t\t\tftruncate(obj_req->local, 0);\n \t\t}\n \t}\ndiff --git a/http-push.c b/http-push.c\nindex 724720c..e3f7675 100644\n--- a/http-push.c\n+++ b/http-push.c\n@@ -312,7 +312,7 @@ static void start_fetch_loose(struct transfer_request *request)\n \t\tSHA1_Init(&request->c);\n \t\tif (prev_posn>0) {\n \t\t\tprev_posn = 0;\n-\t\t\tlseek(request->local_fileno, SEEK_SET, 0);\n+\t\t\tlseek(request->local_fileno, 0, SEEK_SET);\n \t\t\tftruncate(request->local_fileno, 0);\n \t\t}\n \t}\ndiff --git a/index-pack.c b/index-pack.c\nindex 6284fe3..d35c4c4 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -249,7 +249,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_\n \t\tbase_offset = c & 127;\n \t\twhile (c & 128) {\n \t\t\tbase_offset += 1;\n-\t\t\tif (!base_offset || base_offset & ~(~0UL >> 7))\n+\t\t\tif (!base_offset || base_offset & ~(~0UL >> 1))\n \t\t\t\tbad_object(obj->offset, \"offset value overflow for delta base object\");\n \t\t\tp = fill(1);\n \t\t\tc = *p;\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9c26038..7082d3e 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1149,7 +1149,7 @@ static off_t get_delta_base(struct packed_git *p,\n \t\tbase_offset = c & 127;\n \t\twhile (c & 128) {\n \t\t\tbase_offset += 1;\n-\t\t\tif (!base_offset || base_offset & ~(~0UL >> 7))\n+\t\t\tif (!base_offset || base_offset & ~(~0UL >> 1))\n \t\t\t\tdie(\"offset value overflow for delta base object\");\n \t\t\tc = base_info[used++];\n \t\t\tbase_offset = (base_offset << 7) + (c & 127);\n-- \n1.5.1.rc2.18.g9c88-dirty\n\n"},{"id":"38659","messageId":"alpine.LFD.0.98.0704041750030.28181@xanadu.home","threadId":"7516","inReplyTo":"56b7f5510704041340l5997b04dp853e8270a644ab28@mail.gmail.com","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-04T22:04:55Z","receivedAt":"2007-04-04T22:04:55Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 4 Apr 2007, Dana How wrote:\n\n> The motivations are to better support portable media,\n> older filesystems,  and larger repositories without\n> awkward enormous packfiles.\n\nI wouldn't qualify \"enormous\" pack files as \"awkward\".\n\nIt will always be more efficient to have only one pack to deal with \n(when possible of course).\n\n> When --pack-limit[=N] is specified and --stdout is not,\n> all bytes in the resulting packfile(s) appear at offsets\n> less than N (which defaults to 1<<31).  The default\n> guarantees mmap(2) on 32b systems never sees signed off_t's.\n> The object stream may be broken into multiple packfiles\n> as a result,  each properly and conventionally built.\n> \n\nThis sounds fine.  *However* how do you ensure that the second pack (or \nsubsequent packs) is self contained with regards to delta base objects \nwhen it is _not_ meant to be a thin pack?\n\n> When --stdout is also specified,  all objects in the\n> resulting packfile(s) _start_ at offsets less than N.\n> All the packfiles appear concatenated on stdout,\n> and each has its object count set to 0.  The behavior\n> without --stdout cannot be duplicated here since\n> lseek(2) is not generally possible on stdout.\n\nPlease scrap that.  There is simply no point making --pack-limit and \n--stdout work together.  If the amount of data to send over the GIT \nprotocol exceeds 4G (or whatever) it is the receiving end's business to \nsplit it up _if_ it wants/has to.  The alternative is just too ugly.\n\n> When --blob-limit=N is specified,  blobs whose uncompressed\n> size is greater than or equal to N are omitted from the pack(s).\n> If --pack-limit is specified, --blob-limit is not, and\n> --stdout is not,  then --blob-limit defaults to 1/4\n> of the --pack-limit.\n\nIs this really useful?\n\nIf you have a pack size limit and a blob cannot make it even in a pack \nof its \nown then you're screwed anyway.  It is much better to simply fail the \noperation than leaving some blobs behind.  IOW I don't see the \nusefulness of this feature.\n\n\nNicolas\n"},{"id":"38663","messageId":"56b7f5510704041555q4e735961ra9ee8008be0d33db@mail.gmail.com","threadId":"7516","inReplyTo":"alpine.LFD.0.98.0704041750030.28181@xanadu.home","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-04-04T22:55:18Z","receivedAt":"2007-04-04T22:55:18Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 4/4/07, Nicolas Pitre <nico@cam.org> wrote:\n> On Wed, 4 Apr 2007, Dana How wrote:\n> > The motivations are to better support portable media,\n> > older filesystems,  and larger repositories without\n> > awkward enormous packfiles.\n>\n> I wouldn't qualify \"enormous\" pack files as \"awkward\".\n>\n> It will always be more efficient to have only one pack to deal with\n> (when possible of course).\nYes.  \"(when possible of course)\" refers to the remaining motivations\nI didn't explicitly mention: the 32b offset limit in .idx files,\nand keeping the mmap code working on a 32b system.\nI realize there are better solutions in the pipeline,\nbut I'd like to address this now (for my own use) and hopefully\nalso create something useful for 4GB-limited filesystems,\nUSB sticks, etc.\n\n> > When --pack-limit[=N] is specified and --stdout is not,\n> > all bytes in the resulting packfile(s) appear at offsets\n> > less than N (which defaults to 1<<31).  The default\n> > guarantees mmap(2) on 32b systems never sees signed off_t's.\n> > The object stream may be broken into multiple packfiles\n> > as a result,  each properly and conventionally built.\n> >\n>\n> This sounds fine.  *However* how do you ensure that the second pack (or\n> subsequent packs) is self contained with regards to delta base objects\n> when it is _not_ meant to be a thin pack?\nGood question.  Search for \"int usable_delta\" in the patch.\nWith --pack-limit (offset_limit in C), you can use a delta if the base\nis in the same pack and already written out.  The first condition\naddresses your concern, and the second handles the case\nwhere the base object gets pushed to the next pack.\nThese restrictions should be loosened for --thin-pack\nbut I didn't do that yet.\nAlso, --pack-limit turns on --no-reuse-delta.\nThis is not necessary, but not doing it would have meant\nhacking up even more conditions which I didn't want to do\non a newbie submission.\n\n> > When --stdout is also specified,  all objects in the\n> > resulting packfile(s) _start_ at offsets less than N.\n> > All the packfiles appear concatenated on stdout,\n> > and each has its object count set to 0.  The behavior\n> > without --stdout cannot be duplicated here since\n> > lseek(2) is not generally possible on stdout.\n>\n> Please scrap that.  There is simply no point making --pack-limit and\n> --stdout work together.  If the amount of data to send over the GIT\n> protocol exceeds 4G (or whatever) it is the receiving end's business to\n> split it up _if_ it wants/has to.  The alternative is just too ugly.\nI have a similar but much weaker reaction, but Linus specifically asked for\nthis combination to work.  So I made it work as well as possible\ngiven no seeking.\n\n> > When --blob-limit=N is specified,  blobs whose uncompressed\n> > size is greater than or equal to N are omitted from the pack(s).\n> > If --pack-limit is specified, --blob-limit is not, and\n> > --stdout is not,  then --blob-limit defaults to 1/4\n> > of the --pack-limit.\n> Is this really useful?\n>\n> If you have a pack size limit and a blob cannot make it even in a pack\n> of its\n> own then you're screwed anyway.  It is much better to simply fail the\n> operation than leaving some blobs behind.  IOW I don't see the\n> usefulness of this feature.\nI agree if --stdout is specified.  This is why --pack-limit && --stdout\nDON'T turn on --blob-limit if not specified.\n\nHowever, if I'm building packs inside a non-(web-)published\nrepository, I find this useful. First of all, if there's some blob bigger\nthan the --pack-limit I must drop it anyway -- it's not clear to me that\nthe mmap window code works on 32b systems\nwith >2GB-sized objects in packs.  An \"all-or-nothing\" limitation\nwouldn't be helpful to me.\nBut blobs even close to the packfile limit don't seem all that useful\nto pack either (this of course is a weaker argument).\nIn the sample (p4) checkout I'm testing on [i.e. no history],\nI have 56K+ objects consuming ~55GB uncompressed;\nthere are 9 blobs over 500MB each uncompressed.\nI'm guessing packing them is not a performance advantage,\nand I certainly wouldn't want frequently-used objects to be\nstuck between them.  [ I guess my repo stats are going to\nbe a bit strange ;-) ]\n\nPacking plays two roles: archive storage (long life) and\ntransmission (possibly short life).\nThese seem to pull the packing code in different directions.\n\nThanks,\n-- \nDana L. How  danahow@gmail.com  +1 650 804 5991 cell\n"},{"id":"38668","messageId":"alpine.LFD.0.98.0704042203160.28181@xanadu.home","threadId":"7516","inReplyTo":"56b7f5510704041555q4e735961ra9ee8008be0d33db@mail.gmail.com","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-05T03:17:45Z","receivedAt":"2007-04-05T03:17:45Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Wed, 4 Apr 2007, Dana How wrote:\n\n> On 4/4/07, Nicolas Pitre <nico@cam.org> wrote:\n> > On Wed, 4 Apr 2007, Dana How wrote:\n> > > The motivations are to better support portable media,\n> > > older filesystems,  and larger repositories without\n> > > awkward enormous packfiles.\n> > \n> > I wouldn't qualify \"enormous\" pack files as \"awkward\".\n> > \n> > It will always be more efficient to have only one pack to deal with\n> > (when possible of course).\n> Yes.  \"(when possible of course)\" refers to the remaining motivations\n> I didn't explicitly mention: the 32b offset limit in .idx files,\n> and keeping the mmap code working on a 32b system.\n> I realize there are better solutions in the pipeline,\n> but I'd like to address this now (for my own use) and hopefully\n> also create something useful for 4GB-limited filesystems,\n> USB sticks, etc.\n\nI think this is a valid feature to have, no problem there.\n\n> > > When --pack-limit[=N] is specified and --stdout is not,\n> > > all bytes in the resulting packfile(s) appear at offsets\n> > > less than N (which defaults to 1<<31).  The default\n> > > guarantees mmap(2) on 32b systems never sees signed off_t's.\n> > > The object stream may be broken into multiple packfiles\n> > > as a result,  each properly and conventionally built.\n> > >\n> > \n> > This sounds fine.  *However* how do you ensure that the second pack (or\n> > subsequent packs) is self contained with regards to delta base objects\n> > when it is _not_ meant to be a thin pack?\n> Good question.  Search for \"int usable_delta\" in the patch.\n> With --pack-limit (offset_limit in C), you can use a delta if the base\n> is in the same pack and already written out.  The first condition\n> addresses your concern, and the second handles the case\n> where the base object gets pushed to the next pack.\n> These restrictions should be loosened for --thin-pack\n> but I didn't do that yet.\n\nOK.\n\n> Also, --pack-limit turns on --no-reuse-delta.\n> This is not necessary, but not doing it would have meant\n> hacking up even more conditions which I didn't want to do\n> on a newbie submission.\n\nThing is delta reusing is one of the most important feature for good \nperformances so it has to work.\n\nBut let's take a moment to talk about your \"newbie submission\".  This \npatch is _way_ too large and covers too many things at once.  You'll \nhave to split it in many smaller logical units pretty please.  For \nexample, if you have to borrow code from fast-import then make a patch \nthat perform that code movement _only_.  Then you can have another patch \nthat move code around within builtin-pack-objects.c without changing any \nfunctionality just to prepare the way for the next patch which would \nconcentrate on the new feature only.  Then make sure you have the \naddition of the new capability separate from the patch that let other \npart of GIT use it.  Etc.\n\nThat would be much easier for us to comment on smaller patches, and \nespecially easier for you to rework one of the smaller patches than the \nbig one if need be.\n\nI looked at your patch and there are things I like and other things I \ndon't like at all.  Because it is a single large patch I may only NAK \nthe whole of it.\n\n> > > When --stdout is also specified,  all objects in the\n> > \n> > Please scrap that.  There is simply no point making --pack-limit and\n> > --stdout work together.  If the amount of data to send over the GIT\n> > protocol exceeds 4G (or whatever) it is the receiving end's business to\n> > split it up _if_ it wants/has to.  The alternative is just too ugly.\n> I have a similar but much weaker reaction, but Linus specifically asked for\n> this combination to work.\n\nLinus is a total imcompetent who knows nothing about programming or good \ntaste.  So never ever listen to what he says.  He is wrong, always.\n\nAnd in this case he's more wrong than usual.\n\n;-)\n\n> So I made it work as well as possible\n> given no seeking.\n\nThe fact is that there is no point in imposing split packs on the \nreceiving end if it can accept a single pack just fine.  Pack layout is \nand must remain a local policy, not something that the remote end felt \nwas good for the peer.  This is true for the treshold value to decide \nwhether fetched packs are kept as is or unpacked as loose objects.  This \nis the same issue for pack size limit.\n\nAnd as you discovered yourself, it is quite messy to implement in \npack-objects when the output is a stream because you don't know in \nadvance how many objects will be sent so you have to invent special \nmarkers to indicate the end of pack.  This in turn doesn't let \nindex-pack know how much to expect and provide some progress report at \nall.  The appropriate location for the splitting of packs in a fetch \ncontext is really within index-pack not in pack-objects.  And it is \nprobably so much easier to do in index-pack anyway.\n\n> > > When --blob-limit=N is specified,  blobs whose uncompressed\n> > > size is greater than or equal to N are omitted from the pack(s).\n> > > If --pack-limit is specified, --blob-limit is not, and\n> > > --stdout is not,  then --blob-limit defaults to 1/4\n> > > of the --pack-limit.\n> > Is this really useful?\n> > \n> > If you have a pack size limit and a blob cannot make it even in a \n> > pack of its own then you're screwed anyway.  It is much better to \n> > simply fail the operation than leaving some blobs behind.  IOW I \n> > don't see the usefulness of this feature.\n> I agree if --stdout is specified.  This is why --pack-limit && --stdout\n> DON'T turn on --blob-limit if not specified.\n\nLet's forget about --stdout.  I hope I convinced you that it doesn't \nmake sense.\n\n> However, if I'm building packs inside a non-(web-)published\n> repository, I find this useful. First of all, if there's some blob bigger\n> than the --pack-limit I must drop it anyway -- it's not clear to me that\n> the mmap window code works on 32b systems\n> with >2GB-sized objects in packs.  An \"all-or-nothing\" limitation\n> wouldn't be helpful to me.\n\nBut do you realize that if you drop even a single object you are \nscrewed?  The fetching of a repo that is missing objects is a corrupt \nfetch.  You cannot just sent a bunch of objects and leave a couple \nbehind in the hope that the peer will never need them.  For one the peer \nwould never be able to perform a successful git-fsck.\n\nIf you care about your local usage only then this feature is bogus as \nwell.  The blob _has_ to exist as a loose object before ever being \npacked.  If you have blobs larger than 2GB or 4GB and your filesystem is \nunable to cope with that then you're screwed already.  You won't be able \nto check them out, etc.\n\n> But blobs even close to the packfile limit don't seem all that useful\n> to pack either (this of course is a weaker argument).\n\nIn the extreme case where you have a blob that is near the pack size \nlimit then you'll end up with one pack per blob. No problem there.  If \nyou're lucky then you might have 10 big blobs which size is near the \npack size limit, but because they delta well against each other you \nmight be able to stuff them all in a single pack.\n\nAnd if a blob is larger than the pack limit then either you should \nincrease your pack size limit, or if the pack limit is already near the \nfilesystem capacity for a single file then the blob cannot exist on that \nfilesystem in the first place.\n\nSo you still have to convince me this is a useful feature.\n\n> Packing plays two roles: archive storage (long life) and\n> transmission (possibly short life).\n\nPacking is also a _huge_ performance boost.  Don't underestimate that.\n\n\nNicolas\n"},{"id":"38679","messageId":"20070405065433.GD5436@spearce.org","threadId":"7516","inReplyTo":"56b7f5510704041555q4e735961ra9ee8008be0d33db@mail.gmail.com","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-05T06:54:34Z","receivedAt":"2007-04-05T06:54:34Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Dana How <danahow@gmail.com> wrote:\n> it's not clear to me that\n> the mmap window code works on 32b systems\n> with >2GB-sized objects in packs.\n\nHmmph.  Depends on the system.\n\nFor glibc we do try to set _FILE_OFFSET_BITS to 64, so that\neven on 32 bit glibcs we pick up a 64 bit off_t.  But really old\nglibcs won't have any 64 bit off_t support no matter what we do.\nAnd some systems don't know what _FILE_OFFSET_BITS is, so they\ngive us whatever size off_t they want.\n\nForget mmap.  In a 32 bit off_t case there is absolutely no way\nthat open_packed_git_1 can verify the packfile signature (last\n20 bytes) against the index if the packfile is larger than 2 GiB.\nIn this case we will have an off_t that is negative, subtract 20\nfrom it, and its still probably negative.  I doubt SEEK_SET will\nlike a negative offset.  This of course assumes that the earlier\nfstat call actually succeeded on such a file a large file with\nsuch a small off_t.\n\nIf we get through that open_packed_git_1 and actually verify the\nsignature, we either have some random sequence in the middle of the\npackfile that matches the signature in the index (sort of unlikely),\nor our off_t is actually large enough to handle the window position\ntests that the use_pack and in_window functions perform.  In this\nlatter case we shouldn't have any problems with the mmap code.\n\n-- \nShawn.\n"},{"id":"38681","messageId":"20070405071511.GE5436@spearce.org","threadId":"7516","inReplyTo":"alpine.LFD.0.98.0704042203160.28181@xanadu.home","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-05T07:15:12Z","receivedAt":"2007-04-05T07:15:12Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> But let's take a moment to talk about your \"newbie submission\".  This \n> patch is _way_ too large and covers too many things at once.  You'll \n> have to split it in many smaller logical units pretty please.\n\nYes, absolutely!  If I was doing this series I'd probably have this\nbroken into at least 5, maybe even 8 patches.  There's a lot of\nstuff here, and some of it was completely unrelated to each other.\nLike a fix for lseek arguments?  Uhh, that's so not what this patch\nwas about, but is still a good fix.  Nice catch.  Lets get it in,\nbut properly please.\n\nI also noticed a lot of \"/* and here we could do... */\" markers.\nI think these belong more in the commit message than in the code\nitself, as the code gets really cluttered after a while with the\ntodo markers that nobody has done yet.  Odds are, if you don't do\nit in this series, it won't get done for quite a while, if ever.\nMention it in your commit message, so others are aware of it,\nand leave it at that.\n\n> On Wed, 4 Apr 2007, Dana How wrote:\n> > I have a similar but much weaker reaction, but Linus specifically asked for\n> > this combination to work.\n> \n> Linus is a total imcompetent who knows nothing about programming or good \n> taste.  So never ever listen to what he says.  He is wrong, always.\n\nNow now, I wouldn't go that far...\n \n> And in this case he's more wrong than usual.\n\nBut yes, in this case I certainly I agree with you Nico.  ;-)\n \n> The appropriate location for the splitting of packs in a fetch \n> context is really within index-pack not in pack-objects.  And it is \n> probably so much easier to do in index-pack anyway.\n\nOr in a push context.  I have started down the road of doing the\npack splitting in index-pack, but never was able to finish it.\nIt is totally doable in index-pack, but it turned out to be more\nwork than I had time for when I started it.\n\nI still have the topic laying in my repository, and it is pushed\nto my fastimport.git fork on repo.or.cz, if someone wants to take\na look at it.  The patch is far from complete, hence no patch to\nmailing list.\n\n> > But blobs even close to the packfile limit don't seem all that useful\n> > to pack either (this of course is a weaker argument).\n> \n> So you still have to convince me this is a useful feature.\n\nMe too.  Nico's right.  And lets just assume we are in a bad case\nwhere we need to put one (and only one) such blob into each packfile.\nThe packfile on disk will be an additional 32 bytes larger than if\nit were to stay a loose file, and that's assuming the loose file was\ncreated using the newer-style loose file format.  This is unlikely\nto cause a large disk space overhead.\n\nYea, we to have an extra SHA-1 we have to compute when we created\nthe file, but then we also have another SHA-1 to verify that the\nzlib stream is not corrupt, *without* unpacking the zlib stream.\nThat in and of itself could be useful for an fsck, we could do a\nfaster rule that says \"if there's only a couple of blobs in this\npackfile, and the thing is *big*, just do a check of the packfile\nSHA-1 and assume the blobs are OK\".  So there actually might be\nvalue to that extra SHA-1 after all.\n \n-- \nShawn.\n"},{"id":"38713","messageId":"Pine.LNX.4.64.0704050831520.6730@woody.linux-foundation.org","threadId":"7516","inReplyTo":"20070405065433.GD5436@spearce.org","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-05T15:34:56Z","receivedAt":"2007-04-05T15:34:56Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 5 Apr 2007, Shawn O. Pearce wrote:\n> \n> For glibc we do try to set _FILE_OFFSET_BITS to 64\n\nI repeat: that's _broken_.\n\nIt's in no way portable. It's a glibc horror. It should not be used.\n\nIt was a quick hack, but the real way to do it is to use \"loff_t\" and \n\"llseek\".\n\nBut there simply isn't any way to do mmap() or pread() portably outside \nthe 32-bit area. So there are good reasons why we should just limit \npack-files to 32-bits on 32-bit architectures.\n\nSo I think that Dana's approach is just fundamentally correct. Yeah, we \nshould probably have a 64-bit index as a *possibility*, but it simply \nisn't a replacement for \"keep packs under 2GB in size\".\n\n\t\tLinus\n"},{"id":"38715","messageId":"alpine.LFD.0.98.0704051147050.28181@xanadu.home","threadId":"7516","inReplyTo":"20070405071511.GE5436@spearce.org","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-04-05T15:52:05Z","receivedAt":"2007-04-05T15:52:05Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 5 Apr 2007, Shawn O. Pearce wrote:\n> Nicolas Pitre <nico@cam.org> wrote:\n> > On Wed, 4 Apr 2007, Dana How wrote:\n> > > I have a similar but much weaker reaction, but Linus specifically asked for\n> > > this combination to work.\n> > \n> > Linus is a total imcompetent who knows nothing about programming or good \n> > taste.  So never ever listen to what he says.  He is wrong, always.\n> \n> Now now, I wouldn't go that far...\n\nThere was a well meant smiley along with my diatribe of course.\n\n\nNicolas\n"},{"id":"38716","messageId":"20070405155306.GI5436@spearce.org","threadId":"7516","inReplyTo":"Pine.LNX.4.64.0704050831520.6730@woody.linux-foundation.org","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-05T15:53:06Z","receivedAt":"2007-04-05T15:53:06Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> wrote:\n> \n> On Thu, 5 Apr 2007, Shawn O. Pearce wrote:\n> > \n> > For glibc we do try to set _FILE_OFFSET_BITS to 64\n> \n> I repeat: that's _broken_.\n> \n> It's in no way portable. It's a glibc horror. It should not be used.\n> \n> It was a quick hack, but the real way to do it is to use \"loff_t\" and \n> \"llseek\".\n\nSure, OK, but that libc function doesn't exist on Mac OS X:\n\nman llseek:\n\n  This function is Linux-specific, and should not be used in programs\n  intended to be portable.\n\nSo we'd need our own horror to wrap llseek as an lseek fake-alike\nanyway.  That's what that glibc horror does, and we didn't have to\nwrite that code.  :-)\n \n> But there simply isn't any way to do mmap() or pread() portably outside \n> the 32-bit area. So there are good reasons why we should just limit \n> pack-files to 32-bits on 32-bit architectures.\n\nNot unless your off_t is 64 bits, no.  If it is 64 bits then you\nshould be able to do a pread or mmap starting past the first 2 GiB.\nYou just might not be able to ask for a mmap that exceeds 2 GiB in\nsize, as your size_t may not be that large.  E.g., Darwin/Mac OS X.\nHence the sliding window mmap.\n\n> So I think that Dana's approach is just fundamentally correct. Yeah, we \n> should probably have a 64-bit index as a *possibility*, but it simply \n> isn't a replacement for \"keep packs under 2GB in size\".\n\nI'm not disagreeing.  Some filesystems (FAT on a USB stick, Dana's\nexample) just don't want files larger than 2 GiB.  So keeping them\nsmall has a number of advantages.  Plus they are easier to burn on\nDVD: 2 packs per DVD.  ;-)\n\nI was simply trying to point out that the mmap code isn't broken\nif the off_t is able to handle a file of that size; and if it can't\nthen other things are broken, like a simple lseek.\n\n-- \nShawn.\n"},{"id":"38719","messageId":"Pine.LNX.4.64.0704050910590.6730@woody.linux-foundation.org","threadId":"7516","inReplyTo":"20070405155306.GI5436@spearce.org","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2007-04-05T16:21:33Z","receivedAt":"2007-04-05T16:21:33Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Thu, 5 Apr 2007, Shawn O. Pearce wrote:\n> \n> Sure, OK, but that libc function doesn't exist on Mac OS X:\n\nMy bad. It's *not* linux-specific like the OSX man-page apparently says, \nit's very traditional. But the right name is \"lseek64()\" (and offt64_t for \nthe size).\n\nOf course, OSX didn't have some of the backwards-compatibility issues with \ndecades ago, so they just made off_t 64-bit by default. Maybe they don't \neven bother to do the trivial portability things to support programs that \ntry to be portable..\n\nAnyway, we should use open64(), lseek64() and friends to be as portable as \npossible.. And if some system doesn't have them, just use the normal ops, \nand pray that they are already 64-bit safe..\n\n\n\t\tLinus\n"},{"id":"38722","messageId":"20070405171432.GK5436@spearce.org","threadId":"7516","inReplyTo":"Pine.LNX.4.64.0704050910590.6730@woody.linux-foundation.org","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2007-04-05T17:14:32Z","receivedAt":"2007-04-05T17:14:32Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Linus Torvalds <torvalds@linux-foundation.org> wrote:\n> \n> On Thu, 5 Apr 2007, Shawn O. Pearce wrote:\n> > \n> > Sure, OK, but that libc function doesn't exist on Mac OS X:\n> \n> My bad. It's *not* linux-specific like the OSX man-page apparently says, \n> it's very traditional. But the right name is \"lseek64()\" (and offt64_t for \n> the size).\n\nSorry for the confusion, that manpage snippet came from a Gentoo/x86\nsystem.  But I digress, you are right, the right interfaces to be\nusing here is lseek64 and open64.\n\nNow those also don't eixst on OSX, because as you pointed out, they\nhave no legacy to deal with and are just using sizeof off_t == 8.\n\nSo we'd probably have to do something like:\n\n\t#ifndef _LFS_LARGEFILE\n\t#define open64 open\n\t#define lseek64 lseek\n\t#endif\n\nand then start using the open64/lseek64 variants instead.  Or do\nthe reverse #define's.  ;-)\n\n-- \nShawn.\n"},{"id":"38734","messageId":"87r6qy7c6x.fsf@mid.deneb.enyo.de","threadId":"7516","inReplyTo":"20070405171432.GK5436@spearce.org","subject":"Re: [PATCH] git-{repack,pack-objects} accept --{pack,blob}-limit to control pack size","fromName":"Florian Weimer","fromEmail":"fw@deneb.enyo.de","sentAt":"2007-04-05T21:17:58Z","receivedAt":"2007-04-05T21:17:58Z","isPatch":true,"sender":{"key":"fw@deneb.enyo.de","avatar":null},"body":"* Shawn O. Pearce:\n\n> So we'd probably have to do something like:\n>\n> \t#ifndef _LFS_LARGEFILE\n> \t#define open64 open\n> \t#define lseek64 lseek\n> \t#endif\n>\n> and then start using the open64/lseek64 variants instead.  Or do\n> the reverse #define's.  ;-)\n\nThis is actually what \"#define _FILE_OFFSET_BITS 64\" does.  It's\nusually not a bad idea per se, but you must not use off_t in library\nheader files if you do this.\n\nlseek64 and friends are under -D_LARGEFILE64_SOURCE, it seems.  Pitty\nwe couldn't get rid of this mess when switching to libc6. 8-(\n"}]}