* [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs
@ 2026-09-11 23:29 Ravi Mistry via GitGitGadget
2026-09-30 14:59 ` Ravi Mistry
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Ravi Mistry via GitGitGadget @ 2026-09-11 23:29 UTC (permalink / raw)
To: git
Cc: Abhijeetsingh Meena, Kristoffer Haugsbakk, Phillip Wood,
Eric Sunshine, Ravi Mistry, Ravi Mistry
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs
2026-09-11 23:29 [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs Ravi Mistry via GitGitGadget
@ 2026-09-30 14:59 ` Ravi Mistry
2026-10-05 15:37 ` Junio C Hamano
2026-10-08 21:07 ` [PATCH v2 0/2] blame: ignore revs in HEAD:.git-blame-ignore-revs by default Ravi Mistry via GitGitGadget
2 siblings, 0 replies; 11+ messages in thread
From: Ravi Mistry @ 2026-09-30 14:59 UTC (permalink / raw)
To: git; +Cc: gitster, phillip.wood, code, sunshine, abhijeet040403, rmistry
Hi all,
Gentle ping on this patch. Please let me know if you have any feedback or questions on this approach, or if there are other reviewers I should loop in.
TIA!
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs
2026-09-11 23:29 [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs Ravi Mistry via GitGitGadget
2026-09-30 14:59 ` Ravi Mistry
@ 2026-10-05 15:37 ` Junio C Hamano
2026-10-05 21:12 ` Ravi Mistry
2026-10-08 21:07 ` [PATCH v2 0/2] blame: ignore revs in HEAD:.git-blame-ignore-revs by default Ravi Mistry via GitGitGadget
2 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2026-10-05 15:37 UTC (permalink / raw)
To: Ravi Mistry via GitGitGadget
Cc: git, Abhijeetsingh Meena, Kristoffer Haugsbakk, Phillip Wood,
Eric Sunshine, Ravi Mistry
"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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`.
The proposed log message explains that '.git-blame-ignore-revs' is
used as the default for blame.ignoreRevsFile even when the user does
not ask to do so in order to match what hosting sites do, as it
would be confusing if the local repository behaved differently.
While wanting consistency is reasonable, the description above does
not quite match that goal. If an untracked '.git-blame-ignore-revs'
file exists at the root of the working tree, or if a tracked one has
local changes relative to HEAD, the local repository behaves
differently from hosting sites that operate on the
'HEAD:.git-blame-ignore-revs' blob. It may make more sense to say:
"If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
the initial element in the list of ignore-revs files. Other files
listed in the configuration are also used, but an empty element
makes all elements that appeared before in the list forgotten."
This rule should apply whether the repository is bare or not.
The proposed log message also talks about taking only a regular file
and ignoring everything else for "security" [*], but there is
another important thing we need to worry about security-wise.
Somebody has to audit the parser for these files (one unabbreviated
object name per line, ignoring whitespace and lines starting with
'#') and ensure that the implementation is truly secure.
This is a new threat vector introduced by this change. Without this
patch, blame.ignoreRevsFile comes only from the configuration, which
cannot point to an attacker-controlled file under our threat model.
Now, however, the parser must read upstream-controlled content at a
known path, and it must be prepared to cope with attempts to use it
as an attack vector.
[Footnote]
* By the way, "we do not read anything from the working tree.
'HEAD:.git-blame-ignore-revs' is the only thing that is added
to the picture" would make it unnecessary to lstat() and ignore
non-regular files.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs
2026-10-05 15:37 ` Junio C Hamano
@ 2026-10-05 21:12 ` Ravi Mistry
2026-10-07 17:43 ` Junio C Hamano
0 siblings, 1 reply; 11+ messages in thread
From: Ravi Mistry @ 2026-10-05 21:12 UTC (permalink / raw)
To: gitster; +Cc: git, phillip.wood, code, sunshine, abhijeet040403, rmistry
"Junio C Hamano" <gitster@pobox.com> writes:
> While wanting consistency is reasonable, the description above does
> not quite match that goal. If an untracked '.git-blame-ignore-revs'
> file exists at the root of the working tree, or if a tracked one has
> local changes relative to HEAD, the local repository behaves
> differently from hosting sites that operate on the
> 'HEAD:.git-blame-ignore-revs' blob. It may make more sense to say:
> "If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
> the initial element in the list of ignore-revs files. Other files
> listed in the configuration are also used, but an empty element
> makes all elements that appeared before in the list forgotten."
> This rule should apply whether the repository is bare or not.
Thank you very much for the detailed feedback, Junio! Reading the
committed blob from HEAD instead of the working tree totally makes
sense.
> Somebody has to audit the parser for these files (one unabbreviated
> object name per line, ignoring whitespace and lines starting with
> '#') and ensure that the implementation is truly secure.
I looked through the parser in oidset.c (which we can share for
both the HEAD blob and configured files) and peel_to_commit_oid in
builtin/blame.c. Mostly looks good, IMHO, but there may be two edge
cases we can tighten up:
1. Rejecting lines with embedded NUL bytes via memchr in oidset.c
(where strchr and the check after parse_oid_hex_algop currently
stop at the first NUL byte and ignore trailing bytes on the
line).
2. Passing OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK in
peel_to_commit_oid and peeling tags step by step so missing OIDs
or tag targets do not trigger lazy promisor fetches in partial
clones.
Does this plan sound good to you for v2?
Thanks,
Ravi
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs
2026-10-05 21:12 ` Ravi Mistry
@ 2026-10-07 17:43 ` Junio C Hamano
2026-10-07 18:07 ` Ravi Mistry
0 siblings, 1 reply; 11+ messages in thread
From: Junio C Hamano @ 2026-10-07 17:43 UTC (permalink / raw)
To: Ravi Mistry; +Cc: git, phillip.wood, code, sunshine, abhijeet040403
Ravi Mistry <rmistry@google.com> writes:
> "Junio C Hamano" <gitster@pobox.com> writes:
>
>> While wanting consistency is reasonable, the description above does
>> not quite match that goal. If an untracked '.git-blame-ignore-revs'
>> file exists at the root of the working tree, or if a tracked one has
>> local changes relative to HEAD, the local repository behaves
>> differently from hosting sites that operate on the
>> 'HEAD:.git-blame-ignore-revs' blob. It may make more sense to say:
>> "If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
>> the initial element in the list of ignore-revs files. Other files
>> listed in the configuration are also used, but an empty element
>> makes all elements that appeared before in the list forgotten."
>> This rule should apply whether the repository is bare or not.
>
> Thank you very much for the detailed feedback, Junio! Reading the
> committed blob from HEAD instead of the working tree totally makes
> sense.
>
>> Somebody has to audit the parser for these files (one unabbreviated
>> object name per line, ignoring whitespace and lines starting with
>> '#') and ensure that the implementation is truly secure.
>
> I looked through the parser in oidset.c (which we can share for
> both the HEAD blob and configured files) and peel_to_commit_oid in
> builtin/blame.c. Mostly looks good, IMHO, but there may be two edge
> cases we can tighten up:
>
> 1. Rejecting lines with embedded NUL bytes via memchr in oidset.c
> (where strchr and the check after parse_oid_hex_algop currently
> stop at the first NUL byte and ignore trailing bytes on the
> line).
>
> 2. Passing OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK in
> peel_to_commit_oid and peeling tags step by step so missing OIDs
> or tag targets do not trigger lazy promisor fetches in partial
> clones.
>
> Does this plan sound good to you for v2?
Are you presenting a different plan, or just adding details to what
you quoted from my message above?
I delegated because I did not want to spend time on the auditing
part, so if you are asking me that these two are the only things we
need to address, that defeats the point of me delegating it to
"somebody else" X-<. Hopefully a v2 with some tightening the OID
parsing may entice folks (who are hopefully interested in security
related work) to chime in and they would help us decide if it is
good enough to cover these two points and nothing else.
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs
2026-10-07 17:43 ` Junio C Hamano
@ 2026-10-07 18:07 ` Ravi Mistry
0 siblings, 0 replies; 11+ messages in thread
From: Ravi Mistry @ 2026-10-07 18:07 UTC (permalink / raw)
To: gitster; +Cc: git, phillip.wood, code, sunshine, abhijeet040403, rmistry
"Junio C Hamano" <gitster@pobox.com> writes:
> Are you presenting a different plan, or just adding details to what
> you quoted from my message above?
>
> I delegated because I did not want to spend time on the auditing
> part, so if you are asking me that these two are the only things we
> need to address, that defeats the point of me delegating it to
> "somebody else" X-<. Hopefully a v2 with some tightening the OID
> parsing may entice folks (who are hopefully interested in security
> related work) to chime in and they would help us decide if it is
> good enough to cover these two points and nothing else.
Ah, sorry for my confusion, I was adding details to your plan
above (I thought the audit was being delegated to me so wanted to
make sure the approach was correct). Will put up a v2 for the
security reviewers to chime in.
Thanks again!
Ravi
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 0/2] blame: ignore revs in HEAD:.git-blame-ignore-revs by default
2026-09-11 23:29 [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs Ravi Mistry via GitGitGadget
2026-09-30 14:59 ` Ravi Mistry
2026-10-05 15:37 ` Junio C Hamano
@ 2026-10-08 21:07 ` Ravi Mistry via GitGitGadget
2026-10-08 21:07 ` [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling Ravi Mistry via GitGitGadget
2026-10-08 21:07 ` [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs Ravi Mistry via GitGitGadget
2 siblings, 2 replies; 11+ messages in thread
From: Ravi Mistry via GitGitGadget @ 2026-10-08 21:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Abhijeetsingh Meena, Kristoffer Haugsbakk,
Phillip Wood, Eric Sunshine, Ravi Mistry
This series teaches git-blame(1) and git-annotate(1) to automatically use
the HEAD:.git-blame-ignore-revs blob by default if it exists, so local runs
match hosting platforms without requiring manual blame.ignoreRevsFile
configuration in every clone. This restarts the stalled attempt in PR
https://github.com/gitgitgadget/git/pull/1809
(https://lore.kernel.org/git/pull.1809.v2.git.1728707867.gitgitgadget@gmail.com/)
and addresses https://github.com/gitgitgadget/git/issues/1494.
Changes since v1:
* Split the series into two commits.
* Patch 1/2 hardens oidset_parse_file_carefully() in oidset.c and
peel_to_commit_oid() in builtin/blame.c before exposing them to
upstream-controlled content at a well-known path:
* Reject lines containing embedded NUL bytes via memchr() so trailing
bytes after a NUL cannot be silently ignored.
* Pass OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT |
OBJECT_INFO_QUICK to odb_read_object_info_extended() and peel tags one
layer per iteration so missing OIDs or tag targets do not trigger lazy
promisor fetches or pack directory rescans in partial clones, and
verify that each peeled target matches the tag's declared type.
* Patch 2/2 reads the committed HEAD:.git-blame-ignore-revs blob (in both
bare and non-bare repositories) instead of reading a file from the
working tree, matching hosting platforms even when an untracked file is
present or a tracked one has local modifications. The tree entry is
checked with S_ISREG() so non-regular entries (such as committed
symlinks) are skipped, parsed in memory via
oidset_parse_buffer_carefully(), and bypassed without reading if cleared
via blame.ignoreRevsFile="" or --no-ignore-revs-file.
Ravi Mistry (2):
blame: harden ignore-revs parser and tag peeling
blame: ignore revs in HEAD:.git-blame-ignore-revs
Documentation/blame-options.adoc | 8 +-
Documentation/config/blame.adoc | 9 +-
builtin/blame.c | 65 ++++++++-
oidset.c | 81 ++++++++----
oidset.h | 9 ++
t/t8013-blame-ignore-revs.sh | 220 +++++++++++++++++++++++++++++++
6 files changed, 357 insertions(+), 35 deletions(-)
base-commit: fa7f9290efe2bd22dd736689597b474b93798e11
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2224%2Frmistry%2Fblame-default-ignore-revs-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2224/rmistry/blame-default-ignore-revs-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/2224
Range-diff vs v1:
-: ---------- > 1: 2e12486c0d blame: harden ignore-revs parser and tag peeling
1: 22a100d00d ! 2: 35e303d65b blame: default to ignoring revisions in .git-blame-ignore-revs
@@ Metadata
Author: Ravi Mistry <rmistry@google.com>
## Commit message ##
- blame: default to ignoring revisions in .git-blame-ignore-revs
+ blame: ignore revs in HEAD:.git-blame-ignore-revs
git-blame(1) can ignore a list of commits specified via
--ignore-revs-file or the blame.ignoreRevsFile configuration option.
@@ Commit message
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.
+ Teach git-blame(1) and git-annotate(1) to automatically add the
+ HEAD:.git-blame-ignore-revs blob, if it exists, as the initial element
+ in the list of ignore-revs files in both bare and non-bare
+ repositories. Reading the committed blob from HEAD rather than the
+ working tree ensures that local runs match hosting platforms even when
+ an untracked .git-blame-ignore-revs file is present or a tracked one
+ has uncommitted local changes.
- 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.
+ To ensure consistent precedence and override semantics:
+ - The default HEAD:.git-blame-ignore-revs entry is added before reading
+ configuration and CLI options, preserving user and repository config
+ overrides.
+ - In git_blame_config(), blame.ignoreRevsFile entries are appended via
+ string_list_append() rather than inserted in sorted order via
+ string_list_insert() so that configuration entries preserve their
+ order relative to the initial default entry.
+ - The HEAD:.git-blame-ignore-revs tree entry is resolved quietly via
+ get_oid_with_context(). Its mode is checked with S_ISREG() before
+ reading the object so that non-regular tree entries (such as a
+ committed symbolic link whose blob stores a target path rather than
+ revision IDs, a subdirectory, or a gitlink) are skipped instead of
+ being read and rejected as malformed object names. The blob is parsed
+ in memory via a new oidset_parse_buffer_carefully() helper in
+ oidset.c that shares line parsing with oidset_parse_file_carefully().
+ - In build_ignorelist(), ignore-revs entries are processed 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 blob without
+ attempting to read or parse it, allowing users to bypass a malformed
+ default blob.
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.
+ add comprehensive test coverage in t8013 for the default blob lookup,
+ subdirectory invocations, bare repositories, uncommitted and untracked
+ working-tree files, CLI and config overrides, committed symlink
+ entries, and comments and whitespace handling.
Based-on-patch-by: Abhijeetsingh Meena <abhijeet040403@gmail.com>
Helped-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
@@ Commit message
## Documentation/blame-options.adoc ##
@@ Documentation/blame-options.adoc: take effect.
+ `--ignore-revs-file <file>`::
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
+- 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.
++ processed after the default `HEAD:.git-blame-ignore-revs` blob (if it
++ exists) and any files specified with the `blame.ignoreRevsFile` config
++ option. An empty file name, `""`, or `--no-ignore-revs-file` will clear
++ the list of revs from previously processed files, including the default
++ `HEAD:.git-blame-ignore-revs` blob.
`--color-lines`::
Color line annotations in the default format differently if they come from
@@ Documentation/config/blame.adoc: blame.showRoot::
- `#` 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`.
++ `#` are ignored. If the `HEAD:.git-blame-ignore-revs` blob exists, it
++ is added as the initial element in the list of ignore-revs files.
++ Other files listed in the configuration are also used, but an empty
++ element makes all elements that appeared before in the list forgotten.
++ This option will be handled before the command line option
++ `--ignore-revs-file`.
blame.markUnblamableLines::
Mark lines that were changed by an ignored revision that we could not
## builtin/blame.c ##
-@@
- #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"
@@ builtin/blame.c: static int git_blame_config(const char *var, const char *value,
- ret = git_config_pathname(&str, var, value);
if (ret)
return ret;
-- if (str)
+ 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);
-+ }
++ string_list_append(&ignore_revs_file_list, str);
free(str);
return 0;
}
-@@ builtin/blame.c: static void build_ignorelist(struct blame_scoreboard *sb,
+@@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
+ }
+ }
+
++static void parse_default_ignore_revs_blob(struct blame_scoreboard *sb,
++ const char *name)
++{
++ struct object_context oc;
++ struct object_id oid;
++ enum object_type type;
++ size_t size;
++ char *buf;
++
++ if (get_oid_with_context(the_repository, name, GET_OID_QUIETLY,
++ &oid, &oc))
++ goto out;
++ if (!S_ISREG(oc.mode))
++ goto out;
++
++ buf = odb_read_object(the_repository->objects, &oid, &type, &size);
++ if (!buf)
++ goto out;
++ if (type == OBJ_BLOB)
++ oidset_parse_buffer_carefully(&sb->ignore_list, buf, size,
++ the_repository->hash_algo,
++ peel_to_commit_oid, sb);
++ free(buf);
++
++out:
++ object_context_release(&oc);
++}
++
+ static void build_ignorelist(struct blame_scoreboard *sb,
+ struct string_list *ignore_revs_file_list,
+ struct string_list *ignore_rev_list)
{
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++) {
@@ builtin/blame.c: static void build_ignorelist(struct blame_scoreboard *sb,
- 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,
++ i = &ignore_revs_file_list->items[idx];
++ if (i->util)
++ parse_default_ignore_revs_blob(sb, i->string);
+ else
+ oidset_parse_file_carefully(&sb->ignore_list, i->string,
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))
@@ builtin/blame.c: 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);
-+ }
-+ }
++ string_list_append(&ignore_revs_file_list,
++ "HEAD:.git-blame-ignore-revs")->util = &sb;
repo_config(the_repository, git_blame_config, &output_option);
repo_init_revisions(the_repository, &revs, NULL);
revs.date_mode = blame_date_mode;
+ ## oidset.c ##
+@@ oidset.c: void oidset_parse_file(struct oidset *set, const char *path,
+ oidset_parse_file_carefully(set, path, algop, NULL, NULL);
+ }
+
++static void parse_oidset_line(struct oidset *set, struct strbuf *sb,
++ const struct git_hash_algo *algop,
++ oidset_parse_tweak_fn fn, void *cbdata)
++{
++ const char *p;
++ const char *name;
++ struct object_id oid;
++
++ if (memchr(sb->buf, '\0', sb->len))
++ die("invalid object name: %s", sb->buf);
++
++ /*
++ * Allow trailing comments, leading whitespace
++ * (including before commits), and empty or whitespace
++ * only lines.
++ */
++ name = strchr(sb->buf, '#');
++ if (name)
++ strbuf_setlen(sb, name - sb->buf);
++ strbuf_trim(sb);
++ if (!sb->len)
++ return;
++
++ if (parse_oid_hex_algop(sb->buf, &oid, &p, algop) || *p != '\0')
++ die("invalid object name: %s", sb->buf);
++ if (fn && fn(&oid, cbdata))
++ return;
++ oidset_insert(set, &oid);
++}
++
+ void oidset_parse_file_carefully(struct oidset *set, const char *path,
+ const struct git_hash_algo *algop,
+ oidset_parse_tweak_fn fn, void *cbdata)
+ {
+ FILE *fp;
+ struct strbuf sb = STRBUF_INIT;
+- struct object_id oid;
+
+ fp = fopen(path, "r");
+ if (!fp)
+ die("could not open object name list: %s", path);
+- while (!strbuf_getline(&sb, fp)) {
+- const char *p;
+- const char *name;
+-
+- if (memchr(sb.buf, '\0', sb.len))
+- die("invalid object name: %s", sb.buf);
+-
+- /*
+- * Allow trailing comments, leading whitespace
+- * (including before commits), and empty or whitespace
+- * only lines.
+- */
+- name = strchr(sb.buf, '#');
+- if (name)
+- strbuf_setlen(&sb, name - sb.buf);
+- strbuf_trim(&sb);
+- if (!sb.len)
+- continue;
+-
+- if (parse_oid_hex_algop(sb.buf, &oid, &p, algop) || *p != '\0')
+- die("invalid object name: %s", sb.buf);
+- if (fn && fn(&oid, cbdata))
+- continue;
+- oidset_insert(set, &oid);
+- }
++ while (!strbuf_getline(&sb, fp))
++ parse_oidset_line(set, &sb, algop, fn, cbdata);
+ if (ferror(fp))
+ die_errno("Could not read '%s'", path);
+ fclose(fp);
+ strbuf_release(&sb);
+ }
++
++void oidset_parse_buffer_carefully(struct oidset *set, const char *buf,
++ size_t size,
++ const struct git_hash_algo *algop,
++ oidset_parse_tweak_fn fn, void *cbdata)
++{
++ struct strbuf sb = STRBUF_INIT;
++ const char *p = buf, *end;
++
++ if (!size)
++ return;
++ end = buf + size;
++
++ while (p < end) {
++ const char *nl = memchr(p, '\n', end - p);
++ size_t len = (nl ? nl : end) - p;
++
++ strbuf_reset(&sb);
++ if (len && p[len - 1] == '\r')
++ len--;
++ strbuf_add(&sb, p, len);
++ parse_oidset_line(set, &sb, algop, fn, cbdata);
++ p = nl ? nl + 1 : end;
++ }
++ strbuf_release(&sb);
++}
+
+ ## oidset.h ##
+@@ oidset.h: void oidset_parse_file_carefully(struct oidset *set, const char *path,
+ const struct git_hash_algo *algop,
+ oidset_parse_tweak_fn fn, void *cbdata);
+
++/*
++ * Similar to oidset_parse_file_carefully(), but parses lines from an
++ * in-memory buffer of 'size' bytes.
++ */
++void oidset_parse_buffer_carefully(struct oidset *set, const char *buf,
++ size_t size,
++ const struct git_hash_algo *algop,
++ oidset_parse_tweak_fn fn, void *cbdata);
++
+ struct oidset_iter {
+ const kh_oid_set_t *set;
+ khiter_t iter;
+
## t/t8013-blame-ignore-revs.sh ##
-@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+@@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chained tags and skips missing tag t
test_cmp expect actual
'
-+# Tests for default .git-blame-ignore-revs file
-+test_expect_success 'setup default .git-blame-ignore-revs' '
++# Tests for default HEAD:.git-blame-ignore-revs blob
++test_expect_success 'setup default HEAD:.git-blame-ignore-revs' '
+ git checkout -b default-file-branch &&
+ test_write_lines line1 line2 >def-file &&
+ git add def-file &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ git commit -m "default mod" &&
+ git tag DEF_B &&
+
-+ git rev-parse DEF_B >.git-blame-ignore-revs
++ git rev-parse DEF_B >.git-blame-ignore-revs &&
++ git add .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "add .git-blame-ignore-revs"
+'
+
-+test_expect_success 'default .git-blame-ignore-revs is used by default' '
++test_expect_success 'default HEAD:.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 &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ test_cmp expect actual
+'
+
-+test_expect_success 'default .git-blame-ignore-revs respected by git annotate' '
++test_expect_success 'default HEAD:.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' '
++test_expect_success 'default HEAD:.git-blame-ignore-revs works from subdirectory' '
+ mkdir -p sub &&
+ (
+ cd sub &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ )
+'
+
-+test_expect_success 'disable default .git-blame-ignore-revs with --no-ignore-revs-file' '
++test_expect_success 'default HEAD:.git-blame-ignore-revs respected in bare repo' '
++ test_when_finished "rm -rf bare.git" &&
++ 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]* 1/s/ .*//p" blame_raw >actual &&
++ git rev-parse DEF_A >expect &&
++ test_cmp expect actual
++'
++
++test_expect_success 'uncommitted .git-blame-ignore-revs changes in working tree are ignored' '
++ test_when_finished "git checkout -- .git-blame-ignore-revs" &&
++ echo "invalid-oid-value" >.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 'disable default HEAD:.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 &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ test_cmp expect actual
+'
+
-+test_expect_success 'disable default .git-blame-ignore-revs with --ignore-revs-file ""' '
++test_expect_success 'disable default HEAD:.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 &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ test_cmp expect actual
+'
+
-+test_expect_success 'disable default .git-blame-ignore-revs with blame.ignoreRevsFile=""' '
++test_expect_success 'disable default HEAD:.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 &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ 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" &&
++test_expect_success 'default HEAD:.git-blame-ignore-revs handles comments and whitespace' '
++ rev_def_b=$(git rev-parse DEF_B) &&
+ {
+ echo "# Leading comment" &&
+ echo "" &&
-+ echo " $(git rev-parse DEF_B) " &&
++ echo " $rev_def_b # inline comment" &&
+ echo "# Trailing comment"
+ } >.git-blame-ignore-revs &&
++ git add .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "comments and whitespace in .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" &&
++test_expect_success 'empty default HEAD:.git-blame-ignore-revs is harmless' '
+ : >.git-blame-ignore-revs &&
-+ git blame def-file
++ git add .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "empty .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 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" &&
++test_expect_success 'committed symlink .git-blame-ignore-revs in HEAD is ignored' '
++ git rm -f .git-blame-ignore-revs &&
+ git rev-parse DEF_B >target_file &&
-+ ln -sf target_file .git-blame-ignore-revs &&
++ test_ln_s_add target_file .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "symlink .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" &&
++test_expect_success 'malformed default HEAD:.git-blame-ignore-revs fails but can be bypassed' '
++ git rm -f .git-blame-ignore-revs &&
+ echo "invalid-oid-value" >.git-blame-ignore-revs &&
++ git add .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "malformed .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
-+'
++ git blame --ignore-revs-file "" def-file &&
++ git -c blame.ignoreRevsFile="" blame 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
++ rev_def_b=$(git rev-parse DEF_B) &&
++ printf "%sQgarbage\n" "$rev_def_b" | q_to_nul >.git-blame-ignore-revs &&
++ git add .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "NUL in .git-blame-ignore-revs" &&
++ test_must_fail git blame def-file 2>err &&
++ test_grep "invalid object name:" err
+'
+
-+test_expect_success 'default .git-blame-ignore-revs combined with config blame.ignoreRevsFile' '
++test_expect_success 'default HEAD:.git-blame-ignore-revs combined with config blame.ignoreRevsFile' '
++ git rev-parse DEF_B >.git-blame-ignore-revs &&
+ test_write_lines line1-modified line2-c >def-file &&
-+ git add def-file &&
++ git add .git-blame-ignore-revs def-file &&
+ test_tick &&
+ git commit -m C &&
+ git tag DEF_C &&
@@ t/t8013-blame-ignore-revs.sh: test_expect_success ignore_merge '
+ 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 &&
++test_expect_success 'blame works when HEAD:.git-blame-ignore-revs does not exist and ignores untracked file' '
++ git rm -f .git-blame-ignore-revs &&
++ test_tick &&
++ git commit -m "remove .git-blame-ignore-revs" &&
++ git rev-parse DEF_B >.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 &&
--
gitgitgadget
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling
2026-10-08 21:07 ` [PATCH v2 0/2] blame: ignore revs in HEAD:.git-blame-ignore-revs by default Ravi Mistry via GitGitGadget
@ 2026-10-08 21:07 ` Ravi Mistry via GitGitGadget
2026-10-09 5:07 ` Junio C Hamano
2026-10-08 21:07 ` [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs Ravi Mistry via GitGitGadget
1 sibling, 1 reply; 11+ messages in thread
From: Ravi Mistry via GitGitGadget @ 2026-10-08 21:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Abhijeetsingh Meena, Kristoffer Haugsbakk,
Phillip Wood, Eric Sunshine, Ravi Mistry, Ravi Mistry
From: Ravi Mistry <rmistry@google.com>
Currently, blame.ignoreRevsFile and --ignore-revs-file only read paths
explicitly configured by the user, which cannot point to an
attacker-controlled file under Git's threat model. An upcoming commit
will teach git-blame(1) and git-annotate(1) to automatically read the
HEAD:.git-blame-ignore-revs blob by default if it exists, exposing the
ignore-revs parser (oidset_parse_file_carefully() in oidset.c) and its
tag-peeling callback (peel_to_commit_oid() in builtin/blame.c) to
upstream-controlled content at a well-known path.
Harden both code paths before enabling the default blob:
- In oidset_parse_file_carefully(), strbuf_getline() reads up to the
next newline and records the full line length in sb.len, including
any embedded NUL bytes. However, strchr(sb.buf, '#') and
parse_oid_hex_algop(sb.buf, &oid, &p, algop) treat sb.buf as a
NUL-terminated string. If a line contains an embedded NUL byte after
a valid object name (such as "<oid>\0garbage" or "<oid>\0# comment"),
*p is '\0' and trailing bytes on the line are silently ignored.
Reject any line containing an embedded NUL byte via memchr() before
stripping comments and whitespace.
- In peel_to_commit_oid(), odb_read_object_info() is called without
OBJECT_INFO_SKIP_FETCH_OBJECT or OBJECT_INFO_QUICK, and deref_tag()
calls parse_object() on tag targets without checking whether the
target object exists locally first. In a partial clone, any missing
commit OID or tag target listed in the ignore-revs file would trigger
lazy promisor fetches and pack directory rescans during git-blame(1).
Use odb_read_object_info_extended() with OBJECT_INFO_LOOKUP_REPLACE |
OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK and peel OBJ_TAG
objects one layer per iteration, verifying that each target object
exists locally and matches the tag's declared type before parsing it.
Signed-off-by: Ravi Mistry <rmistry@google.com>
---
builtin/blame.c | 20 +++++++++++++++++--
oidset.c | 3 +++
t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++++++
3 files changed, 59 insertions(+), 2 deletions(-)
diff --git a/builtin/blame.c b/builtin/blame.c
index 48d5251c6d..6741a7b9df 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -911,21 +911,37 @@ static int is_a_rev(const char *name)
static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
{
struct repository *r = ((struct blame_scoreboard *)cbdata)->repo;
+ enum object_type expected_type = OBJ_ANY;
struct object_id oid;
oidcpy(&oid, oid_ret);
while (1) {
+ unsigned flags = OBJECT_INFO_LOOKUP_REPLACE |
+ OBJECT_INFO_SKIP_FETCH_OBJECT |
+ OBJECT_INFO_QUICK;
+ struct object_info oi = OBJECT_INFO_INIT;
+ enum object_type kind;
struct object *obj;
- int kind = odb_read_object_info(r->objects, &oid, NULL);
+
+ oi.typep = &kind;
+ if (odb_read_object_info_extended(r->objects, &oid, &oi,
+ flags) < 0)
+ return -1;
+ if (expected_type != OBJ_ANY && kind != expected_type)
+ return -1;
if (kind == OBJ_COMMIT) {
oidcpy(oid_ret, &oid);
return 0;
}
if (kind != OBJ_TAG)
return -1;
- obj = deref_tag(r, parse_object(r, &oid), NULL, 0);
+ obj = parse_object(r, &oid);
+ if (!obj || obj->type != OBJ_TAG)
+ return -1;
+ obj = ((struct tag *)obj)->tagged;
if (!obj)
return -1;
+ expected_type = obj->type;
oidcpy(&oid, &obj->oid);
}
}
diff --git a/oidset.c b/oidset.c
index c8ff0b385c..90d39204d3 100644
--- a/oidset.c
+++ b/oidset.c
@@ -85,6 +85,9 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
const char *p;
const char *name;
+ if (memchr(sb.buf, '\0', sb.len))
+ die("invalid object name: %s", sb.buf);
+
/*
* Allow trailing comments, leading whitespace
* (including before commits), and empty or whitespace
diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
index cace00ae8d..70fe509a64 100755
--- a/t/t8013-blame-ignore-revs.sh
+++ b/t/t8013-blame-ignore-revs.sh
@@ -327,4 +327,42 @@ test_expect_success ignore_merge '
test_cmp expect actual
'
+test_expect_success 'ignore-revs-file rejects lines with embedded NUL bytes' '
+ rev_b=$(git rev-parse B) &&
+ printf "%sQgarbage\n" "$rev_b" | q_to_nul >ignore_nul &&
+ test_must_fail git blame file --ignore-revs-file ignore_nul 2>err &&
+ test_grep "invalid object name:" err &&
+
+ printf "%sQ# comment\n" "$rev_b" | q_to_nul >ignore_nul_comment &&
+ test_must_fail git blame file --ignore-revs-file ignore_nul_comment 2>err &&
+ test_grep "invalid object name:" err
+'
+
+test_expect_success 'ignore-revs-file peels chained tags and skips missing tag targets' '
+ test_write_lines BB L2-modified L3 L4 L5 L6 L7 L8 CC >file &&
+ git add file &&
+ test_tick &&
+ git commit -m D &&
+ git tag -a -m "tag 1" D_TAG1 HEAD &&
+ git tag -a -m "tag 2" D_TAG2 D_TAG1 &&
+ git rev-parse D_TAG2 >ignore_tag_chain &&
+ git blame --line-porcelain file --ignore-revs-file ignore_tag_chain >blame_raw &&
+ sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+ git rev-parse A >expect &&
+ test_cmp expect actual &&
+
+ test_config extensions.partialClone origin &&
+ test_config remote.origin.promisor true &&
+ test_config remote.origin.url /nonexistent &&
+ missing_oid=$(test_oid deadbeef) &&
+ bad_tag=$(printf "object %s\ntype commit\ntag bad-tag\ntagger T <t@example.com> 0 +0000\n\nmsg\n" "$missing_oid" |
+ git hash-object -t tag -w --stdin) &&
+ test_write_lines "$missing_oid" "$bad_tag" >ignore_bad_tag &&
+ git blame --line-porcelain file --ignore-revs-file ignore_bad_tag >blame_raw 2>err &&
+ test_must_be_empty err &&
+ sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
+ git rev-parse HEAD >expect &&
+ test_cmp expect actual
+'
+
test_done
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs
2026-10-08 21:07 ` [PATCH v2 0/2] blame: ignore revs in HEAD:.git-blame-ignore-revs by default Ravi Mistry via GitGitGadget
2026-10-08 21:07 ` [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling Ravi Mistry via GitGitGadget
@ 2026-10-08 21:07 ` Ravi Mistry via GitGitGadget
2026-10-09 20:17 ` Junio C Hamano
1 sibling, 1 reply; 11+ messages in thread
From: Ravi Mistry via GitGitGadget @ 2026-10-08 21:07 UTC (permalink / raw)
To: git
Cc: Junio C Hamano, Abhijeetsingh Meena, Kristoffer Haugsbakk,
Phillip Wood, Eric Sunshine, Ravi Mistry, Ravi Mistry
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 add the
HEAD:.git-blame-ignore-revs blob, if it exists, as the initial element
in the list of ignore-revs files in both bare and non-bare
repositories. Reading the committed blob from HEAD rather than the
working tree ensures that local runs match hosting platforms even when
an untracked .git-blame-ignore-revs file is present or a tracked one
has uncommitted local changes.
To ensure consistent precedence and override semantics:
- The default HEAD:.git-blame-ignore-revs entry is added before reading
configuration and CLI options, preserving user and repository config
overrides.
- In git_blame_config(), blame.ignoreRevsFile entries are appended via
string_list_append() rather than inserted in sorted order via
string_list_insert() so that configuration entries preserve their
order relative to the initial default entry.
- The HEAD:.git-blame-ignore-revs tree entry is resolved quietly via
get_oid_with_context(). Its mode is checked with S_ISREG() before
reading the object so that non-regular tree entries (such as a
committed symbolic link whose blob stores a target path rather than
revision IDs, a subdirectory, or a gitlink) are skipped instead of
being read and rejected as malformed object names. The blob is parsed
in memory via a new oidset_parse_buffer_carefully() helper in
oidset.c that shares line parsing with oidset_parse_file_carefully().
- In build_ignorelist(), ignore-revs entries are processed 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 blob without
attempting to read or parse it, allowing users to bypass a malformed
default blob.
Update documentation in blame-options.adoc and config/blame.adoc, and
add comprehensive test coverage in t8013 for the default blob lookup,
subdirectory invocations, bare repositories, uncommitted and untracked
working-tree files, CLI and config overrides, committed symlink
entries, and comments and whitespace handling.
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>
---
Documentation/blame-options.adoc | 8 +-
Documentation/config/blame.adoc | 9 +-
builtin/blame.c | 45 +++++++-
oidset.c | 84 +++++++++-----
oidset.h | 9 ++
t/t8013-blame-ignore-revs.sh | 182 +++++++++++++++++++++++++++++++
6 files changed, 301 insertions(+), 36 deletions(-)
diff --git a/Documentation/blame-options.adoc b/Documentation/blame-options.adoc
index 1ae1222b6b..8d59dcc8f3 100644
--- a/Documentation/blame-options.adoc
+++ b/Documentation/blame-options.adoc
@@ -132,9 +132,11 @@ take effect.
`--ignore-revs-file <file>`::
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.
+ processed after the default `HEAD:.git-blame-ignore-revs` blob (if it
+ exists) and any files specified with the `blame.ignoreRevsFile` config
+ option. An empty file name, `""`, or `--no-ignore-revs-file` will clear
+ the list of revs from previously processed files, including the default
+ `HEAD:.git-blame-ignore-revs` blob.
`--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..153d4ab524 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 the `HEAD:.git-blame-ignore-revs` blob exists, it
+ is added as the initial element in the list of ignore-revs files.
+ Other files listed in the configuration are also used, but an empty
+ element makes all elements that appeared before in the list forgotten.
+ This option will be 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 6741a7b9df..a730ee87ea 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -769,7 +769,7 @@ static int git_blame_config(const char *var, const char *value,
if (ret)
return ret;
if (str)
- string_list_insert(&ignore_revs_file_list, str);
+ string_list_append(&ignore_revs_file_list, str);
free(str);
return 0;
}
@@ -946,17 +946,52 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
}
}
+static void parse_default_ignore_revs_blob(struct blame_scoreboard *sb,
+ const char *name)
+{
+ struct object_context oc;
+ struct object_id oid;
+ enum object_type type;
+ size_t size;
+ char *buf;
+
+ if (get_oid_with_context(the_repository, name, GET_OID_QUIETLY,
+ &oid, &oc))
+ goto out;
+ if (!S_ISREG(oc.mode))
+ goto out;
+
+ buf = odb_read_object(the_repository->objects, &oid, &type, &size);
+ if (!buf)
+ goto out;
+ if (type == OBJ_BLOB)
+ oidset_parse_buffer_carefully(&sb->ignore_list, buf, size,
+ the_repository->hash_algo,
+ peel_to_commit_oid, sb);
+ free(buf);
+
+out:
+ object_context_release(&oc);
+}
+
static void build_ignorelist(struct blame_scoreboard *sb,
struct string_list *ignore_revs_file_list,
struct string_list *ignore_rev_list)
{
struct string_list_item *i;
struct object_id oid;
+ 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);
+ for (idx = start_idx; idx < ignore_revs_file_list->nr; idx++) {
+ i = &ignore_revs_file_list->items[idx];
+ if (i->util)
+ parse_default_ignore_revs_blob(sb, i->string);
else
oidset_parse_file_carefully(&sb->ignore_list, i->string,
the_repository->hash_algo,
@@ -1036,6 +1071,8 @@ int cmd_blame(int argc,
const char *const *opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;
setup_default_color_by_age();
+ string_list_append(&ignore_revs_file_list,
+ "HEAD:.git-blame-ignore-revs")->util = &sb;
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/oidset.c b/oidset.c
index 90d39204d3..8469d03b9b 100644
--- a/oidset.c
+++ b/oidset.c
@@ -70,44 +70,76 @@ void oidset_parse_file(struct oidset *set, const char *path,
oidset_parse_file_carefully(set, path, algop, NULL, NULL);
}
+static void parse_oidset_line(struct oidset *set, struct strbuf *sb,
+ const struct git_hash_algo *algop,
+ oidset_parse_tweak_fn fn, void *cbdata)
+{
+ const char *p;
+ const char *name;
+ struct object_id oid;
+
+ if (memchr(sb->buf, '\0', sb->len))
+ die("invalid object name: %s", sb->buf);
+
+ /*
+ * Allow trailing comments, leading whitespace
+ * (including before commits), and empty or whitespace
+ * only lines.
+ */
+ name = strchr(sb->buf, '#');
+ if (name)
+ strbuf_setlen(sb, name - sb->buf);
+ strbuf_trim(sb);
+ if (!sb->len)
+ return;
+
+ if (parse_oid_hex_algop(sb->buf, &oid, &p, algop) || *p != '\0')
+ die("invalid object name: %s", sb->buf);
+ if (fn && fn(&oid, cbdata))
+ return;
+ oidset_insert(set, &oid);
+}
+
void oidset_parse_file_carefully(struct oidset *set, const char *path,
const struct git_hash_algo *algop,
oidset_parse_tweak_fn fn, void *cbdata)
{
FILE *fp;
struct strbuf sb = STRBUF_INIT;
- struct object_id oid;
fp = fopen(path, "r");
if (!fp)
die("could not open object name list: %s", path);
- while (!strbuf_getline(&sb, fp)) {
- const char *p;
- const char *name;
-
- if (memchr(sb.buf, '\0', sb.len))
- die("invalid object name: %s", sb.buf);
-
- /*
- * Allow trailing comments, leading whitespace
- * (including before commits), and empty or whitespace
- * only lines.
- */
- name = strchr(sb.buf, '#');
- if (name)
- strbuf_setlen(&sb, name - sb.buf);
- strbuf_trim(&sb);
- if (!sb.len)
- continue;
-
- if (parse_oid_hex_algop(sb.buf, &oid, &p, algop) || *p != '\0')
- die("invalid object name: %s", sb.buf);
- if (fn && fn(&oid, cbdata))
- continue;
- oidset_insert(set, &oid);
- }
+ while (!strbuf_getline(&sb, fp))
+ parse_oidset_line(set, &sb, algop, fn, cbdata);
if (ferror(fp))
die_errno("Could not read '%s'", path);
fclose(fp);
strbuf_release(&sb);
}
+
+void oidset_parse_buffer_carefully(struct oidset *set, const char *buf,
+ size_t size,
+ const struct git_hash_algo *algop,
+ oidset_parse_tweak_fn fn, void *cbdata)
+{
+ struct strbuf sb = STRBUF_INIT;
+ const char *p = buf, *end;
+
+ if (!size)
+ return;
+ end = buf + size;
+
+ while (p < end) {
+ const char *nl = memchr(p, '\n', end - p);
+ size_t len = (nl ? nl : end) - p;
+
+ strbuf_reset(&sb);
+ if (len && p[len - 1] == '\r')
+ len--;
+ strbuf_add(&sb, p, len);
+ parse_oidset_line(set, &sb, algop, fn, cbdata);
+ p = nl ? nl + 1 : end;
+ }
+ strbuf_release(&sb);
+}
diff --git a/oidset.h b/oidset.h
index e0f1a6ff4f..667c390842 100644
--- a/oidset.h
+++ b/oidset.h
@@ -98,6 +98,15 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
const struct git_hash_algo *algop,
oidset_parse_tweak_fn fn, void *cbdata);
+/*
+ * Similar to oidset_parse_file_carefully(), but parses lines from an
+ * in-memory buffer of 'size' bytes.
+ */
+void oidset_parse_buffer_carefully(struct oidset *set, const char *buf,
+ size_t size,
+ const struct git_hash_algo *algop,
+ oidset_parse_tweak_fn fn, void *cbdata);
+
struct oidset_iter {
const kh_oid_set_t *set;
khiter_t iter;
diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
index 70fe509a64..3e1b5291aa 100755
--- a/t/t8013-blame-ignore-revs.sh
+++ b/t/t8013-blame-ignore-revs.sh
@@ -365,4 +365,186 @@ test_expect_success 'ignore-revs-file peels chained tags and skips missing tag t
test_cmp expect actual
'
+# Tests for default HEAD:.git-blame-ignore-revs blob
+test_expect_success 'setup default HEAD:.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 &&
+ git add .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "add .git-blame-ignore-revs"
+'
+
+test_expect_success 'default HEAD:.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 HEAD:.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 HEAD:.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 'default HEAD:.git-blame-ignore-revs respected in bare repo' '
+ test_when_finished "rm -rf bare.git" &&
+ 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]* 1/s/ .*//p" blame_raw >actual &&
+ git rev-parse DEF_A >expect &&
+ test_cmp expect actual
+'
+
+test_expect_success 'uncommitted .git-blame-ignore-revs changes in working tree are ignored' '
+ test_when_finished "git checkout -- .git-blame-ignore-revs" &&
+ echo "invalid-oid-value" >.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 'disable default HEAD:.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 HEAD:.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 HEAD:.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 HEAD:.git-blame-ignore-revs handles comments and whitespace' '
+ rev_def_b=$(git rev-parse DEF_B) &&
+ {
+ echo "# Leading comment" &&
+ echo "" &&
+ echo " $rev_def_b # inline comment" &&
+ echo "# Trailing comment"
+ } >.git-blame-ignore-revs &&
+ git add .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "comments and whitespace in .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 HEAD:.git-blame-ignore-revs is harmless' '
+ : >.git-blame-ignore-revs &&
+ git add .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "empty .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 'committed symlink .git-blame-ignore-revs in HEAD is ignored' '
+ git rm -f .git-blame-ignore-revs &&
+ git rev-parse DEF_B >target_file &&
+ test_ln_s_add target_file .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "symlink .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 HEAD:.git-blame-ignore-revs fails but can be bypassed' '
+ git rm -f .git-blame-ignore-revs &&
+ echo "invalid-oid-value" >.git-blame-ignore-revs &&
+ git add .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "malformed .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 &&
+ git -c blame.ignoreRevsFile="" blame def-file &&
+
+ rev_def_b=$(git rev-parse DEF_B) &&
+ printf "%sQgarbage\n" "$rev_def_b" | q_to_nul >.git-blame-ignore-revs &&
+ git add .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "NUL in .git-blame-ignore-revs" &&
+ test_must_fail git blame def-file 2>err &&
+ test_grep "invalid object name:" err
+'
+
+test_expect_success 'default HEAD:.git-blame-ignore-revs combined with config blame.ignoreRevsFile' '
+ git rev-parse DEF_B >.git-blame-ignore-revs &&
+ test_write_lines line1-modified line2-c >def-file &&
+ git add .git-blame-ignore-revs 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 'blame works when HEAD:.git-blame-ignore-revs does not exist and ignores untracked file' '
+ git rm -f .git-blame-ignore-revs &&
+ test_tick &&
+ git commit -m "remove .git-blame-ignore-revs" &&
+ git rev-parse DEF_B >.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
--
gitgitgadget
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling
2026-10-08 21:07 ` [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling Ravi Mistry via GitGitGadget
@ 2026-10-09 5:07 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2026-10-09 5:07 UTC (permalink / raw)
To: Ravi Mistry via GitGitGadget
Cc: git, Abhijeetsingh Meena, Kristoffer Haugsbakk, Phillip Wood,
Eric Sunshine, Ravi Mistry
"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
> - In oidset_parse_file_carefully(), strbuf_getline() reads up to the
> next newline and records the full line length in sb.len, including
> any embedded NUL bytes. However, strchr(sb.buf, '#') and
> parse_oid_hex_algop(sb.buf, &oid, &p, algop) treat sb.buf as a
> NUL-terminated string. If a line contains an embedded NUL byte after
> a valid object name (such as "<oid>\0garbage" or "<oid>\0# comment"),
> *p is '\0' and trailing bytes on the line are silently ignored.
> Reject any line containing an embedded NUL byte via memchr() before
> stripping comments and whitespace.
Maybe I am slow, but I do not immediately see why ignoring
everything after the first NUL is a problem. A call to
strbuf_trim() is ineffective at trimming whitespace that appears
immediately before such a NUL. For example, while
cf9bdb1...f6092d # comment LF
would feed the leading 'cf9bdb1...f6092d' part (after stripping
whitespace before '#') to parse_oid_hex_algop(), this
cf9bdb1...f6092d NUL comment LF
would keep the whitespace after '92d' and cause the parsing to
fail. I do not see any security implications here.
On the other hand ...
> - In peel_to_commit_oid(), odb_read_object_info() is called without
> OBJECT_INFO_SKIP_FETCH_OBJECT or OBJECT_INFO_QUICK, and deref_tag()
> calls parse_object() on tag targets without checking whether the
> target object exists locally first. In a partial clone, any missing
> commit OID or tag target listed in the ignore-revs file would trigger
> lazy promisor fetches and pack directory rescans during git-blame(1).
> Use odb_read_object_info_extended() with OBJECT_INFO_LOOKUP_REPLACE |
> OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK and peel OBJ_TAG
> objects one layer per iteration, verifying that each target object
> exists locally and matches the tag's declared type before parsing it.
... this may be a very reasonable thing to do, I would think. In a
shallow clone, if we are not auto-deepening the shallow boundary
during a "git blame" session, we have no reason to lazy fetch
entries in the ignore file that are older than the shallow boundary.
> diff --git a/oidset.c b/oidset.c
> index c8ff0b385c..90d39204d3 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -85,6 +85,9 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
> const char *p;
> const char *name;
>
> + if (memchr(sb.buf, '\0', sb.len))
> + die("invalid object name: %s", sb.buf);
A file with such an entry is rejected and the entire operation is
aborted as suspected attack attempt, which feels like striking the
balance between usability and security at a wrong place.
But a line with broken object name already is rejected with "die()"
with the existing code, so it may be OK.
> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
> index cace00ae8d..70fe509a64 100755
> --- a/t/t8013-blame-ignore-revs.sh
> +++ b/t/t8013-blame-ignore-revs.sh
> @@ -327,4 +327,42 @@ test_expect_success ignore_merge '
> test_cmp expect actual
> '
>
> +test_expect_success 'ignore-revs-file rejects lines with embedded NUL bytes' '
> + rev_b=$(git rev-parse B) &&
> + printf "%sQgarbage\n" "$rev_b" | q_to_nul >ignore_nul &&
> + test_must_fail git blame file --ignore-revs-file ignore_nul 2>err &&
> + test_grep "invalid object name:" err &&
> +
> + printf "%sQ# comment\n" "$rev_b" | q_to_nul >ignore_nul_comment &&
> + test_must_fail git blame file --ignore-revs-file ignore_nul_comment 2>err &&
> + test_grep "invalid object name:" err
> +'
> +
> +test_expect_success 'ignore-revs-file peels chained tags and skips missing tag targets' '
> + test_write_lines BB L2-modified L3 L4 L5 L6 L7 L8 CC >file &&
> + git add file &&
> + test_tick &&
> + git commit -m D &&
> + git tag -a -m "tag 1" D_TAG1 HEAD &&
> + git tag -a -m "tag 2" D_TAG2 D_TAG1 &&
> + git rev-parse D_TAG2 >ignore_tag_chain &&
> + git blame --line-porcelain file --ignore-revs-file ignore_tag_chain >blame_raw &&
> + sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
> + git rev-parse A >expect &&
> + test_cmp expect actual &&
> +
> + test_config extensions.partialClone origin &&
> + test_config remote.origin.promisor true &&
> + test_config remote.origin.url /nonexistent &&
> + missing_oid=$(test_oid deadbeef) &&
> + bad_tag=$(printf "object %s\ntype commit\ntag bad-tag\ntagger T <t@example.com> 0 +0000\n\nmsg\n" "$missing_oid" |
> + git hash-object -t tag -w --stdin) &&
> + test_write_lines "$missing_oid" "$bad_tag" >ignore_bad_tag &&
> + git blame --line-porcelain file --ignore-revs-file ignore_bad_tag >blame_raw 2>err &&
> + test_must_be_empty err &&
> + sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
> + git rev-parse HEAD >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs
2026-10-08 21:07 ` [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs Ravi Mistry via GitGitGadget
@ 2026-10-09 20:17 ` Junio C Hamano
0 siblings, 0 replies; 11+ messages in thread
From: Junio C Hamano @ 2026-10-09 20:17 UTC (permalink / raw)
To: Ravi Mistry via GitGitGadget
Cc: git, Abhijeetsingh Meena, Kristoffer Haugsbakk, Phillip Wood,
Eric Sunshine, Ravi Mistry
"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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 add the
> HEAD:.git-blame-ignore-revs blob, if it exists, as the initial element
> in the list of ignore-revs files in both bare and non-bare
> repositories. Reading the committed blob from HEAD rather than the
> working tree ensures that local runs match hosting platforms even when
> an untracked .git-blame-ignore-revs file is present or a tracked one
> has uncommitted local changes.
>
> To ensure consistent precedence and override semantics:
> - The default HEAD:.git-blame-ignore-revs entry is added before reading
> configuration and CLI options, preserving user and repository config
> overrides.
> - In git_blame_config(), blame.ignoreRevsFile entries are appended via
> string_list_append() rather than inserted in sorted order via
> string_list_insert() so that configuration entries preserve their
> order relative to the initial default entry.
> - The HEAD:.git-blame-ignore-revs tree entry is resolved quietly via
> get_oid_with_context(). Its mode is checked with S_ISREG() before
> reading the object so that non-regular tree entries (such as a
> committed symbolic link whose blob stores a target path rather than
> revision IDs, a subdirectory, or a gitlink) are skipped instead of
> being read and rejected as malformed object names. The blob is parsed
> in memory via a new oidset_parse_buffer_carefully() helper in
> oidset.c that shares line parsing with oidset_parse_file_carefully().
> - In build_ignorelist(), ignore-revs entries are processed 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 blob without
> attempting to read or parse it, allowing users to bypass a malformed
> default blob.
>
> Update documentation in blame-options.adoc and config/blame.adoc, and
> add comprehensive test coverage in t8013 for the default blob lookup,
> subdirectory invocations, bare repositories, uncommitted and untracked
> working-tree files, CLI and config overrides, committed symlink
> entries, and comments and whitespace handling.
>
> 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>
These Helped-by: drew my attention as none of these folks commented
on v1 of this series. I do see they have helped the original series
<pull.1809.v2.git.1728707867.gitgitgadget@gmail.com>, but it is not
clear how much their inputs have survivied to this version.
They are all CC'ed so they can give their Acked-by: or Reviewed-by:
on this round if they want.
[...]
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 6741a7b9df..a730ee87ea 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -769,7 +769,7 @@ static int git_blame_config(const char *var, const char *value,
> if (ret)
> return ret;
> if (str)
> - string_list_insert(&ignore_revs_file_list, str);
> + string_list_append(&ignore_revs_file_list, str);
> free(str);
> return 0;
> }
Good, and the log message is clear why we make this change.
> @@ -946,17 +946,52 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
> }
> }
>
> +static void parse_default_ignore_revs_blob(struct blame_scoreboard *sb,
> + const char *name)
> +{
> + struct object_context oc;
> + struct object_id oid;
> + enum object_type type;
> + size_t size;
> + char *buf;
> +
> + if (get_oid_with_context(the_repository, name, GET_OID_QUIETLY,
> + &oid, &oc))
> + goto out;
> + if (!S_ISREG(oc.mode))
> + goto out;
> +
> + buf = odb_read_object(the_repository->objects, &oid, &type, &size);
> + if (!buf)
> + goto out;
> + if (type == OBJ_BLOB)
> + oidset_parse_buffer_carefully(&sb->ignore_list, buf, size,
> + the_repository->hash_algo,
> + peel_to_commit_oid, sb);
> + free(buf);
> +
> +out:
> + object_context_release(&oc);
> +}
OK.
> static void build_ignorelist(struct blame_scoreboard *sb,
> struct string_list *ignore_revs_file_list,
> struct string_list *ignore_rev_list)
> {
> struct string_list_item *i;
> struct object_id oid;
> + 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;
> + }
OK, we make two passes, and during the first pass, we find where the
last "empty" entry that signals "forget everything you have seen" is.
> 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);
> + for (idx = start_idx; idx < ignore_revs_file_list->nr; idx++) {
And we scan starting from there. Very clean.
> + i = &ignore_revs_file_list->items[idx];
> + if (i->util)
> + parse_default_ignore_revs_blob(sb, i->string);
> else
> oidset_parse_file_carefully(&sb->ignore_list, i->string,
> the_repository->hash_algo,
> @@ -1036,6 +1071,8 @@ int cmd_blame(int argc,
> const char *const *opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;
>
> setup_default_color_by_age();
> + string_list_append(&ignore_revs_file_list,
> + "HEAD:.git-blame-ignore-revs")->util = &sb;
Cute. This takes advantage of the fact that everybody else just
appends to the string_list without populating the .util member.
> diff --git a/oidset.c b/oidset.c
> index 90d39204d3..8469d03b9b 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -70,44 +70,76 @@ void oidset_parse_file(struct oidset *set, const char *path,
> oidset_parse_file_carefully(set, path, algop, NULL, NULL);
> }
>
> +static void parse_oidset_line(struct oidset *set, struct strbuf *sb,
> + const struct git_hash_algo *algop,
> + oidset_parse_tweak_fn fn, void *cbdata)
> +{
> + const char *p;
> + const char *name;
> + struct object_id oid;
> +
> + if (memchr(sb->buf, '\0', sb->len))
> + die("invalid object name: %s", sb->buf);
> +
> + /*
> + * Allow trailing comments, leading whitespace
> + * (including before commits), and empty or whitespace
> + * only lines.
> + */
> + name = strchr(sb->buf, '#');
> + if (name)
> + strbuf_setlen(sb, name - sb->buf);
> + strbuf_trim(sb);
> + if (!sb->len)
> + return;
> +
> + if (parse_oid_hex_algop(sb->buf, &oid, &p, algop) || *p != '\0')
> + die("invalid object name: %s", sb->buf);
> + if (fn && fn(&oid, cbdata))
> + return;
> + oidset_insert(set, &oid);
> +}
> +
> void oidset_parse_file_carefully(struct oidset *set, const char *path,
> const struct git_hash_algo *algop,
> oidset_parse_tweak_fn fn, void *cbdata)
> {
> FILE *fp;
> struct strbuf sb = STRBUF_INIT;
> - struct object_id oid;
>
> fp = fopen(path, "r");
> if (!fp)
> die("could not open object name list: %s", path);
> - while (!strbuf_getline(&sb, fp)) {
> - const char *p;
> - const char *name;
> -
> - if (memchr(sb.buf, '\0', sb.len))
> - die("invalid object name: %s", sb.buf);
> -
> - /*
> - * Allow trailing comments, leading whitespace
> - * (including before commits), and empty or whitespace
> - * only lines.
> - */
> - name = strchr(sb.buf, '#');
> - if (name)
> - strbuf_setlen(&sb, name - sb.buf);
> - strbuf_trim(&sb);
> - if (!sb.len)
> - continue;
> -
> - if (parse_oid_hex_algop(sb.buf, &oid, &p, algop) || *p != '\0')
> - die("invalid object name: %s", sb.buf);
> - if (fn && fn(&oid, cbdata))
> - continue;
> - oidset_insert(set, &oid);
> - }
> + while (!strbuf_getline(&sb, fp))
> + parse_oidset_line(set, &sb, algop, fn, cbdata);
> if (ferror(fp))
> die_errno("Could not read '%s'", path);
> fclose(fp);
> strbuf_release(&sb);
> }
Shouldn't the above refactoring have been part of the previous step
instead?
Thanks.
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2026-10-09 20:17 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-11 23:29 [PATCH] blame: default to ignoring revisions in .git-blame-ignore-revs Ravi Mistry via GitGitGadget
2026-09-30 14:59 ` Ravi Mistry
2026-10-05 15:37 ` Junio C Hamano
2026-10-05 21:12 ` Ravi Mistry
2026-10-07 17:43 ` Junio C Hamano
2026-10-07 18:07 ` Ravi Mistry
2026-10-08 21:07 ` [PATCH v2 0/2] blame: ignore revs in HEAD:.git-blame-ignore-revs by default Ravi Mistry via GitGitGadget
2026-10-08 21:07 ` [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling Ravi Mistry via GitGitGadget
2026-10-09 5:07 ` Junio C Hamano
2026-10-08 21:07 ` [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs Ravi Mistry via GitGitGadget
2026-10-09 20:17 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox