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

Commit Message

Abhijeetsingh Meena Oct. 12, 2024, 4:37 a.m. UTC

From: Abhijeetsingh Meena <[e-mail]>

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.

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 <[e-mail]>
---
 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

Comments

Eric Sunshine Oct. 12, 2024, 6:24 a.m. UTC | #1

On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget
<[e-mail]> 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 <[e-mail]>
> ---
> 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.

Eric Sunshine Oct. 12, 2024, 6:26 a.m. UTC | #2

On Sat, Oct 12, 2024 at 2:24 AM Eric Sunshine <[e-mail]> wrote:
> On Sat, Oct 12, 2024 at 12:38 AM Abhijeetsingh Meena via GitGitGadget
> <[e-mail]> wrote:
> > [...]
> > 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.
> > +               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.

I forgot to mention that this patch also deserves a documentation
update; otherwise how are users going to know that the new option
exists?

Kristoffer Haugsbakk Oct. 12, 2024, 2:25 p.m. UTC | #3

On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:
> From: Abhijeetsingh Meena <[e-mail]>
>
> 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 <[e-mail]>
> ---
>  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

Phillip Wood Oct. 13, 2024, 3:20 p.m. UTC | #4

Hi Abhijeetsingh

On 12/10/2024 05:37, Abhijeetsingh Meena via GitGitGadget wrote:
> From: Abhijeetsingh Meena <[e-mail]>
> 
> 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.

As Kristoffer has pointed out --no-ignore-revs-file should be sufficient 
to disable the default file. If it isn't we should fix it so that it is, 
not add a new option.

Best Wishes

Phillip

> 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.
> 
> 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 <[e-mail]>
> ---
>   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);
> +	}
> +
>   	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

Patch

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);
+	}
+
 	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