Skip to content

Add -M/--find-renames option and blame.renames config to control rename detection #755

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
Draft
wants to merge 5 commits into
base: vfs-2.49.0
Choose a base branch
from
Draft

Add -M/--find-renames option and blame.renames config to control rename detection #755

wants to merge 5 commits into from

Conversation

Copilot
Copy link

@Copilot Copilot AI commented May 22, 2025 •
edited
Loading

When running git blame on large repositories, the automatic rename detection can cause performance issues because it unnecessarily compares blob contents to follow inexact renames. This PR adds options to control this behavior:

Added new features:

  1. Command line options to control rename detection in git blame:

    • -M[<n>] / --find-renames[=<n>] to specify the similarity threshold
    • Setting -M0 or --no-find-renames turns off rename detection completely
    • -M100 limits detection to only exact renames (identical blob content)

  2. New configuration option blame.renames that can be set to:

    • true or 1 - follow renames with default similarity index (50%)
    • false or 0 - disable rename detection entirely
    • copy - detect copies as well as renames
    • A number from 0-100 - control the similarity threshold

Documentation improvements:

  • Clarified the behavior of rename detection in git-blame.adoc
  • Added documentation for the -M/--find-renames option in blame-options.adoc
  • Added documentation for blame.renames config in config/blame.adoc

Testing:

  • Added a regression test t/t8015-blame-rename-detection.sh that verifies the new functionality

Example:

# Disable rename detection for better performance in large repos
git blame --no-find-renames file.txt

# Only follow exact renames (100% identical)
git blame -M100 file.txt

# Set blame.renames=false to disable by default
git config blame.renames false

Fixes #753.

Warning

Firewall rules blocked me from connecting to one or more addresses

I tried to connect to the following addresses, but was blocked by firewall rules:

  • cdn.fwupd.org
    • Triggering command: /usr/bin/fwupdmgr refresh (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

@Copilot Copilot AI assigned Copilot and dscho May 22, 2025
@Copilot Copilot AI changed the title [WIP] Performance issues running git blame in a repository cloned via VFS for Git Add -M/--find-renames option and blame.renames config to control rename detection May 22, 2025
@Copilot Copilot AI requested a review from dscho May 22, 2025 08:54
dscho
dscho reviewed May 22, 2025
View reviewed changes
Copy link
Member

@dscho dscho left a comment

Choose a reason for hiding this comment

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

I guess we have to abandon the idea of -M because that already exists (as you found out) but already means something else (yay consistency of Git's user interface).

Documentation/blame-options.adoc
Comment on lines 85 to 87
-M[<num>]::
--find-renames[=<num>]::
Detect moved or copied lines within a file. When a commit
Copy link
Member

Choose a reason for hiding this comment

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

No, you cannot do that. The existing -M option is not about whole-file rename detection, therefore we cannot reuse that, and my instructions regarding -M are moot.

You have to introduce a new --find-renames option instead.

blame.c Outdated
@@ -1423,10 +1423,17 @@ static struct blame_origin *find_rename(struct repository *r,
struct blame_origin *porigin = NULL;
struct diff_options diff_opts;
int i;
extern int rename_detection_mode;
Copy link
Member

Choose a reason for hiding this comment

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

No, this won't work. You declared it as static in builtin/blame.c, which makes it file-local, and you cannot ever see it from this here file.

Instead, you have to introduce a new attribute in blame.h, probably in struct blame_scoreboard next to the xdl_opts (which are also diff-related).

blame.c Outdated
Comment on lines 1429 to 1436
diff_opts.detect_rename = DIFF_DETECT_RENAME;
/*
* Use rename_detection_mode if specified, otherwise default to DIFF_DETECT_RENAME
* For mode values > 0 and < 100, use it as similarity threshold
*/
diff_opts.detect_rename = (rename_detection_mode == 0) ? 0 :
(rename_detection_mode > 0) ?
rename_detection_mode : DIFF_DETECT_RENAME;
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 the only place where you actually use rename_detection_mode, which you described elsewhere as a percentage. However, it is used exclusively as a Boolean. You have to study diff*.c harder to see how the --find-renames value is used there and imitate it here.

t/t8015-blame-rename-detection.sh Outdated
. ./test-lib.sh

test_expect_success 'setup test file rename with content changes' '
git init &&
Copy link
Member

Choose a reason for hiding this comment

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

Do study other t/t-[0-9]*-*blame*.sh scripts. git init is not necessary.

t/t8015-blame-rename-detection.sh Outdated
Comment on lines 9 to 11
echo abc >1.txt &&
echo def >>1.txt &&
echo ghi >>1.txt &&
Copy link
Member

Choose a reason for hiding this comment

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

Use test_write_lines instead.

t/t8015-blame-rename-detection.sh Outdated
echo abc >1.txt &&
echo def >>1.txt &&
echo ghi >>1.txt &&
git add . &&
Copy link
Member

Choose a reason for hiding this comment

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

. is sloppy. Specify 1.txt explicitly.

t/t8015-blame-rename-detection.sh
echo def >>1.txt &&
echo ghi >>1.txt &&
git add . &&
git commit -m "Initial commit" &&
Copy link
Member

Choose a reason for hiding this comment

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

You need to prefix this with test_tick to make the OIDs reproducible.

t/t8015-blame-rename-detection.sh Outdated
Comment on lines 22 to 32

# This test confirms that by default, git blame follows partial-file renames
test_expect_success 'git blame follows inexact renames by default' '
FIXED_1=$(git rev-parse --short HEAD^) &&
FIXED_2=$(git rev-parse --short HEAD) &&

git blame 2.txt >output &&
grep "$FIXED_1" output | grep -q abc &&
grep "$FIXED_2" output | grep -q 123 &&
grep "$FIXED_1" output | grep -q ghi
'
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 too focused, and unnecessary. We really only need to verify that 1.txt is mentioned only when the rename-detection score is high enough.

t/t8015-blame-rename-detection.sh Outdated
Comment on lines 34 to 80
# This test confirms that --no-find-renames or -M0 turns off rename detection
test_expect_success 'git blame can disable rename detection' '
git blame --no-find-renames 2.txt >output &&
! grep -q 1.txt output
'

# This test confirms that -M100 only follows exact renames
test_expect_success 'git blame can restrict to exact renames' '
git blame -M100 2.txt >output &&
! grep -q 1.txt output
'

# This test checks that blame.renames config works
test_expect_success 'blame.renames=false disables rename detection' '
git -c blame.renames=false blame 2.txt >output &&
! grep -q 1.txt output
'

# This test checks that -M with a score works
test_expect_success 'git blame with similarity score follows renames above threshold' '
# Must follow 1.txt->2.txt rename for abc which are identical
git blame -M70 2.txt >output &&
grep "$FIXED_1" output | grep -q abc &&
# Should not follow for others below threshold
grep "$FIXED_2" output | grep -q 123 &&
grep "$FIXED_2" output | grep -q ghi
'

# This test checks that -M overrides blame.renames
test_expect_success '-M overrides blame.renames config' '
# Using blame.renames=false but -M60
git -c blame.renames=false blame -M60 2.txt >output &&
grep "$FIXED_1" output | grep -q abc &&
# The rest would be below 60% threshold
grep "$FIXED_2" output | grep -q 123 &&
grep "$FIXED_2" output | grep -q ghi
'

# This test checks that blame.renames with a score works
test_expect_success 'blame.renames with score controls rename threshold' '
# Set threshold at 70%, abc is identical so above threshold
git -c blame.renames=70 blame 2.txt >output &&
grep "$FIXED_1" output | grep -q abc &&
# Other lines below threshold
grep "$FIXED_2" output | grep -q 123 &&
grep "$FIXED_2" output | grep -q ghi
'
Copy link
Member

Choose a reason for hiding this comment

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

Those are waaaaay too many test cases for the job. All you need to do is to run git blame with various ways to specify the rename score, and then verify that 1.txt is either mentioned or not, depending on that score.

In fact, I highly suspect that this single test case that you need to add for this entire PR would find a much nicer home in one of the existing t/t*-blame*.sh scripts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dscho dscho dscho left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@dscho dscho

Copilot code review Copilot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Performance issues running git blame in a repository cloned via VFS for Git
2 participants
@Copilot @dscho