Hi Eric, Kristoffer, and Phillip,
Thank you for reviewing the v2 of my patch. I appreciate your
thoughtful feedback.
A few days ago, I sent a detailed email addressing each of your
questions and suggestions individually. Before proceeding with v3,
I’d like to consolidate my thoughts on the next possible approach
to respect '.git-blame-ignore-revs' based on Kristoffer's conceptual
explanation.
> Hi Abhijeetsingh
>
> For what it’s worth here’s how I imagine this feature could work
> conceptually:
>
> Before this feature/change, the effective config for Git use looks like this:
>
> ```
> [blame]
> ```
>
> No `blame.ignoreRevsFile`.
>
> But with/after it:
>
> ```
> [blame]
> ignoreRevsFile=.git-blame-ignore-revs
> ```
>
> This is the effective config. Not what the user has typed out.
>
> If the user types out this:
>
> ```
> [blame]
> ignoreRevsFile=.git-blame-more-revs
> ```
>
> Then this becomes their effective config:
>
> ```
> [blame]
> ignoreRevsFile=.git-blame-ignore-revs
> ignoreRevsFile=.git-blame-more-revs
> ```
>
> Now there are two files: the default one and the user-supplied one (this
> config variable is documented as being multi-valued: “This option may be
> repeated multiple times.”).
>
> § How to ignore this new default §§§
>
> Considering users who do not want this new default:
>
> ```
> [blame]
> ignoreRevsFile=
> ```
>
> This is the change they would have to make. Because a blank/empty
> resets/empties the list of files.
Thanks, Kristoffer. Your conceptual explanation gave me a new
perspective on how this feature can be implemented using the
existing configuration flow without disrupting other settings.
Based on Phillip's clue of exploring how this feature would interact
with existing configuration settings and your conceptual workflow,
I explored git_config_set and used it to set the
blame.ignoreRevsFile configuration. This approach aligns well with
the existing configuration logic and provides greater flexibility.
With git_config_set to set blame.ignoreRevsFile:
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.
This would maintain consistency with Git’s existing behavior, allowing
users to modify configurations with a “last-wins” approach and enabling
both global and custom ignore lists as needed.
I hope this approach is better than my previous one. I look forward to
your thoughts!
Best Regards,
Abhijeetsingh
On Wed, Oct 16, 2024 at 11:37 AM Abhijeetsingh Meena
<abhijeetsingh.github@xxxxxxxxx> wrote:
>
> Hi Phillip,
> Thank you for reviewing the patch and providing valuable feedback.
> I’d like to address some of your points below:
>
>
> > Supporting a default file in addition to the files listed in
> > blame.ignoreRevsFile config setting leaves us in an odd position
> > compared to other settings which use a fixed name like .gitignore
> > or have a default that can be overridden by a config setting like
> > core.excludesFile or require a config setting to enable the feature
> > like diff.orderFile.
>
> Yes, I now understand that we can solve this by using the existing method for
> interacting with configurations, as suggested by you and Kristoffer. We can work
> with the existing configuration method like git_config_set to set ignore
> revisions file. This (I hope) will also keep it consistent with how
> other settings like .gitignore
> and core.excludesFile work, making the interaction more predictable for users.
>
>
> > I've left a couple of code comments below but really
> > the most important things are to come up with a convincing
> > reason for changing the behavior and figuring out how
> > the default file should interact with the config setting.
>
> I agree. After revisiting the use case and the flow, I see now that
> the solution can be
> more straightforward with git_config_set than my previous approach. This
> behavior allows for interaction through the configuration system
> without the need to
> introduce new options. Kristoffer’s suggestion clarified that handling
> .git-blame-ignore-revs
> a default file and allowing it to be overridden or disabled via
> --no-ignore-revs-file is sufficient.
>
>
> > As Kristoffer has pointed out --no-ignore-revs-file should
> > be sufficient to disable the default file. If it isn't we
> > should fix it so that it is, not add a new option.
>
> Absolutely, you're right. After revisiting my earlier testing issues,
> I realized that the
> --no-ignore-revs-file and --no-ignore-rev flag works as intended. My
> previous confusion was due to a mistake in my test setup. I agree with your
> suggestion that we should not add a new option and instead focus on ensuring
> that the current flag behavior is clear and functions correctly.
>
>
> Thanks again for your review. I hope this approach is better than my
> previous approach.
> I’ll make sure the changes are implemented correctly in v3
> and test the interaction between the default file and config settings
> more thoroughly.
> Looking forward to your further thoughts!
>
> Best regards,
> Abhijeetsingh
>
> On Sun, Oct 13, 2024 at 8:48 PM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
> >
> > Hi Abhijeetsingh
> >
> > On 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:
> > > From: Abhijeetsingh Meena <abhijeet040403@xxxxxxxxx>
> > >
> > > 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.
> >
> > It's good that the commit message now mentions the config setting. It
> > would be helpful to explain why the original implementation deliberately
> > decided not to implement a default file and explain why it is now a good
> > idea to do so. Supporting a default file in addition to the files listed
> > in blame.ignoreRevsFile config setting leaves us in an odd position
> > compared to other settings which use a fixed name like .gitignore or
> > have a default that can be overridden by a config setting like
> > core.excludesFile or require a config setting to enable the feature like
> > diff.orderFile.
> >
> > I've left a couple of code comments below but really the most important
> > things are to come up with a convincing reason for changing the behavior
> > and figuring out how the default file should interact with the config
> > setting.
> >
> > > + /*
> > > + * 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) {
> >
> > There are some uses of "access(.., F_OK)" in our code base but it is
> > more usual to call file_exists() these days.
> >
> > > + string_list_append(&ignore_revs_file_list, ".git-blame-ignore-revs");
> >
> > If the user already has this path in their config we'll waste time
> > parsing it twice. We could avoid that by using a "struct strset" rather
> > than a "struct string_list". I don't think we have OPT_STRSET but it
> > should be easy to add one by copying OPT_STRING_LIST.
> >
> > > + echo world >>hello.txt &&
> > > + test_commit second-commit hello.txt &&
> >
> > test_commit overwrites the file it is committing so you need to use the
> > --printf option
> >
> > test_commit --printf second-commit hello.txt "hello\nworld\n"
> >
> > > + 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 &&
> > > + test_cmp expect actual
> >
> > I have mixed feelings about this sort of differential testing, comparing
> > the actual output of git blame to what we expect makes it unambiguous
> > that the test is checking what we want it to.
> >
> > Best Wishes
> >
> > Phillip
> >
