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 <sunshine@xxxxxxxxxxxxxx> wrote:
>
> On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget
> <gitgitgadget@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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.
