Re: [PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase
- From
Taylor Blau <me@ttaylorr.com>
- Date
- Sep 28, 2018, 13:55 UTC
- Message-ID
- <20180928135549.GA23652@syl>
- In-Reply-To
- <20180928044936.2919-2-ischis2@cox.net>
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.
Show 50 quoted lines
> 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.
Show 12 quoted lines
> 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?
Show 10 quoted lines
> }
>
> +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.
Show 5 quoted lines
> 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