Re: [PATCH 02/14] log: refactor add_header to drop some magic numbers

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Dec 31, 2015, 06:21 UTC
Message-ID: <CAPig+cRgK5Ey8WNDLLOnAhR+xh6NEHk-hGhGccj4SkZO-RV_-Q@mail.gmail.com>
In-Reply-To: <20151229072048.GB8842@sigill.intra.peff.net>

On Tue, Dec 29, 2015 at 2:20 AM, Jeff King <peff@peff.net> wrote:

Show 37 quoted lines

> We want to chomp newlines off the end of the "value" string.
> But because it's const, we must track its length rather than
> writing a NUL. This leads to us having to tweak that length
> later, to account for moving the pointer forward.
>
> Since we are about to create a copy of it anyway, let's just
> wait and chomp at the end.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/builtin/log.c b/builtin/log.c
> @@ -675,21 +675,18 @@ static struct string_list extra_cc;
>  static void add_header(const char *value)
>  {
>         struct string_list_item *item;
> -       int len = strlen(value);
> -       while (len && value[len - 1] == '\n')
> -               len--;
> +       size_t len;
>
> -       if (!strncasecmp(value, "to: ", 4)) {
> +       if (!strncasecmp(value, "to: ", 4))
>                 item = string_list_append(&extra_to, value + 4);
> -               len -= 4;
> -       } else if (!strncasecmp(value, "cc: ", 4)) {
> +       else if (!strncasecmp(value, "cc: ", 4))
>                 item = string_list_append(&extra_cc, value + 4);
> -               len -= 4;
> -       } else {
> +       else
>                 item = string_list_append(&extra_hdr, value);
> -       }
>
> -       item->string[len] = '\0';
> +       len = strlen(item->string);
> +       while (len && item->string[len - 1] == '\n')
> +               item->string[--len] = '\0';

Not a strong objection, but this implementation may make the reader wonder why NUL needs to be assigned to all "stripped" characters. I'd have expected to see:

    len = strlen(item->string);
    while (len && item->string[len - 1] == '\n')
        len--;
    item->string[len] = '\0';

which indicates clearly that this is a simple truncation rather than some odd NUL-fill operation, and is slightly more easy to reason about since it doesn't involve a pre-decrement as an array subscript.

Show 5 quoted lines

>  }
>
>  #define THREAD_SHALLOW 1
> --
> 2.7.0.rc3.367.g09631da