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

Hi Junio,

On 28/10/2025 16:22, Junio C Hamano wrote:

quoted

     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.

quoted

  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.

quoted

  		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