threads / patch / 47478

v2, 5 partsAdd --no-ahead-behind to status

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

## tl;dr

20 messages between Dec 21, 2017 and Apr 3, 2018. Diffs are folded; open one to read it.

replies: 19people: 8as markdown or json

Jeff Hostetler· Dec 21, 2017, 19:09 UTC · lore
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
Jeff Hostetler· Dec 21, 2017, 19:09 UTC · re: Jeff Hostetler · lore

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

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(+)
Show changes to 4 files +15 −0

Documentation/config.txt, cache.h, config.c, environment.c

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
Igor Djordjevic· Dec 21, 2017, 20:21 UTC · re: Jeff Hostetler · lore

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

Hi Jeff,
On 21/12/2017 20:09, Jeff Hostetler wrote:
Show 10 quoted lines
> 
> 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").
Show 10 quoted lines
> +	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.
Jonathan Nieder· Dec 21, 2017, 20:43 UTC · re: Jeff Hostetler · lore

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

Hi,
Jeff Hostetler wrote:
Show 13 quoted lines
> 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.

[...]
Show 15 quoted lines
> 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

Junio C Hamano· Dec 22, 2017, 18:21 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 45 quoted lines
>> 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.
Jeff King· Dec 24, 2017, 14:33 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 22, 2017 at 10:21:23AM -0800, Junio C Hamano wrote:
Show 16 quoted lines
> >> +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?

Show 6 quoted lines
> > 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
Junio C Hamano· Dec 27, 2017, 17:41 UTC · re: Jeff King · lore

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

Jeff King <peff@peff.net> writes:
Show 9 quoted lines
> 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.

Jeff King· Jan 4, 2018, 19:26 UTC · re: Junio C Hamano · lore

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

On Wed, Dec 27, 2017 at 09:41:30AM -0800, Junio C Hamano wrote:
Show 14 quoted lines
> 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.

Show 5 quoted lines
> 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
Lars Schneider· Apr 3, 2018, 09:54 UTC · re: Jeff King · lore

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

Show 48 quoted lines
> 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
Ævar Arnfjörð Bjarmason· Apr 3, 2018, 10:18 UTC · re: Lars Schneider · lore

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

On Tue, Apr 03 2018, Lars Schneider wrote:
Show 52 quoted lines
>> 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.

Derrick Stolee· Apr 3, 2018, 11:39 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 4/3/2018 6:18 AM, Ævar Arnfjörð Bjarmason wrote:
Show 7 quoted lines
> 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.

Show 5 quoted lines
> 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
Jeff Hostetler· Apr 3, 2018, 13:47 UTC · re: Derrick Stolee · lore

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

On 4/3/2018 7:39 AM, Derrick Stolee wrote:
Show 18 quoted lines
> 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

Jeff Hostetler· Jan 2, 2018, 21:54 UTC · re: Jonathan Nieder · lore

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

On 12/21/2017 3:43 PM, Jonathan Nieder wrote:
Show 28 quoted lines
> 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.

Show 21 quoted lines
> 
> [...]
>> 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

Jonathan Nieder· Jan 2, 2018, 22:17 UTC · re: Jeff Hostetler · lore

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

Hi,
Jeff Hostetler wrote:
> On 12/21/2017 3:43 PM, Jonathan Nieder wrote:
Show 12 quoted lines
>> 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

Jeff Hostetler· Dec 21, 2017, 19:09 UTC · re: Jeff Hostetler · lore

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

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(-)
Show changes to 5 files +109 −7

Documentation/git-status.txt, builtin/commit.c, t/t7064-wtstatus-pv2.sh, wt-status.c, wt-status.h

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
Jonathan Nieder· Dec 21, 2017, 20:57 UTC · re: Jeff Hostetler · lore

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

Hi,
Jeff Hostetler wrote:
Show 7 quoted lines
> --- 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.
[...]
Show 7 quoted lines
> @@ -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.

[...]
Show 9 quoted lines
> @@ -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/
Show 8 quoted lines
> +	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
[...]
Show 16 quoted lines
> --- 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

Jeff Hostetler· Dec 21, 2017, 19:09 UTC · re: Jeff Hostetler · lore

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

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(-)
Show changes to 2 files +19 −3

t/t6040-tracking-info.sh, wt-status.c

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
Jeff Hostetler· Dec 21, 2017, 19:09 UTC · re: Jeff Hostetler · lore

[PATCH v2 5/5] status: support --no-ahead-behind in long format

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(-)
Show changes to 5 files +46 −6

builtin/checkout.c, remote.c, remote.h, t/t6040-tracking-info.sh, wt-status.c

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
Jeff Hostetler· Dec 21, 2017, 19:09 UTC · re: Jeff Hostetler · lore

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

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(-)
Show changes to 4 files +29 −15

ref-filter.c, remote.c, remote.h, wt-status.c

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
Jonathan Nieder· Dec 21, 2017, 20:48 UTC · re: Jeff Hostetler · lore

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

Jeff Hostetler wrote:
Show 8 quoted lines
> --- 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?
[...]
Show 14 quoted lines
> --- 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.

[...]
Show 6 quoted lines
> @@ -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

← back to recent threads