Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

RangeDiff: Commit count in name #8628

Merged
merged 1 commit into from
Nov 28, 2020
Merged

RangeDiff: Commit count in name #8628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
23 changes: 21 additions & 2 deletions GitCommands/Git/GitModule.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -818,11 +818,11 @@ private IGitItem GetSubmoduleCommitHash(string filename, string refName)
return null;
}

public int? GetCommitDiffCount(string parentHash, string childHash)
public int? GetCommitDiffCount(ObjectId baseId, ObjectId parentId)
{
var args = new GitArgumentBuilder("rev-list")
{
$"{parentHash}...{childHash}",
$"{baseId} {parentId}",
"--count"
};
var output = _gitExecutable.GetOutput(args);
Expand All @@ -835,6 +835,25 @@ private IGitItem GetSubmoduleCommitHash(string filename, string refName)
return null;
}

public (int? first, int? second) GetCommitRangeDiffCount(ObjectId firstId, ObjectId secondId)
{
var args = new GitArgumentBuilder("rev-list")
{
$"{firstId}...{secondId}",
"--count",
"--left-right"
};
Comment on lines +840 to +845
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

image

image

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@ghost ghost Nov 27, 2020 •
edited by ghost

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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))
Comment on lines +846 to +849
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How would it help?

{
return (first, second);
}

return (null, null);
}

public string GetCommitCountString(string from, string to)
{
int? removed = GetCommitCount(from, to);
Expand Down
31 changes: 16 additions & 15 deletions GitUI/UserControls/FileStatusDiffCalculator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,12 +168,13 @@ public FileStatusDiffCalculator(Func<GitModule> getModule)
statuses: commonBaseToAandB));

// Add rangeDiff as a separate group (range is not the same as diff with artificial commits)
List<GitItemStatus> statuses = new List<GitItemStatus> { new GitItemStatus { Name = Strings.DiffRange, IsRangeDiff = true } };

var desc = Strings.DiffRange + ": " + GetDescriptionForRevision(describeRevision, firstRevHead) + " -> " +
GetDescriptionForRevision(describeRevision, selectedRevHead);
var statuses = new List<GitItemStatus> { new GitItemStatus { Name = Strings.DiffRange, IsRangeDiff = true } };
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

const int rangeDiffCommitLimit = 100;
var desc = $"{Strings.DiffRange} {baseToFirstCount ?? rangeDiffCommitLimit}↓ {baseToSecondCount ?? rangeDiffCommitLimit}↑";

var rangeDiff = new FileStatusWithDescription(
firstRev: first,
secondRev: selected,
Expand All @@ -185,25 +186,25 @@ public FileStatusDiffCalculator(Func<GitModule> getModule)

// Git range-diff has cubic runtime complexity and can be slow and memory consuming, so just skip if diff is large
// to avoid that GE seem to hang when selecting the range diff
const int maxRangeDiffCommits = 100;
int count = (baseA == null || baseB == null
? module.GetCommitDiffCount(firstRevHead.ToString(), selectedRevHead.ToString())
: module.GetCommitDiffCount(baseA.ToString(), firstRevHead.ToString())
+ module.GetCommitDiffCount(baseB.ToString(), selectedRevHead.ToString()))
?? maxRangeDiffCommits + 1;
if (!GitVersion.Current.SupportRangeDiffTool || count > maxRangeDiffCommits)
? baseToFirstCount + baseToSecondCount
: module.GetCommitDiffCount(baseA, firstRevHead)
+ module.GetCommitDiffCount(baseB, selectedRevHead))
?? rangeDiffCommitLimit;
if (!GitVersion.Current.SupportRangeDiffTool || count >= rangeDiffCommitLimit)
{
var range = baseA is null || baseB is null
? $"{first.ObjectId}...{selected.ObjectId}"
: $"{baseA}..{first.ObjectId} {baseB}..{selected.ObjectId}";
statuses[0].IsStatusOnly = true;

// Message is not translated, considered as an error message
statuses[0].ErrorMessage = $"# The symmetric difference from {first.ObjectId.ToShortString()} to {selected.ObjectId.ToShortString()} is {count} which is higher than the limit {maxRangeDiffCommits}\n" +
"# Git range-diff may take a long time and Git Extensions seem to hang during execution, why the command is not executed\n" +
"# You can still run the command in a Git terminal\n" +
"# Remove '--no-patch' to see changes to files too\n" +
$"git range-diff {range} --no-patch";
statuses[0].ErrorMessage =
$"# The symmetric difference from {first.ObjectId.ToShortString()} to {selected.ObjectId.ToShortString()} is {count} >= {rangeDiffCommitLimit}\n"
+ "# Git range-diff may take a long time and Git Extensions seem to hang during execution, why the command is not executed.\n"
+ "# You can still run the command in a Git terminal.\n"
+ "# Remove '--no-patch' to see changes to files too.\n"
+ $"git range-diff {range} --no-patch";
}

return fileStatusDescs;
Expand Down