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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 12, 2024, 06:07 UTC
Message-ID: <CAPig+cR4WbcDDav0cdXxOMC-EDe2ipWxEzB+0C7zbFjvY_kXtg@mail.gmail.com>
In-Reply-To: <4ed930cab1b7f5e9738e73c7b9374d927a8acd94.1728707867.git.gitgitgadget@gmail.com>

On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 16 quoted lines

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

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 7 quoted lines

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