{"thread":{"id":"9501","subject":"[PATCH] builtin-bundle create - use lock_file semantics","startedAt":"2007-08-12T12:53:02Z","lastAt":"2007-08-12T18:22:55Z","messageCount":4,"participants":["Mark Levedahl","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"50551","messageId":"11869231822803-git-send-email-mdl123@verizon.net","threadId":"9501","inReplyTo":null,"subject":"[PATCH] builtin-bundle create - use lock_file semantics","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-08-12T12:53:02Z","receivedAt":"2007-08-12T12:53:02Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"git bundle create would leave an invalid, partially written bundle if\nan error occured during creation. Fix that using lock_file.\n\nSigned-off-by: Mark Levedahl <mdl123@verizon.net>\n---\n builtin-bundle.c |   51 +++++++++++++++++++++++++++++++--------------------\n 1 files changed, 31 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex f4b4f03..82e00f5 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -186,10 +186,20 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)\n \treturn list_refs(&header->references, argc, argv);\n }\n \n+/* create_bundle uses lock_file, delete if write fails */\n+static inline void lwrite_or_die(int fd, const void *buf, size_t count, struct lock_file *lock)\n+{\n+\tif (write_in_full(fd, buf, count) != count) {\n+\t\trollback_lock_file(lock);\n+\t\tdie(\"Unable to write bundle\");\n+\t}\n+}\n+\n static int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv)\n {\n \tint bundle_fd = -1;\n+\tstruct lock_file lock;\n \tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(5 * sizeof(const char *));\n \tint i, ref_count = 0;\n@@ -198,17 +208,9 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \tstruct child_process rls;\n \tFILE *rls_fout;\n \n-\t/*\n-\t * NEEDSWORK: this should use something like lock-file\n-\t * to create temporary that is cleaned up upon error.\n-\t */\n-\tbundle_fd = (!strcmp(path, \"-\") ? 1 :\n-\t\t\topen(path, O_CREAT | O_EXCL | O_WRONLY, 0666));\n-\tif (bundle_fd < 0)\n-\t\treturn error(\"Could not create '%s': %s\", path, strerror(errno));\n-\n \t/* write signature */\n-\twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n+\tbundle_fd = hold_lock_file_for_update(&lock, path, 1);\n+\tlwrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature), &lock);\n \n \t/* init revs to list objects for pack-objects later */\n \tsave_commit_buffer = 0;\n@@ -230,7 +232,7 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \twhile (fgets(buffer, sizeof(buffer), rls_fout)) {\n \t\tunsigned char sha1[20];\n \t\tif (buffer[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buffer, strlen(buffer));\n+\t\t\tlwrite_or_die(bundle_fd, buffer, strlen(buffer), &lock);\n \t\t\tif (!get_sha1_hex(buffer + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object(sha1);\n \t\t\t\tobject->flags |= UNINTERESTING;\n@@ -242,13 +244,17 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\t}\n \t}\n \tfclose(rls_fout);\n-\tif (finish_command(&rls))\n+\tif (finish_command(&rls)) {\n+\t\trollback_lock_file(&lock);\n \t\treturn error(\"rev-list died\");\n+\t}\n \n \t/* write references */\n \targc = setup_revisions(argc, argv, &revs, NULL);\n-\tif (argc > 1)\n+\tif (argc > 1) {\n+\t\trollback_lock_file(&lock);\n \t\treturn error(\"unrecognized argument: %s'\", argv[1]);\n+\t}\n \n \tfor (i = 0; i < revs.pending.nr; i++) {\n \t\tstruct object_array_entry *e = revs.pending.objects + i;\n@@ -307,17 +313,19 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\t}\n \n \t\tref_count++;\n-\t\twrite_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40);\n-\t\twrite_or_die(bundle_fd, \" \", 1);\n-\t\twrite_or_die(bundle_fd, ref, strlen(ref));\n-\t\twrite_or_die(bundle_fd, \"\\n\", 1);\n+\t\tlwrite_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40, &lock);\n+\t\tlwrite_or_die(bundle_fd, \" \", 1, &lock);\n+\t\tlwrite_or_die(bundle_fd, ref, strlen(ref), &lock);\n+\t\tlwrite_or_die(bundle_fd, \"\\n\", 1, &lock);\n \t\tfree(ref);\n \t}\n-\tif (!ref_count)\n+\tif (!ref_count) {\n+\t\trollback_lock_file(&lock);\n \t\tdie (\"Refusing to create empty bundle.\");\n+\t}\n \n \t/* end header */\n-\twrite_or_die(bundle_fd, \"\\n\", 1);\n+\tlwrite_or_die(bundle_fd, \"\\n\", 1, &lock);\n \n \t/* write pack */\n \targv_pack[0] = \"pack-objects\";\n@@ -339,8 +347,11 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\twrite(rls.in, sha1_to_hex(object->sha1), 40);\n \t\twrite(rls.in, \"\\n\", 1);\n \t}\n-\tif (finish_command(&rls))\n+\tif (finish_command(&rls)) {\n+\t\trollback_lock_file(&lock);\n \t\treturn error (\"pack-objects died\");\n+\t}\n+\tcommit_lock_file(&lock);\n \treturn 0;\n }\n \n-- \n1.5.3.rc4.78.g5acb3-dirty\n"},{"id":"50556","messageId":"11869280491451-git-send-email-mdl123@verizon.net","threadId":"9501","inReplyTo":"11869231822803-git-send-email-mdl123@verizon.net","subject":"[PATCH] builtin-bundle create - use lock_file","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-08-12T14:14:09Z","receivedAt":"2007-08-12T14:14:09Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"git bundle create would leave an invalid, partially written bundle if\nan error occured during creation. Fix that using lock_file.\n\nSigned-off-by: Mark Levedahl <mdl123@verizon.net>\n---\n struct lock_file is now static.\n *caller* closes the lock file before commiting / rolling back.\n\n builtin-bundle.c |   57 +++++++++++++++++++++++++++++++++++------------------\n 1 files changed, 37 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex f4b4f03..e5a2859 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -186,10 +186,21 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)\n \treturn list_refs(&header->references, argc, argv);\n }\n\n+/* create_bundle uses lock_file, delete if write fails */\n+static inline void lwrite_or_die(int fd, const void *buf, size_t count, struct lock_file *lock)\n+{\n+\tif (write_in_full(fd, buf, count) != count) {\n+\t\tclose(fd);\n+\t\trollback_lock_file(lock);\n+\t\tdie(\"Unable to write bundle\");\n+\t}\n+}\n+\n static int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv)\n {\n \tint bundle_fd = -1;\n+\tstatic struct lock_file lock;\n \tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(5 * sizeof(const char *));\n \tint i, ref_count = 0;\n@@ -198,17 +209,9 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \tstruct child_process rls;\n \tFILE *rls_fout;\n\n-\t/*\n-\t * NEEDSWORK: this should use something like lock-file\n-\t * to create temporary that is cleaned up upon error.\n-\t */\n-\tbundle_fd = (!strcmp(path, \"-\") ? 1 :\n-\t\t\topen(path, O_CREAT | O_EXCL | O_WRONLY, 0666));\n-\tif (bundle_fd < 0)\n-\t\treturn error(\"Could not create '%s': %s\", path, strerror(errno));\n-\n \t/* write signature */\n-\twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n+\tbundle_fd = hold_lock_file_for_update(&lock, path, 1);\n+\tlwrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature), &lock);\n\n \t/* init revs to list objects for pack-objects later */\n \tsave_commit_buffer = 0;\n@@ -230,7 +233,7 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \twhile (fgets(buffer, sizeof(buffer), rls_fout)) {\n \t\tunsigned char sha1[20];\n \t\tif (buffer[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buffer, strlen(buffer));\n+\t\t\tlwrite_or_die(bundle_fd, buffer, strlen(buffer), &lock);\n \t\t\tif (!get_sha1_hex(buffer + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object(sha1);\n \t\t\t\tobject->flags |= UNINTERESTING;\n@@ -242,13 +245,19 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\t}\n \t}\n \tfclose(rls_fout);\n-\tif (finish_command(&rls))\n+\tif (finish_command(&rls)) {\n+\t\tclose(bundle_fd);\n+\t\trollback_lock_file(&lock);\n \t\treturn error(\"rev-list died\");\n+\t}\n\n \t/* write references */\n \targc = setup_revisions(argc, argv, &revs, NULL);\n-\tif (argc > 1)\n+\tif (argc > 1) {\n+\t\tclose(bundle_fd);\n+\t\trollback_lock_file(&lock);\n \t\treturn error(\"unrecognized argument: %s'\", argv[1]);\n+\t}\n\n \tfor (i = 0; i < revs.pending.nr; i++) {\n \t\tstruct object_array_entry *e = revs.pending.objects + i;\n@@ -307,17 +316,20 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\t}\n\n \t\tref_count++;\n-\t\twrite_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40);\n-\t\twrite_or_die(bundle_fd, \" \", 1);\n-\t\twrite_or_die(bundle_fd, ref, strlen(ref));\n-\t\twrite_or_die(bundle_fd, \"\\n\", 1);\n+\t\tlwrite_or_die(bundle_fd, sha1_to_hex(e->item->sha1), 40, &lock);\n+\t\tlwrite_or_die(bundle_fd, \" \", 1, &lock);\n+\t\tlwrite_or_die(bundle_fd, ref, strlen(ref), &lock);\n+\t\tlwrite_or_die(bundle_fd, \"\\n\", 1, &lock);\n \t\tfree(ref);\n \t}\n-\tif (!ref_count)\n+\tif (!ref_count) {\n+\t\tclose(bundle_fd);\n+\t\trollback_lock_file(&lock);\n \t\tdie (\"Refusing to create empty bundle.\");\n+\t}\n\n \t/* end header */\n-\twrite_or_die(bundle_fd, \"\\n\", 1);\n+\tlwrite_or_die(bundle_fd, \"\\n\", 1, &lock);\n\n \t/* write pack */\n \targv_pack[0] = \"pack-objects\";\n@@ -339,8 +351,13 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t\twrite(rls.in, sha1_to_hex(object->sha1), 40);\n \t\twrite(rls.in, \"\\n\", 1);\n \t}\n-\tif (finish_command(&rls))\n+\tif (finish_command(&rls)) {\n+\t\tclose(bundle_fd);\n+\t\trollback_lock_file(&lock);\n \t\treturn error (\"pack-objects died\");\n+\t}\n+\tclose(bundle_fd);\n+\tcommit_lock_file(&lock);\n \treturn 0;\n }\n\n--\n1.5.3.rc4.79.gb5c7e-dirty\n"},{"id":"50564","messageId":"7v1we8k5qx.fsf@assigned-by-dhcp.cox.net","threadId":"9501","inReplyTo":"11869280491451-git-send-email-mdl123@verizon.net","subject":"Re: [PATCH] builtin-bundle create - use lock_file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-08-12T17:44:06Z","receivedAt":"2007-08-12T17:44:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mark Levedahl <mdl123@verizon.net> writes:\n\n> git bundle create would leave an invalid, partially written bundle if\n> an error occured during creation. Fix that using lock_file.\n>\n> Signed-off-by: Mark Levedahl <mdl123@verizon.net>\n> ---\n>  struct lock_file is now static.\n>  *caller* closes the lock file before commiting / rolling back.\n\nI think you broke the support to write a bundle out to the\nstandard output by giving \"-\" as the path.\n\nAlso, the whole point of the \"NEEDSWORK\" suggestion to use\nlockfile was because you do not have to do lwrite_or_die() in\nthe first place ;-).  When you exit before committing lockfiles\nyou hold, atexit handler rolls back and unlink them for you.\n\nThe following might be a better replacement.\n\nBy the way, by using lockfile's \"create temporary, generate into\nit and then rename to final\", we are allowing an overwrite of an\nexisting bundle, which we did not allow earlier.  Is this check\nsomething we would want to preserve?\n\n---\n\n builtin-bundle.c |   18 ++++++++++--------\n 1 files changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin-bundle.c b/builtin-bundle.c\nindex f4b4f03..1b65006 100644\n--- a/builtin-bundle.c\n+++ b/builtin-bundle.c\n@@ -189,7 +189,9 @@ static int list_heads(struct bundle_header *header, int argc, const char **argv)\n static int create_bundle(struct bundle_header *header, const char *path,\n \t\tint argc, const char **argv)\n {\n+\tstatic struct lock_file lock;\n \tint bundle_fd = -1;\n+\tint bundle_to_stdout;\n \tconst char **argv_boundary = xmalloc((argc + 4) * sizeof(const char *));\n \tconst char **argv_pack = xmalloc(5 * sizeof(const char *));\n \tint i, ref_count = 0;\n@@ -198,14 +200,11 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \tstruct child_process rls;\n \tFILE *rls_fout;\n \n-\t/*\n-\t * NEEDSWORK: this should use something like lock-file\n-\t * to create temporary that is cleaned up upon error.\n-\t */\n-\tbundle_fd = (!strcmp(path, \"-\") ? 1 :\n-\t\t\topen(path, O_CREAT | O_EXCL | O_WRONLY, 0666));\n-\tif (bundle_fd < 0)\n-\t\treturn error(\"Could not create '%s': %s\", path, strerror(errno));\n+\tbundle_to_stdout = !strcmp(path, \"-\");\n+\tif (bundle_to_stdout)\n+\t\tbundle_fd = 1;\n+\telse\n+\t\tbundle_fd = hold_lock_file_for_update(&lock, path, 1);\n \n \t/* write signature */\n \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\n@@ -341,6 +340,9 @@ static int create_bundle(struct bundle_header *header, const char *path,\n \t}\n \tif (finish_command(&rls))\n \t\treturn error (\"pack-objects died\");\n+\tclose(bundle_fd);\n+\tif (!bundle_to_stdout)\n+\t\tcommit_lock_file(&lock);\n \treturn 0;\n }\n \n"},{"id":"50567","messageId":"46BF4FFF.1090309@verizon.net","threadId":"9501","inReplyTo":"7v1we8k5qx.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] builtin-bundle create - use lock_file","fromName":"Mark Levedahl","fromEmail":"mdl123@verizon.net","sentAt":"2007-08-12T18:22:55Z","receivedAt":"2007-08-12T18:22:55Z","isPatch":true,"sender":{"key":"mdl123@verizon.net","avatar":"https://avatars.githubusercontent.com/u/5302462?v=4"},"body":"Junio C Hamano wrote:\n> When you exit before committing lockfiles\n> you hold, atexit handler rolls back and unlink them for you.\n>\n> The following might be a better replacement.\n>\n> By the way, by using lockfile's \"create temporary, generate into\n> it and then rename to final\", we are allowing an overwrite of an\n> existing bundle, which we did not allow earlier.  Is this check\n> something we would want to preserve?\n>   \nAck - yours is much better. Thanks for the lesson. Also, the former \nbehavior of not allowing overwrite of an existing file was (at least in \nmy opinion) very un-git'ish so I am happy to see that go.\n\nMark\n"}]}