# config.c fixes 3 messages from 2007-12-14 to 2007-12-14. Participants: Kristian Høgsberg, Junio C Hamano. Thread: https://gitlist.dev/t/11295 ## Kristian Høgsberg, 2007-12-14 19:28 Subject: config.c fixes Message-ID: <20071214192852.GA24187@bitplanet.net> URL: https://gitlist.dev/e/20071214192852.GA24187%40bitplanet.net ``` Hi, [ This should have gone out as a cover letter for the patch series but some how I didn't manage to make that work ] While strace'ing builtin-clone I saw this horror: ... write(3, "remote", 6) = 6 write(3, " = ", 3) = 3 write(3, "o", 1) = 1 write(3, "r", 1) = 1 write(3, "i", 1) = 1 write(3, "g", 1) = 1 write(3, "i", 1) = 1 write(3, "n", 1) = 1 write(3, "\n", 1) = 1 munmap(0xb7f72000, 102) = 0 close(3) = 0 close(3) = -1 EBADF (Bad file descriptor) ... That's just terrible. And hey, it turns out the code is terrible too. I think the best solution is to just parse up the entire config file up front and keep it in a data structure, make the changes and then write it all out at the end. That makes it cheap to make a series of changes too: config = open_config(file); set_key(config, key1, value1); set_key(config, key2, value2); set_key(config, key3, value3); commit_config(config); Or something. Anyway, for 1.5.4 I wrote the following two patches that fixes the 1-bytes writes and the double close. config.c | 113 +++++++++++++++++++++++-------------------------------------- 1 files changed, 43 insertions(+), 70 deletions(-) cheers, Kristian ``` ## Junio C Hamano, 2007-12-14 19:43 Subject: Re: config.c fixes Message-ID: <7v3au5rrjv.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3au5rrjv.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20071214192852.GA24187@bitplanet.net> ``` Kristian Høgsberg writes: > While strace'ing builtin-clone I saw this horror: > I think the best solution is to just parse up the entire config file > up front and keep it in a data structure, make the changes and then > write it all out at the end. Yeah, that was what I suggested a few times when other people have done config writing side, but without successfully getting past their skulls (it is not Linus's nor my code). It's about time somebody started to clean up that mess. The timing is a bit unfortunate, though. I would have preferred to have a week or so to cook this in 'next' before merging it part of -rc0. ``` ## Kristian Høgsberg, 2007-12-14 20:19 Subject: Re: config.c fixes Message-ID: <1197663584.615.4.camel@hinata.boston.redhat.com> URL: https://gitlist.dev/e/1197663584.615.4.camel%40hinata.boston.redhat.com In-Reply-To: <7v3au5rrjv.fsf@gitster.siamese.dyndns.org> ``` On Fri, 2007-12-14 at 11:43 -0800, Junio C Hamano wrote: > Kristian Høgsberg writes: > > > While strace'ing builtin-clone I saw this horror: > > I think the best solution is to just parse up the entire config file > > up front and keep it in a data structure, make the changes and then > > write it all out at the end. > > Yeah, that was what I suggested a few times when other people have done > config writing side, but without successfully getting past their skulls > (it is not Linus's nor my code). It's about time somebody started to > clean up that mess. > > The timing is a bit unfortunate, though. I would have preferred to have > a week or so to cook this in 'next' before merging it part of -rc0. Right, what I was describing above was more of a long term thing. I sent two patches more suitable for 1.5.4 that fixes the double close and the 1-byte writes in a less intrusive way. Except as the Johanneses point out, I can't use the lock as a local variable, but must allocate so the atexit handler doesn't break. If it has to be allocated, the API should help/enforce that. I've sent out a couple of new patches that fixes this. Kristian ```