{"thread":{"id":"27004","subject":"[PATCH] config: support values longer than 1024 bytes","startedAt":"2011-04-05T23:30:03Z","lastAt":"2011-04-06T16:16:50Z","messageCount":6,"participants":["Erik Faye-Lund","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"165238","messageId":"1302046203-4408-1-git-send-email-kusmabite@gmail.com","threadId":"27004","inReplyTo":null,"subject":"[PATCH] config: support values longer than 1024 bytes","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-05T23:30:03Z","receivedAt":"2011-04-05T23:30:03Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"parse_value in config.c has a static buffer of 1024 bytes that it\nparse the value into. This can sometimes be a problem when a\nconfig file contains very long values.\n\nIt's particularly amusing that git-config already is able to write\nsuch files, so it should probably be able to read them as well.\n\nFix this by using a strbuf instead of a static buffer.\n\nSigned-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n---\n\nHere's a proper-ish submission with a commit message and all.\n\nThe rather awkward return statement with strdup(\"\") is because\nstrbuf_detach returns NULL when there's nothing allocated. Even\nworse, it returns an uninitialized string if the string has been\ninitialized with a non-zero 'hint'.\n\nPerhaps I should change it to return a heap-allocated, empty\nstring in those cases instead of working around it here?\n\n config.c                |   26 ++++++++++++++------------\n t/t1303-wacky-config.sh |    2 +-\n 2 files changed, 15 insertions(+), 13 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 0abcada..f882f7c 100644\n--- a/config.c\n+++ b/config.c\n@@ -133,23 +133,22 @@ static int get_next_char(void)\n \n static char *parse_value(void)\n {\n-\tstatic char value[1024];\n-\tint quote = 0, comment = 0, len = 0, space = 0;\n+\tstruct strbuf value = STRBUF_INIT;\n+\tint quote = 0, comment = 0, space = 0;\n \n \tfor (;;) {\n \t\tint c = get_next_char();\n-\t\tif (len >= sizeof(value) - 1)\n-\t\t\treturn NULL;\n \t\tif (c == '\\n') {\n \t\t\tif (quote)\n \t\t\t\treturn NULL;\n-\t\t\tvalue[len] = 0;\n-\t\t\treturn value;\n+\t\t\treturn value.len ?\n+\t\t\t    strbuf_detach(&value, NULL) :\n+\t\t\t    strdup(\"\");\n \t\t}\n \t\tif (comment)\n \t\t\tcontinue;\n \t\tif (isspace(c) && !quote) {\n-\t\t\tif (len)\n+\t\t\tif (value.len)\n \t\t\t\tspace++;\n \t\t\tcontinue;\n \t\t}\n@@ -160,7 +159,7 @@ static char *parse_value(void)\n \t\t\t}\n \t\t}\n \t\tfor (; space; space--)\n-\t\t\tvalue[len++] = ' ';\n+\t\t\tstrbuf_addch(&value, ' ');\n \t\tif (c == '\\\\') {\n \t\t\tc = get_next_char();\n \t\t\tswitch (c) {\n@@ -180,16 +179,17 @@ static char *parse_value(void)\n \t\t\t\tbreak;\n \t\t\t/* Reject unknown escape sequences */\n \t\t\tdefault:\n+\t\t\t\tstrbuf_release(&value);\n \t\t\t\treturn NULL;\n \t\t\t}\n-\t\t\tvalue[len++] = c;\n+\t\t\tstrbuf_addch(&value, c);\n \t\t\tcontinue;\n \t\t}\n \t\tif (c == '\"') {\n \t\t\tquote = 1-quote;\n \t\t\tcontinue;\n \t\t}\n-\t\tvalue[len++] = c;\n+\t\tstrbuf_addch(&value, c);\n \t}\n }\n \n@@ -200,7 +200,7 @@ static inline int iskeychar(int c)\n \n static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n {\n-\tint c;\n+\tint c, ret;\n \tchar *value;\n \n \t/* Get the full name */\n@@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n \t\tif (!value)\n \t\t\treturn -1;\n \t}\n-\treturn fn(name, value, data);\n+\tret = fn(name, value, data);\n+\tfree(value);\n+\treturn ret;\n }\n \n static int get_extended_base_var(char *name, int baselen, int c)\ndiff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\nindex 080117c..46103a1 100755\n--- a/t/t1303-wacky-config.sh\n+++ b/t/t1303-wacky-config.sh\n@@ -44,7 +44,7 @@ LONG_VALUE=$(printf \"x%01021dx a\" 7)\n test_expect_success 'do not crash on special long config line' '\n \tsetup &&\n \tgit config section.key \"$LONG_VALUE\" &&\n-\tcheck section.key \"fatal: bad config file line 2 in .git/config\"\n+\tcheck section.key \"$LONG_VALUE\"\n '\n \n test_done\n-- \n1.7.4.msysgit.0.168.g33778\n"},{"id":"165239","messageId":"BANLkTimXEK-_qZrSbV_nBofyyDfa22YEbQ@mail.gmail.com","threadId":"27004","inReplyTo":"1302046203-4408-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] config: support values longer than 1024 bytes","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-05T23:38:57Z","receivedAt":"2011-04-05T23:38:57Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"Ugh, I should learn not to send out patches way past my bed-time. This\nshould have been marked as RFC, and the subject should probably say\n\"1023 bytes\", since one byte was needed for zero-termination.\n\nSorry for the noise.\n\nOn Wed, Apr 6, 2011 at 1:30 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> parse_value in config.c has a static buffer of 1024 bytes that it\n> parse the value into. This can sometimes be a problem when a\n> config file contains very long values.\n>\n> It's particularly amusing that git-config already is able to write\n> such files, so it should probably be able to read them as well.\n>\n> Fix this by using a strbuf instead of a static buffer.\n>\n> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>\n> ---\n>\n> Here's a proper-ish submission with a commit message and all.\n>\n> The rather awkward return statement with strdup(\"\") is because\n> strbuf_detach returns NULL when there's nothing allocated. Even\n> worse, it returns an uninitialized string if the string has been\n> initialized with a non-zero 'hint'.\n>\n> Perhaps I should change it to return a heap-allocated, empty\n> string in those cases instead of working around it here?\n>\n>  config.c                |   26 ++++++++++++++------------\n>  t/t1303-wacky-config.sh |    2 +-\n>  2 files changed, 15 insertions(+), 13 deletions(-)\n>\n> diff --git a/config.c b/config.c\n> index 0abcada..f882f7c 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -133,23 +133,22 @@ static int get_next_char(void)\n>\n>  static char *parse_value(void)\n>  {\n> -       static char value[1024];\n> -       int quote = 0, comment = 0, len = 0, space = 0;\n> +       struct strbuf value = STRBUF_INIT;\n> +       int quote = 0, comment = 0, space = 0;\n>\n>        for (;;) {\n>                int c = get_next_char();\n> -               if (len >= sizeof(value) - 1)\n> -                       return NULL;\n>                if (c == '\\n') {\n>                        if (quote)\n>                                return NULL;\n> -                       value[len] = 0;\n> -                       return value;\n> +                       return value.len ?\n> +                           strbuf_detach(&value, NULL) :\n> +                           strdup(\"\");\n\nShould have used xstrdup()...\n\n>                }\n>                if (comment)\n>                        continue;\n>                if (isspace(c) && !quote) {\n> -                       if (len)\n> +                       if (value.len)\n>                                space++;\n>                        continue;\n>                }\n> @@ -160,7 +159,7 @@ static char *parse_value(void)\n>                        }\n>                }\n>                for (; space; space--)\n> -                       value[len++] = ' ';\n> +                       strbuf_addch(&value, ' ');\n>                if (c == '\\\\') {\n>                        c = get_next_char();\n>                        switch (c) {\n> @@ -180,16 +179,17 @@ static char *parse_value(void)\n>                                break;\n>                        /* Reject unknown escape sequences */\n>                        default:\n> +                               strbuf_release(&value);\n>                                return NULL;\n>                        }\n> -                       value[len++] = c;\n> +                       strbuf_addch(&value, c);\n>                        continue;\n>                }\n>                if (c == '\"') {\n>                        quote = 1-quote;\n>                        continue;\n>                }\n> -               value[len++] = c;\n> +               strbuf_addch(&value, c);\n>        }\n>  }\n>\n> @@ -200,7 +200,7 @@ static inline int iskeychar(int c)\n>\n>  static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n>  {\n> -       int c;\n> +       int c, ret;\n>        char *value;\n>\n>        /* Get the full name */\n> @@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n>                if (!value)\n>                        return -1;\n>        }\n> -       return fn(name, value, data);\n> +       ret = fn(name, value, data);\n> +       free(value);\n> +       return ret;\n>  }\n>\n>  static int get_extended_base_var(char *name, int baselen, int c)\n> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh\n> index 080117c..46103a1 100755\n> --- a/t/t1303-wacky-config.sh\n> +++ b/t/t1303-wacky-config.sh\n> @@ -44,7 +44,7 @@ LONG_VALUE=$(printf \"x%01021dx a\" 7)\n>  test_expect_success 'do not crash on special long config line' '\n>        setup &&\n>        git config section.key \"$LONG_VALUE\" &&\n> -       check section.key \"fatal: bad config file line 2 in .git/config\"\n> +       check section.key \"$LONG_VALUE\"\n>  '\n>\n>  test_done\n> --\n> 1.7.4.msysgit.0.168.g33778\n>\n>\n"},{"id":"165243","messageId":"20110406005223.GA10374@sigill.intra.peff.net","threadId":"27004","inReplyTo":"1302046203-4408-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] config: support values longer than 1024 bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-06T00:52:23Z","receivedAt":"2011-04-06T00:52:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2011 at 01:30:03AM +0200, Erik Faye-Lund wrote:\n\n> The rather awkward return statement with strdup(\"\") is because\n> strbuf_detach returns NULL when there's nothing allocated. Even\n> worse, it returns an uninitialized string if the string has been\n> initialized with a non-zero 'hint'.\n\nThat seems like two bugs in the strbuf code.\n\nI would expect strbuf_detach to _always_ return an allocated buffer,\neven if it is xstrdup(\"\"). Though the code in strbuf_detach is explicit\nabout returning NULL rather than slopbuf, so perhaps there is a case\nwhere it is useful.\n\nBut for the other, one of the invariants of strbuf is that the string is\nalways NUL-terminated. So I would expect strbuf_init to properly\nNUL-terminate after growing based on the hint.\n\n>  static char *parse_value(void)\n>  {\n> -\tstatic char value[1024];\n> -\tint quote = 0, comment = 0, len = 0, space = 0;\n> +\tstruct strbuf value = STRBUF_INIT;\n> +\tint quote = 0, comment = 0, space = 0;\n\nOne thing you could about the strbuf_detach thing is to just use a\nsingle static strbuf instead of the static array, and just reset its\nlength back to zero on each call. So:\n\n  static struct strbuf value = STRBUF_INIT;\n  ...\n  strbuf_reset(&value);\n\n> -\t\t\tvalue[len] = 0;\n> -\t\t\treturn value;\n> +\t\t\treturn value.len ?\n> +\t\t\t    strbuf_detach(&value, NULL) :\n> +\t\t\t    strdup(\"\");\n\nThen this just becomes:\n\n  return value.buf;\n\n>  \t\t\tdefault:\n> +\t\t\t\tstrbuf_release(&value);\n>  \t\t\t\treturn NULL;\n\nAnd you can drop the release.\n\n> @@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n>  \t\tif (!value)\n>  \t\t\treturn -1;\n>  \t}\n> -\treturn fn(name, value, data);\n> +\tret = fn(name, value, data);\n> +\tfree(value);\n> +\treturn ret;\n\nAnd drop this hunk, since callers no longer need to free.\n\nI do wonder, though, if we could be reusing the unquote_c_style()\nfunction in quote.c. They are obviously similar, but I haven't checked\nif there is more going on in the config code.\n\n-Peff\n"},{"id":"165253","messageId":"BANLkTim0N0kM+OX5Tztz-Kh+eRRsNixX0A@mail.gmail.com","threadId":"27004","inReplyTo":"20110406005223.GA10374@sigill.intra.peff.net","subject":"Re: [PATCH] config: support values longer than 1024 bytes","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-06T09:10:42Z","receivedAt":"2011-04-06T09:10:42Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 6, 2011 at 2:52 AM, Jeff King <peff@peff.net> wrote:\n> On Wed, Apr 06, 2011 at 01:30:03AM +0200, Erik Faye-Lund wrote:\n>\n>> The rather awkward return statement with strdup(\"\") is because\n>> strbuf_detach returns NULL when there's nothing allocated. Even\n>> worse, it returns an uninitialized string if the string has been\n>> initialized with a non-zero 'hint'.\n>\n> That seems like two bugs in the strbuf code.\n>\n> I would expect strbuf_detach to _always_ return an allocated buffer,\n> even if it is xstrdup(\"\"). Though the code in strbuf_detach is explicit\n> about returning NULL rather than slopbuf, so perhaps there is a case\n> where it is useful.\n\nYeah, this is why I didn't just go for it. Looking through history, it\nseems that the reason why it explicitly returns NULL when there's\nnothing allocated is that it used to do so \"by artifact\" before\nb315c5c (\"strbuf change: be sure ->buf is never ever NULL.\"), and it\nlooks like the author didn't want to change the semantics. Fair\nenough, changing this takes a lot of code-reading to verify that there\nisn't any call-sites that depend on this behavior.\n\n>\n> But for the other, one of the invariants of strbuf is that the string is\n> always NUL-terminated. So I would expect strbuf_init to properly\n> NUL-terminate after growing based on the hint.\n\nI agree. An unterminated yet non-NULL return from strbuf_detach is\njust dangerous behavior. Something like this should probably be\napplied:\n\n---8<---\ndiff --git a/strbuf.c b/strbuf.c\nindex 77444a9..538035a 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)\n  * buf is non NULL and ->buf is NUL terminated even for a freshly\n  * initialized strbuf.\n  */\n-char strbuf_slopbuf[1];\n+char strbuf_slopbuf[1] = { '\\0' };\n\n void strbuf_init(struct strbuf *sb, size_t hint)\n {\n \tsb->alloc = sb->len = 0;\n \tsb->buf = strbuf_slopbuf;\n-\tif (hint)\n+\tif (hint) {\n \t\tstrbuf_grow(sb, hint);\n+\t\tsb->buf[0] = '\\0';\n+\t}\n }\n\n void strbuf_release(struct strbuf *sb)\n---8<---\n\n>\n>>  static char *parse_value(void)\n>>  {\n>> -     static char value[1024];\n>> -     int quote = 0, comment = 0, len = 0, space = 0;\n>> +     struct strbuf value = STRBUF_INIT;\n>> +     int quote = 0, comment = 0, space = 0;\n>\n> One thing you could about the strbuf_detach thing is to just use a\n> single static strbuf instead of the static array, and just reset its\n> length back to zero on each call. So:\n>\n>  static struct strbuf value = STRBUF_INIT;\n>  ...\n>  strbuf_reset(&value);\n>\n>> -                     value[len] = 0;\n>> -                     return value;\n>> +                     return value.len ?\n>> +                         strbuf_detach(&value, NULL) :\n>> +                         strdup(\"\");\n>\n> Then this just becomes:\n>\n>  return value.buf;\n>\n>>                       default:\n>> +                             strbuf_release(&value);\n>>                               return NULL;\n>\n> And you can drop the release.\n>\n>> @@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)\n>>               if (!value)\n>>                       return -1;\n>>       }\n>> -     return fn(name, value, data);\n>> +     ret = fn(name, value, data);\n>> +     free(value);\n>> +     return ret;\n>\n> And drop this hunk, since callers no longer need to free.\n>\n\nYeah, and this was what I tried first.\n\nI dropped that approach because I misread the code of strbuf_setlen so\nI thought it ended up freeing the buffer (which would have lost the\ngain we get from making it static). This is wrong, it does not do\nthat. But this brings a new issue: leaving potentially huge blocks of\nmemory (especially since this patch is about long lines) allocated\ninside a function can be a bit nasty. But it's probably not a big deal\nhere, the blocks are probably small enough to \"leak\" without worries,\nunless you have a downright evil config file (which I suspect could\nonly happen due to disk-corruption, in which case you're going to have\nmuch much bigger problems). And that problem can also be fixed by\ngiving the buffer file-local scope, and adding a cleanup-function that\ngets called when config-parsing is finished. But it's probably not\neven worth the hassle.\n\nI guess it's my background from handheld/low-power device development\nthat makes me worry too much about problems like these :P\n\nIn other words: I think you're right, it's a much better approach.\nLess allocations, less penalty on the start-up time for every little\ngit-command.\n\n> I do wonder, though, if we could be reusing the unquote_c_style()\n> function in quote.c. They are obviously similar, but I haven't checked\n> if there is more going on in the config code.\n\nHmm, this is an interesting suggestion. It would be a part of a bigger\nchange though: unquote_c_style requires it's input to be in memory,\nwhile parse_value uses a function called get_next_char to feed the\nparser. So we'd either have to read the entire file into memory, or\nfind some way to read the file line-by-line while handling \\n-escaping\ncorrectly.\n\nIt also seems like there's differences in what kind of escaping and\nnormalization the two functions handle; unquote_c_style handles more\nescaped character sequences, while parse_value normalize all\nnon-escaped space-characters ('\\t' et. al) into SP. This might not be\nsuch a big problem in reality.\n"},{"id":"165279","messageId":"20110406153509.GA1864@sigill.intra.peff.net","threadId":"27004","inReplyTo":"BANLkTim0N0kM+OX5Tztz-Kh+eRRsNixX0A@mail.gmail.com","subject":"Re: [PATCH] config: support values longer than 1024 bytes","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-04-06T15:35:09Z","receivedAt":"2011-04-06T15:35:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Apr 06, 2011 at 11:10:42AM +0200, Erik Faye-Lund wrote:\n\n> > But for the other, one of the invariants of strbuf is that the string is\n> > always NUL-terminated. So I would expect strbuf_init to properly\n> > NUL-terminate after growing based on the hint.\n> \n> I agree. An unterminated yet non-NULL return from strbuf_detach is\n> just dangerous behavior. Something like this should probably be\n> applied:\n> \n> ---8<---\n> diff --git a/strbuf.c b/strbuf.c\n> index 77444a9..538035a 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)\n>   * buf is non NULL and ->buf is NUL terminated even for a freshly\n>   * initialized strbuf.\n>   */\n> -char strbuf_slopbuf[1];\n> +char strbuf_slopbuf[1] = { '\\0' };\n\nThis hunk is redundant. slopbuf will already be initialized to 0.\n\n>  void strbuf_init(struct strbuf *sb, size_t hint)\n>  {\n>  \tsb->alloc = sb->len = 0;\n>  \tsb->buf = strbuf_slopbuf;\n> -\tif (hint)\n> +\tif (hint) {\n>  \t\tstrbuf_grow(sb, hint);\n> +\t\tsb->buf[0] = '\\0';\n> +\t}\n>  }\n\nBut this one is the right fix.\n\n> that. But this brings a new issue: leaving potentially huge blocks of\n> memory (especially since this patch is about long lines) allocated\n> inside a function can be a bit nasty. But it's probably not a big deal\n\nYeah. It's just one block, though, and in the normal case it is probably\nonly about 80 characters. So it is more efficient than what's there now. :)\n\nSomebody could have some gigantic value, though, and yes, we'll grow to\nthe biggest one and never free that memory. You could also have\nparse_value take a strbuf parameter to output into, and then free it\nafter config reading is done.\n\n> In other words: I think you're right, it's a much better approach.\n> Less allocations, less penalty on the start-up time for every little\n> git-command.\n\nI doubt the efficiency increase is measurable. We end up xstrdup'ing\nquite a few of the values in the config callbacks anyway. I would do\nwhatever seems most natural for reading/writing the code.\n\n> > I do wonder, though, if we could be reusing the unquote_c_style()\n> > function in quote.c. They are obviously similar, but I haven't checked\n> > if there is more going on in the config code.\n> \n> Hmm, this is an interesting suggestion. It would be a part of a bigger\n> change though: unquote_c_style requires it's input to be in memory,\n> while parse_value uses a function called get_next_char to feed the\n> parser. So we'd either have to read the entire file into memory, or\n> find some way to read the file line-by-line while handling \\n-escaping\n> correctly.\n> \n> It also seems like there's differences in what kind of escaping and\n> normalization the two functions handle; unquote_c_style handles more\n> escaped character sequences, while parse_value normalize all\n> non-escaped space-characters ('\\t' et. al) into SP. This might not be\n> such a big problem in reality.\n\nThis was just a random thought that I had, and I didn't investigate it\nhow hard it would be.  If it turns out to be too much trouble, just\nforget it.\n\n-Peff\n"},{"id":"165285","messageId":"BANLkTikuzfZrv+N0Qm7utfYF6fsYQn7Zcg@mail.gmail.com","threadId":"27004","inReplyTo":"20110406153509.GA1864@sigill.intra.peff.net","subject":"Re: [PATCH] config: support values longer than 1024 bytes","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2011-04-06T16:16:50Z","receivedAt":"2011-04-06T16:16:50Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Apr 6, 2011 at 5:35 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, Apr 06, 2011 at 11:10:42AM +0200, Erik Faye-Lund wrote:\n>\n>> > But for the other, one of the invariants of strbuf is that the string is\n>> > always NUL-terminated. So I would expect strbuf_init to properly\n>> > NUL-terminate after growing based on the hint.\n>>\n>> I agree. An unterminated yet non-NULL return from strbuf_detach is\n>> just dangerous behavior. Something like this should probably be\n>> applied:\n>>\n>> ---8<---\n>> diff --git a/strbuf.c b/strbuf.c\n>> index 77444a9..538035a 100644\n>> --- a/strbuf.c\n>> +++ b/strbuf.c\n>> @@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)\n>>   * buf is non NULL and ->buf is NUL terminated even for a freshly\n>>   * initialized strbuf.\n>>   */\n>> -char strbuf_slopbuf[1];\n>> +char strbuf_slopbuf[1] = { '\\0' };\n>\n> This hunk is redundant. slopbuf will already be initialized to 0.\n\nRight, silly me. I somehow thought that implicit zero-initialization\nonly applied to static variables, but K&R tells me I was wrong. Thanks\nfor pointing it out :)\n\n>\n>>  void strbuf_init(struct strbuf *sb, size_t hint)\n>>  {\n>>       sb->alloc = sb->len = 0;\n>>       sb->buf = strbuf_slopbuf;\n>> -     if (hint)\n>> +     if (hint) {\n>>               strbuf_grow(sb, hint);\n>> +             sb->buf[0] = '\\0';\n>> +     }\n>>  }\n>\n> But this one is the right fix.\n>\n\nOK, I'll turn this into a two-patch series, then.\n\n>> that. But this brings a new issue: leaving potentially huge blocks of\n>> memory (especially since this patch is about long lines) allocated\n>> inside a function can be a bit nasty. But it's probably not a big deal\n>\n> Yeah. It's just one block, though, and in the normal case it is probably\n> only about 80 characters. So it is more efficient than what's there now. :)\n>\n> Somebody could have some gigantic value, though, and yes, we'll grow to\n> the biggest one and never free that memory. You could also have\n> parse_value take a strbuf parameter to output into, and then free it\n> after config reading is done.\n>\n>> In other words: I think you're right, it's a much better approach.\n>> Less allocations, less penalty on the start-up time for every little\n>> git-command.\n>\n> I doubt the efficiency increase is measurable. We end up xstrdup'ing\n> quite a few of the values in the config callbacks anyway. I would do\n> whatever seems most natural for reading/writing the code.\n>\n\nI think I'll just leave the single leak in. Since it would allocate\nthe buffer lazily, it would really only \"waste\" more memory than the\nexisting implementation when the old implementation would fail. So my\nconscience is clear ;)\n\n>> > I do wonder, though, if we could be reusing the unquote_c_style()\n>> > function in quote.c. They are obviously similar, but I haven't checked\n>> > if there is more going on in the config code.\n>>\n>> Hmm, this is an interesting suggestion. It would be a part of a bigger\n>> change though: unquote_c_style requires it's input to be in memory,\n>> while parse_value uses a function called get_next_char to feed the\n>> parser. So we'd either have to read the entire file into memory, or\n>> find some way to read the file line-by-line while handling \\n-escaping\n>> correctly.\n>>\n>> It also seems like there's differences in what kind of escaping and\n>> normalization the two functions handle; unquote_c_style handles more\n>> escaped character sequences, while parse_value normalize all\n>> non-escaped space-characters ('\\t' et. al) into SP. This might not be\n>> such a big problem in reality.\n>\n> This was just a random thought that I had, and I didn't investigate it\n> how hard it would be.  If it turns out to be too much trouble, just\n> forget it.\n>\n\nYeah. I think I'll skip it, since it isn't dead obvious.\n\nThanks for the input, I'll cook up a new version soon.\n"}]}