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

[Taylor Blau <me@ttaylorr.com>]

On Sat, Oct 12, 2024 at 02:07:36AM -0400, Eric Sunshine wrote:
> > 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.

All very good suggestions and questions for Abhijeetsingh to consider.

At a minimum, I think the style nits need to be addressed here. But I
also think it is worth considering seriously whether or not multiple
`.git-blame-ignore-revs` files should be considered, and if so, in what
order and how they override (or not) each other.

I am generally OK with adding one of these special files and having 'git
blame' respect it automatically. But once we do so, it is going to be
considered part of our compatibility guarantee, so we should get it
right the first time.

Thanks,
Taylor