Re: [PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase

[Taylor Blau <me@ttaylorr.com>]

On Thu, Sep 27, 2018 at 09:49:36PM -0700, Stephen P. Smith wrote:
> When updating the collect and print functions, it was found that
> status variables were initialized in the collect phase and some
> variables were later freed in the print functions.

Nit: I think that in the past Eric Sunshine has recommended that I use
active voice in patches, but "it was found" is passive.

I tried to find the message that I was thinking of, but couldn't, so
perhaps I'm inventing it myself ;-).

I'm CC-ing Eric to check my judgement.

> Move the status state structure variables into the status state
> structure and populate them in the collect functions.
>
> Create a new funciton to free the buffers that were being freed in the
> print function.  Call this new function in commit.c where both the
> collect and print functions were being called.
>
> Based on a patch suggestion by Junio C Hamano. [1]
>
> [1] https://public-inbox.org/git/xmqqr2i5ueg4.fsf@gitster-ct.c.googlers.com/
>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---
>  builtin/commit.c |   3 ++
>  wt-status.c      | 135 +++++++++++++++++++++--------------------------
>  wt-status.h      |  38 ++++++-------
>  3 files changed, 83 insertions(+), 93 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 51ecebbec1..e168321e49 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -506,6 +506,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
>
>  	wt_status_collect(s);
>  	wt_status_print(s);
> +	wt_status_collect_free_buffers(s);
>
>  	return s->committable;
>  }
> @@ -1388,6 +1389,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  		s.prefix = prefix;
>
>  	wt_status_print(&s);
> +	wt_status_collect_free_buffers(&s);
> +
>  	return 0;
>  }
>
> diff --git a/wt-status.c b/wt-status.c
> index c7f76d4758..9977f0cdf2 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -744,21 +744,26 @@ static int has_unmerged(struct wt_status *s)
>
>  void wt_status_collect(struct wt_status *s)
>  {
> -	struct wt_status_state state;
>  	wt_status_collect_changes_worktree(s);
> -

Nit: unnecessary diff, but I certainly don't think that this is worth a
re-roll on its own.

>  	if (s->is_initial)
>  		wt_status_collect_changes_initial(s);
>  	else
>  		wt_status_collect_changes_index(s);
>  	wt_status_collect_untracked(s);
>
> -	memset(&state, 0, sizeof(state));
> -	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
> -	if (state.merge_in_progress && !has_unmerged(s))
> +	wt_status_get_state(&s->state, s->branch && !strcmp(s->branch, "HEAD"));
> +	if (s->state.merge_in_progress && !has_unmerged(s))
>  		s->committable = 1;

Should this line be de-dented to match the above?

>  }
>
> +void wt_status_collect_free_buffers(struct wt_status *s)
> +{
> +	free(s->state.branch);
> +	free(s->state.onto);
> +	free(s->state.detached_from);
> +}
> +
> +

Nit: too much whitespace between 'wt_status_collect_free_buffers()' and
'wt_longstatus_print_unmerged()' below. I see that there are two
newlines above, but I think that there should just be one.

>  static void wt_longstatus_print_unmerged(struct wt_status *s)
>  {
>  	int shown_header = 0;
> @@ -1087,8 +1092,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
>  }

The rest of this patch looks sensible to me, but I didn't follow the
original discussion in [1], so take my review with a grain of salt :-).

Thanks,
Taylor