{"thread":{"id":"43417","subject":"Re: [PATCH 3/3] Teach git-index-pack how to keep a pack file.","startedAt":"2006-10-29T09:41:59Z","lastAt":"2006-10-30T03:47:08Z","messageCount":5,"participants":["Jakub Narebski","Shawn Pearce","Junio C Hamano","Nicolas Pitre","Michael S. Tsirkin"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"295233","messageId":"20061029094159.GE3847@spearce.org","threadId":"43417","inReplyTo":null,"subject":"[PATCH 3/3] Teach git-index-pack how to keep a pack file.","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2006-10-29T09:41:59Z","receivedAt":"2006-10-29T09:41:59Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"To prevent a race condition between `index-pack --stdin` and\n`repack -a -d` where the repack deletes the newly created pack\nfile before any refs are updated to reference objects contained\nwithin it we mark the pack file as one that should be kept. This\nremoves it from the list of packs that `repack -a -d` will consider\nfor removal.\n\nCallers such as `receive-pack` which want to invoke `index-pack`\nshould use this new --keep option to prevent the newly created pack\nand index file pair from being deleted before they have finished any\nrelated ref updates. Only after all ref updates have been finished\nshould the associated .keep file be removed.\n\nSigned-off-by: Shawn O. Pearce \n---\n I'm still working on the change to receive-pack. Junio's latest\n patch (send-pack --keep) is a different way of doing at least some\n of what I'm still working on there.\n\n I'm throwing a pair of pipes around index-pack so I can capture\n the pack name thus allowing receive-pack to delete the correct\n .keep file after its completed the ref updates. At that point\n receive-pack can easily examine the object count in the pack header\n and compare that against a config entry to decide if it should be\n keeping the pack or exploding it.\n\n\n Documentation/git-index-pack.txt | 24 ++++++++++++++++++--\n index-pack.c | 43 +++++++++++++++++++++++++++++++++++--\n 2 files changed, 61 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 9fa4847..1235416 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -9,7 +9,7 @@ git-index-pack - Build pack index file f\n SYNOPSIS\n --------\n 'git-index-pack' [-v] [-o ] \n-'git-index-pack' --stdin [--fix-thin] [-v] [-o ] []\n+'git-index-pack' --stdin [--fix-thin] [--keep] [-v] [-o ] []\n \n \n DESCRIPTION\n@@ -38,7 +38,10 @@ OPTIONS\n \tinstead and a copy is then written to . If\n \t is not specified, the pack is written to\n \tobjects/pack/ directory of the current git repository with\n-\ta default name determined from the pack content.\n+\ta default name determined from the pack content. If\n+\t is not specified consider using --keep to\n+\tprevent a race condition between this process and\n+\tgitlink::git-repack[1] .\n \n --fix-thin::\n \tIt is possible for gitlink:git-pack-objects[1] to build\n@@ -48,7 +51,22 @@ OPTIONS\n \tand they must be included in the pack for that pack to be self\n \tcontained and indexable. Without this option any attempt to\n \tindex a thin pack will fail. This option only makes sense in\n-\tconjonction with --stdin.\n+\tconjunction with --stdin.\n+\n+--keep::\n+\tBefore moving the index into its final destination\n+\tcreate an empty .keep file for the associated pack file.\n+\tThis option is usually necessary with --stdin to prevent a\n+\tsimultaneous gitlink:git-repack[1] process from deleting\n+\tthe newly constructed pack and index before refs can be\n+\tupdated to use objects contained in the pack.\n+\n+--keep='why'::\n+\tLike --keep create a .keep file before moving the index into\n+\tits final destination, but rather than creating an empty file\n+\tplace 'why' followed by an LF into the .keep file. The 'why'\n+\tmessage can later be searched for within all .keep files to\n+\tlocate any which have outlived their usefulness.\n \n \n Author\ndiff --git a/index-pack.c b/index-pack.c\nindex 866a054..b37dd78 100644\n--- a/index-pack.c\n+++ b/index-pack.c\n@@ -10,7 +10,7 @@\n #include \n \n static const char index_pack_usage[] =\n-\"git-index-pack [-v] [-o ] { | --stdin [--fix-thin] [] }\";\n+\"git-index-pack [-v] [-o ] [{ ---keep | --keep= }] { | --stdin [--fix-thin] [] }\";\n \n struct object_entry\n {\n@@ -754,6 +754,7 @@ static const char *write_index_file(cons\n \n static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\t const char *final_index_name, const char *curr_index_name,\n+\t\t const char *keep_name, const char *keep_msg,\n \t\t unsigned char *sha1)\n {\n \tchar name[PATH_MAX];\n@@ -780,6 +781,23 @@ static void final(const char *final_pack\n \t\t}\n \t}\n \n+\tif (keep_msg) {\n+\t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n+\t\tif (!keep_name) {\n+\t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.keep\",\n+\t\t\t\t get_object_directory(), sha1_to_hex(sha1));\n+\t\t\tkeep_name = name;\n+\t\t}\n+\t\tkeep_fd = open(keep_name, O_RDWR | O_CREAT, 0600);\n+\t\tif (keep_fd < 0)\n+\t\t\tdie(\"cannot write keep file\");\n+\t\tif (keep_msg_len > 0) {\n+\t\t\twrite_or_die(keep_fd, keep_msg, keep_msg_len);\n+\t\t\twrite_or_die(keep_fd, \"\\n\", 1);\n+\t\t}\n+\t\tclose(keep_fd);\n+\t}\n+\n \tif (final_pack_name != curr_pack_name) {\n \t\tif (!final_pack_name) {\n \t\t\tsnprintf(name, sizeof(name), \"%s/pack/pack-%s.pack\",\n@@ -807,7 +825,8 @@ int main(int argc, char **argv)\n \tint i, fix_thin_pack = 0;\n \tconst char *curr_pack, *pack_name = NULL;\n \tconst char *curr_index, *index_name = NULL;\n-\tchar *index_name_buf = NULL;\n+\tconst char *keep_name = NULL, *keep_msg = NULL;\n+\tchar *index_name_buf = NULL, *keep_name_buf = NULL;\n \tunsigned char sha1[20];\n \n \tfor (i = 1; i < argc; i++) {\n@@ -818,6 +837,10 @@ int main(int argc, char **argv)\n \t\t\t\tfrom_stdin = 1;\n \t\t\t} else if (!strcmp(arg, \"--fix-thin\")) {\n \t\t\t\tfix_thin_pack = 1;\n+\t\t\t} else if (!strcmp(arg, \"--keep\")) {\n+\t\t\t\tkeep_msg = \"\";\n+\t\t\t} else if (!strncmp(arg, \"--keep=\", 7)) {\n+\t\t\t\tkeep_msg = arg + 7;\n \t\t\t} else if (!strcmp(arg, \"-v\")) {\n \t\t\t\tverbose = 1;\n \t\t\t} else if (!strcmp(arg, \"-o\")) {\n@@ -848,6 +871,16 @@ int main(int argc, char **argv)\n \t\tstrcpy(index_name_buf + len - 5, \".idx\");\n \t\tindex_name = index_name_buf;\n \t}\n+\tif (keep_msg && !keep_name && pack_name) {\n+\t\tint len = strlen(pack_name);\n+\t\tif (!has_extension(pack_name, \".pack\"))\n+\t\t\tdie(\"packfile name '%s' does not end with '.pack'\",\n+\t\t\t pack_name);\n+\t\tkeep_name_buf = xmalloc(len);\n+\t\tmemcpy(keep_name_buf, pack_name, len - 5);\n+\t\tstrcpy(keep_name_buf + len - 5, \".keep\");\n+\t\tkeep_name = keep_name_buf;\n+\t}\n \n \tcurr_pack = open_pack_file(pack_name);\n \tparse_pack_header();\n@@ -880,9 +913,13 @@ int main(int argc, char **argv)\n \t}\n \tfree(deltas);\n \tcurr_index = write_index_file(index_name, sha1);\n-\tfinal(pack_name, curr_pack, index_name, curr_index, sha1);\n+\tfinal(pack_name, curr_pack,\n+\t\tindex_name, curr_index,\n+\t\tkeep_name, keep_msg,\n+\t\tsha1);\n \tfree(objects);\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-- \n1.4.3.3.g7d63\n"},{"id":"294672","messageId":"ei210e$dfs$1@sea.gmane.org","threadId":"43417","inReplyTo":"20061029094159.GE3847@spearce.org","subject":"Re: [PATCH 3/3] Teach git-index-pack how to keep a pack file.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2006-10-29T10:49:17Z","receivedAt":"2006-10-29T10:49:17Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Shawn Pearce wrote:\n\n> +--keep::\n> +       Before moving the index into its final destination\n> +       create an empty .keep file for the associated pack file.\n> +       This option is usually necessary with --stdin to prevent a\n> +       simultaneous gitlink:git-repack[1] process from deleting\n> +       the newly constructed pack and index before refs can be\n> +       updated to use objects contained in the pack.\n> +\n> +--keep='why'::\n> +       Like --keep create a .keep file before moving the index into\n> +       its final destination, but rather than creating an empty file\n> +       place 'why' followed by an LF into the .keep file.  The 'why'\n> +       message can later be searched for within all .keep files to\n> +       locate any which have outlived their usefulness.\n\nWouldn't it be better to use 'git-index-pack', perhaps with source URL if\npossible, as the default 'why'?\n-- \nJakub Narebski\nWarsaw, Poland\nShadeHawk on #git\n\n"},{"id":"296036","messageId":"7vr6wr0wa7.fsf@assigned-by-dhcp.cox.net","threadId":"43417","inReplyTo":"ei210e$dfs$1@sea.gmane.org","subject":"Re: [PATCH 3/3] Teach git-index-pack how to keep a pack file.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-29T19:14:24Z","receivedAt":"2006-10-29T19:14:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Wouldn't it be better to use 'git-index-pack', perhaps with source URL if\n> possible, as the default 'why'?\n\nI think it is sensible that Shawn did keep=\"reason\" because\ngit-index-pack usually cannot decide. .keep would be around for\nat least three reasons:\n\n - you as the repository owner want to keep the pack from\n repacked because that is the big \"initial clone\" pack;\n\n - somebody fetched with --keep and index-pack created .keep\n flag, but somehow the fetch-pack after updating the refs\n failed to remove the flag (or has not finished);\n\n - somebody pushed into the repository with --keep but somehow\n the receive-pack after updating the refs failed to remove the\n flag (or has not finished);\n\nFor a shared repository settings, the person who cares about .keep\nis the person who logged onto the machine with the repository and\nran \"clone\" (or \"init-db && fetch-pack\") to prepare the initial\nstate, and fetching would be done by smaller group of people\nthan pushing into it. So these three kinds need to be easily\ndistinguishable by the repository owner. The first kind would\nnot be usually removed; the second one the owner is likely to\nknow if that fetch is still running, and if it is known that\nfetch is not running then .keep should probably be removed. The\nthird kind is harder to tell if it is safe to remove .keep\nbecause usually more people can push into it than fetch into it.\n\nIt might be a good idea to leave process ID of the caller of the\nindex-pack as part of --keep=\"reason\" string, along with the\ntimestamp to help the user decide if the .keep is leftover or\nstill in progress (the timestamp can be seen from ls -l *.keep\nso it is not strictly needed).\n\n\n\n\n"},{"id":"298320","messageId":"20061029194412.GA3346@mellanox.co.il","threadId":"43417","inReplyTo":"7vr6wr0wa7.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH 3/3] Teach git-index-pack how to keep a pack file.","fromName":"Michael S. Tsirkin","fromEmail":"mst@mellanox.co.il","sentAt":"2006-10-29T19:44:12Z","receivedAt":"2006-10-29T19:44:12Z","isPatch":true,"sender":{"key":"mst@kernel.org","avatar":null},"body":"Quoting r. Junio C Hamano :\n> It might be a good idea to leave process ID of the caller of the\n> index-pack as part of --keep=\"reason\" string, along with the\n> timestamp to help the user decide if the .keep is leftover or\n> still in progress (the timestamp can be seen from ls -l *.keep\n> so it is not strictly needed).\n\nAdd hostname to that, for repositories that reside on networked file systems.\n\n-- \n"},{"id":"296817","messageId":"Pine.LNX.4.64.0610292232440.11384@xanadu.home","threadId":"43417","inReplyTo":"20061029094159.GE3847@spearce.org","subject":"Re: [PATCH 3/3] Teach git-index-pack how to keep a pack file.","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2006-10-30T03:47:08Z","receivedAt":"2006-10-30T03:47:08Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Sun, 29 Oct 2006, Shawn Pearce wrote:\n\n> I'm throwing a pair of pipes around index-pack so I can capture\n> the pack name thus allowing receive-pack to delete the correct\n> .keep file after its completed the ref updates. At that point\n> receive-pack can easily examine the object count in the pack header\n> and compare that against a config entry to decide if it should be\n> keeping the pack or exploding it.\n\nIs it really worth the trouble? Especially if you've already written \nthe pack out, why just not keep it instead of burning more CPU cycles \nexploding it? The next repack will delete it anyway.\n\nAt least for the fetch/pull case here's what I've done so far. It is \nnot complete as the .keep file is not deleted as I'm not clear exactly \nwhere in git-fetch.sh it can be safely deleted. But at least it works \nto the point of displaying the name of the file it should consider for \ndeletion.\n\ndiff --git a/fetch-clone.c b/fetch-clone.c\nindex 96cdab4..c5747f4 100644\n--- a/fetch-clone.c\n+++ b/fetch-clone.c\n@@ -81,7 +81,7 @@ 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+\tconst char *argv[6] = { \"index-pack\", \"--stdin\", \"--fix-thin\", \"--keep\",\n \t\t\t\tquiet ? NULL : \"-v\", NULL };\n \treturn get_pack(xd, me, sideband, argv);\n }\ndiff --git a/git-fetch.sh b/git-fetch.sh\nindex 539dff6..dd0c00b 100755\n--- a/git-fetch.sh\n+++ b/git-fetch.sh\n@@ -378,6 +378,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 pack_lockfile=\"$GIT_OBJECT_DIRECTORY/pack/pack-$remote_name.keep\"\n+\t\t [ -e \"$pack_lockfile\" ] && echo >&2 \"$pack_lockfile exists\"\n+\t\t continue ;;\n \t esac\n \t found=\n \t single_force=\ndiff --git a/index-pack.c b/index-pack.c\nindex b37dd78..8db3c93 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@@ -921,8 +930,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"}]}