Skip to contentSkip to conversation

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

PATCH In review Ravi Mistry via GitGitGadget 5 participants 40 messages Sep 11 – Oct 15 Documentation

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 blame: default to ignoring revisions in .git-blame-ignore-revs

From: Ravi Mistry <[email protected]>

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.

---
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/[email protected]/

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(-)
Documentation/blame-options.adoc +4 -2
index 1ae1222b6b..17f5734d61 100644
@@ -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
Documentation/config/blame.adoc +6 -3
index 4d047c1790..6f9be627e1 100644
@@ -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
builtin/blame.c +35 -7

74 lines not on this page · raw message

t/t8013-blame-ignore-revs.sh +151 -0

155 lines not on this page · raw message

base-commit: fa7f9290efe2bd22dd736689597b474b93798e11

Signature · 1 line
gitgitgadget
#1 lore raw

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!

#2 lore raw
In reply to Ravi Mistry via GitGitGadget

Documentation/config/blame.adoc blame.showRoot:: · line 31

Quoted 12 lines · Ravi Mistry via GitGitGadget

blame.ignoreRevsFile::

Documentation/config/blame.adoc +6 -3
 	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.

#3 lore raw
Quoted 11 lines · Junio C Hamano

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.

Quoted 3 lines

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

#4 lore raw
Quoted 32 lines · Ravi Mistry
Quoted 11 lines · Junio C Hamano

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.

Quoted 3 lines

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).

Quoted 6 lines

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.

#5 lore raw
Quoted 10 lines · Junio C Hamano

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

#6 lore raw

v2 cover blame: ignore revs in HEAD:.git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

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/[email protected]/)
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 <[email protected]>
      
       ## 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 <[email protected]>
          Helped-by: Kristoffer Haugsbakk <[email protected]>
     @@ 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 &&
Signature · 1 line
gitgitgadget
#7 lore raw

PATCH v2 1/2 blame: harden ignore-revs parser and tag peeling

From: Ravi Mistry <[email protected]>

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.

---
 builtin/blame.c              | 20 +++++++++++++++++--
 oidset.c                     |  3 +++
 t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)
builtin/blame.c +18 -2
index 48d5251c6d..6741a7b9df 100644
@@ -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);
 	}
 }
oidset.c +3 -0
index c8ff0b385c..90d39204d3 100644
@@ -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
t/t8013-blame-ignore-revs.sh +38 -0
index cace00ae8d..70fe509a64 100755
@@ -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 <[email protected]> 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
Signature · 1 line
gitgitgadget
#10 lore raw

PATCH v2 2/2 blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

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.

---
 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(-)
Documentation/blame-options.adoc +5 -3
index 1ae1222b6b..8d59dcc8f3 100644
@@ -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
Documentation/config/blame.adoc +6 -3
index 4d047c1790..153d4ab524 100644
@@ -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
builtin/blame.c +41 -4

71 lines not on this page · raw message

oidset.c +58 -26

102 lines not on this page · raw message

oidset.h +9 -0
index e0f1a6ff4f..667c390842 100644
@@ -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;
t/t8013-blame-ignore-revs.sh +182 -0

186 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#8 lore raw

Re: [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling

In reply to Ravi Mistry via GitGitGadget
Quoted 9 lines · Ravi Mistry via GitGitGadget

- 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 ...

Quoted 10 lines

- 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.

oidset.c void oidset_parse_file_carefully(struct oidset *set, const char *path, · line 89

Quoted 10 lines
oidset.c +2 -0
index c8ff0b385c..90d39204d3 100644
@@ -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.

Quoted 47 lines
t/t8013-blame-ignore-revs.sh +38 -0
index cace00ae8d..70fe509a64 100755
@@ -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 <[email protected]> 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
#11 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Ravi Mistry via GitGitGadget
Quoted 61 lines · Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

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.

Quoted 4 lines

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 <[email protected]>, 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.

[...]

builtin/blame.c static int git_blame_config(const char *var, const char *value, · line 775

Quoted 13 lines
builtin/blame.c +1 -1
index 6741a7b9df..a730ee87ea 100644
@@ -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.

Quoted 21 lines
builtin/blame.c +17 -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)

builtin/blame.c static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata) · line 975

Quoted 10 lines
builtin/blame.c +10 -0
+		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.

builtin/blame.c static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata) · line 988

Quoted 12 lines
builtin/blame.c +6 -0
 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.

builtin/blame.c static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata) · line 991

Quoted 5 lines
builtin/blame.c +1 -3
 	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.

builtin/blame.c int cmd_blame(int argc, · line 1075

Quoted 12 lines
builtin/blame.c +5 -0
+		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.

Quoted 71 lines
oidset.c +30 -23
index 90d39204d3..8469d03b9b 100644
@@ -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))

oidset.c void oidset_parse_file(struct oidset *set, const char *path, · line 119

Quoted 10 lines
oidset.c +2 -3
-			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.

#12 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

Quoted 6 lines · Junio C Hamano
Quoted 4 lines · Ravi Mistry via GitGitGadget
Quoted 7 lines

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 <[email protected]>, 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.

For what it's worth, having skimmed the old thread, if anything did survive from my reviews/recommendations, it would have been only extremely minor style fixups.

#13 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Junio C Hamano
Quoted 4 lines · Junio C Hamano

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 <[email protected]>, but it is not clear how much their inputs have survivied to this version.

From that earlier thread, Phillip suggested explaining why commit ae3f36dea1 originally avoided a default file, Kristoffer suggested the config override model and documenting --no-ignore-revs-file, and as Eric noted, his comments were minor structural suggestions such as updating the docs and placing the tests in t8013.

Quoted 10 lines

oidset.c void oidset_parse_file(struct oidset *set, const char *path, · line 119

Quoted 7 lines
oidset.c +2 -0
+	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?

I extracted parse_oidset_line() in patch 2/2 because that is where the second caller (oidset_parse_buffer_carefully()) was added, though extracting it in patch 1/2 would avoid touching that loop in both patches. Happy to move this into patch 1/2 if preferred.

Thanks!

#9 lore raw

Re: [PATCH v2 1/2] blame: harden ignore-revs parser and tag peeling

In reply to Junio C Hamano
Quoted 22 lines · Junio C Hamano

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.

oidset.c void oidset_parse_file_carefully(struct oidset *set, const char *path, · line 89

Quoted 2 lines
oidset.c +2 -0
+		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.

Quoted 2 lines

But a line with broken object name already is rejected with "die()" with the existing code, so it may be OK.

Agreed, there is no security implication there. Rejecting "<oid>\0garbage" (when there is no space before the NUL) was only for consistency with how other non-comment trailing garbage on a line is rejected with die(). If we end up rerolling the series, I am happy to drop the NUL check if you prefer.

Thanks!

#14 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Ravi Mistry via GitGitGadget
Quoted 17 lines · Ravi Mistry via GitGitGadget
builtin/blame.c +17 -0
+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)

builtin/blame.c static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata) · line 975

Quoted 10 lines
builtin/blame.c +10 -0
+		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);
+}

Since the scoreboard is passed around throughout the subsystem and has a pointer to the repository it is working with, a new function like this one does not have to add new references to the_repository.

I do not think it is ultra urgent, but I suspect that many of existing references in this file to the_repository happen in functions that take the scoreboard pointer, and they should be able to replace the_repository with sb->repo more or less mechanically.

#22 lore raw

v3 cover blame: ignore revs in HEAD:.git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

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/[email protected]/)
and addresses https://github.com/gitgitgadget/git/issues/1494.

Changes since v2:

* Patch 2/2 uses sb->repo instead of the_repository in parse_default_ignore_revs_blob().

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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2224/rmistry/blame-default-ignore-revs-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/2224

Range-diff vs v2:

 1:  2e12486c0d = 1:  2e12486c0d blame: harden ignore-revs parser and tag peeling
 2:  35e303d65b ! 2:  72a87d1c34 blame: ignore revs in HEAD:.git-blame-ignore-revs
     @@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
      +	size_t size;
      +	char *buf;
      +
     -+	if (get_oid_with_context(the_repository, name, GET_OID_QUIETLY,
     ++	if (get_oid_with_context(sb->repo, 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);
     ++	buf = odb_read_object(sb->repo->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,
     ++					      sb->repo->hash_algo,
      +					      peel_to_commit_oid, sb);
      +	free(buf);
      +
Signature · 1 line
gitgitgadget
#23 lore raw

PATCH v3 1/2 blame: harden ignore-revs parser and tag peeling

From: Ravi Mistry <[email protected]>

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.

---
 builtin/blame.c              | 20 +++++++++++++++++--
 oidset.c                     |  3 +++
 t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)
builtin/blame.c +18 -2
index 48d5251c6d..6741a7b9df 100644
@@ -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);
 	}
 }
oidset.c +3 -0
index c8ff0b385c..90d39204d3 100644
@@ -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
t/t8013-blame-ignore-revs.sh +38 -0
index cace00ae8d..70fe509a64 100755
@@ -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 <[email protected]> 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
Signature · 1 line
gitgitgadget
#24 lore raw

PATCH v3 2/2 blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

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.

---
 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(-)
Documentation/blame-options.adoc +5 -3
index 1ae1222b6b..8d59dcc8f3 100644
@@ -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
Documentation/config/blame.adoc +6 -3
index 4d047c1790..153d4ab524 100644
@@ -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
builtin/blame.c +41 -4

71 lines not on this page · raw message

oidset.c +58 -26

102 lines not on this page · raw message

oidset.h +9 -0
index e0f1a6ff4f..667c390842 100644
@@ -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;
t/t8013-blame-ignore-revs.sh +182 -0

186 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#15 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Junio C Hamano
Quoted 3 lines · Junio C Hamano

Since the scoreboard is passed around throughout the subsystem and has a pointer to the repository it is working with, a new function like this one does not have to add new references to the_repository.

Since there is an ongoing effort to reduce uses of the_repository in the codebase, I went ahead and switched parse_default_ignore_revs_blob() to use sb->repo in v3.

Thanks!

#16 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

Quoted 11 lines · Ravi Mistry
Quoted 3 lines · Junio C Hamano

Since the scoreboard is passed around throughout the subsystem and has a pointer to the repository it is working with, a new function like this one does not have to add new references to the_repository.

Since there is an ongoing effort to reduce uses of the_repository in the codebase, I went ahead and switched parse_default_ignore_revs_blob() to use sb->repo in v3.

Thanks!

Sounds good, and v3 looks good. Let me mark the topic ready for 'next' unless we hear objections in a few days.

Thanks.

#25 lore raw

Re: [PATCH v3 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Ravi Mistry via GitGitGadget

Hi Ravi

Quoted 15 lines · Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

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.

Quoted 8 lines

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.

This is nicely explained

Quoted 7 lines

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.

I can see we'd want to use the committed version in a bare repository, but it seems strange to me to ignore uncommitted changes in a non-bare repository. We don't do that for other special files like .gitattributes. It is rather inconvinient if the user has to explicitly pass the file inorder to test the effect of local modification to .git-brame-ignore-revs. To me it would make more sense to automatically use .git-blame-ignore-revs file from the working tree when that path is tracked by git.

I did wonder if we wanted to automatically support .git-blame-ignore-revs in the root of a subtree, but I don't think there is an easy way to do that without visiting all the directories containing tracked files which makes it too expensive.

One final thought - do we want to limit the size of the blob that we're prepared to parse as we do for .gitattributes? I had a very quick look at the khash code which is used by 'struct oidset' and it seems to use 32 bit integers for the number of entries. I did not spend anytime trying to figure out what happens if you try and store 2^32 entires.

Thanks

Phillip

Quoted 478 lines

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.

---
  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(-)
Documentation/blame-options.adoc +5 -3
index 1ae1222b6b..8d59dcc8f3 100644
@@ -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
Documentation/config/blame.adoc +6 -3
index 4d047c1790..153d4ab524 100644
@@ -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
builtin/blame.c +41 -4
index 6741a7b9df..f5b944f182 100644
@@ -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(sb->repo, name, GET_OID_QUIETLY,
+				 &oid, &oc))
+		goto out;
+	if (!S_ISREG(oc.mode))
+		goto out;
+
+	buf = odb_read_object(sb->repo->objects, &oid, &type, &size);
+	if (!buf)
+		goto out;
+	if (type == OBJ_BLOB)
+		oidset_parse_buffer_carefully(&sb->ignore_list, buf, size,
+					      sb->repo->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;
oidset.c +58 -26
index 90d39204d3..8469d03b9b 100644
@@ -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);
+}
oidset.h +9 -0
index e0f1a6ff4f..667c390842 100644
@@ -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;
t/t8013-blame-ignore-revs.sh +182 -0
index 70fe509a64..3e1b5291aa 100755
@@ -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
#17 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Junio C Hamano
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Ravi Mistry
Quoted 3 lines · Junio C Hamano

Since the scoreboard is passed around throughout the subsystem and has a pointer to the repository it is working with, a new function like this one does not have to add new references to the_repository.

Quoted 8 lines
Quoted 5 lines

Since there is an ongoing effort to reduce uses of the_repository in the codebase, I went ahead and switched parse_default_ignore_revs_blob() to use sb->repo in v3.

Thanks!

Sounds good, and v3 looks good. Let me mark the topic ready for 'next' unless we hear objections in a few days.

I've left a couple of comments on v3. Ignoring uncommitted changes in the file in the working tree seems a bit odd to me and we seem to be prepared to load a file of any size from an untrusted source.

Thanks

Phillip

#26 lore raw

Re: [PATCH v3 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Phillip Wood

Hi Phillip!

Quoted 8 lines · Phillip Wood

I can see we'd want to use the committed version in a bare repository, but it seems strange to me to ignore uncommitted changes in a non-bare repository. We don't do that for other special files like .gitattributes. It is rather inconvinient if the user has to explicitly pass the file inorder to test the effect of local modification to .git-brame-ignore-revs. To me it would make more sense to automatically use .git-blame-ignore-revs file from the working tree when that path is tracked by git.

In v1, this patch originally read .git-blame-ignore-revs from the working tree in non-bare repositories. I switched to reading the HEAD:.git-blame-ignore-revs blob in v2 based on Junio's review and suggestions on v1
(https://lore.kernel.org/git/[email protected]/).

Quoted 4 lines

I did wonder if we wanted to automatically support .git-blame-ignore-revs in the root of a subtree, but I don't think there is an easy way to do that without visiting all the directories containing tracked files which makes it too expensive.

Agreed. By convention across the ecosystem (GitHub, GitLab, and soon Gerrit and Gitiles), .git-blame-ignore-revs is read from the root of the repository, so only looking at the root keeps it simple and avoids the overhead of walking subdirectories.

Quoted 5 lines

One final thought - do we want to limit the size of the blob that we're prepared to parse as we do for .gitattributes? I had a very quick look at the khash code which is used by 'struct oidset' and it seems to use 32 bit integers for the number of entries. I did not spend anytime trying to figure out what happens if you try and store 2^32 entires.

In parse_oidset_line(), peel_to_commit_oid() runs before oidset_insert() and skips any OID that is not an existing local commit in the repository, and duplicate OIDs hash to the same entry. So the oidset (and khash) will not have more entries than the number of local commits in the repository. That said, if you and Junio would like a blob size cap in parse_default_ignore_revs_blob() (similar to ATTR_MAX_FILE_SIZE), I am happy to add one.

Thanks!

#18 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Phillip Wood
Quoted 3 lines · Phillip Wood

I've left a couple of comments on v3. Ignoring uncommitted changes in the file in the working tree seems a bit odd to me and we seem to be prepared to load a file of any size from an untrusted source.

The latter sounds like a sensible thing to worry about. Thanks for bringing it up.

I do not have strong opinions either way on the former myself (the behaviour implemented is to attempt matching what the hosting sites do, which obviously work from the committed copy). It indeed feels more intuitive if the checked out copy (or even the file before it gets added for the first time) were consulted.

Thanks.

#27 lore raw

v4 cover blame: ignore revs in HEAD:.git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

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/[email protected]/)
and addresses https://github.com/gitgitgadget/git/issues/1494.

Changes since v3:

* Patch 2/2 caps the size of the default HEAD:.git-blame-ignore-revs blob at IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) via odb_read_object_info() before calling odb_read_object(), skipping oversized blobs with a warning without loading them into memory.

Changes since v2:

* Patch 2/2 uses sb->repo instead of the_repository in parse_default_ignore_revs_blob().

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                  |  74 +++++++++-
 oidset.c                         |  81 ++++++++---
 oidset.h                         |   9 ++
 t/t8013-blame-ignore-revs.sh     | 235 +++++++++++++++++++++++++++++++
 6 files changed, 381 insertions(+), 35 deletions(-)

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

Range-diff vs v3:

 1:  2e12486c0d = 1:  2e12486c0d blame: harden ignore-revs parser and tag peeling
 2:  72a87d1c34 ! 2:  76d597dfa8 blame: ignore revs in HEAD:.git-blame-ignore-revs
     @@ Commit message
            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().
     +      being read and rejected as malformed object names. Its size is checked
     +      via odb_read_object_info() and capped at IGNORE_REVS_MAX_FILE_SIZE
     +      (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore
     +      overly large gitattributes files", 2022-12-01)) so an oversized blob
     +      is skipped with a warning without being loaded into memory. 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
     @@ Commit message
          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.
     +    entries, overly large blobs, and comments and whitespace handling.
      
          Based-on-patch-by: Abhijeetsingh Meena <[email protected]>
          Helped-by: Kristoffer Haugsbakk <[email protected]>
     @@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
       	}
       }
       
     ++#define IGNORE_REVS_MAX_FILE_SIZE (100 * 1024 * 1024)
     ++
      +static void parse_default_ignore_revs_blob(struct blame_scoreboard *sb,
      +					   const char *name)
      +{
     @@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
      +	if (!S_ISREG(oc.mode))
      +		goto out;
      +
     ++	if (odb_read_object_info(sb->repo->objects, &oid, &size) != OBJ_BLOB)
     ++		goto out;
     ++	if (size >= IGNORE_REVS_MAX_FILE_SIZE) {
     ++		warning(_("ignoring overly large ignore-revs blob '%s'"), name);
     ++		goto out;
     ++	}
     ++
      +	buf = odb_read_object(sb->repo->objects, &oid, &type, &size);
      +	if (!buf)
      +		goto out;
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +'
      +
      +test_expect_success 'blame works when HEAD:.git-blame-ignore-revs does not exist and ignores untracked file' '
     ++	test_when_finished "rm -f .git-blame-ignore-revs" &&
      +	git rm -f .git-blame-ignore-revs &&
      +	test_tick &&
      +	git commit -m "remove .git-blame-ignore-revs" &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	git rev-parse DEF_B >expect &&
      +	test_cmp expect actual
      +'
     ++
     ++test_expect_success EXPENSIVE 'overly large default HEAD:.git-blame-ignore-revs blob is ignored' '
     ++	blob=$(dd if=/dev/zero bs=1048576 count=101 2>/dev/null |
     ++		git hash-object -w --stdin) &&
     ++	git update-index --add --cacheinfo 100644,$blob,.git-blame-ignore-revs &&
     ++	test_tick &&
     ++	git commit -m "overly large .git-blame-ignore-revs" &&
     ++	test_when_finished "git reset --hard HEAD~" &&
     ++	git blame --line-porcelain def-file >blame_raw 2>err &&
     ++	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_grep "warning: ignoring overly large ignore-revs blob '\''HEAD:.git-blame-ignore-revs'\''" err
     ++'
      +
       test_done
Signature · 1 line
gitgitgadget
#28 lore raw

PATCH v4 1/2 blame: harden ignore-revs parser and tag peeling

From: Ravi Mistry <[email protected]>

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.

---
 builtin/blame.c              | 20 +++++++++++++++++--
 oidset.c                     |  3 +++
 t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)
builtin/blame.c +18 -2
index 48d5251c6d..6741a7b9df 100644
@@ -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);
 	}
 }
oidset.c +3 -0
index c8ff0b385c..90d39204d3 100644
@@ -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
t/t8013-blame-ignore-revs.sh +38 -0
index cace00ae8d..70fe509a64 100755
@@ -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 <[email protected]> 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
Signature · 1 line
gitgitgadget
#29 lore raw

PATCH v4 2/2 blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

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. Its size is checked via odb_read_object_info() and capped at IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) so an oversized blob is skipped with a warning without being loaded into memory. 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, overly large blobs, and comments and whitespace handling.

---
 Documentation/blame-options.adoc |   8 +-
 Documentation/config/blame.adoc  |   9 +-
 builtin/blame.c                  |  54 ++++++++-
 oidset.c                         |  84 +++++++++----
 oidset.h                         |   9 ++
 t/t8013-blame-ignore-revs.sh     | 197 +++++++++++++++++++++++++++++++
 6 files changed, 325 insertions(+), 36 deletions(-)
Documentation/blame-options.adoc +5 -3
index 1ae1222b6b..8d59dcc8f3 100644
@@ -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
Documentation/config/blame.adoc +6 -3
index 4d047c1790..153d4ab524 100644
@@ -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
builtin/blame.c +50 -4

80 lines not on this page · raw message

oidset.c +58 -26

102 lines not on this page · raw message

oidset.h +9 -0
index e0f1a6ff4f..667c390842 100644
@@ -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;
t/t8013-blame-ignore-revs.sh +197 -0

201 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#19 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Junio C Hamano
Quoted 6 lines · Junio C Hamano
Quoted 3 lines · Phillip Wood

I've left a couple of comments on v3. Ignoring uncommitted changes in the file in the working tree seems a bit odd to me and we seem to be prepared to load a file of any size from an untrusted source.

Quoted 8 lines

The latter sounds like a sensible thing to worry about. Thanks for bringing it up.

I do not have strong opinions either way on the former myself (the behaviour implemented is to attempt matching what the hosting sites do, which obviously work from the committed copy). It indeed feels more intuitive if the checked out copy (or even the file before it gets added for the first time) were consulted.

I have sent v4, which caps the size of the default HEAD:.git-blame-ignore-revs blob at 100MB (matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) so an oversized blob is skipped with a warning without being loaded into memory.

For v4, I left the behavior as reading from HEAD:.git-blame-ignore-revs to match hosting sites for now. Maybe consulting the working tree could be explored in the future in a separate series if desired, or please let me know if you or Phillip would prefer to change it in this series.

Thanks!

#20 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

Quoted 4 lines · Ravi Mistry

For v4, I left the behavior as reading from HEAD:.git-blame-ignore-revs to match hosting sites for now. Maybe consulting the working tree could be explored in the future in a separate series if desired, or please let me know if you or Phillip would prefer to change it in this series.

It needs to be justified in some way other than "match what the forges do (which is to use the one from HEAD)", but other than that, I think it would be more ergonomic to pick up the copy from the working tree (with local modifications) in non-bare case.

Thanks.

#30 lore raw

v5 cover blame: ignore revs in .git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

This series teaches git-blame(1) and git-annotate(1) to automatically use .git-blame-ignore-revs at the root of the working tree (or HEAD:.git-blame-ignore-revs in bare repositories) by default if it exists, 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/[email protected]/)
and addresses https://github.com/gitgitgadget/git/issues/1494.

Changes since v4:

* In non-bare repositories, Patch 2/2 now reads .git-blame-ignore-revs from the root of the working tree (using open_nofollow(), S_ISREG(), and IGNORE_REVS_MAX_FILE_SIZE) instead of HEAD:.git-blame-ignore-revs, while continuing to use HEAD:.git-blame-ignore-revs in bare repositories (matching the .mailmap model in read_mailmap()). This makes it ergonomic to test uncommitted or untracked changes to .git-blame-ignore-revs locally before committing.
* Updated the commit messages and documentation to justify the default lookup and working-tree behavior on ergonomic grounds.

Changes since v3:

* Patch 2/2 caps the size of the default HEAD:.git-blame-ignore-revs blob at IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) via odb_read_object_info() before calling odb_read_object(), skipping oversized blobs with a warning without loading them into memory.

Changes since v2:

* Patch 2/2 uses sb->repo instead of the_repository in parse_default_ignore_revs_blob().

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 .git-blame-ignore-revs by default

 Documentation/blame-options.adoc |   9 +-
 Documentation/config/blame.adoc  |  10 +-
 builtin/blame.c                  | 115 ++++++++++++-
 oidset.c                         |  81 ++++++---
 oidset.h                         |   9 +
 t/t8013-blame-ignore-revs.sh     | 271 +++++++++++++++++++++++++++++++
 6 files changed, 460 insertions(+), 35 deletions(-)

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

Range-diff vs v4:

1: 2e12486c0d ! 1: e9b2c1dae2 blame: harden ignore-revs parser and tag peeling
@@ Commit message
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
+ will teach git-blame(1) and git-annotate(1) to automatically read
+ .git-blame-ignore-revs 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:
+ Harden both code paths before enabling the default ignore-revs file:

- In oidset_parse_file_carefully(), strbuf_getline() reads up to the next newline and records the full line length in sb.len, including
2: 76d597dfa8 ! 2: 605eb049a1 blame: ignore revs in HEAD:.git-blame-ignore-revs

     @@ Metadata
      Author: Ravi Mistry <[email protected]>
      
       ## Commit message ##
     -    blame: ignore revs in HEAD:.git-blame-ignore-revs
     +    blame: ignore revs in .git-blame-ignore-revs by default
      
          git-blame(1) can ignore a list of commits specified via
          --ignore-revs-file or the blame.ignoreRevsFile configuration option.
     @@ Commit message
          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.
     +    Since then, maintaining a .git-blame-ignore-revs file at the top level
     +    of the repository has become the standard convention across the Git
     +    ecosystem, adopted by many open source projects (such as Chromium and
     +    LLVM) as well as hosting platforms (GitHub, GitLab, Gerrit). However,
     +    git-blame(1) and git-annotate(1) still ignore this file unless each
     +    user remembers to manually configure blame.ignoreRevsFile in every
     +    local clone.
      
     -    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.
     +    Teach git-blame(1) and git-annotate(1) to automatically add
     +    .git-blame-ignore-revs, if it exists, as the initial element in the
     +    list of ignore-revs files. Similar to how read_mailmap() handles
     +    .mailmap, consult .git-blame-ignore-revs at the root of the working
     +    tree when a working tree is available and fall back to the
     +    HEAD:.git-blame-ignore-revs blob in bare repositories. Reading from the
     +    working tree in non-bare repositories makes it ergonomic to prepare a
     +    new .git-blame-ignore-revs file or add revisions to an existing one and
     +    immediately test the effect with git-blame(1) before committing, while
     +    falling back to HEAD:.git-blame-ignore-revs allows bare repositories to
     +    respect the committed ignore list as well.
      
     -    To ensure consistent precedence and override semantics:
     -    - The default HEAD:.git-blame-ignore-revs entry is added before reading
     +    To ensure consistent precedence, override semantics, and safety when
     +    reading upstream-controlled content:
     +    - The default .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. Its size is checked
     -      via odb_read_object_info() and capped at IGNORE_REVS_MAX_FILE_SIZE
     -      (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore
     -      overly large gitattributes files", 2022-12-01)) so an oversized blob
     -      is skipped with a warning without being loaded into memory. 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().
     +    - When a working tree is present, the working-tree
     +      .git-blame-ignore-revs path is checked with lstat() and S_ISREG(),
     +      opened with open_nofollow(), and verified with fstat() and S_ISREG()
     +      so symbolic links and non-regular files are silently skipped. In bare
     +      repositories, HEAD:.git-blame-ignore-revs is resolved quietly via
     +      get_oid_with_context() and its tree entry mode is checked with
     +      S_ISREG() before checking the object.
     +    - In both modes, the file or blob size is capped at
     +      IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from
     +      3c50032ff5 ("attr: ignore overly large gitattributes files",
     +      2022-12-01)) before reading its contents, skipping an oversized file
     +      or blob with a warning without loading it into memory. The contents
     +      are parsed 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
     +      --no-ignore-revs-file cleanly discards the default entry without
            attempting to read or parse it, allowing users to bypass a malformed
     -      default blob.
     +      default ignore-revs file.
      
          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, overly large blobs, and comments and whitespace handling.
     +    add test coverage in t8013 for the default working-tree lookup,
     +    uncommitted and untracked .git-blame-ignore-revs files, subdirectory
     +    invocations, bare repositories, CLI and config overrides, symbolic
     +    links, overly large files and blobs, and comments and whitespace
     +    handling.
      
          Based-on-patch-by: Abhijeetsingh Meena <[email protected]>
          Helped-by: Kristoffer Haugsbakk <[email protected]>
     @@ Documentation/blame-options.adoc: take effect.
      -	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.
     ++	processed after the default `.git-blame-ignore-revs` file at the root of
     ++	the working tree (or `HEAD:.git-blame-ignore-revs` in a bare repository),
     ++	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 ignore-revs file.
       
       `--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 the `HEAD:.git-blame-ignore-revs` blob exists, it
     ++	`#` are ignored.  If `.git-blame-ignore-revs` exists at the root of the
     ++	working tree (or `HEAD:.git-blame-ignore-revs` in a bare repository), 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.
     @@ Documentation/config/blame.adoc: blame.showRoot::
       	Mark lines that were changed by an ignored revision that we could not
      
       ## builtin/blame.c ##
     +@@
     + #include "object-name.h"
     + #include "odb.h"
     + #include "pager.h"
     ++#include "path.h"
     + #include "blame.h"
     + #include "refs.h"
     + #include "setup.h"
      @@ builtin/blame.c: static int git_blame_config(const char *var, const char *value,
       		if (ret)
       			return ret;
     @@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
      +out:
      +	object_context_release(&oc);
      +}
     ++
     ++static void parse_default_ignore_revs_file(struct blame_scoreboard *sb,
     ++					   const char *name)
     ++{
     ++	char *path = repo_worktree_path(sb->repo, "%s", name);
     ++	struct stat st;
     ++	int fd = -1;
     ++	size_t size;
     ++	char *buf = NULL;
     ++
     ++	if (lstat(path, &st) || !S_ISREG(st.st_mode))
     ++		goto out;
     ++	fd = open_nofollow(path, O_RDONLY);
     ++	if (fd < 0 || fstat(fd, &st) || !S_ISREG(st.st_mode))
     ++		goto out;
     ++	if (st.st_size >= IGNORE_REVS_MAX_FILE_SIZE) {
     ++		warning(_("ignoring overly large ignore-revs file '%s'"), name);
     ++		goto out;
     ++	}
     ++	size = xsize_t(st.st_size);
     ++	buf = xmalloc(size);
     ++	if (read_in_full(fd, buf, size) == (ssize_t)size)
     ++		oidset_parse_buffer_carefully(&sb->ignore_list, buf, size,
     ++					      sb->repo->hash_algo,
     ++					      peel_to_commit_oid, sb);
     ++
     ++out:
     ++	if (fd >= 0)
     ++		close(fd);
     ++	free(buf);
     ++	free(path);
     ++}
     ++
     ++static void parse_default_ignore_revs(struct blame_scoreboard *sb)
     ++{
     ++	if (repo_get_work_tree(sb->repo))
     ++		parse_default_ignore_revs_file(sb, ".git-blame-ignore-revs");
     ++	else
     ++		parse_default_ignore_revs_blob(sb, "HEAD:.git-blame-ignore-revs");
     ++}
      +
       static void build_ignorelist(struct blame_scoreboard *sb,
       			     struct string_list *ignore_revs_file_list,
     @@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
      +	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);
     ++			parse_default_ignore_revs(sb);
       		else
       			oidset_parse_file_carefully(&sb->ignore_list, i->string,
       						    the_repository->hash_algo,
     @@ builtin/blame.c: int cmd_blame(int argc,
       
       	setup_default_color_by_age();
      +	string_list_append(&ignore_revs_file_list,
     -+			   "HEAD:.git-blame-ignore-revs")->util = &sb;
     ++			   ".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;
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
       	test_cmp expect actual
       '
       
     -+# Tests for default HEAD:.git-blame-ignore-revs blob
     -+test_expect_success 'setup default HEAD:.git-blame-ignore-revs' '
     ++# Tests for default .git-blame-ignore-revs
     ++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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	git tag DEF_B &&
      +
      +	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_A >expect &&
     ++	test_cmp expect actual &&
     ++
      +	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' '
     ++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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'default HEAD:.git-blame-ignore-revs respected by git annotate' '
     ++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 HEAD:.git-blame-ignore-revs works from subdirectory' '
     ++test_expect_success 'default .git-blame-ignore-revs works from subdirectory' '
     ++	test_when_finished "rm -rf sub" &&
      +	mkdir -p sub &&
     ++	echo "invalid-oid-value" >sub/.git-blame-ignore-revs &&
      +	(
      +		cd sub &&
      +		git blame --line-porcelain ../def-file >blame_raw &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	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 &&
     ++
     ++	git -C .git blame --line-porcelain HEAD -- def-file >blame_raw &&
     ++	sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 1/s/ .*//p" blame_raw >actual &&
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'uncommitted .git-blame-ignore-revs changes in working tree are ignored' '
     ++test_expect_success 'uncommitted .git-blame-ignore-revs changes in working tree are respected' '
      +	test_when_finished "git checkout -- .git-blame-ignore-revs" &&
     -+	echo "invalid-oid-value" >.git-blame-ignore-revs &&
     ++	: >.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
     ++	git rev-parse DEF_B >expect &&
     ++	test_cmp expect actual &&
     ++
     ++	rm .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 &&
     ++	test_cmp expect actual &&
     ++
     ++	echo "invalid-oid-value" >.git-blame-ignore-revs &&
     ++	test_must_fail git blame def-file
      +'
      +
     -+test_expect_success 'disable default HEAD:.git-blame-ignore-revs with --no-ignore-revs-file' '
     ++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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'disable default HEAD:.git-blame-ignore-revs with --ignore-revs-file ""' '
     ++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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'disable default HEAD:.git-blame-ignore-revs with blame.ignoreRevsFile=""' '
     ++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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'default HEAD:.git-blame-ignore-revs handles comments and whitespace' '
     ++test_expect_success 'default .git-blame-ignore-revs handles comments and whitespace' '
      +	rev_def_b=$(git rev-parse DEF_B) &&
      +	{
      +		echo "# Leading comment" &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'empty default HEAD:.git-blame-ignore-revs is harmless' '
     ++test_expect_success 'empty default .git-blame-ignore-revs is harmless' '
      +	: >.git-blame-ignore-revs &&
      +	git add .git-blame-ignore-revs &&
      +	test_tick &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'committed symlink .git-blame-ignore-revs in HEAD is ignored' '
     -+	git rm -f .git-blame-ignore-revs &&
     ++test_expect_success SYMLINKS 'symlink .git-blame-ignore-revs in working tree is ignored' '
     ++	test_when_finished "git checkout -- .git-blame-ignore-revs && rm -f target_file" &&
      +	git rev-parse DEF_B >target_file &&
     -+	test_ln_s_add target_file .git-blame-ignore-revs &&
     ++	rm -f .git-blame-ignore-revs &&
     ++	ln -s 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 'committed symlink .git-blame-ignore-revs in bare repo HEAD is ignored' '
     ++	test_when_finished "git reset --hard HEAD~ && rm -rf bare-symlink.git" &&
     ++	blob=$(printf "%s" "target_file" | git hash-object -w --stdin) &&
     ++	git update-index --replace --cacheinfo 120000,$blob,.git-blame-ignore-revs &&
      +	test_tick &&
      +	git commit -m "symlink .git-blame-ignore-revs" &&
     -+	git blame --line-porcelain def-file >blame_raw &&
     ++	git clone --bare . bare-symlink.git &&
     ++	git -C bare-symlink.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' '
     ++test_expect_success 'malformed default .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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_grep "invalid object name:" err
      +'
      +
     -+test_expect_success 'default HEAD:.git-blame-ignore-revs combined with config blame.ignoreRevsFile' '
     ++test_expect_success 'default .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 &&
     @@ t/t8013-blame-ignore-revs.sh: test_expect_success 'ignore-revs-file peels chaine
      +	test_cmp expect actual
      +'
      +
     -+test_expect_success 'blame works when HEAD:.git-blame-ignore-revs does not exist and ignores untracked file' '
     -+	test_when_finished "rm -f .git-blame-ignore-revs" &&
     ++test_expect_success 'blame works when .git-blame-ignore-revs does not exist' '
      +	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_expect_success EXPENSIVE 'overly large default HEAD:.git-blame-ignore-revs blob is ignored' '
     -+	blob=$(dd if=/dev/zero bs=1048576 count=101 2>/dev/null |
     -+		git hash-object -w --stdin) &&
     ++test_expect_success EXPENSIVE 'overly large default .git-blame-ignore-revs is ignored' '
     ++	test_when_finished "rm -f .git-blame-ignore-revs" &&
     ++	dd if=/dev/zero of=.git-blame-ignore-revs bs=1048576 count=101 2>/dev/null &&
     ++	git blame --line-porcelain def-file >blame_raw 2>err &&
     ++	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_grep "warning: ignoring overly large ignore-revs file '\''.git-blame-ignore-revs'\''" err &&
     ++
     ++	blob=$(git hash-object -w .git-blame-ignore-revs) &&
     ++	rm -f .git-blame-ignore-revs &&
      +	git update-index --add --cacheinfo 100644,$blob,.git-blame-ignore-revs &&
      +	test_tick &&
      +	git commit -m "overly large .git-blame-ignore-revs" &&
      +	test_when_finished "git reset --hard HEAD~" &&
     -+	git blame --line-porcelain def-file >blame_raw 2>err &&
     ++	git -C .git blame --line-porcelain HEAD -- def-file >blame_raw 2>err &&
      +	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_grep "warning: ignoring overly large ignore-revs blob '\''HEAD:.git-blame-ignore-revs'\''" err
      +'
Signature · 1 line
gitgitgadget
#31 lore raw

PATCH v5 1/2 blame: harden ignore-revs parser and tag peeling

From: Ravi Mistry <[email protected]>

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 .git-blame-ignore-revs 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 ignore-revs file:

- 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.

---
 builtin/blame.c              | 20 +++++++++++++++++--
 oidset.c                     |  3 +++
 t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)
builtin/blame.c +18 -2
index 48d5251c6d..6741a7b9df 100644
@@ -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);
 	}
 }
oidset.c +3 -0
index c8ff0b385c..90d39204d3 100644
@@ -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
t/t8013-blame-ignore-revs.sh +38 -0
index cace00ae8d..70fe509a64 100755
@@ -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 <[email protected]> 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
Signature · 1 line
gitgitgadget
#32 lore raw

PATCH v5 2/2 blame: ignore revs in .git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

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 at the top level of the repository has become the standard convention across the Git ecosystem, adopted by many open source projects (such as Chromium and LLVM) as well as hosting platforms (GitHub, GitLab, Gerrit). However, git-blame(1) and git-annotate(1) still ignore this file unless each user remembers to manually configure blame.ignoreRevsFile in every local clone.

Teach git-blame(1) and git-annotate(1) to automatically add .git-blame-ignore-revs, if it exists, as the initial element in the list of ignore-revs files. Similar to how read_mailmap() handles .mailmap, consult .git-blame-ignore-revs at the root of the working tree when a working tree is available and fall back to the HEAD:.git-blame-ignore-revs blob in bare repositories. Reading from the working tree in non-bare repositories makes it ergonomic to prepare a new .git-blame-ignore-revs file or add revisions to an existing one and immediately test the effect with git-blame(1) before committing, while falling back to HEAD:.git-blame-ignore-revs allows bare repositories to respect the committed ignore list as well.

To ensure consistent precedence, override semantics, and safety when reading upstream-controlled content:
- The default .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.
- When a working tree is present, the working-tree .git-blame-ignore-revs path is checked with lstat() and S_ISREG(), opened with open_nofollow(), and verified with fstat() and S_ISREG() so symbolic links and non-regular files are silently skipped. In bare repositories, HEAD:.git-blame-ignore-revs is resolved quietly via get_oid_with_context() and its tree entry mode is checked with S_ISREG() before checking the object.
- In both modes, the file or blob size is capped at IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) before reading its contents, skipping an oversized file or blob with a warning without loading it into memory. The contents are parsed 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 entry without attempting to read or parse it, allowing users to bypass a malformed default ignore-revs file.

Update documentation in blame-options.adoc and config/blame.adoc, and add test coverage in t8013 for the default working-tree lookup, uncommitted and untracked .git-blame-ignore-revs files, subdirectory invocations, bare repositories, CLI and config overrides, symbolic links, overly large files and blobs, and comments and whitespace handling.

---
 Documentation/blame-options.adoc |   9 +-
 Documentation/config/blame.adoc  |  10 +-
 builtin/blame.c                  |  95 ++++++++++++-
 oidset.c                         |  84 +++++++----
 oidset.h                         |   9 ++
 t/t8013-blame-ignore-revs.sh     | 233 +++++++++++++++++++++++++++++++
 6 files changed, 404 insertions(+), 36 deletions(-)
Documentation/blame-options.adoc +6 -3
index 1ae1222b6b..f985788112 100644
@@ -132,9 +132,12 @@ 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 `.git-blame-ignore-revs` file at the root of
+	the working tree (or `HEAD:.git-blame-ignore-revs` in a bare repository),
+	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 ignore-revs file.
 
 `--color-lines`::
 	Color line annotations in the default format differently if they come from
Documentation/config/blame.adoc +7 -3
index 4d047c1790..a46aea8d98 100644
@@ -23,9 +23,13 @@ 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 (or `HEAD:.git-blame-ignore-revs` in a bare repository), 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 +91 -4

127 lines not on this page · raw message

oidset.c +58 -26

102 lines not on this page · raw message

oidset.h +9 -0
index e0f1a6ff4f..667c390842 100644
@@ -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;
t/t8013-blame-ignore-revs.sh +233 -0

237 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#21 lore raw

Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs

In reply to Junio C Hamano
Quoted 10 lines · Junio C Hamano
Quoted 4 lines · Ravi Mistry

For v4, I left the behavior as reading from HEAD:.git-blame-ignore-revs to match hosting sites for now. Maybe consulting the working tree could be explored in the future in a separate series if desired, or please let me know if you or Phillip would prefer to change it in this series.

It needs to be justified in some way other than "match what the forges do (which is to use the one from HEAD)", but other than that, I think it would be more ergonomic to pick up the copy from the working tree (with local modifications) in non-bare case.

Makes sense, I did not have a justification for sticking with HEAD other than matching hosting sites.

I have sent v5, which reads .git-blame-ignore-revs from the root of the working tree in non-bare repositories (and HEAD:.git-blame-ignore-revs in bare repositories) and updates the commit message to justify the behavior on ergonomic grounds.

Thanks!

#33 lore raw

Re: [PATCH v5 0/2] blame: ignore revs in .git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget
Quoted 7 lines · Ravi Mistry via GitGitGadget
     +    tree when a working tree is available and fall back to the
     +    HEAD:.git-blame-ignore-revs blob in bare repositories. Reading from the
     +    working tree in non-bare repositories makes it ergonomic to prepare a
     +    new .git-blame-ignore-revs file or add revisions to an existing one and
     +    immediately test the effect with git-blame(1) before committing, while
     +    falling back to HEAD:.git-blame-ignore-revs allows bare repositories to
     +    respect the committed ignore list as well.

Very well written justification.

Quoted 3 lines

++ processed after the default .git-blame-ignore-revs file at the root of
++ the working tree (or HEAD:.git-blame-ignore-revs in a bare repository),
++ if it exists, and any files specified with the blame.ignoreRevsFile

Good. A bit more comment on this later.

Quoted 1 line

++ config option. An empty file name, "", or --no-ignore-revs-file will

That's "a configuration variable".

Quoted 7 lines
     ++static void parse_default_ignore_revs(struct blame_scoreboard *sb)
     ++{
     ++	if (repo_get_work_tree(sb->repo))
     ++		parse_default_ignore_revs_file(sb, ".git-blame-ignore-revs");
     ++	else
     ++		parse_default_ignore_revs_blob(sb, "HEAD:.git-blame-ignore-revs");
     ++}

Here, HEAD:.git-blame-ignore-revs is used only in bare repositories so that those working in a working tree of the project and on a revision with a committed ignore-revs file can remove it from the working tree, verify the behaviour of "git blame" to test before committing the removal, if they no longer want to use the committed ignore-revs list. Which is great. We do not want to say "we have a working tree, we do not have the file there, fall back to HEAD".

Quoted 6 lines
     @@ builtin/blame.c: int cmd_blame(int argc,
       
       	setup_default_color_by_age();
      +	string_list_append(&ignore_revs_file_list,
     -+			   "HEAD:.git-blame-ignore-revs")->util = &sb;
     ++			   ".git-blame-ignore-revs")->util = &sb;

Hmph, build_ignorelist() ignores the string and only pays attention to the fact that the string_list_item having a non-null .util member, so this diff, and the fact that a specific string is on the list, is a bit misleading in two ways (i.e., the string itself does not matter, and the value of .util member does not metter, either).

In other words, a cute hack like this

		string_list_append(&ignore_revs_file_list,
                		   "<builtin default>")->util =
                                   "see build_ignorelist()";

may still give us the same behaviour, which is *horrible*.

It probably deserves a comment to help future readers, e.g.

		/*
		 * An entry with non-NULL .util member adds the
		 *  .git-blame-ignore-revs at the project root;
                 * see build_ignorelist()
		 */
                string_list_append(&ignore_revs_file_list,
                                   ".git-blame-ignore-revs")->util = &sb;

or something? It is especially misleading to pass &sb here because it is not used as a scoreboard at all anywhere.

Thanks.

#37 lore raw

v6 cover blame: ignore revs in .git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

This series teaches git-blame(1) and git-annotate(1) to automatically use .git-blame-ignore-revs at the root of the working tree (or HEAD:.git-blame-ignore-revs in bare repositories) by default if it exists, 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/[email protected]/)
and addresses https://github.com/gitgitgadget/git/issues/1494.

Changes since v5:

* Passed i->string from build_ignorelist() into parse_default_ignore_revs() so the item string is used for both the working-tree path and "HEAD:" in bare repositories, and changed cmd_blame() to set ->util = (void *)1 with an explanatory comment instead of passing the scoreboard.
* Doc update: Changed "config option" / "configuration option" to "configuration variable" in Documentation/blame-options.adoc and the commit message.

Changes since v4:

* In non-bare repositories, Patch 2/2 now reads .git-blame-ignore-revs from the root of the working tree (using open_nofollow(), S_ISREG(), and IGNORE_REVS_MAX_FILE_SIZE) instead of HEAD:.git-blame-ignore-revs, while continuing to use HEAD:.git-blame-ignore-revs in bare repositories (matching the .mailmap model in read_mailmap()). This makes it ergonomic to test uncommitted or untracked changes to .git-blame-ignore-revs locally before committing.
* Updated the commit messages and documentation to justify the default lookup and working-tree behavior on ergonomic grounds.

Changes since v3:

* Patch 2/2 caps the size of the default HEAD:.git-blame-ignore-revs blob at IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) via odb_read_object_info() before calling odb_read_object(), skipping oversized blobs with a warning without loading them into memory.

Changes since v2:

* Patch 2/2 uses sb->repo instead of the_repository in parse_default_ignore_revs_blob().

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 .git-blame-ignore-revs by default

 Documentation/blame-options.adoc |   9 +-
 Documentation/config/blame.adoc  |  10 +-
 builtin/blame.c                  | 122 +++++++++++++-
 oidset.c                         |  81 ++++++---
 oidset.h                         |   9 +
 t/t8013-blame-ignore-revs.sh     | 271 +++++++++++++++++++++++++++++++
 6 files changed, 467 insertions(+), 35 deletions(-)

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

Range-diff vs v5:

 1:  e9b2c1dae2 = 1:  e9b2c1dae2 blame: harden ignore-revs parser and tag peeling
 2:  605eb049a1 ! 2:  6039d4bc2d blame: ignore revs in .git-blame-ignore-revs by default

@@ Commit message
blame: ignore revs in .git-blame-ignore-revs by default

git-blame(1) can ignore a list of commits specified via
- --ignore-revs-file or the blame.ignoreRevsFile configuration option.
+ --ignore-revs-file or the blame.ignoreRevsFile configuration variable.
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.
@@ Commit message
.git-blame-ignore-revs, if it exists, as the initial element in the list of ignore-revs files. Similar to how read_mailmap() handles .mailmap, consult .git-blame-ignore-revs at the root of the working
- tree when a working tree is available and fall back to the
- HEAD:.git-blame-ignore-revs blob in bare repositories. Reading from the
- working tree in non-bare repositories makes it ergonomic to prepare a
- new .git-blame-ignore-revs file or add revisions to an existing one and
- immediately test the effect with git-blame(1) before committing, while
- falling back to HEAD:.git-blame-ignore-revs allows bare repositories to
+ tree when a working tree is available, and otherwise consult the
+ HEAD:.git-blame-ignore-revs blob in bare repositories. Reading only
+ from the working tree in non-bare repositories makes it ergonomic to
+ prepare a new .git-blame-ignore-revs file, edit an existing one, or
+ remove a committed one and immediately test the effect with
+ git-blame(1) before committing, while consulting
+ HEAD:.git-blame-ignore-revs in bare repositories allows them to
respect the committed ignore list as well.

To ensure consistent precedence, override semantics, and safety when
@@ Documentation/blame-options.adoc: take effect.
+ processed after the default .git-blame-ignore-revs file at the root of
+ the working tree (or HEAD:.git-blame-ignore-revs in a bare repository),
+ 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 ignore-revs file.
++ configuration variable. An empty file name, "", or
++ --no-ignore-revs-file will clear the list of revs from previously
++ processed files, including the default ignore-revs file.

--color-lines::
Color line annotations in the default format differently if they come from
@@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
+#define IGNORE_REVS_MAX_FILE_SIZE (100 * 1024 * 1024)
+
+static void parse_default_ignore_revs_blob(struct blame_scoreboard *sb,
-+ const char *name)
++ const char *path)
+{
++ char *name = xstrfmt("HEAD:%s", path);
+ struct object_context oc;
+ struct object_id oid;
+ enum object_type type;
@@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
+
+out:
+ object_context_release(&oc);
++ free(name);
+}
+
+static void parse_default_ignore_revs_file(struct blame_scoreboard *sb,
@@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
+ free(path);
+}
+
-+static void parse_default_ignore_revs(struct blame_scoreboard *sb) ++static void parse_default_ignore_revs(struct blame_scoreboard *sb,
++ const char *name)
+{
+ if (repo_get_work_tree(sb->repo))
-+ parse_default_ignore_revs_file(sb, ".git-blame-ignore-revs");
++ parse_default_ignore_revs_file(sb, name);
+ else
-+ parse_default_ignore_revs_blob(sb, "HEAD:.git-blame-ignore-revs");
++ parse_default_ignore_revs_blob(sb, name);
+}
+
static void build_ignorelist(struct blame_scoreboard *sb,
@@ builtin/blame.c: static int peel_to_commit_oid(struct object_id *oid_ret, void *
+ 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(sb);
++ parse_default_ignore_revs(sb, i->string);
else
oidset_parse_file_carefully(&sb->ignore_list, i->string,
the_repository->hash_algo,
@@ 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();
++ /*
++ * An entry with a non-NULL .util member names the default
++ * ignore-revs file at the project root; see build_ignorelist().
++ */
+ string_list_append(&ignore_revs_file_list,
-+ ".git-blame-ignore-revs")->util = &sb;
++ ".git-blame-ignore-revs")->util = (void *)1;
repo_config(the_repository, git_blame_config, &output_option); repo_init_revisions(the_repository, &revs, NULL); revs.date_mode = blame_date_mode;

Signature · 1 line
gitgitgadget
#38 lore raw

PATCH v6 1/2 blame: harden ignore-revs parser and tag peeling

From: Ravi Mistry <[email protected]>

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 .git-blame-ignore-revs 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 ignore-revs file:

- 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.

---
 builtin/blame.c              | 20 +++++++++++++++++--
 oidset.c                     |  3 +++
 t/t8013-blame-ignore-revs.sh | 38 ++++++++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 2 deletions(-)
builtin/blame.c +18 -2
index 48d5251c6d..6741a7b9df 100644
@@ -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);
 	}
 }
oidset.c +3 -0
index c8ff0b385c..90d39204d3 100644
@@ -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
t/t8013-blame-ignore-revs.sh +38 -0
index cace00ae8d..70fe509a64 100755
@@ -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 <[email protected]> 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
Signature · 1 line
gitgitgadget
#39 lore raw

PATCH v6 2/2 blame: ignore revs in .git-blame-ignore-revs by default

In reply to Ravi Mistry via GitGitGadget

From: Ravi Mistry <[email protected]>

git-blame(1) can ignore a list of commits specified via --ignore-revs-file or the blame.ignoreRevsFile configuration variable. 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 at the top level of the repository has become the standard convention across the Git ecosystem, adopted by many open source projects (such as Chromium and LLVM) as well as hosting platforms (GitHub, GitLab, Gerrit). However, git-blame(1) and git-annotate(1) still ignore this file unless each user remembers to manually configure blame.ignoreRevsFile in every local clone.

Teach git-blame(1) and git-annotate(1) to automatically add .git-blame-ignore-revs, if it exists, as the initial element in the list of ignore-revs files. Similar to how read_mailmap() handles .mailmap, consult .git-blame-ignore-revs at the root of the working tree when a working tree is available, and otherwise consult the HEAD:.git-blame-ignore-revs blob in bare repositories. Reading only from the working tree in non-bare repositories makes it ergonomic to prepare a new .git-blame-ignore-revs file, edit an existing one, or remove a committed one and immediately test the effect with git-blame(1) before committing, while consulting HEAD:.git-blame-ignore-revs in bare repositories allows them to respect the committed ignore list as well.

To ensure consistent precedence, override semantics, and safety when reading upstream-controlled content:
- The default .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.
- When a working tree is present, the working-tree .git-blame-ignore-revs path is checked with lstat() and S_ISREG(), opened with open_nofollow(), and verified with fstat() and S_ISREG() so symbolic links and non-regular files are silently skipped. In bare repositories, HEAD:.git-blame-ignore-revs is resolved quietly via get_oid_with_context() and its tree entry mode is checked with S_ISREG() before checking the object.
- In both modes, the file or blob size is capped at IGNORE_REVS_MAX_FILE_SIZE (100MB, matching ATTR_MAX_FILE_SIZE from 3c50032ff5 ("attr: ignore overly large gitattributes files", 2022-12-01)) before reading its contents, skipping an oversized file or blob with a warning without loading it into memory. The contents are parsed 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 entry without attempting to read or parse it, allowing users to bypass a malformed default ignore-revs file.

Update documentation in blame-options.adoc and config/blame.adoc, and add test coverage in t8013 for the default working-tree lookup, uncommitted and untracked .git-blame-ignore-revs files, subdirectory invocations, bare repositories, CLI and config overrides, symbolic links, overly large files and blobs, and comments and whitespace handling.

---
 Documentation/blame-options.adoc |   9 +-
 Documentation/config/blame.adoc  |  10 +-
 builtin/blame.c                  | 102 +++++++++++++-
 oidset.c                         |  84 +++++++----
 oidset.h                         |   9 ++
 t/t8013-blame-ignore-revs.sh     | 233 +++++++++++++++++++++++++++++++
 6 files changed, 411 insertions(+), 36 deletions(-)
Documentation/blame-options.adoc +6 -3
index 1ae1222b6b..7e910373bd 100644
@@ -132,9 +132,12 @@ 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 `.git-blame-ignore-revs` file at the root of
+	the working tree (or `HEAD:.git-blame-ignore-revs` in a bare repository),
+	if it exists, and any files specified with the `blame.ignoreRevsFile`
+	configuration variable.  An empty file name, `""`, or
+	`--no-ignore-revs-file` will clear the list of revs from previously
+	processed files, including the default ignore-revs file.
 
 `--color-lines`::
 	Color line annotations in the default format differently if they come from
Documentation/config/blame.adoc +7 -3
index 4d047c1790..a46aea8d98 100644
@@ -23,9 +23,13 @@ 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 (or `HEAD:.git-blame-ignore-revs` in a bare repository), 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 +98 -4

134 lines not on this page · raw message

oidset.c +58 -26

102 lines not on this page · raw message

oidset.h +9 -0
index e0f1a6ff4f..667c390842 100644
@@ -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;
t/t8013-blame-ignore-revs.sh +233 -0

237 lines not on this page · raw message

Signature · 1 line
gitgitgadget
#34 lore raw

Re: [PATCH v5 0/2] blame: ignore revs in .git-blame-ignore-revs by default

In reply to Junio C Hamano
Quoted 3 lines · Junio C Hamano
Quoted 1 line

++ config option. An empty file name, "", or --no-ignore-revs-file will

That's "a configuration variable".

Fixed in Documentation/blame-options.adoc and in the commit log message.

Quoted 5 lines

Hmph, build_ignorelist() ignores the string and only pays attention to the fact that the string_list_item having a non-null .util member, so this diff, and the fact that a specific string is on the list, is a bit misleading in two ways (i.e., the string itself does not matter, and the value of .util member does not metter, either).

...

Quoted 10 lines

It probably deserves a comment to help future readers, e.g.

		/*
		 * An entry with non-NULL .util member adds the
		 *  .git-blame-ignore-revs at the project root;
                 * see build_ignorelist()
		 */
                string_list_append(&ignore_revs_file_list,
                                   ".git-blame-ignore-revs")->util = &sb;
Quoted 2 lines

or something? It is especially misleading to pass &sb here because it is not used as a scoreboard at all anywhere.

In v6, build_ignorelist() passes i->string to parse_default_ignore_revs() so the string is actually used (both for the working-tree path and for "HEAD:<path>" in bare repositories), and cmd_blame() sets ->util = (void *)1 with an explanatory comment (rather than passing the scoreboard).

Thanks!

#35 lore raw

Re: [PATCH v5 0/2] blame: ignore revs in .git-blame-ignore-revs by default

Quoted 5 lines · Ravi Mistry

In v6, build_ignorelist() passes i->string to parse_default_ignore_revs() so the string is actually used (both for the working-tree path and for "HEAD:<path>" in bare repositories), and cmd_blame() sets ->util = (void *)1 with an explanatory comment (rather than passing the scoreboard).

Very cleanly done. I very much like this new design that still honors what the item->string says, instead of "ah, .util is not NULL, so let's do the hardcoded default thing".

Unless I hear objections in a few days, let's mark the topic ready for 'next'.

Will replace. Thanks.

#36 lore raw

Re: [PATCH v5 0/2] blame: ignore revs in .git-blame-ignore-revs by default

Quoted 8 lines · Junio C Hamano
Quoted 5 lines · Ravi Mistry

In v6, build_ignorelist() passes i->string to parse_default_ignore_revs() so the string is actually used (both for the working-tree path and for "HEAD:<path>" in bare repositories), and cmd_blame() sets ->util = (void *)1 with an explanatory comment (rather than passing the scoreboard).

Quoted 6 lines

Very cleanly done. I very much like this new design that still honors what the item->string says, instead of "ah, .util is not NULL, so let's do the hardcoded default thing".

Unless I hear objections in a few days, let's mark the topic ready for 'next'.

I'm not sure we need the lstat() call before open_nofollow() in the second patch of V6 but everything else looked good - I'd be happy to see that version merged.

Thanks

Phillip