From: Derrick Stolee <stolee@gmail.com>
Date: Mon, 14 May 2018 13:27:21 GMT
Subject: Re: [PATCH v2 01/12] commit-graph: add 'verify' subcommand
Message-ID: <3cd99f4e-82cb-43e8-53eb-e8d38d449e57@gmail.com>
In-Reply-To: <CAN0heSotYYgrwMLT1yyFVk4VEZ4OQKF_Xwm9-wZosJ6_-gAVBg@mail.gmail.com>

On 5/12/2018 9:31 AM, Martin Ågren wrote:
> On 11 May 2018 at 23:15, Derrick Stolee <dstolee@microsoft.com> wrote:
>
>>          graph_name = get_commit_graph_filename(opts.obj_dir);
>>          graph = load_commit_graph_one(graph_name);
>> +       FREE_AND_NULL(graph_name);
>>
>>          if (!graph)
>>                  die("graph file %s does not exist", graph_name);
>> -       FREE_AND_NULL(graph_name);
> This is probably because of something I said, but this does not look
> correct. The `die()` would typically print "(null)" or segfault. If the
> `die()` means we don't free `graph_name`, that should be fine.
>
> You're still leaking `graph` here (possibly nothing this patch should
> worry about) and in `graph_verify()`. UNLEAK-ing it immediately before
> calling `verify_commit_graph()` should be ok. I also think punting on
> this UNLEAK-business entirely would be ok; I was just a bit surprised to
> see one variable getting freed and the other one ignored.

Thanks, Martin. I was just blindly searching for FREE_AND_NULL() and 
shouldn't have been so careless.

-Stolee