Re: [PATCH v2 2/2] blame: introduce --override-ignore-revs to bypass ignore revisions list
- From
Kristoffer Haugsbakk <code@khaugsbakk.name>
- Date
- Oct 12, 2024, 14:25 UTC
- Message-ID
- <09a4cec6-c655-44e9-9577-01e6a8cb1a3d@app.fastmail.com>
- In-Reply-To
- <8d2fa3af7964dacd09d454de4325b1d5eb7a5c3d.1728707867.git.gitgitgadget@gmail.com>
On Sat, Oct 12, 2024, at 06:37, Abhijeetsingh Meena via GitGitGadget wrote:
Show 7 quoted lines
> 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`.
Show 8 quoted lines
> 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 ```
Show 45 quoted lines
>
> 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.
Show 37 quoted lines
> 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