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

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

"Ravi Mistry via GitGitGadget" <gitgitgadget@gmail.com> writes:

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

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

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

That's "a configuration variable".

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

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