[PATCH 0/2] Fix --short and --porcelain options for commit

[PATCH 0/2] Fix --short and --porcelain options for commit

[Samuel Lijin]

Hi all - I last contributed about a year ago and I've finally found the
time to start contributing again, and hopefully I'll stick around this
time. Figured I'd start with something small :)

Samuel Lijin (2):
  commit: fix --short and --porcelain
  wt-status: const-ify all printf helper methods

 t/t7501-commit.sh |  4 ++--
 wt-status.c       | 57 +++++++++++++++++++++++++++++++++++--------------------
 wt-status.h       |  4 ++--
 3 files changed, 40 insertions(+), 25 deletions(-)

-- 
2.16.2

[PATCH 1/2] commit: fix --short and --porcelain

[Samuel Lijin]

Make invoking `git commit` with `--short` or `--porcelain` return status
code zero when there is something to commit.

Mark the commitable flag in the wt_status object in the call to
`wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
and simplify the logic in the latter function to take advantage of the
logic shifted to the former.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh |  4 ++--
 wt-status.c       | 39 +++++++++++++++++++++++++++------------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..85a8217fd 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,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
 '
diff --git a/wt-status.c b/wt-status.c
index 50815e5fa..26b0a6221 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,19 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static void wt_status_mark_commitable(struct wt_status *s) {
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+
+		if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
+			s->commitable = 1;
+			return;
+		}
+	}
+}
+
 void wt_status_collect(struct wt_status *s)
 {
 	wt_status_collect_changes_worktree(s);
@@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
 		wt_status_collect_changes_initial(s);
 	else
 		wt_status_collect_changes_index(s);
+
 	wt_status_collect_untracked(s);
+
+	wt_status_mark_commitable(s);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
 
 static void wt_longstatus_print_updated(struct wt_status *s)
 {
-	int shown_header = 0;
-	int i;
+	if (!s->commitable) {
+		return;
+	}
+
+	wt_longstatus_print_cached_header(s);
 
+	int i;
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		struct string_list_item *it;
 		it = &(s->change.items[i]);
 		d = it->util;
-		if (!d->index_status ||
-		    d->index_status == DIFF_STATUS_UNMERGED)
-			continue;
-		if (!shown_header) {
-			wt_longstatus_print_cached_header(s);
-			s->commitable = 1;
-			shown_header = 1;
+		if (d->index_status &&
+		    d->index_status != DIFF_STATUS_UNMERGED) {
+			wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 		}
-		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 	}
-	if (shown_header)
-		wt_longstatus_print_trailer(s);
+
+	wt_longstatus_print_trailer(s);
 }
 
 /*
-- 
2.16.2

Re: [PATCH 1/2] commit: fix --short and --porcelain

[Martin Ågren]

Hi Samuel,

Welcome back. :-)

On 18 April 2018 at 05:06, Samuel Lijin <sxlijin@gmail.com> wrote:
> Make invoking `git commit` with `--short` or `--porcelain` return status
> code zero when there is something to commit.
>
> Mark the commitable flag in the wt_status object in the call to
> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> and simplify the logic in the latter function to take advantage of the
> logic shifted to the former.

The subject is sort of vague about what is being fixed. Maybe "commit:
fix return code of ...", or "wt-status: set `commitable` when
collecting, not when printing". Or something... I can't come up with
something brilliant off the top of my head.

I did not understand the first paragraph until I had read the second and
peaked at the code. Maybe tell the story the other way around? Something
like this:

  Mark the `commitable` flag in the wt_status object in
  `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
  and simplify the logic in the latter function to take advantage of the
  logic shifted to the former.

  This means that callers do need to actually use the printer function
  to collect the `commitable` flag -- it is sufficient to call
  `wt_status_collect()`.

  As a result, invoking `git commit` with `--short` or `--porcelain`
  results in return status code zero when there is something to commit.
  This fixes two bugs documented in our test suite.

>  t/t7501-commit.sh |  4 ++--
>  wt-status.c       | 39 +++++++++++++++++++++++++++------------
>  2 files changed, 29 insertions(+), 14 deletions(-)

I tried to find somewhere in the documentation where this bug was
described (git-commit.txt or git-status.txt), but failed. So there
should be nothing to update there.

> +static void wt_status_mark_commitable(struct wt_status *s) {
> +       int i;
> +
> +       for (i = 0; i < s->change.nr; i++) {
> +               struct wt_status_change_data *d = (s->change.items[i]).util;
> +
> +               if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
> +                       s->commitable = 1;
> +                       return;
> +               }
> +       }
> +}

This helper does exactly what the old code did inside
`wt_longstatus_print_updated()` with regards to `commitable`. Ok.

This function does not reset `commitable` to 0, so reusing a `struct
wt_status` won't necessarily work out. I have not thought about whether
such a caller would be horribly broken for other reasons...

>  void wt_status_collect(struct wt_status *s)
>  {
>         wt_status_collect_changes_worktree(s);
> @@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
>                 wt_status_collect_changes_initial(s);
>         else
>                 wt_status_collect_changes_index(s);
> +
>         wt_status_collect_untracked(s);
> +
> +       wt_status_mark_commitable(s);
>  }

So whenever we `..._collect()`, `commitable` is set for us. This is the
only caller of the new helper, so in order to be able to trust
`commitable`, one needs to call `wt_status_collect()`. Seems a
reasonable assumption to make that the caller will remember to do so
before printing. (And all current users do, so we're not regressing in
some user.)

>  static void wt_longstatus_print_unmerged(struct wt_status *s)
> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
>
>  static void wt_longstatus_print_updated(struct wt_status *s)
>  {
> -       int shown_header = 0;
> -       int i;
> +       if (!s->commitable) {
> +               return;
> +       }

Regarding my comment above: If you forget to `..._collect()` first, this
function is a no-op.

> +
> +       wt_longstatus_print_cached_header(s);
>
> +       int i;

You should leave this variable declaration at the top of the function.

>         for (i = 0; i < s->change.nr; i++) {
>                 struct wt_status_change_data *d;
>                 struct string_list_item *it;
>                 it = &(s->change.items[i]);
>                 d = it->util;
> -               if (!d->index_status ||
> -                   d->index_status == DIFF_STATUS_UNMERGED)
> -                       continue;
> -               if (!shown_header) {
> -                       wt_longstatus_print_cached_header(s);
> -                       s->commitable = 1;
> -                       shown_header = 1;
> +               if (d->index_status &&
> +                   d->index_status != DIFF_STATUS_UNMERGED) {
> +                       wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
>                 }
> -               wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
>         }
> -       if (shown_header)
> -               wt_longstatus_print_trailer(s);
> +
> +       wt_longstatus_print_trailer(s);
>  }

This rewrite matches the original logic, assuming we can trust
`commitable`. The result is a function called `print()` which does not
modify the struct it is given for printing. Nice. So you can make the
argument a `const struct wt_status *`. Except this function uses helpers
that are missing the `const`.

You fix that in patch 2/2. I would probably have made that patch as 1/2,
then done this patch as 2/2 ending the commit message with something
like "As a result, we can mark the argument as `const`.", or even just
silently inserting the `const` for this one function. Just a thought.

Martin

Re: [PATCH 1/2] commit: fix --short and --porcelain

[Samuel Lijin]

On Wed, Apr 18, 2018 at 8:55 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> Thanks for the quick review!
>
> On Wed, Apr 18, 2018 at 11:38 AM, Martin Ågren <martin.agren@gmail.com> wrote:
>> Hi Samuel,
>>
>> Welcome back. :-)
>>
>> On 18 April 2018 at 05:06, Samuel Lijin <sxlijin@gmail.com> wrote:
>>> Make invoking `git commit` with `--short` or `--porcelain` return status
>>> code zero when there is something to commit.
>>>
>>> Mark the commitable flag in the wt_status object in the call to
>>> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
>>> and simplify the logic in the latter function to take advantage of the
>>> logic shifted to the former.
>>
>> The subject is sort of vague about what is being fixed. Maybe "commit:
>> fix return code of ...", or "wt-status: set `commitable` when
>> collecting, not when printing". Or something... I can't come up with
>> something brilliant off the top of my head.
>>
>> I did not understand the first paragraph until I had read the second and
>> peaked at the code. Maybe tell the story the other way around? Something
>> like this:
>>
>>   Mark the `commitable` flag in the wt_status object in
>>   `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
>>   and simplify the logic in the latter function to take advantage of the
>>   logic shifted to the former.
>>
>>   This means that callers do need to actually use the printer function
>>   to collect the `commitable` flag -- it is sufficient to call
>>   `wt_status_collect()`.
>>
>>   As a result, invoking `git commit` with `--short` or `--porcelain`
>>   results in return status code zero when there is something to commit.
>>   This fixes two bugs documented in our test suite.
>
> That definitely works better. Will fix when I reroll.
>
>>>  t/t7501-commit.sh |  4 ++--
>>>  wt-status.c       | 39 +++++++++++++++++++++++++++------------
>>>  2 files changed, 29 insertions(+), 14 deletions(-)
>>
>> I tried to find somewhere in the documentation where this bug was
>> described (git-commit.txt or git-status.txt), but failed. So there
>> should be nothing to update there.
>>
>>> +static void wt_status_mark_commitable(struct wt_status *s) {
>>> +       int i;
>>> +
>>> +       for (i = 0; i < s->change.nr; i++) {
>>> +               struct wt_status_change_data *d = (s->change.items[i]).util;
>>> +
>>> +               if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
>>> +                       s->commitable = 1;
>>> +                       return;
>>> +               }
>>> +       }
>>> +}
>>
>> This helper does exactly what the old code did inside
>> `wt_longstatus_print_updated()` with regards to `commitable`. Ok.
>>
>> This function does not reset `commitable` to 0, so reusing a `struct
>> wt_status` won't necessarily work out. I have not thought about whether
>> such a caller would be horribly broken for other reasons...
>>
>>>  void wt_status_collect(struct wt_status *s)
>>>  {
>>>         wt_status_collect_changes_worktree(s);
>>> @@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
>>>                 wt_status_collect_changes_initial(s);
>>>         else
>>>                 wt_status_collect_changes_index(s);
>>> +
>>>         wt_status_collect_untracked(s);
>>> +
>>> +       wt_status_mark_commitable(s);
>>>  }
>>
>> So whenever we `..._collect()`, `commitable` is set for us. This is the
>> only caller of the new helper, so in order to be able to trust
>> `commitable`, one needs to call `wt_status_collect()`. Seems a
>> reasonable assumption to make that the caller will remember to do so
>> before printing. (And all current users do, so we're not regressing in
>> some user.)
>>
>>>  static void wt_longstatus_print_unmerged(struct wt_status *s)
>>> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
>>>
>>>  static void wt_longstatus_print_updated(struct wt_status *s)
>>>  {
>>> -       int shown_header = 0;
>>> -       int i;
>>> +       if (!s->commitable) {
>>> +               return;
>>> +       }
>>
>> Regarding my comment above: If you forget to `..._collect()` first, this
>> function is a no-op.
>>
>>> +
>>> +       wt_longstatus_print_cached_header(s);
>>>
>>> +       int i;
>>
>> You should leave this variable declaration at the top of the function.
>>
>>>         for (i = 0; i < s->change.nr; i++) {
>>>                 struct wt_status_change_data *d;
>>>                 struct string_list_item *it;
>>>                 it = &(s->change.items[i]);
>>>                 d = it->util;
>>> -               if (!d->index_status ||
>>> -                   d->index_status == DIFF_STATUS_UNMERGED)
>>> -                       continue;
>>> -               if (!shown_header) {
>>> -                       wt_longstatus_print_cached_header(s);
>>> -                       s->commitable = 1;
>>> -                       shown_header = 1;
>>> +               if (d->index_status &&
>>> +                   d->index_status != DIFF_STATUS_UNMERGED) {
>>> +                       wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
>>>                 }
>>> -               wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
>>>         }
>>> -       if (shown_header)
>>> -               wt_longstatus_print_trailer(s);
>>> +
>>> +       wt_longstatus_print_trailer(s);
>>>  }
>>
>> This rewrite matches the original logic, assuming we can trust
>> `commitable`. The result is a function called `print()` which does not
>> modify the struct it is given for printing. Nice. So you can make the
>> argument a `const struct wt_status *`. Except this function uses helpers
>> that are missing the `const`.
>>
>> You fix that in patch 2/2. I would probably have made that patch as 1/2,
>> then done this patch as 2/2 ending the commit message with something
>> like "As a result, we can mark the argument as `const`.", or even just
>> silently inserting the `const` for this one function. Just a thought.
>
> I originally ordered it the way I did because in the constify-first
> scenario, "fix t7501" and "const-ify wt_longstatus_print_updated"
> seemed like two logically separate patches to me (which would have
> made the patch series three patches instead of two). I'm happy to
> reroll in whichever fashion if people care strongly though.
>
>> Martin

Re: [PATCH 1/2] commit: fix --short and --porcelain

[Eric Sunshine]

On Tue, Apr 17, 2018 at 11:06 PM, Samuel Lijin <sxlijin@gmail.com> wrote:
> Make invoking `git commit` with `--short` or `--porcelain` return status
> code zero when there is something to commit.
>
> Mark the commitable flag in the wt_status object in the call to
> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> and simplify the logic in the latter function to take advantage of the
> logic shifted to the former.
>
> Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
> ---
> diff --git a/wt-status.c b/wt-status.c
> @@ -754,26 +770,25 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
>  static void wt_longstatus_print_updated(struct wt_status *s)
>  {
> -       int shown_header = 0;
> -       int i;
> +       if (!s->commitable) {
> +               return;
> +       }
> +
> +       wt_longstatus_print_cached_header(s);
>
> +       int i;
>         for (i = 0; i < s->change.nr; i++) {

Declaration after statement: Declare 'i' at the top of the function as
it was before this patch.

[PATCH 2/2] wt-status: const-ify all printf helper methods

[Samuel Lijin]

Change the method signatures of all printf helper methods to take a
`const struct wt_status *` rather than a `struct wt_status *`.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 wt-status.c | 18 +++++++++---------
 wt-status.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 26b0a6221..55d29bc09 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
 	const char *c = "";
 	if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char *color,
 		const char *fmt, va_list ap, const char *trail)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 	strbuf_release(&sb);
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
 			       const char *fmt, ...)
 {
 	va_list ap;
@@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
@@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct wt_status *s,
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
 	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
 	strbuf_release(&onebuf);
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
 					    int change_type,
 					    struct string_list_item *it)
 {
@@ -768,7 +768,7 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
 	if (!s->commitable) {
 		return;
diff --git a/wt-status.h b/wt-status.h
index 430770b85..83a1f7c00 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -135,9 +135,9 @@ int wt_status_check_bisect(const struct worktree *wt,
 			   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf(const struct wt_status *s, const char *color, const char *fmt, ...);
 
 /* The following functions expect that the caller took care of reading the index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.16.2

[PATCH v2 0/2] Fix --short and --porcelain options for commit

[Samuel Lijin]

Rerolling patch series to fix t7501.

Samuel Lijin (2):
  commit: fix --short and --porcelain options
  wt-status: const-ify all printf helper methods

 t/t7501-commit.sh |  4 ++--
 wt-status.c       | 56 ++++++++++++++++++++++++++++++-----------------
 wt-status.h       |  4 ++--
 3 files changed, 40 insertions(+), 24 deletions(-)

-- 
2.17.0

[PATCH v3 0/3] Fix --short/--porcelain options for git commit

[Samuel Lijin]

Take 3. Addressed the issue that Junio turned up the last time I sent
this out for review.

I'm not entirely sure I like the way I added the tests in the first
patch, but it's unclear to me if there's actually a pattern for setting
up and tearing down the same env for multiple test methods. There are
also other tests in t7501 that rely on state left from earlier tests, so
it's not really clear to me what the best thing to do here is.

Also added a FIXME in the second patch for something I think should be
fixed, but doesn't make sense to fix in this patch series.

Samuel Lijin (3):
  t7501: add merge conflict tests for dry run
  wt-status: teach wt_status_collect about merges in progress
  commit: fix exit code for --short/--porcelain

 builtin/commit.c  |  32 +++---
 ref-filter.c      |   3 +-
 t/t7501-commit.sh |  49 +++++++--
 wt-status.c       | 260 +++++++++++++++++++++++++---------------------
 wt-status.h       |  13 +--
 5 files changed, 208 insertions(+), 149 deletions(-)

-- 
2.18.0

[PATCH v4 0/4] Rerolling patch series to fix t7501

[Samuel Lijin]

Following up on Junio's review from last time.

Samuel Lijin (4):
  t7501: add coverage for flags which imply dry runs
  wt-status: rename commitable to committable
  wt-status: teach wt_status_collect about merges in progress
  commit: fix exit code when doing a dry run

 builtin/commit.c  |  32 +++---
 ref-filter.c      |   3 +-
 t/t7501-commit.sh | 150 ++++++++++++++++++++++++---
 wt-status.c       | 258 ++++++++++++++++++++++++----------------------
 wt-status.h       |  13 +--
 5 files changed, 298 insertions(+), 158 deletions(-)

-- 
2.18.0

Re: [PATCH v4 0/4] Rerolling patch series to fix t7501

[Junio C Hamano]

Samuel Lijin <sxlijin@gmail.com> writes:

> Following up on Junio's review from last time.
>
> Samuel Lijin (4):
>   t7501: add coverage for flags which imply dry runs
>   wt-status: rename commitable to committable
>   wt-status: teach wt_status_collect about merges in progress
>   commit: fix exit code when doing a dry run
>
>  builtin/commit.c  |  32 +++---
>  ref-filter.c      |   3 +-
>  t/t7501-commit.sh | 150 ++++++++++++++++++++++++---
>  wt-status.c       | 258 ++++++++++++++++++++++++----------------------
>  wt-status.h       |  13 +--
>  5 files changed, 298 insertions(+), 158 deletions(-)


It seems that t7512 & t7060 break with this topic queued.

[PATCH v4 1/4] t7501: add coverage for flags which imply dry runs

[Samuel Lijin]

The behavior of git commit, when doing a dry run, changes if there are
unresolved/resolved merge conflicts, but the test suite currently only
asserts that `git commit --dry-run` succeeds when all merge conflicts
are resolved.

Add tests to document the behavior of all flags (i.e. `--dry-run`,
`--short`, `--porcelain`, and `--long`) which imply a dry run when (1)
there are only unresolved merge conflicts, (2) when there are both
unresolved and resolved merge conflicts, and (3) when all merge
conflicts are resolved.

When testing behavior involving resolved merge conflicts, resolve merge
conflicts by replacing each merge conflict with completely new contents,
rather than choosing the contents associated with one of the parent
commits, since the latter decision has no bearing on the behavior of a
dry run commit invocation.

Verify that a dry run invocation of git commit does not create a new
commit by asserting that HEAD has not changed, instead of by crafting
the commit.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh | 146 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 132 insertions(+), 14 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index 9dbbd01fc..e49dfd0a2 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -664,24 +664,142 @@ test_expect_success '--only works on to-be-born branch' '
 	test_cmp expected actual
 '
 
-test_expect_success '--dry-run with conflicts fixed from a merge' '
-	# setup two branches with conflicting information
-	# in the same file, resolve the conflict,
-	# call commit with --dry-run
-	echo "Initial contents, unimportant" >test-file &&
-	git add test-file &&
+test_expect_success 'prepare commits that can be used to trigger a merge conflict' '
+	# setup two branches with conflicting contents in two paths
+	echo "Initial contents, unimportant" | tee test-file1 test-file2 &&
+	git add test-file1 test-file2 &&
 	git commit -m "Initial commit" &&
-	echo "commit-1-state" >test-file &&
-	git commit -m "commit 1" -i test-file &&
+	echo "commit-1-state" | tee test-file1 test-file2 &&
+	git commit -m "commit 1" -i test-file1 test-file2 &&
 	git tag commit-1 &&
 	git checkout -b branch-2 HEAD^1 &&
-	echo "commit-2-state" >test-file &&
-	git commit -m "commit 2" -i test-file &&
-	! $(git merge --no-commit commit-1) &&
-	echo "commit-2-state" >test-file &&
-	git add test-file &&
+	echo "commit-2-state" | tee test-file1 test-file2 &&
+	git commit -m "commit 2" -i test-file1 test-file2 &&
+	git tag commit-2
+'
+
+test_expect_success '--dry-run with only unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	test_must_fail git commit --dry-run &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--short with only unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	test_must_fail git commit --short &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--porcelain with only unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	test_must_fail git commit --porcelain &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--long with only unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	test_must_fail git commit --long &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure '--dry-run with resolved and unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve one merge conflict" >test-file1 &&
+	git add test-file1 &&
+	test_must_fail git commit --dry-run &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--short with resolved and unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve one merge conflict" >test-file1 &&
+	git add test-file1 &&
+	test_must_fail git commit --short &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--porcelain with resolved and unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve one merge conflict" >test-file1 &&
+	git add test-file1 &&
+	test_must_fail git commit --porcelain &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure '--long with resolved and unresolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve one merge conflict" >test-file1 &&
+	git add test-file1 &&
+	test_must_fail git commit --long &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--dry-run with only resolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
+	git add test-file1 test-file2 &&
 	git commit --dry-run &&
-	git commit -m "conflicts fixed from merge."
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure '--short with only resolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
+	git add test-file1 test-file2 &&
+	git commit --short &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_failure '--porcelain with only resolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
+	git add test-file1 test-file2 &&
+	git commit --porcelain &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
+'
+
+test_expect_success '--long with only resolved merge conflicts' '
+	git reset --hard commit-2 &&
+	test_must_fail git merge --no-commit commit-1 &&
+	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
+	git add test-file1 test-file2 &&
+	git commit --long &&
+	git rev-parse commit-2 >expected &&
+	git rev-parse HEAD >actual &&
+	test_cmp expected actual
 '
 
 test_done
-- 
2.18.0

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

[Samuel Lijin]

Fix a typo in the name of the committable bit in wt_status_state.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 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 158e3f843..32f9db33b 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 8827a256d..18ea333a5 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -773,7 +773,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);
@@ -1008,7 +1008,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 v4 3/4] wt-status: teach wt_status_collect about merges in progress

[Samuel Lijin]

To fix the breakages documented by t7501, the next patch in this series
will teach wt_status_collect() how to set the committable bit, instead
of having wt_longstatus_print_updated() and show_merge_in_progress() set
it (which is what currently happens). To set the committable bit
correctly, however, wt_status_collect() needs to know whether or not
there is a merge in progress (if a merge is in progress, the bit (1)
should not be set if there are unresolved merge conflicts and (2) should
be set even if the index is the same as HEAD), so teach its (two)
callers to create, initialize, and pass in
instances of wt_status_state, which records this metadata.

Since wt_longstatus_print() and show_merge_in_progress() are in the same
callpaths and currently create and init copies of wt_status_state,
remove that logic and instead pass wt_status_state through.

Make wt_status_get_state() easier to use, add a helper method to clean up
wt_status_state, const-ify as many struct pointers in method signatures
as possible, and add a FIXME for a struct pointer which should be const
but isn't (that this patch series will not address).

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
gitster: I kept the FIXME around because it wasn't clear whether or not
you were opposed to it. For what it's worth, there are only two
callsites that can't be const-ified because of this one item.

 builtin/commit.c |  14 ++--
 ref-filter.c     |   3 +-
 wt-status.c      | 178 +++++++++++++++++++++++------------------------
 wt-status.h      |  11 +--
 4 files changed, 105 insertions(+), 101 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 32f9db33b..dd3e83053 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -485,6 +485,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,
 		      struct wt_status *s)
 {
+	struct wt_status_state state;
 	struct object_id oid;
 
 	if (s->relative_paths)
@@ -504,8 +505,10 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	s->status_format = status_format;
 	s->ignore_submodule_arg = ignore_submodule_arg;
 
-	wt_status_collect(s);
-	wt_status_print(s);
+	wt_status_get_state(s, &state);
+	wt_status_collect(s, &state);
+	wt_status_print(s, &state);
+	wt_status_clear_state(&state);
 
 	return s->committable;
 }
@@ -1295,6 +1298,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	static int no_renames = -1;
 	static const char *rename_score_arg = (const char *)-1;
 	static struct wt_status s;
+	struct wt_status_state state;
 	int fd;
 	struct object_id oid;
 	static struct option builtin_status_options[] = {
@@ -1379,7 +1383,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			s.rename_score = parse_rename_score(&rename_score_arg);
 	}
 
-	wt_status_collect(&s);
+	wt_status_get_state(&s, &state);
+	wt_status_collect(&s, &state);
 
 	if (0 <= fd)
 		update_index_if_able(&the_index, &index_lock);
@@ -1387,7 +1392,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	if (s.relative_paths)
 		s.prefix = prefix;
 
-	wt_status_print(&s);
+	wt_status_print(&s, &state);
+	wt_status_clear_state(&state);
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 492f2b770..bc9b6b274 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1381,8 +1381,7 @@ char *get_head_description(void)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct wt_status_state state;
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, 1);
+	wt_status_get_state(NULL, &state);
 	if (state.rebase_in_progress ||
 	    state.rebase_interactive_in_progress) {
 		if (state.branch)
diff --git a/wt-status.c b/wt-status.c
index 18ea333a5..af83fae68 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
 	const char *c = "";
 	if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char *color,
 		const char *fmt, va_list ap, const char *trail)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 	strbuf_release(&sb);
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
 			       const char *fmt, ...)
 {
 	va_list ap;
@@ -143,7 +143,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->rename_limit = -1;
 }
 
-static void wt_longstatus_print_unmerged_header(struct wt_status *s)
+static void wt_longstatus_print_unmerged_header(const struct wt_status *s)
 {
 	int i;
 	int del_mod_conflict = 0;
@@ -195,7 +195,7 @@ static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
@@ -211,7 +211,7 @@ static void wt_longstatus_print_cached_header(struct wt_status *s)
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_dirty_header(struct wt_status *s,
+static void wt_longstatus_print_dirty_header(const struct wt_status *s,
 					     int has_deleted,
 					     int has_dirty_submodules)
 {
@@ -230,7 +230,7 @@ static void wt_longstatus_print_dirty_header(struct wt_status *s,
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_other_header(struct wt_status *s,
+static void wt_longstatus_print_other_header(const struct wt_status *s,
 					     const char *what,
 					     const char *how)
 {
@@ -242,7 +242,7 @@ static void wt_longstatus_print_other_header(struct wt_status *s,
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
 	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -308,7 +308,7 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval)
 	return result;
 }
 
-static void wt_longstatus_print_unmerged_data(struct wt_status *s,
+static void wt_longstatus_print_unmerged_data(const struct wt_status *s,
 					      struct string_list_item *it)
 {
 	const char *c = color(WT_STATUS_UNMERGED, s);
@@ -335,7 +335,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
 	strbuf_release(&onebuf);
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
 					    int change_type,
 					    struct string_list_item *it)
 {
@@ -724,7 +724,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
-void wt_status_collect(struct wt_status *s)
+void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 {
 	wt_status_collect_changes_worktree(s);
 
@@ -732,10 +732,11 @@ void wt_status_collect(struct wt_status *s)
 		wt_status_collect_changes_initial(s);
 	else
 		wt_status_collect_changes_index(s);
+
 	wt_status_collect_untracked(s);
 }
 
-static void wt_longstatus_print_unmerged(struct wt_status *s)
+static void wt_longstatus_print_unmerged(const struct wt_status *s)
 {
 	int shown_header = 0;
 	int i;
@@ -787,7 +788,7 @@ static void wt_longstatus_print_updated(struct wt_status *s)
  *  0 : no change
  *  1 : some change but no delete
  */
-static int wt_status_check_worktree_changes(struct wt_status *s,
+static int wt_status_check_worktree_changes(const struct wt_status *s,
 					     int *dirty_submodules)
 {
 	int i;
@@ -811,7 +812,7 @@ static int wt_status_check_worktree_changes(struct wt_status *s,
 	return changes;
 }
 
-static void wt_longstatus_print_changed(struct wt_status *s)
+static void wt_longstatus_print_changed(const struct wt_status *s)
 {
 	int i, dirty_submodules;
 	int worktree_changes = wt_status_check_worktree_changes(s, &dirty_submodules);
@@ -843,7 +844,7 @@ static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
 	return 0;
 }
 
-static void wt_longstatus_print_stash_summary(struct wt_status *s)
+static void wt_longstatus_print_stash_summary(const struct wt_status *s)
 {
 	int stash_count = 0;
 
@@ -855,7 +856,7 @@ static void wt_longstatus_print_stash_summary(struct wt_status *s)
 				 stash_count);
 }
 
-static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncommitted)
+static void wt_longstatus_print_submodule_summary(const struct wt_status *s, int uncommitted)
 {
 	struct child_process sm_summary = CHILD_PROCESS_INIT;
 	struct strbuf cmd_stdout = STRBUF_INIT;
@@ -901,8 +902,8 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
 	strbuf_release(&summary);
 }
 
-static void wt_longstatus_print_other(struct wt_status *s,
-				      struct string_list *l,
+static void wt_longstatus_print_other(const struct wt_status *s,
+				      const struct string_list *l,
 				      const char *what,
 				      const char *how)
 {
@@ -975,7 +976,7 @@ void wt_status_add_cut_line(FILE *fp)
 	strbuf_release(&buf);
 }
 
-static void wt_longstatus_print_verbose(struct wt_status *s)
+static void wt_longstatus_print_verbose(const struct wt_status *s)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
@@ -1029,7 +1030,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 	}
 }
 
-static void wt_longstatus_print_tracking(struct wt_status *s)
+static void wt_longstatus_print_tracking(const struct wt_status *s)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *cp, *ep, *branch_name;
@@ -1063,7 +1064,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(struct wt_status *s)
+static int has_unmerged(const struct wt_status *s)
 {
 	int i;
 
@@ -1077,7 +1078,7 @@ static int has_unmerged(struct wt_status *s)
 }
 
 static void show_merge_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	if (has_unmerged(s)) {
@@ -1099,8 +1100,8 @@ static void show_merge_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_am_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+static void show_am_in_progress(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	status_printf_ln(s, color,
@@ -1138,7 +1139,7 @@ static char *read_line_from_git_path(const char *filename)
 	}
 }
 
-static int split_commit_in_progress(struct wt_status *s)
+static int split_commit_in_progress(const struct wt_status *s)
 {
 	int split_in_progress = 0;
 	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
@@ -1232,8 +1233,8 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
 	return 0;
 }
 
-static void show_rebase_information(struct wt_status *s,
-					struct wt_status_state *state,
+static void show_rebase_information(const struct wt_status *s,
+					const struct wt_status_state *state,
 					const char *color)
 {
 	if (state->rebase_interactive_in_progress) {
@@ -1286,8 +1287,8 @@ static void show_rebase_information(struct wt_status *s,
 	}
 }
 
-static void print_rebase_state(struct wt_status *s,
-				struct wt_status_state *state,
+static void print_rebase_state(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	if (state->branch)
@@ -1300,8 +1301,8 @@ static void print_rebase_state(struct wt_status *s,
 				 _("You are currently rebasing."));
 }
 
-static void show_rebase_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+static void show_rebase_in_progress(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	struct stat st;
@@ -1353,8 +1354,8 @@ static void show_rebase_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_cherry_pick_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
+static void show_cherry_pick_in_progress(const struct wt_status *s,
+					const struct wt_status_state *state,
 					const char *color)
 {
 	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
@@ -1372,8 +1373,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_revert_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
+static void show_revert_in_progress(const struct wt_status *s,
+					const struct wt_status_state *state,
 					const char *color)
 {
 	status_printf_ln(s, color, _("You are currently reverting commit %s."),
@@ -1391,8 +1392,8 @@ static void show_revert_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_bisect_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+static void show_bisect_in_progress(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	if (state->branch)
@@ -1546,12 +1547,16 @@ int wt_status_check_bisect(const struct worktree *wt,
 	return 0;
 }
 
-void wt_status_get_state(struct wt_status_state *state,
-			 int get_detached_from)
+void wt_status_get_state(
+		const struct wt_status *s, struct wt_status_state *state)
 {
+	int get_detached_from =
+		(s == NULL) || (s->branch && !strcmp(s->branch, "HEAD"));
 	struct stat st;
 	struct object_id oid;
 
+	memset(state, 0, sizeof(*state));
+
 	if (!stat(git_path_merge_head(the_repository), &st)) {
 		state->merge_in_progress = 1;
 	} else if (wt_status_check_rebase(NULL, state)) {
@@ -1572,8 +1577,15 @@ void wt_status_get_state(struct wt_status_state *state,
 		wt_status_get_detached_from(state);
 }
 
+void wt_status_clear_state(struct wt_status_state *state)
+{
+	free(state->branch);
+	free(state->onto);
+	free(state->detached_from);
+}
+
 static void wt_longstatus_print_state(struct wt_status *s,
-				      struct wt_status_state *state)
+				      const struct wt_status_state *state)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
 	if (state->merge_in_progress)
@@ -1590,30 +1602,25 @@ static void wt_longstatus_print_state(struct wt_status *s,
 		show_bisect_in_progress(s, state, state_color);
 }
 
-static void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s, const struct wt_status_state *state)
 {
 	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 (state->rebase_in_progress || state->rebase_interactive_in_progress) {
+				if (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 = state->onto;
+			} else if (state->detached_from) {
+				branch_name = state->detached_from;
+				if (state->detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");
@@ -1630,10 +1637,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, state);
 
 	if (s->is_initial) {
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
@@ -1708,7 +1712,7 @@ static void wt_longstatus_print(struct wt_status *s)
 }
 
 static void wt_shortstatus_unmerged(struct string_list_item *it,
-			   struct wt_status *s)
+			   const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 	const char *how = "??";
@@ -1735,7 +1739,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 }
 
 static void wt_shortstatus_status(struct string_list_item *it,
-			 struct wt_status *s)
+			 const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 
@@ -1778,7 +1782,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 }
 
 static void wt_shortstatus_other(struct string_list_item *it,
-				 struct wt_status *s, const char *sign)
+				 const struct wt_status *s, const char *sign)
 {
 	if (s->null_termination) {
 		fprintf(stdout, "%s %s%c", sign, it->string, 0);
@@ -1792,7 +1796,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	}
 }
 
-static void wt_shortstatus_print_tracking(struct wt_status *s)
+static void wt_shortstatus_print_tracking(const struct wt_status *s)
 {
 	struct branch *branch;
 	const char *header_color = color(WT_STATUS_HEADER, s);
@@ -1868,7 +1872,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-static void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(const struct wt_status *s)
 {
 	struct string_list_item *it;
 
@@ -1932,18 +1936,14 @@ static void wt_porcelain_print(struct wt_status *s)
  * upstream.  When AHEAD_BEHIND_QUICK is requested and the branches
  * are different, '?' will be substituted for the actual count.
  */
-static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+static void wt_porcelain_v2_print_tracking(const struct wt_status *s, const struct wt_status_state *state)
 {
 	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);
@@ -1954,10 +1954,10 @@ 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 (state->rebase_in_progress || state->rebase_interactive_in_progress)
+				branch_name = state->onto;
+			else if (state->detached_from)
+				branch_name = state->detached_from;
 			else
 				branch_name = "";
 		} else {
@@ -1991,10 +1991,6 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 			}
 		}
 	}
-
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
 }
 
 /*
@@ -2002,7 +1998,7 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
  * fixed-length string of characters in the buffer provided.
  */
 static void wt_porcelain_v2_submodule_state(
-	struct wt_status_change_data *d,
+	const struct wt_status_change_data *d,
 	char sub[5])
 {
 	if (S_ISGITLINK(d->mode_head) ||
@@ -2025,8 +2021,8 @@ static void wt_porcelain_v2_submodule_state(
  * Fix-up changed entries before we print them.
  */
 static void wt_porcelain_v2_fix_up_changed(
-	struct string_list_item *it,
-	struct wt_status *s)
+	const struct string_list_item *it,
+	const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 
@@ -2074,8 +2070,8 @@ static void wt_porcelain_v2_fix_up_changed(
  * Print porcelain v2 info for tracked entries with changes.
  */
 static void wt_porcelain_v2_print_changed_entry(
-	struct string_list_item *it,
-	struct wt_status *s)
+	const struct string_list_item *it,
+	const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 	struct strbuf buf = STRBUF_INIT;
@@ -2138,8 +2134,8 @@ static void wt_porcelain_v2_print_changed_entry(
  * Print porcelain v2 status info for unmerged entries.
  */
 static void wt_porcelain_v2_print_unmerged_entry(
-	struct string_list_item *it,
-	struct wt_status *s)
+	const struct string_list_item *it,
+	const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 	const struct cache_entry *ce;
@@ -2219,8 +2215,8 @@ static void wt_porcelain_v2_print_unmerged_entry(
  * Print porcelain V2 status info for untracked and ignored entries.
  */
 static void wt_porcelain_v2_print_other(
-	struct string_list_item *it,
-	struct wt_status *s,
+	const struct string_list_item *it,
+	const struct wt_status *s,
 	char prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -2250,14 +2246,14 @@ static void wt_porcelain_v2_print_other(
  * [<v2_ignored_items>]*
  *
  */
-static void wt_porcelain_v2_print(struct wt_status *s)
+static void wt_porcelain_v2_print(const struct wt_status *s, const struct wt_status_state *state)
 {
 	struct wt_status_change_data *d;
 	struct string_list_item *it;
 	int i;
 
 	if (s->show_branch)
-		wt_porcelain_v2_print_tracking(s);
+		wt_porcelain_v2_print_tracking(s, state);
 
 	for (i = 0; i < s->change.nr; i++) {
 		it = &(s->change.items[i]);
@@ -2284,7 +2280,9 @@ static void wt_porcelain_v2_print(struct wt_status *s)
 	}
 }
 
-void wt_status_print(struct wt_status *s)
+// FIXME: `struct wt_status *` should be `const struct wt_status` but because
+// `wt_porcelain_print()` modifies it, that has to first be fixed
+void wt_status_print(struct wt_status *s, const struct wt_status_state *state)
 {
 	switch (s->status_format) {
 	case STATUS_FORMAT_SHORT:
@@ -2294,14 +2292,14 @@ void wt_status_print(struct wt_status *s)
 		wt_porcelain_print(s);
 		break;
 	case STATUS_FORMAT_PORCELAIN_V2:
-		wt_porcelain_v2_print(s);
+		wt_porcelain_v2_print(s, state);
 		break;
 	case STATUS_FORMAT_UNSPECIFIED:
 		BUG("finalize_deferred_config() should have been called");
 		break;
 	case STATUS_FORMAT_NONE:
 	case STATUS_FORMAT_LONG:
-		wt_longstatus_print(s);
+		wt_longstatus_print(s, state);
 		break;
 	}
 }
diff --git a/wt-status.h b/wt-status.h
index 937b2c352..341bda9dc 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -128,18 +128,19 @@ struct wt_status_state {
 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_get_state(struct wt_status_state *state, int get_detached_from);
+void wt_status_print(struct wt_status *s, const struct wt_status_state *state);
+void wt_status_collect(struct wt_status *s, const struct wt_status_state *state);
+void wt_status_get_state(const struct wt_status *s, struct wt_status_state *state);
+void wt_status_clear_state(struct wt_status_state *state);
 int wt_status_check_rebase(const struct worktree *wt,
 			   struct wt_status_state *state);
 int wt_status_check_bisect(const struct worktree *wt,
 			   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf(const struct wt_status *s, const char *color, const char *fmt, ...);
 
 /* The following functions expect that the caller took care of reading the index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.18.0

[PATCH v4 4/4] commit: fix exit code when doing a dry run

[Samuel Lijin]

In wt-status.c, the s->committable bit is set only in the call tree of
wt_longstatus_print(), and it is not always set correctly. This means
that in normal cases, if there are changes to be committed, or if there
is a merge in progress and all conflicts have been resolved, `--dry-run`
and `--long` return the correct exit code but `--short` and
`--porcelain` do not, even though all four flags imply dry run.
Moreover, if there is a merge in progress and some but not all conflicts
have been resolved, `--short` and `--porcelain` only return the correct
exit code by coincidence (because the codepaths they follow never touch
the committable bit), whereas `--dry-run` and `--long` return the wrong
exit code.

Teach wt_status_collect() to set s->committable correctly (if a merge is
in progress, committable should be set iff there are no unmerged
changes; otherwise, committable should be set iff there are changes in
the index) so that all four flags which imply dry runs return the
correct exit code in the above described situations and mark the
documenting tests as fixed.

Use the index_status field in the wt_status_change_data structs in
has_unmerged() to determine whether or not there are unmerged paths,
instead of the stagemask field, to improve readability.

Also stop setting s->committable in wt_longstatus_print_updated() and
show_merge_in_progress(), and const-ify wt_status_state in the method
signatures in those callpaths.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh | 12 +++----
 wt-status.c       | 80 +++++++++++++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 37 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index e49dfd0a2..6dba526e6 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
 '
@@ -714,7 +714,7 @@ test_expect_success '--long with only unresolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--dry-run with resolved and unresolved merge conflicts' '
+test_expect_success '--dry-run with resolved and unresolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve one merge conflict" >test-file1 &&
@@ -747,7 +747,7 @@ test_expect_success '--porcelain with resolved and unresolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--long with resolved and unresolved merge conflicts' '
+test_expect_success '--long with resolved and unresolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve one merge conflict" >test-file1 &&
@@ -769,7 +769,7 @@ test_expect_success '--dry-run with only resolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--short with only resolved merge conflicts' '
+test_expect_success '--short with only resolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
@@ -780,7 +780,7 @@ test_expect_failure '--short with only resolved merge conflicts' '
 	test_cmp expected actual
 '
 
-test_expect_failure '--porcelain with only resolved merge conflicts' '
+test_expect_success '--porcelain with only resolved merge conflicts' '
 	git reset --hard commit-2 &&
 	test_must_fail git merge --no-commit commit-1 &&
 	echo "resolve all merge conflicts" | tee test-file1 test-file2 &&
diff --git a/wt-status.c b/wt-status.c
index af83fae68..fc239f61c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -724,6 +724,38 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static int has_unmerged(const struct wt_status *s)
+{
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+		if (d->index_status == DIFF_STATUS_UNMERGED)
+			return 1;
+	}
+	return 0;
+}
+
+static void wt_status_mark_committable(
+		struct wt_status *s, const struct wt_status_state *state)
+{
+	int i;
+
+	if (state->merge_in_progress) {
+		s->committable = !has_unmerged(s);
+		return;
+	}
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+
+		if (d->index_status) {
+			s->committable = 1;
+			return;
+		}
+	}
+}
+
 void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 {
 	wt_status_collect_changes_worktree(s);
@@ -734,6 +766,8 @@ void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 		wt_status_collect_changes_index(s);
 
 	wt_status_collect_untracked(s);
+
+	wt_status_mark_committable(s, state);
 }
 
 static void wt_longstatus_print_unmerged(const struct wt_status *s)
@@ -759,28 +793,27 @@ static void wt_longstatus_print_unmerged(const struct wt_status *s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
-	int shown_header = 0;
 	int i;
 
+	if (!s->committable)
+		return;
+
+	wt_longstatus_print_cached_header(s);
+
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		struct string_list_item *it;
 		it = &(s->change.items[i]);
 		d = it->util;
-		if (!d->index_status ||
-		    d->index_status == DIFF_STATUS_UNMERGED)
-			continue;
-		if (!shown_header) {
-			wt_longstatus_print_cached_header(s);
-			s->committable = 1;
-			shown_header = 1;
+		if (d->index_status &&
+		    d->index_status != DIFF_STATUS_UNMERGED) {
+			wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 		}
-		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 	}
-	if (shown_header)
-		wt_longstatus_print_trailer(s);
+
+	wt_longstatus_print_trailer(s);
 }
 
 /*
@@ -1064,21 +1097,7 @@ static void wt_longstatus_print_tracking(const struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(const 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,
-				const struct wt_status_state *state,
+static void show_merge_in_progress(const struct wt_status *s,
 				const char *color)
 {
 	if (has_unmerged(s)) {
@@ -1090,7 +1109,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)
@@ -1584,12 +1602,12 @@ void wt_status_clear_state(struct wt_status_state *state)
 	free(state->detached_from);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
+static void wt_longstatus_print_state(const struct wt_status *s,
 				      const struct wt_status_state *state)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
 	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);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
@@ -1602,7 +1620,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
 		show_bisect_in_progress(s, state, state_color);
 }
 
-static void wt_longstatus_print(struct wt_status *s, const struct wt_status_state *state)
+static void wt_longstatus_print(const struct wt_status *s, const struct wt_status_state *state)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-- 
2.18.0

[PATCH v3 1/3] t7501: add merge conflict tests for dry run

[Samuel Lijin]

The behavior of git commit when doing a dry run changes if there are
unfixed/fixed merge conflits, but the test suite currently only asserts
that `git commit --dry-run` succeeds when all merge conflicts are fixed.

Add tests to document the behavior of all flags which imply a dry run
when (1) there is at least one unfixed merge conflict and (2) when all
merge conflicts are all fixed.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh | 45 ++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 40 insertions(+), 5 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..be087e73f 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -652,7 +652,8 @@ test_expect_success '--only works on to-be-born branch' '
 	test_cmp expected actual
 '
 
-test_expect_success '--dry-run with conflicts fixed from a merge' '
+# set up env for tests of --dry-run given fixed/unfixed merge conflicts
+test_expect_success 'setup env with unfixed merge conflicts' '
 	# setup two branches with conflicting information
 	# in the same file, resolve the conflict,
 	# call commit with --dry-run
@@ -665,11 +666,45 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git checkout -b branch-2 HEAD^1 &&
 	echo "commit-2-state" >test-file &&
 	git commit -m "commit 2" -i test-file &&
-	! $(git merge --no-commit commit-1) &&
-	echo "commit-2-state" >test-file &&
+	test_expect_code 1 git merge --no-commit commit-1
+'
+
+test_expect_success '--dry-run with unfixed merge conflicts' '
+	test_expect_code 1 git commit --dry-run
+'
+
+test_expect_success '--short with unfixed merge conflicts' '
+	test_expect_code 1 git commit --short
+'
+
+test_expect_success '--porcelain with unfixed merge conflicts' '
+	test_expect_code 1 git commit --porcelain
+'
+
+test_expect_success '--long with unfixed merge conflicts' '
+	test_expect_code 1 git commit --long
+'
+
+test_expect_success '--dry-run with conflicts fixed from a merge' '
+	echo "merge-conflicts-fixed" >test-file &&
 	git add test-file &&
-	git commit --dry-run &&
-	git commit -m "conflicts fixed from merge."
+	git commit --dry-run
+'
+
+test_expect_failure '--short with conflicts fixed from a merge' '
+	git commit --short
+'
+
+test_expect_failure '--porcelain with conflicts fixed from a merge' '
+	git commit --porcelain
+'
+
+test_expect_success '--long with conflicts fixed from a merge' '
+	git commit --long
+'
+
+test_expect_success '--message with conflicts fixed from a merge' '
+	git commit --message "conflicts fixed from merge."
 '
 
 test_done
-- 
2.18.0

Re: [PATCH v3 1/3] t7501: add merge conflict tests for dry run

[Junio C Hamano]

Samuel Lijin <sxlijin@gmail.com> writes:

> The behavior of git commit when doing a dry run changes if there are
> unfixed/fixed merge conflits, but the test suite currently only asserts
> that `git commit --dry-run` succeeds when all merge conflicts are fixed.
>
> Add tests to document the behavior of all flags which imply a dry run
> when (1) there is at least one unfixed merge conflict and (2) when all
> merge conflicts are all fixed.

s/conflits/conflicts/
s/fixed/resolved/g	(both above and in the patch text)
s/unfixed/unresolved/g  (both above and in the patch text)

> Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
> ---
>  t/t7501-commit.sh | 45 ++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index fa61b1a4e..be087e73f 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -652,7 +652,8 @@ test_expect_success '--only works on to-be-born branch' '
>  	test_cmp expected actual
>  '
>  
> -test_expect_success '--dry-run with conflicts fixed from a merge' '
> +# set up env for tests of --dry-run given fixed/unfixed merge conflicts
> +test_expect_success 'setup env with unfixed merge conflicts' '
>  	# setup two branches with conflicting information
>  	# in the same file, resolve the conflict,
>  	# call commit with --dry-run
> @@ -665,11 +666,45 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
>  	git checkout -b branch-2 HEAD^1 &&
>  	echo "commit-2-state" >test-file &&
>  	git commit -m "commit 2" -i test-file &&
> -	! $(git merge --no-commit commit-1) &&
> -	echo "commit-2-state" >test-file &&
> +	test_expect_code 1 git merge --no-commit commit-1

The original is bad and also embarrassing.  Whatever comes out of
the standard output of "git merge" is $IFS split and executed as a
shell command (which likely results in "no such command" failure)
and it tries to make sure that a failure happens.

The right way to write that line (without your enhancement in this
patch) would have been:

	test_must_fail git merge --no-commit commit-1 &&

I doubt it is a good idea to hardcode exit status of 1 by using
test_expect_code, though.  "git merge --help" does not say anything
about "1 means this failure, 2 means that failure, 3 means that
other failure".  And my quick forward scan of this series does not
tell me that you are trying to declare that from here on we _will_
make that promise to the end users by carving the exit status(es) in
stone.  The same about "git commit"'s exit code in the following
four tests.

> +'
> +
> +test_expect_success '--dry-run with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --dry-run
> +'
> +
> +test_expect_success '--short with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --short
> +'
> +
> +test_expect_success '--porcelain with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --porcelain
> +'
> +
> +test_expect_success '--long with unfixed merge conflicts' '
> +	test_expect_code 1 git commit --long
> +'
> +
> +test_expect_success '--dry-run with conflicts fixed from a merge' '
> +	echo "merge-conflicts-fixed" >test-file &&

The original test pretended that we resolved favouring the current
state with "commit-2-state" in the file, as if we ran "-s ours".
Is there a reason why we now use a different contents, or is this
just a change based on subjective preference?  

    Not saying that the latter is necessrily bad; just trying to
    understand why we are making this change.

>  	git add test-file &&
> -	git commit --dry-run &&
> -	git commit -m "conflicts fixed from merge."
> +	git commit --dry-run

OK, the original tried --dry-run to ensure it exited with 0 status
(i.e. have something to commit) and then did a commit to record the
updated state with a message.  You are checking only the dry-run
part, leaving the check of the final commit's status to another
test.

> +'
> +
> +test_expect_failure '--short with conflicts fixed from a merge' '
> +	git commit --short
> +'

With "test_expect_failure", you are saying that "--short" _should_
exit with 0 but currently it does not.  An untold expectation is
that even with the breakage with the exit code, the command still
honors the (implicit) --dry-run correctly and does not create a
new commit.

That was actually tested in the original.  By &&-chaining like this

	git commit --dry-run &&
	git commit -m "conflicts fixed from merge."

we would have noticed if a newly introduced bug caused the first
step "commit --dry-run" to return non-zero status (because then the
step would fail), or if it stopped being dry-run and made a commit
(because then the next step would fail with "nothing to commit").

But by splitting these into separate tests, the patch makes such a
potential failure with "git commit --short" break the later steps.

Not very nice.

It may be a better change to just do in the original one

	git add test-file &&
	git commit --dry-run &&
+	git commit --short &&
+	git commit --long &&
+	git commit --porcelain &&
	git commit -m "conflicts fixed from merge."

without adding these new and separate tests, and then mark that one
to expect a failure (because it would pass up to the --dry-run
commit, but the --short commit would fail) at this step, perhaps?

> +test_expect_failure '--porcelain with conflicts fixed from a merge' '
> +	git commit --porcelain
> +'
> +
> +test_expect_success '--long with conflicts fixed from a merge' '
> +	git commit --long
> +'
> +
> +test_expect_success '--message with conflicts fixed from a merge' '
> +	git commit --message "conflicts fixed from merge."
>  '
>  
>  test_done

Re: [PATCH v3 1/3] t7501: add merge conflict tests for dry run

[Junio C Hamano]

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

> But by splitting these into separate tests, the patch makes such a
> potential failure with "git commit --short" break the later steps.
>
> Not very nice.
>
> It may be a better change to just do in the original one
>
> 	git add test-file &&
> 	git commit --dry-run &&
> +	git commit --short &&
> +	git commit --long &&
> +	git commit --porcelain &&
> 	git commit -m "conflicts fixed from merge."
>
> without adding these new and separate tests, and then mark that one
> to expect a failure (because it would pass up to the --dry-run
> commit, but the --short commit would fail) at this step, perhaps?

Of course, if you want to be more thorough, anticipating that other
people in their future updates may break --short but not --long or
--porcelain, testing each option in separate test_expect_success is
a necessary way to do so, but then you'd need to actually be more
thorough, by not merely running each of them in separate
test_expect_success block but also arranging that each of them start
in an expected state to try the thing we want it to try.  That is

	for opt in --dry-run --short --long --porcelain
	do
		test_expect_success "commit $opt" '
			set up the conflicted state after merge &&
			git commit $opt
		'
	done

where the "set up the state" part makes sure it can tolerate
potential mistakes of previous run of "git commit $opt" (e.g. it
by mistake made a commit, making the index identical to HEAD and
taking us out of "merge in progress" state).

But from your 1/3 I did not get the impression that you particularly
want to be more thorough, and from your 3/3 I did not get the
impression that you anticipate --short/--long/--porcelain may get
broken independently.  And if that is the case, then chaining all of
them together like the above is a more honest way to express that we
are only doing a minimum set of testing.

Thanks.

[PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress

[Samuel Lijin]

To fix the breakages documented by t7501, the next patch in this series
will teach wt_status_collect() to set the committable bit, instead of
having wt_longstatus_print_updated() and show_merge_in_progress() set it
(which is what currently happens). Unfortunately, wt_status_collect()
needs to know whether or not there is a merge in progress to set the bit
correctly, so teach its (two) callers to create, initialize, and pass
in instances of wt_status_state, which records this metadata.

Since wt_longstatus_print() and show_merge_in_progress() are in the same
callpaths and currently create and init copies of wt_status_state,
remove that logic and instead pass wt_status_state through.

Make wt_status_get_state easier to use, add a helper method to clean up
wt_status_state, const-ify as many struct pointers in method signatures
as possible, and add a FIXME for a struct pointer which should be const
but isn't (that this patch series will not address).

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 builtin/commit.c |  32 ++++----
 ref-filter.c     |   3 +-
 wt-status.c      | 188 +++++++++++++++++++++++------------------------
 wt-status.h      |  13 ++--
 4 files changed, 120 insertions(+), 116 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 37fcb55ab..79ef4f11a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -463,6 +463,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
 static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,
 		      struct wt_status *s)
 {
+	struct wt_status_state state;
 	struct object_id oid;
 
 	if (s->relative_paths)
@@ -482,10 +483,12 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
 	s->status_format = status_format;
 	s->ignore_submodule_arg = ignore_submodule_arg;
 
-	wt_status_collect(s);
-	wt_status_print(s);
+	wt_status_get_state(s, &state);
+	wt_status_collect(s, &state);
+	wt_status_print(s, &state);
+	wt_status_clear_state(&state);
 
-	return s->commitable;
+	return s->committable;
 }
 
 static int is_a_merge(const struct commit *current_head)
@@ -631,7 +634,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;
@@ -848,7 +851,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;
@@ -866,7 +869,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
@@ -882,7 +885,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);
@@ -894,7 +897,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);
@@ -1164,14 +1167,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;
 }
 
 static int parse_status_slot(const char *slot)
@@ -1266,6 +1269,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
 int cmd_status(int argc, const char **argv, const char *prefix)
 {
 	static struct wt_status s;
+	struct wt_status_state state;
 	int fd;
 	struct object_id oid;
 	static struct option builtin_status_options[] = {
@@ -1338,7 +1342,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	s.status_format = status_format;
 	s.verbose = verbose;
 
-	wt_status_collect(&s);
+	wt_status_get_state(&s, &state);
+	wt_status_collect(&s, &state);
 
 	if (0 <= fd)
 		update_index_if_able(&the_index, &index_lock);
@@ -1346,7 +1351,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 	if (s.relative_paths)
 		s.prefix = prefix;
 
-	wt_status_print(&s);
+	wt_status_print(&s, &state);
+	wt_status_clear_state(&state);
 	return 0;
 }
 
diff --git a/ref-filter.c b/ref-filter.c
index 9a333e21b..280ef9713 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1306,8 +1306,7 @@ char *get_head_description(void)
 {
 	struct strbuf desc = STRBUF_INIT;
 	struct wt_status_state state;
-	memset(&state, 0, sizeof(state));
-	wt_status_get_state(&state, 1);
+	wt_status_get_state(NULL, &state);
 	if (state.rebase_in_progress ||
 	    state.rebase_interactive_in_progress)
 		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
diff --git a/wt-status.c b/wt-status.c
index 50815e5fa..75d389944 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
 	const char *c = "";
 	if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char *color,
 		const char *fmt, va_list ap, const char *trail)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 	strbuf_release(&sb);
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
 			       const char *fmt, ...)
 {
 	va_list ap;
@@ -140,7 +140,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->display_comment_prefix = 0;
 }
 
-static void wt_longstatus_print_unmerged_header(struct wt_status *s)
+static void wt_longstatus_print_unmerged_header(const struct wt_status *s)
 {
 	int i;
 	int del_mod_conflict = 0;
@@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
@@ -208,7 +208,7 @@ static void wt_longstatus_print_cached_header(struct wt_status *s)
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_dirty_header(struct wt_status *s,
+static void wt_longstatus_print_dirty_header(const struct wt_status *s,
 					     int has_deleted,
 					     int has_dirty_submodules)
 {
@@ -227,7 +227,7 @@ static void wt_longstatus_print_dirty_header(struct wt_status *s,
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_other_header(struct wt_status *s,
+static void wt_longstatus_print_other_header(const struct wt_status *s,
 					     const char *what,
 					     const char *how)
 {
@@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct wt_status *s,
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
 	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -305,7 +305,7 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval)
 	return result;
 }
 
-static void wt_longstatus_print_unmerged_data(struct wt_status *s,
+static void wt_longstatus_print_unmerged_data(const struct wt_status *s,
 					      struct string_list_item *it)
 {
 	const char *c = color(WT_STATUS_UNMERGED, s);
@@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
 	strbuf_release(&onebuf);
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
 					    int change_type,
 					    struct string_list_item *it)
 {
@@ -718,7 +718,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
-void wt_status_collect(struct wt_status *s)
+void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 {
 	wt_status_collect_changes_worktree(s);
 
@@ -726,10 +726,11 @@ void wt_status_collect(struct wt_status *s)
 		wt_status_collect_changes_initial(s);
 	else
 		wt_status_collect_changes_index(s);
+
 	wt_status_collect_untracked(s);
 }
 
-static void wt_longstatus_print_unmerged(struct wt_status *s)
+static void wt_longstatus_print_unmerged(const struct wt_status *s)
 {
 	int shown_header = 0;
 	int i;
@@ -767,7 +768,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);
@@ -781,7 +782,7 @@ static void wt_longstatus_print_updated(struct wt_status *s)
  *  0 : no change
  *  1 : some change but no delete
  */
-static int wt_status_check_worktree_changes(struct wt_status *s,
+static int wt_status_check_worktree_changes(const struct wt_status *s,
 					     int *dirty_submodules)
 {
 	int i;
@@ -805,7 +806,7 @@ static int wt_status_check_worktree_changes(struct wt_status *s,
 	return changes;
 }
 
-static void wt_longstatus_print_changed(struct wt_status *s)
+static void wt_longstatus_print_changed(const struct wt_status *s)
 {
 	int i, dirty_submodules;
 	int worktree_changes = wt_status_check_worktree_changes(s, &dirty_submodules);
@@ -837,7 +838,7 @@ static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
 	return 0;
 }
 
-static void wt_longstatus_print_stash_summary(struct wt_status *s)
+static void wt_longstatus_print_stash_summary(const struct wt_status *s)
 {
 	int stash_count = 0;
 
@@ -849,7 +850,7 @@ static void wt_longstatus_print_stash_summary(struct wt_status *s)
 				 stash_count);
 }
 
-static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncommitted)
+static void wt_longstatus_print_submodule_summary(const struct wt_status *s, int uncommitted)
 {
 	struct child_process sm_summary = CHILD_PROCESS_INIT;
 	struct strbuf cmd_stdout = STRBUF_INIT;
@@ -895,8 +896,8 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
 	strbuf_release(&summary);
 }
 
-static void wt_longstatus_print_other(struct wt_status *s,
-				      struct string_list *l,
+static void wt_longstatus_print_other(const struct wt_status *s,
+				      const struct string_list *l,
 				      const char *what,
 				      const char *how)
 {
@@ -969,7 +970,7 @@ void wt_status_add_cut_line(FILE *fp)
 	strbuf_release(&buf);
 }
 
-static void wt_longstatus_print_verbose(struct wt_status *s)
+static void wt_longstatus_print_verbose(const struct wt_status *s)
 {
 	struct rev_info rev;
 	struct setup_revision_opt opt;
@@ -1000,7 +1001,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);
@@ -1021,7 +1022,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
 	}
 }
 
-static void wt_longstatus_print_tracking(struct wt_status *s)
+static void wt_longstatus_print_tracking(const struct wt_status *s)
 {
 	struct strbuf sb = STRBUF_INIT;
 	const char *cp, *ep, *branch_name;
@@ -1055,7 +1056,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(struct wt_status *s)
+static int has_unmerged(const struct wt_status *s)
 {
 	int i;
 
@@ -1069,7 +1070,7 @@ static int has_unmerged(struct wt_status *s)
 }
 
 static void show_merge_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	if (has_unmerged(s)) {
@@ -1081,7 +1082,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)
@@ -1091,8 +1092,8 @@ static void show_merge_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_am_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+static void show_am_in_progress(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	status_printf_ln(s, color,
@@ -1130,7 +1131,7 @@ static char *read_line_from_git_path(const char *filename)
 	}
 }
 
-static int split_commit_in_progress(struct wt_status *s)
+static int split_commit_in_progress(const struct wt_status *s)
 {
 	int split_in_progress = 0;
 	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
@@ -1224,8 +1225,8 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
 	return 0;
 }
 
-static void show_rebase_information(struct wt_status *s,
-					struct wt_status_state *state,
+static void show_rebase_information(const struct wt_status *s,
+					const struct wt_status_state *state,
 					const char *color)
 {
 	if (state->rebase_interactive_in_progress) {
@@ -1278,8 +1279,8 @@ static void show_rebase_information(struct wt_status *s,
 	}
 }
 
-static void print_rebase_state(struct wt_status *s,
-				struct wt_status_state *state,
+static void print_rebase_state(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	if (state->branch)
@@ -1292,8 +1293,8 @@ static void print_rebase_state(struct wt_status *s,
 				 _("You are currently rebasing."));
 }
 
-static void show_rebase_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+static void show_rebase_in_progress(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	struct stat st;
@@ -1345,8 +1346,8 @@ static void show_rebase_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_cherry_pick_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
+static void show_cherry_pick_in_progress(const struct wt_status *s,
+					const struct wt_status_state *state,
 					const char *color)
 {
 	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
@@ -1364,8 +1365,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_revert_in_progress(struct wt_status *s,
-					struct wt_status_state *state,
+static void show_revert_in_progress(const struct wt_status *s,
+					const struct wt_status_state *state,
 					const char *color)
 {
 	status_printf_ln(s, color, _("You are currently reverting commit %s."),
@@ -1383,8 +1384,8 @@ static void show_revert_in_progress(struct wt_status *s,
 	wt_longstatus_print_trailer(s);
 }
 
-static void show_bisect_in_progress(struct wt_status *s,
-				struct wt_status_state *state,
+static void show_bisect_in_progress(const struct wt_status *s,
+				const struct wt_status_state *state,
 				const char *color)
 {
 	if (state->branch)
@@ -1538,12 +1539,16 @@ int wt_status_check_bisect(const struct worktree *wt,
 	return 0;
 }
 
-void wt_status_get_state(struct wt_status_state *state,
-			 int get_detached_from)
+void wt_status_get_state(
+		const struct wt_status *s, struct wt_status_state *state)
 {
+	int get_detached_from =
+		(s == NULL) || (s->branch && !strcmp(s->branch, "HEAD"));
 	struct stat st;
 	struct object_id oid;
 
+	memset(state, 0, sizeof(*state));
+
 	if (!stat(git_path_merge_head(), &st)) {
 		state->merge_in_progress = 1;
 	} else if (wt_status_check_rebase(NULL, state)) {
@@ -1564,8 +1569,15 @@ void wt_status_get_state(struct wt_status_state *state,
 		wt_status_get_detached_from(state);
 }
 
+void wt_status_clear_state(struct wt_status_state *state)
+{
+	free(state->branch);
+	free(state->onto);
+	free(state->detached_from);
+}
+
 static void wt_longstatus_print_state(struct wt_status *s,
-				      struct wt_status_state *state)
+				      const struct wt_status_state *state)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
 	if (state->merge_in_progress)
@@ -1582,30 +1594,25 @@ static void wt_longstatus_print_state(struct wt_status *s,
 		show_bisect_in_progress(s, state, state_color);
 }
 
-static void wt_longstatus_print(struct wt_status *s)
+static void wt_longstatus_print(struct wt_status *s, const struct wt_status_state *state)
 {
 	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 (state->rebase_in_progress || state->rebase_interactive_in_progress) {
+				if (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 = state->onto;
+			} else if (state->detached_from) {
+				branch_name = state->detached_from;
+				if (state->detached_at)
 					on_what = _("HEAD detached at ");
 				else
 					on_what = _("HEAD detached from ");
@@ -1622,10 +1629,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, state);
 
 	if (s->is_initial) {
 		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
@@ -1657,14 +1661,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)
@@ -1700,7 +1704,7 @@ static void wt_longstatus_print(struct wt_status *s)
 }
 
 static void wt_shortstatus_unmerged(struct string_list_item *it,
-			   struct wt_status *s)
+			   const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 	const char *how = "??";
@@ -1727,7 +1731,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
 }
 
 static void wt_shortstatus_status(struct string_list_item *it,
-			 struct wt_status *s)
+			 const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 
@@ -1770,7 +1774,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
 }
 
 static void wt_shortstatus_other(struct string_list_item *it,
-				 struct wt_status *s, const char *sign)
+				 const struct wt_status *s, const char *sign)
 {
 	if (s->null_termination) {
 		fprintf(stdout, "%s %s%c", sign, it->string, 0);
@@ -1784,7 +1788,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 	}
 }
 
-static void wt_shortstatus_print_tracking(struct wt_status *s)
+static void wt_shortstatus_print_tracking(const struct wt_status *s)
 {
 	struct branch *branch;
 	const char *header_color = color(WT_STATUS_HEADER, s);
@@ -1860,7 +1864,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
 
-static void wt_shortstatus_print(struct wt_status *s)
+static void wt_shortstatus_print(const struct wt_status *s)
 {
 	struct string_list_item *it;
 
@@ -1924,18 +1928,14 @@ static void wt_porcelain_print(struct wt_status *s)
  * upstream.  When AHEAD_BEHIND_QUICK is requested and the branches
  * are different, '?' will be substituted for the actual count.
  */
-static void wt_porcelain_v2_print_tracking(struct wt_status *s)
+static void wt_porcelain_v2_print_tracking(const struct wt_status *s, const struct wt_status_state *state)
 {
 	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);
@@ -1946,10 +1946,10 @@ 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 (state->rebase_in_progress || state->rebase_interactive_in_progress)
+				branch_name = state->onto;
+			else if (state->detached_from)
+				branch_name = state->detached_from;
 			else
 				branch_name = "";
 		} else {
@@ -1983,10 +1983,6 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 			}
 		}
 	}
-
-	free(state.branch);
-	free(state.onto);
-	free(state.detached_from);
 }
 
 /*
@@ -1994,7 +1990,7 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
  * fixed-length string of characters in the buffer provided.
  */
 static void wt_porcelain_v2_submodule_state(
-	struct wt_status_change_data *d,
+	const struct wt_status_change_data *d,
 	char sub[5])
 {
 	if (S_ISGITLINK(d->mode_head) ||
@@ -2017,8 +2013,8 @@ static void wt_porcelain_v2_submodule_state(
  * Fix-up changed entries before we print them.
  */
 static void wt_porcelain_v2_fix_up_changed(
-	struct string_list_item *it,
-	struct wt_status *s)
+	const struct string_list_item *it,
+	const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 
@@ -2066,8 +2062,8 @@ static void wt_porcelain_v2_fix_up_changed(
  * Print porcelain v2 info for tracked entries with changes.
  */
 static void wt_porcelain_v2_print_changed_entry(
-	struct string_list_item *it,
-	struct wt_status *s)
+	const struct string_list_item *it,
+	const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 	struct strbuf buf = STRBUF_INIT;
@@ -2130,8 +2126,8 @@ static void wt_porcelain_v2_print_changed_entry(
  * Print porcelain v2 status info for unmerged entries.
  */
 static void wt_porcelain_v2_print_unmerged_entry(
-	struct string_list_item *it,
-	struct wt_status *s)
+	const struct string_list_item *it,
+	const struct wt_status *s)
 {
 	struct wt_status_change_data *d = it->util;
 	const struct cache_entry *ce;
@@ -2211,8 +2207,8 @@ static void wt_porcelain_v2_print_unmerged_entry(
  * Print porcelain V2 status info for untracked and ignored entries.
  */
 static void wt_porcelain_v2_print_other(
-	struct string_list_item *it,
-	struct wt_status *s,
+	const struct string_list_item *it,
+	const struct wt_status *s,
 	char prefix)
 {
 	struct strbuf buf = STRBUF_INIT;
@@ -2242,14 +2238,14 @@ static void wt_porcelain_v2_print_other(
  * [<v2_ignored_items>]*
  *
  */
-static void wt_porcelain_v2_print(struct wt_status *s)
+static void wt_porcelain_v2_print(const struct wt_status *s, const struct wt_status_state *state)
 {
 	struct wt_status_change_data *d;
 	struct string_list_item *it;
 	int i;
 
 	if (s->show_branch)
-		wt_porcelain_v2_print_tracking(s);
+		wt_porcelain_v2_print_tracking(s, state);
 
 	for (i = 0; i < s->change.nr; i++) {
 		it = &(s->change.items[i]);
@@ -2276,7 +2272,9 @@ static void wt_porcelain_v2_print(struct wt_status *s)
 	}
 }
 
-void wt_status_print(struct wt_status *s)
+// FIXME: `struct wt_status *` should be `const struct wt_status` but because
+// `wt_porcelain_print()` modifies it, that has to first be fixed
+void wt_status_print(struct wt_status *s, const struct wt_status_state *state)
 {
 	switch (s->status_format) {
 	case STATUS_FORMAT_SHORT:
@@ -2286,14 +2284,14 @@ void wt_status_print(struct wt_status *s)
 		wt_porcelain_print(s);
 		break;
 	case STATUS_FORMAT_PORCELAIN_V2:
-		wt_porcelain_v2_print(s);
+		wt_porcelain_v2_print(s, state);
 		break;
 	case STATUS_FORMAT_UNSPECIFIED:
 		die("BUG: finalize_deferred_config() should have been called");
 		break;
 	case STATUS_FORMAT_NONE:
 	case STATUS_FORMAT_LONG:
-		wt_longstatus_print(s);
+		wt_longstatus_print(s, state);
 		break;
 	}
 }
diff --git a/wt-status.h b/wt-status.h
index 430770b85..6cccfd7a0 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -94,7 +94,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;
@@ -126,18 +126,19 @@ struct wt_status_state {
 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_get_state(struct wt_status_state *state, int get_detached_from);
+void wt_status_print(struct wt_status *s, const struct wt_status_state *state);
+void wt_status_collect(struct wt_status *s, const struct wt_status_state *state);
+void wt_status_get_state(const struct wt_status *s, struct wt_status_state *state);
+void wt_status_clear_state(struct wt_status_state *state);
 int wt_status_check_rebase(const struct worktree *wt,
 			   struct wt_status_state *state);
 int wt_status_check_bisect(const struct worktree *wt,
 			   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf(const struct wt_status *s, const char *color, const char *fmt, ...);
 
 /* The following functions expect that the caller took care of reading the index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.18.0

Re: [PATCH v3 2/3] wt-status: teach wt_status_collect about merges in progress

[Junio C Hamano]

Samuel Lijin <sxlijin@gmail.com> writes:

> To fix the breakages documented by t7501, the next patch in this series
> will teach wt_status_collect() to set the committable bit, instead of
> having wt_longstatus_print_updated() and show_merge_in_progress() set it
> (which is what currently happens). Unfortunately, wt_status_collect()
> needs to know whether or not there is a merge in progress to set the bit
> correctly,

s/correctly,/correctly (a brief desription of why),/

would be nicer.  The description might be

	(after a merge, it is OK for the result to be identical to HEAD,
	which usually causes a "nothing to commit" error)

or something like that.

> so teach its (two) callers to create, initialize, and pass
> in instances of wt_status_state, which records this metadata.
>
> Since wt_longstatus_print() and show_merge_in_progress() are in the same
> callpaths and currently create and init copies of wt_status_state,
> remove that logic and instead pass wt_status_state through.

OK.  Sounds like a good clean-up.

> Make wt_status_get_state easier to use, add a helper method to clean up

Your description so far marked function names with trailing ();
let's do so consistently for wt_status_get_state(), too.

> wt_status_state, const-ify as many struct pointers in method signatures
> as possible, and add a FIXME for a struct pointer which should be const
> but isn't (that this patch series will not address).

"should be but isn't" because...?  I am wondering if it is better to
leave _all_ constifying to a later effort, if we are leaving some of
them behind anyway.  It would be better only if it will make this
patch easier to read if we did so.

Also you did s/commitable/committable/ everywhere, which was
somewhat distracting.  It would have been nicer to follow if that
were a separate preparatory clean-up patch.

>
> Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
> ---
>  builtin/commit.c |  32 ++++----
>  ref-filter.c     |   3 +-
>  wt-status.c      | 188 +++++++++++++++++++++++------------------------
>  wt-status.h      |  13 ++--
>  4 files changed, 120 insertions(+), 116 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 37fcb55ab..79ef4f11a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -463,6 +463,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix
>  static int run_status(FILE *fp, const char *index_file, const char *prefix, int nowarn,
>  		      struct wt_status *s)
>  {
> +	struct wt_status_state state;
>  	struct object_id oid;
>  
>  	if (s->relative_paths)
> @@ -482,10 +483,12 @@ static int run_status(FILE *fp, const char *index_file, const char *prefix, int
>  	s->status_format = status_format;
>  	s->ignore_submodule_arg = ignore_submodule_arg;
>  
> -	wt_status_collect(s);
> -	wt_status_print(s);
> +	wt_status_get_state(s, &state);
> +	wt_status_collect(s, &state);
> +	wt_status_print(s, &state);
> +	wt_status_clear_state(&state);
>  
> -	return s->commitable;
> +	return s->committable;
>  }
>  
>  static int is_a_merge(const struct commit *current_head)
> @@ -631,7 +634,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;
> @@ -848,7 +851,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;
> @@ -866,7 +869,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
> @@ -882,7 +885,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);
> @@ -894,7 +897,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);
> @@ -1164,14 +1167,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;
>  }
>  
>  static int parse_status_slot(const char *slot)
> @@ -1266,6 +1269,7 @@ static int git_status_config(const char *k, const char *v, void *cb)
>  int cmd_status(int argc, const char **argv, const char *prefix)
>  {
>  	static struct wt_status s;
> +	struct wt_status_state state;
>  	int fd;
>  	struct object_id oid;
>  	static struct option builtin_status_options[] = {
> @@ -1338,7 +1342,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	s.status_format = status_format;
>  	s.verbose = verbose;
>  
> -	wt_status_collect(&s);
> +	wt_status_get_state(&s, &state);
> +	wt_status_collect(&s, &state);
>  
>  	if (0 <= fd)
>  		update_index_if_able(&the_index, &index_lock);
> @@ -1346,7 +1351,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  	if (s.relative_paths)
>  		s.prefix = prefix;
>  
> -	wt_status_print(&s);
> +	wt_status_print(&s, &state);
> +	wt_status_clear_state(&state);
>  	return 0;
>  }
>  
> diff --git a/ref-filter.c b/ref-filter.c
> index 9a333e21b..280ef9713 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -1306,8 +1306,7 @@ char *get_head_description(void)
>  {
>  	struct strbuf desc = STRBUF_INIT;
>  	struct wt_status_state state;
> -	memset(&state, 0, sizeof(state));
> -	wt_status_get_state(&state, 1);
> +	wt_status_get_state(NULL, &state);
>  	if (state.rebase_in_progress ||
>  	    state.rebase_interactive_in_progress)
>  		strbuf_addf(&desc, _("(no branch, rebasing %s)"),
> diff --git a/wt-status.c b/wt-status.c
> index 50815e5fa..75d389944 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
>  	GIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */
>  };
>  
> -static const char *color(int slot, struct wt_status *s)
> +static const char *color(int slot, const struct wt_status *s)
>  {
>  	const char *c = "";
>  	if (want_color(s->use_color))
> @@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
>  	return c;
>  }
>  
> -static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
> +static void status_vprintf(const struct wt_status *s, int at_bol, const char *color,
>  		const char *fmt, va_list ap, const char *trail)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> @@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
>  	strbuf_release(&sb);
>  }
>  
> -void status_printf_ln(struct wt_status *s, const char *color,
> +void status_printf_ln(const struct wt_status *s, const char *color,
>  			const char *fmt, ...)
>  {
>  	va_list ap;
> @@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
>  	va_end(ap);
>  }
>  
> -void status_printf(struct wt_status *s, const char *color,
> +void status_printf(const struct wt_status *s, const char *color,
>  			const char *fmt, ...)
>  {
>  	va_list ap;
> @@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
>  	va_end(ap);
>  }
>  
> -static void status_printf_more(struct wt_status *s, const char *color,
> +static void status_printf_more(const struct wt_status *s, const char *color,
>  			       const char *fmt, ...)
>  {
>  	va_list ap;
> @@ -140,7 +140,7 @@ void wt_status_prepare(struct wt_status *s)
>  	s->display_comment_prefix = 0;
>  }
>  
> -static void wt_longstatus_print_unmerged_header(struct wt_status *s)
> +static void wt_longstatus_print_unmerged_header(const struct wt_status *s)
>  {
>  	int i;
>  	int del_mod_conflict = 0;
> @@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct wt_status *s)
>  	status_printf_ln(s, c, "%s", "");
>  }
>  
> -static void wt_longstatus_print_cached_header(struct wt_status *s)
> +static void wt_longstatus_print_cached_header(const struct wt_status *s)
>  {
>  	const char *c = color(WT_STATUS_HEADER, s);
>  
> @@ -208,7 +208,7 @@ static void wt_longstatus_print_cached_header(struct wt_status *s)
>  	status_printf_ln(s, c, "%s", "");
>  }
>  
> -static void wt_longstatus_print_dirty_header(struct wt_status *s,
> +static void wt_longstatus_print_dirty_header(const struct wt_status *s,
>  					     int has_deleted,
>  					     int has_dirty_submodules)
>  {
> @@ -227,7 +227,7 @@ static void wt_longstatus_print_dirty_header(struct wt_status *s,
>  	status_printf_ln(s, c, "%s", "");
>  }
>  
> -static void wt_longstatus_print_other_header(struct wt_status *s,
> +static void wt_longstatus_print_other_header(const struct wt_status *s,
>  					     const char *what,
>  					     const char *how)
>  {
> @@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct wt_status *s,
>  	status_printf_ln(s, c, "%s", "");
>  }
>  
> -static void wt_longstatus_print_trailer(struct wt_status *s)
> +static void wt_longstatus_print_trailer(const struct wt_status *s)
>  {
>  	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
>  }
> @@ -305,7 +305,7 @@ static int maxwidth(const char *(*label)(int), int minval, int maxval)
>  	return result;
>  }
>  
> -static void wt_longstatus_print_unmerged_data(struct wt_status *s,
> +static void wt_longstatus_print_unmerged_data(const struct wt_status *s,
>  					      struct string_list_item *it)
>  {
>  	const char *c = color(WT_STATUS_UNMERGED, s);
> @@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
>  	strbuf_release(&onebuf);
>  }
>  
> -static void wt_longstatus_print_change_data(struct wt_status *s,
> +static void wt_longstatus_print_change_data(const struct wt_status *s,
>  					    int change_type,
>  					    struct string_list_item *it)
>  {
> @@ -718,7 +718,7 @@ static void wt_status_collect_untracked(struct wt_status *s)
>  		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
>  }
>  
> -void wt_status_collect(struct wt_status *s)
> +void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
>  {
>  	wt_status_collect_changes_worktree(s);
>  
> @@ -726,10 +726,11 @@ void wt_status_collect(struct wt_status *s)
>  		wt_status_collect_changes_initial(s);
>  	else
>  		wt_status_collect_changes_index(s);
> +
>  	wt_status_collect_untracked(s);
>  }
>  
> -static void wt_longstatus_print_unmerged(struct wt_status *s)
> +static void wt_longstatus_print_unmerged(const struct wt_status *s)
>  {
>  	int shown_header = 0;
>  	int i;
> @@ -767,7 +768,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);
> @@ -781,7 +782,7 @@ static void wt_longstatus_print_updated(struct wt_status *s)
>   *  0 : no change
>   *  1 : some change but no delete
>   */
> -static int wt_status_check_worktree_changes(struct wt_status *s,
> +static int wt_status_check_worktree_changes(const struct wt_status *s,
>  					     int *dirty_submodules)
>  {
>  	int i;
> @@ -805,7 +806,7 @@ static int wt_status_check_worktree_changes(struct wt_status *s,
>  	return changes;
>  }
>  
> -static void wt_longstatus_print_changed(struct wt_status *s)
> +static void wt_longstatus_print_changed(const struct wt_status *s)
>  {
>  	int i, dirty_submodules;
>  	int worktree_changes = wt_status_check_worktree_changes(s, &dirty_submodules);
> @@ -837,7 +838,7 @@ static int stash_count_refs(struct object_id *ooid, struct object_id *noid,
>  	return 0;
>  }
>  
> -static void wt_longstatus_print_stash_summary(struct wt_status *s)
> +static void wt_longstatus_print_stash_summary(const struct wt_status *s)
>  {
>  	int stash_count = 0;
>  
> @@ -849,7 +850,7 @@ static void wt_longstatus_print_stash_summary(struct wt_status *s)
>  				 stash_count);
>  }
>  
> -static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncommitted)
> +static void wt_longstatus_print_submodule_summary(const struct wt_status *s, int uncommitted)
>  {
>  	struct child_process sm_summary = CHILD_PROCESS_INIT;
>  	struct strbuf cmd_stdout = STRBUF_INIT;
> @@ -895,8 +896,8 @@ static void wt_longstatus_print_submodule_summary(struct wt_status *s, int uncom
>  	strbuf_release(&summary);
>  }
>  
> -static void wt_longstatus_print_other(struct wt_status *s,
> -				      struct string_list *l,
> +static void wt_longstatus_print_other(const struct wt_status *s,
> +				      const struct string_list *l,
>  				      const char *what,
>  				      const char *how)
>  {
> @@ -969,7 +970,7 @@ void wt_status_add_cut_line(FILE *fp)
>  	strbuf_release(&buf);
>  }
>  
> -static void wt_longstatus_print_verbose(struct wt_status *s)
> +static void wt_longstatus_print_verbose(const struct wt_status *s)
>  {
>  	struct rev_info rev;
>  	struct setup_revision_opt opt;
> @@ -1000,7 +1001,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);
> @@ -1021,7 +1022,7 @@ static void wt_longstatus_print_verbose(struct wt_status *s)
>  	}
>  }
>  
> -static void wt_longstatus_print_tracking(struct wt_status *s)
> +static void wt_longstatus_print_tracking(const struct wt_status *s)
>  {
>  	struct strbuf sb = STRBUF_INIT;
>  	const char *cp, *ep, *branch_name;
> @@ -1055,7 +1056,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
>  	strbuf_release(&sb);
>  }
>  
> -static int has_unmerged(struct wt_status *s)
> +static int has_unmerged(const struct wt_status *s)
>  {
>  	int i;
>  
> @@ -1069,7 +1070,7 @@ static int has_unmerged(struct wt_status *s)
>  }
>  
>  static void show_merge_in_progress(struct wt_status *s,
> -				struct wt_status_state *state,
> +				const struct wt_status_state *state,
>  				const char *color)
>  {
>  	if (has_unmerged(s)) {
> @@ -1081,7 +1082,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)
> @@ -1091,8 +1092,8 @@ static void show_merge_in_progress(struct wt_status *s,
>  	wt_longstatus_print_trailer(s);
>  }
>  
> -static void show_am_in_progress(struct wt_status *s,
> -				struct wt_status_state *state,
> +static void show_am_in_progress(const struct wt_status *s,
> +				const struct wt_status_state *state,
>  				const char *color)
>  {
>  	status_printf_ln(s, color,
> @@ -1130,7 +1131,7 @@ static char *read_line_from_git_path(const char *filename)
>  	}
>  }
>  
> -static int split_commit_in_progress(struct wt_status *s)
> +static int split_commit_in_progress(const struct wt_status *s)
>  {
>  	int split_in_progress = 0;
>  	char *head, *orig_head, *rebase_amend, *rebase_orig_head;
> @@ -1224,8 +1225,8 @@ static int read_rebase_todolist(const char *fname, struct string_list *lines)
>  	return 0;
>  }
>  
> -static void show_rebase_information(struct wt_status *s,
> -					struct wt_status_state *state,
> +static void show_rebase_information(const struct wt_status *s,
> +					const struct wt_status_state *state,
>  					const char *color)
>  {
>  	if (state->rebase_interactive_in_progress) {
> @@ -1278,8 +1279,8 @@ static void show_rebase_information(struct wt_status *s,
>  	}
>  }
>  
> -static void print_rebase_state(struct wt_status *s,
> -				struct wt_status_state *state,
> +static void print_rebase_state(const struct wt_status *s,
> +				const struct wt_status_state *state,
>  				const char *color)
>  {
>  	if (state->branch)
> @@ -1292,8 +1293,8 @@ static void print_rebase_state(struct wt_status *s,
>  				 _("You are currently rebasing."));
>  }
>  
> -static void show_rebase_in_progress(struct wt_status *s,
> -				struct wt_status_state *state,
> +static void show_rebase_in_progress(const struct wt_status *s,
> +				const struct wt_status_state *state,
>  				const char *color)
>  {
>  	struct stat st;
> @@ -1345,8 +1346,8 @@ static void show_rebase_in_progress(struct wt_status *s,
>  	wt_longstatus_print_trailer(s);
>  }
>  
> -static void show_cherry_pick_in_progress(struct wt_status *s,
> -					struct wt_status_state *state,
> +static void show_cherry_pick_in_progress(const struct wt_status *s,
> +					const struct wt_status_state *state,
>  					const char *color)
>  {
>  	status_printf_ln(s, color, _("You are currently cherry-picking commit %s."),
> @@ -1364,8 +1365,8 @@ static void show_cherry_pick_in_progress(struct wt_status *s,
>  	wt_longstatus_print_trailer(s);
>  }
>  
> -static void show_revert_in_progress(struct wt_status *s,
> -					struct wt_status_state *state,
> +static void show_revert_in_progress(const struct wt_status *s,
> +					const struct wt_status_state *state,
>  					const char *color)
>  {
>  	status_printf_ln(s, color, _("You are currently reverting commit %s."),
> @@ -1383,8 +1384,8 @@ static void show_revert_in_progress(struct wt_status *s,
>  	wt_longstatus_print_trailer(s);
>  }
>  
> -static void show_bisect_in_progress(struct wt_status *s,
> -				struct wt_status_state *state,
> +static void show_bisect_in_progress(const struct wt_status *s,
> +				const struct wt_status_state *state,
>  				const char *color)
>  {
>  	if (state->branch)
> @@ -1538,12 +1539,16 @@ int wt_status_check_bisect(const struct worktree *wt,
>  	return 0;
>  }
>  
> -void wt_status_get_state(struct wt_status_state *state,
> -			 int get_detached_from)
> +void wt_status_get_state(
> +		const struct wt_status *s, struct wt_status_state *state)
>  {
> +	int get_detached_from =
> +		(s == NULL) || (s->branch && !strcmp(s->branch, "HEAD"));
>  	struct stat st;
>  	struct object_id oid;
>  
> +	memset(state, 0, sizeof(*state));
> +
>  	if (!stat(git_path_merge_head(), &st)) {
>  		state->merge_in_progress = 1;
>  	} else if (wt_status_check_rebase(NULL, state)) {
> @@ -1564,8 +1569,15 @@ void wt_status_get_state(struct wt_status_state *state,
>  		wt_status_get_detached_from(state);
>  }
>  
> +void wt_status_clear_state(struct wt_status_state *state)
> +{
> +	free(state->branch);
> +	free(state->onto);
> +	free(state->detached_from);
> +}
> +
>  static void wt_longstatus_print_state(struct wt_status *s,
> -				      struct wt_status_state *state)
> +				      const struct wt_status_state *state)
>  {
>  	const char *state_color = color(WT_STATUS_HEADER, s);
>  	if (state->merge_in_progress)
> @@ -1582,30 +1594,25 @@ static void wt_longstatus_print_state(struct wt_status *s,
>  		show_bisect_in_progress(s, state, state_color);
>  }
>  
> -static void wt_longstatus_print(struct wt_status *s)
> +static void wt_longstatus_print(struct wt_status *s, const struct wt_status_state *state)
>  {
>  	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 (state->rebase_in_progress || state->rebase_interactive_in_progress) {
> +				if (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 = state->onto;
> +			} else if (state->detached_from) {
> +				branch_name = state->detached_from;
> +				if (state->detached_at)
>  					on_what = _("HEAD detached at ");
>  				else
>  					on_what = _("HEAD detached from ");
> @@ -1622,10 +1629,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, state);
>  
>  	if (s->is_initial) {
>  		status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
> @@ -1657,14 +1661,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)
> @@ -1700,7 +1704,7 @@ static void wt_longstatus_print(struct wt_status *s)
>  }
>  
>  static void wt_shortstatus_unmerged(struct string_list_item *it,
> -			   struct wt_status *s)
> +			   const struct wt_status *s)
>  {
>  	struct wt_status_change_data *d = it->util;
>  	const char *how = "??";
> @@ -1727,7 +1731,7 @@ static void wt_shortstatus_unmerged(struct string_list_item *it,
>  }
>  
>  static void wt_shortstatus_status(struct string_list_item *it,
> -			 struct wt_status *s)
> +			 const struct wt_status *s)
>  {
>  	struct wt_status_change_data *d = it->util;
>  
> @@ -1770,7 +1774,7 @@ static void wt_shortstatus_status(struct string_list_item *it,
>  }
>  
>  static void wt_shortstatus_other(struct string_list_item *it,
> -				 struct wt_status *s, const char *sign)
> +				 const struct wt_status *s, const char *sign)
>  {
>  	if (s->null_termination) {
>  		fprintf(stdout, "%s %s%c", sign, it->string, 0);
> @@ -1784,7 +1788,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
>  	}
>  }
>  
> -static void wt_shortstatus_print_tracking(struct wt_status *s)
> +static void wt_shortstatus_print_tracking(const struct wt_status *s)
>  {
>  	struct branch *branch;
>  	const char *header_color = color(WT_STATUS_HEADER, s);
> @@ -1860,7 +1864,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>  	fputc(s->null_termination ? '\0' : '\n', s->fp);
>  }
>  
> -static void wt_shortstatus_print(struct wt_status *s)
> +static void wt_shortstatus_print(const struct wt_status *s)
>  {
>  	struct string_list_item *it;
>  
> @@ -1924,18 +1928,14 @@ static void wt_porcelain_print(struct wt_status *s)
>   * upstream.  When AHEAD_BEHIND_QUICK is requested and the branches
>   * are different, '?' will be substituted for the actual count.
>   */
> -static void wt_porcelain_v2_print_tracking(struct wt_status *s)
> +static void wt_porcelain_v2_print_tracking(const struct wt_status *s, const struct wt_status_state *state)
>  {
>  	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);
> @@ -1946,10 +1946,10 @@ 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 (state->rebase_in_progress || state->rebase_interactive_in_progress)
> +				branch_name = state->onto;
> +			else if (state->detached_from)
> +				branch_name = state->detached_from;
>  			else
>  				branch_name = "";
>  		} else {
> @@ -1983,10 +1983,6 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
>  			}
>  		}
>  	}
> -
> -	free(state.branch);
> -	free(state.onto);
> -	free(state.detached_from);
>  }
>  
>  /*
> @@ -1994,7 +1990,7 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
>   * fixed-length string of characters in the buffer provided.
>   */
>  static void wt_porcelain_v2_submodule_state(
> -	struct wt_status_change_data *d,
> +	const struct wt_status_change_data *d,
>  	char sub[5])
>  {
>  	if (S_ISGITLINK(d->mode_head) ||
> @@ -2017,8 +2013,8 @@ static void wt_porcelain_v2_submodule_state(
>   * Fix-up changed entries before we print them.
>   */
>  static void wt_porcelain_v2_fix_up_changed(
> -	struct string_list_item *it,
> -	struct wt_status *s)
> +	const struct string_list_item *it,
> +	const struct wt_status *s)
>  {
>  	struct wt_status_change_data *d = it->util;
>  
> @@ -2066,8 +2062,8 @@ static void wt_porcelain_v2_fix_up_changed(
>   * Print porcelain v2 info for tracked entries with changes.
>   */
>  static void wt_porcelain_v2_print_changed_entry(
> -	struct string_list_item *it,
> -	struct wt_status *s)
> +	const struct string_list_item *it,
> +	const struct wt_status *s)
>  {
>  	struct wt_status_change_data *d = it->util;
>  	struct strbuf buf = STRBUF_INIT;
> @@ -2130,8 +2126,8 @@ static void wt_porcelain_v2_print_changed_entry(
>   * Print porcelain v2 status info for unmerged entries.
>   */
>  static void wt_porcelain_v2_print_unmerged_entry(
> -	struct string_list_item *it,
> -	struct wt_status *s)
> +	const struct string_list_item *it,
> +	const struct wt_status *s)
>  {
>  	struct wt_status_change_data *d = it->util;
>  	const struct cache_entry *ce;
> @@ -2211,8 +2207,8 @@ static void wt_porcelain_v2_print_unmerged_entry(
>   * Print porcelain V2 status info for untracked and ignored entries.
>   */
>  static void wt_porcelain_v2_print_other(
> -	struct string_list_item *it,
> -	struct wt_status *s,
> +	const struct string_list_item *it,
> +	const struct wt_status *s,
>  	char prefix)
>  {
>  	struct strbuf buf = STRBUF_INIT;
> @@ -2242,14 +2238,14 @@ static void wt_porcelain_v2_print_other(
>   * [<v2_ignored_items>]*
>   *
>   */
> -static void wt_porcelain_v2_print(struct wt_status *s)
> +static void wt_porcelain_v2_print(const struct wt_status *s, const struct wt_status_state *state)
>  {
>  	struct wt_status_change_data *d;
>  	struct string_list_item *it;
>  	int i;
>  
>  	if (s->show_branch)
> -		wt_porcelain_v2_print_tracking(s);
> +		wt_porcelain_v2_print_tracking(s, state);
>  
>  	for (i = 0; i < s->change.nr; i++) {
>  		it = &(s->change.items[i]);
> @@ -2276,7 +2272,9 @@ static void wt_porcelain_v2_print(struct wt_status *s)
>  	}
>  }
>  
> -void wt_status_print(struct wt_status *s)
> +// FIXME: `struct wt_status *` should be `const struct wt_status` but because
> +// `wt_porcelain_print()` modifies it, that has to first be fixed
> +void wt_status_print(struct wt_status *s, const struct wt_status_state *state)
>  {
>  	switch (s->status_format) {
>  	case STATUS_FORMAT_SHORT:
> @@ -2286,14 +2284,14 @@ void wt_status_print(struct wt_status *s)
>  		wt_porcelain_print(s);
>  		break;
>  	case STATUS_FORMAT_PORCELAIN_V2:
> -		wt_porcelain_v2_print(s);
> +		wt_porcelain_v2_print(s, state);
>  		break;
>  	case STATUS_FORMAT_UNSPECIFIED:
>  		die("BUG: finalize_deferred_config() should have been called");
>  		break;
>  	case STATUS_FORMAT_NONE:
>  	case STATUS_FORMAT_LONG:
> -		wt_longstatus_print(s);
> +		wt_longstatus_print(s, state);
>  		break;
>  	}
>  }
> diff --git a/wt-status.h b/wt-status.h
> index 430770b85..6cccfd7a0 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -94,7 +94,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;
> @@ -126,18 +126,19 @@ struct wt_status_state {
>  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_get_state(struct wt_status_state *state, int get_detached_from);
> +void wt_status_print(struct wt_status *s, const struct wt_status_state *state);
> +void wt_status_collect(struct wt_status *s, const struct wt_status_state *state);
> +void wt_status_get_state(const struct wt_status *s, struct wt_status_state *state);
> +void wt_status_clear_state(struct wt_status_state *state);
>  int wt_status_check_rebase(const struct worktree *wt,
>  			   struct wt_status_state *state);
>  int wt_status_check_bisect(const struct worktree *wt,
>  			   struct wt_status_state *state);
>  
>  __attribute__((format (printf, 3, 4)))
> -void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);
> +void status_printf_ln(const struct wt_status *s, const char *color, const char *fmt, ...);
>  __attribute__((format (printf, 3, 4)))
> -void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
> +void status_printf(const struct wt_status *s, const char *color, const char *fmt, ...);
>  
>  /* The following functions expect that the caller took care of reading the index. */
>  int has_unstaged_changes(int ignore_submodules);

[PATCH v3 3/3] commit: fix exit code for --short/--porcelain

[Samuel Lijin]

In wt-status.c, the s->commitable bit is set only in the call tree of
wt_longstatus_print(), which means that when there are changes to be
committed or all merge conflicts have been resolved, --dry-run and
--long return the correct exit code, but --short and --porcelain do not,
even though they both imply --dry-run.

Teach wt_status_collect() to set s->committable correctly so that
--short and --porcelain return the correct exit code in the above
described situations and mark the documenting tests as fixed.

Also stop setting s->committable in wt_longstatus_print_updated() and
show_merge_in_progress(), and const-ify wt_status_state in the method
signatures in those callpaths.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh |  8 ++---
 wt-status.c       | 82 +++++++++++++++++++++++++++++------------------
 2 files changed, 55 insertions(+), 35 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index be087e73f..b6492322f 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,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
 '
@@ -691,11 +691,11 @@ test_expect_success '--dry-run with conflicts fixed from a merge' '
 	git commit --dry-run
 '
 
-test_expect_failure '--short with conflicts fixed from a merge' '
+test_expect_success '--short with conflicts fixed from a merge' '
 	git commit --short
 '
 
-test_expect_failure '--porcelain with conflicts fixed from a merge' '
+test_expect_success '--porcelain with conflicts fixed from a merge' '
 	git commit --porcelain
 '
 
diff --git a/wt-status.c b/wt-status.c
index 75d389944..4ba657978 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,39 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static int has_unmerged(const 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 wt_status_mark_committable(
+		struct wt_status *s, const struct wt_status_state *state)
+{
+	int i;
+
+	if (state->merge_in_progress && !has_unmerged(s)) {
+		s->committable = 1;
+		return;
+	}
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+
+		if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
+			s->committable = 1;
+			return;
+		}
+	}
+}
+
 void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 {
 	wt_status_collect_changes_worktree(s);
@@ -728,6 +761,8 @@ void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
 		wt_status_collect_changes_index(s);
 
 	wt_status_collect_untracked(s);
+
+	wt_status_mark_committable(s, state);
 }
 
 static void wt_longstatus_print_unmerged(const struct wt_status *s)
@@ -753,28 +788,28 @@ static void wt_longstatus_print_unmerged(const struct wt_status *s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
-	int shown_header = 0;
 	int i;
 
+	if (!s->committable) {
+		return;
+	}
+
+	wt_longstatus_print_cached_header(s);
+
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		struct string_list_item *it;
 		it = &(s->change.items[i]);
 		d = it->util;
-		if (!d->index_status ||
-		    d->index_status == DIFF_STATUS_UNMERGED)
-			continue;
-		if (!shown_header) {
-			wt_longstatus_print_cached_header(s);
-			s->committable = 1;
-			shown_header = 1;
+		if (d->index_status &&
+		    d->index_status != DIFF_STATUS_UNMERGED) {
+			wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 		}
-		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 	}
-	if (shown_header)
-		wt_longstatus_print_trailer(s);
+
+	wt_longstatus_print_trailer(s);
 }
 
 /*
@@ -1056,21 +1091,7 @@ static void wt_longstatus_print_tracking(const struct wt_status *s)
 	strbuf_release(&sb);
 }
 
-static int has_unmerged(const 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,
-				const struct wt_status_state *state,
+static void show_merge_in_progress(const struct wt_status *s,
 				const char *color)
 {
 	if (has_unmerged(s)) {
@@ -1082,7 +1103,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)
@@ -1576,12 +1596,12 @@ void wt_status_clear_state(struct wt_status_state *state)
 	free(state->detached_from);
 }
 
-static void wt_longstatus_print_state(struct wt_status *s,
+static void wt_longstatus_print_state(const struct wt_status *s,
 				      const struct wt_status_state *state)
 {
 	const char *state_color = color(WT_STATUS_HEADER, s);
 	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);
 	else if (state->rebase_in_progress || state->rebase_interactive_in_progress)
@@ -1594,7 +1614,7 @@ static void wt_longstatus_print_state(struct wt_status *s,
 		show_bisect_in_progress(s, state, state_color);
 }
 
-static void wt_longstatus_print(struct wt_status *s, const struct wt_status_state *state)
+static void wt_longstatus_print(const struct wt_status *s, const struct wt_status_state *state)
 {
 	const char *branch_color = color(WT_STATUS_ONBRANCH, s);
 	const char *branch_status_color = color(WT_STATUS_HEADER, s);
-- 
2.18.0

Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain

[Junio C Hamano]

Samuel Lijin <sxlijin@gmail.com> writes:

> diff --git a/wt-status.c b/wt-status.c
> index 75d389944..4ba657978 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -718,6 +718,39 @@ static void wt_status_collect_untracked(struct wt_status *s)
>  		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
>  }
>  
> +static int has_unmerged(const 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 wt_status_mark_committable(
> +		struct wt_status *s, const struct wt_status_state *state)
> +{
> +	int i;
> +
> +	if (state->merge_in_progress && !has_unmerged(s)) {
> +		s->committable = 1;
> +		return;
> +	}

Is this trying to say:

	During/after a merge, if there is no higher stage entry in
	the index, we can commit.

I am wondering if we also should say:

	During/after a merge, if there is any unresolved conflict in
	the index, we cannot commit.
	
in which case the above becomes more like this:

	if (state->merge_in_progress) {
		s->committable = !has_unmerged(s);
		return;
	}

But with your patch, with no remaining conflict in the index during
a merge, the control comes here and goes into the next loop.

> +	for (i = 0; i < s->change.nr; i++) {
> +		struct wt_status_change_data *d = (s->change.items[i]).util;
> +
> +		if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
> +			s->committable = 1;
> +			return;
> +		}
> +	}

The loop seems to say "As long as there is one entry in the index
that is not in conflict and is different from the HEAD, then we can
commit".  Is that correct?  

Imagine there are two paths A and B in the branches involved in a
merge, and A cleanly resolves (say, we take their version because
our history did not touch it since we diverged) while B has
conflict.  We'll come to this loop (because we are in a merge but
have some unmerged paths) and we find that A is different from HEAD,
happily set committable bit and return.

I _think_ with the change to "what happens during merge" above that
I suggested, this loop automatically becomes correct, but I didn't
think it through.  If there are ways other than .merge_in_progress
that place conflicted entries in the index, then this loop is still
incorrect and would want to be more like:

	for (i = 0; i < s->change.nr; i++) {
		struct wt_status_change_data *d = (s->change.items[i]).util;

		if (d->index_status == DIFF_STATUS_UNMERGED) {
			s->committable = 0;
			return;
		}
		if (d->index_status)
			s->committable = 1;
	}

i.e. we declare "not ready to commit" if there is *any* conflicted
entry, but otherwise set committable to 1 if we see any entry that
is different from HEAD (to declare succcess once we successfully
loop through to the last entry without seeing any conflict).

>  void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
>  {
>  	wt_status_collect_changes_worktree(s);
> @@ -728,6 +761,8 @@ void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
>  		wt_status_collect_changes_index(s);
>  
>  	wt_status_collect_untracked(s);
> +
> +	wt_status_mark_committable(s, state);
>  }
>  
>  static void wt_longstatus_print_unmerged(const struct wt_status *s)
> @@ -753,28 +788,28 @@ static void wt_longstatus_print_unmerged(const struct wt_status *s)
>  
>  }
>  
> -static void wt_longstatus_print_updated(struct wt_status *s)
> +static void wt_longstatus_print_updated(const struct wt_status *s)
>  {
> -	int shown_header = 0;
>  	int i;
>  
> +	if (!s->committable) {
> +		return;
> +	}

No need to have {} around a single statement.  Especially when you
know you won't be touching the line (e.g. to later add more
statements in the block) in this last patch in a series.

> +	wt_longstatus_print_cached_header(s);
> +

Re: [PATCH v3 3/3] commit: fix exit code for --short/--porcelain

[Samuel Lijin]

Thanks for the review.

On Tue, Jul 17, 2018 at 10:33 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Samuel Lijin <sxlijin@gmail.com> writes:
>
> > diff --git a/wt-status.c b/wt-status.c
> > index 75d389944..4ba657978 100644
> > --- a/wt-status.c
> > +++ b/wt-status.c
> > @@ -718,6 +718,39 @@ static void wt_status_collect_untracked(struct wt_status *s)
> >               s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
> >  }
> >
> > +static int has_unmerged(const 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 wt_status_mark_committable(
> > +             struct wt_status *s, const struct wt_status_state *state)
> > +{
> > +     int i;
> > +
> > +     if (state->merge_in_progress && !has_unmerged(s)) {
> > +             s->committable = 1;
> > +             return;
> > +     }
>
> Is this trying to say:
>
>         During/after a merge, if there is no higher stage entry in
>         the index, we can commit.
>
> I am wondering if we also should say:
>
>         During/after a merge, if there is any unresolved conflict in
>         the index, we cannot commit.
>
> in which case the above becomes more like this:
>
>         if (state->merge_in_progress) {
>                 s->committable = !has_unmerged(s);
>                 return;
>         }
>
> But with your patch, with no remaining conflict in the index during
> a merge, the control comes here and goes into the next loop.
>
> > +     for (i = 0; i < s->change.nr; i++) {
> > +             struct wt_status_change_data *d = (s->change.items[i]).util;
> > +
> > +             if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
> > +                     s->committable = 1;
> > +                     return;
> > +             }
> > +     }
>
> The loop seems to say "As long as there is one entry in the index
> that is not in conflict and is different from the HEAD, then we can
> commit".  Is that correct?
>
> Imagine there are two paths A and B in the branches involved in a
> merge, and A cleanly resolves (say, we take their version because
> our history did not touch it since we diverged) while B has
> conflict.  We'll come to this loop (because we are in a merge but
> have some unmerged paths) and we find that A is different from HEAD,
> happily set committable bit and return.

I'll be honest: when I wrote this, I didn't think too much about what
the code was actually doing, semantically speaking: I was assuming
that the behavior that set the commitable bit in the call tree of
wt_longstatus_print() was correct, and that it was just a matter of
mechanically copying that logic over to the --short/--porcelain call
paths.

Looking into this more deeply, I think you're right, but more
problematically, this is technically a bug with the current Git code
that seems to be cancelled out by another bug: wt_status_state
apparently does not correctly reflect the state of the index when it
reaches wt_longstatus_print_updated(). Working from master
(f55ff46c9), I modified the last test in t7501 to look like this:

→.echo "Initial contents, unimportant" | tee test-file1 test-file2 &&
→.git add test-file1 test-file2 &&
→.git commit -m "Initial commit" &&
→.echo "commit-1-state" | tee test-file1 test-file2 &&
→.git commit -m "commit 1" -i test-file1 test-file2 &&
→.git tag commit-1 &&
→.git checkout -b branch-2 HEAD^1 &&
→.echo "commit-2-state" | tee test-file1 test-file2 &&
→.git commit -m "commit 2" -i test-file1 test-file2 &&
→.! $(git merge --no-commit commit-1) &&
→.echo "commit-2-state" | tee test-file1 &&
→.git add test-file1 &&
→.git commit --dry-run &&
→.git commit -m "conflicts fixed from merge."

And once inside gdb did this:

(gdb) b wt-status.c:766
Breakpoint 1 at 0x205d73: file wt-status.c, line 766.
(gdb) r
Starting program: /home/pockets/git/git commit --dry-run
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/usr/lib/libthread_db.so.1".
On branch branch-2
You have unmerged paths.
  (fix conflicts and run "git commit")
  (use "git merge --abort" to abort the merge)


Breakpoint 1, wt_longstatus_print_updated (s=0x555555a29960 <s>) at
wt-status.c:766
warning: Source file is more recent than executable.
760             for (i = 0; i < s->change.nr; i++) {
(gdb) print s->change.nr
$1 = 1

Can you confirm I'm not crazy, and am analyzing this correctly?

> I _think_ with the change to "what happens during merge" above that
> I suggested, this loop automatically becomes correct, but I didn't
> think it through.  If there are ways other than .merge_in_progress
> that place conflicted entries in the index, then this loop is still
> incorrect and would want to be more like:
>
>         for (i = 0; i < s->change.nr; i++) {
>                 struct wt_status_change_data *d = (s->change.items[i]).util;
>
>                 if (d->index_status == DIFF_STATUS_UNMERGED) {
>                         s->committable = 0;
>                         return;
>                 }
>                 if (d->index_status)
>                         s->committable = 1;
>         }
>
> i.e. we declare "not ready to commit" if there is *any* conflicted
> entry, but otherwise set committable to 1 if we see any entry that
> is different from HEAD (to declare succcess once we successfully
> loop through to the last entry without seeing any conflict).
>
> >  void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
> >  {
> >       wt_status_collect_changes_worktree(s);
> > @@ -728,6 +761,8 @@ void wt_status_collect(struct wt_status *s, const struct wt_status_state *state)
> >               wt_status_collect_changes_index(s);
> >
> >       wt_status_collect_untracked(s);
> > +
> > +     wt_status_mark_committable(s, state);
> >  }
> >
> >  static void wt_longstatus_print_unmerged(const struct wt_status *s)
> > @@ -753,28 +788,28 @@ static void wt_longstatus_print_unmerged(const struct wt_status *s)
> >
> >  }
> >
> > -static void wt_longstatus_print_updated(struct wt_status *s)
> > +static void wt_longstatus_print_updated(const struct wt_status *s)
> >  {
> > -     int shown_header = 0;
> >       int i;
> >
> > +     if (!s->committable) {
> > +             return;
> > +     }
>
> No need to have {} around a single statement.  Especially when you
> know you won't be touching the line (e.g. to later add more
> statements in the block) in this last patch in a series.
>
> > +     wt_longstatus_print_cached_header(s);
> > +

[PATCH v2 1/2] commit: fix --short and --porcelain options

[Samuel Lijin]

Mark the commitable flag in the wt_status object in the call to
`wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
and simplify the logic in the latter function to take advantage of the
logic shifted to the former. This means that callers do not need to use
`wt_longstatus_print_updated()` to collect the `commitable` flag;
calling `wt_status_collect()` is sufficient.

As a result, invoking `git commit` with `--short` or `--porcelain`
(which imply `--dry-run`, but previously returned an inconsistent error
code inconsistent with dry run behavior) correctly returns status code
zero when there is something to commit. This fixes two bugs documented
in the test suite.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 t/t7501-commit.sh |  4 ++--
 wt-status.c       | 38 +++++++++++++++++++++++++++-----------
 2 files changed, 29 insertions(+), 13 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index fa61b1a4e..85a8217fd 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -87,12 +87,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
 '
diff --git a/wt-status.c b/wt-status.c
index 50815e5fa..2e5452731 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -718,6 +718,19 @@ static void wt_status_collect_untracked(struct wt_status *s)
 		s->untracked_in_ms = (getnanotime() - t_begin) / 1000000;
 }
 
+static void wt_status_mark_commitable(struct wt_status *s) {
+	int i;
+
+	for (i = 0; i < s->change.nr; i++) {
+		struct wt_status_change_data *d = (s->change.items[i]).util;
+
+		if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
+			s->commitable = 1;
+			return;
+		}
+	}
+}
+
 void wt_status_collect(struct wt_status *s)
 {
 	wt_status_collect_changes_worktree(s);
@@ -726,7 +739,10 @@ void wt_status_collect(struct wt_status *s)
 		wt_status_collect_changes_initial(s);
 	else
 		wt_status_collect_changes_index(s);
+
 	wt_status_collect_untracked(s);
+
+	wt_status_mark_commitable(s);
 }
 
 static void wt_longstatus_print_unmerged(struct wt_status *s)
@@ -754,26 +770,26 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
 
 static void wt_longstatus_print_updated(struct wt_status *s)
 {
-	int shown_header = 0;
 	int i;
 
+	if (!s->commitable) {
+		return;
+	}
+
+	wt_longstatus_print_cached_header(s);
+
 	for (i = 0; i < s->change.nr; i++) {
 		struct wt_status_change_data *d;
 		struct string_list_item *it;
 		it = &(s->change.items[i]);
 		d = it->util;
-		if (!d->index_status ||
-		    d->index_status == DIFF_STATUS_UNMERGED)
-			continue;
-		if (!shown_header) {
-			wt_longstatus_print_cached_header(s);
-			s->commitable = 1;
-			shown_header = 1;
+		if (d->index_status &&
+		    d->index_status != DIFF_STATUS_UNMERGED) {
+			wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 		}
-		wt_longstatus_print_change_data(s, WT_STATUS_UPDATED, it);
 	}
-	if (shown_header)
-		wt_longstatus_print_trailer(s);
+
+	wt_longstatus_print_trailer(s);
 }
 
 /*
-- 
2.17.0

Re: [PATCH v2 1/2] commit: fix --short and --porcelain options

[Junio C Hamano]

Samuel Lijin <sxlijin@gmail.com> writes:

> Mark the commitable flag in the wt_status object in the call to
> `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> and simplify the logic in the latter function to take advantage of the
> logic shifted to the former. This means that callers do not need to use
> `wt_longstatus_print_updated()` to collect the `commitable` flag;
> calling `wt_status_collect()` is sufficient.
>
> As a result, invoking `git commit` with `--short` or `--porcelain`
> (which imply `--dry-run`, but previously returned an inconsistent error
> code inconsistent with dry run behavior) correctly returns status code
> zero when there is something to commit. This fixes two bugs documented
> in the test suite.


Hmm, I couldn't quite get what the above two paragraphs were trying
to say, but I think I figured out by looking at wt_status.c before
applying this patch, so let me see if I correctly understand what
this patch is about by thinking a bit aloud.

There are only two assignments to s->commitable in wt-status.c; one
happens in wt_longstatus_print_updated(), when the function notices
there is even one record to be shown (i.e. there is an "updated"
path) and the other in show_merge_in_progress() which is called by
wt_longstatus_prpint_state().  The latter codepath happens when we
are in a merge and there is no remaining conflicted paths (the code
allows the contents to be committed to be identical to HEAD).  Both
are called from wt_longstatus_print(), which in turn is called by
wt_status_print().

The implication of the above observation is that we do not set
commitable bit (by the way, shouldn't we spell it with two 'T's?)
if we are not doing the long format status.  The title hints at it
but "fix" is too vague.  It would be easier to understand if it
began like this (i.e. state problem clearly first, before outlining
the solution):

	[PATCH 1/2] commit: fix exit status under --short/--porcelain options

	In wt-status.c, s->commitable bit is set only in the
	codepaths reachable from wt_status_print() when output
	format is STATUS_FORMAT_LONG as a side effect of printing
	bits of status.  Consequently, when running with --short and
	--porcelain options, the bit is not set to reflect if there
	is anything to be committed, and "git commit --short" or
	"--porcelain" (both of which imply "--dry-run") failed to
	signal if there is anything to commit with its exit status.

	Instead, update s->commitable bit in wt_status_collect(),
	regardless of the output format. ...

Is that what is going on here?  Yours made it sound as if moving the
code to _collect() was done for the sake of moving code around and
simplifying the logic, and bugfix fell out of the move merely as a
side effect, which probably was the source of my confusion.

> +static void wt_status_mark_commitable(struct wt_status *s) {
> +	int i;
> +
> +	for (i = 0; i < s->change.nr; i++) {
> +		struct wt_status_change_data *d = (s->change.items[i]).util;
> +
> +		if (d->index_status && d->index_status != DIFF_STATUS_UNMERGED) {
> +			s->commitable = 1;
> +			return;
> +		}
> +	}
> +}

I am not sure if this is sufficient.  From a cursory look of the
existing code (and vague recollection in my ageing brain ;-), I
think we say it is committable if

 (1) when not merging, there is something to show in the "to be
     committed" section (i.e. there must be something changed since
     HEAD in the index).

 (2) when merging, no conflicting paths remain (i.e. change.nr being
     zero is fine).

So it is unclear to me how you are dealing with (2) under "--short"
option, which does not call show_merge_in_progress() to catch that
case.

Re: [PATCH v2 1/2] commit: fix --short and --porcelain options

[Samuel Lijin]

On Tue, May 1, 2018 at 10:50 PM Junio C Hamano <gitster@pobox.com> wrote:

> Samuel Lijin <sxlijin@gmail.com> writes:

> > Mark the commitable flag in the wt_status object in the call to
> > `wt_status_collect()`, instead of in `wt_longstatus_print_updated()`,
> > and simplify the logic in the latter function to take advantage of the
> > logic shifted to the former. This means that callers do not need to use
> > `wt_longstatus_print_updated()` to collect the `commitable` flag;
> > calling `wt_status_collect()` is sufficient.
> >
> > As a result, invoking `git commit` with `--short` or `--porcelain`
> > (which imply `--dry-run`, but previously returned an inconsistent error
> > code inconsistent with dry run behavior) correctly returns status code
> > zero when there is something to commit. This fixes two bugs documented
> > in the test suite.


> Hmm, I couldn't quite get what the above two paragraphs were trying
> to say, but I think I figured out by looking at wt_status.c before
> applying this patch, so let me see if I correctly understand what
> this patch is about by thinking a bit aloud.

> There are only two assignments to s->commitable in wt-status.c; one
> happens in wt_longstatus_print_updated(), when the function notices
> there is even one record to be shown (i.e. there is an "updated"
> path) and the other in show_merge_in_progress() which is called by
> wt_longstatus_prpint_state().  The latter codepath happens when we
> are in a merge and there is no remaining conflicted paths (the code
> allows the contents to be committed to be identical to HEAD).  Both
> are called from wt_longstatus_print(), which in turn is called by
> wt_status_print().

> The implication of the above observation is that we do not set
> commitable bit (by the way, shouldn't we spell it with two 'T's?)

Yep, MW confirms: https://www.merriam-webster.com/dictionary/commitable

I didn't think to check how common "commitable" is in the codebase, but it
doesn't seem to be too many, looking at the output of `git grep
commitable`, so I'll add that to the patch series when I reroll.

> if we are not doing the long format status.  The title hints at it
> but "fix" is too vague.  It would be easier to understand if it
> began like this (i.e. state problem clearly first, before outlining
> the solution):

>          [PATCH 1/2] commit: fix exit status under --short/--porcelain
options

>          In wt-status.c, s->commitable bit is set only in the
>          codepaths reachable from wt_status_print() when output
>          format is STATUS_FORMAT_LONG as a side effect of printing
>          bits of status.  Consequently, when running with --short and
>          --porcelain options, the bit is not set to reflect if there
>          is anything to be committed, and "git commit --short" or
>          "--porcelain" (both of which imply "--dry-run") failed to
>          signal if there is anything to commit with its exit status.

>          Instead, update s->commitable bit in wt_status_collect(),
>          regardless of the output format. ...

> Is that what is going on here?  Yours made it sound as if moving the
> code to _collect() was done for the sake of moving code around and
> simplifying the logic, and bugfix fell out of the move merely as a
> side effect, which probably was the source of my confusion.

Yep, that's right. I wasn't sure if the imperative tone was required for
the whole commit or just the description, hence the awkward structure. (I
also wasn't sure how strict the 70 char limit on the description was.)

> > +static void wt_status_mark_commitable(struct wt_status *s) {
> > +     int i;
> > +
> > +     for (i = 0; i < s->change.nr; i++) {
> > +             struct wt_status_change_data *d =
(s->change.items[i]).util;
> > +
> > +             if (d->index_status && d->index_status !=
DIFF_STATUS_UNMERGED) {
> > +                     s->commitable = 1;
> > +                     return;
> > +             }
> > +     }
> > +}

> I am not sure if this is sufficient.  From a cursory look of the
> existing code (and vague recollection in my ageing brain ;-), I
> think we say it is committable if

>   (1) when not merging, there is something to show in the "to be
>       committed" section (i.e. there must be something changed since
>       HEAD in the index).

>   (2) when merging, no conflicting paths remain (i.e. change.nr being
>       zero is fine).

> So it is unclear to me how you are dealing with (2) under "--short"
> option, which does not call show_merge_in_progress() to catch that
> case.

And the answer there is that I'm not :) I had hoped that there was a test
to catch mistakes like this but evidently not. Thanks for pointing that
out, I'll add a test to catch that.

I'm also realizing that I didn't const-ify wt_longstatus_print() in the
next patch, which is another reason I didn't catch this.

This seems a bit trickier to handle. What do you think of this approach:
(1) move wt_status_state into wt_status and (2) move the call to
wt_status_get_state from wt_longstatus_print/wt_porcelain_v2_print_tracking
to wt_status_collect?

(I kind of also want to suggest enum-ifying some of wt_status_state, but
that feels beyond the scope of this and also prone to other pitfalls.)

[PATCH v2 2/2] wt-status: const-ify all printf helper methods

[Samuel Lijin]

Change the method signatures of all printf helper methods to take a
`const struct wt_status *` rather than a `struct wt_status *`.

Signed-off-by: Samuel Lijin <sxlijin@gmail.com>
---
 wt-status.c | 18 +++++++++---------
 wt-status.h |  4 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/wt-status.c b/wt-status.c
index 2e5452731..4360bbfd7 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -33,7 +33,7 @@ static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NIL,    /* WT_STATUS_ONBRANCH */
 };
 
-static const char *color(int slot, struct wt_status *s)
+static const char *color(int slot, const struct wt_status *s)
 {
 	const char *c = "";
 	if (want_color(s->use_color))
@@ -43,7 +43,7 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
-static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+static void status_vprintf(const struct wt_status *s, int at_bol, const char *color,
 		const char *fmt, va_list ap, const char *trail)
 {
 	struct strbuf sb = STRBUF_INIT;
@@ -89,7 +89,7 @@ static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
 	strbuf_release(&sb);
 }
 
-void status_printf_ln(struct wt_status *s, const char *color,
+void status_printf_ln(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -99,7 +99,7 @@ void status_printf_ln(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-void status_printf(struct wt_status *s, const char *color,
+void status_printf(const struct wt_status *s, const char *color,
 			const char *fmt, ...)
 {
 	va_list ap;
@@ -109,7 +109,7 @@ void status_printf(struct wt_status *s, const char *color,
 	va_end(ap);
 }
 
-static void status_printf_more(struct wt_status *s, const char *color,
+static void status_printf_more(const struct wt_status *s, const char *color,
 			       const char *fmt, ...)
 {
 	va_list ap;
@@ -192,7 +192,7 @@ static void wt_longstatus_print_unmerged_header(struct wt_status *s)
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_cached_header(struct wt_status *s)
+static void wt_longstatus_print_cached_header(const struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
@@ -239,7 +239,7 @@ static void wt_longstatus_print_other_header(struct wt_status *s,
 	status_printf_ln(s, c, "%s", "");
 }
 
-static void wt_longstatus_print_trailer(struct wt_status *s)
+static void wt_longstatus_print_trailer(const struct wt_status *s)
 {
 	status_printf_ln(s, color(WT_STATUS_HEADER, s), "%s", "");
 }
@@ -332,7 +332,7 @@ static void wt_longstatus_print_unmerged_data(struct wt_status *s,
 	strbuf_release(&onebuf);
 }
 
-static void wt_longstatus_print_change_data(struct wt_status *s,
+static void wt_longstatus_print_change_data(const struct wt_status *s,
 					    int change_type,
 					    struct string_list_item *it)
 {
@@ -768,7 +768,7 @@ static void wt_longstatus_print_unmerged(struct wt_status *s)
 
 }
 
-static void wt_longstatus_print_updated(struct wt_status *s)
+static void wt_longstatus_print_updated(const struct wt_status *s)
 {
 	int i;
 
diff --git a/wt-status.h b/wt-status.h
index 430770b85..83a1f7c00 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -135,9 +135,9 @@ int wt_status_check_bisect(const struct worktree *wt,
 			   struct wt_status_state *state);
 
 __attribute__((format (printf, 3, 4)))
-void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf_ln(const struct wt_status *s, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
-void status_printf(struct wt_status *s, const char *color, const char *fmt, ...);
+void status_printf(const struct wt_status *s, const char *color, const char *fmt, ...);
 
 /* The following functions expect that the caller took care of reading the index. */
 int has_unstaged_changes(int ignore_submodules);
-- 
2.17.0