Re: [PATCH v3 4/4] wt-status.c: Set the committable flag in the collect phase.

From: Stephen Smith <ischis2@cox.net>
Date: Sep 24, 2018, 03:15 UTC
Message-ID: <2295579.U4Xb9QnJqG@thunderbird>
In-Reply-To: <xmqqworxufuv.fsf@gitster-ct.c.googlers.com>

On Friday, September 7, 2018 3:31:55 PM MST Junio C Hamano wrote:

Show 8 quoted lines

> For example, I noticed that both of the old
> callsites of wt_status_get_state() have free() of a few fiedls in
> the structure, and I kept the code as close to the original, but I
> suspect they should not be freed there in the functions in the
> "print" phase, but rather the caller of the "collect" and "print"
> should be made responsible for deciding when to dispose the entire
> wt_status (and wt_status_state as part of it).  This illustration
> patch does not address that kind of details (yet).

I followed the call tree back to original callers run_status() and cmd_status() in commit.c

This leads to a philosophical question. We want to move the state information out of the print functions because it doesn't seem correct. For the case in question this includes the calls to free() . By doing this we seem go have traded one location that shouldn't be touching the state variables for another.

I can see three solutions and could support any of the three:
1) Move the free calls to run_status() and cmd_status().
2) Move the calls calls to wt_status_print since that is the last function 
from wt_status.c that is called befor the structure goes out of scope in  
run_status() and cmd_status().
3) Add a new wt_collect*() function to free the variables. This would have an 
advantage that the free calls could be grouped in on place and not done in to 
functions.  A second advantage is that the free calls would be located where 
the pointers are initialized.

Personally I like solutions 1 and 3 over 2. What do others think?

sps