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

[Duy Nguyen <pclouds@gmail.com>]

On Fri, Mar 30, 2018 at 8:32 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

Actually no, NULL is the right value. I was trying to say that this
mysterious code was about _deliberately_ ignore core.worktree. By
keeping repo_fmt->worktree as NULL we tell the caller "core.worktree
is not set". The current code also does that but in a different way:
it sets git_work_tree_cfg based on candidate->worktree, but only for
the "!has_common" block.

>> 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?

Let's drop this for now. I'm a bit further along in refactoring this
code that I thought I could. It'll be clearer when the caller is also
updated to show what's wrong.
-- 
Duy