{"thread":{"id":"5792","subject":"[PATCH] pack-refs: use lockfile as everybody else does.","startedAt":"2006-10-03T09:19:20Z","lastAt":"2006-10-05T14:51:19Z","messageCount":4,"participants":["Junio C Hamano","Linus Torvalds"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"28143","messageId":"7v7izhrc2v.fsf@assigned-by-dhcp.cox.net","threadId":"5792","inReplyTo":null,"subject":"[PATCH] pack-refs: use lockfile as everybody else does.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-03T09:19:20Z","receivedAt":"2006-10-03T09:19:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Signed-off-by: Junio C Hamano <junkio@cox.net>\n---\n\n I failed to spot a difference from the result/lock-path thing in\n builtin-pack-ref and what lockfile interface does for refs and\n index file.  They seem to do the essentially same thing.  Did I\n miss something?\n\n Also I am not sure about the \"fsync(fd); fclose(refs_file)\"\n sequence I did not touch with this patch.  Doesn't stdio still\n have stuff buffered when you run fsync()?  Would adding fflush()\n in between help?\n\n builtin-pack-refs.c |   21 ++++-----------------\n 1 files changed, 4 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin-pack-refs.c b/builtin-pack-refs.c\nindex 4093973..ede4743 100644\n--- a/builtin-pack-refs.c\n+++ b/builtin-pack-refs.c\n@@ -1,7 +1,6 @@\n #include \"cache.h\"\n #include \"refs.h\"\n \n-static const char *result_path, *lock_path;\n static const char builtin_pack_refs_usage[] =\n \"git-pack-refs [--prune]\";\n \n@@ -17,12 +16,6 @@ struct pack_refs_cb_data {\n \tFILE *refs_file;\n };\n \n-static void remove_lock_file(void)\n-{\n-\tif (lock_path)\n-\t\tunlink(lock_path);\n-}\n-\n static int do_not_prune(int flags)\n {\n \t/* If it is already packed or if it is a symref,\n@@ -69,6 +62,8 @@ static void prune_refs(struct ref_to_pru\n \t}\n }\n \n+static struct lock_file packed;\n+\n int cmd_pack_refs(int argc, const char **argv, const char *prefix)\n {\n \tint fd, i;\n@@ -88,14 +83,7 @@ int cmd_pack_refs(int argc, const char *\n \tif (i != argc)\n \t\tusage(builtin_pack_refs_usage);\n \n-\tresult_path = xstrdup(git_path(\"packed-refs\"));\n-\tlock_path = xstrdup(mkpath(\"%s.lock\", result_path));\n-\n-\tfd = open(lock_path, O_CREAT | O_EXCL | O_WRONLY, 0666);\n-\tif (fd < 0)\n-\t\tdie(\"unable to create new ref-pack file (%s)\", strerror(errno));\n-\tatexit(remove_lock_file);\n-\n+\tfd = hold_lock_file_for_update(&packed, git_path(\"packed-refs\"), 1);\n \tcbdata.refs_file = fdopen(fd, \"w\");\n \tif (!cbdata.refs_file)\n \t\tdie(\"unable to create ref-pack file structure (%s)\",\n@@ -103,9 +91,8 @@ int cmd_pack_refs(int argc, const char *\n \tfor_each_ref(handle_one_ref, &cbdata);\n \tfsync(fd);\n \tfclose(cbdata.refs_file);\n-\tif (rename(lock_path, result_path) < 0)\n+\tif (commit_lock_file(&packed) < 0)\n \t\tdie(\"unable to overwrite old ref-pack file (%s)\", strerror(errno));\n-\tlock_path = NULL;\n \tif (cbdata.prune)\n \t\tprune_refs(cbdata.ref_to_prune);\n \treturn 0;\n-- \n1.4.2.3.gd1e9e\n"},{"id":"28228","messageId":"Pine.LNX.4.64.0610041954480.3952@g5.osdl.org","threadId":"5792","inReplyTo":"7v7izhrc2v.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] pack-refs: use lockfile as everybody else does.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-10-05T02:55:42Z","receivedAt":"2006-10-05T02:55:42Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Tue, 3 Oct 2006, Junio C Hamano wrote:\n> \n>  Also I am not sure about the \"fsync(fd); fclose(refs_file)\"\n>  sequence I did not touch with this patch.  Doesn't stdio still\n>  have stuff buffered when you run fsync()?  Would adding fflush()\n>  in between help?\n\nYes, there should be a fflush() there before the fsync.\n\n\t\tLinus\n"},{"id":"28229","messageId":"7vodsrguej.fsf@assigned-by-dhcp.cox.net","threadId":"5792","inReplyTo":"Pine.LNX.4.64.0610041954480.3952@g5.osdl.org","subject":"Re: [PATCH] pack-refs: use lockfile as everybody else does.","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-10-05T06:15:48Z","receivedAt":"2006-10-05T06:15:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds <torvalds@osdl.org> writes:\n\n> Yes, there should be a fflush() there before the fsync.\n\nWhile I have your attention...  The code that follows the\ncommitting of packed-refs file by calling rename() to move the\nlock file to its final name prunes already packed refs, and we\nshould really make sure the effect of rename() survives before\nstarting to remove loose refs from the filesystem.\n\nHow would one ensure the effect of rename(2) hits the disk\nplatter before proceeding to do something else?  We seem to do\nsync(1) in git-repack.sh for similar reasons, and I wonder if we\nshould do a sync(2) there.  I doubt it would be worth it though;\nthe function can return before the actual writing is done.\n\nIf the write-out of metainfo is ordered enough that if we rename\nthe packed-refs lock file to its final destination and then we\nunlink loose refs, it might be reasonably safe to assume that\nrename(2)'s effect has already hit the disk when the effect of\nunlink(2) does, of the machine crashes in the meantime, as long\nas none of the later unlink hits the disk before rename does it\nis Ok.\n"},{"id":"28243","messageId":"Pine.LNX.4.64.0610050747590.3952@g5.osdl.org","threadId":"5792","inReplyTo":"7vodsrguej.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] pack-refs: use lockfile as everybody else does.","fromName":"Linus Torvalds","fromEmail":"torvalds@osdl.org","sentAt":"2006-10-05T14:51:19Z","receivedAt":"2006-10-05T14:51:19Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\n\nOn Wed, 4 Oct 2006, Junio C Hamano wrote:\n> \n> How would one ensure the effect of rename(2) hits the disk\n> platter before proceeding to do something else?  We seem to do\n> sync(1) in git-repack.sh for similar reasons, and I wonder if we\n> should do a sync(2) there.  I doubt it would be worth it though;\n> the function can return before the actual writing is done.\n\nsync() is supposed to wait until the data is on the disk, anything else is \nlikely to be a bad implementation (even if POSIX may not _guarantee_ it).\n\nAlso, most filesystems (for various reasons, not the least of which is \n\"speed of fsck\") will journal metadata, but not file contents. Which means \nthat if a crash happens and one of the loose refs has already been \nremoved, then (thanks to journaling) that means that on most filesystems \nthe rename would already have been guaranteed to hit the disk - even \nregardless of any sync().\n\nSo in practice, it's much more important to guarantee the file contents \n(the fsync) part, because the OS already tends to guarantee metadata \nordering.\n\n\t\t\tLinus\n"}]}