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

[Junio C Hamano <gitster@pobox.com>]

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