[PATCH 0/4] Add --no-ahead-behind to status

[PATCH 0/4] Add --no-ahead-behind to status

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

This patch series adds a "--no-ahead-behind" option to status
to request that it avoid a possibly expensive ahead/behind
computation for the current branch.  Instead, it just prints a
not up to date message in place of the detailed counts.

This idea was previously discussed in [1].  Working with the
enormous Windows repository, we found that 20+ seconds was being
spent in the ahead/behind computation when the current branch was
150K commits behind the upstream branch.  (Yes, this happens and
only took 3 weeks on the reporter's system.)


I've only modified "git status" in this patch series.  A similar
change could be added to "git branch -vv" and "git checkout" to
avoid delays there too.  I avoided doing it here to keep this
patch series focused.


[1] https://public-inbox.org/git/030bf57c-7a23-3391-4fc0-93efee791543@jeffhostetler.com/T/

Jeff Hostetler (4):
  status: add --no-ahead-behind to porcelain V2
  stat_tracking_info: return +1 when branch and upstream differ
  status: update short status to use --no-ahead-behind
  status: support --no-ahead-behind in long status format.

 Documentation/config.txt     |  5 +++++
 Documentation/git-status.txt | 16 ++++++++++++++++
 builtin/checkout.c           |  2 +-
 builtin/commit.c             |  6 ++++++
 ref-filter.c                 |  4 ++--
 remote.c                     | 36 ++++++++++++++++++++++++++++--------
 remote.h                     |  4 +++-
 t/t6040-tracking-info.sh     | 42 ++++++++++++++++++++++++++++++++++++++++++
 t/t7064-wtstatus-pv2.sh      | 23 +++++++++++++++++++++++
 wt-status.c                  | 24 ++++++++++++++++++++----
 wt-status.h                  |  1 +
 11 files changed, 147 insertions(+), 16 deletions(-)

-- 
2.9.3

[PATCH 1/4] status: add --no-ahead-behind to porcelain V2

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach "status --porcelain=v2 --branch" to omit "# branch.ab x y"
when "--no-ahead-behind" argument is used.

This allows the user to omit the (possibly extremely expensive)
ahead/behind computation when not needed.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/config.txt     |  5 +++++
 Documentation/git-status.txt | 10 ++++++++++
 builtin/commit.c             |  6 ++++++
 t/t7064-wtstatus-pv2.sh      | 23 +++++++++++++++++++++++
 wt-status.c                  | 11 ++++++++++-
 wt-status.h                  |  1 +
 6 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfa..9ccdf2b 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3082,6 +3082,11 @@ status.submoduleSummary::
 	submodule summary' command, which shows a similar output but does
 	not honor these settings.
 
+status.noaheadbehind::
+	Do not compute ahead/behind counts for a branch relative to its
+	upstream branch.  This can be used to avoid a possibly very
+	expensive computation on extremely large repositories.
+
 stash.showPatch::
 	If this is set to true, the `git stash show` command without an
 	option will show the stash entry in patch form.  Defaults to false.
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a..6ce8cf8 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -111,6 +111,13 @@ configuration variable documented in linkgit:git-config[1].
 	without options are equivalent to 'always' and 'never'
 	respectively.
 
+--no-ahead-behind::
+	Do not compute ahead/behind counts for the current branch relative
+	to the upstream branch.  This can be used to avoid a possibly very
+	expensive computation on extremely large repositories.
++
+	In porcelain V2 format, the 'branch.ab' line will not be present.
+
 <pathspec>...::
 	See the 'pathspec' entry in linkgit:gitglossary[7].
 
@@ -253,6 +260,9 @@ information about the current branch.
 					     the commit is present.
     ------------------------------------------------------------
 
+If `--no-ahead-behind` is given or 'status.noaheadbehind' is set, the
+'branch.ab' line will not be present.
+
 ### Changed Tracked Entries
 
 Following the headers, a series of lines are printed for tracked
diff --git a/builtin/commit.c b/builtin/commit.c
index be370f6..99ca5cb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1335,6 +1335,10 @@ static int git_status_config(const char *k, const char *v, void *cb)
 			return error(_("Invalid untracked files mode '%s'"), v);
 		return 0;
 	}
+	if (!strcmp(k, "status.noaheadbehind")) {
+		s->no_ahead_behind = git_config_bool(k, v);
+		return 0;
+	}
 	return git_diff_ui_config(k, v, NULL);
 }
 
@@ -1369,6 +1373,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		  N_("ignore changes to submodules, optional when: all, dirty, untracked. (Default: all)"),
 		  PARSE_OPT_OPTARG, NULL, (intptr_t)"all" },
 		OPT_COLUMN(0, "column", &s.colopts, N_("list untracked files in columns")),
+		OPT_BOOL(0, "no-ahead-behind", &s.no_ahead_behind,
+			 N_("omit branch ahead/behind counts")),
 		OPT_END(),
 	};
 
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index e319fa2..4be2b20 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -390,6 +390,29 @@ test_expect_success 'verify upstream fields in branch header' '
 	)
 '
 
+test_expect_success 'verify --no-ahead-behind omits branch.ab' '
+	git checkout master &&
+	test_when_finished "rm -rf sub_repo" &&
+	git clone . sub_repo &&
+	(
+		## Confirm local master tracks remote master.
+		cd sub_repo &&
+		HUF=$(git rev-parse HEAD) &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		EOF
+
+		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		git -c status.noaheadbehind=true status --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success 'create and add submodule, submodule appears clean (A. S...)' '
 	git checkout master &&
 	git clone . sub_repo &&
diff --git a/wt-status.c b/wt-status.c
index 94e5eba..1bc53e1 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1889,6 +1889,8 @@ static void wt_porcelain_print(struct wt_status *s)
  *                 <eol> ::= NUL when -z,
  *                           LF when NOT -z.
  *
+ * When 'status.noaheadbehind' is true or '--no-ahead-behind'
+ * is given on the command line, the 'branch.ab' line is omitted.
  */
 static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 {
@@ -1928,7 +1930,14 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 		/* Lookup stats on the upstream tracking branch, if set. */
 		branch = branch_get(branch_name);
 		base = NULL;
-		ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind, &base) == 0);
+
+		if (s->no_ahead_behind) {
+			base = branch_get_upstream(branch, NULL);
+			ab_info = 0;
+		} else {
+			ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind, &base) == 0);
+		}
+
 		if (base) {
 			base = shorten_unambiguous_ref(base, 0);
 			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
diff --git a/wt-status.h b/wt-status.h
index 64f4d33..8708977 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -80,6 +80,7 @@ struct wt_status {
 	int show_branch;
 	int show_stash;
 	int hints;
+	int no_ahead_behind;
 
 	enum wt_status_format status_format;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
-- 
2.9.3

Re: [PATCH 1/4] status: add --no-ahead-behind to porcelain V2

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:42PM +0000, Jeff Hostetler wrote:

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 9f3a78a..6ce8cf8 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -111,6 +111,13 @@ configuration variable documented in linkgit:git-config[1].
>  	without options are equivalent to 'always' and 'never'
>  	respectively.
>  
> +--no-ahead-behind::
> +	Do not compute ahead/behind counts for the current branch relative
> +	to the upstream branch.  This can be used to avoid a possibly very
> +	expensive computation on extremely large repositories.
> ++
> +	In porcelain V2 format, the 'branch.ab' line will not be present.
> +

This second paragraph after the "+" continuation shouldn't be indented
(sadly it makes the source much uglier, but it's one of the vagaries of
asciidoc).

-Peff

Re: [PATCH 1/4] status: add --no-ahead-behind to porcelain V2

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:42PM +0000, Jeff Hostetler wrote:

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9593bfa..9ccdf2b 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -3082,6 +3082,11 @@ status.submoduleSummary::
>  	submodule summary' command, which shows a similar output but does
>  	not honor these settings.
>  
> +status.noaheadbehind::
> +	Do not compute ahead/behind counts for a branch relative to its
> +	upstream branch.  This can be used to avoid a possibly very
> +	expensive computation on extremely large repositories.

I got tripped up by double-negatives here while testing something out
with your series. Could this be "status.aheadbehind", and default to
true?

-Peff

Re: [PATCH 1/4] status: add --no-ahead-behind to porcelain V2

[Jeff Hostetler]

On 12/20/2017 11:33 AM, Jeff King wrote:
> On Wed, Dec 20, 2017 at 02:42:42PM +0000, Jeff Hostetler wrote:
> 
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9593bfa..9ccdf2b 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -3082,6 +3082,11 @@ status.submoduleSummary::
>>   	submodule summary' command, which shows a similar output but does
>>   	not honor these settings.
>>   
>> +status.noaheadbehind::
>> +	Do not compute ahead/behind counts for a branch relative to its
>> +	upstream branch.  This can be used to avoid a possibly very
>> +	expensive computation on extremely large repositories.
> 
> I got tripped up by double-negatives here while testing something out
> with your series. Could this be "status.aheadbehind", and default to
> true?
> 
> -Peff
> 

Yeah, we debated internally how to name it and that was the least bad.
I'll change it to the positive and (based on later comments) move it
to "core." so that we can pick up "branch -vv" in this round.

Thanks
Jeff

[PATCH 2/4] stat_tracking_info: return +1 when branch and upstream differ

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Extend stat_tracking_info() to return 1 when the branch is
not up to date with its upstream branch and only return 0
when they are equal.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 ref-filter.c | 4 ++--
 remote.c     | 6 ++++--
 wt-status.c  | 2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e728b15..10ab4cd 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1239,7 +1239,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 		*s = show_ref(&atom->u.remote_ref.refname, refname);
 	else if (atom->u.remote_ref.option == RR_TRACK) {
 		if (stat_tracking_info(branch, &num_ours,
-				       &num_theirs, NULL)) {
+				       &num_theirs, NULL) < 0) {
 			*s = xstrdup(msgs.gone);
 		} else if (!num_ours && !num_theirs)
 			*s = "";
@@ -1257,7 +1257,7 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 		}
 	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
 		if (stat_tracking_info(branch, &num_ours,
-				       &num_theirs, NULL))
+				       &num_theirs, NULL) < 0)
 			return;
 
 		if (!num_ours && !num_theirs)
diff --git a/remote.c b/remote.c
index b220f0d..a38b42e 100644
--- a/remote.c
+++ b/remote.c
@@ -1983,7 +1983,9 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
  * is not itself NULL.
  *
  * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
- * upstream defined, or ref does not exist), 0 otherwise.
+ * upstream defined, or ref does not exist).
+ * Returns 0 if the commits are the same (the branch is up to date).
+ * Returns 1 if the commits are different (the branch is not up to date).
  */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 		       const char **upstream_name)
@@ -2051,7 +2053,7 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 	clear_commit_marks(theirs, ALL_REV_FLAGS);
 
 	argv_array_clear(&argv);
-	return 0;
+	return 1;
 }
 
 /*
diff --git a/wt-status.c b/wt-status.c
index 1bc53e1..471ba15 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1935,7 +1935,7 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 			base = branch_get_upstream(branch, NULL);
 			ab_info = 0;
 		} else {
-			ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind, &base) == 0);
+			ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind, &base) >= 0);
 		}
 
 		if (base) {
-- 
2.9.3

Re: [PATCH 2/4] stat_tracking_info: return +1 when branch and upstream differ

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:43PM +0000, Jeff Hostetler wrote:

> Extend stat_tracking_info() to return 1 when the branch is
> not up to date with its upstream branch and only return 0
> when they are equal.

This means that callers all need to be updated, but there's no change
that the compiler could catch. You've updated all of the calls here, but
any topics in flight would need to be fixed, too.

I don't see any any in pu, but there are a number of long-running forks
hanging around these days.

Is it worth introducing a small change so that any other callers which
get merged in force a human to look at them? I'm wondering if we could
just re-order the "upstream_name" argument or something.

> --- a/remote.c
> +++ b/remote.c
> @@ -1983,7 +1983,9 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
>   * is not itself NULL.
>   *
>   * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
> - * upstream defined, or ref does not exist), 0 otherwise.
> + * upstream defined, or ref does not exist).
> + * Returns 0 if the commits are the same (the branch is up to date).
> + * Returns 1 if the commits are different (the branch is not up to date).

Slightly pedantic, but we'd return 1 here also if the branch is ahead of
its upstream, right?

-Peff

Re: [PATCH 2/4] stat_tracking_info: return +1 when branch and upstream differ

[Jeff King]

On Wed, Dec 20, 2017 at 11:14:07AM -0500, Jeff King wrote:

> On Wed, Dec 20, 2017 at 02:42:43PM +0000, Jeff Hostetler wrote:
> 
> > Extend stat_tracking_info() to return 1 when the branch is
> > not up to date with its upstream branch and only return 0
> > when they are equal.
> 
> This means that callers all need to be updated, but there's no change
> that the compiler could catch. You've updated all of the calls here, but
> any topics in flight would need to be fixed, too.
> 
> I don't see any any in pu, but there are a number of long-running forks
> hanging around these days.
> 
> Is it worth introducing a small change so that any other callers which
> get merged in force a human to look at them? I'm wondering if we could
> just re-order the "upstream_name" argument or something.

Having seen the change in the next patch, I wonder if we should add a
flag field to specify "don't bother doing extra work" rather than
passing NULL for the ours/theirs parameters. I.e., most callers would
become:

  if (stat_tracking_info(branch, &ours, &theirs, &base, 0) >= 0)

and the ones you touch later in the series would become:

  
  if (stat_tracking_info(branch, NULL, NULL, &base, TRACKING_QUICK) >= 0)

or similar. And then any newly added calls would get flagged by the
compiler as missing the final parameter.

-Peff

Re: [PATCH 2/4] stat_tracking_info: return +1 when branch and upstream differ

[Jeff Hostetler]

On 12/20/2017 11:14 AM, Jeff King wrote:
> On Wed, Dec 20, 2017 at 02:42:43PM +0000, Jeff Hostetler wrote:
> 
>> Extend stat_tracking_info() to return 1 when the branch is
>> not up to date with its upstream branch and only return 0
>> when they are equal.
> 
> This means that callers all need to be updated, but there's no change
> that the compiler could catch. You've updated all of the calls here, but
> any topics in flight would need to be fixed, too.
> 
> I don't see any any in pu, but there are a number of long-running forks
> hanging around these days.
> 
> Is it worth introducing a small change so that any other callers which
> get merged in force a human to look at them? I'm wondering if we could
> just re-order the "upstream_name" argument or something.

Good point.  I usually try to avoid signature changes to minimize the
disruption, but as you say that can be a good thing too.  I'll add an
argument rather than using the null field trick.

> 
>> --- a/remote.c
>> +++ b/remote.c
>> @@ -1983,7 +1983,9 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
>>    * is not itself NULL.
>>    *
>>    * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
>> - * upstream defined, or ref does not exist), 0 otherwise.
>> + * upstream defined, or ref does not exist).
>> + * Returns 0 if the commits are the same (the branch is up to date).
>> + * Returns 1 if the commits are different (the branch is not up to date).
> 
> Slightly pedantic, but we'd return 1 here also if the branch is ahead of
> its upstream, right?

Yeah, poor word choice on my part.  I'll fix.

Thanks
Jeff

[PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach "git status --short --branch" to use "--no-ahead-behind"
flag to skip computing ahead/behind counts for the branch and
its upstream and just report '[different]'.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-status.txt |  3 +++
 remote.c                     | 12 +++++++++---
 t/t6040-tracking-info.sh     | 13 +++++++++++++
 wt-status.c                  | 11 +++++++++--
 4 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 6ce8cf8..ea029ad 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -117,6 +117,9 @@ configuration variable documented in linkgit:git-config[1].
 	expensive computation on extremely large repositories.
 +
 	In porcelain V2 format, the 'branch.ab' line will not be present.
++
+	In short format with --branch, '[different]' will printed rather
+	than detailed ahead/behind counts.
 
 <pathspec>...::
 	See the 'pathspec' entry in linkgit:gitglossary[7].
diff --git a/remote.c b/remote.c
index a38b42e..0a63ac1 100644
--- a/remote.c
+++ b/remote.c
@@ -1978,9 +1978,12 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
 
 /*
  * Compare a branch with its upstream, and save their differences (number
- * of commits) in *num_ours and *num_theirs. The name of the upstream branch
- * (or NULL if no upstream is defined) is returned via *upstream_name, if it
- * is not itself NULL.
+ * of commits) in *num_ours and *num_theirs.  If either num_ours or num_theirs
+ * are NULL, we skip counting the commits and just return whether they are
+ * different.
+ *
+ * The name of the upstream branch (or NULL if no upstream is defined) is
+ * returned via *upstream_name, if it is not itself NULL.
  *
  * Returns -1 if num_ours and num_theirs could not be filled in (e.g., no
  * upstream defined, or ref does not exist).
@@ -2016,6 +2019,9 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 	if (!ours)
 		return -1;
 
+	if (!num_ours || !num_theirs)
+		return theirs != ours;
+
 	/* are we the same? */
 	if (theirs == ours) {
 		*num_theirs = *num_ours = 0;
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 8f17fd9..0190220 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -147,6 +147,19 @@ test_expect_success 'status -s -b (diverged from upstream)' '
 '
 
 cat >expect <<\EOF
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git status -s -b --no-ahead-behind | head -1
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index 471ba15..6b4f969 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1767,6 +1767,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	const char *branch_name;
 	int num_ours, num_theirs;
 	int upstream_is_gone = 0;
+	int sti;
 
 	color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## ");
 
@@ -1791,7 +1792,11 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 
 	color_fprintf(s->fp, branch_color_local, "%s", branch_name);
 
-	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base) < 0) {
+	if (s->no_ahead_behind)
+		sti = stat_tracking_info(branch, NULL, NULL, &base);
+	else
+		sti = stat_tracking_info(branch, &num_ours, &num_theirs, &base);
+	if (sti < 0) {
 		if (!base)
 			goto conclude;
 
@@ -1803,12 +1808,14 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	color_fprintf(s->fp, branch_color_remote, "%s", short_base);
 	free(short_base);
 
-	if (!upstream_is_gone && !num_ours && !num_theirs)
+	if (!upstream_is_gone && !sti)
 		goto conclude;
 
 	color_fprintf(s->fp, header_color, " [");
 	if (upstream_is_gone) {
 		color_fprintf(s->fp, header_color, LABEL(N_("gone")));
+	} else if (s->no_ahead_behind) {
+		color_fprintf(s->fp, header_color, LABEL(N_("different")));
 	} else if (!num_ours) {
 		color_fprintf(s->fp, header_color, LABEL(N_("behind ")));
 		color_fprintf(s->fp, branch_color_remote, "%d", num_theirs);
-- 
2.9.3

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:44PM +0000, Jeff Hostetler wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> Teach "git status --short --branch" to use "--no-ahead-behind"
> flag to skip computing ahead/behind counts for the branch and
> its upstream and just report '[different]'.

How come "--short" and "--long" get this smaller bit of data, but
"--porcelain=v2" just omits the line entirely?

I don't have a real preference for or against the "[different]" message
myself, but if we can get the information cheaply, it seems odd not to
provide it in all cases.

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index 6ce8cf8..ea029ad 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -117,6 +117,9 @@ configuration variable documented in linkgit:git-config[1].
>  	expensive computation on extremely large repositories.
>  +
>  	In porcelain V2 format, the 'branch.ab' line will not be present.
> ++
> +	In short format with --branch, '[different]' will printed rather
> +	than detailed ahead/behind counts.

s/will/will be/ ?

> diff --git a/remote.c b/remote.c
> index a38b42e..0a63ac1 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -1978,9 +1978,12 @@ int ref_newer(const struct object_id *new_oid, const struct object_id *old_oid)
>  
>  /*
>   * Compare a branch with its upstream, and save their differences (number
> - * of commits) in *num_ours and *num_theirs. The name of the upstream branch
> - * (or NULL if no upstream is defined) is returned via *upstream_name, if it
> - * is not itself NULL.
> + * of commits) in *num_ours and *num_theirs.  If either num_ours or num_theirs
> + * are NULL, we skip counting the commits and just return whether they are
> + * different.

OK, this makes sense. I wondered in the last one why the caller could
not simply check "num_ours != num_theirs" themselves. And this is why:
we want to be able to signal to stat_tracking_info() that we want the
"cheap" version.

> diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
> index 8f17fd9..0190220 100755
> --- a/t/t6040-tracking-info.sh
> +++ b/t/t6040-tracking-info.sh
> @@ -147,6 +147,19 @@ test_expect_success 'status -s -b (diverged from upstream)' '
>  '
>  
>  cat >expect <<\EOF
> +## b1...origin/master [different]
> +EOF
> +
> +test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '

This patch will affect "git status --porcelain", too. That's not
supposed to change in incompatible ways. I guess it's up for debate
whether callers are meant to handle any arbitrary string inside the []
(we already show "[gone]" for some cases), since AFAICT the format of
the tracking info is left completely vague in the documentation.

(I'd also hope that everybody is using --porcelain=v2 if they can, but
we should still avoid breaking v1).

-Peff

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

On 12/20/2017 11:26 AM, Jeff King wrote:
> On Wed, Dec 20, 2017 at 02:42:44PM +0000, Jeff Hostetler wrote:
> 
>> From: Jeff Hostetler <jeffhost@microsoft.com>
>>
>> Teach "git status --short --branch" to use "--no-ahead-behind"
>> flag to skip computing ahead/behind counts for the branch and
>> its upstream and just report '[different]'.
> 
> How come "--short" and "--long" get this smaller bit of data, but
> "--porcelain=v2" just omits the line entirely?
> 
> I don't have a real preference for or against the "[different]" message
> myself, but if we can get the information cheaply, it seems odd not to
> provide it in all cases.

I was only thinking of VS usage.  But you're right, I can include an
alternate line with eq|neq since we already have the data on hand.


[...]
>> +test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
> 
> This patch will affect "git status --porcelain", too. That's not
> supposed to change in incompatible ways. I guess it's up for debate
> whether callers are meant to handle any arbitrary string inside the []
> (we already show "[gone]" for some cases), since AFAICT the format of
> the tracking info is left completely vague in the documentation.
> 
> (I'd also hope that everybody is using --porcelain=v2 if they can, but
> we should still avoid breaking v1).

I hadn't intended to alter V1 output.  I'll disable the new feature
when V1 is selected.

Thanks
Jeff

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff King]

On Thu, Dec 21, 2017 at 09:18:17AM -0500, Jeff Hostetler wrote:

> > This patch will affect "git status --porcelain", too. That's not
> > supposed to change in incompatible ways. I guess it's up for debate
> > whether callers are meant to handle any arbitrary string inside the []
> > (we already show "[gone]" for some cases), since AFAICT the format of
> > the tracking info is left completely vague in the documentation.
> > 
> > (I'd also hope that everybody is using --porcelain=v2 if they can, but
> > we should still avoid breaking v1).
> 
> I hadn't intended to alter V1 output.  I'll disable the new feature
> when V1 is selected.

To be clear, I am on the fence regarding the "is it a breaking change"
line. Certainly if the caller says "--no-ahead-behind", I don't see any
harm in doing what they asked.

But one further complication is that this may be triggered by config.
And that goes for --porcelain=v2, as well. Even though the v2
documentation specifically says "ignore headers you don't recognize",
would any callers be confused if a header is omitted due to a user's
config?

I guess for "branch.ab", the answer is "probably not", since it is
already documented to appear only if certain conditions are met. So
probably "omit branch.ab" is an OK change, as is "add a new header".
But I just wonder if it would be simpler to ignore the config entirely
for porcelain outputs (and require the explicit command-line option).

Personally, I am not a purist when it comes to config and plumbing, and
I'd be fine with having the config impact v2 (it is a hint from the user
that they do not want to spend time on the computation, so having
scripts respect that would be what the user wants). If you really have a
script that is unhappy missing "branch.ab", then you can either choose
not to set that config, or you can fix the script to use "--ahead-behind"
to override the config. But I'm not sure everybody in the community
would necessarily agree with me.

-Peff

Re: [PATCH 3/4] status: update short status to use --no-ahead-behind

[Jeff Hostetler]

On 12/21/2017 10:39 AM, Jeff King wrote:
> On Thu, Dec 21, 2017 at 09:18:17AM -0500, Jeff Hostetler wrote:
> 
>>> This patch will affect "git status --porcelain", too. That's not
>>> supposed to change in incompatible ways. I guess it's up for debate
>>> whether callers are meant to handle any arbitrary string inside the []
>>> (we already show "[gone]" for some cases), since AFAICT the format of
>>> the tracking info is left completely vague in the documentation.
>>>
>>> (I'd also hope that everybody is using --porcelain=v2 if they can, but
>>> we should still avoid breaking v1).
>>
>> I hadn't intended to alter V1 output.  I'll disable the new feature
>> when V1 is selected.
> 
> To be clear, I am on the fence regarding the "is it a breaking change"
> line. Certainly if the caller says "--no-ahead-behind", I don't see any
> harm in doing what they asked.
> 
> But one further complication is that this may be triggered by config.
> And that goes for --porcelain=v2, as well. Even though the v2
> documentation specifically says "ignore headers you don't recognize",
> would any callers be confused if a header is omitted due to a user's
> config?
> 
> I guess for "branch.ab", the answer is "probably not", since it is
> already documented to appear only if certain conditions are met. So
> probably "omit branch.ab" is an OK change, as is "add a new header".
> But I just wonder if it would be simpler to ignore the config entirely
> for porcelain outputs (and require the explicit command-line option).

I like that actually.  Have the porcelain versions only use the
command line option and do what is asked.  And let the short/long
forms use both the command line option and the config setting.
That makes it explicit for scripts that parse the output.

Thanks!

> 
> Personally, I am not a purist when it comes to config and plumbing, and
> I'd be fine with having the config impact v2 (it is a hint from the user
> that they do not want to spend time on the computation, so having
> scripts respect that would be what the user wants). If you really have a
> script that is unhappy missing "branch.ab", then you can either choose
> not to set that config, or you can fix the script to use "--ahead-behind"
> to override the config. But I'm not sure everybody in the community
> would necessarily agree with me.
> 
> -Peff
> 

[PATCH 4/4] status: support --no-ahead-behind in long status format.

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach long (normal) status format to respect the --no-ahead-behind
argument and skip the possibly expensive ahead/behind computation
when printing the branch tracking information.

When --no-ahead-behind is given or status.noaheadbehind is true,
status prints "Your branch is out of date with '<upstream>'."
instead of the various ahead/behind messages.

TODO Should we have an advice hint for this case?

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-status.txt |  3 +++
 builtin/checkout.c           |  2 +-
 remote.c                     | 18 +++++++++++++++---
 remote.h                     |  4 +++-
 t/t6040-tracking-info.sh     | 29 +++++++++++++++++++++++++++++
 wt-status.c                  |  2 +-
 6 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index ea029ad..9a2f209 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -120,6 +120,9 @@ configuration variable documented in linkgit:git-config[1].
 +
 	In short format with --branch, '[different]' will printed rather
 	than detailed ahead/behind counts.
++
+	In long (normal) format, a simple out of date message will be
+	printed rather than detailed ahead/behind counts.
 
 <pathspec>...::
 	See the 'pathspec' entry in linkgit:gitglossary[7].
diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd..a3e7bde 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -605,7 +605,7 @@ static void report_tracking(struct branch_info *new)
 	struct strbuf sb = STRBUF_INIT;
 	struct branch *branch = branch_get(new->name);
 
-	if (!format_tracking_info(branch, &sb))
+	if (!format_tracking_info(branch, 0, &sb))
 		return;
 	fputs(sb.buf, stdout);
 	strbuf_release(&sb);
diff --git a/remote.c b/remote.c
index 0a63ac1..b75e62f 100644
--- a/remote.c
+++ b/remote.c
@@ -2065,14 +2065,20 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 /*
  * Return true when there is anything to report, otherwise false.
  */
-int format_tracking_info(struct branch *branch, struct strbuf *sb)
+int format_tracking_info(struct branch *branch, int no_ahead_behind,
+			 struct strbuf *sb)
 {
 	int ours, theirs;
 	const char *full_base;
 	char *base;
 	int upstream_is_gone = 0;
+	int sti;
 
-	if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {
+	if (no_ahead_behind)
+		sti = stat_tracking_info(branch, NULL, NULL, &full_base);
+	else
+		sti = stat_tracking_info(branch, &ours, &theirs, &full_base);
+	if (sti < 0) {
 		if (!full_base)
 			return 0;
 		upstream_is_gone = 1;
@@ -2086,10 +2092,16 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 		if (advice_status_hints)
 			strbuf_addstr(sb,
 				_("  (use \"git branch --unset-upstream\" to fixup)\n"));
-	} else if (!ours && !theirs) {
+	} else if (!sti) {
 		strbuf_addf(sb,
 			_("Your branch is up to date with '%s'.\n"),
 			base);
+	} else if (no_ahead_behind) {
+		strbuf_addf(sb, _("Your branch is out of date with '%s'.\n"),
+			    base);
+
+		/* TODO Do we need a generic hint here? */
+
 	} else if (!theirs) {
 		strbuf_addf(sb,
 			Q_("Your branch is ahead of '%s' by %d commit.\n",
diff --git a/remote.h b/remote.h
index 2ecf4c8..559649d 100644
--- a/remote.h
+++ b/remote.h
@@ -258,7 +258,9 @@ enum match_refs_flags {
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 		       const char **upstream_name);
-int format_tracking_info(struct branch *branch, struct strbuf *sb);
+
+int format_tracking_info(struct branch *branch, int no_ahead_behind,
+			 struct strbuf *sb);
 
 struct ref *get_local_heads(void);
 /*
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 0190220..00fbd0a 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -160,6 +160,35 @@ test_expect_success 'status -s -b --no-ahead-behind (diverged from upstream)' '
 '
 
 cat >expect <<\EOF
+On branch b1
+Your branch and 'origin/master' have diverged,
+and have 1 and 1 different commits each, respectively.
+EOF
+
+test_expect_success 'status --long --branch' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git status --long -b | head -3
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
+On branch b1
+Your branch is out of date with 'origin/master'.
+EOF
+
+test_expect_success 'status --long --branch --no-ahead-behind' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git status --long -b --no-ahead-behind | head -2
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index 6b4f969..1e7cd57 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1005,7 +1005,7 @@ static void wt_longstatus_print_tracking(struct wt_status *s)
 	if (!skip_prefix(s->branch, "refs/heads/", &branch_name))
 		return;
 	branch = branch_get(branch_name);
-	if (!format_tracking_info(branch, &sb))
+	if (!format_tracking_info(branch, s->no_ahead_behind, &sb))
 		return;
 
 	i = 0;
-- 
2.9.3

Re: [PATCH 4/4] status: support --no-ahead-behind in long status format.

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:45PM +0000, Jeff Hostetler wrote:

> diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
> index ea029ad..9a2f209 100644
> --- a/Documentation/git-status.txt
> +++ b/Documentation/git-status.txt
> @@ -120,6 +120,9 @@ configuration variable documented in linkgit:git-config[1].
>  +
>  	In short format with --branch, '[different]' will printed rather
>  	than detailed ahead/behind counts.
> ++
> +	In long (normal) format, a simple out of date message will be
> +	printed rather than detailed ahead/behind counts.

Same asciidoc trickery here as in the first patch (and in the --short
one, too, but I forgot to mention it).

> +	} else if (no_ahead_behind) {
> +		strbuf_addf(sb, _("Your branch is out of date with '%s'.\n"),
> +			    base);
> +
> +		/* TODO Do we need a generic hint here? */
> +

I'm not sure what we'd advise here. I'd consider:

  git log --oneline --graph HEAD..@{upstream}

to see the actual differences, but that's a bit verbose. Is there an
easy way to re-enable it? I guess repeating the command with
"--ahead-behind" should do so.

-Peff

Re: [PATCH 0/4] Add --no-ahead-behind to status

[Jeff King]

On Wed, Dec 20, 2017 at 02:42:41PM +0000, Jeff Hostetler wrote:

> From: Jeff Hostetler <jeffhost@microsoft.com>
> 
> This patch series adds a "--no-ahead-behind" option to status
> to request that it avoid a possibly expensive ahead/behind
> computation for the current branch.  Instead, it just prints a
> not up to date message in place of the detailed counts.
> 
> This idea was previously discussed in [1].  Working with the
> enormous Windows repository, we found that 20+ seconds was being
> spent in the ahead/behind computation when the current branch was
> 150K commits behind the upstream branch.  (Yes, this happens and
> only took 3 weeks on the reporter's system.)

Overall this looks cleanly done. I raised a few minor points in the
individual patches, but certainly nothing that would be a show-stopper
for the general idea.

> I've only modified "git status" in this patch series.  A similar
> change could be added to "git branch -vv" and "git checkout" to
> avoid delays there too.  I avoided doing it here to keep this
> patch series focused.

I have a feeling that the same user who got annoyed by "git status" is
going to get annoyed by "git checkout" sooner or later. I'm OK with
handling the other commands separately, but I suspect we may want at
least "git checkout" support in the near future.

There is one thing that it's worth thinking about (because it will be
hard to take back later): config option naming. If your repository is so
large that ahead/behind checks are annoying, would you want to be able
to set a single config to tell Git that, rather than one for each
command? If so, then do we want to ditch "status.aheadbehind" in favor
of a more unified name?

-Peff