{"thread":{"id":"11293","subject":"[PATCH 2/2] Use a strbuf for building up section header and key/value pair strings.","startedAt":"2007-12-14T19:22:35Z","lastAt":"2007-12-14T21:18:30Z","messageCount":8,"participants":["Kristian Høgsberg","Johannes Schindelin","Johannes Sixt","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"63186","messageId":"1197660157-24109-1-git-send-email-krh@redhat.com","threadId":"11293","inReplyTo":null,"subject":"config.c fixes","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T19:22:35Z","receivedAt":"2007-12-14T19:22:35Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"Hi,\n\nWhile strace'ing builtin-clone I saw this horror:\n\n...\nwrite(3, \"remote\", 6)                   = 6\nwrite(3, \" = \", 3)                      = 3\nwrite(3, \"o\", 1)                        = 1\nwrite(3, \"r\", 1)                        = 1\nwrite(3, \"i\", 1)                        = 1\nwrite(3, \"g\", 1)                        = 1\nwrite(3, \"i\", 1)                        = 1\nwrite(3, \"n\", 1)                        = 1\nwrite(3, \"\\n\", 1)                       = 1\nmunmap(0xb7f72000, 102)                 = 0\nclose(3)                                = 0\nclose(3)                                = -1 EBADF (Bad file descriptor)\n...\n\nThat's just terrible.  And hey, it turns out the code is terrible too.\nI think the best solution is to just parse up the entire config file\nup front and keep it in a data structure, make the changes and then\nwrite it all out at the end.  That makes it cheap to make a series of\nchanges too:\n\n\tconfig = open_config(file);\n\tset_key(config, key1, value1);\n\tset_key(config, key2, value2);\n\tset_key(config, key3, value3);\n\tcommit_config(config);\n\nOr something.  Anyway, for 1.5.4 I wrote the following two patches that\nfixes the 1-bytes writes and the double close.\n\n config.c |  113 +++++++++++++++++++++++--------------------------------------\n 1 files changed, 43 insertions(+), 70 deletions(-)\n\ncheers,\nKristian\n"},{"id":"63184","messageId":"1197660157-24109-2-git-send-email-krh@redhat.com","threadId":"11293","inReplyTo":"1197660157-24109-1-git-send-email-krh@redhat.com","subject":"[PATCH 1/2] Fix config lockfile handling.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T19:22:36Z","receivedAt":"2007-12-14T19:22:36Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"When we commit or roll back the lock file the fd is automatically closed,\nso don't do that again.  Also, just keep the lock on the stack.\n\nSigned-off-by: Kristian Høgsberg <krh@redhat.com>\n---\n config.c |   22 +++++-----------------\n 1 files changed, 5 insertions(+), 17 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 49d2b42..0725563 100644\n--- a/config.c\n+++ b/config.c\n@@ -754,7 +754,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-\tstruct lock_file *lock = NULL;\n+\tstruct lock_file lock;\n \tconst char* last_dot = strrchr(key, '.');\n \n \tconfig_filename = getenv(CONFIG_ENVIRONMENT);\n@@ -811,8 +811,7 @@ int git_config_set_multivar(const char* key, const char* value,\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-\tlock = xcalloc(sizeof(struct lock_file), 1);\n-\tfd = hold_lock_file_for_update(lock, config_filename, 0);\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@@ -955,28 +954,17 @@ int git_config_set_multivar(const char* key, const char* value,\n \t\tmunmap(contents, contents_sz);\n \t}\n \n-\tif (close(fd) || commit_lock_file(lock) < 0) {\n+\tif (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+\t/* If we already committed the lock file, the rollback is a no-op. */\n+\trollback_lock_file(&lock);\n \tfree(config_filename);\n \treturn ret;\n \n-- \n1.5.3.4\n"},{"id":"63183","messageId":"1197660157-24109-3-git-send-email-krh@redhat.com","threadId":"11293","inReplyTo":"1197660157-24109-2-git-send-email-krh@redhat.com","subject":"[PATCH 2/2] Use a strbuf for building up section header and key/value pair strings.","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T19:22:37Z","receivedAt":"2007-12-14T19:22:37Z","isPatch":true,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"Avoids horrible 1-byte write(2) calls and cleans up the logic a bit.\n\nSigned-off-by: Kristian Høgsberg <krh@redhat.com>\n---\n config.c |   91 ++++++++++++++++++++++++++------------------------------------\n 1 files changed, 38 insertions(+), 53 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 0725563..3392bc4 100644\n--- a/config.c\n+++ b/config.c\n@@ -610,46 +610,36 @@ static int write_error(void)\n \n static int store_write_section(int fd, const char* key)\n {\n-\tconst char *dot = strchr(key, '.');\n-\tint len1 = store.baselen, len2 = -1;\n+\tconst char *dot;\n+\tint i, success;\n+\tstruct strbuf sb;\n \n-\tdot = strchr(key, '.');\n+\tstrbuf_init(&sb, 0);\n+\tdot = memchr(key, '.', store.baselen);\n \tif (dot) {\n-\t\tint dotlen = dot - key;\n-\t\tif (dotlen < len1) {\n-\t\t\tlen2 = len1 - dotlen - 1;\n-\t\t\tlen1 = dotlen;\n+\t\tstrbuf_addf(&sb, \"[%.*s \\\"\", dot - key, key);\n+\t\tfor (i = dot - key + 1; i < store.baselen; i++) {\n+\t\t\tif (key[i] == '\"')\n+\t\t\t\tstrbuf_addch(&sb, '\\\\');\n+\t\t\tstrbuf_addch(&sb, key[i]);\n \t\t}\n+\t\tstrbuf_addstr(&sb, \"\\\"]\\n\");\n+\t} else {\n+\t\tstrbuf_addf(&sb, \"[%.*s]\\n\", store.baselen, key);\n \t}\n \n-\tif (write_in_full(fd, \"[\", 1) != 1 ||\n-\t    write_in_full(fd, key, len1) != len1)\n-\t\treturn 0;\n-\tif (len2 >= 0) {\n-\t\tif (write_in_full(fd, \" \\\"\", 2) != 2)\n-\t\t\treturn 0;\n-\t\twhile (--len2 >= 0) {\n-\t\t\tunsigned char c = *++dot;\n-\t\t\tif (c == '\"')\n-\t\t\t\tif (write_in_full(fd, \"\\\\\", 1) != 1)\n-\t\t\t\t\treturn 0;\n-\t\t\tif (write_in_full(fd, &c, 1) != 1)\n-\t\t\t\treturn 0;\n-\t\t}\n-\t\tif (write_in_full(fd, \"\\\"\", 1) != 1)\n-\t\t\treturn 0;\n-\t}\n-\tif (write_in_full(fd, \"]\\n\", 2) != 2)\n-\t\treturn 0;\n+\tsuccess = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tstrbuf_release(&sb);\n \n-\treturn 1;\n+\treturn success;\n }\n \n static int store_write_pair(int fd, const char* key, const char* value)\n {\n-\tint i;\n-\tint length = strlen(key+store.baselen+1);\n-\tint quote = 0;\n+\tint i, success;\n+\tint length = strlen(key + store.baselen + 1);\n+\tconst char *quote = \"\";\n+\tstruct strbuf sb;\n \n \t/*\n \t * Check to see if the value needs to be surrounded with a dq pair.\n@@ -659,43 +649,38 @@ static int store_write_pair(int fd, const char* key, const char* value)\n \t * configuration parser.\n \t */\n \tif (value[0] == ' ')\n-\t\tquote = 1;\n+\t\tquote = \"\\\"\";\n \tfor (i = 0; value[i]; i++)\n \t\tif (value[i] == ';' || value[i] == '#')\n-\t\t\tquote = 1;\n-\tif (i && value[i-1] == ' ')\n-\t\tquote = 1;\n+\t\t\tquote = \"\\\"\";\n+\tif (i && value[i - 1] == ' ')\n+\t\tquote = \"\\\"\";\n+\n+\tstrbuf_init(&sb, 0);\n+\tstrbuf_addf(&sb, \"\\t%.*s = %s\", \n+\t\t    length, key + store.baselen + 1, quote);\n \n-\tif (write_in_full(fd, \"\\t\", 1) != 1 ||\n-\t    write_in_full(fd, key+store.baselen+1, length) != length ||\n-\t    write_in_full(fd, \" = \", 3) != 3)\n-\t\treturn 0;\n-\tif (quote && write_in_full(fd, \"\\\"\", 1) != 1)\n-\t\treturn 0;\n \tfor (i = 0; value[i]; i++)\n \t\tswitch (value[i]) {\n \t\tcase '\\n':\n-\t\t\tif (write_in_full(fd, \"\\\\n\", 2) != 2)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addstr(&sb, \"\\\\n\");\n \t\t\tbreak;\n \t\tcase '\\t':\n-\t\t\tif (write_in_full(fd, \"\\\\t\", 2) != 2)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addstr(&sb, \"\\\\t\");\n \t\t\tbreak;\n \t\tcase '\"':\n \t\tcase '\\\\':\n-\t\t\tif (write_in_full(fd, \"\\\\\", 1) != 1)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addch(&sb, '\\\\');\n \t\tdefault:\n-\t\t\tif (write_in_full(fd, value+i, 1) != 1)\n-\t\t\t\treturn 0;\n+\t\t\tstrbuf_addch(&sb, value[i]);\n \t\t\tbreak;\n \t\t}\n-\tif (quote && write_in_full(fd, \"\\\"\", 1) != 1)\n-\t\treturn 0;\n-\tif (write_in_full(fd, \"\\n\", 1) != 1)\n-\t\treturn 0;\n-\treturn 1;\n+\tstrbuf_addf(&sb, \"%s\\n\", quote);\n+\n+\tsuccess = write_in_full(fd, sb.buf, sb.len) == sb.len;\n+\tstrbuf_release(&sb);\n+\n+\treturn success;\n }\n \n static ssize_t find_beginning_of_line(const char* contents, size_t size,\n-- \n1.5.3.4\n"},{"id":"63191","messageId":"Pine.LNX.4.64.0712141928240.27959@racer.site","threadId":"11293","inReplyTo":"1197660157-24109-2-git-send-email-krh@redhat.com","subject":"Re: [PATCH 1/2] Fix config lockfile handling.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-14T19:29:03Z","receivedAt":"2007-12-14T19:29:03Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 14 Dec 2007, Kristian Høgsberg wrote:\n\n> -\tstruct lock_file *lock = NULL;\n> +\tstruct lock_file lock;\n\nAFAICT this cannot work.  At least not reliably.  An atexit() handler will \naccess all (even closed) lockfiles.\n\nCiao,\nDscho"},{"id":"63192","messageId":"20071214193216.13C795A873@dx.sixt.local","threadId":"11293","inReplyTo":"1197660157-24109-2-git-send-email-krh@redhat.com","subject":"Re: [PATCH 1/2] Fix config lockfile handling.","fromName":"Johannes Sixt","fromEmail":"johannes.sixt@telecom.at","sentAt":"2007-12-14T19:32:15Z","receivedAt":"2007-12-14T19:32:15Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Kristian Høgsberg wrote:\n\n> When we commit or roll back the lock file the fd is automatically closed,\n> so don't do that again.  Also, just keep the lock on the stack.\n\nIIRC, locks are accessed from atexit(), e.g. during a die(). So you must not\nput one on the stack.\n\n-- Hannes\n"},{"id":"63196","messageId":"7vmysdqbui.fsf@gitster.siamese.dyndns.org","threadId":"11293","inReplyTo":"Pine.LNX.4.64.0712141928240.27959@racer.site","subject":"Re: [PATCH 1/2] Fix config lockfile handling.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T20:07:33Z","receivedAt":"2007-12-14T20:07:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi,\n>\n> On Fri, 14 Dec 2007, Kristian Høgsberg wrote:\n>\n>> -\tstruct lock_file *lock = NULL;\n>> +\tstruct lock_file lock;\n>\n> AFAICT this cannot work.  At least not reliably.  An atexit() handler will \n> access all (even closed) lockfiles.\n\nCorrect.  It cannot be on the stack.\n"},{"id":"63198","messageId":"Pine.LNX.4.64.0712142015240.27959@racer.site","threadId":"11293","inReplyTo":"7vmysdqbui.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 1/2] Fix config lockfile handling.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2007-12-14T20:15:54Z","receivedAt":"2007-12-14T20:15:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 14 Dec 2007, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Fri, 14 Dec 2007, Kristian Høgsberg wrote:\n> >\n> >> -\tstruct lock_file *lock = NULL;\n> >> +\tstruct lock_file lock;\n> >\n> > AFAICT this cannot work.  At least not reliably.  An atexit() handler \n> > will access all (even closed) lockfiles.\n> \n> Correct.  It cannot be on the stack.\n\nNote that this behaviour will be another obstacle to libification.\n\nCiao,\nDscho\n"},{"id":"63210","messageId":"7vwsrhotzt.fsf@gitster.siamese.dyndns.org","threadId":"11293","inReplyTo":"Pine.LNX.4.64.0712142015240.27959@racer.site","subject":"Re: [PATCH 1/2] Fix config lockfile handling.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T21:18:30Z","receivedAt":"2007-12-14T21:18:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> > AFAICT this cannot work.  At least not reliably.  An atexit() handler \n>> > will access all (even closed) lockfiles.\n>> \n>> Correct.  It cannot be on the stack.\n>\n> Note that this behaviour will be another obstacle to libification.\n\nBy your definition of \"obstacle\", there is no work at all in\nlibification if the system is obstacle free.\n\nLibification is all about removing run-once-and-exit and atexit() is\njust a part of it.\n\nI think we can do this step-by-step by first introducing a new function\n\"get_lockfile()\" that takes a list of active lockfiles (perhaps that\nwould be a part of \"client context\" thing in the library) and gives back\na lockfile structure allocated on heap, registers it to the list of\nlockfiles that need to be eventually cleaned up, and another function\n\"rollback_lockfiles()\" that take the list of lockfiles in the \"client\ncontext\" and rolls them all back.  Once there is such a \"client contex\",\nin the current unlibified \"main\" routines can then declare a global\nclient context, obtain and use lockfiles for that context, and directly\ncall rollback_lockfiles() from the atexit() handler.\n\nBut that is all post 1.5.4.\n"}]}