RangeDiff: Commit count in name #8628

gerhardol · 2020-11-21T19:55:31Z

Proposed changes

#8516 etc added git-range-diff. After some usage, I would like to change the presentation a little.

Currently the first and second commits are presented. I have found that I often want to know the commit count for first/second to the merge-base, similar to ahead/behind info. The first/second is (normally, see below) listed anyway in other groups and the second commit is not often seen as as the group name is too long.

If one of the first/second selected is artificial, the range diff uses HEAD. This is not listed in the other group names, but should be obvious.
To avoid a second call to git-rev-list (adding about 40 ms on my PC), a modified git-rev-list call was added.

Screenshots

Before

FileStatusList must be wide to see the second revision

After

Test methodology

Manual

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

codecov · 2020-11-21T20:11:54Z

Codecov Report

Merging #8628 (9bdef2b) into master (e0d47d8) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #8628      +/-   ##
==========================================
- Coverage   55.11%   55.10%   -0.02%     
==========================================
  Files         906      906              
  Lines       65550    65562      +12     
  Branches    11872    11874       +2     
==========================================
- Hits        36130    36129       -1     
- Misses      26589    26600      +11     
- Partials     2831     2833       +2

Flag	Coverage Δ
production	`42.10% <0.00%> (-0.02%)`	⬇️
tests	`94.91% <ø> (ø)`

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

RussKie

The actual implementation LGTM with a comment

RussKie · 2020-11-22T11:59:00Z

GitUI/UserControls/FileStatusDiffCalculator.cs

            var first = firstRev.ObjectId == firstRevHead ? firstRev : new GitRevision(firstRevHead);
            var selected = selectedRev.ObjectId == selectedRevHead ? selectedRev : new GitRevision(selectedRevHead);
+            var (baseToFirstCount, baseToSecondCount) = module.GetCommitRangeDiffCount(first.ObjectId, selected.ObjectId);


module should really be IGitModule, and thus the changes to the API should be exposed at the interface level.
This aligns with @ivangrek's work in #8574.

There are some more methods needed for that to happen.
Also, I would prefer to not share this with plugins. If so, the methods should change to strings instead of ObjectId (as there may be situations where treeish identifiers like HEAD could be used.)

Severity Code Description Project File Line Suppression State Error CS1061 'IGitModule' does not contain a definition for 'GetTreeFiles' and no accessible extension method 'GetTreeFiles' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 40 Active Error CS1061 'IGitModule' does not contain a definition for 'GetCombinedDiffFileList' and no accessible extension method 'GetCombinedDiffFileList' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 61 Active Error CS1061 'IGitModule' does not contain a definition for 'GetCommitDiffCount' and no accessible extension method 'GetCommitDiffCount' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 191 Active Error CS1061 'IGitModule' does not contain a definition for 'GetCommitDiffCount' and no accessible extension method 'GetCommitDiffCount' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 192 Active Error CS1061 'IGitModule' does not contain a definition for 'GetCommitRangeDiffCount' and no accessible extension method 'GetCommitRangeDiffCount' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 173 Active Error CS1061 'IGitModule' does not contain a definition for 'GetDiffFilesWithSubmodulesStatus' and no accessible extension method 'GetDiffFilesWithSubmodulesStatus' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 54 Active Error CS1061 'IGitModule' does not contain a definition for 'GetDiffFilesWithSubmodulesStatus' and no accessible extension method 'GetDiffFilesWithSubmodulesStatus' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 87 Active Error CS1061 'IGitModule' does not contain a definition for 'GetDiffFilesWithSubmodulesStatus' and no accessible extension method 'GetDiffFilesWithSubmodulesStatus' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 138 Active Error CS1061 'IGitModule' does not contain a definition for 'GetDiffFilesWithSubmodulesStatus' and no accessible extension method 'GetDiffFilesWithSubmodulesStatus' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 147 Active Error CS1061 'IGitModule' does not contain a definition for 'GetDiffFilesWithSubmodulesStatus' and no accessible extension method 'GetDiffFilesWithSubmodulesStatus' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 148 Active Error CS1061 'IGitModule' does not contain a definition for 'GetMergeBase' and no accessible extension method 'GetMergeBase' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 103 Active Error CS1061 'IGitModule' does not contain a definition for 'GetMergeBase' and no accessible extension method 'GetMergeBase' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 108 Active Error CS1061 'IGitModule' does not contain a definition for 'GetMergeBase' and no accessible extension method 'GetMergeBase' accepting a first argument of type 'IGitModule' could be found (are you missing a using directive or an assembly reference?) GitUI F:\dev\gc\gitextensions\GitUI\UserControls\FileStatusDiffCalculator.cs 111 Active

This is something we'll need to fix going forward.

mstv · 2020-11-24T23:18:35Z

I can follow the reasoning for this change.
Just curious: What is such interesting in how many commits a branch is ahead & behind? I just want to know whether it is ahead & behind. BTW: "(gone)" as text is distracting.
range-diff is a little special because of its limitations (max. distance in commits; commits must match, i.e. not usable for squashed branches -- or have I missed it?)
So finally, I don't have objections.

mstv · 2020-11-24T23:23:04Z

GitCommands/Git/GitModule.cs

+            var output = _gitExecutable.GetOutput(args);
+
+            var counts = output.Split('\t');
+            if (counts.Length == 2 && int.TryParse(counts[0], out var first) && int.TryParse(counts[1], out var second))


This is another reason for adding a generic detection of git error and warning messages to GetOutput etc.

How would it help?

mstv · 2020-11-24T23:27:03Z

GitUI/UserControls/FileStatusDiffCalculator.cs

            var first = firstRev.ObjectId == firstRevHead ? firstRev : new GitRevision(firstRevHead);
            var selected = selectedRev.ObjectId == selectedRevHead ? selectedRev : new GitRevision(selectedRevHead);
+            var (baseToFirstCount, baseToSecondCount) = module.GetCommitRangeDiffCount(first.ObjectId, selected.ObjectId);
+            const int maxRangeDiffCommits = 100;
+            var desc = $"{Strings.DiffRange} {baseToFirstCount ?? maxRangeDiffCommits + 1}↓ {baseToSecondCount ?? maxRangeDiffCommits + 1}↑";


"100+" or ">100" instead of "101"?

changed to angeDiffCommitLimit

RussKie · 2020-11-25T03:56:20Z

100+ has my vote

gerhardol · 2020-11-25T22:05:57Z

Just curious: What is such interesting in how many commits a branch is ahead & behind? I just want to know whether it is ahead & behind.

To some extent, this change was initiated by that First->Second is too long and therefore not really useful.
The exact number is not so interesting, but to see where the branches are attached. Like if my local branch is about 4 commits and I compare to another branch with about 4 commits and the count is 8-9, there are other commits involved to, if it is 4-4 there are likely no other commits involved.

BTW: "(gone)" as text is distracting.

I agree but this is Git name. May be changed to "-".

range-diff is a little special because of its limitations (max. distance in commits; commits must match, i.e. not usable for squashed branches -- or have I missed it?)

Correct, the actual diff is hard to pretend (and even not always correct).
But for rebased branches it is very useful: Has someone else changed anything when rebasing, was the rebase correct?
I often have branches with 30 commits for review that I rearrange before creating a PR, with range-diff I get a better overview.

ghost · 2020-11-26T16:38:46Z

GitCommands/Git/GitModule.cs

+            var args = new GitArgumentBuilder("rev-list")
+            {
+                $"{firstId}...{secondId}",
+                "--count",
+                "--left-right"
+            };


Could we use constants for these commands?

public static class GitCommands { public static class RevList { public const string Command = "rev-list"; public const string CountParameter = "--count"; public const string LeftRightParameter = "--left-right"; } }

or

public static class GitCommands { public const string RevList = "rev-list"; public static class RevListParameters { public const string Count = "--count"; public const string LeftRight = "--left-right"; } }

Most likely there will be many common parameters.

If this is changed, it should be a generic change for all commands, not in this PR.

Personally I see just one reason to make constants for commands: It is possible to search where they are used.
"rev-list" is unique though, but "log" is not.

Other than that, for Git commands you get down to a text string anyway. Using constants will just add another layer when viewing the code.

Other than that, I would like to merge this.
Changes to IGitModule will affect more commands than GetCommitRangeDiffCount, like GetDiffFilesWithSubmodulesStatus.

Personally I see just one reason to make constants for commands: It is possible to search where they are used.
"rev-list" is unique though, but "log" is not.

The search itself is important.

I use the rule: "Everything that can be calculated at the stage of creating a program must be calculated."
Example:

Other than that, for Git commands you get down to a text string anyway. Using constants will just add another layer when viewing the code.

And that's the key.
Why not use it like that?

var args = new GitArgumentBuilder($"rev-list {firstId}...{secondId} --count --left-right");

Or so?

var args = $"rev-list {firstId}...{secondId} --count --left-right";

I think we could find this door if we looked, the key is already in our hands. 😉

Other than that, I would like to merge this.

Well, Ok.

Why not use it like that?

The Git command (like "rev-list") is a hook for options etc so that must be separate.
The (generic) argument builder handles some types native and simplifies use of booleans etc.

Another layer will require me too look up the constant when browsing the code which is not an advantage to me.

var args = new GitArgumentBuilder("rev-list") { "--count", "--left-right" }; var args = new GitArgumentBuilder(GitCommands.RevList) { GitCommands.RevListParameters.Count, GitCommands.RevListParameters.LeftRight };

I understand correctly, do you think that the second method may require additional actions?

I understand correctly, do you think that the second method may require additional actions?

No additional actions, but I do not see the actual commands at a glance.

RussKie · 2020-11-28T01:11:41Z

@gerhardol squash and merge

Commit hash and branch names are too long to read

ghost assigned gerhardol Nov 21, 2020

RussKie reviewed Nov 22, 2020

View reviewed changes

ghost mentioned this pull request Nov 22, 2020

Cleanup the plugin interface #8574

Closed

gerhardol requested a review from mstv November 22, 2020 15:58

mstv approved these changes Nov 24, 2020

View reviewed changes

gerhardol force-pushed the feature/rangediff-commitcount branch from b3cbe6d to 1e94bca Compare November 25, 2020 21:33

ghost reviewed Nov 26, 2020

View reviewed changes

RussKie approved these changes Nov 28, 2020

View reviewed changes

RussKie added the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Nov 28, 2020

RangeDiff: Commit count in name

9bdef2b

Commit hash and branch names are too long to read

gerhardol force-pushed the feature/rangediff-commitcount branch from 1e94bca to 9bdef2b Compare November 28, 2020 09:47

ghost removed the 📭 needs: author feedback More info/confirmation awaited from OP; issues typically get closed after 30 days of inactivity label Nov 28, 2020

gerhardol merged commit 5996dc9 into gitextensions:master Nov 28, 2020

ghost added this to the 4.0 milestone Nov 28, 2020

gerhardol deleted the feature/rangediff-commitcount branch November 28, 2020 10:11

gerhardol mentioned this pull request Nov 28, 2020

Fix rebase status displayed during a rebase #8638

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RangeDiff: Commit count in name #8628

RangeDiff: Commit count in name #8628

gerhardol commented Nov 21, 2020 •

edited

codecov bot commented Nov 21, 2020 •

edited

RussKie left a comment

RussKie Nov 22, 2020

gerhardol Nov 22, 2020

RussKie Nov 28, 2020

mstv commented Nov 24, 2020

mstv Nov 24, 2020

RussKie Nov 28, 2020

mstv Nov 24, 2020

gerhardol Nov 25, 2020

RussKie commented Nov 25, 2020 via email •

edited by ghost

gerhardol commented Nov 25, 2020

ghost Nov 26, 2020

gerhardol Nov 26, 2020

ghost Nov 26, 2020

gerhardol Nov 27, 2020

ghost Nov 27, 2020 •

edited by ghost

gerhardol Nov 27, 2020

RussKie commented Nov 28, 2020

RangeDiff: Commit count in name #8628

RangeDiff: Commit count in name #8628

Conversation

gerhardol commented Nov 21, 2020 • edited

Proposed changes

Screenshots

Before

After

Test methodology

codecov bot commented Nov 21, 2020 • edited

Codecov Report

RussKie left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mstv commented Nov 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Nov 25, 2020 via email • edited by ghost

gerhardol commented Nov 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Nov 27, 2020 • edited by ghost

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RussKie commented Nov 28, 2020

gerhardol commented Nov 21, 2020 •

edited

codecov bot commented Nov 21, 2020 •

edited

RussKie commented Nov 25, 2020 via email •

edited by ghost

ghost Nov 27, 2020 •

edited by ghost