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

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

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

This is version 2 of my patch series to avoid expensive
ahead/behind calculations in status.  This version addresses
Peff's comments on V1.

This version renames the command line parameter to have
positive sense "--ahead-behind" and avoids confusing double
negatives throughout.  It also changes the config setting
from "status." to "core." in anticipation of use by other
commands (like branch and checkout).

The output for porcelain status formats does change, but
ONLY if the "--no-ahead-behind" option is given on the
command line; porcelain formats DO NOT inherit the config
setting.  The intent here is that only scripts explicitly
requesting the new feature will see any format changes.

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 (5):
  core.aheadbehind: add new config setting
  stat_tracking_info: return +1 when branches are not equal
  status: add --[no-]ahead-behind to porcelain V2 output
  status: update short status to use --no-ahead-behind
  status: support --no-ahead-behind in long format

 Documentation/config.txt     |  8 ++++++
 Documentation/git-status.txt | 11 ++++++--
 builtin/checkout.c           |  2 +-
 builtin/commit.c             | 19 ++++++++++++++
 cache.h                      |  1 +
 config.c                     |  5 ++++
 environment.c                |  1 +
 ref-filter.c                 |  4 +--
 remote.c                     | 38 ++++++++++++++++++++--------
 remote.h                     | 10 ++++++--
 t/t6040-tracking-info.sh     | 42 +++++++++++++++++++++++++++++++
 t/t7064-wtstatus-pv2.sh      | 60 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 34 +++++++++++++++++++------
 wt-status.h                  |  2 ++
 14 files changed, 212 insertions(+), 25 deletions(-)

-- 
2.9.3

[PATCH v2 1/5] core.aheadbehind: add new config setting

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Created core.aheadbehind config setting and core_ahead_behind
global variable.  This value defaults to true.

This value will be used in the next few commits as the default value
for the --ahead-behind parameter.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/config.txt | 8 ++++++++
 cache.h                  | 1 +
 config.c                 | 5 +++++
 environment.c            | 1 +
 4 files changed, 15 insertions(+)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfa..c78d6be 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -895,6 +895,14 @@ core.abbrev::
 	abbreviated object names to stay unique for some time.
 	The minimum length is 4.
 
+core.aheadbehind::
+	If true, tells commands like status and branch to print ahead and
+	behind counts for the branch relative to its upstream branch.
+	This computation may be very expensive when there is a great
+	distance between the two branches.  If false, these commands
+	only print that the two branches refer to different commits.
+	Defaults to true.
+
 add.ignoreErrors::
 add.ignore-errors (deprecated)::
 	Tells 'git add' to continue adding files when some files cannot be
diff --git a/cache.h b/cache.h
index 6440e2b..5757d8f 100644
--- a/cache.h
+++ b/cache.h
@@ -735,6 +735,7 @@ extern int assume_unchanged;
 extern int prefer_symlink_refs;
 extern int warn_ambiguous_refs;
 extern int warn_on_object_refname_ambiguity;
+extern int core_ahead_behind;
 extern const char *apply_default_whitespace;
 extern const char *apply_default_ignorewhitespace;
 extern const char *git_attributes_file;
diff --git a/config.c b/config.c
index c38401a..6a4b49c 100644
--- a/config.c
+++ b/config.c
@@ -1241,6 +1241,11 @@ static int git_default_core_config(const char *var, const char *value)
 		return 0;
 	}
 
+	if (!strcmp(var, "core.aheadbehind")) {
+		core_ahead_behind = git_config_bool(var, value);
+		return 0;
+	}
+
 	/* Add other config variables here and to Documentation/config.txt. */
 	return 0;
 }
diff --git a/environment.c b/environment.c
index 8289c25..5822c15 100644
--- a/environment.c
+++ b/environment.c
@@ -25,6 +25,7 @@ int prefer_symlink_refs;
 int is_bare_repository_cfg = -1; /* unspecified */
 int warn_ambiguous_refs = 1;
 int warn_on_object_refname_ambiguity = 1;
+int core_ahead_behind = 1;
 int ref_paranoia = -1;
 int repository_format_precious_objects;
 const char *git_commit_encoding;
-- 
2.9.3

[PATCH v2 2/5] stat_tracking_info: return +1 when branches are not equal

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Extend stat_tracking_info() return +1 when the branches are not equal
and to take a new "enum ahead_behind_flag" ABF_QUICK to avoid the
expensive ahead/behind computation when requested.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 ref-filter.c |  4 ++--
 remote.c     | 24 ++++++++++++++++--------
 remote.h     |  7 ++++++-
 wt-status.c  |  9 +++++----
 4 files changed, 29 insertions(+), 15 deletions(-)

diff --git a/ref-filter.c b/ref-filter.c
index e728b15..6191cb4 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, ABF_FULL) < 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, ABF_FULL) < 0)
 			return;
 
 		if (!num_ours && !num_theirs)
diff --git a/remote.c b/remote.c
index b220f0d..91b5afb 100644
--- a/remote.c
+++ b/remote.c
@@ -1977,16 +1977,22 @@ 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.
+ * Compare a branch with its upstream and report on their differences.
+ * If abf is ABF_FULL, save their differences (number of commits) in
+ * *num_ours and *num_theirs.
+ * If abf is ABF_QUICK, skip the (possibly expensive) ahead/behind
+ * computation (and leave *num_ours and *num_theirs undefined).
+ *
+ * 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), 0 otherwise.
+ * upstream defined, or ref does not exist).
+ * Returns 0 if the commits are the same.
+ * Returns 1 if the commits are different (ahead, behind, or both).
  */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-		       const char **upstream_name)
+		       const char **upstream_name, enum ahead_behind_flags abf)
 {
 	struct object_id oid;
 	struct commit *ours, *theirs;
@@ -2019,6 +2025,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 		*num_theirs = *num_ours = 0;
 		return 0;
 	}
+	if (abf == ABF_QUICK)
+		return 1;
 
 	/* Run "rev-list --left-right ours...theirs" internally... */
 	argv_array_push(&argv, ""); /* ignored */
@@ -2051,7 +2059,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;
 }
 
 /*
@@ -2064,7 +2072,7 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
 	char *base;
 	int upstream_is_gone = 0;
 
-	if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {
+	if (stat_tracking_info(branch, &ours, &theirs, &full_base, ABF_FULL) < 0) {
 		if (!full_base)
 			return 0;
 		upstream_is_gone = 1;
diff --git a/remote.h b/remote.h
index 2ecf4c8..1e12dfb 100644
--- a/remote.h
+++ b/remote.h
@@ -255,9 +255,14 @@ enum match_refs_flags {
 	MATCH_REFS_FOLLOW_TAGS	= (1 << 3)
 };
 
+enum ahead_behind_flags {
+	ABF_QUICK = 0,  /* just eq/neq reporting */
+	ABF_FULL  = 1,  /* traditional ahead/behind reporting */
+};
+
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
-		       const char **upstream_name);
+		       const char **upstream_name, enum ahead_behind_flags abf);
 int format_tracking_info(struct branch *branch, struct strbuf *sb);
 
 struct ref *get_local_heads(void);
diff --git a/wt-status.c b/wt-status.c
index 94e5eba..80c23ba 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1791,7 +1791,7 @@ 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 (stat_tracking_info(branch, &num_ours, &num_theirs, &base, ABF_FULL) < 0) {
 		if (!base)
 			goto conclude;
 
@@ -1896,7 +1896,7 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 	const char *base;
 	const char *branch_name;
 	struct wt_status_state state;
-	int ab_info, nr_ahead, nr_behind;
+	int have_ab_info, nr_ahead, nr_behind;
 	char eol = s->null_termination ? '\0' : '\n';
 
 	memset(&state, 0, sizeof(state));
@@ -1928,13 +1928,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);
+		have_ab_info = (stat_tracking_info(branch, &nr_ahead,
+						   &nr_behind, &base, ABF_FULL) >= 0);
 		if (base) {
 			base = shorten_unambiguous_ref(base, 0);
 			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
 			free((char *)base);
 
-			if (ab_info)
+			if (have_ab_info)
 				fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
 		}
 	}
-- 
2.9.3

[PATCH v2 3/5] status: add --[no-]ahead-behind to porcelain V2 output

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach "status --porcelain=v2 --branch" to omit detailed ahead/behind
information when "--no-ahead-behind" argument is used.

When "--no-ahead-behind" is given, the existing "branch.ab x y" line
is replaced with a new "branch.qab eq|neq" line.

This allows the user to omit the (possibly extremely expensive)
ahead/behind computation when not wanted.  In its place, a single
equal/not-equal line is reported.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/git-status.txt | 11 ++++++--
 builtin/commit.c             | 19 ++++++++++++++
 t/t7064-wtstatus-pv2.sh      | 60 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 24 ++++++++++++++----
 wt-status.h                  |  2 ++
 5 files changed, 109 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a..d0e5f89 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -111,6 +111,12 @@ configuration variable documented in linkgit:git-config[1].
 	without options are equivalent to 'always' and 'never'
 	respectively.
 
+--ahead-behind::
+--no-ahead-behind::
+	Display or do not display detailed ahead/behind counts for the branch
+	relative to its upstream branch.  Defaults to true.  Overrides the
+	value of "core.aheadbehind".
+
 <pathspec>...::
 	See the 'pathspec' entry in linkgit:gitglossary[7].
 
@@ -242,7 +248,8 @@ don't recognize.
 ### Branch Headers
 
 If `--branch` is given, a series of header lines are printed with
-information about the current branch.
+information about the current branch.  If `--no-ahead-behind` is given,
+the branch.ab line is replaced with the branch.qab line.
 
     Line                                     Notes
     ------------------------------------------------------------
@@ -250,7 +257,7 @@ information about the current branch.
     # branch.head <branch> | (detached)      Current branch.
     # branch.upstream <upstream_branch>      If upstream is set.
     # branch.ab +<ahead> -<behind>           If upstream is set and
-					     the commit is present.
+    # branch.qab eq | neq                    the commit is present.
     ------------------------------------------------------------
 
 ### Changed Tracked Entries
diff --git a/builtin/commit.c b/builtin/commit.c
index be370f6..d6e2717 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -141,6 +141,7 @@ static int sequencer_in_use;
 static int use_editor = 1, include_status = 1;
 static int show_ignored_in_status, have_option_m;
 static struct strbuf message = STRBUF_INIT;
+static int ahead_behind_opt = -1;
 
 static enum wt_status_format status_format = STATUS_FORMAT_UNSPECIFIED;
 
@@ -1369,6 +1370,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, "ahead-behind", &ahead_behind_opt,
+			 N_("compute branch ahead/behind values")),
 		OPT_END(),
 	};
 
@@ -1389,6 +1392,21 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 		       PATHSPEC_PREFER_FULL,
 		       prefix, argv);
 
+	/*
+	 * Porcelain formats only look at the --[no-]ahead-behind command
+	 * line argument and DO NOT look at the config setting.  Non-porcelain
+	 * formats use both.
+	 */
+	if (status_format == STATUS_FORMAT_PORCELAIN ||
+	    status_format == STATUS_FORMAT_PORCELAIN_V2) {
+		if (ahead_behind_opt < 0)
+			ahead_behind_opt = ABF_FULL;
+	} else {
+		if (ahead_behind_opt < 0)
+			ahead_behind_opt = core_ahead_behind;
+	}
+	s.ab_flags = ((ahead_behind_opt) ? ABF_FULL : ABF_QUICK);
+
 	read_cache_preload(&s.pathspec);
 	refresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &s.pathspec, NULL, NULL);
 
@@ -1667,6 +1685,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	s.commit_template = 1;
 	status_format = STATUS_FORMAT_NONE; /* Ignore status.short */
 	s.colopts = 0;
+	s.ab_flags = core_ahead_behind;
 
 	if (get_oid("HEAD", &oid))
 		current_head = NULL;
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index e319fa2..e132183 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -390,6 +390,66 @@ test_expect_success 'verify upstream fields in branch header' '
 	)
 '
 
+test_expect_success 'verify --no-ahead-behind generates branch.qab' '
+	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
+		# branch.qab eq
+		EOF
+
+		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +0 -0
+		EOF
+
+		# V2 does not use the config setting, only the command line argument.
+		git -c core.aheadbehind=false status --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual
+
+		## Test ahead/behind.
+		echo xyz >file_xyz &&
+		git add file_xyz &&
+		git commit -m xyz &&
+
+		HUF=$(git rev-parse HEAD) &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.qab neq
+		EOF
+
+		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +1 -0
+		EOF
+
+		# V2 does not use the config setting, only the command line argument.
+		git -c core.aheadbehind=false 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 80c23ba..d03b47a 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1867,6 +1867,12 @@ static void wt_porcelain_print(struct wt_status *s)
  *   [# branch.upstream <upstream><eol>
  *   [# branch.ab +<ahead> -<behind><eol>]]
  *
+ * When the '--no-ahead-behind' parameter is given and an upstream
+ * is set and present, the branch.ab line is omitted and the following
+ * line is added (for quick ahead/behind):
+ *
+ *   [# branch.qab <equal><eol>]
+ *
  *      <commit> ::= the current commit hash or the the literal
  *                   "(initial)" to indicate an initialized repo
  *                   with no commits.
@@ -1883,6 +1889,8 @@ static void wt_porcelain_print(struct wt_status *s)
  *      <behind> ::= integer behind value, when upstream set
  *                   and commit is present.
  *
+ *       <equal> ::= literal string "eq" or "neq".
+ *
  *
  * The end-of-line is defined by the -z flag.
  *
@@ -1896,7 +1904,7 @@ static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 	const char *base;
 	const char *branch_name;
 	struct wt_status_state state;
-	int have_ab_info, nr_ahead, nr_behind;
+	int nr_ahead, nr_behind, sti;
 	char eol = s->null_termination ? '\0' : '\n';
 
 	memset(&state, 0, sizeof(state));
@@ -1928,15 +1936,21 @@ 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;
-		have_ab_info = (stat_tracking_info(branch, &nr_ahead,
-						   &nr_behind, &base, ABF_FULL) >= 0);
+		sti = stat_tracking_info(branch, &nr_ahead, &nr_behind, &base,
+					 s->ab_flags);
 		if (base) {
 			base = shorten_unambiguous_ref(base, 0);
 			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
 			free((char *)base);
 
-			if (have_ab_info)
-				fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
+			if (sti >= 0) {
+				if (s->ab_flags == ABF_FULL)
+					fprintf(s->fp, "# branch.ab +%d -%d%c",
+						nr_ahead, nr_behind, eol);
+				else
+					fprintf(s->fp, "# branch.qab %s%c",
+						(sti ? "neq" : "eq"), eol);
+			}
 		}
 	}
 
diff --git a/wt-status.h b/wt-status.h
index 64f4d33..5075d95 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -5,6 +5,7 @@
 #include "string-list.h"
 #include "color.h"
 #include "pathspec.h"
+#include "remote.h"
 
 struct worktree;
 
@@ -80,6 +81,7 @@ struct wt_status {
 	int show_branch;
 	int show_stash;
 	int hints;
+	enum ahead_behind_flags ab_flags;
 
 	enum wt_status_format status_format;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
-- 
2.9.3

[PATCH v2 4/5] 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>
---
 t/t6040-tracking-info.sh | 13 +++++++++++++
 wt-status.c              |  9 ++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

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 d03b47a..3235ec2 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1765,7 +1765,7 @@ static void wt_shortstatus_print_tracking(struct wt_status *s)
 	const char *base;
 	char *short_base;
 	const char *branch_name;
-	int num_ours, num_theirs;
+	int num_ours, num_theirs, sti;
 	int upstream_is_gone = 0;
 
 	color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "## ");
@@ -1791,7 +1791,8 @@ 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, ABF_FULL) < 0) {
+	sti = stat_tracking_info(branch, &num_ours, &num_theirs, &base, s->ab_flags);
+	if (sti < 0) {
 		if (!base)
 			goto conclude;
 
@@ -1803,12 +1804,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->ab_flags == ABF_QUICK) {
+		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

[PATCH v2 5/5] status: support --no-ahead-behind in long 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
between the branch and the upstream.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd..b005139 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, &sb, ABF_FULL))
 		return;
 	fputs(sb.buf, stdout);
 	strbuf_release(&sb);
diff --git a/remote.c b/remote.c
index 91b5afb..76fa96c 100644
--- a/remote.c
+++ b/remote.c
@@ -2065,14 +2065,17 @@ 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, struct strbuf *sb,
+			 enum ahead_behind_flags abf)
 {
 	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, ABF_FULL) < 0) {
+	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, abf);
+	if (sti < 0) {
 		if (!full_base)
 			return 0;
 		upstream_is_gone = 1;
@@ -2086,10 +2089,17 @@ 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 (abf == ABF_QUICK) {
+		strbuf_addf(sb,
+			    _("Your branch and '%s' refer to different commits.\n"),
+			    base);
+		if (advice_status_hints)
+			strbuf_addf(sb, _("  (use \"%s\" for details)\n"),
+				    "git status --ahead-behind");
 	} 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 1e12dfb..33f88de 100644
--- a/remote.h
+++ b/remote.h
@@ -263,7 +263,8 @@ enum ahead_behind_flags {
 /* Reporting of tracking info */
 int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 		       const char **upstream_name, enum ahead_behind_flags abf);
-int format_tracking_info(struct branch *branch, struct strbuf *sb);
+int format_tracking_info(struct branch *branch, struct strbuf *sb,
+			 enum ahead_behind_flags abf);
 
 struct ref *get_local_heads(void);
 /*
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 0190220..716283b 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 and 'origin/master' refer to different commits.
+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 3235ec2..4a4c7b7 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, &sb, s->ab_flags))
 		return;
 
 	i = 0;
-- 
2.9.3

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Igor Djordjevic]

Hi Jeff,

On 21/12/2017 20:09, Jeff Hostetler wrote:
> 
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9593bfa..c78d6be 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -895,6 +895,14 @@ core.abbrev::
>  	abbreviated object names to stay unique for some time.
>  	The minimum length is 4.
>  
> +core.aheadbehind::
            ^^^
A small nitpick - you may want to use "core.aheadBehind" throughout 
the series (note capital "B"), making it more readable and aligning 
with the rest of `git config` variable names (using "bumpyCaps" as 
per coding guidelines[1], and as seen at the end of this very patch, 
too, "add.ignoreErrors").

> +	If true, tells commands like status and branch to print ahead and
> +	behind counts for the branch relative to its upstream branch.
> +	This computation may be very expensive when there is a great
> +	distance between the two branches.  If false, these commands
> +	only print that the two branches refer to different commits.
> +	Defaults to true.
> +
>  add.ignoreErrors::
>  add.ignore-errors (deprecated)::
>  	Tells 'git add' to continue adding files when some files cannot be

Regards, Buga

[1] https://github.com/git/git/blob/master/Documentation/CodingGuidelines

  Externally Visible Names
  
  ...
  
  The section and variable names that consist of multiple words are
  formed by concatenating the words without punctuations (e.g. `-`),
  and are broken using bumpyCaps in documentation as a hint to the
  reader.

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:

> Created core.aheadbehind config setting and core_ahead_behind
> global variable.  This value defaults to true.
>
> This value will be used in the next few commits as the default value
> for the --ahead-behind parameter.
>
> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
> ---
>  Documentation/config.txt | 8 ++++++++
>  cache.h                  | 1 +
>  config.c                 | 5 +++++
>  environment.c            | 1 +
>  4 files changed, 15 insertions(+)

Not a reason to reroll on its own, but this seems out of order: the
series is easier to explain and easier to merge down in stages if the
patch for --ahead-behind comes first, then the config setting.

More generally, new commandline flags tend to be less controversial
than new config settings since they cannot affect a script by mistake,
and for that reason, they can go earlier in the series.

As a bonus, that makes it possible to include tests.  It's probably
worth adding a test or two for this new config setting.

[...]
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 9593bfa..c78d6be 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -895,6 +895,14 @@ core.abbrev::
>  	abbreviated object names to stay unique for some time.
>  	The minimum length is 4.
>  
> +core.aheadbehind::
> +	If true, tells commands like status and branch to print ahead and
> +	behind counts for the branch relative to its upstream branch.
> +	This computation may be very expensive when there is a great
> +	distance between the two branches.  If false, these commands
> +	only print that the two branches refer to different commits.
> +	Defaults to true.

This doesn't seem like a particularly core feature to me.  Should it be
e.g. status.aheadbehind (even though it also affects "git branch") or
even something like diff.aheadbehind?  I'm not sure.

I also wonder if there's a way to achieve the same benefit without
having it be configurable.  E.g. if a branch is way behind, couldn't
we terminate the walk early to get the same bounded cost per branch
without requiring configuration?

Thanks,
Jonathan

Re: [PATCH v2 2/5] stat_tracking_info: return +1 when branches are not equal

[Jonathan Nieder]

Jeff Hostetler wrote:
> --- 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, ABF_FULL) < 0) {

What does ABF stand for?  It made me think of airport codes.

Would a name like AHEADBEHIND_FULL work?

[...]
> --- a/remote.c
> +++ b/remote.c
> @@ -1977,16 +1977,22 @@ 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.
> + * Compare a branch with its upstream and report on their differences.
> + * If abf is ABF_FULL, save their differences (number of commits) in
> + * *num_ours and *num_theirs.
> + * If abf is ABF_QUICK, skip the (possibly expensive) ahead/behind

Please format these comments as paragraphs, with a consistent
line-width and a "blank" (space-star-newline) line between paragraphs.
That makes them much easier to read.

[...]
> @@ -2019,6 +2025,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
>  		*num_theirs = *num_ours = 0;
>  		return 0;
>  	}
> +	if (abf == ABF_QUICK)
> +		return 1;

nit: I think this is missing a blank line before the 'if'.

Thanks and hope that helps,
Jonathan

Re: [PATCH v2 3/5] status: add --[no-]ahead-behind to porcelain V2 output

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:

> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -141,6 +141,7 @@ static int sequencer_in_use;
>  static int use_editor = 1, include_status = 1;
>  static int show_ignored_in_status, have_option_m;
>  static struct strbuf message = STRBUF_INIT;
> +static int ahead_behind_opt = -1;

nit: is there a logical place amid these constants to put the new option
instead of chronological order to make it easier to read through later?
That also has the side-benefit of making the new option less likely to
collidate with other patches that add a new option to commit.

That collection of options seems to be mostly about how the commit
message is generated.  Maybe this one could go after status_format:

	static enum wt_status_format status_format = ...;
	static int ahead_behind;

Even better if it can be made into a local in cmd_status.

[...]
> @@ -1369,6 +1370,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, "ahead-behind", &ahead_behind_opt,
> +			 N_("compute branch ahead/behind values")),
>  		OPT_END(),

Similar question: is there a natural place in "git status -h" to show
the new option instead of chronological order?

What does the value of the ahead_behind variable represent?  -1 means
unset so that we use config?  A comment might help.

[...]
> @@ -1389,6 +1392,21 @@ int cmd_status(int argc, const char **argv, const char *prefix)
>  		       PATHSPEC_PREFER_FULL,
>  		       prefix, argv);
>  
> +	/*
> +	 * Porcelain formats only look at the --[no-]ahead-behind command
> +	 * line argument and DO NOT look at the config setting.  Non-porcelain
> +	 * formats use both.
> +	 */

nit: No need to shout: s/DO NOT/do not/

> +	if (status_format == STATUS_FORMAT_PORCELAIN ||
> +	    status_format == STATUS_FORMAT_PORCELAIN_V2) {
> +		if (ahead_behind_opt < 0)
> +			ahead_behind_opt = ABF_FULL;
> +	} else {
> +		if (ahead_behind_opt < 0)
> +			ahead_behind_opt = core_ahead_behind;
> +	}

Can be more concise, to save the reader some time if they don't care
about the defaulting behavior:

	if (ahead_behind_opt == -1) {
		if (status_format == ...)
			ahead_behind_opt = ...;
		else
			ahead_behind_opt = ...;
		}
	}

> +	s.ab_flags = ((ahead_behind_opt) ? ABF_FULL : ABF_QUICK);

nit: both parens here are unnecessary and don't make the code clearer

[...]
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -390,6 +390,66 @@ test_expect_success 'verify upstream fields in branch header' '
>  	)
>  '
>  
> +test_expect_success 'verify --no-ahead-behind generates branch.qab' '
> +	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 &&
[...]

This looks like a collection of multiple tests.  Is there a
straightforward way to split them into multiple independent
test_expect_successes?

That way, it's easier to tell which failed if there is a regression
later and to run only one of them (using GIT_SKIP_TESTS) when
debugging such a failure.

Thanks,
Jonathan

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

>> Created core.aheadbehind config setting and core_ahead_behind
>> global variable.  This value defaults to true.
>>
>> This value will be used in the next few commits as the default value
>> for the --ahead-behind parameter.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>>  Documentation/config.txt | 8 ++++++++
>>  cache.h                  | 1 +
>>  config.c                 | 5 +++++
>>  environment.c            | 1 +
>>  4 files changed, 15 insertions(+)
>
> Not a reason to reroll on its own, but this seems out of order: the
> series is easier to explain and easier to merge down in stages if the
> patch for --ahead-behind comes first, then the config setting.
>
> More generally, new commandline flags tend to be less controversial
> than new config settings since they cannot affect a script by mistake,
> and for that reason, they can go earlier in the series.
>
> As a bonus, that makes it possible to include tests.  It's probably
> worth adding a test or two for this new config setting.
>
> [...]
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9593bfa..c78d6be 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -895,6 +895,14 @@ core.abbrev::
>>  	abbreviated object names to stay unique for some time.
>>  	The minimum length is 4.
>>  
>> +core.aheadbehind::
>> +	If true, tells commands like status and branch to print ahead and
>> +	behind counts for the branch relative to its upstream branch.
>> +	This computation may be very expensive when there is a great
>> +	distance between the two branches.  If false, these commands
>> +	only print that the two branches refer to different commits.
>> +	Defaults to true.
>
> This doesn't seem like a particularly core feature to me.  Should it be
> e.g. status.aheadbehind (even though it also affects "git branch") or
> even something like diff.aheadbehind?  I'm not sure.

FWIW, I do not think it is core at all, either; sorry for not
anticipating that a wrong name will be picked without a proper
guidance when I saw the "not limited to status" mentioned in the
discussion, but I was sick and offline for a few days, so...

> I also wonder if there's a way to achieve the same benefit without
> having it be configurable.  E.g. if a branch is way behind, couldn't
> we terminate the walk early to get the same bounded cost per branch
> without requiring configuration?

Hmm, that is an interesting thought.

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jeff King]

On Fri, Dec 22, 2017 at 10:21:23AM -0800, Junio C Hamano wrote:

> >> +core.aheadbehind::
> >> +	If true, tells commands like status and branch to print ahead and
> >> +	behind counts for the branch relative to its upstream branch.
> >> +	This computation may be very expensive when there is a great
> >> +	distance between the two branches.  If false, these commands
> >> +	only print that the two branches refer to different commits.
> >> +	Defaults to true.
> >
> > This doesn't seem like a particularly core feature to me.  Should it be
> > e.g. status.aheadbehind (even though it also affects "git branch") or
> > even something like diff.aheadbehind?  I'm not sure.
> 
> FWIW, I do not think it is core at all, either; sorry for not
> anticipating that a wrong name will be picked without a proper
> guidance when I saw the "not limited to status" mentioned in the
> discussion, but I was sick and offline for a few days, so...

I, too, had a funny feeling about calling this "core". But I didn't have
a better name, as I'm not sure what other place we have for config
options that cross many command boundaries. "diff" and "status" don't
seem quite right to me. While you can argue they are subsystems, it
seems too easy for users to confuse them with the commands of the same
names.

Maybe there should be a "ui.*" config hierarchy for these kinds of
cross-command interface options?

> > I also wonder if there's a way to achieve the same benefit without
> > having it be configurable.  E.g. if a branch is way behind, couldn't
> > we terminate the walk early to get the same bounded cost per branch
> > without requiring configuration?
> 
> Hmm, that is an interesting thought.

Yes, it is. Two thoughts:

  - It probably doesn't let us punt on the config naming, because we'd
    probably still want a knob for "how much work".

  - I wondered if we could give a better answer than "these two are
    different" based on a partial walk. But certainly not in the general
    case. E.g., imagine:

      ... -- master -- A -- B -- ... -- Y -- Z -- origin/master

    If we walk back from origin/master and give up somewhere in the
    middle, we can't say anything intelligent about the relationship.

-Peff

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I, too, had a funny feeling about calling this "core". But I didn't have
> a better name, as I'm not sure what other place we have for config
> options that cross many command boundaries. "diff" and "status" don't
> seem quite right to me. While you can argue they are subsystems, it
> seems too easy for users to confuse them with the commands of the same
> names.
>
> Maybe there should be a "ui.*" config hierarchy for these kinds of
> cross-command interface options?

I had an impression that ui.* was primarily pretty-printing,
colouring and things of such nature.  I do not think it is such a
bad idea to honor a status.frotz variable that affects how (e.g. to
what degree of detailedness) status on frotz are reported in Git
subcommands other than 'git status' if they report the same sort of
information on 'frotz' that 'git status' makes.

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jeff Hostetler]

On 12/21/2017 3:43 PM, Jonathan Nieder wrote:
> Hi,
> 
> Jeff Hostetler wrote:
> 
>> Created core.aheadbehind config setting and core_ahead_behind
>> global variable.  This value defaults to true.
>>
>> This value will be used in the next few commits as the default value
>> for the --ahead-behind parameter.
>>
>> Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
>> ---
>>   Documentation/config.txt | 8 ++++++++
>>   cache.h                  | 1 +
>>   config.c                 | 5 +++++
>>   environment.c            | 1 +
>>   4 files changed, 15 insertions(+)
> 
> Not a reason to reroll on its own, but this seems out of order: the
> series is easier to explain and easier to merge down in stages if the
> patch for --ahead-behind comes first, then the config setting.
> 
> More generally, new commandline flags tend to be less controversial
> than new config settings since they cannot affect a script by mistake,
> and for that reason, they can go earlier in the series.
> 
> As a bonus, that makes it possible to include tests.  It's probably
> worth adding a test or two for this new config setting.

I'll look at restacking the commits and make this later in the
series.  I have tests in a later commit that uses the config setting,
but at this point nothing uses it, so I didn't add any tests for it.
So maybe with a restacking, I can split up the tests to go with this
change.

> 
> [...]
>> diff --git a/Documentation/config.txt b/Documentation/config.txt
>> index 9593bfa..c78d6be 100644
>> --- a/Documentation/config.txt
>> +++ b/Documentation/config.txt
>> @@ -895,6 +895,14 @@ core.abbrev::
>>   	abbreviated object names to stay unique for some time.
>>   	The minimum length is 4.
>>   
>> +core.aheadbehind::
>> +	If true, tells commands like status and branch to print ahead and
>> +	behind counts for the branch relative to its upstream branch.
>> +	This computation may be very expensive when there is a great
>> +	distance between the two branches.  If false, these commands
>> +	only print that the two branches refer to different commits.
>> +	Defaults to true.
> 
> This doesn't seem like a particularly core feature to me.  Should it be
> e.g. status.aheadbehind (even though it also affects "git branch") or
> even something like diff.aheadbehind?  I'm not sure.

I wasn't sure where to put it after the earlier conversation in V1.

> I also wonder if there's a way to achieve the same benefit without
> having it be configurable.  E.g. if a branch is way behind, couldn't
> we terminate the walk early to get the same bounded cost per branch
> without requiring configuration?

I created a config setting because we don't want to force users to
type "git status --no-ahead-behind" on every interactive command to
get the benefit of it.  I guess we could ask them to alias it, if we
don't want a config setting.

Also, I didn't want to change the time-tested behavior that users see,
so I didn't want to change the algorithm in any way -- just not call it.


Would it make more sense to name this something like "status.aheadBehindLimit"
where 0 would mean no limit and match existing behavior and a positive
number be the number of commits we are allowed to search before giving up.
A value of 1 would match the "different" case in the current patch series.
For most users, a value of say 1000 would be sufficient most of the time
(and report at most a 999 a/b value), but keep us from going off into the
weeds for those ridiculous cases with very old branches.


Thanks,
Jeff

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jonathan Nieder]

Hi,

Jeff Hostetler wrote:
> On 12/21/2017 3:43 PM, Jonathan Nieder wrote:

>> I also wonder if there's a way to achieve the same benefit without
>> having it be configurable.  E.g. if a branch is way behind, couldn't
>> we terminate the walk early to get the same bounded cost per branch
>> without requiring configuration?
>
> I created a config setting because we don't want to force users to
> type "git status --no-ahead-behind" on every interactive command to
> get the benefit of it.  I guess we could ask them to alias it, if we
> don't want a config setting.
>
> Also, I didn't want to change the time-tested behavior that users see,
> so I didn't want to change the algorithm in any way -- just not call it.

I'm not too worried about people relying on the time-tested behavior
in this case.  It's a convenience feature for human users --- any
scripts looking for this information would be likely to use a more
convenient command like rev-list.

The one exception is "git status --porcelain=v2".  For that command,
scripts are likely to expect to be able to parse the "+<ahead>
-<behind>" line.  Alas, guarding it with a config doesn't help such
scripts --- they still need to be able to cope with the new behavior.

git-status(1) says:

	Parsers should ignore headers that they don't recognize.

so introducing a new line like "# branch.matchesupstream (true |
false)" like you did seems reasonable enough.  I *suspect* it should
be okay to omit the "# branch.ab" line as long as the script didn't
explicitly pass --ahead-behind but that depends on whether any scripts
in the wild were relying on the "# branch.ab" line being present.

> Would it make more sense to name this something like "status.aheadBehindLimit"
> where 0 would mean no limit and match existing behavior and a positive
> number be the number of commits we are allowed to search before giving up.

Sounds good to me.

Thanks,
Jonathan

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jeff King]

On Wed, Dec 27, 2017 at 09:41:30AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I, too, had a funny feeling about calling this "core". But I didn't have
> > a better name, as I'm not sure what other place we have for config
> > options that cross many command boundaries. "diff" and "status" don't
> > seem quite right to me. While you can argue they are subsystems, it
> > seems too easy for users to confuse them with the commands of the same
> > names.
> >
> > Maybe there should be a "ui.*" config hierarchy for these kinds of
> > cross-command interface options?
> 
> I had an impression that ui.* was primarily pretty-printing,
> colouring and things of such nature.

I didn't think we had a "ui.*" so far. We have "color.ui" and
"column.ui", but I think that's it.

At any rate, my intent was to consider this a "ui" issue, in that we are
deciding how the ahead/behind hints should be shown to the user.

> I do not think it is such a
> bad idea to honor a status.frotz variable that affects how (e.g. to
> what degree of detailedness) status on frotz are reported in Git
> subcommands other than 'git status' if they report the same sort of
> information on 'frotz' that 'git status' makes.

Is ahead/behind uniquely attached to git-status? IOW, could this be called
"branch.aheadbehind" and git-status respects it? It seems like putting
it in status introduces a weird asymmetry.

I buy the argument more that "status" here is not "this is a git-status
config option", but "this config section encompasses various things
about the status of a repository reported by many commands". But then
it's kind of funny to have many of the existing options there that
really are specific to git-status.

In can be both of those things, of course, but then it becomes less
clear to the user which config options affect which command.

I dunno. It is probably not _that_ big a deal, and I can live with it
wherever. But Git has a reputation for having inconsistencies and weird
asymmetries in its UI, so I like to give some thought to squashing them
preemptively.

-Peff

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Lars Schneider]

> On 04 Jan 2018, at 20:26, Jeff King <peff@peff.net> wrote:
> 
> On Wed, Dec 27, 2017 at 09:41:30AM -0800, Junio C Hamano wrote:
> 
>> Jeff King <peff@peff.net> writes:
>> 
>>> I, too, had a funny feeling about calling this "core". But I didn't have
>>> a better name, as I'm not sure what other place we have for config
>>> options that cross many command boundaries. "diff" and "status" don't
>>> seem quite right to me. While you can argue they are subsystems, it
>>> seems too easy for users to confuse them with the commands of the same
>>> names.
>>> 
>>> Maybe there should be a "ui.*" config hierarchy for these kinds of
>>> cross-command interface options?
>> 
>> I had an impression that ui.* was primarily pretty-printing,
>> colouring and things of such nature.
> 
> I didn't think we had a "ui.*" so far. We have "color.ui" and
> "column.ui", but I think that's it.
> 
> At any rate, my intent was to consider this a "ui" issue, in that we are
> deciding how the ahead/behind hints should be shown to the user.
> 
>> I do not think it is such a
>> bad idea to honor a status.frotz variable that affects how (e.g. to
>> what degree of detailedness) status on frotz are reported in Git
>> subcommands other than 'git status' if they report the same sort of
>> information on 'frotz' that 'git status' makes.
> 
> Is ahead/behind uniquely attached to git-status? IOW, could this be called
> "branch.aheadbehind" and git-status respects it? It seems like putting
> it in status introduces a weird asymmetry.
> 
> I buy the argument more that "status" here is not "this is a git-status
> config option", but "this config section encompasses various things
> about the status of a repository reported by many commands". But then
> it's kind of funny to have many of the existing options there that
> really are specific to git-status.
> 
> In can be both of those things, of course, but then it becomes less
> clear to the user which config options affect which command.
> 
> I dunno. It is probably not _that_ big a deal, and I can live with it
> wherever. But Git has a reputation for having inconsistencies and weird
> asymmetries in its UI, so I like to give some thought to squashing them
> preemptively.

What is the state of this series? I can't find it in git/git nor in 
git-for-windows/git. I think Stolee mentioned the config in
his Git Merge talk [1] and I was about to test it/roll it out :-)

- Lars


[1] https://youtu.be/oOMzi983Qmw

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Ævar Arnfjörð Bjarmason]

On Tue, Apr 03 2018, Lars Schneider wrote:

>> On 04 Jan 2018, at 20:26, Jeff King <peff@peff.net> wrote:
>>
>> On Wed, Dec 27, 2017 at 09:41:30AM -0800, Junio C Hamano wrote:
>>
>>> Jeff King <peff@peff.net> writes:
>>>
>>>> I, too, had a funny feeling about calling this "core". But I didn't have
>>>> a better name, as I'm not sure what other place we have for config
>>>> options that cross many command boundaries. "diff" and "status" don't
>>>> seem quite right to me. While you can argue they are subsystems, it
>>>> seems too easy for users to confuse them with the commands of the same
>>>> names.
>>>>
>>>> Maybe there should be a "ui.*" config hierarchy for these kinds of
>>>> cross-command interface options?
>>>
>>> I had an impression that ui.* was primarily pretty-printing,
>>> colouring and things of such nature.
>>
>> I didn't think we had a "ui.*" so far. We have "color.ui" and
>> "column.ui", but I think that's it.
>>
>> At any rate, my intent was to consider this a "ui" issue, in that we are
>> deciding how the ahead/behind hints should be shown to the user.
>>
>>> I do not think it is such a
>>> bad idea to honor a status.frotz variable that affects how (e.g. to
>>> what degree of detailedness) status on frotz are reported in Git
>>> subcommands other than 'git status' if they report the same sort of
>>> information on 'frotz' that 'git status' makes.
>>
>> Is ahead/behind uniquely attached to git-status? IOW, could this be called
>> "branch.aheadbehind" and git-status respects it? It seems like putting
>> it in status introduces a weird asymmetry.
>>
>> I buy the argument more that "status" here is not "this is a git-status
>> config option", but "this config section encompasses various things
>> about the status of a repository reported by many commands". But then
>> it's kind of funny to have many of the existing options there that
>> really are specific to git-status.
>>
>> In can be both of those things, of course, but then it becomes less
>> clear to the user which config options affect which command.
>>
>> I dunno. It is probably not _that_ big a deal, and I can live with it
>> wherever. But Git has a reputation for having inconsistencies and weird
>> asymmetries in its UI, so I like to give some thought to squashing them
>> preemptively.
>
> What is the state of this series? I can't find it in git/git nor in
> git-for-windows/git. I think Stolee mentioned the config in
> his Git Merge talk [1] and I was about to test it/roll it out :-)

It's in the gvfs branch of git@github.com:Microsoft/git.git, i.e. it's
not in Git for Windows, but used in Microsoft's own in-house version
used for Windows.git.

I may be misunderstanding this feature, but my impression was that it
was a kludge as a workaround until the commit graph code landed, because
once we have that then surely we can just cheaply report the actual (or
approximate?) number in the common case, but of course it may still be
slow if your commit graph file is out of date.

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Derrick Stolee]

On 4/3/2018 6:18 AM, Ævar Arnfjörð Bjarmason wrote:
> On Tue, Apr 03 2018, Lars Schneider wrote:
>> What is the state of this series? I can't find it in git/git nor in
>> git-for-windows/git. I think Stolee mentioned the config in
>> his Git Merge talk [1] and I was about to test it/roll it out :-)
> It's in the gvfs branch of git@github.com:Microsoft/git.git, i.e. it's
> not in Git for Windows, but used in Microsoft's own in-house version
> used for Windows.git.

Thanks for adding me to CC. I mentioned it in my talk because that was 
one thing we shipped internally as a "quick fix" until we could do the 
right thing.

If I remember correctly, Jeff abandoned shipping this upstream because 
it did have the feel of a hack and we wanted to see if users used the 
config setting or really cared about the output values. We saw fast 
adoption of the feature and even turned the config setting on 
automatically in the following version of GVFS.

> I may be misunderstanding this feature, but my impression was that it
> was a kludge as a workaround until the commit graph code landed, because
> once we have that then surely we can just cheaply report the actual (or
> approximate?) number in the common case, but of course it may still be
> slow if your commit graph file is out of date.

You are correct that the commit-graph file may be out of date, causing 
slower performance. Even worse: the current graph patch only provides a 
constant-multiple speedup (still walking the same number of commits, but 
each commit is parsed much faster).

Speaking of our GVFS-specific fork [0], the 'gvfs' branch was updated 
just yesterday with a couple of changes that I am prepping for 
submission upstream:

* Lazy-load trees when parsing commits from commit-graph [1]
* Compute and consume generation numbers [2]

Each of these will speed up this ahead/behind calculation in different 
ways. [1] makes the cost of loading each commit a bit faster, saving up 
to 20% overall. [2] uses generation numbers in paint_down_to_common() to 
make the while() condition O(1) instead of O(Q) where Q is the size of 
the priority queue. The Windows repo is particularly "wide" with many 
parallel branches being merged in complicated ways, so the queue becomes 
quite large. This use of generation numbers saves about 4% on some 
ahead/behind calculations. This speedup is modest, but the existing code 
already made good use of limiting the commit walk to be mostly the 
"important" commits.

The real benefit of generation numbers will manifest in a way to make 
--topo-order much faster when rendering a small number of commits.

The generation numbers _could_ be used to approximate the ahead/behind 
calculation in the following way: When comparing A and B, and gen(A) < 
gen(B), then A is at least (gen(B) - gen(A)) behind. That's the only 
information that can be gathered directly from those values, but may be 
enough to short circuit an exact count.

To truly accelerate these ahead/behind calculations to be sub-linear* in 
the ahead/behind counts, we would need a bitmap-based approach. The 
object-reachability bitmap is a non-starter for client machines in the 
Windows repo, but perhaps a commit-reachability bitmap could be 
interesting. Performing set operations on the bitmaps could more quickly 
answer these questions. Just thinking about it makes me want to go down 
a deep rabbit hole, investigating ways to compute, store, and use these 
bitmaps. However: let's wait and see how necessary it is as the 
commit-graph feature stabilizes. (*These bitmap approaches are not 
guaranteed to be sub-linear, because it may include iterating through a 
list of O(N) bits, but good run-length encodings will likely make the 
count operation very fast, even with a set-difference operation included.)

There are too many fun things to work on, not enough time!

Thanks,
-Stolee

[0] https://github.com/microsoft/git
     Fork of GitForWindows that ships to Windows developers

[1] 
https://github.com/Microsoft/git/commit/29114bf86f591f5c87075f779a1faa2d0f17b92f
     Lazy-load trees when parsing commits from commit-graph 
(accidentally squashed to one commit)

[2] 
https://github.com/microsoft/git/compare/879b7d3b1bddea2587b28cdd656c9c655018683a...a0731ca93a35fd042560c4b30e8e0edbdfa4bf9f
     Compute and consume generation numbers

Re: [PATCH v2 1/5] core.aheadbehind: add new config setting

[Jeff Hostetler]

On 4/3/2018 7:39 AM, Derrick Stolee wrote:
> On 4/3/2018 6:18 AM, Ævar Arnfjörð Bjarmason wrote:
>> On Tue, Apr 03 2018, Lars Schneider wrote:
>>> What is the state of this series? I can't find it in git/git nor in
>>> git-for-windows/git. I think Stolee mentioned the config in
>>> his Git Merge talk [1] and I was about to test it/roll it out :-)
>> It's in the gvfs branch of git@github.com:Microsoft/git.git, i.e. it's
>> not in Git for Windows, but used in Microsoft's own in-house version
>> used for Windows.git.
> 
> Thanks for adding me to CC. I mentioned it in my talk because that was one thing we shipped internally as a "quick fix" until we could do the right thing.
> 
> If I remember correctly, Jeff abandoned shipping this upstream because it did have the feel of a hack and we wanted to see if users used the config setting or really cared about the output values. We saw fast adoption of the feature and even turned the config setting on automatically in the following version of GVFS.
> 
>> I may be misunderstanding this feature, but my impression was that it
>> was a kludge as a workaround until the commit graph code landed, because
>> once we have that then surely we can just cheaply report the actual (or
>> approximate?) number in the common case, but of course it may still be
>> slow if your commit graph file is out of date.

Right, the only thing in master are the changes to take the new
command line option and to alter the output of status.  We did not
reach consensus on the need for the config setting and/or whether
it should be in "core." or "status." or another namespace and/or
how it should work.

And yes, it was also seen as a hack (just turn it off) until the
client-side commit graph was ready (at least for interactive use).
Because there are callers that don't need the answer (regardless
of whether it is cheap to compute) and so the explicit command line
arg limitation is sufficient for them.


This part is in upstream master:
     commit 4094e47fd2c49fcdbd0152d20ed4d610d72680d7
     Merge: c710d182ea f39a757dd9
     Author: Junio C Hamano <gitster@pobox.com>
     Date:   Thu Mar 8 12:36:24 2018 -0800
     
         Merge branch 'jh/status-no-ahead-behind'


These parts are in the 'gvfs' branch in the git@github.com:Microsoft/git.git repo:

     commit 039f65946968fa654a9c3bca27a4f4e93c1c9381
     Author: Jeff Hostetler <jeffhost@microsoft.com>
     Date:   Wed Jan 10 13:50:24 2018 -0500
     
         status: add warning when a/b calculation takes too long for long/normal format

     commit 0d6756f06d0ad6f1fdc8dba0ead7911e411c9704
     Author: Jeff Hostetler <jeffhost@microsoft.com>
     Date:   Mon Feb 5 09:44:04 2018 -0500
     
         status: ignore status.aheadbehind in porcelain formats

         Teach porcelain V[12] formats to ignore the status.aheadbehind
         config setting. They only respect the --[no-]ahead-behind
         command line argument.  This is for backwards compatibility
         with existing scripts.

     commit 0dd122d6cd43106a5928587d768a7381cfe9e7a3
     Author: Jeff Hostetler <jeffhost@microsoft.com>
     Date:   Tue Jan 9 14:16:07 2018 -0500
     
         status: add status.aheadbehind setting
     
         Add "status.aheadbehind" config setting to change the default
         behavior of ALL git status formats.

Hope this helps,
Jeff