Re: [PATCH v2 1/2] blame: respect .git-blame-ignore-revs automatically

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Abhijeetsingh

On 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:
> From: Abhijeetsingh Meena <abhijeet040403@gmail.com>
> 
> git-blame(1) can ignore a list of commits with `--ignore-revs-file`.
> This is useful for marking uninteresting commits like formatting
> changes, refactors and whatever else should not be “blamed”.  Some
> projects even version control this file so that all contributors can
> use it; the conventional name is `.git-blame-ignore-revs`.
> 
> But each user still has to opt-in to the standard ignore list,
> either with this option or with the config `blame.ignoreRevsFile`.
> Let’s teach git-blame(1) to respect this conventional file in order
> to streamline the process.

It's good that the commit message now mentions the config setting. It 
would be helpful to explain why the original implementation deliberately 
decided not to implement a default file and explain why it is now a good 
idea to do so. Supporting a default file in addition to the files listed 
in blame.ignoreRevsFile config setting leaves us in an odd position 
compared to other settings which use a fixed name like .gitignore or 
have a default that can be overridden by a config setting like 
core.excludesFile or require a config setting to enable the feature like 
diff.orderFile.

I've left a couple of code comments below but really the most important 
things are to come up with a convincing reason for changing the behavior 
and figuring out how the default file should interact with the config 
setting.

> +	/*
> +	* By default, add .git-blame-ignore-revs to the list of files
> +	* containing revisions to ignore if it exists.
> +	*/
> +	if (access(".git-blame-ignore-revs", F_OK) == 0) {

There are some uses of "access(.., F_OK)" in our code base but it is 
more usual to call file_exists() these days.

> +		string_list_append(&ignore_revs_file_list, ".git-blame-ignore-revs");

If the user already has this path in their config we'll waste time 
parsing it twice. We could avoid that by using a "struct strset" rather 
than a "struct string_list". I don't think we have OPT_STRSET but it 
should be easy to add one by copying OPT_STRING_LIST.

> +    echo world >>hello.txt &&
> +    test_commit second-commit hello.txt &&

test_commit overwrites the file it is committing so you need to use the 
--printf option

	test_commit --printf second-commit hello.txt "hello\nworld\n"

> +    git rev-parse HEAD >ignored-file &&
> +    git blame --ignore-revs-file=ignored-file hello.txt >expect &&
> +    git rev-parse HEAD >.git-blame-ignore-revs &&
> +    git blame hello.txt >actual &&
> +    test_cmp expect actual

I have mixed feelings about this sort of differential testing, comparing 
the actual output of git blame to what we expect makes it unambiguous 
that the test is checking what we want it to.

Best Wishes

Phillip