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

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

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

This is version 3 of my patch series to avoid expensive
ahead/behind calculations in status.  This version tries
to address most of the comments in V2.

I've switched back to a "status.aheadBehind" config setting
rather than in "core.*".  This has been better integrated
with the existing status_deferred_config mechanism in
builtin/commit.c and lets both status and commit inherit it.

Config values of true and false control non-porcelain formats
for compatibility reasons as previously discussed.  In the
last commit I added a new value of 2 for the config setting
to allow porcelain formats to inherit the new setting.  I've
marked this experimental for now or so that we can discuss
it.

Jeff Hostetler (5):
  stat_tracking_info: return +1 when branches not equal
  status: add --[no-]ahead-behind to status and commit for V2 format.
  status: update short status to respect --no-ahead-behind
  status: support --no-ahead-behind in long format
  status: add status.aheadBehind value for porcelain output

 Documentation/config.txt     | 11 ++++++
 Documentation/git-status.txt |  5 +++
 builtin/checkout.c           |  2 +-
 builtin/commit.c             | 37 +++++++++++++++++++-
 ref-filter.c                 |  8 ++---
 remote.c                     | 42 +++++++++++++++++------
 remote.h                     | 20 +++++++++--
 t/t6040-tracking-info.sh     | 82 ++++++++++++++++++++++++++++++++++++++++++++
 t/t7064-wtstatus-pv2.sh      | 73 +++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 38 +++++++++++++++-----
 wt-status.h                  |  2 ++
 11 files changed, 292 insertions(+), 28 deletions(-)

-- 
2.9.3

[PATCH v3 1/5] stat_tracking_info: return +1 when branches not equal

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Extend stat_tracking_info() to return +1 when branches are not equal and to
take a new "enum ahead_behind_flags" argument to allow skipping the (possibly
expensive) ahead/behind computation.

This will be used in the next commit to allow "git status" to avoid full
ahead/behind calculations for performance reasons.

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

diff --git a/ref-filter.c b/ref-filter.c
index e728b15..23bcdc4 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -1238,8 +1238,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 	if (atom->u.remote_ref.option == RR_REF)
 		*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)) {
+		if (stat_tracking_info(branch, &num_ours, &num_theirs,
+				       NULL, AHEAD_BEHIND_FULL) < 0) {
 			*s = xstrdup(msgs.gone);
 		} else if (!num_ours && !num_theirs)
 			*s = "";
@@ -1256,8 +1256,8 @@ static void fill_remote_ref_details(struct used_atom *atom, const char *refname,
 			free((void *)to_free);
 		}
 	} else if (atom->u.remote_ref.option == RR_TRACKSHORT) {
-		if (stat_tracking_info(branch, &num_ours,
-				       &num_theirs, NULL))
+		if (stat_tracking_info(branch, &num_ours, &num_theirs,
+				       NULL, AHEAD_BEHIND_FULL) < 0)
 			return;
 
 		if (!num_ours && !num_theirs)
diff --git a/remote.c b/remote.c
index b220f0d..ca5a416 100644
--- a/remote.c
+++ b/remote.c
@@ -1977,16 +1977,23 @@ 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.
+ * Lookup the upstream branch for the given branch and if present, optionally
+ * compute the commit ahead/behind values for the pair.
+ *
+ * If abf is AHEAD_BEHIND_FULL, compute the full ahead/behind and return the
+ * counts in *num_ours and *num_theirs.  If abf is AHEAD_BEHIND_QUICK, skip
+ * the (potentially expensive) a/b computation (*num_ours and *num_theirs are
+ * left 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
+ * identical.  Returns 1 if commits are different.
  */
 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 +2026,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 		*num_theirs = *num_ours = 0;
 		return 0;
 	}
+	if (abf == AHEAD_BEHIND_QUICK)
+		return 1;
 
 	/* Run "rev-list --left-right ours...theirs" internally... */
 	argv_array_push(&argv, ""); /* ignored */
@@ -2051,7 +2060,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 +2073,8 @@ 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,
+			       AHEAD_BEHIND_FULL) < 0) {
 		if (!full_base)
 			return 0;
 		upstream_is_gone = 1;
diff --git a/remote.h b/remote.h
index 2ecf4c8..00932f5 100644
--- a/remote.h
+++ b/remote.h
@@ -255,9 +255,15 @@ enum match_refs_flags {
 	MATCH_REFS_FOLLOW_TAGS	= (1 << 3)
 };
 
+/* Flags for --ahead-behind option. */
+enum ahead_behind_flags {
+	AHEAD_BEHIND_QUICK = 0,  /* just eq/neq reporting */
+	AHEAD_BEHIND_FULL  = 1,  /* traditional a/b 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..8f7fdc6 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -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) < 0) {
+	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base,
+			       AHEAD_BEHIND_FULL) < 0) {
 		if (!base)
 			goto conclude;
 
@@ -1928,7 +1929,8 @@ 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);
+		ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind,
+					      &base, AHEAD_BEHIND_FULL) >= 0);
 		if (base) {
 			base = shorten_unambiguous_ref(base, 0);
 			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
-- 
2.9.3

Re: [PATCH v3 1/5] stat_tracking_info: return +1 when branches not equal

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> -		if (stat_tracking_info(branch, &num_ours,
> -				       &num_theirs, NULL)) {
> +		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> +				       NULL, AHEAD_BEHIND_FULL) < 0) {
> ...
> -		if (stat_tracking_info(branch, &num_ours,
> -				       &num_theirs, NULL))
> +		if (stat_tracking_info(branch, &num_ours, &num_theirs,
> +				       NULL, AHEAD_BEHIND_FULL) < 0)

Mental note: any code that reacted to stat_tracking_info() returning
non-zero was reacting to "no useful info in num_{ours,theirs}".
They now have to compare the returned value with "< 0" for the same
purpose.

> ...
>   * 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
> + * identical.  Returns 1 if commits are different.
>   */
>  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 +2026,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
>  		*num_theirs = *num_ours = 0;
>  		return 0;
>  	}
> +	if (abf == AHEAD_BEHIND_QUICK)
> +		return 1;
> ...
>  	argv_array_clear(&argv);
> -	return 0;
> +	return 1;
>  }

When a caller gets +1 from the function, it is not safe to peek into
*num_{ours,theirs} if it passed _QUICK.

> @@ -2064,7 +2073,8 @@ int format_tracking_info(struct branch *branch, struct strbuf *sb)
> -	if (stat_tracking_info(branch, &ours, &theirs, &full_base) < 0) {
> +	if (stat_tracking_info(branch, &ours, &theirs, &full_base,
> +			       AHEAD_BEHIND_FULL) < 0) {

Sane conversion to the new return value convention.

> diff --git a/wt-status.c b/wt-status.c
> index 94e5eba..8f7fdc6 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -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) < 0) {
> +	if (stat_tracking_info(branch, &num_ours, &num_theirs, &base,
> +			       AHEAD_BEHIND_FULL) < 0) {

Ditto.

> @@ -1928,7 +1929,8 @@ 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);
> +		ab_info = (stat_tracking_info(branch, &nr_ahead, &nr_behind,
> +					      &base, AHEAD_BEHIND_FULL) >= 0);

If a later step plans to (conditionally) allow _QUICK to be passed
here, this conversion is questionable, because ab_info being true
no longer is a sign that nr_{ahead,behind} are valid.

I suspect that moving the "s/ab_info/sti/" bits around here from
step [2/5] to this commit may make the result after this patch more
consistent, but it is not a big deal either way.

>  		if (base) {
>  			base = shorten_unambiguous_ref(base, 0);
>  			fprintf(s->fp, "# branch.upstream %s%c", base, eol);

[PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Teach "git status" and "git commit" to accept "--no-ahead-behind"
and "--ahead-behind" arguments to request quick or full ahead/behind
reporting.

When "--no-ahead-behind" is given, the existing porcelain V2 line
"branch.ab x y" is replaced with a new "branch equal eq|neq" line.
This indicates that the branch and its upstream are or are not equal
without the expense of computing the full ahead/behind values.

Added "status.aheadBehind" config setting.  This is only used by
non-porcelain format for backward-compatibility.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/config.txt     |  6 ++++
 Documentation/git-status.txt |  5 ++++
 builtin/commit.c             | 18 +++++++++++-
 remote.c                     |  2 ++
 remote.h                     |  5 ++--
 t/t7064-wtstatus-pv2.sh      | 69 ++++++++++++++++++++++++++++++++++++++++++++
 wt-status.c                  | 27 +++++++++++++----
 wt-status.h                  |  2 ++
 8 files changed, 125 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 9593bfa..affb0d6 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3035,6 +3035,12 @@ status.branch::
 	Set to true to enable --branch by default in linkgit:git-status[1].
 	The option --no-branch takes precedence over this variable.
 
+status.aheadBehind::
+	Set to true to enable --ahead-behind and to false to enable
+	--no-ahead-behind by default in linkgit:git-status[1] for
+	non-porcelain formats.  This setting is ignored by porcelain
+	formats for backwards compatibility.
+
 status.displayCommentPrefix::
 	If set to true, linkgit:git-status[1] will insert a comment
 	prefix before each output line (starting with
diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f3a78a..603bf40 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -111,6 +111,11 @@ 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.
+
 <pathspec>...::
 	See the 'pathspec' entry in linkgit:gitglossary[7].
 
diff --git a/builtin/commit.c b/builtin/commit.c
index be370f6..416fe2c 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1109,9 +1109,11 @@ static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
 	enum wt_status_format status_format;
 	int show_branch;
+	enum ahead_behind_flags ahead_behind;
 } status_deferred_config = {
 	STATUS_FORMAT_UNSPECIFIED,
-	-1 /* unspecified */
+	-1, /* unspecified */
+	AHEAD_BEHIND_UNSPECIFIED,
 };
 
 static void finalize_deferred_config(struct wt_status *s)
@@ -1137,6 +1139,12 @@ static void finalize_deferred_config(struct wt_status *s)
 		s->show_branch = status_deferred_config.show_branch;
 	if (s->show_branch < 0)
 		s->show_branch = 0;
+
+	if (use_deferred_config &&
+	    s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+		s->ahead_behind_flags = status_deferred_config.ahead_behind;
+	if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
+		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1297,6 +1305,10 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		status_deferred_config.show_branch = git_config_bool(k, v);
 		return 0;
 	}
+	if (!strcmp(k, "status.aheadbehind")) {
+		status_deferred_config.ahead_behind = git_config_bool(k, v);
+		return 0;
+	}
 	if (!strcmp(k, "status.showstash")) {
 		s->show_stash = git_config_bool(k, v);
 		return 0;
@@ -1351,6 +1363,8 @@ int cmd_status(int argc, const char **argv, const char *prefix)
 			 N_("show branch information")),
 		OPT_BOOL(0, "show-stash", &s.show_stash,
 			 N_("show stash information")),
+		OPT_BOOL(0, "ahead-behind", &s.ahead_behind_flags,
+			 N_("compute full ahead/behind values")),
 		{ OPTION_CALLBACK, 0, "porcelain", &status_format,
 		  N_("version"), N_("machine-readable output"),
 		  PARSE_OPT_OPTARG, opt_parse_porcelain },
@@ -1628,6 +1642,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 		OPT_SET_INT(0, "short", &status_format, N_("show status concisely"),
 			    STATUS_FORMAT_SHORT),
 		OPT_BOOL(0, "branch", &s.show_branch, N_("show branch information")),
+		OPT_BOOL(0, "ahead-behind", &s.ahead_behind_flags,
+			 N_("compute full ahead/behind values")),
 		OPT_SET_INT(0, "porcelain", &status_format,
 			    N_("machine-readable output"), STATUS_FORMAT_PORCELAIN),
 		OPT_SET_INT(0, "long", &status_format,
diff --git a/remote.c b/remote.c
index ca5a416..32706bc 100644
--- a/remote.c
+++ b/remote.c
@@ -2028,6 +2028,8 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs,
 	}
 	if (abf == AHEAD_BEHIND_QUICK)
 		return 1;
+	if (abf != AHEAD_BEHIND_FULL)
+		BUG("stat_tracking_info: invalid abf '%d'", abf);
 
 	/* Run "rev-list --left-right ours...theirs" internally... */
 	argv_array_push(&argv, ""); /* ignored */
diff --git a/remote.h b/remote.h
index 00932f5..27feb63 100644
--- a/remote.h
+++ b/remote.h
@@ -257,8 +257,9 @@ enum match_refs_flags {
 
 /* Flags for --ahead-behind option. */
 enum ahead_behind_flags {
-	AHEAD_BEHIND_QUICK = 0,  /* just eq/neq reporting */
-	AHEAD_BEHIND_FULL  = 1,  /* traditional a/b reporting */
+	AHEAD_BEHIND_UNSPECIFIED = -1,
+	AHEAD_BEHIND_QUICK       =  0,  /* just eq/neq reporting */
+	AHEAD_BEHIND_FULL        =  1,  /* traditional a/b reporting */
 };
 
 /* Reporting of tracking info */
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index e319fa2..67b90cd 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -390,6 +390,75 @@ test_expect_success 'verify upstream fields in branch header' '
 	)
 '
 
+test_expect_success 'verify --[no-]ahead-behind with V2 format' '
+	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) &&
+
+		# Confirm --no-ahead-behind reports branch.equal with "eq".
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.equal eq
+		EOF
+
+		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		# Confirm --ahead-behind reports traditional branch.ab with 0/0.
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +0 -0
+		EOF
+
+		git status --ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		# Confirm that porcelain=v2 format does not inherit status.aheadBehind value.
+		git -c status.aheadbehind=false status --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		git -c status.aheadbehind=true 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) &&
+
+		# Confirm --no-ahead-behind reports branch.equal with "neq".
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.equal neq
+		EOF
+
+		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
+		# Confirm --ahead-behind reports traditional branch.ab with 1/0.
+		cat >expect <<-EOF &&
+		# branch.oid $HUF
+		# branch.head master
+		# branch.upstream origin/master
+		# branch.ab +1 -0
+		EOF
+
+		git status --ahead-behind --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 8f7fdc6..3959d31 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -136,6 +136,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->ignored.strdup_strings = 1;
 	s->show_branch = -1;  /* unspecified */
 	s->show_stash = 0;
+	s->ahead_behind_flags = AHEAD_BEHIND_UNSPECIFIED;
 	s->display_comment_prefix = 0;
 }
 
@@ -1866,7 +1867,8 @@ static void wt_porcelain_print(struct wt_status *s)
  *    # branch.oid <commit><eol>
  *    # branch.head <head><eol>
  *   [# branch.upstream <upstream><eol>
- *   [# branch.ab +<ahead> -<behind><eol>]]
+ *   [# branch.ab +<ahead> -<behind><eol>]
+ *   [# branch.equal <equal><eol>]]
  *
  *      <commit> ::= the current commit hash or the the literal
  *                   "(initial)" to indicate an initialized repo
@@ -1884,12 +1886,16 @@ 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.
  *
  *                 <eol> ::= NUL when -z,
  *                           LF when NOT -z.
  *
+ * When an upstream is set and present, the 'branch.ab' line will
+ * be printed when the ahead_behind_flags are set to _FULL and
+ * the 'branch.equal' line will be printed when set to _QUICK.
  */
 static void wt_porcelain_v2_print_tracking(struct wt_status *s)
 {
@@ -1897,7 +1903,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 sti, nr_ahead, nr_behind;
 	char eol = s->null_termination ? '\0' : '\n';
 
 	memset(&state, 0, sizeof(state));
@@ -1929,15 +1935,24 @@ 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, AHEAD_BEHIND_FULL) >= 0);
+		sti = stat_tracking_info(branch, &nr_ahead, &nr_behind,
+					 &base, s->ahead_behind_flags);
 		if (base) {
 			base = shorten_unambiguous_ref(base, 0);
 			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
 			free((char *)base);
 
-			if (ab_info)
-				fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
+			if (sti >= 0) {
+				if (s->ahead_behind_flags == AHEAD_BEHIND_FULL)
+					fprintf(s->fp, "# branch.ab +%d -%d%c",
+						nr_ahead, nr_behind, eol);
+				else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
+					fprintf(s->fp, "# branch.equal %s%c",
+						sti ? "neq" : "eq", eol);
+				else
+					BUG("invalid ahead_behind_flag '%d'",
+					    s->ahead_behind_flags);
+			}
 		}
 	}
 
diff --git a/wt-status.h b/wt-status.h
index 64f4d33..b450b53 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 ahead_behind_flags;
 
 	enum wt_status_format status_format;
 	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
-- 
2.9.3

Re: [PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> +		sti = stat_tracking_info(branch, &nr_ahead, &nr_behind,
> +					 &base, s->ahead_behind_flags);
>  		if (base) {
>  			base = shorten_unambiguous_ref(base, 0);
>  			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
>  			free((char *)base);
>  
> -			if (ab_info)
> -				fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
> +			if (sti >= 0) {
> +				if (s->ahead_behind_flags == AHEAD_BEHIND_FULL)
> +					fprintf(s->fp, "# branch.ab +%d -%d%c",
> +						nr_ahead, nr_behind, eol);
> +				else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
> +					fprintf(s->fp, "# branch.equal %s%c",
> +						sti ? "neq" : "eq", eol);

This is related to what I said in the review of [1/5], but this
arrangement to require the caller to know that _QUICK request
results in incomplete information smells like a bit of maintenance
hassle.

We'd rather allow the caller to tell if it was given incomplete
information from the values returned by stat_tracking_info()
function (notice the plural "values" here; what is returned in
nr_{ahead,behind} also counts).  For example, we can keep the (-1 =>
"no relation", 0 => "identical", 1 => "different") as return values
of the function, but have it clear nr_{ahead,behind} when it only
knows the two are different but not by how many commits.  That way,
this step can instead do:

	ab_info = stat_tracking_info(...);
	if (base) {
		... do the base thing ...
		if (ab_info > 0) {
			/* different */
			if (nr_ahead || nr_behind)
				fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
			else
				fprintf(... "neq" ...);
		} else if (!ab_info) {
			/* same */
			fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
		}
		...

That would allow us to later add different kinds of _QUICK that do
not give full information without having to update this consumer
with something like

-	else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
+	else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK ||
+		 s->ahead_behind_flags == AHEAD_BEHIND_QUICK_DIFFERENTLY)

when it happens.

Two related tangents.  

 - I do not see why "eq" need to exist at all.  Even in _QUICK mode,
   when you determine that two are identical, you know your branch
   is ahead and behind by 0 commit each.

 - A short-format that is easy to parse by machines does not have to
   be cryptic to humans.  s/neq/not-equal/ or something may be an
   improvement.

Thanks.

Re: [PATCH v3 2/5] status: add --[no-]ahead-behind to status and commit for V2 format.

[Jeff Hostetler]

On 1/4/2018 5:05 PM, Junio C Hamano wrote:
> Jeff Hostetler <git@jeffhostetler.com> writes:
> 
>> +		sti = stat_tracking_info(branch, &nr_ahead, &nr_behind,
>> +					 &base, s->ahead_behind_flags);
>>   		if (base) {
>>   			base = shorten_unambiguous_ref(base, 0);
>>   			fprintf(s->fp, "# branch.upstream %s%c", base, eol);
>>   			free((char *)base);
>>   
>> -			if (ab_info)
>> -				fprintf(s->fp, "# branch.ab +%d -%d%c", nr_ahead, nr_behind, eol);
>> +			if (sti >= 0) {
>> +				if (s->ahead_behind_flags == AHEAD_BEHIND_FULL)
>> +					fprintf(s->fp, "# branch.ab +%d -%d%c",
>> +						nr_ahead, nr_behind, eol);
>> +				else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
>> +					fprintf(s->fp, "# branch.equal %s%c",
>> +						sti ? "neq" : "eq", eol);
> 
> This is related to what I said in the review of [1/5], but this
> arrangement to require the caller to know that _QUICK request
> results in incomplete information smells like a bit of maintenance
> hassle.
> 
> We'd rather allow the caller to tell if it was given incomplete
> information from the values returned by stat_tracking_info()
> function (notice the plural "values" here; what is returned in
> nr_{ahead,behind} also counts).  For example, we can keep the (-1 =>
> "no relation", 0 => "identical", 1 => "different") as return values
> of the function, but have it clear nr_{ahead,behind} when it only
> knows the two are different but not by how many commits.  That way,
> this step can instead do:
> 
> 	ab_info = stat_tracking_info(...);
> 	if (base) {
> 		... do the base thing ...
> 		if (ab_info > 0) {
> 			/* different */
> 			if (nr_ahead || nr_behind)
> 				fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
> 			else
> 				fprintf(... "neq" ...);
> 		} else if (!ab_info) {
> 			/* same */
> 			fprintf(... +%d -%d ... nr_ahead, nr_behind, ...);
> 		}
> 		...
> 
> That would allow us to later add different kinds of _QUICK that do
> not give full information without having to update this consumer
> with something like
> 
> -	else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK)
> +	else if (s->ahead_behind_flags == AHEAD_BEHIND_QUICK ||
> +		 s->ahead_behind_flags == AHEAD_BEHIND_QUICK_DIFFERENTLY)
> 
> when it happens.
> 
> Two related tangents.
> 
>   - I do not see why "eq" need to exist at all.  Even in _QUICK mode,
>     when you determine that two are identical, you know your branch
>     is ahead and behind by 0 commit each.
> 
>   - A short-format that is easy to parse by machines does not have to
>     be cryptic to humans.  s/neq/not-equal/ or something may be an
>     improvement.
> 
> Thanks.
> 

Thanks for the review.  Let me update it along the lines suggested here.
It felt odd to define the nr_{ahead,behind} as undefined, but short of
making them negative or something I wasn't sure what was best.

Thanks,
Jeff

[PATCH v3 3/5] status: update short status to respect --no-ahead-behind

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

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

Short status also respect the "status.aheadBehind" config setting.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 t/t6040-tracking-info.sh | 26 ++++++++++++++++++++++++++
 wt-status.c              | 11 +++++++----
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index 8f17fd9..053dff3 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -147,6 +147,32 @@ 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
+## b1...origin/master [different]
+EOF
+
+test_expect_success 'status.aheadbehind=false status -s -b (diverged from upstream)' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=false status -s -b | head -1
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index 3959d31..df6cc33 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1766,7 +1766,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), "## ");
@@ -1792,8 +1792,9 @@ 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,
-			       AHEAD_BEHIND_FULL) < 0) {
+	sti = stat_tracking_info(branch, &num_ours, &num_theirs, &base,
+				 s->ahead_behind_flags);
+	if (sti < 0) {
 		if (!base)
 			goto conclude;
 
@@ -1805,12 +1806,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->ahead_behind_flags == AHEAD_BEHIND_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 v3 4/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
parameter and skip the possibly expensive ahead/behind computation
between the branch and the upstream.

Long status also respects "status.aheadBehind" config setting.

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

diff --git a/builtin/checkout.c b/builtin/checkout.c
index fc4f8fd..655dac2 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, AHEAD_BEHIND_FULL))
 		return;
 	fputs(sb.buf, stdout);
 	strbuf_release(&sb);
diff --git a/remote.c b/remote.c
index 32706bc..bbbe3e5 100644
--- a/remote.c
+++ b/remote.c
@@ -2068,15 +2068,16 @@ 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;
+	int ours, theirs, sti;
 	const char *full_base;
 	char *base;
 	int upstream_is_gone = 0;
 
-	if (stat_tracking_info(branch, &ours, &theirs, &full_base,
-			       AHEAD_BEHIND_FULL) < 0) {
+	sti = stat_tracking_info(branch, &ours, &theirs, &full_base, abf);
+	if (sti < 0) {
 		if (!full_base)
 			return 0;
 		upstream_is_gone = 1;
@@ -2090,10 +2091,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 == AHEAD_BEHIND_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 27feb63..b2fa5cc 100644
--- a/remote.h
+++ b/remote.h
@@ -265,7 +265,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 053dff3..febf63f 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -173,6 +173,53 @@ test_expect_success 'status.aheadbehind=false status -s -b (diverged from upstre
 '
 
 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
+'
+
+test_expect_success 'status --long --branch' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=true status --long -b | head -3
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
+On branch b1
+Your branch and 'origin/master' refer to different commits.
+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
+'
+
+test_expect_success 'status.aheadbehind=false status --long --branch' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=false status --long -b | head -2
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
+cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
 
diff --git a/wt-status.c b/wt-status.c
index df6cc33..0a40b85 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1006,7 +1006,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->ahead_behind_flags))
 		return;
 
 	i = 0;
-- 
2.9.3

[PATCH v3 5/5] status: add status.aheadBehind value for porcelain output

[Jeff Hostetler]

From: Jeff Hostetler <jeffhost@microsoft.com>

Add status.aheadBehind=2 value to enable --no-ahead-behind
for all formats (both porcelain and non-porcelain).  The
current boolean values only affect non-porcelain formats.

Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
---
 Documentation/config.txt |  5 +++++
 builtin/commit.c         | 31 +++++++++++++++++++++++++------
 remote.h                 |  8 ++++++++
 t/t6040-tracking-info.sh |  9 +++++++++
 t/t7064-wtstatus-pv2.sh  |  6 +++++-
 5 files changed, 52 insertions(+), 7 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index affb0d6..eaa1058 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -3040,6 +3040,11 @@ status.aheadBehind::
 	--no-ahead-behind by default in linkgit:git-status[1] for
 	non-porcelain formats.  This setting is ignored by porcelain
 	formats for backwards compatibility.
++
+(EXPERIMENTAL) Set to 2 to allow both porcelain and non-porcelain
+formats to inherit --no-ahead-behind.  This may break backward
+compatibility for scripts using porcelain status formats and expecting
+ahead/behind information in the output.
 
 status.displayCommentPrefix::
 	If set to true, linkgit:git-status[1] will insert a comment
diff --git a/builtin/commit.c b/builtin/commit.c
index 416fe2c..194a6eb 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1109,13 +1109,32 @@ static const char *read_commit_message(const char *name)
 static struct status_deferred_config {
 	enum wt_status_format status_format;
 	int show_branch;
-	enum ahead_behind_flags ahead_behind;
+	enum ahead_behind_config_flags ahead_behind_config;
 } status_deferred_config = {
 	STATUS_FORMAT_UNSPECIFIED,
 	-1, /* unspecified */
 	AHEAD_BEHIND_UNSPECIFIED,
 };
 
+static inline enum ahead_behind_flags inherit_deferred_ab_flags(
+	int is_porcelain)
+{
+	switch (status_deferred_config.ahead_behind_config) {
+	case AHEAD_BEHIND_CONFIG_UNSPECIFIED:
+	case AHEAD_BEHIND_CONFIG_FULL:
+		return AHEAD_BEHIND_FULL;
+
+	case AHEAD_BEHIND_CONFIG_QUICK2:
+		return AHEAD_BEHIND_QUICK;
+
+	case AHEAD_BEHIND_CONFIG_QUICK:
+		return is_porcelain ? AHEAD_BEHIND_FULL : AHEAD_BEHIND_QUICK;
+
+	default: /* don't complain about bogus config settings */
+		return AHEAD_BEHIND_FULL;
+	}
+}
+
 static void finalize_deferred_config(struct wt_status *s)
 {
 	int use_deferred_config = (status_format != STATUS_FORMAT_PORCELAIN &&
@@ -1140,11 +1159,9 @@ static void finalize_deferred_config(struct wt_status *s)
 	if (s->show_branch < 0)
 		s->show_branch = 0;
 
-	if (use_deferred_config &&
-	    s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
-		s->ahead_behind_flags = status_deferred_config.ahead_behind;
 	if (s->ahead_behind_flags == AHEAD_BEHIND_UNSPECIFIED)
-		s->ahead_behind_flags = AHEAD_BEHIND_FULL;
+		s->ahead_behind_flags =
+			inherit_deferred_ab_flags(!use_deferred_config);
 }
 
 static int parse_and_validate_options(int argc, const char *argv[],
@@ -1306,7 +1323,9 @@ static int git_status_config(const char *k, const char *v, void *cb)
 		return 0;
 	}
 	if (!strcmp(k, "status.aheadbehind")) {
-		status_deferred_config.ahead_behind = git_config_bool(k, v);
+		int is_bool;
+		status_deferred_config.ahead_behind_config =
+			git_config_bool_or_int(k, v, &is_bool);
 		return 0;
 	}
 	if (!strcmp(k, "status.showstash")) {
diff --git a/remote.h b/remote.h
index b2fa5cc..bcf846a 100644
--- a/remote.h
+++ b/remote.h
@@ -262,6 +262,14 @@ enum ahead_behind_flags {
 	AHEAD_BEHIND_FULL        =  1,  /* traditional a/b reporting */
 };
 
+/* Flags for status.aheadBehind values. */
+enum ahead_behind_config_flags {
+	AHEAD_BEHIND_CONFIG_UNSPECIFIED = -1,
+	AHEAD_BEHIND_CONFIG_QUICK       =  0, /* eq/neq for non-porcelain only */
+	AHEAD_BEHIND_CONFIG_FULL        =  1, /* a/b reporting for all formats */
+	AHEAD_BEHIND_CONFIG_QUICK2      =  2, /* eq/neq for all formats */
+};
+
 /* 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);
diff --git a/t/t6040-tracking-info.sh b/t/t6040-tracking-info.sh
index febf63f..5003366 100755
--- a/t/t6040-tracking-info.sh
+++ b/t/t6040-tracking-info.sh
@@ -219,6 +219,15 @@ test_expect_success 'status.aheadbehind=false status --long --branch' '
 	test_i18ncmp expect actual
 '
 
+test_expect_success 'status.aheadbehind=2 status --long --branch' '
+	(
+		cd test &&
+		git checkout b1 >/dev/null &&
+		git -c status.aheadbehind=false status --long -b | head -2
+	) >actual &&
+	test_i18ncmp expect actual
+'
+
 cat >expect <<\EOF
 ## b5...brokenbase [gone]
 EOF
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 67b90cd..7171a43 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -410,6 +410,10 @@ test_expect_success 'verify --[no-]ahead-behind with V2 format' '
 		git status --no-ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual &&
 
+		# Confirm that porcelain=v2 format inherits status.aheadBehind value when _QUICK2.
+		git -c status.aheadbehind=2 status --porcelain=v2 --branch --untracked-files=all >actual &&
+		test_cmp expect actual &&
+
 		# Confirm --ahead-behind reports traditional branch.ab with 0/0.
 		cat >expect <<-EOF &&
 		# branch.oid $HUF
@@ -421,7 +425,7 @@ test_expect_success 'verify --[no-]ahead-behind with V2 format' '
 		git status --ahead-behind --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual &&
 
-		# Confirm that porcelain=v2 format does not inherit status.aheadBehind value.
+		# Confirm that porcelain=v2 format does not inherit status.aheadBehind value when _FULL/_QUICK.
 		git -c status.aheadbehind=false status --porcelain=v2 --branch --untracked-files=all >actual &&
 		test_cmp expect actual &&
 
-- 
2.9.3

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

[Jeff King]

On Wed, Jan 03, 2018 at 09:47:28PM +0000, Jeff Hostetler wrote:

> Config values of true and false control non-porcelain formats
> for compatibility reasons as previously discussed.  In the
> last commit I added a new value of 2 for the config setting
> to allow porcelain formats to inherit the new setting.  I've
> marked this experimental for now or so that we can discuss
> it.

I'm mildly negative on this "level 2" config. If influencing the
porcelain via config creates compatibility headaches, then why would we
allow it here? And if it doesn't, then why do we need to protect against
it? This seems to exist in a funny middle ground that cannot decide
whether it is bad or not.

It's like we're inserting a foot-gun, but putting it just far enough out
of reach that we can blame the user when they shoot themselves with it.

Is there a compelling use case for this? From the previous discussion,
this is the strawman I came up with:

  Scripted callers like Visual Studio don't want to unconditionally pass
  --no-ahead-behind, because it makes sense only for large repositories
  (and small ones would prefer the more exact answer, if we can get it
  cheaply). So we'd like the user to trigger "this is large" on a
  per-repo basis, and accept the consequences of possibly broken
  porcelain callers.

I think we could have the best of both worlds, though, if the existing
config option were coupled with a command-line option to say "yes, I
understand no-ahead-behind, so use it for the porcelain if applicable".
IOW, the user does:

  git config status.aheadbehind false

and VS does:

  git status --ahead-behind=maybe

and together both sides have assented to the "quick" thing.

-Peff

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

[Jeff Hostetler]

On 1/4/2018 6:06 PM, Jeff King wrote:
> On Wed, Jan 03, 2018 at 09:47:28PM +0000, Jeff Hostetler wrote:
> 
>> Config values of true and false control non-porcelain formats
>> for compatibility reasons as previously discussed.  In the
>> last commit I added a new value of 2 for the config setting
>> to allow porcelain formats to inherit the new setting.  I've
>> marked this experimental for now or so that we can discuss
>> it.
> 
> I'm mildly negative on this "level 2" config. If influencing the
> porcelain via config creates compatibility headaches, then why would we
> allow it here? And if it doesn't, then why do we need to protect against
> it? This seems to exist in a funny middle ground that cannot decide
> whether it is bad or not.
> 
> It's like we're inserting a foot-gun, but putting it just far enough out
> of reach that we can blame the user when they shoot themselves with it.
> 
> Is there a compelling use case for this? From the previous discussion,
> this is the strawman I came up with:
> 
>    Scripted callers like Visual Studio don't want to unconditionally pass
>    --no-ahead-behind, because it makes sense only for large repositories
>    (and small ones would prefer the more exact answer, if we can get it
>    cheaply). So we'd like the user to trigger "this is large" on a
>    per-repo basis, and accept the consequences of possibly broken
>    porcelain callers.
> 
> I think we could have the best of both worlds, though, if the existing
> config option were coupled with a command-line option to say "yes, I
> understand no-ahead-behind, so use it for the porcelain if applicable".
> IOW, the user does:
> 
>    git config status.aheadbehind false
> 
> and VS does:
> 
>    git status --ahead-behind=maybe
> 
> and together both sides have assented to the "quick" thing.
> 
> -Peff

I kinda trying to serve 2 masters here.  As we discussed earlier, we
don't want config options to change porcelain formats, hence the
true/false thing only affecting non-porcelain formats.  On the other
hand, VS and git.exe are on different release schedules.  Normally,
I'd just have VS emit a "git status --no-ahead-behind --porcelain=v2"
and be done, but that requires that git.exe gets updated before VS.
We do control some of that, but if VS gets updated first, that causes
an error, whereas "git -c status.aheadbehind=<x> status --porcelain=v2"
does not.  It is respected if/when git is updated and ignored until
then.  Likewise, if they update git first, we can tell them to set a
config setting on the repo and inherit it for porcelain v2 output
without VS knowing about it.  Sorry, if that's too much detail.

It is OK with me if we omit the last commit in the patch series (that
does the experimental =2 extension) and I'll deal with this separately
(maybe differently) in the gvfs fork.

Thanks,
Jeff

  

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

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

> I kinda trying to serve 2 masters here.  As we discussed earlier, we
> don't want config options to change porcelain formats, hence the
> true/false thing only affecting non-porcelain formats.  On the other
> hand, VS and git.exe are on different release schedules.  Normally,
> I'd just have VS emit a "git status --no-ahead-behind --porcelain=v2"
> and be done, but that requires that git.exe gets updated before VS.
> We do control some of that, but if VS gets updated first, that causes
> an error, whereas "git -c status.aheadbehind=<x> status --porcelain=v2"
> does not.  It is respected if/when git is updated and ignored until
> then.  Likewise, if they update git first, we can tell them to set a
> config setting on the repo and inherit it for porcelain v2 output
> without VS knowing about it.  Sorry, if that's too much detail.

That is not really too much detail.  

But if the above is your plan, then boolean=2 cannot merely be "an
experimental" as described in [5/5]; it needs to be carried for some
extended period of time, depending on the release schedule of the
other half of the coin.

> It is OK with me if we omit the last commit in the patch series (that
> does the experimental =2 extension) and I'll deal with this separately
> (maybe differently) in the gvfs fork.

I think that sounds more sensible.  Thanks.

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

[Jeff King]

On Fri, Jan 05, 2018 at 11:46:24AM -0500, Jeff Hostetler wrote:

> > I'm mildly negative on this "level 2" config. If influencing the
> > porcelain via config creates compatibility headaches, then why would we
> > allow it here? And if it doesn't, then why do we need to protect against
> > it? This seems to exist in a funny middle ground that cannot decide
> > whether it is bad or not.
> > 
> > It's like we're inserting a foot-gun, but putting it just far enough out
> > of reach that we can blame the user when they shoot themselves with it.
> > 
> > Is there a compelling use case for this? From the previous discussion,
> > this is the strawman I came up with:
> [...]
> 
> I kinda trying to serve 2 masters here.  As we discussed earlier, we
> don't want config options to change porcelain formats, hence the
> true/false thing only affecting non-porcelain formats.  On the other
> hand, VS and git.exe are on different release schedules.  Normally,
> I'd just have VS emit a "git status --no-ahead-behind --porcelain=v2"
> and be done, but that requires that git.exe gets updated before VS.
> We do control some of that, but if VS gets updated first, that causes
> an error, whereas "git -c status.aheadbehind=<x> status --porcelain=v2"
> does not.  It is respected if/when git is updated and ignored until
> then.  Likewise, if they update git first, we can tell them to set a
> config setting on the repo and inherit it for porcelain v2 output
> without VS knowing about it.  Sorry, if that's too much detail.

OK, so my strawman was totally off-base. :)

That explanation is interesting. I do think you're facing a real problem
with moving the versions in lockstep. But shoe-horning the feature into
config like this seems like a pretty ugly solution:

  1. Then we're stuck with this weird foot-gun config option forever.

  2. It only solves the problem for this one option. Is there a more
     general solution?

The more general solutions I can come up with are:

  1. Is there a way for a caller to query Git to say "do you understand
     --no-ahead-behind?".

     You can ask "git version", but parsing version numbers is
     problematic. We don't have any kind of "feature flags" output, and
     I'm not sure we'd want to get down to the level of specific
     command-line options there.

     One thing you can do is speculatively run "status --no-ahead-behind",
     and if it returns 129, try again without as a fallback. That incurs
     a failed invocation for the fallback case, but it's quick (we fail
     before looking at any data) and you can cache it for the duration
     of a VS session.

  2. There could be some way to tell the option parser that the next
     flag is optional. E.g.:

       git status --optional=--no-ahead-behind

     That would be pretty easy to implement globally in parse-options.c.
     It knows when an option is unrecognized, so it could just treat
     that as a silent noop rather than barfing.

     Of course, that doesn't solve your problem today. It wouldn't be
     safe to start using "--optional" until it had been in several
     released versions.

     And I have a feeling it may not be sufficient without further
     feedback to the caller. For this flag, the caller is happy to say
     "do this if you know how, but otherwise I will cope". But there are
     probably flag where it would need to know whether it had any effect
     or not. So this whole direction is probably crazy.

Of the two, I think (1) is not so bad.

> It is OK with me if we omit the last commit in the patch series (that
> does the experimental =2 extension) and I'll deal with this separately
> (maybe differently) in the gvfs fork.

That would be my preference. Thanks.

-Peff

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

[Jeff Hostetler]

On 1/8/2018 1:37 AM, Jeff King wrote:
> On Fri, Jan 05, 2018 at 11:46:24AM -0500, Jeff Hostetler wrote:
> 
>>> I'm mildly negative on this "level 2" config. If influencing the
>>> porcelain via config creates compatibility headaches, then why would we
>>> allow it here? And if it doesn't, then why do we need to protect against
>>> it? This seems to exist in a funny middle ground that cannot decide
>>> whether it is bad or not.
>>>
>>> It's like we're inserting a foot-gun, but putting it just far enough out
>>> of reach that we can blame the user when they shoot themselves with it.
>>>
>>> Is there a compelling use case for this? From the previous discussion,
>>> this is the strawman I came up with:
>> [...]
>>
>> I kinda trying to serve 2 masters here.  As we discussed earlier, we
>> don't want config options to change porcelain formats, hence the
>> true/false thing only affecting non-porcelain formats.  On the other
>> hand, VS and git.exe are on different release schedules.  Normally,
>> I'd just have VS emit a "git status --no-ahead-behind --porcelain=v2"
>> and be done, but that requires that git.exe gets updated before VS.
>> We do control some of that, but if VS gets updated first, that causes
>> an error, whereas "git -c status.aheadbehind=<x> status --porcelain=v2"
>> does not.  It is respected if/when git is updated and ignored until
>> then.  Likewise, if they update git first, we can tell them to set a
>> config setting on the repo and inherit it for porcelain v2 output
>> without VS knowing about it.  Sorry, if that's too much detail.
> 
> OK, so my strawman was totally off-base. :)
> 
> That explanation is interesting. I do think you're facing a real problem
> with moving the versions in lockstep. But shoe-horning the feature into
> config like this seems like a pretty ugly solution:
> 
>    1. Then we're stuck with this weird foot-gun config option forever.
> 
>    2. It only solves the problem for this one option. Is there a more
>       general solution?
> 
> The more general solutions I can come up with are:
> 
>    1. Is there a way for a caller to query Git to say "do you understand
>       --no-ahead-behind?".
> 
>       You can ask "git version", but parsing version numbers is
>       problematic. We don't have any kind of "feature flags" output, and
>       I'm not sure we'd want to get down to the level of specific
>       command-line options there.
> 
>       One thing you can do is speculatively run "status --no-ahead-behind",
>       and if it returns 129, try again without as a fallback. That incurs
>       a failed invocation for the fallback case, but it's quick (we fail
>       before looking at any data) and you can cache it for the duration
>       of a VS session.
> 
>    2. There could be some way to tell the option parser that the next
>       flag is optional. E.g.:
> 
>         git status --optional=--no-ahead-behind
> 
>       That would be pretty easy to implement globally in parse-options.c.
>       It knows when an option is unrecognized, so it could just treat
>       that as a silent noop rather than barfing.
> 
>       Of course, that doesn't solve your problem today. It wouldn't be
>       safe to start using "--optional" until it had been in several
>       released versions.
> 
>       And I have a feeling it may not be sufficient without further
>       feedback to the caller. For this flag, the caller is happy to say
>       "do this if you know how, but otherwise I will cope". But there are
>       probably flag where it would need to know whether it had any effect
>       or not. So this whole direction is probably crazy.
> 
> Of the two, I think (1) is not so bad.
> 
>> It is OK with me if we omit the last commit in the patch series (that
>> does the experimental =2 extension) and I'll deal with this separately
>> (maybe differently) in the gvfs fork.
> 
> That would be my preference. Thanks.
> 
> -Peff
> 

Interesting ideas, but probably overkill for now.  I'll pull it out
of my next version and deal with it differently our gvfs fork.

Thanks,
Jeff