{"thread":{"id":"8309","subject":"[PATCH] Enhance unpack-objects for extracting large objects","startedAt":"2007-05-25T08:20:07Z","lastAt":"2007-05-25T20:05:37Z","messageCount":7,"participants":["Dana How","Nicolas Pitre","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"43219","messageId":"46569C37.5000201@gmail.com","threadId":"8309","inReplyTo":null,"subject":"[PATCH] Enhance unpack-objects for extracting large objects","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-05-25T08:20:07Z","receivedAt":"2007-05-25T08:20:07Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"\nNicolas Pitre wrote:\n> I wouldn't mind a _separate_ tool that would load a pack index,\n> determine object sizes from it, and then extract big objects to write\n> them as loose objects ...\n\nBelow we add two new options to git-unpack-objects:\n\n--min-blob-size=<n>::  Unpacking is only done for objects\nlarger than or equal to n kB (uncompressed size by Junio).\n\n--force::  Loose objects will be created even if they\nalready exist in the repository packed.  This is an option\nI've wanted before for other reasons.\n\nThis passes the tests in \"t\" but has not yet been used on my large repos.\nBased on \"next\" but should apply to \"master\" as well.\n\nSigned-off-by: Dana L. How <danahow@gmail.com>\n---\n Documentation/git-unpack-objects.txt |   17 +++++++++++++----\n builtin-unpack-objects.c             |   20 ++++++++++++++++++--\n cache.h                              |    2 ++\n sha1_file.c                          |   11 +++++++++--\n 4 files changed, 42 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\nindex ff6184b..4513d8d 100644\n--- a/Documentation/git-unpack-objects.txt\n+++ b/Documentation/git-unpack-objects.txt\n@@ -8,7 +8,7 @@ git-unpack-objects - Unpack objects from a packed archive\n \n SYNOPSIS\n --------\n-'git-unpack-objects' [-n] [-q] [-r] <pack-file\n+'git-unpack-objects' [-n] [-q] [-r] [--force] [--min-blob-size=N] <pack-file\n \n \n DESCRIPTION\n@@ -17,9 +17,10 @@ Read a packed archive (.pack) from the standard input, expanding\n the objects contained within and writing them into the repository in\n \"loose\" (one object per file) format.\n \n-Objects that already exist in the repository will *not* be unpacked\n-from the pack-file.  Therefore, nothing will be unpacked if you use\n-this command on a pack-file that exists within the target repository.\n+By default,  objects that already exist in the repository will *not*\n+be unpacked from the pack-file.  Therefore, nothing will be unpacked\n+if you use this command on a pack-file that exists within the target\n+repository,  unless you specify --force.\n \n Please see the `git-repack` documentation for options to generate\n new packs and replace existing ones.\n@@ -40,6 +41,14 @@ OPTIONS\n \tand make the best effort to recover as many objects as\n \tpossible.\n \n+--force::\n+\tAllow loose objects to be created in the same repository that\n+\tcontains the packfile.\n+\n+--min-blob-size=<n>::\n+\tSmallest loose object to create,  expressed in kB.\n+\tBlobs smaller than this will not be unpacked.  Default is 0.\n+\n \n Author\n ------\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex a6ff62f..a42bf0d 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -10,13 +10,16 @@\n #include \"progress.h\"\n \n static int dry_run, quiet, recover, has_errors;\n-static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n+static const char unpack_usage[] =\n+\"git-unpack-objects [-n] [-q] [-r] [--force] [--min-blob-size=N] < pack-file\";\n \n /* We always read in 4kB chunks. */\n static unsigned char buffer[4096];\n static unsigned int offset, len;\n static off_t consumed_bytes;\n static SHA_CTX ctx;\n+static int force = 0;\n+uint32_t min_blob_size;\n \n /*\n  * Make sure at least \"min\" bytes are available in the buffer, and\n@@ -131,7 +134,9 @@ static void added_object(unsigned nr, enum object_type type,\n static void write_object(unsigned nr, enum object_type type,\n \t\t\t void *buf, unsigned long size)\n {\n-\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n+\tint force2 = size < min_blob_size ? -1 : force;\n+\tif (write_sha1_file_maybe(buf, size, typename(type),\n+\t\t\t\t  force2, obj_list[nr].sha1) < 0)\n \t\tdie(\"failed to write object\");\n \tadded_object(nr, type, buf, size);\n }\n@@ -361,6 +366,17 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\trecover = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n+\t\t\tif (!strcmp(arg, \"--force\")) {\n+\t\t\t\tforce = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!prefixcmp(arg, \"--min-blob-size=\")) {\n+\t\t\t\tchar *end;\n+\t\t\t\tmin_blob_size = strtoul(arg+16, &end, 0) * 1024;\n+\t\t\t\tif (!arg[16] || *end)\n+\t\t\t\t\tusage(unpack_usage);\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tif (!prefixcmp(arg, \"--pack_header=\")) {\n \t\t\t\tstruct pack_header *hdr;\n \t\t\t\tchar *c;\ndiff --git a/cache.h b/cache.h\nindex ec85d93..d0c3030 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -343,6 +343,8 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);\n extern void * read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);\n extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n extern int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n+extern int write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n+\t\t\t\t int ignore, unsigned char *return_sha1);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n \n extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 12d2ef2..68b8db8 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -1979,7 +1979,8 @@ int hash_sha1_file(const void *buf, unsigned long len, const char *type,\n \treturn 0;\n }\n \n-int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n+int write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n+\t\t\t  int ignore, unsigned char *returnsha1)\n {\n \tint size, ret;\n \tunsigned char *compressed;\n@@ -1997,7 +1998,7 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \tfilename = sha1_file_name(sha1);\n \tif (returnsha1)\n \t\thashcpy(returnsha1, sha1);\n-\tif (has_sha1_file(sha1))\n+\tif (ignore < 0 || !ignore && has_sha1_file(sha1))\n \t\treturn 0;\n \tfd = open(filename, O_RDONLY);\n \tif (fd >= 0) {\n@@ -2062,6 +2063,12 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n \treturn move_temp_to_file(tmpfile, filename);\n }\n \n+int write_sha1_file(void *buf, unsigned long len, const char *type,\n+\t\t    unsigned char *returnsha1)\n+{\n+\treturn write_sha1_file_maybe(buf, len, type, 0, returnsha1);\n+}\n+\n /*\n  * We need to unpack and recompress the object for writing\n  * it out to a different file.\n-- \n1.5.2.762.gd8c6-dirty\n"},{"id":"43247","messageId":"alpine.LFD.0.99.0705250930450.3366@xanadu.home","threadId":"8309","inReplyTo":"46569C37.5000201@gmail.com","subject":"Re: [PATCH] Enhance unpack-objects for extracting large objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-05-25T13:41:42Z","receivedAt":"2007-05-25T13:41:42Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 25 May 2007, Dana How wrote:\n\n> \n> Nicolas Pitre wrote:\n> > I wouldn't mind a _separate_ tool that would load a pack index,\n> > determine object sizes from it, and then extract big objects to write\n> > them as loose objects ...\n> \n> Below we add two new options to git-unpack-objects:\n> \n> --min-blob-size=<n>::  Unpacking is only done for objects\n> larger than or equal to n kB (uncompressed size by Junio).\n> \n> --force::  Loose objects will be created even if they\n> already exist in the repository packed.  This is an option\n> I've wanted before for other reasons.\n> \n> This passes the tests in \"t\" but has not yet been used on my large repos.\n> Based on \"next\" but should apply to \"master\" as well.\n> \n> Signed-off-by: Dana L. How <danahow@gmail.com>\n\nThis is clever, and the --force option is a nice thing to have.  Not \nthat I find it particularly useful (I'd personally pack large objects \ntogether rather than keeping them loose), but at least having the option \nto explode any pack even in a live repository is a good thing to have at \nthe plumbing level given the simplicity of the patch.\n\nACK.\n\n\n> ---\n>  Documentation/git-unpack-objects.txt |   17 +++++++++++++----\n>  builtin-unpack-objects.c             |   20 ++++++++++++++++++--\n>  cache.h                              |    2 ++\n>  sha1_file.c                          |   11 +++++++++--\n>  4 files changed, 42 insertions(+), 8 deletions(-)\n> \n> diff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\n> index ff6184b..4513d8d 100644\n> --- a/Documentation/git-unpack-objects.txt\n> +++ b/Documentation/git-unpack-objects.txt\n> @@ -8,7 +8,7 @@ git-unpack-objects - Unpack objects from a packed archive\n>  \n>  SYNOPSIS\n>  --------\n> -'git-unpack-objects' [-n] [-q] [-r] <pack-file\n> +'git-unpack-objects' [-n] [-q] [-r] [--force] [--min-blob-size=N] <pack-file\n>  \n>  \n>  DESCRIPTION\n> @@ -17,9 +17,10 @@ Read a packed archive (.pack) from the standard input, expanding\n>  the objects contained within and writing them into the repository in\n>  \"loose\" (one object per file) format.\n>  \n> -Objects that already exist in the repository will *not* be unpacked\n> -from the pack-file.  Therefore, nothing will be unpacked if you use\n> -this command on a pack-file that exists within the target repository.\n> +By default,  objects that already exist in the repository will *not*\n> +be unpacked from the pack-file.  Therefore, nothing will be unpacked\n> +if you use this command on a pack-file that exists within the target\n> +repository,  unless you specify --force.\n>  \n>  Please see the `git-repack` documentation for options to generate\n>  new packs and replace existing ones.\n> @@ -40,6 +41,14 @@ OPTIONS\n>  \tand make the best effort to recover as many objects as\n>  \tpossible.\n>  \n> +--force::\n> +\tAllow loose objects to be created in the same repository that\n> +\tcontains the packfile.\n> +\n> +--min-blob-size=<n>::\n> +\tSmallest loose object to create,  expressed in kB.\n> +\tBlobs smaller than this will not be unpacked.  Default is 0.\n> +\n>  \n>  Author\n>  ------\n> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\n> index a6ff62f..a42bf0d 100644\n> --- a/builtin-unpack-objects.c\n> +++ b/builtin-unpack-objects.c\n> @@ -10,13 +10,16 @@\n>  #include \"progress.h\"\n>  \n>  static int dry_run, quiet, recover, has_errors;\n> -static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n> +static const char unpack_usage[] =\n> +\"git-unpack-objects [-n] [-q] [-r] [--force] [--min-blob-size=N] < pack-file\";\n>  \n>  /* We always read in 4kB chunks. */\n>  static unsigned char buffer[4096];\n>  static unsigned int offset, len;\n>  static off_t consumed_bytes;\n>  static SHA_CTX ctx;\n> +static int force = 0;\n> +uint32_t min_blob_size;\n>  \n>  /*\n>   * Make sure at least \"min\" bytes are available in the buffer, and\n> @@ -131,7 +134,9 @@ static void added_object(unsigned nr, enum object_type type,\n>  static void write_object(unsigned nr, enum object_type type,\n>  \t\t\t void *buf, unsigned long size)\n>  {\n> -\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n> +\tint force2 = size < min_blob_size ? -1 : force;\n> +\tif (write_sha1_file_maybe(buf, size, typename(type),\n> +\t\t\t\t  force2, obj_list[nr].sha1) < 0)\n>  \t\tdie(\"failed to write object\");\n>  \tadded_object(nr, type, buf, size);\n>  }\n> @@ -361,6 +366,17 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n>  \t\t\t\trecover = 1;\n>  \t\t\t\tcontinue;\n>  \t\t\t}\n> +\t\t\tif (!strcmp(arg, \"--force\")) {\n> +\t\t\t\tforce = 1;\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n> +\t\t\tif (!prefixcmp(arg, \"--min-blob-size=\")) {\n> +\t\t\t\tchar *end;\n> +\t\t\t\tmin_blob_size = strtoul(arg+16, &end, 0) * 1024;\n> +\t\t\t\tif (!arg[16] || *end)\n> +\t\t\t\t\tusage(unpack_usage);\n> +\t\t\t\tcontinue;\n> +\t\t\t}\n>  \t\t\tif (!prefixcmp(arg, \"--pack_header=\")) {\n>  \t\t\t\tstruct pack_header *hdr;\n>  \t\t\t\tchar *c;\n> diff --git a/cache.h b/cache.h\n> index ec85d93..d0c3030 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -343,6 +343,8 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);\n>  extern void * read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);\n>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n>  extern int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n> +extern int write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n> +\t\t\t\t int ignore, unsigned char *return_sha1);\n>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n>  \n>  extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 12d2ef2..68b8db8 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1979,7 +1979,8 @@ int hash_sha1_file(const void *buf, unsigned long len, const char *type,\n>  \treturn 0;\n>  }\n>  \n> -int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n> +int write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n> +\t\t\t  int ignore, unsigned char *returnsha1)\n>  {\n>  \tint size, ret;\n>  \tunsigned char *compressed;\n> @@ -1997,7 +1998,7 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n>  \tfilename = sha1_file_name(sha1);\n>  \tif (returnsha1)\n>  \t\thashcpy(returnsha1, sha1);\n> -\tif (has_sha1_file(sha1))\n> +\tif (ignore < 0 || !ignore && has_sha1_file(sha1))\n>  \t\treturn 0;\n>  \tfd = open(filename, O_RDONLY);\n>  \tif (fd >= 0) {\n> @@ -2062,6 +2063,12 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n>  \treturn move_temp_to_file(tmpfile, filename);\n>  }\n>  \n> +int write_sha1_file(void *buf, unsigned long len, const char *type,\n> +\t\t    unsigned char *returnsha1)\n> +{\n> +\treturn write_sha1_file_maybe(buf, len, type, 0, returnsha1);\n> +}\n> +\n>  /*\n>   * We need to unpack and recompress the object for writing\n>   * it out to a different file.\n> -- \n> 1.5.2.762.gd8c6-dirty\n> \n> -\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n> \n\n\nNicolas\n"},{"id":"43275","messageId":"7vsl9kr9mz.fsf@assigned-by-dhcp.cox.net","threadId":"8309","inReplyTo":"46569C37.5000201@gmail.com","subject":"Re: [PATCH] Enhance unpack-objects for extracting large objects","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-25T19:22:28Z","receivedAt":"2007-05-25T19:22:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dana How <danahow@gmail.com> writes:\n\n> Nicolas Pitre wrote:\n>> I wouldn't mind a _separate_ tool that would load a pack index,\n>> determine object sizes from it, and then extract big objects to write\n>> them as loose objects ...\n>\n> Below we add two new options to git-unpack-objects:\n>\n> --min-blob-size=<n>::  Unpacking is only done for objects\n> larger than or equal to n kB (uncompressed size by Junio).\n\nElsewhere you wanted to use --max-* and that was counted in megs;\nisn't using kilo here and meg there inconsistent?\n\n> --force::  Loose objects will be created even if they\n> already exist in the repository packed.  This is an option\n> I've wanted before for other reasons.\n\n        ... but if they already exist in the repository as loose\n        objects, do not replace it.\n\nUsually we do not overwrite existing loose objects and it is one\nof the security measure --- if you have an object already, that\ncannot be touched by somebody who maliciously creats a hash\ncolliding loose object and tries to inject it into your\nrepository via unpack-objects.  It's good that you kept this\nbehaviour intact.\n\n> This passes the tests in \"t\" but has not yet been used on my large repos.\n> Based on \"next\" but should apply to \"master\" as well.\n>\n> Signed-off-by: Dana L. How <danahow@gmail.com>\n> ---\n>  Documentation/git-unpack-objects.txt |   17 +++++++++++++----\n>  builtin-unpack-objects.c             |   20 ++++++++++++++++++--\n>  cache.h                              |    2 ++\n>  sha1_file.c                          |   11 +++++++++--\n>  4 files changed, 42 insertions(+), 8 deletions(-)\n>\n> diff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\n> index ff6184b..4513d8d 100644\n> --- a/Documentation/git-unpack-objects.txt\n> +++ b/Documentation/git-unpack-objects.txt\n> ...\n> @@ -17,9 +17,10 @@ Read a packed archive (.pack) from the standard input, expanding\n>  the objects contained within and writing them into the repository in\n>  \"loose\" (one object per file) format.\n>  \n> -Objects that already exist in the repository will *not* be unpacked\n> -from the pack-file.  Therefore, nothing will be unpacked if you use\n> -this command on a pack-file that exists within the target repository.\n> +By default,  objects that already exist in the repository will *not*\n> +be unpacked from the pack-file.  Therefore, nothing will be unpacked\n> +if you use this command on a pack-file that exists within the target\n> +repository,  unless you specify --force.\n\nI would want to add:\n\n\tIf an object already exists unpacked in the repository,\n\tit will not be replaced with the copy from the pack,\n\twith or without `--force`.\n\n> diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\n> index a6ff62f..a42bf0d 100644\n> --- a/builtin-unpack-objects.c\n> +++ b/builtin-unpack-objects.c\n> @@ -10,13 +10,16 @@\n>  #include \"progress.h\"\n>  \n>  static int dry_run, quiet, recover, has_errors;\n> -static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n> +static const char unpack_usage[] =\n> +\"git-unpack-objects [-n] [-q] [-r] [--force] [--min-blob-size=N] < pack-file\";\n\nMaybe we would want to call it '-f' for consistency.  Another\npossibility is the other way around, giving others a longer\nsynonyms, like --quiet, but this command is plumbing and I do\nnot think long options matters that much, so my preference is to\ndo '-f' not '--force'.\n\n> @@ -131,7 +134,9 @@ static void added_object(unsigned nr, enum object_type type,\n>  static void write_object(unsigned nr, enum object_type type,\n>  \t\t\t void *buf, unsigned long size)\n>  {\n> -\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n> +\tint force2 = size < min_blob_size ? -1 : force;\n> +\tif (write_sha1_file_maybe(buf, size, typename(type),\n> +\t\t\t\t  force2, obj_list[nr].sha1) < 0)\n>  \t\tdie(\"failed to write object\");\n>  \tadded_object(nr, type, buf, size);\n>  }\n\nWithout --min-blob-size option, min_blob_size is initialized to\n0u and force2 always gets the value of force.  With the option,\nblobs smaller than the threshold gets -1 and otherwise the value\nof force.\n\n\"write_sha1_file_maybe()\" can take 0, 1, or -1 as its fourth\nparameter.  The reader is left puzzled what the distinction\namong these three and decides to read on to figure it out before\ncomplaining too much about the code, but no matter what it does,\ndoesn't the above logic already feel wrong?\n\n * You already have the size here, so if min_blob_size is set\n   and the size is larger, you do not even have to call\n   write_sha1_file() at all.\n\n * If you do so, write_sha1_file_maybe()'s additional parameter\n   can be \"skip the check to see if we have one packed\".\n\n> diff --git a/cache.h b/cache.h\n> index ec85d93..d0c3030 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -343,6 +343,8 @@ extern int sha1_object_info(const unsigned char *, unsigned long *);\n>  extern void * read_sha1_file(const unsigned char *sha1, enum object_type *type, unsigned long *size);\n>  extern int hash_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *sha1);\n>  extern int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n> +extern int write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n> +\t\t\t\t int ignore, unsigned char *return_sha1);\n>  extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n>  \n>  extern int check_sha1_signature(const unsigned char *sha1, void *buf, unsigned long size, const char *type);\n\n... and it says \"ignore\".  The reader is still puzzled and reads on...\n\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 12d2ef2..68b8db8 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -1979,7 +1979,8 @@ int hash_sha1_file(const void *buf, unsigned long len, const char *type,\n>  \treturn 0;\n>  }\n>  \n> -int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned char *returnsha1)\n> +int write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n> +\t\t\t  int ignore, unsigned char *returnsha1)\n>  {\n>  \tint size, ret;\n>  \tunsigned char *compressed;\n> @@ -1997,7 +1998,7 @@ int write_sha1_file(void *buf, unsigned long len, const char *type, unsigned cha\n>  \tfilename = sha1_file_name(sha1);\n>  \tif (returnsha1)\n>  \t\thashcpy(returnsha1, sha1);\n> -\tif (has_sha1_file(sha1))\n> +\tif (ignore < 0 || !ignore && has_sha1_file(sha1))\n>  \t\treturn 0;\n>  \tfd = open(filename, O_RDONLY);\n>  \tif (fd >= 0) {\n\nSo \"ignore\" means:\n\n        negative:       never write it out, even if it does not exist.\n\n        zero:           do not write it out if it is available (in pack,\n                        or loose, either local or alternate), do\n                        write it out otherwise; it is the same\n                        as the current behaviour of write_sha1_file().\n\n        positive:       always write it out.\n\nThat does not sound like \"ignore\".\n\nMy suggestion would be:\n\n>  static void write_object(unsigned nr, enum object_type type,\n>  \t\t\t void *buf, unsigned long size)\n>  {\n        if (!min_blob_size || size < min_blob_size) {\n             if (write_sha1_file_maybe(buf, size, typename(type),\n                             force, obj_list[nr].sha1) < 0)\n                     die(\"failed to write object\");\n             }\n        }\n \tadded_object(nr, type, buf, size);\n>  }\n\nAnd then.\n\nint write_sha1_file_maybe(void *buf, unsigned long len, const char *type,\n\t\tint make_loose, unsigned char *returnsha1)\n{\n\t...\n>  \tfilename = sha1_file_name(sha1);\n>  \tif (returnsha1)\n>  \t\thashcpy(returnsha1, sha1);\n\tif (!make_loose && has_sha1_file(sha1))\n>  \t\treturn 0;\n>  \tfd = open(filename, O_RDONLY);\n>  \tif (fd >= 0) {\n"},{"id":"43277","messageId":"56b7f5510705251249u74b754f1y4f8cafd5f5c35f19@mail.gmail.com","threadId":"8309","inReplyTo":"7vsl9kr9mz.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Enhance unpack-objects for extracting large objects","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-05-25T19:49:08Z","receivedAt":"2007-05-25T19:49:08Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 5/25/07, Junio C Hamano <junkio@cox.net> wrote:\n> Dana How <danahow@gmail.com> writes:\n> > Nicolas Pitre wrote:\n> >> I wouldn't mind a _separate_ tool that would load a pack index,\n> >> determine object sizes from it, and then extract big objects to write\n> >> them as loose objects ...\n> >\n> > Below we add two new options to git-unpack-objects:\n> >\n> > --min-blob-size=<n>::  Unpacking is only done for objects\n> > larger than or equal to n kB (uncompressed size by Junio).\n>\n> Elsewhere you wanted to use --max-* and that was counted in megs;\n> isn't using kilo here and meg there inconsistent?\nFor git-repack --max-pack-size=N I used MB to be consistent\nwith git-fast-import.  I think it makes sense to use MB everywhere\nwe talk about packfile size.\n\nFor the old degunking patch's -max-blob-size=N ,  and this patch's\n-min-blob-size=N ,  I was using KB to describe blob size.\nIt looked like I needed finer granularity,  at least for experiments.\n\nI think if we always use MB for packfile sizes, and KB\nfor blob sizes,  we should be OK.\n\n> > --force::  Loose objects will be created even if they\n> > already exist in the repository packed.  This is an option\n> > I've wanted before for other reasons.\n>\n>         ... but if they already exist in the repository as loose\n>         objects, do not replace it.\n>\n> Usually we do not overwrite existing loose objects and it is one\n> of the security measure --- if you have an object already, that\n> cannot be touched by somebody who maliciously creats a hash\n> colliding loose object and tries to inject it into your\n> repository via unpack-objects.  It's good that you kept this\n> behaviour intact.\nI agree.\nI'll add the \"... but\" part to the corrected patch's commit msg.\n\n> > -Objects that already exist in the repository will *not* be unpacked\n> > -from the pack-file.  Therefore, nothing will be unpacked if you use\n> > -this command on a pack-file that exists within the target repository.\n> > +By default,  objects that already exist in the repository will *not*\n> > +be unpacked from the pack-file.  Therefore, nothing will be unpacked\n> > +if you use this command on a pack-file that exists within the target\n> > +repository,  unless you specify --force.\n> I would want to add:\n>         If an object already exists unpacked in the repository,\n>         it will not be replaced with the copy from the pack,\n>         with or without `--force`.\nOK, that's clearer.\n\n> > -static const char unpack_usage[] = \"git-unpack-objects [-n] [-q] [-r] < pack-file\";\n> > +static const char unpack_usage[] =\n> > +\"git-unpack-objects [-n] [-q] [-r] [--force] [--min-blob-size=N] < pack-file\";\n>\n> Maybe we would want to call it '-f' for consistency.  Another\n> possibility is the other way around, giving others a longer\n> synonyms, like --quiet, but this command is plumbing and I do\n> not think long options matters that much, so my preference is to\n> do '-f' not '--force'.\nI picked the longer one only because I didn't view it as frequently used.\nBut it will be more used than min-blob-size.  I'll change it to -f.\n\n>  * You already have the size here, so if min_blob_size is set\n>    and the size is larger, you do not even have to call\n>    write_sha1_file() at all.\nThe way I read the code,  it looks like unpack-objects needs\nthe last argument always to be initialized with the SHA-1 computed\nfrom the object contents.  Therefore I always need to call\nwrite_sha1_file(),  even if I don't want it to write anything.\n\n> So \"ignore\" means:\n>         negative:       never write it out, even if it does not exist.\n>         zero:           do not write it out if it is available (in pack,\n>                         or loose, either local or alternate), do\n>                         write it out otherwise; it is the same\n>                         as the current behaviour of write_sha1_file().\n>         positive:       always write it out.\n> That does not sound like \"ignore\".\nI agree \"ignore\" is confusing;\nI will change it to \"when\",  which is more consistent with the\n_maybe prefix on the function name.\n\n> My suggestion would be:\nI like this suggestion,  but since I need to call the function\nto get the SHA-1,  I don't think I can follow it.\n\nI'll send you an updated patch in a moment.\n\nThanks,\n-- \nDana L. How  danahow@gmail.com  +1 650 804 5991 cell\n"},{"id":"43278","messageId":"alpine.LFD.0.99.0705251546260.3366@xanadu.home","threadId":"8309","inReplyTo":"7vsl9kr9mz.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Enhance unpack-objects for extracting large objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-05-25T19:52:48Z","receivedAt":"2007-05-25T19:52:48Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 25 May 2007, Junio C Hamano wrote:\n\n> Maybe we would want to call it '-f' for consistency.  Another\n> possibility is the other way around, giving others a longer\n> synonyms, like --quiet, but this command is plumbing and I do\n> not think long options matters that much, so my preference is to\n> do '-f' not '--force'.\n\nOTOH, I like to have long options for weird or obscur parameters.  Their \naction is less likely to be presumed by casual inspection of a script \nusing them.  I don't feel strongly about it either ways though.\n\n> > @@ -131,7 +134,9 @@ static void added_object(unsigned nr, enum object_type type,\n> >  static void write_object(unsigned nr, enum object_type type,\n> >  \t\t\t void *buf, unsigned long size)\n> >  {\n> > -\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n> > +\tint force2 = size < min_blob_size ? -1 : force;\n> > +\tif (write_sha1_file_maybe(buf, size, typename(type),\n> > +\t\t\t\t  force2, obj_list[nr].sha1) < 0)\n> >  \t\tdie(\"failed to write object\");\n> >  \tadded_object(nr, type, buf, size);\n> >  }\n> \n> Without --min-blob-size option, min_blob_size is initialized to\n> 0u and force2 always gets the value of force.  With the option,\n> blobs smaller than the threshold gets -1 and otherwise the value\n> of force.\n> \n> \"write_sha1_file_maybe()\" can take 0, 1, or -1 as its fourth\n> parameter.  The reader is left puzzled what the distinction\n> among these three and decides to read on to figure it out before\n> complaining too much about the code, but no matter what it does,\n> doesn't the above logic already feel wrong?\n> \n>  * You already have the size here, so if min_blob_size is set\n>    and the size is larger, you do not even have to call\n>    write_sha1_file() at all.\n\nYou still do to get the object's SHA1.\n\n\nNicolas\n"},{"id":"43280","messageId":"7viragr7xb.fsf@assigned-by-dhcp.cox.net","threadId":"8309","inReplyTo":"56b7f5510705251249u74b754f1y4f8cafd5f5c35f19@mail.gmail.com","subject":"Re: [PATCH] Enhance unpack-objects for extracting large objects","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2007-05-25T19:59:28Z","receivedAt":"2007-05-25T19:59:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Dana How\" <danahow@gmail.com> writes:\n\n>>  * You already have the size here, so if min_blob_size is set\n>>    and the size is larger, you do not even have to call\n>>    write_sha1_file() at all.\n> The way I read the code,  it looks like unpack-objects needs\n> the last argument always to be initialized with the SHA-1 computed\n> from the object contents.  Therefore I always need to call\n> write_sha1_file(),  even if I don't want it to write anything.\n\nAh, that is what I missed.\n\nThere is a separate function to only hash, named (surprisingly)\n\"hash_sha1_file().  Maybe you can teach the caller's \"don't\nwrite it out\" codepath to call it.\n"},{"id":"43281","messageId":"alpine.LFD.0.99.0705251605090.3366@xanadu.home","threadId":"8309","inReplyTo":"7viragr7xb.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] Enhance unpack-objects for extracting large objects","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-05-25T20:05:37Z","receivedAt":"2007-05-25T20:05:37Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Fri, 25 May 2007, Junio C Hamano wrote:\n\n> \"Dana How\" <danahow@gmail.com> writes:\n> \n> >>  * You already have the size here, so if min_blob_size is set\n> >>    and the size is larger, you do not even have to call\n> >>    write_sha1_file() at all.\n> > The way I read the code,  it looks like unpack-objects needs\n> > the last argument always to be initialized with the SHA-1 computed\n> > from the object contents.  Therefore I always need to call\n> > write_sha1_file(),  even if I don't want it to write anything.\n> \n> Ah, that is what I missed.\n> \n> There is a separate function to only hash, named (surprisingly)\n> \"hash_sha1_file().  Maybe you can teach the caller's \"don't\n> write it out\" codepath to call it.\n\nThat would be clearer indeed.\n\n\nNicolas\n"}]}