Skip to content

Default to histogram diff #23257

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

Closed
wants to merge 1 commit into from
Closed

Default to histogram diff #23257

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions modules/git/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,13 @@ func syncGitConfig() (err error) {
}
}

if CheckGitVersionAtLeast("1.8.2") == nil {
// The `histogram` algorithm itself was added in 1.7.7, but `diff.algorithm` was supported in Git 1.8.2.
if err := configSet("diff.algorithm", "histogram"); err != nil {
return err
}
}
Comment on lines +245 to +250
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if CheckGitVersionAtLeast("1.8.2") == nil {
// The `histogram` algorithm itself was added in 1.7.7, but `diff.algorithm` was supported in Git 1.8.2.
if err := configSet("diff.algorithm", "histogram"); err != nil {
return err
}
}
if err := configSet("diff.algorithm", "histogram"); err != nil {
return err
}

The minimal required git version is already 2.0

However, I can't LGTM yet as I don't know what the side effects are.
Does it need more memory?
More CPU time?
Produce worse outputs?
Or why isn't it the default?

Copy link
Author

Choose a reason for hiding this comment

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

Much of these questions are answered in this post: https://stackoverflow.com/questions/32365271/whats-the-difference-between-git-diff-patience-and-git-diff-histogram

Does it need more memory?

There has been a short history of bad memory usage that has since been fixed:

Note: "git diff --histogram" had a bad memory usage pattern, which has been rearranged to reduce the peak usage, with Git 2.19 (Q3 2018).

Note: the patience and histogram algorithms had memory leaks, fixed with Git 2.36 (Q2 2022)

I can bump the version requirement if that's desirable.

More CPU time?

It appears to be faster than both Myers and Patience.

Commit 8c912ee actually introduced --histogram to diff:
Port JGit's HistogramDiff algorithm over to C. Rough numbers (TODO) show that it is faster than its --patience cousin, as well as the default Meyers algorithm.

8c912ee (teach --histogram to diff, 2011-07-12) claimed histogram diff was faster than both Myers and patience.

Produce worse outputs?

Based on the research article I posted in the ticket, it consistently produces better outputs than Myers. I'm broadly summarizing, but this is measured by finding over x2 more bugs in aggregate in the research.

Why isn't it the default?

... uhh, idk. It's newer? 🤷‍♂️


if SupportProcReceive {
// set support for AGit flow
if err := configAddNonExist("receive.procReceiveRefs", "refs/for"); err != nil {
Expand Down