Re: [PATCH] setup.c: reset candidate->work_tree after freeing it

[Junio C Hamano <gitster@pobox.com>]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Dangling pointers are usually bad news. Reset it back to NULL.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Before abade65b ("setup: expose enumerated repo info", 2017-11-12),
candidate was an on-stack variable local to this function, so there
was no need to do anything before returning.  After that commit, we
pass &repo_fmt down the codepath so theoretically the caller could
peek into repo_fmt.work_tree after this codepath, which may be bad.
But if candidate->work_tree were not NULL at this point, that would
mean that such a caller that peeks is getting a WRONG information,
no?  It thinks there were no core.worktree set but in reality there
was the configuration set in the repository, no?

Which fields in candidate are safe to peek by the caller?  How can a
caller tell?

It seems that setup_git_directory_gently() uses repo_fmt when
calling various variants of setup_*_git_dir() and then uses the
repo_fmt.hash_algo field later.

If we want to keep fields of repo_fmt alive and valid after
check_repository_format_gently() and callers of it like
setup_*_git_dir() returns, then perhaps the right fix is not to free
candidate->work_tree here, and instead give an interface to clean up
repository_format instance, so that the ultimate caller like
setup_git_directory_gently() can safely peek into any fields in it
and then clean it up after it is done?

>  setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/setup.c b/setup.c
> index 7287779642..d193bee192 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -482,7 +482,7 @@ static int check_repository_format_gently(const char *gitdir, struct repository_
>  			inside_work_tree = -1;
>  		}
>  	} else {
> -		free(candidate->work_tree);
> +		FREE_AND_NULL(candidate->work_tree);
>  	}
>  
>  	return 0;