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

Hi Eric,

Thank you for your thoughtful feedback on v2 of the patch.
Before I proceed with v3, I'd like to address some of the
non-code-related questions and seek your input.

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?

Yes, it seems that the current-working-directory is set to the root of
the repository,
as I tested this behaviour locally. The .git-blame-ignore-revs file in
the root worked
as expected, while a similar file in a subdirectory did not.

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.

I think both approaches have their merits:

1. Single file
*Purpose:* Having a single .git-blame-ignore-revs file aligns with the idea
of globally ignoring revisions, making it easier for maintainers to
control irrelevant commits.
*Simplicity:* Keeping the file in the root ensures centralized management,
simplifying configuration.

2. Multiple files:
*Large repositories:* In large monorepos, different teams working in separate
subdirectories may want to manage their own ignored revisions. Multiple files
would offer flexibility, particularly for modular projects or those
with distinct submodules.
*Flexibility:* Subdirectory-level .git-blame-ignore-revs files could
allow users to
 fine-tune blame results for their specific areas, especially when
local refactors
are limited to certain parts of the codebase.

Given this, I would like to know your suggestions, as I’m not too
experienced with
the user workflows and what would be more helpful to them. For now, I think we
should stick with the single .git-blame-ignore-revs file at the top level.
However, we could keep the option open for future enhancements, allowing
multiple files to be consulted by setting a configuration flag if a
specific use case arises.

Is the all-or-nothing behavior implemented by this patch desirable? If
so, should the command warn or error out if the user gives conflicting
options like --ignore-revs-file and --override-ignore-revs together?

A common behavior of many Git commands when dealing with options is
"last wins", and following that precedent could make this new option
even much more useful by allowing the user to ignore project-supplied
ignore-revs but still take advantage of the feature with a different
set of ignore-revs that make sense to the local user. For instance:

    git blame --override-ignore-revs --ignore-revs-file=my-ignore-revs ...

I don’t think the all-or-nothing approach is ideal. Based on Phillip's
suggestions and
Kristoffer's conceptual workflow, I explored using `git_config_set` to
set `blame.ignoreRevsFile`
configuration. This would integrate well with existing configuration
logic and provide greater flexibility.

With `git_config_set`:

git blame hello.txt
would consult the default .git-blame-ignore-revs file.

git blame --no-ignore-revs-file hello.txt
would disable the default ignore file.

git blame --no-ignore-revs-file --ignore-revs-file=ignore-list hello.txt
would allow the user to specify a custom ignore list while bypassing
the global list,
offering the flexibility you suggested.

What is this test actually checking? It doesn't seem to use
--override-ignore-revs at all.

Actually, I used the short form -O to represent --override-ignore-revs
in this test.

Thank you again for your time and feedback. I look forward to your thoughts on
these points before finalising the next patch revision.

Best regards,
Abhijeet

On Sat, Oct 12, 2024 at 11:37 AM Eric Sunshine [off-list ref] wrote:

On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget
[off-list ref] wrote:

quoted

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

quoted

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.

quoted

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.

quoted

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

quoted

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