Hi Barret,
On Thu, 17 Jan 2019, Barret Rhoden wrote:
> [...]
>
> Users can ignore a revision with --ignore-rev=rev, which may be
> repeated. They can specify a file of full SHA-1 hashes of revs (one per
> line) with the blame.ignoreRevFile config option. They can also specify
> a file with --ignore-rev-file=file, which overrides the config option.
This sounds like that is already the case in Git, but the truth is: this
patch *introduces* that feature. Maybe start the paragraph with "With this
patch, ..."?
I cannot speak for the correctness of the changes to blame.c, others on
the Cc: list are already much more familiar with that code, so I'll leave
it to them to comment on that.
However, I am missing a regression test for this behavior, the best idea
would be likely to add t/t8013-blame-ignore-revs.sh (copy-edit it from
t/t8009-blame-vs-topicbranches.sh, maybe).
And there is another thing:
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 6d798f99392e..2f9183fb5fbd 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -52,6 +52,7 @@ static int no_whole_file_rename;
> static int show_progress;
> static char repeated_meta_color[COLOR_MAXLEN];
> static int coloring_mode;
> +static const char *ignore_revs_file;
... this...
>
> static struct date_mode blame_date_mode = { DATE_ISO8601 };
> static size_t blame_date_width;
> @@ -695,6 +696,8 @@ static int git_blame_config(const char *var, const char
> *value, void *cb)
> parse_date_format(value, &blame_date_mode);
> return 0;
> }
> + if (!strcmp(var, "blame.ignorerevsfile"))
> + return git_config_pathname(&ignore_revs_file, var, value);
... this...
> if (!strcmp(var, "color.blame.repeatedlines")) {
> if (color_parse_mem(value, strlen(value), repeated_meta_color))
> warning(_("invalid color '%s' in
> color.blame.repeatedLines"),
> @@ -806,6 +826,8 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix)
> OPT_BIT('s', NULL, &output_option, N_("Suppress author name and
> timestamp (Default: off)"), OUTPUT_NO_AUTHOR),
> OPT_BIT('e', "show-email", &output_option, N_("Show author
> email instead of name (Default: off)"), OUTPUT_SHOW_EMAIL),
> 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(0, "ignore-revs-file", &ignore_revs_file,
> N_("file"), N_("Ignore revisions from <file>")),
... and this change limit the user to specifying a single file, for no
good reason. Worse: specifying two different files via two
`--ignore-revs-file` parameters will only heed the latter and skip the
former without any warning.
A better idea IMHO would be to use an OPT_STRING_LIST() for
`--ignore-revs-file`, too, and to allow for multiple
`blame.ignoreRevsFile` config entries (with our usual trick of handling an
empty setting by resetting the list of paths that were accumulated so
far, see e.g. how `credential.helper` is handled).
Ciao,
Johannes
> 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),
>
> @@ -995,6 +1017,8 @@ int cmd_blame(int argc, const char **argv, const char
> *prefix)
> sb.contents_from = contents_from;
> sb.reverse = reverse;
> sb.repo = the_repository;
> + build_ignorelist(&sb, &ignore_rev_list);
> + string_list_clear(&ignore_rev_list, 0);
> setup_scoreboard(&sb, path, &o);
> lno = sb.num_lines;
>
> --
> 2.20.1.321.g9e740568ce-goog
>
>
