[PATCH] status: show in-progress info for short status

[PATCH] status: show in-progress info for short status

[Michael J Gruber]

Ordinary (long) status shows information about bisect, revert, am,
rebase, cherry-pick in progress, and so does git-prompt.sh. status
--short currently shows none of this information.

Introduce an `--inprogress` argument to git status so that, when used with
`--short --branch`, in-progress information is shown next to the branch
information. Just like `--branch`, this comes with a config option.

The wording for the in-progress information is taken over from
git-prompt.sh.

Signed-off-by: Michael J Gruber <git@grubix.eu>
---
When used with --porcelain, this gives an easy way to amend the powerline-gitstatus
prompt for example (in use locally here), and certainly others.

I don't touch --porcelain=v2 - that one reads in-progress state but does not seem
to display it. I don't know which parts are supposed to be stable for v2. 

 Documentation/config.txt     |  4 +++
 Documentation/git-status.txt |  5 ++++
 builtin/commit.c             | 13 ++++++++++
 t/t7512-status-help.sh       | 58 ++++++++++++++++++++++++++++++++++++++++----
 wt-status.c                  | 32 +++++++++++++++++++++---
 wt-status.h                  |  1 +
 6 files changed, 105 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 475e874d51..3adc65f9d3 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2957,6 +2957,10 @@ status.branch::
 	Set to true to enable --branch by default in linkgit:git-status[1].
 	The option --no-branch takes precedence over this variable.
 
+status.inprogress::
+	Set to true to enable --inprogress by default in linkgit:git-status[1].
+	The option --no-inprogress takes precedence over this variable.
+
 status.displayCommentPrefix::
 	If set to true, linkgit:git-status[1] will insert a comment
 	prefix before each output line (starting with
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ba873657cf..4fac073247 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -32,6 +32,11 @@ OPTIONS
 --branch::
 	Show the branch and tracking info even in short-format.
 
+-p::
+--inprogress::
+	When showing branch and tracking info in short-format,
+	show in-progress information (BISECTING, MERGING etc.), too.
+
 --porcelain[=<version>]::
 	Give the output in an easy-to-parse format for scripts.
 	This is similar to the short output, but will remain stable
diff --git a/builtin/commit.c b/builtin/commit.c
index 4e288bc513..6176dd2c2f 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1105,8 +1105,10 @@ static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
 	enum wt_status_format status_format;
 	int show_branch;
+	int show_inprogress;
 } status_deferred_config = {
 	STATUS_FORMAT_UNSPECIFIED,
+	-1, /* unspecified */
 	-1 /* unspecified */
 };
 
@@ -1133,6 +1135,10 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->show_branch = status_deferred_config.show_branch;
 	if (s->show_branch < 0)
 		s->show_branch = 0;
+	if (use_deferred_config && s->show_inprogress < 0)
+		s->show_inprogress = status_deferred_config.show_inprogress;
+	if (s->show_inprogress < 0)
+		s->show_inprogress = 0;
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1291,6 +1297,10 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		status_deferred_config.show_branch = git_config_bool(k, v);
 		return 0;
 	}
+	if (!strcmp(k, "status.inprogress")) {
+		status_deferred_config.show_inprogress = git_config_bool(k, v);
+		return 0;
+	}
 	if (!strcmp(k, "status.color") || !strcmp(k, "color.status")) {
 		s->use_color = git_config_colorbool(k, v);
 		return 0;
@@ -1339,6 +1349,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			    N_("show status concisely"), STATUS_FORMAT_SHORT),
 		OPT_BOOL('b', "branch", &s.show_branch,
 			 N_("show branch information")),
+		OPT_BOOL('p', "inprogress", &s.show_inprogress,
+			 N_("show in-progress information")),
 		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
 		  N_("version"), N_("machine-readable output"),
 		  PARSE_OPT_OPTARG, opt_parse_porcelain },
@@ -1612,6 +1624,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),
 		OPT_BOOL(0, "branch", &s.show_branch, N_("show branch information")),
+		OPT_BOOL(0, "inprogress", &s.show_inprogress, N_("show in-progress information")),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    N_("machine-readable output"), STATUS_FORMAT_PORCELAIN),
 		OPT_SET_INT(0, "long", &status_format,
diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
index 458608cc1e..103e006249 100755
--- a/t/t7512-status-help.sh
+++ b/t/t7512-status-help.sh
@@ -74,7 +74,6 @@ test_expect_success 'prepare for rebase conflicts' '
 
 
 test_expect_success 'status when rebase in progress before resolving conflicts' '
-	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short HEAD^^) &&
 	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
 	cat >expected <<EOF &&
@@ -96,6 +95,15 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'short status when rebase in progress' '
+	test_when_finished "git rebase --abort" &&
+	cat >expected <<EOF &&
+## HEAD (no branch); REBASE-m
+UU main.txt
+EOF
+	git status --untracked-files=no --short --branch --inprogress >actual &&
+	test_i18ncmp expected actual
+'
 
 test_expect_success 'status when rebase in progress before rebase --continue' '
 	git reset --hard rebase_conflicts &&
@@ -133,7 +141,6 @@ test_expect_success 'prepare for rebase_i_conflicts' '
 
 
 test_expect_success 'status during rebase -i when conflicts unresolved' '
-	test_when_finished "git rebase --abort" &&
 	ONTO=$(git rev-parse --short rebase_i_conflicts) &&
 	LAST_COMMIT=$(git rev-parse --short rebase_i_conflicts_second) &&
 	test_must_fail git rebase -i rebase_i_conflicts &&
@@ -159,6 +166,16 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'short status during rebase -i when conflicts unresolved' '
+	test_when_finished "git rebase --abort" &&
+	cat >expected <<EOF &&
+## HEAD (no branch); REBASE-i
+UU main.txt
+EOF
+	git status --untracked-files=no --short --branch --inprogress >actual &&
+	test_i18ncmp expected actual
+'
+
 
 test_expect_success 'status during rebase -i after resolving conflicts' '
 	git reset --hard rebase_i_conflicts_second &&
@@ -613,7 +630,6 @@ test_expect_success 'prepare am_session' '
 
 test_expect_success 'status in an am session: file already exists' '
 	git checkout -b am_already_exists &&
-	test_when_finished "rm Maildir/* && git am --abort" &&
 	git format-patch -1 -oMaildir &&
 	test_must_fail git am Maildir/*.patch &&
 	cat >expected <<\EOF &&
@@ -629,6 +645,14 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'short status in an am session: file already exists' '
+	test_when_finished "rm Maildir/* && git am --abort" &&
+	cat >expected <<\EOF &&
+## am_already_exists; AM
+EOF
+	git status --untracked-files=no --short --branch --inprogress >actual &&
+	test_i18ncmp expected actual
+'
 
 test_expect_success 'status in an am session: file does not exist' '
 	git reset --hard am_session &&
@@ -681,7 +705,6 @@ test_expect_success 'status when bisecting' '
 	test_commit one_bisect main.txt one &&
 	test_commit two_bisect main.txt two &&
 	test_commit three_bisect main.txt three &&
-	test_when_finished "git bisect reset" &&
 	git bisect start &&
 	git bisect bad &&
 	git bisect good one_bisect &&
@@ -697,6 +720,14 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'short status when bisecting' '
+	test_when_finished "git bisect reset" &&
+	cat >expected <<EOF &&
+## HEAD (no branch); BISECTING
+EOF
+	git status --untracked-files=no --short --branch --inprogress >actual &&
+	test_i18ncmp expected actual
+'
 
 test_expect_success 'status when rebase conflicts with statushints disabled' '
 	git reset --hard master &&
@@ -736,7 +767,6 @@ test_expect_success 'prepare for cherry-pick conflicts' '
 
 
 test_expect_success 'status when cherry-picking before resolving conflicts' '
-	test_when_finished "git cherry-pick --abort" &&
 	test_must_fail git cherry-pick cherry_branch_second &&
 	TO_CHERRY_PICK=$(git rev-parse --short CHERRY_PICK_HEAD) &&
 	cat >expected <<EOF &&
@@ -756,6 +786,15 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'short status when cherry-picking before resolving conflicts' '
+	test_when_finished "git cherry-pick --abort" &&
+	cat >expected <<EOF &&
+## cherry_branch; CHERRY-PICKING
+UU main.txt
+EOF
+	git status --untracked-files=no --short --branch --inprogress >actual &&
+	test_i18ncmp expected actual
+'
 
 test_expect_success 'status when cherry-picking after resolving conflicts' '
 	git reset --hard cherry_branch &&
@@ -827,6 +866,15 @@ EOF
 	test_i18ncmp expected actual
 '
 
+test_expect_success 'short status while reverting commit (conflicts)' '
+	cat >expected <<EOF &&
+## master; REVERTING
+UU to-revert.txt
+EOF
+	git status --untracked-files=no --short --branch --inprogress >actual &&
+	test_i18ncmp expected actual
+'
+
 test_expect_success 'status while reverting commit (conflicts resolved)' '
 	echo reverted >to-revert.txt &&
 	git add to-revert.txt &&
diff --git a/wt-status.c b/wt-status.c
index 308cf3779e..f4606d27c8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1715,6 +1715,7 @@ static void wt_shortstatus_other(struct string_list_item *it,
 static void wt_shortstatus_print_tracking(struct wt_status *s)
 {
 	struct branch *branch;
+	struct wt_status_state state;
 	const char *header_color = color(WT_STATUS_HEADER, s);
 	const char *branch_color_local = color(WT_STATUS_LOCAL_BRANCH, s);
 	const char *branch_color_remote = color(WT_STATUS_REMOTE_BRANCH, s);
@@ -1738,7 +1739,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	if (!strcmp(s->branch, "HEAD")) {
 		color_fprintf(s->fp, color(WT_STATUS_NOBRANCH, s), "%s",
 			      LABEL(N_("HEAD (no branch)")));
-		goto conclude;
+		goto inprogress;
 	}
 
 	skip_prefix(branch_name, "refs/heads/", &branch_name);
@@ -1749,7 +1750,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 
 	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {
 		if (!base)
-			goto conclude;
+			goto inprogress;
 
 		upstream_is_gone = 1;
 	}
@@ -1760,7 +1761,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	free((char *)base);
 
 	if (!upstream_is_gone && !num_ours && !num_theirs)
-		goto conclude;
+		goto inprogress;
 
 	color_fprintf(s->fp, header_color, " [");
 	if (upstream_is_gone) {
@@ -1779,6 +1780,31 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	}
 
 	color_fprintf(s->fp, header_color, "]");
+
+ inprogress:
+	if (!s->show_inprogress)
+		goto conclude;
+	memset(&state, 0, sizeof(state));
+	wt_status_get_state(&state,
+			    s->branch && !strcmp(s->branch, "HEAD"));
+	if (state.merge_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("MERGING")));
+	else if (state.am_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("AM")));
+	else if (state.rebase_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-m")));
+	else if (state.rebase_interactive_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-i")));
+	else if (state.cherry_pick_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("CHERRY-PICKING")));
+	else if (state.revert_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REVERTING")));
+	if (state.bisect_in_progress)
+		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("BISECTING")));
+	free(state.branch);
+	free(state.onto);
+	free(state.detached_from);
+
  conclude:
 	fputc(s->null_termination ? '\0' : '\n', s->fp);
 }
diff --git a/wt-status.h b/wt-status.h
index 54fec77032..8f6c9e0e62 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -77,6 +77,7 @@ struct wt_status {
 	unsigned colopts;
 	int null_termination;
 	int show_branch;
+	int show_inprogress;
 	int hints;
 
 	enum wt_status_format status_format;
-- 
2.12.2.739.gfc3eb97820

Re: [PATCH] status: show in-progress info for short status

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> Ordinary (long) status shows information about bisect, revert, am,
> rebase, cherry-pick in progress, and so does git-prompt.sh. status
> --short currently shows none of this information.
>
> Introduce an `--inprogress` argument to git status so that, when used with
> `--short --branch`, in-progress information is shown next to the branch
> information. Just like `--branch`, this comes with a config option.
>
> The wording for the in-progress information is taken over from
> git-prompt.sh.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>

I haven't formed an opinion on the feature itself, or the way it is
triggered, so I won't comment on them.  I just say --porcelain (any
version) may (or may not) want to be extended in backward compatible
way (but again I haven't formed an opinion on the issue--I just know
and say there is an issue there that needs to be considered at this
point).

> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
> index 458608cc1e..103e006249 100755
> --- a/t/t7512-status-help.sh
> +++ b/t/t7512-status-help.sh
> @@ -74,7 +74,6 @@ test_expect_success 'prepare for rebase conflicts' '
>  
>  
>  test_expect_success 'status when rebase in progress before resolving conflicts' '
> -	test_when_finished "git rebase --abort" &&
>  	ONTO=$(git rev-parse --short HEAD^^) &&
>  	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
>  	cat >expected <<EOF &&
> @@ -96,6 +95,15 @@ EOF
>  	test_i18ncmp expected actual
>  '
>  
> +test_expect_success 'short status when rebase in progress' '
> +	test_when_finished "git rebase --abort" &&
> +	cat >expected <<EOF &&
> +## HEAD (no branch); REBASE-m
> +UU main.txt
> +EOF
> +	git status --untracked-files=no --short --branch --inprogress >actual &&
> +	test_i18ncmp expected actual
> +'

This is not a good way to structure the test.  If the one in the
previous hunk is what creates a conflicted state by running
"rebase", check the status output from within that test, after the
conflicting "rebase" fails and other things the existing test checks
are tested.  That way, you do not have to worry about this new check
getting confused if the previous one fails in the middle.

Likewise for the most (if not all---I didn't check very carefully)
of the remaining hunks in this test script.

Re: [PATCH] status: show in-progress info for short status

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 846 bytes --]

On Fri, Mar 31, 2017 at 03:59:17PM +0200, Michael J Gruber wrote:
> Ordinary (long) status shows information about bisect, revert, am,
> rebase, cherry-pick in progress, and so does git-prompt.sh. status
> --short currently shows none of this information.
> 
> Introduce an `--inprogress` argument to git status so that, when used with
> `--short --branch`, in-progress information is shown next to the branch
> information. Just like `--branch`, this comes with a config option.

I don't have a strong opinion on this feature, but I would note that to
be consistent with other options, we'd probably want to write this as
--in-progress (with an extra dash).
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

Re: [PATCH] status: show in-progress info for short status

[SZEDER Gábor]

> @@ -1779,6 +1780,31 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>  	}
>  
>  	color_fprintf(s->fp, header_color, "]");
> +
> + inprogress:
> +	if (!s->show_inprogress)
> +		goto conclude;
> +	memset(&state, 0, sizeof(state));
> +	wt_status_get_state(&state,
> +			    s->branch && !strcmp(s->branch, "HEAD"));
> +	if (state.merge_in_progress)
> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("MERGING")));
> +	else if (state.am_in_progress)
> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("AM")));
> +	else if (state.rebase_in_progress)
> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-m")));
> +	else if (state.rebase_interactive_in_progress)
> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-i")));
> +	else if (state.cherry_pick_in_progress)
> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("CHERRY-PICKING")));
> +	else if (state.revert_in_progress)
> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REVERTING")));
> +	if (state.bisect_in_progress)

else if?

> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("BISECTING")));
> +	free(state.branch);
> +	free(state.onto);
> +	free(state.detached_from);
> +
>   conclude:
>  	fputc(s->null_termination ? '\0' : '\n', s->fp);
>  }

This reminded me of a patch that I have been using for almost two
years now...

git-prompt.sh's similar long conditional chain to show the ongoing
operation has an else-branch at the end showing "AM/REBASE".  Your
patch doesn't add an equivalent branch.  Is this intentional or an
oversight?

I suppose it's intentional, because that "AM/REBASE" branch in the
prompt seems to be unreachable (see below), but I never took the
effort to actually check that (hence the "seems" and that's why I
never submitted it).



 -- >8 --

Subject: [PATCH] bash prompt: remove unreachable ongoing "AM/REBASE" fallback

Back in the day it was impossible to tell an ongoing 'am' and "plain"
'rebase' (i.e. non-interactive, non-merge) apart, thus the prompt
displayed "AM/REBASE" in those cases, see e75201963 (Improve bash
prompt to detect various states like an unfinished merge, 2007-09-30).
Later 3041c3243 (am: --rebasing, 2008-03-04) made it possible to tell
those cases apart and made __git_ps1() use this right away to display
either "AM" or "REBASE", respectively.  However, it left that
"AM/REBASE" in an else-branch as a fallback, but it seems to have been
impossible to reach even back then.

This ancient unreachable else-branch has survived to this day, remove
it finally.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-prompt.sh | 2 --
 1 file changed, 2 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index c6cbef38c..d13af41f2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -411,8 +411,6 @@ __git_ps1 ()
 				r="|REBASE"
 			elif [ -f "$g/rebase-apply/applying" ]; then
 				r="|AM"
-			else
-				r="|AM/REBASE"
 			fi
 		elif [ -f "$g/MERGE_HEAD" ]; then
 			r="|MERGING"
-- 
2.12.2.613.g9c5b79913

Re: [PATCH] status: show in-progress info for short status

[Michael J Gruber]

SZEDER Gábor venit, vidit, dixit 06.04.2017 16:33:
>> @@ -1779,6 +1780,31 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>>  	}
>>  
>>  	color_fprintf(s->fp, header_color, "]");
>> +
>> + inprogress:
>> +	if (!s->show_inprogress)
>> +		goto conclude;
>> +	memset(&state, 0, sizeof(state));
>> +	wt_status_get_state(&state,
>> +			    s->branch && !strcmp(s->branch, "HEAD"));
>> +	if (state.merge_in_progress)
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("MERGING")));
>> +	else if (state.am_in_progress)
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("AM")));
>> +	else if (state.rebase_in_progress)
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-m")));
>> +	else if (state.rebase_interactive_in_progress)
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-i")));
>> +	else if (state.cherry_pick_in_progress)
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("CHERRY-PICKING")));
>> +	else if (state.revert_in_progress)
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REVERTING")));
>> +	if (state.bisect_in_progress)
> 
> else if?

No. You can do all of the above during a bisect.

> 
>> +		color_fprintf(s->fp, header_color, "; %s", LABEL(N_("BISECTING")));
>> +	free(state.branch);
>> +	free(state.onto);
>> +	free(state.detached_from);
>> +
>>   conclude:
>>  	fputc(s->null_termination ? '\0' : '\n', s->fp);
>>  }
> 
> This reminded me of a patch that I have been using for almost two
> years now...
> 
> git-prompt.sh's similar long conditional chain to show the ongoing
> operation has an else-branch at the end showing "AM/REBASE".  Your
> patch doesn't add an equivalent branch.  Is this intentional or an
> oversight?

I go over all states that wt_status_get_state can give.

> I suppose it's intentional, because that "AM/REBASE" branch in the
> prompt seems to be unreachable (see below), but I never took the
> effort to actually check that (hence the "seems" and that's why I
> never submitted it).

Note that wt_status_get_state and the prompt script do things quite
differently.

I suppose that the prompt could make use of the in-progress info being
exposed by "git status" rather doing its on thing.

Michael

Re: [PATCH] status: show in-progress info for short status

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 31.03.2017 20:14:
> Michael J Gruber <git@grubix.eu> writes:
> 
>> Ordinary (long) status shows information about bisect, revert, am,
>> rebase, cherry-pick in progress, and so does git-prompt.sh. status
>> --short currently shows none of this information.
>>
>> Introduce an `--inprogress` argument to git status so that, when used with
>> `--short --branch`, in-progress information is shown next to the branch
>> information. Just like `--branch`, this comes with a config option.
>>
>> The wording for the in-progress information is taken over from
>> git-prompt.sh.
>>
>> Signed-off-by: Michael J Gruber <git@grubix.eu>
> 
> I haven't formed an opinion on the feature itself, or the way it is
> triggered, so I won't comment on them.  I just say --porcelain (any
> version) may (or may not) want to be extended in backward compatible
> way (but again I haven't formed an opinion on the issue--I just know
> and say there is an issue there that needs to be considered at this
> point).

With my change, "git status --porcelain" output does not change at all
(and neither does that of "git status --short"). They change only when
"--inprogress" (and "--branch") is requested, just like with "--branch".

I don't know the v2 format - it seems that it's built to be extensible,
but I have not checked whether that#s documented somewhere, and I didn't
change it anyways.

>> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
>> index 458608cc1e..103e006249 100755
>> --- a/t/t7512-status-help.sh
>> +++ b/t/t7512-status-help.sh
>> @@ -74,7 +74,6 @@ test_expect_success 'prepare for rebase conflicts' '
>>  
>>  
>>  test_expect_success 'status when rebase in progress before resolving conflicts' '
>> -	test_when_finished "git rebase --abort" &&
>>  	ONTO=$(git rev-parse --short HEAD^^) &&
>>  	test_must_fail git rebase HEAD^ --onto HEAD^^ &&
>>  	cat >expected <<EOF &&
>> @@ -96,6 +95,15 @@ EOF
>>  	test_i18ncmp expected actual
>>  '
>>  
>> +test_expect_success 'short status when rebase in progress' '
>> +	test_when_finished "git rebase --abort" &&
>> +	cat >expected <<EOF &&
>> +## HEAD (no branch); REBASE-m
>> +UU main.txt
>> +EOF
>> +	git status --untracked-files=no --short --branch --inprogress >actual &&
>> +	test_i18ncmp expected actual
>> +'
> 
> This is not a good way to structure the test.  If the one in the
> previous hunk is what creates a conflicted state by running
> "rebase", check the status output from within that test, after the
> conflicting "rebase" fails and other things the existing test checks
> are tested.  That way, you do not have to worry about this new check
> getting confused if the previous one fails in the middle.
> 
> Likewise for the most (if not all---I didn't check very carefully)
> of the remaining hunks in this test script.

All followed the same structure. I did it that way so that it's clearer
which test is failing, but I don't mind putting things together as you
describe. Most of the time, one has to check where in the &&-chain a
test failed, anyways.

Michael

Re: [PATCH] status: show in-progress info for short status

[Jacob Keller]

On Fri, Mar 31, 2017 at 11:14 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Michael J Gruber <git@grubix.eu> writes:
>
>> Ordinary (long) status shows information about bisect, revert, am,
>> rebase, cherry-pick in progress, and so does git-prompt.sh. status
>> --short currently shows none of this information.
>>
>> Introduce an `--inprogress` argument to git status so that, when used with
>> `--short --branch`, in-progress information is shown next to the branch
>> information. Just like `--branch`, this comes with a config option.
>>
>> The wording for the in-progress information is taken over from
>> git-prompt.sh.
>>
>> Signed-off-by: Michael J Gruber <git@grubix.eu>
>
> I haven't formed an opinion on the feature itself, or the way it is
> triggered, so I won't comment on them.  I just say --porcelain (any
> version) may (or may not) want to be extended in backward compatible
> way (but again I haven't formed an opinion on the issue--I just know
> and say there is an issue there that needs to be considered at this
> point).
>

Personally, I would want this to become the default and not have a new
option to trigger it. I think we could also extend the porcelain
format to include this information as well, but I'm not too familiar
with how the v2 format extends or not.

Thanks,
Jake

>> diff --git a/t/t7512-status-help.sh b/t/t7512-status-help.sh
>> index 458608cc1e..103e006249 100755
>> --- a/t/t7512-status-help.sh
>> +++ b/t/t7512-status-help.sh
>> @@ -74,7 +74,6 @@ test_expect_success 'prepare for rebase conflicts' '
>>
>>
>>  test_expect_success 'status when rebase in progress before resolving conflicts' '
>> -     test_when_finished "git rebase --abort" &&
>>       ONTO=$(git rev-parse --short HEAD^^) &&
>>       test_must_fail git rebase HEAD^ --onto HEAD^^ &&
>>       cat >expected <<EOF &&
>> @@ -96,6 +95,15 @@ EOF
>>       test_i18ncmp expected actual
>>  '
>>
>> +test_expect_success 'short status when rebase in progress' '
>> +     test_when_finished "git rebase --abort" &&
>> +     cat >expected <<EOF &&
>> +## HEAD (no branch); REBASE-m
>> +UU main.txt
>> +EOF
>> +     git status --untracked-files=no --short --branch --inprogress >actual &&
>> +     test_i18ncmp expected actual
>> +'
>
> This is not a good way to structure the test.  If the one in the
> previous hunk is what creates a conflicted state by running
> "rebase", check the status output from within that test, after the
> conflicting "rebase" fails and other things the existing test checks
> are tested.  That way, you do not have to worry about this new check
> getting confused if the previous one fails in the middle.
>
> Likewise for the most (if not all---I didn't check very carefully)
> of the remaining hunks in this test script.

Re: [PATCH] status: show in-progress info for short status

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> Personally, I would want this to become the default and not have a new
> option to trigger it. I think we could also extend the porcelain
> format to include this information as well, but I'm not too familiar
> with how the v2 format extends or not.

I think the general rule of thumb for --porcelain is that we can
freely introduce new record types without version bump, and expect
the reading scripts to ignore unrecognised records (we may need to
describe this a bit more strongly in our document, though), while
changes to the format of existing records must require a command
line option that cannot be turned on by default with configuration
(or a version bump, if you want to change the output format by
default).

I am getting the impression that this "we are doing X" is a new and
discinct record type that existing readers can safely ignore?  If
that is the case, it may be better to add it without making it
optional.

Re: [PATCH] status: show in-progress info for short status

[Jacob Keller]

On Wed, Apr 12, 2017 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Personally, I would want this to become the default and not have a new
>> option to trigger it. I think we could also extend the porcelain
>> format to include this information as well, but I'm not too familiar
>> with how the v2 format extends or not.
>
> I think the general rule of thumb for --porcelain is that we can
> freely introduce new record types without version bump, and expect
> the reading scripts to ignore unrecognised records (we may need to
> describe this a bit more strongly in our document, though), while
> changes to the format of existing records must require a command
> line option that cannot be turned on by default with configuration
> (or a version bump, if you want to change the output format by
> default).
>
> I am getting the impression that this "we are doing X" is a new and
> discinct record type that existing readers can safely ignore?  If
> that is the case, it may be better to add it without making it
> optional.

Correct. But either way we should be free to change and extend the
non-porcelain format without worry I thought?

Thanks,
Jake

Re: [PATCH] status: show in-progress info for short status

[Jacob Keller]

On Wed, Apr 12, 2017 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Personally, I would want this to become the default and not have a new
>> option to trigger it. I think we could also extend the porcelain
>> format to include this information as well, but I'm not too familiar
>> with how the v2 format extends or not.
>
> I think the general rule of thumb for --porcelain is that we can
> freely introduce new record types without version bump, and expect
> the reading scripts to ignore unrecognised records (we may need to
> describe this a bit more strongly in our document, though), while
> changes to the format of existing records must require a command
> line option that cannot be turned on by default with configuration
> (or a version bump, if you want to change the output format by
> default).
>
> I am getting the impression that this "we are doing X" is a new and
> discinct record type that existing readers can safely ignore?  If
> that is the case, it may be better to add it without making it
> optional.

I think we are safe in extending porcelain v2.

Thanks,
Jake

Re: [PATCH] status: show in-progress info for short status

[SZEDER Gábor]

On Fri, Apr 7, 2017 at 4:05 PM, Michael J Gruber <git@grubix.eu> wrote:
> SZEDER Gábor venit, vidit, dixit 06.04.2017 16:33:
>>> @@ -1779,6 +1780,31 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
>>>      }
>>>
>>>      color_fprintf(s->fp, header_color, "]");
>>> +
>>> + inprogress:
>>> +    if (!s->show_inprogress)
>>> +            goto conclude;
>>> +    memset(&state, 0, sizeof(state));
>>> +    wt_status_get_state(&state,
>>> +                        s->branch && !strcmp(s->branch, "HEAD"));
>>> +    if (state.merge_in_progress)
>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("MERGING")));
>>> +    else if (state.am_in_progress)
>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("AM")));
>>> +    else if (state.rebase_in_progress)
>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-m")));

In the prompt "REBASE-m" is only shown during 'rebase --merge', while
"REBASE" is shown during a plain 'rebase'.  Here "REBASE-m" will be
shown during both.

>>> +    else if (state.rebase_interactive_in_progress)
>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REBASE-i")));
>>> +    else if (state.cherry_pick_in_progress)
>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("CHERRY-PICKING")));
>>> +    else if (state.revert_in_progress)
>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("REVERTING")));
>>> +    if (state.bisect_in_progress)
>>
>> else if?
>
> No. You can do all of the above during a bisect.

Right indeed.  I think the prompt should do the same.  Onto the TODO
list it goes, then.

>>> +            color_fprintf(s->fp, header_color, "; %s", LABEL(N_("BISECTING")));
>>> +    free(state.branch);
>>> +    free(state.onto);
>>> +    free(state.detached_from);
>>> +
>>>   conclude:
>>>      fputc(s->null_termination ? '\0' : '\n', s->fp);
>>>  }
>>
>> This reminded me of a patch that I have been using for almost two
>> years now...
>>
>> git-prompt.sh's similar long conditional chain to show the ongoing
>> operation has an else-branch at the end showing "AM/REBASE".  Your
>> patch doesn't add an equivalent branch.  Is this intentional or an
>> oversight?
>
> I go over all states that wt_status_get_state can give.

Yeah, but are those states exclusive?  Or is it possible that both
'am_in_progress' and 'rebase_in_progress' are set?  I suppose it's not
possible.
It would certainly be more obvious if it were a single enum field
instead of a bunch of ints.

>> I suppose it's intentional, because that "AM/REBASE" branch in the
>> prompt seems to be unreachable (see below), but I never took the
>> effort to actually check that (hence the "seems" and that's why I
>> never submitted it).
>
> Note that wt_status_get_state and the prompt script do things quite
> differently.
>
> I suppose that the prompt could make use of the in-progress info being
> exposed by "git status" rather doing its on thing.

The prompt currently gets all this in-progress info using only Bash
builtins, which is much faster than running a git process in a
subshell.  This is especially important on Windows, where the overhead
of running a git process is large enough to make the runtime of
__git_ps1() noticeable when displaying the prompt.  That's the main
reason for much of the shell complexity and ugliness in git-prompt.sh.

Besides, current output formats make 'git status' unsuitable for the
prompt.

Re: [PATCH] status: show in-progress info for short status

[Junio C Hamano]

Michael J Gruber <git@grubix.eu> writes:

> Ordinary (long) status shows information about bisect, revert, am,
> rebase, cherry-pick in progress, and so does git-prompt.sh. status
> --short currently shows none of this information.
>
> Introduce an `--inprogress` argument to git status so that, when used with
> `--short --branch`, in-progress information is shown next to the branch
> information. Just like `--branch`, this comes with a config option.
>
> The wording for the in-progress information is taken over from
> git-prompt.sh.
>
> Signed-off-by: Michael J Gruber <git@grubix.eu>
> ---
> When used with --porcelain, this gives an easy way to amend the powerline-gitstatus
> prompt for example (in use locally here), and certainly others.
>
> I don't touch --porcelain=v2 - that one reads in-progress state but does not seem
> to display it. I don't know which parts are supposed to be stable for v2. 
>
>  Documentation/config.txt     |  4 +++
>  Documentation/git-status.txt |  5 ++++
>  builtin/commit.c             | 13 ++++++++++
>  t/t7512-status-help.sh       | 58 ++++++++++++++++++++++++++++++++++++++++----
>  wt-status.c                  | 32 +++++++++++++++++++++---
>  wt-status.h                  |  1 +
>  6 files changed, 105 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 475e874d51..3adc65f9d3 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2957,6 +2957,10 @@ status.branch::
>  	Set to true to enable --branch by default in linkgit:git-status[1].
>  	The option --no-branch takes precedence over this variable.
>  
> +status.inprogress::
> +	Set to true to enable --inprogress by default in linkgit:git-status[1].
> +	The option --no-inprogress takes precedence over this variable.
> +

Has the name of the option been discussed already and the list
reached a consensus?  "in progress" is a two-word phrase, and I am
wondering if this should be status.inProgress (and the option name
should be "--in-progress").

> +-p::
> +--inprogress::
> +	When showing branch and tracking info in short-format,
> +	show in-progress information (BISECTING, MERGING etc.), too.
> +

I do not think we want to give short-and-sweet "-p" to this option.
"git log -p" gives a patch output, "git status -p" sounds like it
does so, too (i.e. "git show" plus various status).  Besides, for
those who are lazy, you already have a configuration variable.

Re: [PATCH] status: show in-progress info for short status

[Junio C Hamano]

Jacob Keller <jacob.keller@gmail.com> writes:

> On Wed, Apr 12, 2017 at 11:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Jacob Keller <jacob.keller@gmail.com> writes:
>>
>>> Personally, I would want this to become the default and not have a new
>>> option to trigger it. I think we could also extend the porcelain
>>> format to include this information as well, but I'm not too familiar
>>> with how the v2 format extends or not.

I started to do a simple s/inprogress/in-progress/ while waiting,
but Ævar reminded me of this discussion---and I agree with you that
this probably should be on.  Moreover, I think this should not be
optional (which makes s/inprogress/in-progress/ a moot point).  

Michael went the cautious route to make it optional just like the
"-b/--branch" option, but I think "-b/--branch" was a design mistake
and not something we want to mimic.  The long output format gives
the same information without "--branch", and giving "--no-branch"
does not even disable the information in the long format; "--branch"
is only effective in the short format, even though giving it to long
format does not diagnose any error.  

We should have just enhanced the feature unconditionally, I would
think.  But fixing that past mistake is a separate issue.

>> I think the general rule of thumb for --porcelain is that we can
>> freely introduce new record types without version bump, and expect
>> the reading scripts to ignore unrecognised records (we may need to
>> describe this a bit more strongly in our document, though), while
>> changes to the format of existing records must require a command
>> line option that cannot be turned on by default with configuration
>> (or a version bump, if you want to change the output format by
>> default).
>>
>> I am getting the impression that this "we are doing X" is a new and
>> discinct record type that existing readers can safely ignore?  If
>> that is the case, it may be better to add it without making it
>> optional.
>
> Correct. But either way we should be free to change and extend the
> non-porcelain format without worry I thought?

While I think we should update "short" and "porcelain" at the same
time when it is clear that we can (e.g. we are adding a new record
type and we make sure that the current readers of "porcelain" output
would ignore the new record type), an update to "porcelain" can come
later, as long as we don't forget.  Otherwise people will script
around "short", ignoring "porcelain", no?

Thanks.