On Tue, 2022-05-24 at 13:41 -0400, Derrick Stolee wrote:
> On 5/24/2022 10:51 AM, Kevin Locke wrote:> - /* Normalize the directory */
>> - strbuf_realpath(&tmp, tmp_original_cwd, 1);
>> - free((char*)tmp_original_cwd);
>> - tmp_original_cwd = NULL;
>> - startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>> + /* Try to normalize the directory. */
>> + if (strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
>> + free((char*)tmp_original_cwd);
>> + tmp_original_cwd = NULL;
>> + startup_info->original_cwd = strbuf_detach(&tmp, NULL);
>> + } else {
>> + trace2_data_string("setup", the_repository,
>> + "realpath-path", tmp_original_cwd);
>> + trace2_data_string("setup", the_repository,
>> + "realpath-failure", strerror(errno));
>> + tmp_original_cwd = NULL;
>
> I didn't catch this in v2, but should this line instead be
>
> startup_info->original_cwd = NULL;
>
> ? Especially based on this comment:
No worries. It's a good question. Setting startup_info->original_cwd
to NULL is handled by the no_prevention_needed label. But I just
realized it's not actually required in this case, since original_cwd
is NULL when setup_original_cwd() is called. I should probably return
rather than jumping to no_prevention_needed from the else block to
avoid the unnecessary free(NULL) and assignment.
Your comment made me realize that v2 and later neglect to free
tmp_original_cwd when strbuf_realpath() fails. How embarrassing.
I'll send an update to fix both those issues shortly.
Thanks again,
Kevin
