On Tue, Dec 29, 2015 at 2:20 AM, Jeff King <peff@xxxxxxxx> wrote:
> 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@xxxxxxxx>
> ---
> 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.
> }
>
> #define THREAD_SHALLOW 1
> --
> 2.7.0.rc3.367.g09631da
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html