Re: [PATCH v2] blame: make diff algorithm configurable

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 28, 2025, 15:22 UTC
Message-ID: <xmqqjz0fdpa3.fsf@gitster.g>
In-Reply-To: <pull.2075.v2.git.git.1761658643278.gitgitgadget@gmail.com>

"Antonin Delpeuch via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Antonin Delpeuch <antonin@delpeuch.eu>
>
> The diff algorithm used in 'git-blame(1)' is set to 'myers',
> without the possibility to change it aside from the `--minimal` option.

Hmph. It is very unfortunate that we had --minimal already. We should have done --diff-algorithm=<which> instead, but that is way too late.

Show 5 quoted lines

> There has been long-standing interest in changing the default diff
> algorithm to "histogram", and Git 3.0 was floated as a possible occasion
> for taking some steps towards that:
>
> https://lore.kernel.org/git/xmqqed873vgn.fsf@gitster.g/

Micronit. I think the reference to 3.0 only about potentially breaking backward compatibility by making the family of diff-* plumbing commands ignore diff.algorithm configuration, and other usability changes like this one are fair game without having to wait for 3.0 boundary (the plumbing commands do ignore the configuration already, so there is nothing we have to wait 3.0 before doing).

>     Changes since v1:
>     
>      * add tests
>      * ignore --diff-algorithm when it is provided before --minimal

Sensible.

I presume the reverse is true, i.e. giving "--minimal" and then "--diff-algorithm=histogram" in this order would make "histogram" survive, in other words, the usual "last one wins" rule is applied?

Show 13 quoted lines

> +static int blame_diff_algorithm_minimal(const struct option *option,
> +					const char *arg, int unset)
> +{
> +	int *opt = option->value;
> +
> +	BUG_ON_OPT_NEG(unset);
> +	BUG_ON_OPT_ARG(arg);
> +
> +	*opt &= ~XDF_DIFF_ALGORITHM_MASK;
> +	*opt |= XDF_NEED_MINIMAL;
> +
> +	return 0;
> +}

This and diff.c:diff_opt_diff_algorithm_no_arg(), which I think is the original from which this was copied from, look somewhat different, but this can afford to be simpler, as it does not have to parse "--histogram", "--patience", etc., as independent command line options. OK.

Show 17 quoted lines

> +static int blame_diff_algorithm_callback(const struct option *option,
> +					 const char *arg, int unset)
> +{
> +	int *opt = option->value;
> +	long value = parse_algorithm_value(arg);
> +
> +	BUG_ON_OPT_NEG(unset);
> +
> +	if (value < 0)
> +		return error(_("option diff-algorithm accepts \"myers\", "
> +			       "\"minimal\", \"patience\" and \"histogram\""));
> +
> +	*opt &= ~(XDF_NEED_MINIMAL | XDF_DIFF_ALGORITHM_MASK);
> +	*opt |= value;
> +
> +	return 0;
> +}

Quite straight-forward and sensible.

Show 19 quoted lines

>  static int is_a_rev(const char *name)
>  {
>  	struct object_id oid;
> @@ -915,10 +960,17 @@ int cmd_blame(int argc,
>  		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_CALLBACK_F(0, "diff-algorithm", &xdl_opts, N_("<algorithm>"),
> +			       N_("choose a diff algorithm"),
> +			       PARSE_OPT_NONEG, blame_diff_algorithm_callback),
>  		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_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_CALLBACK_F(0, "minimal", &xdl_opts, NULL,
> +			       N_("spend extra cycles to find better match"),
> +			       PARSE_OPT_NONEG | PARSE_OPT_NOARG,
> +			       blame_diff_algorithm_minimal),
>  		OPT_BIT(0, "minimal", &xdl_opts, N_("spend extra cycles to find better match"), XDF_NEED_MINIMAL),

This OPT_BIT() can stay here? I thought parse_options_check() was capable of detecting duplicated long-form commands as programming error, but apparently it does not. (#leftoverbits) We should look into teaching parse_options_check() to check duplicated option names.

Show 22 quoted lines

>  		OPT_STRING('S', NULL, &revs_file, N_("file"), N_("use revisions from <file> instead of calling git-rev-list")),
>  		OPT_STRING(0, "contents", &contents_from, N_("file"), N_("use <file>'s contents as the final image")),
> ...
> +test_expect_success 'blame honors --minimal option' '
> +	cat >expected <<-\EOF &&
> +	Initial
> +	Initial
> +	Initial
> +	Second
> +	Second
> +	Second
> +	Second
> +	Initial
> +	Second
> +	Second
> +	Second
> +	EOF
> +
> +	git blame file.txt --minimal | \
> +		grep --only-matching -e Initial -e Second > actual &&
> +	test_cmp expected actual
> +'

Do we need to test combination of configuration variables and command line options (to verify that options trump configuration), or two command line options (to verify that the last one wins)?

When xdiff/ part of the system gets improved, the above expected patterns may have to change, these tests may fail. Whoever updates the diff algorithm to cause such a failure has to tell between a genuine _bug_ in their update to diff implementation and the test expecting a suboptimal result based on the behaviour of the diff algorithm before their improvement. And for that, they need to debug these tests. But I suspect that these tests will probably be very difficult to debug, as it is almost impossible to see which line in the original each of these lines correspond to.

I guess that's inevitable, and we'll cross that bridge when it becomes necessary.

Thanks, will queue, but I do find the leftover --minimal bit disturbing.