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....@gitster-ct.c.googlers.com/ > > Signed-off-by: Stephen P. Smith <isch...@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
