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

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

Duy Nguyen <pclouds@gmail.com> writes:

> On Fri, Mar 30, 2018 at 7:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Which fields in candidate are safe to peek by the caller?  How can a
>> caller tell?
>
> To me, all fields should be valid after
> check_repository_format_gently().

If so, free() is wrong in the first place, and FREE_AND_NULL() is
making it even worse, no?  We learned there is work_tree set to
somewhere, the original code by Peff before abade65b ("setup: expose
enumerated repo info", 2017-11-12) freed it because the code no
longer needed that piece of information.  If we are passing all we
learned back to the caller, we should not free the field in the
function at all.  But it seems (below) the codepath is messier than
that.

> We still need to free and set NULL here though in addition to a
> cleanup interface. The reason is, when checking repo config from a
> worktree, we deliberately ignore core.worktree (which belongs to the
> main repo only). The implicit line near this
> free(candidate->work_tree) is "leave git_work_tree_cfg alone, we don't
> recognize core.worktree". Once we move setting git_work_tree_cfg out
> of this function, this becomes clear.

So in other words, there is a code that looks at the field and it
_wants_ to see NULL there---otherwise that brittle code misbehaves
and FREE_AND_NULL() is a bad-aid to work it around?

Then proposed log message "leaving it dangling is unsanitary" is
*not* what is going on here, and the real reason why the code should
be like so deserve to be described both in the log message and in a
large in-code comment, no?