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

From: Kristoffer Haugsbakk <code@khaugsbakk.name>
Date: Oct 12, 2024, 13:58 UTC
Message-ID: <4f835282-d921-4cd3-9680-d8522bef7b39@app.fastmail.com>
In-Reply-To: <4ed930cab1b7f5e9738e73c7b9374d927a8acd94.1728707867.git.gitgitgadget@gmail.com>

Hi Abhijeetsingh

For what it’s worth here’s how I imagine this feature could work conceptually:

Before this feature/change, the effective config for Git use looks like this:

``` [blame] ```

No `blame.ignoreRevsFile`.

But with/after it:

```
[blame]
	ignoreRevsFile=.git-blame-ignore-revs
```

This is the effective config.  Not what the user has typed out.

If the user types out this:

```
[blame]
	ignoreRevsFile=.git-blame-more-revs
```

Then this becomes their effective config:

```
[blame]
	ignoreRevsFile=.git-blame-ignore-revs
	ignoreRevsFile=.git-blame-more-revs
```

Now there are two files: the default one and the user-supplied one (this config variable is documented as being multi-valued: “This option may be repeated multiple times.”).

§ How to ignore this new default §§§

Considering users who do not want this new default:

```
[blame]
	ignoreRevsFile=
```

This is the change they would have to make. Because a blank/empty resets/empties the list of files.

On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:

Show 36 quoted lines

> 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.
>
> Signed-off-by: Abhijeetsingh Meena <abhijeet040403@gmail.com>
> ---
>  builtin/blame.c                      |  8 ++++++++
>  t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++
>  2 files changed, 34 insertions(+)
>  create mode 100755 t/t8015-blame-default-ignore-revs.sh
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index e407a22da3b..1eddabaf60f 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -1105,6 +1105,14 @@ parse_done:
>  		add_pending_object(&revs, &head_commit->object, "HEAD");
>  	}
>
> +	/*
> +	* 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) {
> +		string_list_append(&ignore_revs_file_list, ".git-blame-ignore-revs");
> +	}
> +

I have not tested these patches.  But I see why you check for file access/existence.  Because with this config:

```
[blame]
	ignoreRevsFile=.git-blame-ignore-revs
```

I get this warning in repositories that don’t have the file:

``` fatal: could not open object name list: .git-blame-ignore-revs ```

Which is just noise.

I get the same thing with Git Notes namespace configurations. I need to configure them for certain repositories (like `amlog` in this project), but then I get warnings about them when using the relevant commands in a project that does not have them.

Maybe this is totally off-topic but I think it would make more sense if `blame.ignoreRevsFile` just didn’t say anything if it didn’t find the file. Because the point of the config might be to opt-in to this file for those projects that does have it.

Show 38 quoted lines

>  	init_scoreboard(&sb);
>  	sb.revs = &revs;
>  	sb.contents_from = contents_from;
> diff --git a/t/t8015-blame-default-ignore-revs.sh
> b/t/t8015-blame-default-ignore-revs.sh
> new file mode 100755
> index 00000000000..d4ab686f14d
> --- /dev/null
> +++ b/t/t8015-blame-default-ignore-revs.sh
> @@ -0,0 +1,26 @@
> +#!/bin/sh
> +
> +test_description='default revisions to ignore when blaming'
> +
> +TEST_PASSES_SANITIZE_LEAK=true
> +. ./test-lib.sh
> +
> +test_expect_success 'blame: default-ignore-revs-file' '
> +    test_commit first-commit hello.txt hello &&
> +
> +    echo world >>hello.txt &&
> +    test_commit second-commit hello.txt &&
> +
> +    sed "1s/hello/hi/" <hello.txt > hello.txt.tmp &&
> +    mv hello.txt.tmp hello.txt &&
> +    test_commit third-commit hello.txt &&
> +
> +    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
> +'
> +
> +test_done
> --
> gitgitgadget

-- 
Kristoffer