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

[Eric Sunshine <sunshine@sunshineco.com>]

On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> 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(+)

This change should be accompanied by a documentation update, I would think.

> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -1105,6 +1105,14 @@ parse_done:
> +       /*
> +       * 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");
> +       }

A couple style nits and a couple questions...

nit: drop the braces around the one-line `if` body

nit: this project uses `!foo(...)` rather than `foo(...) == 0`

Presumably this consults ".git-blame-ignore-revs" in the top-level
directory (as you intended) rather than ".git-blame-ignore-revs" in
whatever subdirectory you happen to issue the command because the
current-working-directory has already been set to the top-level
directory by the time cmd_blame() has been called, right?

But that leads to the next question. Should automatic consulting of
".git-blame-ignore-revs" be restricted to just the top-level
directory, or should it be modeled after, say, ".gitignore" which may
be strewn around project directories and in which ".gitignore" files
are consulted rootward starting from the directory in which the
command is invoked. My knee-jerk thought was that the ".gitignore"
model may not make sense for ".git-blame-ignore-revs", but the fact
that `git blame` can accept and work with multiple ignore-revs files
makes me question that knee-jerk response.

> diff --git a/t/t8015-blame-default-ignore-revs.sh b/t/t8015-blame-default-ignore-revs.sh
> new file mode 100755

Let's avoid allocating a new test number just for this single new
test. Instead, the existing t8013-blame-ignore-revs.sh would probably
be a good home for this new test.

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

style: drop space after redirection operator

    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 &&

I would suggest copying or renaming "ignored-file" to
".git-blame-ignore-revs" rather than running `git rev-parse HEAD`
twice. This way readers won't have to waste mental effort verifying
that the result of `git rev-parse HEAD` isn't intended to change
between invocations.