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

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

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

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