From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Tue, 05 Apr 2011 23:38:57 GMT
Subject: Re: [PATCH] config: support values longer than 1024 bytes
Message-ID: <BANLkTimXEK-_qZrSbV_nBofyyDfa22YEbQ@mail.gmail.com>
In-Reply-To: <1302046203-4408-1-git-send-email-kusmabite@gmail.com>

Ugh, I should learn not to send out patches way past my bed-time. This
should have been marked as RFC, and the subject should probably say
"1023 bytes", since one byte was needed for zero-termination.

Sorry for the noise.

On Wed, Apr 6, 2011 at 1:30 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> parse_value in config.c has a static buffer of 1024 bytes that it
> parse the value into. This can sometimes be a problem when a
> config file contains very long values.
>
> It's particularly amusing that git-config already is able to write
> such files, so it should probably be able to read them as well.
>
> Fix this by using a strbuf instead of a static buffer.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>
> Here's a proper-ish submission with a commit message and all.
>
> The rather awkward return statement with strdup("") is because
> strbuf_detach returns NULL when there's nothing allocated. Even
> worse, it returns an uninitialized string if the string has been
> initialized with a non-zero 'hint'.
>
> Perhaps I should change it to return a heap-allocated, empty
> string in those cases instead of working around it here?
>
>  config.c                |   26 ++++++++++++++------------
>  t/t1303-wacky-config.sh |    2 +-
>  2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/config.c b/config.c
> index 0abcada..f882f7c 100644
> --- a/config.c
> +++ b/config.c
> @@ -133,23 +133,22 @@ static int get_next_char(void)
>
>  static char *parse_value(void)
>  {
> -       static char value[1024];
> -       int quote = 0, comment = 0, len = 0, space = 0;
> +       struct strbuf value = STRBUF_INIT;
> +       int quote = 0, comment = 0, space = 0;
>
>        for (;;) {
>                int c = get_next_char();
> -               if (len >= sizeof(value) - 1)
> -                       return NULL;
>                if (c == '\n') {
>                        if (quote)
>                                return NULL;
> -                       value[len] = 0;
> -                       return value;
> +                       return value.len ?
> +                           strbuf_detach(&value, NULL) :
> +                           strdup("");

Should have used xstrdup()...

>                }
>                if (comment)
>                        continue;
>                if (isspace(c) && !quote) {
> -                       if (len)
> +                       if (value.len)
>                                space++;
>                        continue;
>                }
> @@ -160,7 +159,7 @@ static char *parse_value(void)
>                        }
>                }
>                for (; space; space--)
> -                       value[len++] = ' ';
> +                       strbuf_addch(&value, ' ');
>                if (c == '\\') {
>                        c = get_next_char();
>                        switch (c) {
> @@ -180,16 +179,17 @@ static char *parse_value(void)
>                                break;
>                        /* Reject unknown escape sequences */
>                        default:
> +                               strbuf_release(&value);
>                                return NULL;
>                        }
> -                       value[len++] = c;
> +                       strbuf_addch(&value, c);
>                        continue;
>                }
>                if (c == '"') {
>                        quote = 1-quote;
>                        continue;
>                }
> -               value[len++] = c;
> +               strbuf_addch(&value, c);
>        }
>  }
>
> @@ -200,7 +200,7 @@ static inline int iskeychar(int c)
>
>  static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
>  {
> -       int c;
> +       int c, ret;
>        char *value;
>
>        /* Get the full name */
> @@ -226,7 +226,9 @@ static int get_value(config_fn_t fn, void *data, char *name, unsigned int len)
>                if (!value)
>                        return -1;
>        }
> -       return fn(name, value, data);
> +       ret = fn(name, value, data);
> +       free(value);
> +       return ret;
>  }
>
>  static int get_extended_base_var(char *name, int baselen, int c)
> diff --git a/t/t1303-wacky-config.sh b/t/t1303-wacky-config.sh
> index 080117c..46103a1 100755
> --- a/t/t1303-wacky-config.sh
> +++ b/t/t1303-wacky-config.sh
> @@ -44,7 +44,7 @@ LONG_VALUE=$(printf "x%01021dx a" 7)
>  test_expect_success 'do not crash on special long config line' '
>        setup &&
>        git config section.key "$LONG_VALUE" &&
> -       check section.key "fatal: bad config file line 2 in .git/config"
> +       check section.key "$LONG_VALUE"
>  '
>
>  test_done
> --
> 1.7.4.msysgit.0.168.g33778
>
>