On 5/24/2022 3:20 PM, Kevin Locke wrote:
> Changes since v3:
> * Free tmp_original_cwd in both codepaths.
> * Return after strbuf_realpath() fails, rather than jumping to
> no_prevention_needed, to avoid unnecessary free(NULL) and NULL
> reassignment.
> * Invert the condition and remove the else block to match the
> return-on-error code style for better readability.
> * Stop adding "Try" to comment, since strbuf_realpath() hasn't
> been optional since v1.
...
> /* Normalize the directory */
> - strbuf_realpath(&tmp, tmp_original_cwd, 1);
> + if (!strbuf_realpath(&tmp, tmp_original_cwd, 0)) {
> + trace2_data_string("setup", the_repository,
> + "realpath-path", tmp_original_cwd);
> + trace2_data_string("setup", the_repository,
> + "realpath-failure", strerror(errno));
> + free((char*)tmp_original_cwd);
> + tmp_original_cwd = NULL;
> + return;
> + }
This is much easier to reason about.
> free((char*)tmp_original_cwd);
> tmp_original_cwd = NULL;
> startup_info->original_cwd = strbuf_detach(&tmp, NULL);
I had considered trying to remove this duplicate code freeing
temp_original_cwd. It requires adding a variable storing the
return from strbuf_realpath() _or_ knowing that tmp will have
zero length if strbuf_realpath() fails. It would look gross,
though:
strbuf_realpath(&tmp, tmp_original_cwd, 0);
if (!tmp->len) {
trace2_data_string("setup", the_repository,
"realpath-path", tmp_original_cwd);
trace2_data_string("setup", the_repository,
"realpath-failure", strerror(errno));
}
free((char*)tmp_original_cwd);
tmp_original_cwd = NULL;
if (!tmp->len)
return;
startup_info->original_cwd = strbuf_detach(&tmp, NULL);
...and that doesn't look very good at all. Thus, I think your v4
is ready to merge. Thanks for working on it!
Thanks,
-Stolee
