[PATCH v3 0/4] wt-status.c: commitable flag

[PATCH v3 0/4] wt-status.c: commitable flag

[Stephen P. Smith]

A couple of years ago, during a patch review Junio found that the
commitable bit as implemented in wt-status.c was broken.

Stephen P. Smith (4):
  Move has_unmerged earlier in the file.
  wt-status: rename commitable to committable
  t7501: add test of "commit --dry-run --short"
  wt-status.c: Set the committable flag in the collect phase.

 builtin/commit.c  | 18 +++++++++---------
 t/t7501-commit.sh | 10 ++++++++--
 wt-status.c       | 45 +++++++++++++++++++++++++++------------------
 wt-status.h       |  2 +-
 4 files changed, 45 insertions(+), 30 deletions(-)

-- 
2.18.0

[PATCH v3 1/4] Move has_unmerged earlier in the file.

[Stephen P. Smith]

Move has_unmerged up in the file so that has_unmerged can be called in
wt_status_collect where we need to place a merge check.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 wt-status.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 5ffab6101..180faf6ba 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -724,6 +724,19 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static int has_unmerged(struct wt_status *s)
+{
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d;
+		d = s->change.items[i].util;
+		if (d->stagemask)
+			return 1;
+	}
+	return 0;
+}
+
 void wt_status_collect(struct wt_status *s)
 {
 	wt_status_collect_changes_worktree(s);
@@ -1063,19 +1076,6 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(struct wt_status *s)
-{
-	int i;
-
-	for (i = 0; i < s->change.nr; i++) {
-		struct wt_status_change_data *d;
-		d = s->change.items[i].util;
-		if (d->stagemask)
-			return 1;
-	}
-	return 0;
-}
-
 static void show_merge_in_progress(struct wt_status *s,
 				struct wt_status_state *state,
 				const char *color)
-- 
2.18.0

[PATCH v3 2/4] wt-status: rename commitable to committable

[Stephen P. Smith]

Fix variable spelling error.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 builtin/commit.c | 18 +++++++++---------
 wt-status.c      | 10 +++++-----
 wt-status.h      |  2 +-
 3 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 0d9828e29..51ecebbec 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -507,7 +507,7 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	wt_status_collect(s);
 	wt_status_print(s);
 
-	return s->commitable;
+	return s->committable;
 }
 
 static int is_a_merge(const struct commit *current_head)
@@ -653,7 +653,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 {
 	struct stat statbuf;
 	struct strbuf committer_ident = STRBUF_INIT;
-	int commitable;
+	int committable;
 	struct strbuf sb = STRBUF_INIT;
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
@@ -870,7 +870,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
-		commitable = run_status(s->fp, index_file, prefix, 1, s);
+		committable = run_status(s->fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
 	} else {
 		struct object_id oid;
@@ -888,7 +888,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			for (i = 0; i < active_nr; i++)
 				if (ce_intent_to_add(active_cache[i]))
 					ita_nr++;
-			commitable = active_nr - ita_nr > 0;
+			committable = active_nr - ita_nr > 0;
 		} else {
 			/*
 			 * Unless the user did explicitly request a submodule
@@ -904,7 +904,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			if (ignore_submodule_arg &&
 			    !strcmp(ignore_submodule_arg, "all"))
 				flags.ignore_submodules = 1;
-			commitable = index_differs_from(parent, &flags, 1);
+			committable = index_differs_from(parent, &flags, 1);
 		}
 	}
 	strbuf_release(&committer_ident);
@@ -916,7 +916,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	 * explicit --allow-empty. In the cherry-pick case, it may be
 	 * empty due to conflict resolution, which the user should okay.
 	 */
-	if (!commitable && whence != FROM_MERGE && !allow_empty &&
+	if (!committable && whence != FROM_MERGE && !allow_empty &&
 	    !(amend && is_a_merge(current_head))) {
 		s->display_comment_prefix = old_display_comment_prefix;
 		run_status(stdout, index_file, prefix, 0, s);
@@ -1186,14 +1186,14 @@ static int parse_and_validate_options(int argc, const char *argv[],
 static int dry_run_commit(int argc, const char **argv, const char *prefix,
 			  const struct commit *current_head, struct wt_status *s)
 {
-	int commitable;
+	int committable;
 	const char *index_file;
 
 	index_file = prepare_index(argc, argv, prefix, current_head, 1);
-	commitable = run_status(stdout, index_file, prefix, 0, s);
+	committable = run_status(stdout, index_file, prefix, 0, s);
 	rollback_index_files();
 
-	return commitable ? 0 : 1;
+	return committable ? 0 : 1;
 }
 
 define_list_config_array_extra(color_status_slots, {"added"});
diff --git a/wt-status.c b/wt-status.c
index 180faf6ba..4962b5bc8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -786,7 +786,7 @@ static void wt_longstatus_print_updated(struct wt_status *s)
 			continue;
 		if (!shown_header) {
 			wt_longstatus_print_cached_header(s);
-			s->commitable = 1;
+			s->committable = 1;
 			shown_header = 1;
 		}
 		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
@@ -1021,7 +1021,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 		rev.diffopt.use_color = 0;
 		wt_status_add_cut_line(s->fp);
 	}
-	if (s->verbose > 1 && s->commitable) {
+	if (s->verbose > 1 && s->committable) {
 		/* print_updated() printed a header, so do we */
 		if (s->fp != stdout)
 			wt_longstatus_print_trailer(s);
@@ -1089,7 +1089,7 @@ static void show_merge_in_progress(struct wt_status *s,
 					 _("  (use \"git merge --abort\" to abort the merge)"));
 		}
 	} else {
-		s-> commitable = 1;
+		s-> committable = 1;
 		status_printf_ln(s, color,
 			_("All conflicts fixed but you are still merging."));
 		if (s->hints)
@@ -1665,14 +1665,14 @@ static void wt_longstatus_print(struct wt_status *s)
 					   "new files yourself (see 'git help status')."),
 					 s->untracked_in_ms / 1000.0);
 		}
-	} else if (s->commitable)
+	} else if (s->committable)
 		status_printf_ln(s, GIT_COLOR_NORMAL, _("Untracked files not listed%s"),
 			s->hints
 			? _(" (use -u option to show untracked files)") : "");
 
 	if (s->verbose)
 		wt_longstatus_print_verbose(s);
-	if (!s->commitable) {
+	if (!s->committable) {
 		if (s->amend)
 			status_printf_ln(s, GIT_COLOR_NORMAL, _("No changes"));
 		else if (s->nowarn)
diff --git a/wt-status.h b/wt-status.h
index 1673d146f..937b2c352 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -96,7 +96,7 @@ struct wt_status {
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
 
 	/* These are computed during processing of the individual sections */
-	int commitable;
+	int committable;
 	int workdir_dirty;
 	const char *index_file;
 	FILE *fp;
-- 
2.18.0

[PATCH v3 3/4] t7501: add test of "commit --dry-run --short"

[Stephen P. Smith]

Add test for commit with --dry-run --short for a new file of zero
length.

The test demonstrates that the setting of the committable flag is
broken.

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 t/t7501-commit.sh | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 4cae92804..cf2a4c539 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -682,4 +682,10 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git commit -m "conflicts fixed from merge."
 '
 
+test_expect_failure '--dry-run --short' '
+	>test-file &&
+	git add test-file &&
+	git commit --dry-run --short
+'
+
 test_done
-- 
2.18.0

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

[Stephen P. Smith]

In an update to fix a bug with "commit --dry-run" it was found that
the committable flag was broken. The update was, at the time, accepted
as it was better than the previous version. [1]

Since the setting of the committable flag had been done in
wt_longstatus_print_updated, move it to wt_status_collect_updated_cb.

Set the committable flag in wt_status_collect_changes_initial to keep
from introducing a rebase regression.

Instead of setting the committable flag in show_merge_in_progress, in
wt_status_cllect check for a merge that has not been committed. If
present then set the committable flag.

Change the tests to expect success since updates to the wt-status
broken code section is being fixed.

[1] https://public-inbox.org/git/xmqqr3gcj9i5.fsf@gitster.mtv.corp.google.com/

Signed-off-by: Stephen P. Smith <ischis2@cox.net>
---
 t/t7501-commit.sh |  6 +++---
 wt-status.c       | 13 +++++++++++--
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index cf2a4c539..e18c0b4a6 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -99,12 +99,12 @@ test_expect_success '--dry-run with stuff to commit returns ok' '
 	git commit -m next -a --dry-run
 '
 
-test_expect_failure '--short with stuff to commit returns ok' '
+test_expect_success '--short with stuff to commit returns ok' '
 	echo bongo bongo bongo >>file &&
 	git commit -m next -a --short
 '
 
-test_expect_failure '--porcelain with stuff to commit returns ok' '
+test_expect_success '--porcelain with stuff to commit returns ok' '
 	echo bongo bongo bongo >>file &&
 	git commit -m next -a --porcelain
 '
@@ -682,7 +682,7 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git commit -m "conflicts fixed from merge."
 '
 
-test_expect_failure '--dry-run --short' '
+test_expect_success '--dry-run --short' '
 	>test-file &&
 	git add test-file &&
 	git commit --dry-run --short
diff --git a/wt-status.c b/wt-status.c
index 4962b5bc8..c7f76d475 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -540,10 +540,12 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
 			/* Leave {mode,oid}_head zero for an add. */
 			d->mode_index = p->two->mode;
 			oidcpy(&d->oid_index, &p->two->oid);
+			s->committable = 1;
 			break;
 		case DIFF_STATUS_DELETED:
 			d->mode_head = p->one->mode;
 			oidcpy(&d->oid_head, &p->one->oid);
+			s->committable = 1;
 			/* Leave {mode,oid}_index zero for a delete. */
 			break;
 
@@ -561,6 +563,7 @@ static void wt_status_collect_updated_cb(struct diff_queue_struct *q,
 			d->mode_index = p->two->mode;
 			oidcpy(&d->oid_head, &p->one->oid);
 			oidcpy(&d->oid_index, &p->two->oid);
+			s->committable = 1;
 			break;
 		case DIFF_STATUS_UNMERGED:
 			d->stagemask = unmerged_mask(p->two->path);
@@ -665,11 +668,13 @@ static void wt_status_collect_changes_initial(struct wt_status *s)
 			 * code will output the stage values directly and not use the
 			 * values in these fields.
 			 */
+			s->committable = 1;
 		} else {
 			d->index_status = DIFF_STATUS_ADDED;
 			/* Leave {mode,oid}_head zero for adds. */
 			d->mode_index = ce->ce_mode;
 			oidcpy(&d->oid_index, &ce->oid);
+			s->committable = 1;
 		}
 	}
 }
@@ -739,6 +744,7 @@ 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)
@@ -746,6 +752,11 @@ void wt_status_collect(struct wt_status *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))
+		s->committable = 1;
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -786,7 +797,6 @@ static void wt_longstatus_print_updated(struct wt_status *s)
 			continue;
 		if (!shown_header) {
 			wt_longstatus_print_cached_header(s);
-			s->committable = 1;
 			shown_header = 1;
 		}
 		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
@@ -1089,7 +1099,6 @@ static void show_merge_in_progress(struct wt_status *s,
 					 _("  (use \"git merge --abort\" to abort the merge)"));
 		}
 	} else {
-		s-> committable = 1;
 		status_printf_ln(s, color,
 			_("All conflicts fixed but you are still merging."));
 		if (s->hints)
-- 
2.18.0

Re: [PATCH v3 0/4] wt-status.c: commitable flag

[Ævar Arnfjörð Bjarmason]

On Thu, Sep 06 2018, Stephen P. Smith wrote:

This all looks good to me this time around.

> Stephen P. Smith (4):
>   Move has_unmerged earlier in the file.
>   wt-status: rename commitable to committable
>   t7501: add test of "commit --dry-run --short"
>   wt-status.c: Set the committable flag in the collect phase.

Sometimes you send mail from this address as "Stephen & Linda Smith
<ischis2@cox.net>", do we also need Linda Smith's Signed-Off-By? :)

Re: [PATCH v3 0/4] wt-status.c: commitable flag

[Stephen & Linda Smith]

On Thursday, September 6, 2018 12:38:55 AM MST Ævar Arnfjörð Bjarmason wrote:
> On Thu, Sep 06 2018, Stephen P. Smith wrote:
> Sometimes you send mail from this address as "Stephen & Linda Smith
> <ischis2@cox.net>", do we also need Linda Smith's Signed-Off-By? :)

My wife and I share one email account.   If I use the GUI email client to 
respond to emails then her name is on the From line (I could edit future 
emails).

When I am submitting patches/updates, I do so under my name since it is 
"Stephen P. Smith" that is creating the patch.   

I've never had anyone ask before.

Re: [PATCH v3 2/4] wt-status: rename commitable to committable

[Junio C Hamano]

"Stephen P. Smith" <ischis2@cox.net> writes:

> Fix variable spelling error.
>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>
> ---

Thanks ;-)

Re: [PATCH v3 0/4] wt-status.c: commitable flag

[Junio C Hamano]

"Stephen P. Smith" <ischis2@cox.net> writes:

> A couple of years ago, during a patch review Junio found that the
> commitable bit as implemented in wt-status.c was broken.
>
> Stephen P. Smith (4):
>   Move has_unmerged earlier in the file.
>   wt-status: rename commitable to committable
>   t7501: add test of "commit --dry-run --short"
>   wt-status.c: Set the committable flag in the collect phase.
>
>  builtin/commit.c  | 18 +++++++++---------
>  t/t7501-commit.sh | 10 ++++++++--
>  wt-status.c       | 45 +++++++++++++++++++++++++++------------------
>  wt-status.h       |  2 +-
>  4 files changed, 45 insertions(+), 30 deletions(-)

Thanks.

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

[Junio C Hamano]

"Stephen P. Smith" <ischis2@cox.net> writes:

>  void wt_status_collect(struct wt_status *s)
>  {
> +	struct wt_status_state state;
>  	wt_status_collect_changes_worktree(s);
>  
>  	if (s->is_initial)
> @@ -746,6 +752,11 @@ void wt_status_collect(struct wt_status *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))
> +		s->committable = 1;
>  }

I do not think this is wrong per-se, but now we have three calls to
wt_status_get_state() in wt-status.c, and it smells (at least to me)
that each of these callsites does so only because their callers
do not give them enough information, forcing them to find the state
out for themselves.

Given that the ideal paradigm to come up with the "work tree status"
is to do the collection followed by printing, and among three
callers of get_state(), two appear in the "printing" side of the
callchain [*1*], I wonder if it makes a better organization to

 - embed struct wt_status_state in struct wt_status

 - make the new call to wt_status_get_state() added above in this
   patch to populate the wt_status_state embedded in 's'

 - change the other two callers of wt_status_get_state() in
   wt_longstatus_print() and wt_porcelain_v2_print_tracking(), both
   of which will receive 's' that has been populated by a previous
   call to wt_status_collect(), so that they do *not* call
   get_state() themselves, but instead use the result recorded in
   wt_status_state embedded in 's', which was populated by
   wt_status_collect() before they got called.

That would bring the resulting code even closer to the ideal,
i.e. the "collect" phase learns _everything_ we need about the
current state that is necessary in order to later show to the user,
and the "print" phase does not do its own separate discovery.

What do you think?

Thanks.


[Reference]

*1* Here are the selected functions and what other functions they
    call.

    wt_status_collect()

     -> wt_status_collect_changes_initial()
     -> wt_status_collect_changes_index()
     -> wt_status_collect_untracked()
     -> wt_status_get_state()

    wt_longstatus_print()

     -> wt_status_get_state()

    wt_porcelain_v2_print_tracking()

     -> wt_status_get_state()


    wt_status_print()

     -> wt_porcelain_v2_print()
        -> wt_porcelain_v2_print_tracking()
     -> wt_longstatus_print()


    run_status()

     -> wt_status_collect()
     -> wt_status_print()

    cmd_status()

     -> wt_status_collect()
     -> wt_status_print()


    prepare_to_commit(), dry_run_commit()

     -> run_status()


    Most notably, wt_status_collect() always happens before
    wt_status_print(), which is natural because the former is to
    collect information in 's' that is used by the latter to print.

    And in various functions wt_status_print() calls indirectly, the
    two other callers of wt_status_get_state() appear.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> "Stephen P. Smith" <ischis2@cox.net> writes:
>
>>  void wt_status_collect(struct wt_status *s)
>>  {
>> +	struct wt_status_state state;
>>  	wt_status_collect_changes_worktree(s);
>>  
>>  	if (s->is_initial)
>> @@ -746,6 +752,11 @@ void wt_status_collect(struct wt_status *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))
>> +		s->committable = 1;
>>  }
>
> I do not think this is wrong per-se, but now we have three calls to
> wt_status_get_state() in wt-status.c, and it smells (at least to me)
> that each of these callsites does so only because their callers
> do not give them enough information, forcing them to find the state
> out for themselves.
>
> Given that the ideal paradigm to come up with the "work tree status"
> is to do the collection followed by printing, and among three
> callers of get_state(), two appear in the "printing" side of the
> callchain [*1*], I wonder if it makes a better organization to
>
>  - embed struct wt_status_state in struct wt_status
>
>  - make the new call to wt_status_get_state() added above in this
>    patch to populate the wt_status_state embedded in 's'
>
>  - change the other two callers of wt_status_get_state() in
>    wt_longstatus_print() and wt_porcelain_v2_print_tracking(), both
>    of which will receive 's' that has been populated by a previous
>    call to wt_status_collect(), so that they do *not* call
>    get_state() themselves, but instead use the result recorded in
>    wt_status_state embedded in 's', which was populated by
>    wt_status_collect() before they got called.
>
> That would bring the resulting code even closer to the ideal,
> i.e. the "collect" phase learns _everything_ we need about the
> current state that is necessary in order to later show to the user,
> and the "print" phase does not do its own separate discovery.
>
> What do you think?
>
> Thanks.

Such a "clean-up" may look like this patch:

 - add .state field to wt_status to embed a wt_status_state instance

 - remove a parameter of type struct wt_status_state from all
   functions where wt_status is already passed; we'd use .state
   field of the wt_status instead

The patch is mostly for illustration of the idea.

The result seems to compile and pass the test suite, but I haven't
carefully thought about what else I may be breaking with this
mechanical change.  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).


 wt-status.c | 132 ++++++++++++++++++++++++++----------------------------------
 wt-status.h |  37 ++++++++---------
 2 files changed, 77 insertions(+), 92 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index c7f76d4758..69f2cbdca9 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -744,18 +744,15 @@ 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(&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;
 }
 
@@ -1087,8 +1084,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 +1105,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 +1237,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 +1290,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,
+				 s->state.onto);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently rebasing."));
 }
 
 static void show_rebase_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
-				const char *color)
+				    const char *color)
 {
 	struct stat st;
 
-	show_rebase_information(s, state, color);
+	show_rebase_information(s, color);
 	if (has_unmerged(s)) {
-		print_rebase_state(s, state, color);
+		print_rebase_state(s, color);
 		if (s->hints) {
 			status_printf_ln(s, color,
 				_("  (fix conflicts and then run \"git rebase --continue\")"));
@@ -1326,17 +1318,18 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git rebase --abort\" to check out the original branch)"));
 		}
-	} else if (state->rebase_in_progress || !stat(git_path_merge_msg(the_repository), &st)) {
-		print_rebase_state(s, state, color);
+	} else if (s->state.rebase_in_progress ||
+		   !stat(git_path_merge_msg(the_repository), &st)) {
+		print_rebase_state(s, color);
 		if (s->hints)
 			status_printf_ln(s, color,
 				_("  (all conflicts fixed: run \"git rebase --continue\")"));
 	} else if (split_commit_in_progress(s)) {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit during a rebase."));
@@ -1344,11 +1337,11 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (Once your working directory is clean, run \"git rebase --continue\")"));
 	} else {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit during a rebase."));
@@ -1363,11 +1356,10 @@ static void show_rebase_in_progress(struct wt_status *s,
 }
 
 static void show_cherry_pick_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+					 const char *color)
 {
 	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
-			find_unique_abbrev(&state->cherry_pick_head_oid, DEFAULT_ABBREV));
+			find_unique_abbrev(&s->state.cherry_pick_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1382,11 +1374,10 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 }
 
 static void show_revert_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+				    const char *color)
 {
 	status_printf_ln(s, color, _("You are currently reverting commit %s."),
-			 find_unique_abbrev(&state->revert_head_oid, DEFAULT_ABBREV));
+			 find_unique_abbrev(&s->state.revert_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1401,13 +1392,12 @@ static void show_revert_in_progress(struct wt_status *s,
 }
 
 static void show_bisect_in_progress(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 bisecting, started from branch '%s'."),
-				 state->branch);
+				 s->state.branch);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently bisecting."));
@@ -1581,48 +1571,45 @@ void wt_status_get_state(struct wt_status_state *state,
 		wt_status_get_detached_from(state);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
-				      struct wt_status_state *state)
+static void wt_longstatus_print_state(struct wt_status *s)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
+	struct wt_status_state *state = &s->state;
+
 	if (state->merge_in_progress)
-		show_merge_in_progress(s, state, state_color);
+		show_merge_in_progress(s, state_color);
 	else if (state->am_in_progress)
-		show_am_in_progress(s, state, state_color);
+		show_am_in_progress(s, state_color);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
-		show_rebase_in_progress(s, state, state_color);
+		show_rebase_in_progress(s, state_color);
 	else if (state->cherry_pick_in_progress)
-		show_cherry_pick_in_progress(s, state, state_color);
+		show_cherry_pick_in_progress(s, state_color);
 	else if (state->revert_in_progress)
-		show_revert_in_progress(s, state, state_color);
+		show_revert_in_progress(s, state_color);
 	if (state->bisect_in_progress)
-		show_bisect_in_progress(s, state, state_color);
+		show_bisect_in_progress(s, state_color);
 }
 
 static void wt_longstatus_print(struct wt_status *s)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-	struct wt_status_state state;
-
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state,
-			    s->branch && !strcmp(s->branch, "HEAD"));
 
 	if (s->branch) {
 		const char *on_what = _("On branch ");
 		const char *branch_name = s->branch;
 		if (!strcmp(branch_name, "HEAD")) {
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
-				if (state.rebase_interactive_in_progress)
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress) {
+				if (s->state.rebase_interactive_in_progress)
 					on_what = _("interactive rebase in progress; onto ");
 				else
 					on_what = _("rebase in progress; onto ");
-				branch_name = state.onto;
-			} else if (state.detached_from) {
-				branch_name = state.detached_from;
-				if (state.detached_at)
+				branch_name = s->state.onto;
+			} else if (s->state.detached_from) {
+				branch_name = s->state.detached_from;
+				if (s->state.detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");
@@ -1639,10 +1626,10 @@ static void wt_longstatus_print(struct wt_status *s)
 			wt_longstatus_print_tracking(s);
 	}
 
-	wt_longstatus_print_state(s, &state);
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
+	wt_longstatus_print_state(s);
+	free(s->state.branch);
+	free(s->state.onto);
+	free(s->state.detached_from);
 
 	if (s->is_initial) {
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
@@ -1946,13 +1933,9 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 	struct branch *branch;
 	const char *base;
 	const char *branch_name;
-	struct wt_status_state state;
 	int ab_info, nr_ahead, nr_behind;
 	char eol = s->null_termination ? '\0' : '\n';
 
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
-
 	fprintf(s->fp, "# branch.oid %s%c",
 			(s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)),
 			eol);
@@ -1963,10 +1946,11 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 		if (!strcmp(s->branch, "HEAD")) {
 			fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
 
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress)
-				branch_name = state.onto;
-			else if (state.detached_from)
-				branch_name = state.detached_from;
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress)
+				branch_name = s->state.onto;
+			else if (s->state.detached_from)
+				branch_name = s->state.detached_from;
 			else
 				branch_name = "";
 		} else {
@@ -2001,9 +1985,9 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 		}
 	}
 
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
+	free(s->state.branch);
+	free(s->state.onto);
+	free(s->state.detached_from);
 }
 
 /*
diff --git a/wt-status.h b/wt-status.h
index 937b2c3521..f9115c59ae 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -64,6 +64,24 @@ enum wt_status_format {
 	STATUS_FORMAT_UNSPECIFIED
 };
 
+struct wt_status_state {
+	int merge_in_progress;
+	int am_in_progress;
+	int am_empty_patch;
+	int rebase_in_progress;
+	int rebase_interactive_in_progress;
+	int cherry_pick_in_progress;
+	int bisect_in_progress;
+	int revert_in_progress;
+	int detached_at;
+	char *branch;
+	char *onto;
+	char *detached_from;
+	struct object_id detached_oid;
+	struct object_id revert_head_oid;
+	struct object_id cherry_pick_head_oid;
+};
+
 struct wt_status {
 	int is_initial;
 	char *branch;
@@ -93,6 +111,7 @@ struct wt_status {
 	int rename_score;
 	int rename_limit;
 	enum wt_status_format status_format;
+	struct wt_status_state state;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
 
 	/* These are computed during processing of the individual sections */
@@ -107,24 +126,6 @@ struct wt_status {
 	uint32_t untracked_in_ms;
 };
 
-struct wt_status_state {
-	int merge_in_progress;
-	int am_in_progress;
-	int am_empty_patch;
-	int rebase_in_progress;
-	int rebase_interactive_in_progress;
-	int cherry_pick_in_progress;
-	int bisect_in_progress;
-	int revert_in_progress;
-	int detached_at;
-	char *branch;
-	char *onto;
-	char *detached_from;
-	struct object_id detached_oid;
-	struct object_id revert_head_oid;
-	struct object_id cherry_pick_head_oid;
-};
-
 size_t wt_status_locate_end(const char *s, size_t len);
 void wt_status_add_cut_line(FILE *fp);
 void wt_status_prepare(struct wt_status *s);

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

[Stephen P. Smith]

On Friday, September 7, 2018 3:31:55 PM MST you wrote:
> Junio C Hamano <gitster@pobox.com> writes:

> The patch is mostly for illustration of the idea.
> 
> The result seems to compile and pass the test suite, but I haven't
> carefully thought about what else I may be breaking with this
> mechanical change.  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).

If we use this as a basis of a follow on patch, how do I handle credit.   You 
obviously wrote this patch and I did not.

So how is the mechanics of that normally done?   Thanks for the patch I will 
work with it.

sps

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

[Junio C Hamano]

"Stephen P. Smith" <ischis2@cox.net> writes:

> On Friday, September 7, 2018 3:31:55 PM MST you wrote:
>> Junio C Hamano <gitster@pobox.com> writes:
>
>> The patch is mostly for illustration of the idea.
>> 
>> The result seems to compile and pass the test suite, but I haven't
>> carefully thought about what else I may be breaking with this
>> mechanical change.  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).
>
> If we use this as a basis of a follow on patch, how do I handle credit.   You 
> obviously wrote this patch and I did not.

Often people just mention "This was based on an earlier work by ..."
at/near the end of the log message.  When the result ends up to be
very different from the earlier work, just adding "Helped-by: ..."
before your sign-off is often sufficient.

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

[Stephen Smith]

On Friday, September 7, 2018 3:31:55 PM MST Junio C Hamano wrote:
> 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

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

[Junio C Hamano]

Stephen Smith <ischis2@cox.net> writes:

> 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?

I think freeing at the top level caller (i.e. #1) once it finished
using the information collected would make the most sense---it
initiated the collection, then fed the collected info to shower, and
now it knows it is done with the pieces of memory it used to make
these two parts communicate with each other.

And for keeping multiple "pieces of memory" as a unit, introducing a
helper is a good technique (i.e. #3); but I view that mostly as an
implementation detail of #1.

Thanks.

[PATCH 0/1] wt-status-state-cleanup

[Stephen P. Smith]

Junio suggested a cleanup patch, jc/wt-status-state-cleanup, which is
the basis for this patch.

This patch uses ss/wt-status-committable.

The main update from the patch suggestion was cleanup of the free
calls for three strings in the status structure.

Stephen P. Smith (1):
  roll wt_status_state into wt_status and populate in the collect phase

 builtin/commit.c |   3 ++
 wt-status.c      | 135 +++++++++++++++++++++--------------------------
 wt-status.h      |  38 ++++++-------
 3 files changed, 83 insertions(+), 93 deletions(-)

-- 
2.19.0

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

[Stephen P. Smith]

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.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);
-
 	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;
 }
 
+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,
+				 s->state.onto);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently rebasing."));
 }
 
 static void show_rebase_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
-				const char *color)
+				    const char *color)
 {
 	struct stat st;
 
-	show_rebase_information(s, state, color);
+	show_rebase_information(s, color);
 	if (has_unmerged(s)) {
-		print_rebase_state(s, state, color);
+		print_rebase_state(s, color);
 		if (s->hints) {
 			status_printf_ln(s, color,
 				_("  (fix conflicts and then run \"git rebase --continue\")"));
@@ -1326,17 +1326,18 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git rebase --abort\" to check out the original branch)"));
 		}
-	} else if (state->rebase_in_progress || !stat(git_path_merge_msg(the_repository), &st)) {
-		print_rebase_state(s, state, color);
+	} else if (s->state.rebase_in_progress ||
+		   !stat(git_path_merge_msg(the_repository), &st)) {
+		print_rebase_state(s, color);
 		if (s->hints)
 			status_printf_ln(s, color,
 				_("  (all conflicts fixed: run \"git rebase --continue\")"));
 	} else if (split_commit_in_progress(s)) {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit during a rebase."));
@@ -1344,11 +1345,11 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (Once your working directory is clean, run \"git rebase --continue\")"));
 	} else {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit during a rebase."));
@@ -1363,11 +1364,10 @@ static void show_rebase_in_progress(struct wt_status *s,
 }
 
 static void show_cherry_pick_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+					 const char *color)
 {
 	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
-			find_unique_abbrev(&state->cherry_pick_head_oid, DEFAULT_ABBREV));
+			find_unique_abbrev(&s->state.cherry_pick_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1382,11 +1382,10 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 }
 
 static void show_revert_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+				    const char *color)
 {
 	status_printf_ln(s, color, _("You are currently reverting commit %s."),
-			 find_unique_abbrev(&state->revert_head_oid, DEFAULT_ABBREV));
+			 find_unique_abbrev(&s->state.revert_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1401,13 +1400,12 @@ static void show_revert_in_progress(struct wt_status *s,
 }
 
 static void show_bisect_in_progress(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 bisecting, started from branch '%s'."),
-				 state->branch);
+				 s->state.branch);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently bisecting."));
@@ -1581,48 +1579,45 @@ void wt_status_get_state(struct wt_status_state *state,
 		wt_status_get_detached_from(state);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
-				      struct wt_status_state *state)
+static void wt_longstatus_print_state(struct wt_status *s)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
+	struct wt_status_state *state = &s->state;
+
 	if (state->merge_in_progress)
-		show_merge_in_progress(s, state, state_color);
+		show_merge_in_progress(s, state_color);
 	else if (state->am_in_progress)
-		show_am_in_progress(s, state, state_color);
+		show_am_in_progress(s, state_color);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
-		show_rebase_in_progress(s, state, state_color);
+		show_rebase_in_progress(s, state_color);
 	else if (state->cherry_pick_in_progress)
-		show_cherry_pick_in_progress(s, state, state_color);
+		show_cherry_pick_in_progress(s, state_color);
 	else if (state->revert_in_progress)
-		show_revert_in_progress(s, state, state_color);
+		show_revert_in_progress(s, state_color);
 	if (state->bisect_in_progress)
-		show_bisect_in_progress(s, state, state_color);
+		show_bisect_in_progress(s, state_color);
 }
 
 static void wt_longstatus_print(struct wt_status *s)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-	struct wt_status_state state;
-
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state,
-			    s->branch && !strcmp(s->branch, "HEAD"));
 
 	if (s->branch) {
 		const char *on_what = _("On branch ");
 		const char *branch_name = s->branch;
 		if (!strcmp(branch_name, "HEAD")) {
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
-				if (state.rebase_interactive_in_progress)
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress) {
+				if (s->state.rebase_interactive_in_progress)
 					on_what = _("interactive rebase in progress; onto ");
 				else
 					on_what = _("rebase in progress; onto ");
-				branch_name = state.onto;
-			} else if (state.detached_from) {
-				branch_name = state.detached_from;
-				if (state.detached_at)
+				branch_name = s->state.onto;
+			} else if (s->state.detached_from) {
+				branch_name = s->state.detached_from;
+				if (s->state.detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");
@@ -1639,10 +1634,7 @@ static void wt_longstatus_print(struct wt_status *s)
 			wt_longstatus_print_tracking(s);
 	}
 
-	wt_longstatus_print_state(s, &state);
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
+	wt_longstatus_print_state(s);
 
 	if (s->is_initial) {
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
@@ -1946,13 +1938,9 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 	struct branch *branch;
 	const char *base;
 	const char *branch_name;
-	struct wt_status_state state;
 	int ab_info, nr_ahead, nr_behind;
 	char eol = s->null_termination ? '\0' : '\n';
 
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
-
 	fprintf(s->fp, "# branch.oid %s%c",
 			(s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)),
 			eol);
@@ -1963,10 +1951,11 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 		if (!strcmp(s->branch, "HEAD")) {
 			fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
 
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress)
-				branch_name = state.onto;
-			else if (state.detached_from)
-				branch_name = state.detached_from;
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress)
+				branch_name = s->state.onto;
+			else if (s->state.detached_from)
+				branch_name = s->state.detached_from;
 			else
 				branch_name = "";
 		} else {
@@ -2000,10 +1989,6 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 			}
 		}
 	}
-
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
 }
 
 /*
diff --git a/wt-status.h b/wt-status.h
index 937b2c3521..1fcf93afbf 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -64,6 +64,24 @@ enum wt_status_format {
 	STATUS_FORMAT_UNSPECIFIED
 };
 
+struct wt_status_state {
+	int merge_in_progress;
+	int am_in_progress;
+	int am_empty_patch;
+	int rebase_in_progress;
+	int rebase_interactive_in_progress;
+	int cherry_pick_in_progress;
+	int bisect_in_progress;
+	int revert_in_progress;
+	int detached_at;
+	char *branch;
+	char *onto;
+	char *detached_from;
+	struct object_id detached_oid;
+	struct object_id revert_head_oid;
+	struct object_id cherry_pick_head_oid;
+};
+
 struct wt_status {
 	int is_initial;
 	char *branch;
@@ -93,6 +111,7 @@ struct wt_status {
 	int rename_score;
 	int rename_limit;
 	enum wt_status_format status_format;
+	struct wt_status_state state;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
 
 	/* These are computed during processing of the individual sections */
@@ -107,29 +126,12 @@ struct wt_status {
 	uint32_t untracked_in_ms;
 };
 
-struct wt_status_state {
-	int merge_in_progress;
-	int am_in_progress;
-	int am_empty_patch;
-	int rebase_in_progress;
-	int rebase_interactive_in_progress;
-	int cherry_pick_in_progress;
-	int bisect_in_progress;
-	int revert_in_progress;
-	int detached_at;
-	char *branch;
-	char *onto;
-	char *detached_from;
-	struct object_id detached_oid;
-	struct object_id revert_head_oid;
-	struct object_id cherry_pick_head_oid;
-};
-
 size_t wt_status_locate_end(const char *s, size_t len);
 void wt_status_add_cut_line(FILE *fp);
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
+void wt_status_collect_free_buffers(struct wt_status *s);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
 int wt_status_check_rebase(const struct worktree *wt,
 			   struct wt_status_state *state);
-- 
2.19.0

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

[Taylor Blau]

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

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

[Junio C Hamano]

Taylor Blau <me@ttaylorr.com> 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(&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?

I am not sure if I follow.

Thanks.

[PATCH v2 0/1] wt-status-state-cleanup

[Stephen P. Smith]

Junio suggested a cleanup patch, jc/wt-status-state-cleanup, which is
the basis for this patch.

This patch uses ss/wt-status-committable.

Updated commit comment and removed one extra blank line insertion.
Stephen P. Smith (1):
  roll wt_status_state into wt_status and populate in the collect phase

 builtin/commit.c |   3 ++
 wt-status.c      | 134 +++++++++++++++++++++--------------------------
 wt-status.h      |  38 +++++++-------
 3 files changed, 82 insertions(+), 93 deletions(-)

-- 
2.19.0

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

[Stephen P. Smith]

Status variables were initialized in the collect phase and some
variables were later freed in the print functions.

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 were added and output
phase started filling the fields.

A "struct wt_status_state" that was used in other codepaths turned out
to be useful in the "git status" output.  This is not tied to "struct
wt_status", so filling in the collect phase was not consistently
followed.

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      | 134 +++++++++++++++++++++--------------------------
 wt-status.h      |  38 +++++++-------
 3 files changed, 82 insertions(+), 93 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d6efd996f8..c91af2fe38 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -508,6 +508,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;
 }
@@ -1390,6 +1391,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 1822e95f65..cc3e84b997 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -744,21 +744,25 @@ 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(&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;
 }
 
+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 +1091,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 +1112,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 +1244,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 +1297,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,
+				 s->state.onto);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently rebasing."));
 }
 
 static void show_rebase_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
-				const char *color)
+				    const char *color)
 {
 	struct stat st;
 
-	show_rebase_information(s, state, color);
+	show_rebase_information(s, color);
 	if (has_unmerged(s)) {
-		print_rebase_state(s, state, color);
+		print_rebase_state(s, color);
 		if (s->hints) {
 			status_printf_ln(s, color,
 				_("  (fix conflicts and then run \"git rebase --continue\")"));
@@ -1326,17 +1325,18 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git rebase --abort\" to check out the original branch)"));
 		}
-	} else if (state->rebase_in_progress || !stat(git_path_merge_msg(the_repository), &st)) {
-		print_rebase_state(s, state, color);
+	} else if (s->state.rebase_in_progress ||
+		   !stat(git_path_merge_msg(the_repository), &st)) {
+		print_rebase_state(s, color);
 		if (s->hints)
 			status_printf_ln(s, color,
 				_("  (all conflicts fixed: run \"git rebase --continue\")"));
 	} else if (split_commit_in_progress(s)) {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit during a rebase."));
@@ -1344,11 +1344,11 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (Once your working directory is clean, run \"git rebase --continue\")"));
 	} else {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit during a rebase."));
@@ -1363,11 +1363,10 @@ static void show_rebase_in_progress(struct wt_status *s,
 }
 
 static void show_cherry_pick_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+					 const char *color)
 {
 	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
-			find_unique_abbrev(&state->cherry_pick_head_oid, DEFAULT_ABBREV));
+			find_unique_abbrev(&s->state.cherry_pick_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1382,11 +1381,10 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 }
 
 static void show_revert_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+				    const char *color)
 {
 	status_printf_ln(s, color, _("You are currently reverting commit %s."),
-			 find_unique_abbrev(&state->revert_head_oid, DEFAULT_ABBREV));
+			 find_unique_abbrev(&s->state.revert_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1401,13 +1399,12 @@ static void show_revert_in_progress(struct wt_status *s,
 }
 
 static void show_bisect_in_progress(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 bisecting, started from branch '%s'."),
-				 state->branch);
+				 s->state.branch);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently bisecting."));
@@ -1581,48 +1578,45 @@ void wt_status_get_state(struct wt_status_state *state,
 		wt_status_get_detached_from(state);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
-				      struct wt_status_state *state)
+static void wt_longstatus_print_state(struct wt_status *s)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
+	struct wt_status_state *state = &s->state;
+
 	if (state->merge_in_progress)
-		show_merge_in_progress(s, state, state_color);
+		show_merge_in_progress(s, state_color);
 	else if (state->am_in_progress)
-		show_am_in_progress(s, state, state_color);
+		show_am_in_progress(s, state_color);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
-		show_rebase_in_progress(s, state, state_color);
+		show_rebase_in_progress(s, state_color);
 	else if (state->cherry_pick_in_progress)
-		show_cherry_pick_in_progress(s, state, state_color);
+		show_cherry_pick_in_progress(s, state_color);
 	else if (state->revert_in_progress)
-		show_revert_in_progress(s, state, state_color);
+		show_revert_in_progress(s, state_color);
 	if (state->bisect_in_progress)
-		show_bisect_in_progress(s, state, state_color);
+		show_bisect_in_progress(s, state_color);
 }
 
 static void wt_longstatus_print(struct wt_status *s)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-	struct wt_status_state state;
-
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state,
-			    s->branch && !strcmp(s->branch, "HEAD"));
 
 	if (s->branch) {
 		const char *on_what = _("On branch ");
 		const char *branch_name = s->branch;
 		if (!strcmp(branch_name, "HEAD")) {
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
-				if (state.rebase_interactive_in_progress)
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress) {
+				if (s->state.rebase_interactive_in_progress)
 					on_what = _("interactive rebase in progress; onto ");
 				else
 					on_what = _("rebase in progress; onto ");
-				branch_name = state.onto;
-			} else if (state.detached_from) {
-				branch_name = state.detached_from;
-				if (state.detached_at)
+				branch_name = s->state.onto;
+			} else if (s->state.detached_from) {
+				branch_name = s->state.detached_from;
+				if (s->state.detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");
@@ -1639,10 +1633,7 @@ static void wt_longstatus_print(struct wt_status *s)
 			wt_longstatus_print_tracking(s);
 	}
 
-	wt_longstatus_print_state(s, &state);
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
+	wt_longstatus_print_state(s);
 
 	if (s->is_initial) {
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
@@ -1946,13 +1937,9 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 	struct branch *branch;
 	const char *base;
 	const char *branch_name;
-	struct wt_status_state state;
 	int ab_info, nr_ahead, nr_behind;
 	char eol = s->null_termination ? '\0' : '\n';
 
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
-
 	fprintf(s->fp, "# branch.oid %s%c",
 			(s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)),
 			eol);
@@ -1963,10 +1950,11 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 		if (!strcmp(s->branch, "HEAD")) {
 			fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
 
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress)
-				branch_name = state.onto;
-			else if (state.detached_from)
-				branch_name = state.detached_from;
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress)
+				branch_name = s->state.onto;
+			else if (s->state.detached_from)
+				branch_name = s->state.detached_from;
 			else
 				branch_name = "";
 		} else {
@@ -2000,10 +1988,6 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 			}
 		}
 	}
-
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
 }
 
 /*
diff --git a/wt-status.h b/wt-status.h
index 937b2c3521..1fcf93afbf 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -64,6 +64,24 @@ enum wt_status_format {
 	STATUS_FORMAT_UNSPECIFIED
 };
 
+struct wt_status_state {
+	int merge_in_progress;
+	int am_in_progress;
+	int am_empty_patch;
+	int rebase_in_progress;
+	int rebase_interactive_in_progress;
+	int cherry_pick_in_progress;
+	int bisect_in_progress;
+	int revert_in_progress;
+	int detached_at;
+	char *branch;
+	char *onto;
+	char *detached_from;
+	struct object_id detached_oid;
+	struct object_id revert_head_oid;
+	struct object_id cherry_pick_head_oid;
+};
+
 struct wt_status {
 	int is_initial;
 	char *branch;
@@ -93,6 +111,7 @@ struct wt_status {
 	int rename_score;
 	int rename_limit;
 	enum wt_status_format status_format;
+	struct wt_status_state state;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
 
 	/* These are computed during processing of the individual sections */
@@ -107,29 +126,12 @@ struct wt_status {
 	uint32_t untracked_in_ms;
 };
 
-struct wt_status_state {
-	int merge_in_progress;
-	int am_in_progress;
-	int am_empty_patch;
-	int rebase_in_progress;
-	int rebase_interactive_in_progress;
-	int cherry_pick_in_progress;
-	int bisect_in_progress;
-	int revert_in_progress;
-	int detached_at;
-	char *branch;
-	char *onto;
-	char *detached_from;
-	struct object_id detached_oid;
-	struct object_id revert_head_oid;
-	struct object_id cherry_pick_head_oid;
-};
-
 size_t wt_status_locate_end(const char *s, size_t len);
 void wt_status_add_cut_line(FILE *fp);
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
+void wt_status_collect_free_buffers(struct wt_status *s);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
 int wt_status_check_rebase(const struct worktree *wt,
 			   struct wt_status_state *state);
-- 
2.19.0

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

[Eric Sunshine]

On Fri, Sep 28, 2018 at 9:55 AM Taylor Blau <me@ttaylorr.com> 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+1KSTswqbnBr4w@mail.gmail.com/

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

[Eric Sunshine]

On Sat, Sep 29, 2018 at 2:55 PM Stephen P. Smith <ischis2@cox.net> wrote:
> Status variables were initialized in the collect phase and some
> variables were later freed in the print functions.
>
> 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 were added and output
> phase started filling the fields.
>
> A "struct wt_status_state" that was used in other codepaths turned out
> to be useful in the "git status" output.  This is not tied to "struct
> wt_status", so filling in the collect phase was not consistently
> followed.
>
> 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.
>
> Signed-off-by: Stephen P. Smith <ischis2@cox.net>

[PATCH v3 0/1] wt-status-state-cleanup

[Stephen P. Smith]

Junio suggested a cleanup patch, jc/wt-status-state-cleanup, which is
the basis for this patch.

This patch uses ss/wt-status-committable.

Update to fix a spelling error in the commet message

Stephen P. Smith (1):
  roll wt_status_state into wt_status and populate in the collect phase

 builtin/commit.c |   3 ++
 wt-status.c      | 134 +++++++++++++++++++++--------------------------
 wt-status.h      |  38 +++++++-------
 3 files changed, 82 insertions(+), 93 deletions(-)

-- 
2.19.0

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

[Stephen P. Smith]

Status variables were initialized in the collect phase and some
variables were later freed in the print functions.

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 were added and output
phase started filling the fields.

A "struct wt_status_state" that was used in other codepaths turned out
to be useful in the "git status" output.  This is not tied to "struct
wt_status", so filling in the collect phase was not consistently
followed.

Move the status state structure variables into the status state
structure and populate them in the collect functions.

Create a new function 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      | 134 +++++++++++++++++++++--------------------------
 wt-status.h      |  38 +++++++-------
 3 files changed, 82 insertions(+), 93 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d6efd996f8..c91af2fe38 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -508,6 +508,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;
 }
@@ -1390,6 +1391,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 1822e95f65..cc3e84b997 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -744,21 +744,25 @@ 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(&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;
 }
 
+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 +1091,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 +1112,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 +1244,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 +1297,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,
+				 s->state.onto);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently rebasing."));
 }
 
 static void show_rebase_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
-				const char *color)
+				    const char *color)
 {
 	struct stat st;
 
-	show_rebase_information(s, state, color);
+	show_rebase_information(s, color);
 	if (has_unmerged(s)) {
-		print_rebase_state(s, state, color);
+		print_rebase_state(s, color);
 		if (s->hints) {
 			status_printf_ln(s, color,
 				_("  (fix conflicts and then run \"git rebase --continue\")"));
@@ -1326,17 +1325,18 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (use \"git rebase --abort\" to check out the original branch)"));
 		}
-	} else if (state->rebase_in_progress || !stat(git_path_merge_msg(the_repository), &st)) {
-		print_rebase_state(s, state, color);
+	} else if (s->state.rebase_in_progress ||
+		   !stat(git_path_merge_msg(the_repository), &st)) {
+		print_rebase_state(s, color);
 		if (s->hints)
 			status_printf_ln(s, color,
 				_("  (all conflicts fixed: run \"git rebase --continue\")"));
 	} else if (split_commit_in_progress(s)) {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently splitting a commit during a rebase."));
@@ -1344,11 +1344,11 @@ static void show_rebase_in_progress(struct wt_status *s,
 			status_printf_ln(s, color,
 				_("  (Once your working directory is clean, run \"git rebase --continue\")"));
 	} else {
-		if (state->branch)
+		if (s->state.branch)
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit while rebasing branch '%s' on '%s'."),
-					 state->branch,
-					 state->onto);
+					 s->state.branch,
+					 s->state.onto);
 		else
 			status_printf_ln(s, color,
 					 _("You are currently editing a commit during a rebase."));
@@ -1363,11 +1363,10 @@ static void show_rebase_in_progress(struct wt_status *s,
 }
 
 static void show_cherry_pick_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+					 const char *color)
 {
 	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
-			find_unique_abbrev(&state->cherry_pick_head_oid, DEFAULT_ABBREV));
+			find_unique_abbrev(&s->state.cherry_pick_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1382,11 +1381,10 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 }
 
 static void show_revert_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
-					const char *color)
+				    const char *color)
 {
 	status_printf_ln(s, color, _("You are currently reverting commit %s."),
-			 find_unique_abbrev(&state->revert_head_oid, DEFAULT_ABBREV));
+			 find_unique_abbrev(&s->state.revert_head_oid, DEFAULT_ABBREV));
 	if (s->hints) {
 		if (has_unmerged(s))
 			status_printf_ln(s, color,
@@ -1401,13 +1399,12 @@ static void show_revert_in_progress(struct wt_status *s,
 }
 
 static void show_bisect_in_progress(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 bisecting, started from branch '%s'."),
-				 state->branch);
+				 s->state.branch);
 	else
 		status_printf_ln(s, color,
 				 _("You are currently bisecting."));
@@ -1581,48 +1578,45 @@ void wt_status_get_state(struct wt_status_state *state,
 		wt_status_get_detached_from(state);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
-				      struct wt_status_state *state)
+static void wt_longstatus_print_state(struct wt_status *s)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
+	struct wt_status_state *state = &s->state;
+
 	if (state->merge_in_progress)
-		show_merge_in_progress(s, state, state_color);
+		show_merge_in_progress(s, state_color);
 	else if (state->am_in_progress)
-		show_am_in_progress(s, state, state_color);
+		show_am_in_progress(s, state_color);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
-		show_rebase_in_progress(s, state, state_color);
+		show_rebase_in_progress(s, state_color);
 	else if (state->cherry_pick_in_progress)
-		show_cherry_pick_in_progress(s, state, state_color);
+		show_cherry_pick_in_progress(s, state_color);
 	else if (state->revert_in_progress)
-		show_revert_in_progress(s, state, state_color);
+		show_revert_in_progress(s, state_color);
 	if (state->bisect_in_progress)
-		show_bisect_in_progress(s, state, state_color);
+		show_bisect_in_progress(s, state_color);
 }
 
 static void wt_longstatus_print(struct wt_status *s)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-	struct wt_status_state state;
-
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state,
-			    s->branch && !strcmp(s->branch, "HEAD"));
 
 	if (s->branch) {
 		const char *on_what = _("On branch ");
 		const char *branch_name = s->branch;
 		if (!strcmp(branch_name, "HEAD")) {
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress) {
-				if (state.rebase_interactive_in_progress)
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress) {
+				if (s->state.rebase_interactive_in_progress)
 					on_what = _("interactive rebase in progress; onto ");
 				else
 					on_what = _("rebase in progress; onto ");
-				branch_name = state.onto;
-			} else if (state.detached_from) {
-				branch_name = state.detached_from;
-				if (state.detached_at)
+				branch_name = s->state.onto;
+			} else if (s->state.detached_from) {
+				branch_name = s->state.detached_from;
+				if (s->state.detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");
@@ -1639,10 +1633,7 @@ static void wt_longstatus_print(struct wt_status *s)
 			wt_longstatus_print_tracking(s);
 	}
 
-	wt_longstatus_print_state(s, &state);
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
+	wt_longstatus_print_state(s);
 
 	if (s->is_initial) {
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
@@ -1946,13 +1937,9 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 	struct branch *branch;
 	const char *base;
 	const char *branch_name;
-	struct wt_status_state state;
 	int ab_info, nr_ahead, nr_behind;
 	char eol = s->null_termination ? '\0' : '\n';
 
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, s->branch && !strcmp(s->branch, "HEAD"));
-
 	fprintf(s->fp, "# branch.oid %s%c",
 			(s->is_initial ? "(initial)" : sha1_to_hex(s->sha1_commit)),
 			eol);
@@ -1963,10 +1950,11 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 		if (!strcmp(s->branch, "HEAD")) {
 			fprintf(s->fp, "# branch.head %s%c", "(detached)", eol);
 
-			if (state.rebase_in_progress || state.rebase_interactive_in_progress)
-				branch_name = state.onto;
-			else if (state.detached_from)
-				branch_name = state.detached_from;
+			if (s->state.rebase_in_progress ||
+			    s->state.rebase_interactive_in_progress)
+				branch_name = s->state.onto;
+			else if (s->state.detached_from)
+				branch_name = s->state.detached_from;
 			else
 				branch_name = "";
 		} else {
@@ -2000,10 +1988,6 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 			}
 		}
 	}
-
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
 }
 
 /*
diff --git a/wt-status.h b/wt-status.h
index 937b2c3521..1fcf93afbf 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -64,6 +64,24 @@ enum wt_status_format {
 	STATUS_FORMAT_UNSPECIFIED
 };
 
+struct wt_status_state {
+	int merge_in_progress;
+	int am_in_progress;
+	int am_empty_patch;
+	int rebase_in_progress;
+	int rebase_interactive_in_progress;
+	int cherry_pick_in_progress;
+	int bisect_in_progress;
+	int revert_in_progress;
+	int detached_at;
+	char *branch;
+	char *onto;
+	char *detached_from;
+	struct object_id detached_oid;
+	struct object_id revert_head_oid;
+	struct object_id cherry_pick_head_oid;
+};
+
 struct wt_status {
 	int is_initial;
 	char *branch;
@@ -93,6 +111,7 @@ struct wt_status {
 	int rename_score;
 	int rename_limit;
 	enum wt_status_format status_format;
+	struct wt_status_state state;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
 
 	/* These are computed during processing of the individual sections */
@@ -107,29 +126,12 @@ struct wt_status {
 	uint32_t untracked_in_ms;
 };
 
-struct wt_status_state {
-	int merge_in_progress;
-	int am_in_progress;
-	int am_empty_patch;
-	int rebase_in_progress;
-	int rebase_interactive_in_progress;
-	int cherry_pick_in_progress;
-	int bisect_in_progress;
-	int revert_in_progress;
-	int detached_at;
-	char *branch;
-	char *onto;
-	char *detached_from;
-	struct object_id detached_oid;
-	struct object_id revert_head_oid;
-	struct object_id cherry_pick_head_oid;
-};
-
 size_t wt_status_locate_end(const char *s, size_t len);
 void wt_status_add_cut_line(FILE *fp);
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);
+void wt_status_collect_free_buffers(struct wt_status *s);
 void wt_status_get_state(struct wt_status_state *state, int get_detached_from);
 int wt_status_check_rebase(const struct worktree *wt,
 			   struct wt_status_state *state);
-- 
2.19.0