{"thread":{"id":"9244","subject":"[PATCH] use lockfile.c routines in git_commit_set_multivar()","startedAt":"2007-07-26T16:55:28Z","lastAt":"2007-07-27T18:24:34Z","messageCount":7,"participants":["Bradford C. Smith","Johannes Schindelin","Bradford Smith","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"48716","messageId":"11854689283208-git-send-email-bradford.carl.smith@gmail.com","threadId":"9244","inReplyTo":null,"subject":"[PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Bradford C. Smith","fromEmail":"bradford.carl.smith@gmail.com","sentAt":"2007-07-26T16:55:28Z","receivedAt":"2007-07-26T16:55:28Z","isPatch":true,"sender":{"key":"bradford.carl.smith@gmail.com","avatar":"https://gravatar.com/avatar/699930ad8ca562e38156ee60270b2fee830cc0cb5d94c2f70c6db4a705723c54?d=mp&s=160"},"body":"Changed git_commit_set_multivar() to use the routines provided by\nlockfile.c to reduce code duplication and ensure consistent behavior.\n\nSigned-off-by: Bradford C. Smith \n---\n\nI am resubmitting this patch to be considered separately from the\nsymlink resolution change. It differs from the first time I submitted\nit only in that I have corrected the multi-line comment formatting I\nused.\n\nI ran the full test suite (make test) on this patch without failures.\n\n config.c | 30 ++++++++++++++++++------------\n 1 files changed, 18 insertions(+), 12 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex f89a611..dd2de6e 100644\n--- a/config.c\n+++ b/config.c\n@@ -715,7 +715,7 @@ int git_config_set_multivar(const char* key, const char* value,\n \tint fd = -1, in_fd;\n \tint ret;\n \tchar* config_filename;\n-\tchar* lock_file;\n+\tstruct lock_file *lock = NULL;\n \tconst char* last_dot = strrchr(key, '.');\n \n \tconfig_filename = getenv(CONFIG_ENVIRONMENT);\n@@ -725,7 +725,6 @@ int git_config_set_multivar(const char* key, const char* value,\n \t\t\tconfig_filename = git_path(\"config\");\n \t}\n \tconfig_filename = xstrdup(config_filename);\n-\tlock_file = xstrdup(mkpath(\"%s.lock\", config_filename));\n \n \t/*\n \t * Since \"key\" actually contains the section name and the real\n@@ -770,11 +769,12 @@ int git_config_set_multivar(const char* key, const char* value,\n \tstore.key[i] = 0;\n \n \t/*\n-\t * The lock_file serves a purpose in addition to locking: the new\n+\t * The lock serves a purpose in addition to locking: the new\n \t * contents of .git/config will be written into it.\n \t */\n-\tfd = open(lock_file, O_WRONLY | O_CREAT | O_EXCL, 0666);\n-\tif (fd < 0 || adjust_shared_perm(lock_file)) {\n+\tlock = xcalloc(sizeof(struct lock_file), 1);\n+\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tif (fd < 0) {\n \t\tfprintf(stderr, \"could not lock config file\\n\");\n \t\tfree(store.key);\n \t\tret = -1;\n@@ -914,25 +914,31 @@ int git_config_set_multivar(const char* key, const char* value,\n \t\t\t\tgoto write_err_out;\n \n \t\tmunmap(contents, contents_sz);\n-\t\tunlink(config_filename);\n \t}\n \n-\tif (rename(lock_file, config_filename) < 0) {\n-\t\tfprintf(stderr, \"Could not rename the lock file?\\n\");\n+\tif (close(fd) || commit_lock_file(lock) < 0) {\n+\t\tfprintf(stderr, \"Cannot commit config file!\\n\");\n \t\tret = 4;\n \t\tgoto out_free;\n \t}\n \n+\t/* fd is closed, so don't try to close it below. */\n+\tfd = -1;\n+\t/*\n+\t * lock is committed, so don't try to roll it back below.\n+\t * NOTE: Since lockfile.c keeps a linked list of all created\n+\t * lock_file structures, it isn't safe to free(lock). It's\n+\t * better to just leave it hanging around.\n+\t */\n+\tlock = NULL;\n \tret = 0;\n \n out_free:\n \tif (0 <= fd)\n \t\tclose(fd);\n+\tif (lock)\n+\t\trollback_lock_file(lock);\n \tfree(config_filename);\n-\tif (lock_file) {\n-\t\tunlink(lock_file);\n-\t\tfree(lock_file);\n-\t}\n \treturn ret;\n \n write_err_out:\n-- \n1.5.3.rc3.9.g1b487\n"},{"id":"48726","messageId":"Pine.LNX.4.64.0707261926590.14781@racer.site","threadId":"9244","inReplyTo":"11854689283208-git-send-email-bradford.carl.smith@gmail.com","subject":"Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-26T18:31:14Z","receivedAt":"2007-07-26T18:31:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nI like the general idea. Thanks.\n\n\nOn Thu, 26 Jul 2007, Bradford C. Smith wrote:\n\n> +\t/* fd is closed, so don't try to close it below. */\n> +\tfd = -1;\n> +\t/*\n> +\t * lock is committed, so don't try to roll it back below.\n> +\t * NOTE: Since lockfile.c keeps a linked list of all created\n> +\t * lock_file structures, it isn't safe to free(lock). It's\n> +\t * better to just leave it hanging around.\n> +\t */\n> +\tlock = NULL;\n> \tret = 0;\n> \n> out_free:\n> \tif (0 <= fd)\n> \t\tclose(fd);\n> +\tif (lock)\n> +\t\trollback_lock_file(lock);\n\nWouldn't it be better to put the rollback_lock_file() into the if clause \nwhen commit failed?\n\nBesides, I think you can safely call rollback_lock_file(lock) on a \ncommitted lock_file, since the name will be set to \"\" by the latter, which \nis checked by the former.\n\nBut I am fine with the patch as is (have not tested it, though).\n\nCiao,\nDscho\n"},{"id":"48729","messageId":"f158199e0707261148r29419a39h7d83fc7bd0ea7df1@mail.gmail.com","threadId":"9244","inReplyTo":"Pine.LNX.4.64.0707261926590.14781@racer.site","subject":"Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Bradford Smith","fromEmail":"bradford.carl.smith@gmail.com","sentAt":"2007-07-26T18:48:49Z","receivedAt":"2007-07-26T18:48:49Z","isPatch":true,"sender":{"key":"bradford.carl.smith@gmail.com","avatar":"https://gravatar.com/avatar/699930ad8ca562e38156ee60270b2fee830cc0cb5d94c2f70c6db4a705723c54?d=mp&s=160"},"body":"On 7/26/07, Johannes Schindelin wrote:\n> On Thu, 26 Jul 2007, Bradford C. Smith wrote:\n>\n> > + /* fd is closed, so don't try to close it below. */\n> > + fd = -1;\n> > + /*\n> > + * lock is committed, so don't try to roll it back below.\n> > + * NOTE: Since lockfile.c keeps a linked list of all created\n> > + * lock_file structures, it isn't safe to free(lock). It's\n> > + * better to just leave it hanging around.\n> > + */\n> > + lock = NULL;\n> > ret = 0;\n> >\n> > out_free:\n> > if (0 <= fd)\n> > close(fd);\n> > + if (lock)\n> > + rollback_lock_file(lock);\n>\n> Wouldn't it be better to put the rollback_lock_file() into the if clause\n> when commit failed?\n\nActually no. There are multiple goto statements that lead to\nout_free. It isn't even needed at the point that the commit failed,\nbecause commit_lock_file() sets the lock file name to \"\" even when it\nfails.\n\n> Besides, I think you can safely call rollback_lock_file(lock) on a\n> committed lock_file, since the name will be set to \"\" by the latter, which\n> is checked by the former.\n\nQuite right. I really just put in the comment and 'lock= NULL' line\nto increase readability. I wanted to make it very clear to the reader\nthat the commit wouldn't be undone by the rollback.\n\n> But I am fine with the patch as is (have not tested it, though).\n\nThanks!\n\nFWIW, I have successfully run 'make test' and also verified that it\nbehaves as I expect with my ~/.gitconfig symlink (in conjunction with\nthe my other patch for resolving symlinks).\n\nBest Regards,\n\nBradford\n"},{"id":"48754","messageId":"7vfy3a5uzv.fsf@assigned-by-dhcp.cox.net","threadId":"9244","inReplyTo":"f158199e0707261148r29419a39h7d83fc7bd0ea7df1@mail.gmail.com","subject":"Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-27T04:30:12Z","receivedAt":"2007-07-27T04:30:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Bradford Smith\" writes:\n\n> FWIW, I have successfully run 'make test' and also verified that it\n> behaves as I expect with my ~/.gitconfig symlink (in conjunction with\n> the my other patch for resolving symlinks).\n\nExisting \"make test\" testsuite is not an appropriate thing to\nsay this patch is safe, as we do not have much symlinking in the\ntest git repository there. Care to add a new test or two?\n"},{"id":"48757","messageId":"7v7iom5twd.fsf@assigned-by-dhcp.cox.net","threadId":"9244","inReplyTo":"7vfy3a5uzv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-07-27T04:53:54Z","receivedAt":"2007-07-27T04:53:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Bradford Smith\" writes:\n>\n>> FWIW, I have successfully run 'make test' and also verified that it\n>> behaves as I expect with my ~/.gitconfig symlink (in conjunction with\n>> the my other patch for resolving symlinks).\n>\n> Existing \"make test\" testsuite is not an appropriate thing to\n> say this patch is safe, as we do not have much symlinking in the\n> test git repository there. Care to add a new test or two?\n\nHow about this? On top of your \"lockfile to keep symlink\" and\n\"set-multivar to use lockfile protocol\" patches.\n\n---\n\n t/t1300-repo-config.sh | 15 +++++++++++++++\n 1 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\nindex 1c43cc3..187ca2d 100755\n--- a/t/t1300-repo-config.sh\n+++ b/t/t1300-repo-config.sh\n@@ -595,4 +595,19 @@ echo >>result\n \n test_expect_success '--null --get-regexp' 'cmp result expect'\n \n+test_expect_success 'symlinked configuration' '\n+\n+\tln -s notyet myconfig &&\n+\tGIT_CONFIG=myconfig git config test.frotz nitfol &&\n+\ttest -h myconfig &&\n+\ttest -f notyet &&\n+\ttest \"z$(GIT_CONFIG=notyet git config test.frotz)\" = znitfol &&\n+\tGIT_CONFIG=myconfig git config test.xyzzy rezrov &&\n+\ttest -h myconfig &&\n+\ttest -f notyet &&\n+\ttest \"z$(GIT_CONFIG=notyet git config test.frotz)\" = znitfol &&\n+\ttest \"z$(GIT_CONFIG=notyet git config test.xyzzy)\" = zrezrov\n+\n+'\n+\n test_done\n"},{"id":"48781","messageId":"Pine.LNX.4.64.0707271004580.14781@racer.site","threadId":"9244","inReplyTo":"7v7iom5twd.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-07-27T09:05:24Z","receivedAt":"2007-07-27T09:05:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 26 Jul 2007, Junio C Hamano wrote:\n\n> How about this? On top of your \"lockfile to keep symlink\" and\n> \"set-multivar to use lockfile protocol\" patches.\n\nLooks very good to me!\n\nCiao,\nDscho\n"},{"id":"48810","messageId":"f158199e0707271124t7a8e449dld5a7bb8af98151ac@mail.gmail.com","threadId":"9244","inReplyTo":"7v7iom5twd.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] use lockfile.c routines in git_commit_set_multivar()","fromName":"Bradford Smith","fromEmail":"bradford.carl.smith@gmail.com","sentAt":"2007-07-27T18:24:34Z","receivedAt":"2007-07-27T18:24:34Z","isPatch":true,"sender":{"key":"bradford.carl.smith@gmail.com","avatar":"https://gravatar.com/avatar/699930ad8ca562e38156ee60270b2fee830cc0cb5d94c2f70c6db4a705723c54?d=mp&s=160"},"body":"That's great!\n\nI've added this patch to my local branch and confirmed that all tests,\nincluding the new ones, run successfully.\n\nThanks!\n\nBradford\n\nOn 7/27/07, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n> > \"Bradford Smith\" writes:\n> >\n> >> FWIW, I have successfully run 'make test' and also verified that it\n> >> behaves as I expect with my ~/.gitconfig symlink (in conjunction with\n> >> the my other patch for resolving symlinks).\n> >\n> > Existing \"make test\" testsuite is not an appropriate thing to\n> > say this patch is safe, as we do not have much symlinking in the\n> > test git repository there. Care to add a new test or two?\n>\n> How about this? On top of your \"lockfile to keep symlink\" and\n> \"set-multivar to use lockfile protocol\" patches.\n>\n> ---\n>\n> t/t1300-repo-config.sh | 15 +++++++++++++++\n> 1 files changed, 15 insertions(+), 0 deletions(-)\n>\n> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh\n> index 1c43cc3..187ca2d 100755\n> --- a/t/t1300-repo-config.sh\n> +++ b/t/t1300-repo-config.sh\n> @@ -595,4 +595,19 @@ echo >>result\n>\n> test_expect_success '--null --get-regexp' 'cmp result expect'\n>\n> +test_expect_success 'symlinked configuration' '\n> +\n> + ln -s notyet myconfig &&\n> + GIT_CONFIG=myconfig git config test.frotz nitfol &&\n> + test -h myconfig &&\n> + test -f notyet &&\n> + test \"z$(GIT_CONFIG=notyet git config test.frotz)\" = znitfol &&\n> + GIT_CONFIG=myconfig git config test.xyzzy rezrov &&\n> + test -h myconfig &&\n> + test -f notyet &&\n> + test \"z$(GIT_CONFIG=notyet git config test.frotz)\" = znitfol &&\n> + test \"z$(GIT_CONFIG=notyet git config test.xyzzy)\" = zrezrov\n> +\n> +'\n> +\n> test_done\n>\n>\n"}]}