Hi Kristoffer,
Thank you for reviewing the v2 of my patch. I appreciate your
thoughtful feedback.
Before proceeding with v3, I’d like to address some of your questions
and suggestions.
> 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. It has helped shape the solution, as I described in my
response to Eric earlier.
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 fits 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 have not tested these patches. But I see why you check for file access/existence. Because with this config:
>
> ```
> [blame]
> ignoreRevsFile=.git-blame-ignore-revs
> ```
>
> I get this warning in repositories that don’t have the file:
>
> ```
> fatal: could not open object name list: .git-blame-ignore-revs
> ```
>
> Which is just noise.
>
> I get the same thing with Git Notes namespace configurations. I need to
> configure them for certain repositories (like `amlog` in this project),
> but then I get warnings about them when using the relevant commands in a
> project that does not have them.
>
> Maybe this is totally off-topic but I think it would make more sense if
> `blame.ignoreRevsFile` just didn’t say anything if it didn’t find the
> file. Because the point of the config might be to opt-in to this file
> for those projects that does have it.
Yes, I agree. For a default ignore file, we shouldn't raise a fatal
error if the file is missing, especially if it’s not present in every
repository.
Suppressing the warning for the default file would improve user experience
and prevent unnecessary noise.
> > However, users may encounter cases where they need to
> > temporarily override these configurations to inspect all commits,
> > even those excluded by the ignore list. Currently, there is no
> > simple way to bypass all ignore revisions settings in one go.
>
> “No simple way” gives me pause. But there are those options/methods
> that we discussed before:
>
> • `--no-ignore-rev`
> • `--no-ignore-revs-file`
>
> These are not documented but I can provide these options and get a
> different output from git-blame(1).
>
> `builtin/blame.c` uses `parse-options.h` which provides automatic
> negated options. I just looked at the code today (so it’s new to me)
> but it seems like it will empty the lists that are associated with these
> options. See `parse-options-cb.c:parse_opt_string_list`.
>
> So I think this should be sufficient to reset all “ignore” options:
>
> ```
> git blame --no-ignore-rev --no-ignore-revs-file
> ```
>
> However I tested with this:
>
> ```
> git blame --ignore-revs-file=.git-blame-ignore-revs --no-ignore-revs
> ```
>
> And the output suggests to me that `--no-ignore-revs` affect the result
> of the before-mentioned list of files. Even though these are two
> different lists. I can’t make sense of that from the code. But I’m not
> a C programmer so this might just be a me-problem.
Yes, --no-ignore-revs-file and --no-ignore-rev flags work as intended
to bypass the configuration that ignores revisions. They are separate lists, so
--no-ignore-revs shouldn’t affect the --ignore-revs-file list. My previous
testing post v1 had some issues in test setup, which led me to believe that
the --no-ignore flags don’t work and I worked on --override-ignore-revs.
> > which allows users to easily bypass the --ignore-revs-file
> > option, --ignore-rev option and the blame.ignoreRevsFile
>
> I can see no precedence for the name “override” for an option in this
> project. The convention is `--[no-]option`.
>
> Like Eric Sunshine discussed: a common convention is to let the user
> activate and negate options according to the last-wins rule. This is
> pretty useful in my opinion. Because I can then make an alias which
> displays some Git Note:
>
> ```
> timber = log [options] --notes=results
> ```
>
> But then what if I don’t want any notes for a specific invocation? I
> don’t have to copy the whole alias and modify it. I can just:
>
> ```
> git timber --no-notes
> ```
>
> And the same goes for an alias which disables notes:
>
> ```
> timber = log [options] --no-notes
> ```
>
> Because then I can use `git timber --notes=results`.
I agree that the override option is unnecessary, as both
--no-ignore-rev and --no-ignore-revs-file
already allow users to bypass the ignore configurations. Also, the
“last-wins” approach is more
useful and aligns with how Git typically handles configurations. It’s
flexible and user-friendly,
allowing for easy toggling within aliases or individual commands.
Implementing this using the
existing configuration method, such as git_config_set, would be a
clean and effective solution
to ensure that users can quickly modify or negate options as needed.
> > configuration. When this option is used, git blame will completely
> > disregard all configured ignore revisions lists.
> >> The motivation behind this feature is to provide users with more
> > flexibility when dealing with large codebases that rely on
> > .git-blame-ignore-revs files for shared configurations, while
> > still allowing them to disable the ignore list when necessary
> > for troubleshooting or deeper inspections.
>
> You might be able to achieve the same thing with the existing negated
> options.
>
> If you *cannot* disable all “ignore” config and options in one negated
> one then you might want an option like `--no-ignores` which acts like:
>
> ```
> git blame --no-ignore-rev --no-ignore-revs-file
> ```
Yes, the override option isn’t necessary since the existing flags work
as intended.
If needed in the future, we can add a single flag to reset both lists, or as you
mentioned it can be an alias too.
> > + if (!override_ignore_revs) {
> > + build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);
> > + }
> > +
>
> This demonstrates the more limited behavior: you either override
> (discard) the ignores or you don’t. With the negated options you build
> up and reset/empty those lists before you get to this point. That ends
> up being more flexible for the user.
Yes, this approach was more limited, we can follow the approach
described earlier that uses git_config_set to handle ignoring
revisions and revision lists more flexibly.
Thanks again for your detailed feedback. I hope this approach is
better than my previous approach.
I’ll incorporate these changes and move forward with v3. Looking
forward to your further thoughts!
Best regards,
Abhijeetsingh
On Sat, Oct 12, 2024 at 7:28 PM Kristoffer Haugsbakk
<code@xxxxxxxxxxxxxxx> wrote:
>
> 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.
>
> On Sat, Oct 12, 2024, at 06: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.
> >
> > Signed-off-by: Abhijeetsingh Meena <abhijeet040403@xxxxxxxxx>
> > ---
> > builtin/blame.c | 8 ++++++++
> > t/t8015-blame-default-ignore-revs.sh | 26 ++++++++++++++++++++++++++
> > 2 files changed, 34 insertions(+)
> > create mode 100755 t/t8015-blame-default-ignore-revs.sh
> >
> > diff --git a/builtin/blame.c b/builtin/blame.c
> > index e407a22da3b..1eddabaf60f 100644
> > --- a/builtin/blame.c
> > +++ b/builtin/blame.c
> > @@ -1105,6 +1105,14 @@ parse_done:
> > add_pending_object(&revs, &head_commit->object, "HEAD");
> > }
> >
> > + /*
> > + * 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");
> > + }
> > +
>
> I have not tested these patches. But I see why you check for file access/existence. Because with this config:
>
> ```
> [blame]
> ignoreRevsFile=.git-blame-ignore-revs
> ```
>
> I get this warning in repositories that don’t have the file:
>
> ```
> fatal: could not open object name list: .git-blame-ignore-revs
> ```
>
> Which is just noise.
>
> I get the same thing with Git Notes namespace configurations. I need to
> configure them for certain repositories (like `amlog` in this project),
> but then I get warnings about them when using the relevant commands in a
> project that does not have them.
>
> Maybe this is totally off-topic but I think it would make more sense if
> `blame.ignoreRevsFile` just didn’t say anything if it didn’t find the
> file. Because the point of the config might be to opt-in to this file
> for those projects that does have it.
>
> > init_scoreboard(&sb);
> > sb.revs = &revs;
> > sb.contents_from = contents_from;
> > diff --git a/t/t8015-blame-default-ignore-revs.sh
> > b/t/t8015-blame-default-ignore-revs.sh
> > new file mode 100755
> > index 00000000000..d4ab686f14d
> > --- /dev/null
> > +++ b/t/t8015-blame-default-ignore-revs.sh
> > @@ -0,0 +1,26 @@
> > +#!/bin/sh
> > +
> > +test_description='default revisions to ignore when blaming'
> > +
> > +TEST_PASSES_SANITIZE_LEAK=true
> > +. ./test-lib.sh
> > +
> > +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 &&
> > + 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 &&
> > +
> > + test_cmp expect actual
> > +'
> > +
> > +test_done
> > --
> > gitgitgadget
>
> --
> Kristoffer
