Re: [PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase
On Fri, Sep 28, 2018 at 9:55 AM Taylor Blau wrote: > 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. You're probably thinking of "imperative mood" (and perhaps [1]), which this commit message already uses when it says "Move the..." and "Create a new function..." (in the couple paragraphs following the part you quoted). > > 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 s/funciton/function/ > > print function. Call this new function in commit.c where both the > > collect and print functions were being called. [1]: https://public-inbox.org/git/capig+ctozduqsaxh+w4h85m7en72yo09asdx+1kstswqbnb...@mail.gmail.com/
Re: [PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase
Taylor Blau writes:
> 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.
Yeah, and when/how it was found is much less interesting backstory
than _why_ we are doing this follow-thru. I think the first line
can just simply go without losing clarity.
>> Move the status state structure variables into the status state
>> structure and populate them in the collect functions.
On the other hand this one may deserve a bit more backstory. If I
understand correctly, what happened over time was
- A "struct wt_status" used to be sufficient for the output phase
to work. It was designed to be filled in the collect phase and
consumed in the output phase, but over time some fields are added
and output phase started filling it; we recently corrected it so
that .committable field is filled in the collect phase.
A "struct wt_status_state" that was used in other codepaths
turned out to be useful in showing the "git status" output, so
some output phase functions started taking it. This is not tied
to "struct wt_status", so the discipline of filling in the
collect phase to be consumed in the output phase was never
followed.
I am not suggesting to write that much in the log message, but and
with a backstory like that, embedding a wt_status_state inside
wt_status and fill it in the collect phase, which this patch does,
starts to make sense, I would think.
>> 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.
I do not think it is unnecessary to remove the blank between three
things this function does (i.e. (1) inspect working tree, (2)
inspect index and (3) inspect untrackeed; if there is no blank line
between (2) and (3), we shouldn't have a blank between (1) and (2)).
I do agree with you it is an unrelated change. Its correctness (not
to the compiler, but to the humans due to the above) is so trivial
that it probably is a good taste to include it in this patch.
>> if (s->is_initial)
>> wt_status_collect_changes_initial(s);
>> else
>> wt_status_collect_changes_index(s);
>> wt_status_collect_untracked(s);
>>
>> -memset(, 0, sizeof(state));
>> -wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
>> -if (state.merge_in_progress && !has_unmerged(s))
>> +wt_status_get_state(>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?
I am not sure if I follow.
Thanks.
Re: [PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase
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
> ---
> 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();
> + wt_status_collect_free_buffers();
> +
> 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(, 0, sizeof(state));
> - wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
> - if (state.merge_in_progress && !has_unmerged(s))
> + wt_status_get_state(>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
[PATCH 1/1] roll wt_status_state into wt_status and populate in the collect phase
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.
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
---
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();
+ wt_status_collect_free_buffers();
+
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);
-
if (s->is_initial)
wt_status_collect_changes_initial(s);
else
wt_status_collect_changes_index(s);
wt_status_collect_untracked(s);
- memset(, 0, sizeof(state));
- wt_status_get_state(, s->branch && !strcmp(s->branch, "HEAD"));
- if (state.merge_in_progress && !has_unmerged(s))
+ wt_status_get_state(>state, s->branch && !strcmp(s->branch, "HEAD"));
+ if (s->state.merge_in_progress && !has_unmerged(s))
s->committable = 1;
}
+void wt_status_collect_free_buffers(struct wt_status *s)
+{
+ free(s->state.branch);
+ free(s->state.onto);
+ free(s->state.detached_from);
+}
+
+
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)
}
static void show_merge_in_progress(struct wt_status *s,
- struct wt_status_state *state,
- const char *color)
+ const char *color)
{
if (has_unmerged(s)) {
status_printf_ln(s, color, _("You have unmerged paths."));
@@ -1109,16 +1113,15 @@ static void show_merge_in_progress(struct wt_status *s,
}
static void show_am_in_progress(struct wt_status *s,
- struct wt_status_state *state,
const char *color)
{
status_printf_ln(s, color,
_("You are in the middle of an am session."));
- if (state->am_empty_patch)
+ if (s->state.am_empty_patch)
status_printf_ln(s, color,
_("The current patch is empty."));
if (s->hints) {
- if (!state->am_empty_patch)
+ if (!s->state.am_empty_patch)
status_printf_ln(s, color,
_(" (fix conflicts and then run \"git am
--continue\")"));
status_printf_ln(s, color,
@@ -1242,10 +1245,9 @@ static int read_rebase_todolist(const char *fname,
struct string_list *lines)
}
static void show_rebase_information(struct wt_status *s,
- struct wt_status_state *state,
- const char *color)
+ const char *color)
{
- if (state->rebase_interactive_in_progress) {
+ if (s->state.rebase_interactive_in_progress) {
int i;
int nr_lines_to_show = 2;
@@ -1296,28 +1298,26 @@ static void show_rebase_information(struct wt_status *s,
}
static void print_rebase_state(struct wt_status *s,
- struct wt_status_state *state,
- const char *color)
+ const char *color)
{
- if (state->branch)
+ if (s->state.branch)
status_printf_ln(s, color,
_("You are currently rebasing branch '%s' on
'%s'."),
-state->branch,
-state->onto);
+s->state.branch,
+
