From: Junio C Hamano Date: Fri, 09 Oct 2026 20:17:09 GMT Subject: Re: [PATCH v2 2/2] blame: ignore revs in HEAD:.git-blame-ignore-revs Message-ID: In-Reply-To: <35e303d65bc378e733b1e9e8d6908a829352e857.1791493644.git.gitgitgadget@gmail.com> "Ravi Mistry via GitGitGadget" writes: > From: Ravi Mistry > > 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 > Helped-by: Kristoffer Haugsbakk > Helped-by: Phillip Wood > Helped-by: Eric Sunshine 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 , 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.