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

["Kristoffer Haugsbakk" <code@khaugsbakk.name>]

On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:
> From: Abhijeetsingh Meena <abhijeet040403@gmail.com>
>
> 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
```

>
> Signed-off-by: Abhijeetsingh Meena <abhijeet040403@gmail.com>
> ---
>  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.sh
>
> diff --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.

>  	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.sh
> b/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