{"thread":{"id":"11295","subject":"config.c fixes","startedAt":"2007-12-14T19:28:52Z","lastAt":"2007-12-14T20:19:44Z","messageCount":3,"participants":["Kristian Høgsberg","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"63187","messageId":"20071214192852.GA24187@bitplanet.net","threadId":"11295","inReplyTo":null,"subject":"config.c fixes","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T19:28:52Z","receivedAt":"2007-12-14T19:28:52Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"Hi,\n\n[ This should have gone out as a cover letter for the patch series\n  but some how I didn't manage to make that work ]\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":"63193","messageId":"7v3au5rrjv.fsf@gitster.siamese.dyndns.org","threadId":"11295","inReplyTo":"20071214192852.GA24187@bitplanet.net","subject":"Re: config.c fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2007-12-14T19:43:00Z","receivedAt":"2007-12-14T19:43:00Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kristian Høgsberg <krh@redhat.com> writes:\n\n> While strace'ing builtin-clone I saw this horror:\n> I think the best solution is to just parse up the entire config file\n> up front and keep it in a data structure, make the changes and then\n> write it all out at the end.\n\nYeah, that was what I suggested a few times when other people have done\nconfig writing side, but without successfully getting past their skulls\n(it is not Linus's nor my code).  It's about time somebody started to\nclean up that mess.\n\nThe timing is a bit unfortunate, though.  I would have preferred to have\na week or so to cook this in 'next' before merging it part of -rc0.\n"},{"id":"63202","messageId":"1197663584.615.4.camel@hinata.boston.redhat.com","threadId":"11295","inReplyTo":"7v3au5rrjv.fsf@gitster.siamese.dyndns.org","subject":"Re: config.c fixes","fromName":"Kristian Høgsberg","fromEmail":"krh@redhat.com","sentAt":"2007-12-14T20:19:44Z","receivedAt":"2007-12-14T20:19:44Z","isPatch":false,"sender":{"key":"krh@redhat.com","avatar":"https://gravatar.com/avatar/763dee6f9594ac474f725b137a39565792928e583ddf59b32befc2907409027e?d=mp&s=160"},"body":"On Fri, 2007-12-14 at 11:43 -0800, Junio C Hamano wrote:\n> Kristian Høgsberg <krh@redhat.com> writes:\n> \n> > While strace'ing builtin-clone I saw this horror:\n> > I think the best solution is to just parse up the entire config file\n> > up front and keep it in a data structure, make the changes and then\n> > write it all out at the end.\n> \n> Yeah, that was what I suggested a few times when other people have done\n> config writing side, but without successfully getting past their skulls\n> (it is not Linus's nor my code).  It's about time somebody started to\n> clean up that mess.\n> \n> The timing is a bit unfortunate, though.  I would have preferred to have\n> a week or so to cook this in 'next' before merging it part of -rc0.\n\nRight, what I was describing above was more of a long term thing.  I\nsent two patches more suitable for 1.5.4 that fixes the double close and\nthe 1-byte writes in a less intrusive way.  Except as the Johanneses\npoint out, I can't use the lock as a local variable, but must allocate\nso the atexit handler doesn't break.  If it has to be allocated, the API\nshould help/enforce that.  I've sent out a couple of new patches that\nfixes this.\n\nKristian\n"}]}