{"thread":{"id":"43170","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","startedAt":"2006-10-31T07:57:04Z","lastAt":"2006-10-31T22:27:36Z","messageCount":7,"participants":["Nicolas Pitre","Junio C Hamano","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"296628","messageId":"20061031075704.GB7691@spearce.org","threadId":"43170","inReplyTo":null,"subject":"[PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-31T07:57:04Z","receivedAt":"2006-10-31T07:57:04Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Since keeping a pushed pack or exploding it into loose objects\nshould be a local repository decision this teaches receive-pack\nto decide if it should call unpack-objects or index-pack --stdin\n--fix-thin based on the setting of receive.unpackLimit and the\nnumber of objects contained in the received pack.\n\nIf the number of objects (hdr_entries) in the received pack is\nbelow the value of receive.unpackLimit (which is 5000 by default)\nthen we unpack-objects as we have in the past.\n\nIf the hdr_entries >= receive.unpackLimit then we call index-pack and\nask it to include our pid and hostname in the .keep file to make it\neasier to identify why a given pack has been kept in the repository.\n\nCurrently this leaves every received pack as a kept pack.  We really\ndon't want that as received packs will tend to be small.  Instead we\nwant to delete the .keep file automatically after all refs have\nbeen updated.  That is being left as room for future improvement.\n\nSigned-off-by: Shawn O. Pearce <spearce@spearce.org>\n---\n Documentation/config.txt |   11 +++++++-\n cache.h                  |    1 +\n receive-pack.c           |   66 +++++++++++++++++++++++++++++++++++++++++++--\n sha1_file.c              |    2 +-\n 4 files changed, 75 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex d9e73da..9d3c71c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -301,7 +301,16 @@ imap::\n \tThe configuration variables in the 'imap' section are described\n \tin gitlink:git-imap-send[1].\n \n-receive.denyNonFastforwads::\n+receive.unpackLimit::\n+\tIf the number of objects received in a push is below this\n+\tlimit then the objects will be unpacked into loose object\n+\tfiles. However if the number of received objects equals or\n+\texceeds this limit then the received pack will be stored as\n+\ta pack, after adding any missing delta bases.  Storing the\n+\tpack from a push can make the push operation complete faster,\n+\tespecially on slow filesystems.\n+\n+receive.denyNonFastForwards::\n \tIf set to true, git-receive-pack will deny a ref update which is\n \tnot a fast forward. Use this to prevent such an update via a push,\n \teven if that push is forced. This configuration variable is\ndiff --git a/cache.h b/cache.h\nindex e997a85..6cb7e1d 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -376,6 +376,7 @@ extern struct packed_git *parse_pack_ind\n \t\t\t\t\t\tchar *idx_path);\n \n extern void prepare_packed_git(void);\n+extern void reprepare_packed_git(void);\n extern void install_packed_git(struct packed_git *pack);\n \n extern struct packed_git *find_sha1_pack(const unsigned char *sha1, \ndiff --git a/receive-pack.c b/receive-pack.c\nindex 7e154c5..b394833 100644\n--- a/receive-pack.c\n+++ b/receive-pack.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"pack.h\"\n #include \"refs.h\"\n #include \"pkt-line.h\"\n #include \"run-command.h\"\n@@ -7,9 +8,8 @@\n \n static const char receive_pack_usage[] = \"git-receive-pack <git-dir>\";\n \n-static const char *unpacker[] = { \"unpack-objects\", NULL };\n-\n static int deny_non_fast_forwards = 0;\n+static int unpack_limit = 5000;\n static int report_status;\n \n static char capabilities[] = \"report-status\";\n@@ -25,6 +25,12 @@ static int receive_pack_config(const cha\n \t\treturn 0;\n \t}\n \n+\tif (strcmp(var, \"receive.unpacklimit\") == 0)\n+\t{\n+\t\tunpack_limit = git_config_int(var, value);\n+\t\treturn 0;\n+\t}\n+\n \treturn 0;\n }\n \n@@ -227,9 +233,63 @@ static void read_head_info(void)\n \t}\n }\n \n+static const char *parse_pack_header(struct pack_header *hdr)\n+{\n+\tchar *c = (char*)hdr;\n+\tssize_t remaining = sizeof(struct pack_header);\n+\tdo {\n+\t\tssize_t r = xread(0, c, remaining);\n+\t\tif (r <= 0)\n+\t\t\treturn \"eof before pack header was fully read\";\n+\t\tremaining -= r;\n+\t\tc += r;\n+\t} while (remaining > 0);\n+\tif (hdr->hdr_signature != htonl(PACK_SIGNATURE))\n+\t\treturn \"protocol error (pack signature mismatch detected)\";\n+\tif (!pack_version_ok(hdr->hdr_version))\n+\t\treturn \"protocol error (pack version unsupported)\";\n+\treturn NULL;\n+}\n+\n static const char *unpack()\n {\n-\tint code = run_command_v_opt(1, unpacker, RUN_GIT_CMD);\n+\tstruct pack_header hdr;\n+\tconst char *hdr_err;\n+\tchar hdr_arg[38];\n+\tint code;\n+\n+\thdr_err = parse_pack_header(&hdr);\n+\tif (hdr_err)\n+\t\treturn hdr_err;\n+\tsnprintf(hdr_arg, sizeof(hdr_arg), \"--pack_header=%u,%u\",\n+\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n+\n+\tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n+\t\tconst char *unpacker[3];\n+\t\tunpacker[0] = \"unpack-objects\";\n+\t\tunpacker[1] = hdr_arg;\n+\t\tunpacker[2] = NULL;\n+\t\tcode = run_command_v_opt(1, unpacker, RUN_GIT_CMD);\n+\t} else {\n+\t\tconst char *keeper[6];\n+\t\tchar my_host[255], keep_arg[128 + 255];\n+\n+\t\tif (gethostname(my_host, sizeof(my_host)))\n+\t\t\tstrcpy(my_host, \"localhost\");\n+\t\tsnprintf(keep_arg, sizeof(keep_arg),\n+\t\t\t\"--keep=receive-pack %i on %s\",\n+\t\t\tgetpid(), my_host);\n+\n+\t\tkeeper[0] = \"index-pack\";\n+\t\tkeeper[1] = \"--stdin\";\n+\t\tkeeper[2] = \"--fix-thin\";\n+\t\tkeeper[3] = hdr_arg;\n+\t\tkeeper[4] = keep_arg;\n+\t\tkeeper[5] = NULL;\n+\t\tcode = run_command_v_opt(1, keeper, RUN_GIT_CMD);\n+\t\tif (!code)\n+\t\t\treprepare_packed_git();\n+\t}\n \n \tswitch (code) {\n \tcase 0:\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 5e6c8b8..7bda2d4 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -663,7 +663,7 @@ void prepare_packed_git(void)\n \tprepare_packed_git_run_once = 1;\n }\n \n-static void reprepare_packed_git(void)\n+void reprepare_packed_git(void)\n {\n \tprepare_packed_git_run_once = 0;\n \tprepare_packed_git();\n-- \n1.4.3.3.g7d63\n"},{"id":"295208","messageId":"Pine.LNX.4.64.0610311447250.11384@xanadu.home","threadId":"43170","inReplyTo":"20061031075704.GB7691@spearce.org","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-10-31T19:56:50Z","receivedAt":"2006-10-31T19:56:50Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 31 Oct 2006, Shawn Pearce wrote:\n\n> Since keeping a pushed pack or exploding it into loose objects\n> should be a local repository decision this teaches receive-pack\n> to decide if it should call unpack-objects or index-pack --stdin\n> --fix-thin based on the setting of receive.unpackLimit and the\n> number of objects contained in the received pack.\n\nThis works fine when used with my replacement patch for your [1/2] one.\n\n> Currently this leaves every received pack as a kept pack.  We really\n> don't want that as received packs will tend to be small.  Instead we\n> want to delete the .keep file automatically after all refs have\n> been updated.  That is being left as room for future improvement.\n\nI think this should be solved before rx packs are actually stored as \npacks though.  Otherwise people will end up with unwanted .keep files \nleft around.  Maybe having a much bigger default for object number \ntreshold for the time being?  (unless this patch is applied to \"next\" at \nthe same time as another one that actually deals with those .keep \nfiles).\n\n\n"},{"id":"297981","messageId":"20061031201148.GD23671@spearce.org","threadId":"43170","inReplyTo":"Pine.LNX.4.64.0610311447250.11384@xanadu.home","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-31T20:11:48Z","receivedAt":"2006-10-31T20:11:48Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> On Tue, 31 Oct 2006, Shawn Pearce wrote:\n> \n> > Since keeping a pushed pack or exploding it into loose objects\n> > should be a local repository decision this teaches receive-pack\n> > to decide if it should call unpack-objects or index-pack --stdin\n> > --fix-thin based on the setting of receive.unpackLimit and the\n> > number of objects contained in the received pack.\n> \n> This works fine when used with my replacement patch for your [1/2] one.\n> \n> > Currently this leaves every received pack as a kept pack.  We really\n> > don't want that as received packs will tend to be small.  Instead we\n> > want to delete the .keep file automatically after all refs have\n> > been updated.  That is being left as room for future improvement.\n> \n> I think this should be solved before rx packs are actually stored as \n> packs though.  Otherwise people will end up with unwanted .keep files \n> left around.  Maybe having a much bigger default for object number \n> treshold for the time being?  (unless this patch is applied to \"next\" at \n> the same time as another one that actually deals with those .keep \n> files).\n\nIts next on my list of things to do.  Hopefully I'll be able to\nimplement it today.\n\nI'm thinking of just brute forcing it: put enough identifying data\ninto the .keep file to make it unique, then go through every local\npack and look at their .keep file; if the content matches what\nreceive-pack asked index-pack to put there then remove it.\n\n-- \n"},{"id":"293963","messageId":"Pine.LNX.4.64.0610311559150.11384@xanadu.home","threadId":"43170","inReplyTo":"20061031201148.GD23671@spearce.org","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-10-31T21:08:32Z","receivedAt":"2006-10-31T21:08:32Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 31 Oct 2006, Shawn Pearce wrote:\n> Nicolas Pitre <nico@cam.org> wrote:\n> > I think this should be solved before rx packs are actually stored as \n> > packs though.  Otherwise people will end up with unwanted .keep files \n> > left around.  Maybe having a much bigger default for object number \n> > treshold for the time being?  (unless this patch is applied to \"next\" at \n> > the same time as another one that actually deals with those .keep \n> > files).\n> \n> Its next on my list of things to do.  Hopefully I'll be able to\n> implement it today.\n> \n> I'm thinking of just brute forcing it: put enough identifying data\n> into the .keep file to make it unique, then go through every local\n> pack and look at their .keep file; if the content matches what\n> receive-pack asked index-pack to put there then remove it.\n\nOuch.  What about the patch below?  It covers only the pull/fetch case, \nbut covering the push case shouldn't be that hard either (simply use a \npipe to read index-pack's stdout and capture the pack name).\n\nI used \"pack\" <tab> <sha1> so it is easy to pick out of the list of refs \nthat usually comes over the stream in the fetch case (if I understood \nthat part right).\n\ndiff --git a/fetch-clone.c b/fetch-clone.c\nindex f629d8d..579307d 100644\n--- a/fetch-clone.c\n+++ b/fetch-clone.c\n@@ -81,7 +81,18 @@ int receive_unpack_pack(int xd[2], const\n \n int receive_keep_pack(int xd[2], const char *me, int quiet, int sideband)\n {\n-\tconst char *argv[5] = { \"index-pack\", \"--stdin\", \"--fix-thin\",\n-\t\t\t\tquiet ? NULL : \"-v\", NULL };\n+\tconst char *argv[6];\n+\tchar my_host[255], keep_arg[128 + 255];\n+\n+\tif (gethostname(my_host, sizeof(my_host)))\n+\t\tstrcpy(my_host, \"localhost\");\n+\tsnprintf(keep_arg, sizeof(keep_arg), \"--keep=fetch-pack %i on %s\",\n+\t\t getpid(), my_host);\n+\targv[0] = \"index-pack\";\n+\targv[1] = \"--stdin\";\n+\targv[2] = \"--fix-thin\";\n+\targv[3] = keep_arg;\n+\targv[4] = quiet ? NULL : \"-v\";\n+\targv[5] = NULL;\n \treturn get_pack(xd, me, sideband, argv);\n }\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex 539dff6..366014d 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -368,6 +368,7 @@ fetch_main () {\n       ;; # we are already done.\n   *)\n     ( : subshell because we muck with IFS\n+      pack_lockfile=\n       IFS=\" \t$LF\"\n       (\n \t  git-fetch-pack $exec $keep \"$remote\" $rref || echo failed \"$remote\"\n@@ -378,6 +379,10 @@ fetch_main () {\n \t  failed)\n \t\t  echo >&2 \"Fetch failure: $remote\"\n \t\t  exit 1 ;;\n+\t  pack)\n+\t\t  # special line coming from index-pack with the pack name\n+\t\t  pack_lockfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n+\t\t  continue ;;\n \t  esac\n \t  found=\n \t  single_force=\n@@ -408,6 +413,7 @@ fetch_main () {\n \t  append_fetch_head \"$sha1\" \"$remote\" \\\n \t\t  \"$remote_name\" \"$remote_nick\" \"$local_name\" \"$not_for_merge\"\n       done\n+      [ \"$pack_lockfile\" ] && rm -f \"$pack_lockfile\"\n     ) || exit ;;\n   esac\n \ndiff --git a/index-pack.c b/index-pack.c\nindex a3b55f9..c8f66da 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -767,18 +767,6 @@ static void final(const char *final_pack\n \t\tif (err)\n \t\t\tdie(\"error while closing pack file: %s\", strerror(errno));\n \t\tchmod(curr_pack_name, 0444);\n-\n-\t\t/*\n-\t\t * Let's just mimic git-unpack-objects here and write\n-\t\t * the last part of the buffer to stdout.\n-\t\t */\n-\t\twhile (input_len) {\n-\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n-\t\t\tif (err <= 0)\n-\t\t\t\tbreak;\n-\t\t\tinput_len -= err;\n-\t\t\tinput_offset += err;\n-\t\t}\n \t}\n \n \tif (keep_msg) {\n@@ -818,6 +806,27 @@ static void final(const char *final_pack\n \t\tif (move_temp_to_file(curr_index_name, final_index_name))\n \t\t\tdie(\"cannot store index file\");\n \t}\n+\n+\tif (!from_stdin) {\n+\t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n+\t} else {\n+\t\tchar buf[48];\n+\t\tint len = snprintf(buf, sizeof(buf), \"pack\\t%s\\n\",\n+\t\t\t\t   sha1_to_hex(sha1));\n+\t\txwrite(1, buf, len);\n+\n+\t\t/*\n+\t\t * Let's just mimic git-unpack-objects here and write\n+\t\t * the last part of the input buffer to stdout.\n+\t\t */\n+\t\twhile (input_len) {\n+\t\t\terr = xwrite(1, input_buffer + input_offset, input_len);\n+\t\t\tif (err <= 0)\n+\t\t\t\tbreak;\n+\t\t\tinput_len -= err;\n+\t\t\tinput_offset += err;\n+\t\t}\n+\t}\n }\n \n int main(int argc, char **argv)\n@@ -934,8 +943,5 @@ int main(int argc, char **argv)\n \tfree(index_name_buf);\n \tfree(keep_name_buf);\n \n-\tif (!from_stdin)\n-\t\tprintf(\"%s\\n\", sha1_to_hex(sha1));\n-\n \treturn 0;\n"},{"id":"298270","messageId":"20061031212942.GA24184@spearce.org","threadId":"43170","inReplyTo":"Pine.LNX.4.64.0610311559150.11384@xanadu.home","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-31T21:29:42Z","receivedAt":"2006-10-31T21:29:42Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre <nico@cam.org> wrote:\n> On Tue, 31 Oct 2006, Shawn Pearce wrote:\n> > Nicolas Pitre <nico@cam.org> wrote:\n> > > I think this should be solved before rx packs are actually stored as \n> > > packs though.  Otherwise people will end up with unwanted .keep files \n> > > left around.  Maybe having a much bigger default for object number \n> > > treshold for the time being?  (unless this patch is applied to \"next\" at \n> > > the same time as another one that actually deals with those .keep \n> > > files).\n> > \n> > Its next on my list of things to do.  Hopefully I'll be able to\n> > implement it today.\n> > \n> > I'm thinking of just brute forcing it: put enough identifying data\n> > into the .keep file to make it unique, then go through every local\n> > pack and look at their .keep file; if the content matches what\n> > receive-pack asked index-pack to put there then remove it.\n> \n> Ouch.  What about the patch below?  It covers only the pull/fetch case, \n> but covering the push case shouldn't be that hard either (simply use a \n> pipe to read index-pack's stdout and capture the pack name).\n> \n> I used \"pack\" <tab> <sha1> so it is easy to pick out of the list of refs \n> that usually comes over the stream in the fetch case (if I understood \n> that part right).\n\nI thought about using a pipe too, but in the case of receive-pack\nit looked like index-pack was sending something back to the push\nend of the connection.  I didn't dig into the code enough to see\nwhat that was and how to do the same in receive-pack itself.  The\nbrute force approach is horrible but simple.  ;-)\n\nI'll look at your patch and what I need to do make a pipe work here,\nbecause its clearly the better solution.\n\n-- \n"},{"id":"298448","messageId":"Pine.LNX.4.64.0610311659310.11384@xanadu.home","threadId":"43170","inReplyTo":"20061031212942.GA24184@spearce.org","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-10-31T22:06:47Z","receivedAt":"2006-10-31T22:06:47Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Tue, 31 Oct 2006, Shawn Pearce wrote:\n> Nicolas Pitre <nico@cam.org> wrote:\n> > I used \"pack\" <tab> <sha1> so it is easy to pick out of the list of refs \n> > that usually comes over the stream in the fetch case (if I understood \n> > that part right).\n> \n> I thought about using a pipe too, but in the case of receive-pack\n> it looked like index-pack was sending something back to the push\n> end of the connection.  I didn't dig into the code enough to see\n> what that was and how to do the same in receive-pack itself.\n\nWell, I think it goes like this:\n\nunpack-objects (and now index-pack --stdin) reads from stdin in 4kb \nchunks.  When the pack has been entirely parsed, it is possible that the \n4kb chunk contains data from the stream past the actual pack data which \nis why the remaining of the buffer is flushed out to stdout for the next \ntool in the chain to pick up.\n\nSo if you use a pipe with index-pack to retrieve the pack name, you also \nmust consume all extra data from it and pass it on as well.\n\n\n"},{"id":"295107","messageId":"7vslh4qfxj.fsf@assigned-by-dhcp.cox.net","threadId":"43170","inReplyTo":"Pine.LNX.4.64.0610311559150.11384@xanadu.home","subject":"Re: [PATCH 2/2] Teach receive-pack how to keep pack files based on object count.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-31T22:27:36Z","receivedAt":"2006-10-31T22:27:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nicolas Pitre <nico@cam.org> writes:\n\n> I used \"pack\" <tab> <sha1> so it is easy to pick out of the list of refs \n> that usually comes over the stream in the fetch case (if I understood \n> that part right).\n\nThis sounds like a very sane thing to do.\n"}]}