Repository navigation
Conversation
Welcome to GitGitGadgetHi @rmistry, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
Member
|
/allow |
|
User rmistry is now allowed to use GitGitGadget. |
Author
|
Thank you @dscho for allowing this pull request. /submit |
Author
|
/submit |
|
Submitted as pull.2224.git.1789169384240.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Thanks for picking it up @rmistry. I started working on it but got carried away in another direction, looking into detecting and considering .git-blame-ignore-revs from each project subdirectory, how they take precedence over each other, and how to override them (see #1809). This implementation looks great! It seems to auto-detect the ignore revs file at the root only. I just glanced through the changes, though, and haven’t tested it yet. |
Author
|
Thank you @Ethan0456 for the kind words and for kicking off the initial work! Keeping the scope focused strictly on the repository root helps keep things clean and aligned with how hosting platforms (GitHub, GitLab) and major repositories (Chromium, LLVM, Android) use the blame file. |
|
Ravi Mistry wrote on the Git mailing list (how to reply to this email): 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! |
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): "Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
> blame.ignoreRevsFile::
> Ignore revisions listed in the file, one unabbreviated object name per
> line, in linkgit:git-blame[1]. Whitespace and comments beginning with
> - `#` are ignored. This option may be repeated multiple times. Empty
> - file names will reset the list of ignored revisions. This option will
> - be handled before the command line option `--ignore-revs-file`.
> + `#` are ignored. If `.git-blame-ignore-revs` exists at the root of the
> + working tree in a non-bare repository, it is used by default. This option
> + may be repeated multiple times; files specified here are processed after
> + the default file. An empty file name will reset the list of ignored
> + revisions from previously processed files and disable the default file.
> + This option is handled before the command-line option `--ignore-revs-file`.
The proposed log message explains that '.git-blame-ignore-revs' is
used as the default for blame.ignoreRevsFile even when the user does
not ask to do so in order to match what hosting sites do, as it
would be confusing if the local repository behaved differently.
While wanting consistency is reasonable, the description above does
not quite match that goal. If an untracked '.git-blame-ignore-revs'
file exists at the root of the working tree, or if a tracked one has
local changes relative to HEAD, the local repository behaves
differently from hosting sites that operate on the
'HEAD:.git-blame-ignore-revs' blob. It may make more sense to say:
"If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
the initial element in the list of ignore-revs files. Other files
listed in the configuration are also used, but an empty element
makes all elements that appeared before in the list forgotten."
This rule should apply whether the repository is bare or not.
The proposed log message also talks about taking only a regular file
and ignoring everything else for "security" [*], but there is
another important thing we need to worry about security-wise.
Somebody has to audit the parser for these files (one unabbreviated
object name per line, ignoring whitespace and lines starting with
'#') and ensure that the implementation is truly secure.
This is a new threat vector introduced by this change. Without this
patch, blame.ignoreRevsFile comes only from the configuration, which
cannot point to an attacker-controlled file under our threat model.
Now, however, the parser must read upstream-controlled content at a
known path, and it must be prepared to cope with attempts to use it
as an attack vector.
[Footnote]
* By the way, "we do not read anything from the working tree.
'HEAD:.git-blame-ignore-revs' is the only thing that is added
to the picture" would make it unnecessary to lstat() and ignore
non-regular files. |
|
Ravi Mistry wrote on the Git mailing list (how to reply to this email): "Junio C Hamano" <gitster@pobox.com> writes:
> While wanting consistency is reasonable, the description above does
> not quite match that goal. If an untracked '.git-blame-ignore-revs'
> file exists at the root of the working tree, or if a tracked one has
> local changes relative to HEAD, the local repository behaves
> differently from hosting sites that operate on the
> 'HEAD:.git-blame-ignore-revs' blob. It may make more sense to say:
> "If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
> the initial element in the list of ignore-revs files. Other files
> listed in the configuration are also used, but an empty element
> makes all elements that appeared before in the list forgotten."
> This rule should apply whether the repository is bare or not.
Thank you very much for the detailed feedback, Junio! Reading the
committed blob from HEAD instead of the working tree totally makes
sense.
> Somebody has to audit the parser for these files (one unabbreviated
> object name per line, ignoring whitespace and lines starting with
> '#') and ensure that the implementation is truly secure.
I looked through the parser in oidset.c (which we can share for
both the HEAD blob and configured files) and peel_to_commit_oid in
builtin/blame.c. Mostly looks good, IMHO, but there may be two edge
cases we can tighten up:
1. Rejecting lines with embedded NUL bytes via memchr in oidset.c
(where strchr and the check after parse_oid_hex_algop currently
stop at the first NUL byte and ignore trailing bytes on the
line).
2. Passing OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK in
peel_to_commit_oid and peeling tags step by step so missing OIDs
or tag targets do not trigger lazy promisor fetches in partial
clones.
Does this plan sound good to you for v2?
Thanks,
Ravi |
Currently, blame.ignoreRevsFile and --ignore-revs-file only read paths explicitly configured by the user, which cannot point to an attacker-controlled file under Git's threat model. An upcoming commit will teach git-blame(1) and git-annotate(1) to automatically read the HEAD:.git-blame-ignore-revs blob by default if it exists, exposing the ignore-revs parser (oidset_parse_file_carefully() in oidset.c) and its tag-peeling callback (peel_to_commit_oid() in builtin/blame.c) to upstream-controlled content at a well-known path. Harden both code paths before enabling the default blob: - In oidset_parse_file_carefully(), strbuf_getline() reads up to the next newline and records the full line length in sb.len, including any embedded NUL bytes. However, strchr(sb.buf, '#') and parse_oid_hex_algop(sb.buf, &oid, &p, algop) treat sb.buf as a NUL-terminated string. If a line contains an embedded NUL byte after a valid object name (such as "<oid>\0garbage" or "<oid>\0# comment"), *p is '\0' and trailing bytes on the line are silently ignored. Reject any line containing an embedded NUL byte via memchr() before stripping comments and whitespace. - In peel_to_commit_oid(), odb_read_object_info() is called without OBJECT_INFO_SKIP_FETCH_OBJECT or OBJECT_INFO_QUICK, and deref_tag() calls parse_object() on tag targets without checking whether the target object exists locally first. In a partial clone, any missing commit OID or tag target listed in the ignore-revs file would trigger lazy promisor fetches and pack directory rescans during git-blame(1). Use odb_read_object_info_extended() with OBJECT_INFO_LOOKUP_REPLACE | OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK and peel OBJ_TAG objects one layer per iteration, verifying that each target object exists locally and matches the tag's declared type before parsing it. Signed-off-by: Ravi Mistry <rmistry@google.com>
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 ae3f36d ("blame: add blame.ignoreRevsFile config option", 2019-10-18), it intentionally avoided adopting a default ignore file. At the time, the capability was new and unproven, so avoiding unrequested filesystem I/O or unexpected attribution shifts took priority over a project-wide default. Requiring explicit opt-in per clone was therefore the prudent design. Since then, maintaining a .git-blame-ignore-revs file in the repository root has become the de facto standard across the Git ecosystem, adopted by major hosting platforms (GitHub, GitLab, Gerrit) and prominent open source projects (such as Chromium and LLVM). As a consequence, developers frequently encounter a jarring mismatch: web interfaces seamlessly ignore formatting commits, but local git-blame(1) and git-annotate(1) runs do not, unless each user manually configures blame.ignoreRevsFile for every local checkout. Teach git-blame(1) and git-annotate(1) to automatically add the HEAD:.git-blame-ignore-revs blob, if it exists, as the initial element in the list of ignore-revs files in both bare and non-bare repositories. Reading the committed blob from HEAD rather than the working tree ensures that local runs match hosting platforms even when an untracked .git-blame-ignore-revs file is present or a tracked one has uncommitted local changes. To ensure consistent precedence and override semantics: - The default HEAD:.git-blame-ignore-revs entry is added before reading configuration and CLI options, preserving user and repository config overrides. - In git_blame_config(), blame.ignoreRevsFile entries are appended via string_list_append() rather than inserted in sorted order via string_list_insert() so that configuration entries preserve their order relative to the initial default entry. - The HEAD:.git-blame-ignore-revs tree entry is resolved quietly via get_oid_with_context(). Its mode is checked with S_ISREG() before reading the object so that non-regular tree entries (such as a committed symbolic link whose blob stores a target path rather than revision IDs, a subdirectory, or a gitlink) are skipped instead of being read and rejected as malformed object names. The blob is parsed in memory via a new oidset_parse_buffer_carefully() helper in oidset.c that shares line parsing with oidset_parse_file_carefully(). - In build_ignorelist(), ignore-revs entries are processed starting after the last empty string entry. This ensures setting blame.ignoreRevsFile to "" or passing --ignore-revs-file "" or --no-ignore-revs-file cleanly discards the default blob without attempting to read or parse it, allowing users to bypass a malformed default blob. Update documentation in blame-options.adoc and config/blame.adoc, and add comprehensive test coverage in t8013 for the default blob lookup, subdirectory invocations, bare repositories, uncommitted and untracked working-tree files, CLI and config overrides, committed symlink entries, and comments and whitespace handling. Based-on-patch-by: Abhijeetsingh Meena <abhijeet040403@gmail.com> Helped-by: Kristoffer Haugsbakk <code@khaugsbakk.name> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Ravi Mistry <rmistry@google.com>
rmistry
force-pushed
the
blame-default-ignore-revs
branch
from
October 7, 2026 21:16
22a100d to
35e303d
Compare
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Ravi Mistry <rmistry@google.com> writes:
> "Junio C Hamano" <gitster@pobox.com> writes:
>
>> While wanting consistency is reasonable, the description above does
>> not quite match that goal. If an untracked '.git-blame-ignore-revs'
>> file exists at the root of the working tree, or if a tracked one has
>> local changes relative to HEAD, the local repository behaves
>> differently from hosting sites that operate on the
>> 'HEAD:.git-blame-ignore-revs' blob. It may make more sense to say:
>> "If the 'HEAD:.git-blame-ignore-revs' blob exists, it is added as
>> the initial element in the list of ignore-revs files. Other files
>> listed in the configuration are also used, but an empty element
>> makes all elements that appeared before in the list forgotten."
>> This rule should apply whether the repository is bare or not.
>
> Thank you very much for the detailed feedback, Junio! Reading the
> committed blob from HEAD instead of the working tree totally makes
> sense.
>
>> Somebody has to audit the parser for these files (one unabbreviated
>> object name per line, ignoring whitespace and lines starting with
>> '#') and ensure that the implementation is truly secure.
>
> I looked through the parser in oidset.c (which we can share for
> both the HEAD blob and configured files) and peel_to_commit_oid in
> builtin/blame.c. Mostly looks good, IMHO, but there may be two edge
> cases we can tighten up:
>
> 1. Rejecting lines with embedded NUL bytes via memchr in oidset.c
> (where strchr and the check after parse_oid_hex_algop currently
> stop at the first NUL byte and ignore trailing bytes on the
> line).
>
> 2. Passing OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK in
> peel_to_commit_oid and peeling tags step by step so missing OIDs
> or tag targets do not trigger lazy promisor fetches in partial
> clones.
>
> Does this plan sound good to you for v2?
Are you presenting a different plan, or just adding details to what
you quoted from my message above?
I delegated because I did not want to spend time on the auditing
part, so if you are asking me that these two are the only things we
need to address, that defeats the point of me delegating it to
"somebody else" X-<. Hopefully a v2 with some tightening the OID
parsing may entice folks (who are hopefully interested in security
related work) to chime in and they would help us decide if it is
good enough to cover these two points and nothing else.
Thanks. |
|
Ravi Mistry wrote on the Git mailing list (how to reply to this email): "Junio C Hamano" <gitster@pobox.com> writes:
> Are you presenting a different plan, or just adding details to what
> you quoted from my message above?
>
> I delegated because I did not want to spend time on the auditing
> part, so if you are asking me that these two are the only things we
> need to address, that defeats the point of me delegating it to
> "somebody else" X-<. Hopefully a v2 with some tightening the OID
> parsing may entice folks (who are hopefully interested in security
> related work) to chime in and they would help us decide if it is
> good enough to cover these two points and nothing else.
Ah, sorry for my confusion, I was adding details to your plan
above (I thought the audit was being delegated to me so wanted to
make sure the approach was correct). Will put up a v2 for the
security reviewers to chime in.
Thanks again!
Ravi |
rmistry
changed the title
Author
|
/preview |
|
Preview email sent as pull.2224.v2.git.1791491999.gitgitgadget@gmail.com |
Author
|
/submit |
|
Submitted as pull.2224.v2.git.1791493644.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
![gitgitgadget[bot]](https://avatars.githubusercontent.com/in/12836?s=60&v=4){kind=small}
| @@ -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) | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
> - In oidset_parse_file_carefully(), strbuf_getline() reads up to the
> next newline and records the full line length in sb.len, including
> any embedded NUL bytes. However, strchr(sb.buf, '#') and
> parse_oid_hex_algop(sb.buf, &oid, &p, algop) treat sb.buf as a
> NUL-terminated string. If a line contains an embedded NUL byte after
> a valid object name (such as "<oid>\0garbage" or "<oid>\0# comment"),
> *p is '\0' and trailing bytes on the line are silently ignored.
> Reject any line containing an embedded NUL byte via memchr() before
> stripping comments and whitespace.
Maybe I am slow, but I do not immediately see why ignoring
everything after the first NUL is a problem. A call to
strbuf_trim() is ineffective at trimming whitespace that appears
immediately before such a NUL. For example, while
cf9bdb1...f6092d # comment LF
would feed the leading 'cf9bdb1...f6092d' part (after stripping
whitespace before '#') to parse_oid_hex_algop(), this
cf9bdb1...f6092d NUL comment LF
would keep the whitespace after '92d' and cause the parsing to
fail. I do not see any security implications here.
On the other hand ...
> - In peel_to_commit_oid(), odb_read_object_info() is called without
> OBJECT_INFO_SKIP_FETCH_OBJECT or OBJECT_INFO_QUICK, and deref_tag()
> calls parse_object() on tag targets without checking whether the
> target object exists locally first. In a partial clone, any missing
> commit OID or tag target listed in the ignore-revs file would trigger
> lazy promisor fetches and pack directory rescans during git-blame(1).
> Use odb_read_object_info_extended() with OBJECT_INFO_LOOKUP_REPLACE |
> OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK and peel OBJ_TAG
> objects one layer per iteration, verifying that each target object
> exists locally and matches the tag's declared type before parsing it.
... this may be a very reasonable thing to do, I would think. In a
shallow clone, if we are not auto-deepening the shallow boundary
during a "git blame" session, we have no reason to lazy fetch
entries in the ignore file that are older than the shallow boundary.
> diff --git a/oidset.c b/oidset.c
> index c8ff0b385c..90d39204d3 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -85,6 +85,9 @@ void oidset_parse_file_carefully(struct oidset *set, const char *path,
> const char *p;
> const char *name;
>
> + if (memchr(sb.buf, '\0', sb.len))
> + die("invalid object name: %s", sb.buf);
A file with such an entry is rejected and the entire operation is
aborted as suspected attack attempt, which feels like striking the
balance between usability and security at a wrong place.
But a line with broken object name already is rejected with "die()"
with the existing code, so it may be OK.
> diff --git a/t/t8013-blame-ignore-revs.sh b/t/t8013-blame-ignore-revs.sh
> index cace00ae8d..70fe509a64 100755
> --- a/t/t8013-blame-ignore-revs.sh
> +++ b/t/t8013-blame-ignore-revs.sh
> @@ -327,4 +327,42 @@ test_expect_success ignore_merge '
> test_cmp expect actual
> '
>
> +test_expect_success 'ignore-revs-file rejects lines with embedded NUL bytes' '
> + rev_b=$(git rev-parse B) &&
> + printf "%sQgarbage\n" "$rev_b" | q_to_nul >ignore_nul &&
> + test_must_fail git blame file --ignore-revs-file ignore_nul 2>err &&
> + test_grep "invalid object name:" err &&
> +
> + printf "%sQ# comment\n" "$rev_b" | q_to_nul >ignore_nul_comment &&
> + test_must_fail git blame file --ignore-revs-file ignore_nul_comment 2>err &&
> + test_grep "invalid object name:" err
> +'
> +
> +test_expect_success 'ignore-revs-file peels chained tags and skips missing tag targets' '
> + test_write_lines BB L2-modified L3 L4 L5 L6 L7 L8 CC >file &&
> + git add file &&
> + test_tick &&
> + git commit -m D &&
> + git tag -a -m "tag 1" D_TAG1 HEAD &&
> + git tag -a -m "tag 2" D_TAG2 D_TAG1 &&
> + git rev-parse D_TAG2 >ignore_tag_chain &&
> + git blame --line-porcelain file --ignore-revs-file ignore_tag_chain >blame_raw &&
> + sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
> + git rev-parse A >expect &&
> + test_cmp expect actual &&
> +
> + test_config extensions.partialClone origin &&
> + test_config remote.origin.promisor true &&
> + test_config remote.origin.url /nonexistent &&
> + missing_oid=$(test_oid deadbeef) &&
> + bad_tag=$(printf "object %s\ntype commit\ntag bad-tag\ntagger T <t@example.com> 0 +0000\n\nmsg\n" "$missing_oid" |
> + git hash-object -t tag -w --stdin) &&
> + test_write_lines "$missing_oid" "$bad_tag" >ignore_bad_tag &&
> + git blame --line-porcelain file --ignore-revs-file ignore_bad_tag >blame_raw 2>err &&
> + test_must_be_empty err &&
> + sed -ne "/^[0-9a-f][0-9a-f]* [0-9][0-9]* 2/s/ .*//p" blame_raw >actual &&
> + git rev-parse HEAD >expect &&
> + test_cmp expect actual
> +'
> +
> test_done
| @@ -132,9 +132,11 @@ take effect. | |||
| `--ignore-revs-file <file>`:: | |||
There was a problem hiding this comment.
Junio C Hamano wrote on the Git mailing list (how to reply to this email):
"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Ravi Mistry <rmistry@google.com>
>
> git-blame(1) can ignore a list of commits specified via
> --ignore-revs-file or the blame.ignoreRevsFile configuration option.
> This is useful for skipping uninteresting revisions such as tree-wide
> formatting changes, large-scale refactors, and code modernizations that
> would otherwise obscure genuine historical authorship.
>
> When revision-ignoring was introduced in commit ae3f36dea1 ("blame: add
> blame.ignoreRevsFile config option", 2019-10-18), it intentionally
> avoided adopting a default ignore file. At the time, the capability was
> new and unproven, so avoiding unrequested filesystem I/O or unexpected
> attribution shifts took priority over a project-wide default.
> Requiring explicit opt-in per clone was therefore the prudent design.
>
> Since then, maintaining a .git-blame-ignore-revs file in the repository
> root has become the de facto standard across the Git ecosystem, adopted
> by major hosting platforms (GitHub, GitLab, Gerrit) and prominent open
> source projects (such as Chromium and LLVM). As a consequence,
> developers frequently encounter a jarring mismatch: web interfaces
> seamlessly ignore formatting commits, but local git-blame(1) and
> git-annotate(1) runs do not, unless each user manually configures
> blame.ignoreRevsFile for every local checkout.
>
> Teach git-blame(1) and git-annotate(1) to automatically add the
> HEAD:.git-blame-ignore-revs blob, if it exists, as the initial element
> in the list of ignore-revs files in both bare and non-bare
> repositories. Reading the committed blob from HEAD rather than the
> working tree ensures that local runs match hosting platforms even when
> an untracked .git-blame-ignore-revs file is present or a tracked one
> has uncommitted local changes.
>
> To ensure consistent precedence and override semantics:
> - The default HEAD:.git-blame-ignore-revs entry is added before reading
> configuration and CLI options, preserving user and repository config
> overrides.
> - In git_blame_config(), blame.ignoreRevsFile entries are appended via
> string_list_append() rather than inserted in sorted order via
> string_list_insert() so that configuration entries preserve their
> order relative to the initial default entry.
> - The HEAD:.git-blame-ignore-revs tree entry is resolved quietly via
> get_oid_with_context(). Its mode is checked with S_ISREG() before
> reading the object so that non-regular tree entries (such as a
> committed symbolic link whose blob stores a target path rather than
> revision IDs, a subdirectory, or a gitlink) are skipped instead of
> being read and rejected as malformed object names. The blob is parsed
> in memory via a new oidset_parse_buffer_carefully() helper in
> oidset.c that shares line parsing with oidset_parse_file_carefully().
> - In build_ignorelist(), ignore-revs entries are processed starting
> after the last empty string entry. This ensures setting
> blame.ignoreRevsFile to "" or passing --ignore-revs-file "" or
> --no-ignore-revs-file cleanly discards the default blob without
> attempting to read or parse it, allowing users to bypass a malformed
> default blob.
>
> Update documentation in blame-options.adoc and config/blame.adoc, and
> add comprehensive test coverage in t8013 for the default blob lookup,
> subdirectory invocations, bare repositories, uncommitted and untracked
> working-tree files, CLI and config overrides, committed symlink
> entries, and comments and whitespace handling.
>
> Based-on-patch-by: Abhijeetsingh Meena <abhijeet040403@gmail.com>
> Helped-by: Kristoffer Haugsbakk <code@khaugsbakk.name>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
These Helped-by: drew my attention as none of these folks commented
on v1 of this series. I do see they have helped the original series
<pull.1809.v2.git.1728707867.gitgitgadget@gmail.com>, but it is not
clear how much their inputs have survivied to this version.
They are all CC'ed so they can give their Acked-by: or Reviewed-by:
on this round if they want.
[...]
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 6741a7b9df..a730ee87ea 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -769,7 +769,7 @@ static int git_blame_config(const char *var, const char *value,
> if (ret)
> return ret;
> if (str)
> - string_list_insert(&ignore_revs_file_list, str);
> + string_list_append(&ignore_revs_file_list, str);
> free(str);
> return 0;
> }
Good, and the log message is clear why we make this change.
> @@ -946,17 +946,52 @@ static int peel_to_commit_oid(struct object_id *oid_ret, void *cbdata)
> }
> }
>
> +static void parse_default_ignore_revs_blob(struct blame_scoreboard *sb,
> + const char *name)
> +{
> + struct object_context oc;
> + struct object_id oid;
> + enum object_type type;
> + size_t size;
> + char *buf;
> +
> + if (get_oid_with_context(the_repository, name, GET_OID_QUIETLY,
> + &oid, &oc))
> + goto out;
> + if (!S_ISREG(oc.mode))
> + goto out;
> +
> + buf = odb_read_object(the_repository->objects, &oid, &type, &size);
> + if (!buf)
> + goto out;
> + if (type == OBJ_BLOB)
> + oidset_parse_buffer_carefully(&sb->ignore_list, buf, size,
> + the_repository->hash_algo,
> + peel_to_commit_oid, sb);
> + free(buf);
> +
> +out:
> + object_context_release(&oc);
> +}
OK.
> static void build_ignorelist(struct blame_scoreboard *sb,
> struct string_list *ignore_revs_file_list,
> struct string_list *ignore_rev_list)
> {
> struct string_list_item *i;
> struct object_id oid;
> + size_t start_idx = 0, idx;
> +
> + for (idx = 0; idx < ignore_revs_file_list->nr; idx++) {
> + if (!*ignore_revs_file_list->items[idx].string)
> + start_idx = idx + 1;
> + }
OK, we make two passes, and during the first pass, we find where the
last "empty" entry that signals "forget everything you have seen" is.
> oidset_init(&sb->ignore_list, 0);
> - for_each_string_list_item(i, ignore_revs_file_list) {
> - if (!strcmp(i->string, ""))
> - oidset_clear(&sb->ignore_list);
> + for (idx = start_idx; idx < ignore_revs_file_list->nr; idx++) {
And we scan starting from there. Very clean.
> + i = &ignore_revs_file_list->items[idx];
> + if (i->util)
> + parse_default_ignore_revs_blob(sb, i->string);
> else
> oidset_parse_file_carefully(&sb->ignore_list, i->string,
> the_repository->hash_algo,
> @@ -1036,6 +1071,8 @@ int cmd_blame(int argc,
> const char *const *opt_usage = cmd_is_annotate ? annotate_opt_usage : blame_opt_usage;
>
> setup_default_color_by_age();
> + string_list_append(&ignore_revs_file_list,
> + "HEAD:.git-blame-ignore-revs")->util = &sb;
Cute. This takes advantage of the fact that everybody else just
appends to the string_list without populating the .util member.
> diff --git a/oidset.c b/oidset.c
> index 90d39204d3..8469d03b9b 100644
> --- a/oidset.c
> +++ b/oidset.c
> @@ -70,44 +70,76 @@ void oidset_parse_file(struct oidset *set, const char *path,
> oidset_parse_file_carefully(set, path, algop, NULL, NULL);
> }
>
> +static void parse_oidset_line(struct oidset *set, struct strbuf *sb,
> + const struct git_hash_algo *algop,
> + oidset_parse_tweak_fn fn, void *cbdata)
> +{
> + const char *p;
> + const char *name;
> + struct object_id oid;
> +
> + if (memchr(sb->buf, '\0', sb->len))
> + die("invalid object name: %s", sb->buf);
> +
> + /*
> + * Allow trailing comments, leading whitespace
> + * (including before commits), and empty or whitespace
> + * only lines.
> + */
> + name = strchr(sb->buf, '#');
> + if (name)
> + strbuf_setlen(sb, name - sb->buf);
> + strbuf_trim(sb);
> + if (!sb->len)
> + return;
> +
> + if (parse_oid_hex_algop(sb->buf, &oid, &p, algop) || *p != '\0')
> + die("invalid object name: %s", sb->buf);
> + if (fn && fn(&oid, cbdata))
> + return;
> + oidset_insert(set, &oid);
> +}
> +
> void oidset_parse_file_carefully(struct oidset *set, const char *path,
> const struct git_hash_algo *algop,
> oidset_parse_tweak_fn fn, void *cbdata)
> {
> FILE *fp;
> struct strbuf sb = STRBUF_INIT;
> - struct object_id oid;
>
> fp = fopen(path, "r");
> if (!fp)
> die("could not open object name list: %s", path);
> - while (!strbuf_getline(&sb, fp)) {
> - const char *p;
> - const char *name;
> -
> - if (memchr(sb.buf, '\0', sb.len))
> - die("invalid object name: %s", sb.buf);
> -
> - /*
> - * Allow trailing comments, leading whitespace
> - * (including before commits), and empty or whitespace
> - * only lines.
> - */
> - name = strchr(sb.buf, '#');
> - if (name)
> - strbuf_setlen(&sb, name - sb.buf);
> - strbuf_trim(&sb);
> - if (!sb.len)
> - continue;
> -
> - if (parse_oid_hex_algop(sb.buf, &oid, &p, algop) || *p != '\0')
> - die("invalid object name: %s", sb.buf);
> - if (fn && fn(&oid, cbdata))
> - continue;
> - oidset_insert(set, &oid);
> - }
> + while (!strbuf_getline(&sb, fp))
> + parse_oidset_line(set, &sb, algop, fn, cbdata);
> if (ferror(fp))
> die_errno("Could not read '%s'", path);
> fclose(fp);
> strbuf_release(&sb);
> }
Shouldn't the above refactoring have been part of the previous step
instead?
Thanks.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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 #1809
(https://lore.kernel.org/git/pull.1809.v2.git.1728707867.gitgitgadget@gmail.com/)
and addresses #1494.
Changes since v1:
peel_to_commit_oid() in builtin/blame.c before exposing them to
upstream-controlled content at a well-known path:
trailing bytes after a NUL cannot be silently ignored.
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.
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.
CC: Junio C Hamano gitster@pobox.com
CC: Abhijeetsingh Meena abhijeet040403@gmail.com
CC: Kristoffer Haugsbakk code@khaugsbakk.name
CC: Phillip Wood phillip.wood@dunelm.org.uk
CC: Eric Sunshine sunshine@sunshineco.com