Re: [PATCH v4] setup: don't die if realpath(3) fails on getcwd(3)

[Derrick Stolee <derrickstolee@github.com>]

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