Re: [PATCH v2 2/3] blame: add the ability to ignore commits and their changes

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jan 18, 2019, 09:47 UTC
Message-ID: <nycvar.QRO.7.76.6.1901181038540.41@tvgsbejvaqbjf.bet>
In-Reply-To: <20190117202919.157326-3-brho@google.com>

Hi Barret,

On Thu, 17 Jan 2019, Barret Rhoden wrote:

Show 6 quoted lines

> [...]
> 
> 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:

Show 9 quoted lines

> 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...

Show 9 quoted lines

>  
>  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...

Show 9 quoted lines

>  	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

Show 16 quoted lines

>  		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
> 
>