On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget
<gitgitgadget@xxxxxxxxx> wrote:
> 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.
>
> 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.
>
> This patch introduces the --override-ignore-revs option (or -O),
> which allows users to easily bypass the --ignore-revs-file
> option, --ignore-rev option and the blame.ignoreRevsFile
> configuration. When this option is used, git blame will completely
> disregard all configured ignore revisions lists.
It's not clear from this description whether ".git-blame-ignore-revs"
is also ignored by --override-ignore-revs. Looking at the
implementation, it appears that it is, but it would be good to state
so here.
> 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.
>
> Signed-off-by: Abhijeetsingh Meena <abhijeet040403@xxxxxxxxx>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -901,6 +902,7 @@ int cmd_blame(int argc,
> 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")),
We don't normally allocate a short option name ("-O" in this case)
when introducing a new option since short option names are considered
a valuable and limited resource. A short option name may be added
*later* if experience shows that the option is popular enough that the
convenience of the short option name is warranted.
> @@ -1119,7 +1121,11 @@ parse_done:
> - build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);
> +
> + if (!override_ignore_revs) {
> + build_ignorelist(&sb, &ignore_revs_file_list, &ignore_rev_list);
> + }
A style nit and questions and observations...
nit: drop the braces around the one-line `if` body
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 ...
> diff --git a/t/t8016-blame-override-ignore-revs.sh b/t/t8016-blame-override-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.
> @@ -0,0 +1,25 @@
> +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 &&
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 blame hello.txt >expect &&
> + git rev-parse HEAD >.git-blame-ignore-revs &&
> + git blame -O hello.txt >actual &&
> +
> + test_cmp expect actual
> +'
What is this test actually checking? It doesn't seem to use
--override-ignore-revs at all.
