Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Oct 12, 2024, 06:24 UTC
Message-ID: <CAPig+cRkp3ZrykLvgDaGnKgZNV5GiFqc5zq6ZZ-fM7RJcqxM7A@mail.gmail.com>
In-Reply-To: <8d2fa3af7964dacd09d454de4325b1d5eb7a5c3d.1728707867.git.gitgitgadget@gmail.com>

On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 16 quoted lines

> 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.

Show 13 quoted lines

> 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@gmail.com>
> ---
> 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.

Show 6 quoted lines

> @@ -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.

Show 8 quoted lines

> @@ -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 &&

Show 9 quoted lines

> +    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.