Re: [PATCH] blame: drop strdup of string literal

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Aug 3, 2016, 07:36 UTC
Message-ID: <CAPig+cT4x2ndMde8RMsseHcuSZpOoPgfXLRFUOyBK_iAz-xKyg@mail.gmail.com>
In-Reply-To: <20160802105252.jyl5x7eanttw2swj@sigill.intra.peff.net>

On Tue, Aug 2, 2016 at 6:52 AM, Jeff King <peff@peff.net> wrote:

Show 22 quoted lines

> On Tue, Jun 14, 2016 at 01:05:41AM -0400, Jeff King wrote:>
>> On Tue, Jun 14, 2016 at 12:32:15AM -0400, Eric Sunshine wrote:
>> > > +       struct string_list range_list = STRING_LIST_INIT_NODUP;
>> >
>> > Related to this series, there's an additional "fix" which ought to be
>> > made, probably as a separate patch. In particular, in cmd_blame():
>> >
>> >     if (lno && !range_list.nr)
>> >         string_list_append(&range_list, xstrdup("1"));
>> >
>> > which supplies a default range ("line 1 through end of file") if -L
>> > was not specified. I used xstrdup() on the literal "1" in 58dbfa2
>> > (blame: accept multiple -L ranges, 2013-08-06) to be consistent with
>> > parse_opt_string_list() which was unconditionally xstrdup'ing the
>> > argument (but no longer does as of patch 1/3 of this series).
>>
>> Yeah, I'd agree that this is a minor bug both before and after the
>> series due to the leak. Want to roll a patch on top?
>
> Here it is, just to tie up a loose end. I marked you as the author since
> the hard part was noticing the issue and explaining the history, which
> you already did above.

Thanks for picking up the slack, and my apologies for not being able to find time to submit the patch myself (my computer time is severely limited these days).

Show 13 quoted lines

> -- >8 --
> From: Eric Sunshine <sunshine@sunshineco.com>
> Subject: [PATCH] blame: drop strdup of string literal
>
> This strdup was added as part of 58dbfa2 (blame: accept
> multiple -L ranges, 2013-08-06) to be consistent with
> parse_opt_string_list(), which appends to the same list.
>
> But as of 7a7a517 (parse_opt_string_list: stop allocating
> new strings, 2016-06-13), we should stop using strdup (to
> match parse_opt_string_list, and for all the reasons
> described in that commit; namely that it does nothing useful
> and causes us to leak the memory).

A nice explanation, and the patch itself looks "obviously correct".

> Signed-off-by: Jeff King <peff@peff.net>

Signed-off-by: Eric Sunshine <sunshine@sunshineco.com>

Show 19 quoted lines

> ---
>  builtin/blame.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index ab66cde..29bd479 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -2805,7 +2805,7 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>         lno = prepare_lines(&sb);
>
>         if (lno && !range_list.nr)
> -               string_list_append(&range_list, xstrdup("1"));
> +               string_list_append(&range_list, "1");
>
>         anchor = 1;
>         range_set_init(&ranges, range_list.nr);
> --
> 2.9.2.670.g42e63de