From: Antonin Delpeuch <antonin@delpeuch.eu>
Date: Tue, 28 Oct 2025 16:00:40 GMT
Subject: Re: [PATCH v2] blame: make diff algorithm configurable
Message-ID: <362c7dc4-c35f-440a-ae20-e1d06e183fce@delpeuch.eu>
In-Reply-To: <xmqqjz0fdpa3.fsf@gitster.g>

Hi Junio,

On 28/10/2025 16:22, Junio C Hamano wrote:
>>      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?
Yes, that was already the case in the version 1 of this patch. Philipp 
noticed that it was only true in one direction, so this fixes it.
>>   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.

Oops, that's an oversight on my part indeed. I'll fix it.

>
>>   		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)?
It's easy enough to add such tests, I can add a few more.
>
> 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'm happy to make the tests a bit clearer by including the lines in the 
expected output.

I'll submit a new version with those changes.

Best,

Antonin