Cache Git commands related to submodules

[gerhardol]

Part of #8862

Proposed changes

• Cache Git commands related to submodules
  Cache commands with sha input and no variable input like Git Notes
  Only fetch Git Notes for commands that are not cached (Notes are not displayed in Submodule scenarios anyway).

• Cache GetCommitRangeDiffCount
  Used when selecting multiple commits in RevDiff

• Increase Git command cache to 50
  Not strictly needed, 40 is OK also for my use after this, but it seems like a reasonable change.

Screenshots

Example with one submodule changed, on a relative fast PC (Git at work is 5 times slower).

Startup followed by refresh (similar to build), marked with red line.
Commands that are cached are marked with green (so cache have effect already at first display).

Note: The refresh was done a little early, a diff command will normally be required also after the update, but for current master four more commands would be required.

Before

After

Test methodology

review and manual

---

✒️ I contribute this code under The Developer Certificate of Origin.

[codecov]

Codecov Report

Merging #8866 (be1893d) into master (e77f8c3) will decrease coverage by 0.02%.
The diff coverage is 45.00%.

@@            Coverage Diff             @@
##           master    #8866      +/-   ##
==========================================
- Coverage   56.20%   56.18%   -0.03%     
==========================================
  Files         921      921              
  Lines       65591    65593       +2     
  Branches    11991    11995       +4     
==========================================
- Hits        36867    36853      -14     
- Misses      25737    25742       +5     
- Partials     2987     2998      +11     

Flag	Coverage Δ
production	43.35% <40.00%> (-0.04%)	⬇️
tests	94.82% <ø> (-0.02%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

[ghost]

How about this option?
To separate by parameters.

var output = cache switch
{
    true => _gitExecutable.GetOutput(args, cache: GitCommandCache),
    false => _gitExecutable.GetOutput(args)
};

[gerhardol]

For this PR, I suggest to follow the current code style (i.e. not change).

But this could be changed in general.
I prefer the current ?: syntax, of course partly because I am used to that but I see no benefit with the switch here.
The current is easier to read for me and less duplicated code. Long commands where options wrap over several lines can be hard to compare.
This scales better when there are more options affecting a command too.

[ghost]

This scales better when there are more options affecting a command too.

Could you please to show example there is you mean.

[gerhardol]

For this specific option, there is no additional ?:, but other usages could have

            return _gitExecutable.GetOutput(
                new GitArgumentBuilder("diff")
                {
                    "--no-color",
                    "-M -C",
                    "--name-status",
                    { nullSeparated, "-z" },
                    _revisionDiffProvider.Get(firstRevision, secondRevision)
                },
                cache: noCache ? null : GitCommandCache);

This could be split, but then I prefer the following:

            var args = new GitArgumentBuilder("diff")
            {
                "--no-color",
                extraDiffArguments,
                { AppSettings.UseHistogramDiffAlgorithm, "--histogram" },
                "-M -C",
                diffOptions
            };

            var cache = cacheResult &&
                        !string.IsNullOrEmpty(secondRevision) &&
                        !string.IsNullOrEmpty(firstRevision) &&
                        !secondRevision.IsArtificial() &&
                        !firstRevision.IsArtificial()
                ? GitCommandCache
                : null;

            var patch = _gitExecutable.GetOutput(
                args,
                cache: cache,
                outputEncoding: LosslessEncoding);

[ghost]

In the case of the switch, the state machine is clearer.
And the appearance of "null" can signal a potential error.

[mstv]

👍

[gerhardol]

@msftbot merge in 72 hours

[ghost]

Hello @gerhardol!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

• I won't merge this pull request until after the UTC date Tue, 23 Feb 2021 23:52:19 GMT, which is in 3 days

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

[gerhardol]

Will of course squash before merge, will create a PR for 3.5 too

[gerhardol]

One fixup, text summary for new submodules could be cached
(common scenario: submodule exists in one branch but not next checked out branch without deleting the submodule directory).