Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes
From: Johannes Schindelin <hidden>
Date: 2019-01-18 09:48:18
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:
quoted hunk ↗ jump to hunk
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...
quoted hunk ↗ jump to hunk
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...
quoted hunk ↗ jump to hunk
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
quoted hunk ↗ jump to hunk
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