Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list
From: Kristoffer Haugsbakk <hidden>
Date: 2024-10-12 14:25:54
On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:
From: Abhijeetsingh Meena <redacted> The git blame command can ignore a list of revisions specified either through the --ignore-revs-file option or the blame.ignoreRevsFile configuration. This feature is useful for excluding irrelevant commits, such as formatting changes or large refactors, from blame annotations.
In a later paragraph you mention `--ignore-rev` but not here.
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.
This patch introduces the --override-ignore-revs option (or -O),
Phrases like “this patch” is discouraged compared to the imperative mood style of commanding the code base to change (so to speak). See `imperative-mood` in Documentation/SubmittingPatches.
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`.
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
quoted hunk ↗ jump to hunk
Signed-off-by: Abhijeetsingh Meena <redacted> --- builtin/blame.c | 8 +++++++- t/t8016-blame-override-ignore-revs.sh | 25 +++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) create mode 100755 t/t8016-blame-override-ignore-revs.shdiff --git a/builtin/blame.c b/builtin/blame.c index 1eddabaf60f..956520edcd9 100644 --- a/builtin/blame.c +++ b/builtin/blame.c@@ -69,6 +69,7 @@ static int coloring_mode; static struct string_list ignore_revs_file_list = STRING_LIST_INIT_DUP; static int mark_unblamable_lines; static int mark_ignored_lines; +static int override_ignore_revs = 0; static struct date_mode blame_date_mode = { DATE_ISO8601 }; static size_t blame_date_width;@@ -901,6 +902,7 @@ int cmd_blame(int argc, OPT_BIT('w', NULL, &xdl_opts, N_("ignore whitespace differences"),XDF_IGNORE_WHITESPACE), OPT_STRING_LIST(0, "ignore-rev", &ignore_rev_list, N_("rev"), N_("ignore <rev> when blaming")), OPT_STRING_LIST(0, "ignore-revs-file", &ignore_revs_file_list, N_("file"), N_("ignore revisions from <file>")), + OPT_BOOL('O', "override-ignore-revs", &override_ignore_revs, N_("override all configurations that exclude revisions")), OPT_BIT(0, "color-lines", &output_option, N_("color redundant metadata from previous line differently"), OUTPUT_COLOR_LINE), OPT_BIT(0, "color-by-age", &output_option, N_("color lines by age"), OUTPUT_SHOW_AGE_WITH_COLOR), OPT_BIT(0, "minimal", &xdl_opts, N_("spend extra cycles to find better match"), XDF_NEED_MINIMAL),@@ -1119,7 +1121,11 @@ parse_done: sb.reverse = reverse; sb.repo = the_repository; sb.path = path; - build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list); + + 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.
quoted hunk ↗ jump to hunk
string_list_clear(&ignore_revs_file_list, 0); string_list_clear(&ignore_rev_list, 0); setup_scoreboard(&sb, &o);diff --git a/t/t8016-blame-override-ignore-revs.shb/t/t8016-blame-override-ignore-revs.sh new file mode 100755 index 00000000000..b5899729f8e--- /dev/null +++ b/t/t8016-blame-override-ignore-revs.sh@@ -0,0 +1,25 @@ +#!/bin/sh + +test_description='default revisions to ignore when blaming' + +TEST_PASSES_SANITIZE_LEAK=true +. ./test-lib.sh + +test_expect_success 'blame: override-ignore-revs' ' + 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 blame hello.txt >expect && + git rev-parse HEAD >.git-blame-ignore-revs && + git blame -O hello.txt >actual && + + test_cmp expect actual +' + +test_done --gitgitgadget