[PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs

[PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs

[Ravi Mistry via GitGitGadget]

From: Ravi Mistry <rmistry@google.com>

git-blame(1) can ignore a list of commits specified via
--ignore-revs-file or the blame.ignoreRevsFile configuration option.
This is useful for skipping uninteresting revisions such as tree-wide
formatting changes, large-scale refactors, and code modernizations that
would otherwise obscure genuine historical authorship.

When revision-ignoring was introduced in commit ae3f36dea1 ("blame: add
blame.ignoreRevsFile config option", 2019-10-18), it intentionally
avoided adopting a default ignore file. At the time, the capability was
new and unproven, so avoiding unrequested filesystem I/O or unexpected
attribution shifts took priority over a project-wide default.
Requiring explicit opt-in per clone was therefore the prudent design.

Since then, maintaining a .git-blame-ignore-revs file in the repository
root has become the de facto standard across the Git ecosystem, adopted
by major hosting platforms (GitHub, GitLab, Gerrit) and prominent open
source projects (such as Chromium and LLVM). As a consequence,
developers frequently encounter a jarring mismatch: web interfaces
seamlessly ignore formatting commits, but local git-blame(1) and
git-annotate(1) runs do not, unless each user manually configures
blame.ignoreRevsFile for every local checkout.

Teach git-blame(1) and git-annotate(1) to automatically check for a
regular .git-blame-ignore-revs file at the root of the working tree when
operating in a non-bare repository.

To ensure consistent precedence, security, and override semantics:
- Loading the default file occurs before reading configuration and CLI
  options, preserving user and repository config overrides.
- Path resolution is anchored to repo_get_work_tree() and verified via
  lstat() to ensure it is a regular file. Symbolic links, directories,
  FIFOs, and sockets are safely skipped, preventing local information
  disclosure and denial-of-service hangs.
- In build_ignorelist(), ignore-rev files are parsed starting after the
  last empty string entry. This ensures setting blame.ignoreRevsFile to
  "" or passing --ignore-revs-file "" or --no-ignore-revs-file cleanly
  discards the default file without attempting to open or parse it,
  allowing users to bypass corrupted default files.
- Duplicate parsing is prevented by tracking seen files in a strset.

Update documentation in blame-options.adoc and config/blame.adoc, and
add comprehensive test coverage in t8013 for the default file lookup,
subdirectory invocations, CLI and config overrides, symlink rejection,
comments and whitespace handling, and bare repositories.

Based-on-patch-by: Abhijeetsingh Meena <abhijeet040403@gmail.com>
Helped-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Ravi Mistry <rmistry@google.com>
---
    blame: default to ignoring revisions in .git-blame-ignore-revs
    
    This series restarts the conversation from the stalled attempt in PR
    https://github.com/gitgitgadget/git/pull/1809 and addresses feature
    request https://github.com/gitgitgadget/git/issues/1494
    
    See the previous discussion in
    https://lore.kernel.org/git/pull.1809.v2.git.1728707867.gitgitgadget@gmail.com/
    
    In addition to resolving the questions raised by reviewers during the
    previous discussion, this updated iteration introduces important
    security hardening, bug fixes, and test improvements:
    
     1. Commit message rationale: The commit message now details why
        revision ignoring originally avoided a default file when the feature
        was first added:
        https://github.com/git/git/commit/ae3f36dea16e51041c56ba9ed6b38380c8421816
        It explains why ecosystem standardization across GitHub, GitLab,
        Gerrit, Chromium, and LLVM makes a default file desirable today,
        addressing the previous feedback from Phillip Wood. Trailers
        acknowledge earlier patch authorship and reviewer contributions.
    
     2. Security and path resolution: Path lookup is anchored to
        repo_get_work_tree(). Using lstat ensures that symbolic links,
        directories, FIFOs, and sockets are skipped safely.
    
     3. Configuration and option override semantics: The build_ignorelist()
        function begins processing after the last empty string entry.
        Setting blame.ignoreRevsFile to an empty string or providing an
        empty filename option on the command line allows users to bypass a
        corrupted default file without encountering a fatal error.
    
     4. Documentation updates: Documentation clarifies that configured files
        are processed after the default file. It explains how providing an
        empty filename disables the default file and notes that bare
        repositories do not search for the file. Documentation checks pass
        without warning.
    
     5. Test harness improvements: The test suite removes the destructive
        repository reset in t8013 and adds test coverage for comments,
        whitespace, zero byte files, symlink rejection, corrupted file
        overrides, and git annotate parity. Test lint checks pass without
        error.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2224%2Frmistry%2Fblame-default-ignore-revs-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2224/rmistry/blame-default-ignore-revs-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2224

 Documentation/blame-options.adoc |   6 +-
 Documentation/config/blame.adoc  |   9 +-
 builtin/blame.c                  |  42 +++++++--
 t/t8013-blame-ignore-revs.sh     | 151 +++++++++++++++++++++++++++++++
 4 files changed, 196 insertions(+), 12 deletions(-)

diff --git a/Documentation/blame-options.adoc b/Documentation/blame-options.adoc
index 1ae1222b6b..17f5734d61 100644
--- a/Documentation/blame-options.adoc
+++ b/Documentation/blame-options.adoc
@@ -133,8 +133,10 @@ take effect.
 	Ignore revisions listed in _<file>_, which must be in the same format as an
 	`fsck.skipList`.  This option may be repeated, and these files will be
 	processed after any files specified with the `blame.ignoreRevsFile` config
-	option.  An empty file name, `""`, will clear the list of revs from
-	previously processed files.
+	option or the default `.git-blame-ignore-revs` file.  An empty file name,
+	`""`, will clear the list of revs from previously processed files.
+	`--no-ignore-revs-file` will clear all previously specified ignore revs
+	files, including the default `.git-blame-ignore-revs` file.
 
 `--color-lines`::
 	Color line annotations in the default format differently if they come from
diff --git a/Documentation/config/blame.adoc b/Documentation/config/blame.adoc
index 4d047c1790..6f9be627e1 100644
--- a/Documentation/config/blame.adoc
+++ b/Documentation/config/blame.adoc
@@ -23,9 +23,12 @@ blame.showRoot::
 blame.ignoreRevsFile::
 	Ignore revisions listed in the file, one unabbreviated object name per
 	line, in linkgit:git-blame[1].  Whitespace and comments beginning with
-	`#` are ignored.  This option may be repeated multiple times.  Empty
-	file names will reset the list of ignored revisions.  This option will
-	be handled before the command line option `--ignore-revs-file`.
+	`#` are ignored.  If `.git-blame-ignore-revs` exists at the root of the
+	working tree in a non-bare repository, it is used by default.  This option
+	may be repeated multiple times; files specified here are processed after
+	the default file.  An empty file name will reset the list of ignored
+	revisions from previously processed files and disable the default file.
+	This option is handled before the command-line option `--ignore-revs-file`.
 
 blame.markUnblamableLines::
 	Mark lines that were changed by an ignored revision that we could not
diff --git a/builtin/blame.c b/builtin/blame.c
index 48d5251c6d..0935d864ad 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -15,9 +15,11 @@
 #include "hex.h"
 #include "commit.h"
 #include "diff.h"
+#include "path.h"
 #include "revision.h"
 #include "quote.h"
 #include "string-list.h"
+#include "strmap.h"
 #include "mailmap.h"
 #include "parse-options.h"
 #include "prio-queue.h"
@@ -768,8 +770,12 @@ static int git_blame_config(const char *var, const char *value,
 		ret = git_config_pathname(&str, var, value);
 		if (ret)
 			return ret;
-		if (str)
-			string_list_insert(&ignore_revs_file_list, str);
+		if (str) {
+			if (!*str)
+				string_list_clear(&ignore_revs_file_list, 0);
+			else
+				string_list_append(&ignore_revs_file_list, str);
+		}
 		free(str);
 		return 0;
 	}
@@ -936,16 +942,24 @@ static void build_ignorelist(struct blame_scoreboard *sb,
 {
 	struct string_list_item *i;
 	struct object_id oid;
+	struct strset seen_files = STRSET_INIT;
+	size_t start_idx = 0, idx;
+
+	for (idx = 0; idx < ignore_revs_file_list->nr; idx++) {
+		if (!*ignore_revs_file_list->items[idx].string)
+			start_idx = idx + 1;
+	}
 
 	oidset_init(&sb->ignore_list, 0);
-	for_each_string_list_item(i, ignore_revs_file_list) {
-		if (!strcmp(i->string, ""))
-			oidset_clear(&sb->ignore_list);
-		else
-			oidset_parse_file_carefully(&sb->ignore_list, i->string,
+	for (idx = start_idx; idx < ignore_revs_file_list->nr; idx++) {
+		const char *path = ignore_revs_file_list->items[idx].string;
+
+		if (strset_add(&seen_files, path))
+			oidset_parse_file_carefully(&sb->ignore_list, path,
 						    the_repository->hash_algo,
 						    peel_to_commit_oid, sb);
 	}
+	strset_clear(&seen_files);
 	for_each_string_list_item(i, ignore_rev_list) {
 		if (repo_get_oid_committish(the_repository, i->string, &oid) ||
 		    peel_to_commit_oid(&oid, sb))
@@ -1020,6 +1034,20 @@ int cmd_blame(int argc,
 	const char *const *opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;
 
 	setup_default_color_by_age();
+	{
+		const char *work_tree = repo_get_work_tree(the_repository);
+
+		if (work_tree) {
+			char *default_file = mkpathdup("%s/%s", work_tree,
+						       ".git-blame-ignore-revs");
+			struct stat st;
+
+			if (!lstat(default_file, &st) && S_ISREG(st.st_mode) &&
+			    !access(default_file, R_OK))
+				string_list_append(&ignore_revs_file_list, default_file);
+			free(default_file);
+		}
+	}
 	repo_config(the_repository, git_blame_config, &output_option);
 	repo_init_revisions(the_repository, &revs, NULL);
 	revs.date_mode = blame_date_mode;
diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
index cace00ae8d..a43654a7ba 100755
--- a/t/t8013-blame-ignore-revs.sh
+++ b/t/t8013-blame-ignore-revs.sh
@@ -327,4 +327,155 @@ test_expect_success ignore_merge '
 	test_cmp expect actual
 '
 
+# Tests for default .git-blame-ignore-revs file
+test_expect_success 'setup default .git-blame-ignore-revs' '
+	git checkout -b default-file-branch &&
+	test_write_lines line1 line2 >def-file &&
+	git add def-file &&
+	test_tick &&
+	git commit -m "default base" &&
+	git tag DEF_A &&
+
+	test_write_lines line1-modified line2-modified >def-file &&
+	git add def-file &&
+	test_tick &&
+	git commit -m "default mod" &&
+	git tag DEF_B &&
+
+	git rev-parse DEF_B >.git-blame-ignore-revs
+'
+
+test_expect_success 'default .git-blame-ignore-revs is used by default' '
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_A >expect &&
+	test_cmp expect actual &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'default .git-blame-ignore-revs respected by git annotate' '
+	git rev-parse --short DEF_A >expect_sha &&
+	git annotate def-file >actual &&
+	test_grep "^$(cat expect_sha)" actual
+'
+
+test_expect_success 'default .git-blame-ignore-revs works from subdirectory' '
+	mkdir -p sub &&
+	(
+		cd sub &&
+		git blame --line-porcelain ../def-file >blame_raw &&
+		sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+		git rev-parse DEF_A >expect &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'disable default .git-blame-ignore-revs with --no-ignore-revs-file' '
+	git blame --line-porcelain --no-ignore-revs-file def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_B >expect &&
+	test_cmp expect actual &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'disable default .git-blame-ignore-revs with --ignore-revs-file ""' '
+	git blame --line-porcelain --ignore-revs-file "" def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_B >expect &&
+	test_cmp expect actual &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'disable default .git-blame-ignore-revs with blame.ignoreRevsFile=""' '
+	test_config blame.ignoreRevsFile "" &&
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_B >expect &&
+	test_cmp expect actual &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'default .git-blame-ignore-revs handles comments and whitespace' '
+	test_when_finished "git rev-parse DEF_B >.git-blame-ignore-revs" &&
+	{
+		echo "# Leading comment" &&
+		echo "" &&
+		echo "   $(git rev-parse DEF_B)   " &&
+		echo "# Trailing comment"
+	} >.git-blame-ignore-revs &&
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_A >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'empty default .git-blame-ignore-revs is harmless' '
+	test_when_finished "git rev-parse DEF_B >.git-blame-ignore-revs" &&
+	: >.git-blame-ignore-revs &&
+	git blame def-file
+'
+
+test_expect_success SYMLINKS 'symlink .git-blame-ignore-revs is ignored' '
+	test_when_finished "rm -f target_file .git-blame-ignore-revs && git rev-parse DEF_B >.git-blame-ignore-revs" &&
+	git rev-parse DEF_B >target_file &&
+	ln -sf target_file .git-blame-ignore-revs &&
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_B >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'malformed default .git-blame-ignore-revs fails but can be bypassed' '
+	test_when_finished "git rev-parse DEF_B >.git-blame-ignore-revs" &&
+	echo "invalid-oid-value" >.git-blame-ignore-revs &&
+	test_must_fail git blame def-file &&
+	git blame --no-ignore-revs-file def-file &&
+	git blame --ignore-revs-file "" def-file
+'
+
+test_expect_success 'default .git-blame-ignore-revs deduplicated when also set in config' '
+	test_config blame.ignoreRevsFile .git-blame-ignore-revs &&
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_A >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'default .git-blame-ignore-revs combined with config blame.ignoreRevsFile' '
+	test_write_lines line1-modified line2-c >def-file &&
+	git add def-file &&
+	test_tick &&
+	git commit -m C &&
+	git tag DEF_C &&
+	git rev-parse DEF_C >custom_ignore &&
+	test_config blame.ignoreRevsFile custom_ignore &&
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_A >expect &&
+	test_cmp expect actual &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_A >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'default .git-blame-ignore-revs ignored in bare repo' '
+	git clone --bare . bare.git &&
+	git -C bare.git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_C >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'blame works when .git-blame-ignore-revs does not exist' '
+	rm -f .git-blame-ignore-revs &&
+	git blame --line-porcelain def-file >blame_raw &&
+	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
+	git rev-parse DEF_B >expect &&
+	test_cmp expect actual
+'
+
 test_done

base-commit: fa7f9290efe2bd22dd736689597b474b93798e11
-- 
gitgitgadget