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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2018, 18:32 UTC
Message-ID: <xmqqo9j52z4m.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <CACsJy8DS2pRaDhXgKqFQaqjFft_EW5G=8t9PhNcAgMsSDsS0jA@mail.gmail.com>

Duy Nguyen <pclouds@gmail.com> writes:

Show 7 quoted lines

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

Show 7 quoted lines

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