[1/3] status: add status.aheadbehind setting
diff mbox series

Message ID 2f0f2a2df9410947b985d83a924d2549d62bb65f.1560889284.git.gitgitgadget@gmail.com
State New
Headers show
Series
  • git-status: create config for ahead/behind calculation
Related show

Commit Message

Matthew Rogers via GitGitGadget June 18, 2019, 8:21 p.m. UTC
From: Jeff Hostetler <jeffhost@microsoft.com>

The --[no-]ahead-behind option was introduced in fd9b544a
(status: add --[no-]ahead-behind to status and commit for V2
format, 2018-01-09). This is a necessary change of behavior
in repos where the remote tracking branches can move very
quickly ahead of the local branches. However, users need to
remember to provide the command-line argument every time.

Add a new "status.aheadBehind" config setting to change the
default behavior of all git status formats.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
---
 Documentation/config/status.txt |  5 +++++
 builtin/commit.c                | 17 ++++++++++++++++-
 t/t6040-tracking-info.sh        | 31 +++++++++++++++++++++++++++++++
 t/t7064-wtstatus-pv2.sh         |  4 ++++
 4 files changed, 56 insertions(+), 1 deletion(-)

Comments

Junio C Hamano June 21, 2019, 4:33 p.m. UTC | #1
"Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Jeff Hostetler <jeffhost@microsoft.com>
>
> The --[no-]ahead-behind option was introduced in fd9b544a
> (status: add --[no-]ahead-behind to status and commit for V2
> format, 2018-01-09). This is a necessary change of behavior
> in repos where the remote tracking branches can move very
> quickly ahead of the local branches. However, users need to
> remember to provide the command-line argument every time.
>
> Add a new "status.aheadBehind" config setting to change the
> default behavior of all git status formats.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  Documentation/config/status.txt |  5 +++++
>  builtin/commit.c                | 17 ++++++++++++++++-
>  t/t6040-tracking-info.sh        | 31 +++++++++++++++++++++++++++++++
>  t/t7064-wtstatus-pv2.sh         |  4 ++++
>  4 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
> index ed72fa7dae..0fc704ab80 100644
> --- a/Documentation/config/status.txt
> +++ b/Documentation/config/status.txt
> @@ -12,6 +12,11 @@ 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.aheadBehind::
> +	Set to true to enable `--ahead-behind` and false to enable
> +	`--no-ahead-behind` by default in linkgit:git-status[1] for
> +	non-porcelain status formats.  Defaults to true.

Sensible.

> @@ -1078,9 +1078,11 @@ static const char *read_commit_message(const char *name)
>  static struct status_deferred_config {
>  	enum wt_status_format status_format;
>  	int show_branch;
> +	enum ahead_behind_flags ahead_behind;
>  } status_deferred_config = {
>  	STATUS_FORMAT_UNSPECIFIED,
> -	-1 /* unspecified */
> +	-1, /* unspecified */
> +	AHEAD_BEHIND_UNSPECIFIED,

This obviously is not a problem introduced by this patch, but is
there a plan to extend this beyond a boolean?

Otherwise, a separate enum is way overkill.  Naming the field so
that it is clear it is either true or false (e.g.  perhaps call it
"ahead_behind_detailed" as the current "QUICK" is merely "are they
equal?" which corresponds to "false", and "FULL" is to show the
detailed info), and then use the usual "-1 is unspecified, 0 and 1
are usual bools" convention would be more appropriate.
Jeff Hostetler June 26, 2019, 8:02 p.m. UTC | #2
On 6/21/2019 12:33 PM, Junio C Hamano wrote:
> "Jeff Hostetler via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> The --[no-]ahead-behind option was introduced in fd9b544a
>> (status: add --[no-]ahead-behind to status and commit for V2
>> format, 2018-01-09). This is a necessary change of behavior
>> in repos where the remote tracking branches can move very
>> quickly ahead of the local branches. However, users need to
>> remember to provide the command-line argument every time.
>>
>> Add a new "status.aheadBehind" config setting to change the
>> default behavior of all git status formats.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
>> ---
>>   Documentation/config/status.txt |  5 +++++
>>   builtin/commit.c                | 17 ++++++++++++++++-
>>   t/t6040-tracking-info.sh        | 31 +++++++++++++++++++++++++++++++
>>   t/t7064-wtstatus-pv2.sh         |  4 ++++
>>   4 files changed, 56 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
>> index ed72fa7dae..0fc704ab80 100644
>> --- a/Documentation/config/status.txt
>> +++ b/Documentation/config/status.txt
>> @@ -12,6 +12,11 @@ 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.aheadBehind::
>> +	Set to true to enable `--ahead-behind` and false to enable
>> +	`--no-ahead-behind` by default in linkgit:git-status[1] for
>> +	non-porcelain status formats.  Defaults to true.
> 
> Sensible.
> 
>> @@ -1078,9 +1078,11 @@ static const char *read_commit_message(const char *name)
>>   static struct status_deferred_config {
>>   	enum wt_status_format status_format;
>>   	int show_branch;
>> +	enum ahead_behind_flags ahead_behind;
>>   } status_deferred_config = {
>>   	STATUS_FORMAT_UNSPECIFIED,
>> -	-1 /* unspecified */
>> +	-1, /* unspecified */
>> +	AHEAD_BEHIND_UNSPECIFIED,
> 
> This obviously is not a problem introduced by this patch, but is
> there a plan to extend this beyond a boolean?
> 
> Otherwise, a separate enum is way overkill.  Naming the field so
> that it is clear it is either true or false (e.g.  perhaps call it
> "ahead_behind_detailed" as the current "QUICK" is merely "are they
> equal?" which corresponds to "false", and "FULL" is to show the
> detailed info), and then use the usual "-1 is unspecified, 0 and 1
> are usual bools" convention would be more appropriate.
> 

At one point[1] we talked about having an intermediate option
to try N or less commits before giving up.  The enum would let
us add that case if we wanted it.

So far, I've not heard any complaints with just having QUICK or FULL
from our Windows developers, so adding a 3rd mode hasn't been a
priority.  But the enum is here if we do decide to do so.

Jeff

[1] 
https://public-inbox.org/git/20180111093943.GC9190@sigill.intra.peff.net/
SZEDER Gábor June 29, 2019, 5:30 p.m. UTC | #3
On Tue, Jun 18, 2019 at 01:21:25PM -0700, Jeff Hostetler via GitGitGadget wrote:
> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 716283b274..febf63f28a 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -159,6 +159,19 @@ test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
>  	test_i18ncmp expect actual
>  '
>  
> +cat >expect <<\EOF
> +## b1...origin/master [different]
> +EOF
> +
> +test_expect_success 'status.aheadbehind=false status -s -b (diverged from upstream)' '
> +	(
> +		cd test &&
> +		git checkout b1 >/dev/null &&
> +		git -c status.aheadbehind=false status -s -b | head -1

These tests specifically check 'git status', but the pipe hides its
exit code.  Please use an intermediate file instead.

> +	) >actual &&

I found it odd to save the output of a whole subshell and redirect
'git checkout's stdout to /dev/null to prevent it from itnerfering
with the stdout of the subshell, instead of saving only the stdout of
the command the test focuses on.

> +	test_i18ncmp expect actual
> +'
> +
>  cat >expect <<\EOF
>  On branch b1
>  Your branch and 'origin/master' have diverged,
> @@ -174,6 +187,15 @@ test_expect_success 'status --long --branch' '
>  	test_i18ncmp expect actual
>  '
>  
> +test_expect_success 'status --long --branch' '
> +	(
> +		cd test &&
> +		git checkout b1 >/dev/null &&
> +		git -c status.aheadbehind=true status --long -b | head -3
> +	) >actual &&
> +	test_i18ncmp expect actual
> +'
> +
>  cat >expect <<\EOF
>  On branch b1
>  Your branch and 'origin/master' refer to different commits.
> @@ -188,6 +210,15 @@ test_expect_success 'status --long --branch --no-ahead-behind' '
>  	test_i18ncmp expect actual
>  '
>  
> +test_expect_success 'status.aheadbehind=false status --long --branch' '
> +	(
> +		cd test &&
> +		git checkout b1 >/dev/null &&
> +		git -c status.aheadbehind=false status --long -b | head -2
> +	) >actual &&
> +	test_i18ncmp expect actual
> +'
> +
>  cat >expect <<\EOF
>  ## b5...brokenbase [gone]
>  EOF

Patch
diff mbox series

diff --git a/Documentation/config/status.txt b/Documentation/config/status.txt
index ed72fa7dae..0fc704ab80 100644
--- a/Documentation/config/status.txt
+++ b/Documentation/config/status.txt
@@ -12,6 +12,11 @@  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.aheadBehind::
+	Set to true to enable `--ahead-behind` and false to enable
+	`--no-ahead-behind` by default in linkgit:git-status[1] for
+	non-porcelain status formats.  Defaults to true.
+
 status.displayCommentPrefix::
 	If set to true, linkgit:git-status[1] will insert a comment
 	prefix before each output line (starting with
diff --git a/builtin/commit.c b/builtin/commit.c
index 1c9e8e2228..71305073ad 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1078,9 +1078,11 @@  static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
 	enum wt_status_format status_format;
 	int show_branch;
+	enum ahead_behind_flags ahead_behind;
 } status_deferred_config = {
 	STATUS_FORMAT_UNSPECIFIED,
-	-1 /* unspecified */
+	-1, /* unspecified */
+	AHEAD_BEHIND_UNSPECIFIED,
 };
 
 static void finalize_deferred_config(struct wt_status *s)
@@ -1107,6 +1109,15 @@  static void finalize_deferred_config(struct wt_status *s)
 	if (s->show_branch < 0)
 		s->show_branch = 0;
 
+	/*
+	 * If the user did not give a "--[no]-ahead-behind" command
+	 * line argument, then we inherit the a/b config setting.
+	 * If is not set, then we inherit _FULL for backwards
+	 * compatibility.
+	 */
+	if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+		s->ahead_behind_flags = status_deferred_config.ahead_behind;
+
 	if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
 		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
@@ -1246,6 +1257,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.aheadbehind")) {
+		status_deferred_config.ahead_behind = git_config_bool(k, v);
+		return 0;
+	}
 	if (!strcmp(k, "status.showstash")) {
 		s->show_stash = git_config_bool(k, v);
 		return 0;
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 716283b274..febf63f28a 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -159,6 +159,19 @@  test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
 	test_i18ncmp expect actual
 '
 
+cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status.aheadbehind=false status -s -b (diverged from upstream)' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=false status -s -b | head -1
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
 cat >expect <<\EOF
 On branch b1
 Your branch and 'origin/master' have diverged,
@@ -174,6 +187,15 @@  test_expect_success 'status --long --branch' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'status --long --branch' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=true status --long -b | head -3
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
 cat >expect <<\EOF
 On branch b1
 Your branch and 'origin/master' refer to different commits.
@@ -188,6 +210,15 @@  test_expect_success 'status --long --branch --no-ahead-behind' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'status.aheadbehind=false status --long --branch' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=false status --long -b | head -2
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
 cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 11eccc231a..a0baf6e8b0 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -436,6 +436,10 @@  test_expect_success 'verify --[no-]ahead-behind with V2 format' '
 		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual &&
 
+		# Confirmat that "status.aheadbehind" works on V2 format.
+		git -c status.aheadbehind=false status --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
 		# Confirm --ahead-behind reports traditional branch.ab with 1/0.
 		cat >expect <<-EOF &&
 		# branch.oid $HUF