From: Phillip Wood Date: Mon, 03 Nov 2025 14:32:39 GMT Subject: Re: [PATCH v4 2/2] blame: make diff algorithm configurable Message-ID: In-Reply-To: <920a6f3acbc86e72c6ea236f8dbd3d559398409a.1762034252.git.gitgitgadget@gmail.com> Hi Antonin Thanks for re-rolling, this is looking pretty sound, I've left a couple of fairly minor comments below. On 01/11/2025 21:57, Antonin Delpeuch via GitGitGadget wrote: > +static int blame_diff_algorithm_minimal(const struct option *option, > + const char *arg, int unset) > +{ > + int *opt = option->value; > + > + BUG_ON_OPT_ARG(arg); > + > + *opt &= ~XDF_DIFF_ALGORITHM_MASK; > + if (!unset) > + *opt |= XDF_NEED_MINIMAL; One thing I'd not thought about before was the interaction between "--no-minimal" and "--diff-algorithm" The code above makes "--no-minimal" behave like "diff-algorithm=myers" which is consistent with the current behavior where the only options for the diff algorithm are "minimal" or "myers". An alternative would be for "--no-minimal" to just clear XDF_NEED_MINIMAL and behave like a no-op if it is given after "--diff-algorithm=patience" or "--diff-algorithm=histogram". I don't really have a strong preference either way. > +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); We can just use XDF_DIFF_ALGORITHM_MASK now that we've added XDF_NEED_MINMAL to it in the last commit. > @@ -915,11 +960,16 @@ 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_(""), > + 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 when blaming")), > OPT_STRING_LIST(0, "ignore-revs-file", &ignore_revs_file_list, N_("file"), N_("ignore revisions from ")), > 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_BIT(0, "minimal", &xdl_opts, N_("spend extra cycles to find better match"), XDF_NEED_MINIMAL), > + OPT_CALLBACK_F(0, "minimal", &xdl_opts, NULL, > + N_("spend extra cycles to find a better match"), > + PARSE_OPT_NOARG, blame_diff_algorithm_minimal), Given the potential for confusing interactions between "--no-minimal" and "--diff-algorithm" I think it would be worth adding OPT_HIDDEN here. > diff --git a/t/t8015-blame-diff-algorithm.sh b/t/t8015-blame-diff-algorithm.sh > new file mode 100755 > index 0000000000..5318e18cb3 > --- /dev/null > +++ b/t/t8015-blame-diff-algorithm.sh > @@ -0,0 +1,203 @@ > + [...] > + git blame file.c > output && > + sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" output > without_varying_parts && > + sed -e "s/ *$//g" without_varying_parts > actual && This would be more efficient if it was written as sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" \ -e "s/ *$//g" output >actual Our test suite is really slow on windows so it is worth trying to avoid creating unnecessary processes. Thanks Phillip > + test_cmp expected actual > +' > + > +test_expect_success 'blame honors --diff-algorithm option' ' > + cat >expected <<-\EOF && > + Commit_1 int g(size_t u) > + Commit_1 { > + Commit_1 while (u < 30) > + Commit_1 { > + Commit_1 u++; > + Commit_1 } > + Commit_1 return u; > + Commit_1 } > + Commit_2 > + Commit_2 int h(int x, int y, int z) > + Commit_2 { > + Commit_2 if (z == 0) > + Commit_2 { > + Commit_2 return x; > + Commit_2 } > + Commit_2 return y; > + Commit_2 } > + EOF > + > + git blame file.c --diff-algorithm histogram > output && > + sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" output > without_varying_parts && > + sed -e "s/ *$//g" without_varying_parts > actual && > + test_cmp expected actual > +' > + > +test_expect_success 'blame honors diff.algorithm config variable' ' > + cat >expected <<-\EOF && > + Commit_1 int g(size_t u) > + Commit_1 { > + Commit_1 while (u < 30) > + Commit_1 { > + Commit_1 u++; > + Commit_1 } > + Commit_1 return u; > + Commit_1 } > + Commit_2 > + Commit_2 int h(int x, int y, int z) > + Commit_2 { > + Commit_2 if (z == 0) > + Commit_2 { > + Commit_2 return x; > + Commit_2 } > + Commit_2 return y; > + Commit_2 } > + EOF > + > + git -c diff.algorithm=histogram blame file.c > output && > + sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" output > without_varying_parts && > + sed -e "s/ *$//g" without_varying_parts > actual && > + test_cmp expected actual > +' > + > +test_expect_success 'blame gives priority to --diff-algorithm over diff.algorithm' ' > + cat >expected <<-\EOF && > + Commit_1 int g(size_t u) > + Commit_1 { > + Commit_1 while (u < 30) > + Commit_1 { > + Commit_1 u++; > + Commit_1 } > + Commit_1 return u; > + Commit_1 } > + Commit_2 > + Commit_2 int h(int x, int y, int z) > + Commit_2 { > + Commit_2 if (z == 0) > + Commit_2 { > + Commit_2 return x; > + Commit_2 } > + Commit_2 return y; > + Commit_2 } > + EOF > + > + git -c diff.algorithm=myers blame file.c --diff-algorithm histogram && > + sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" output > without_varying_parts && > + sed -e "s/ *$//g" without_varying_parts > actual && > + test_cmp expected actual > +' > + > +test_expect_success 'blame honors --minimal option' ' > + cat >expected <<-\EOF && > + Commit_1 x > + Commit_1 x > + Commit_1 x > + Commit_2 A > + Commit_2 B > + Commit_2 C > + Commit_2 D > + Commit_1 x > + Commit_2 E > + Commit_2 F > + Commit_2 G > + EOF > + > + git blame file.txt --minimal > output && > + sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" output > actual && > + test_cmp expected actual > +' > + > +test_expect_success 'blame respects the order of diff options' ' > + cat >expected <<-\EOF && > + Commit_1 x > + Commit_1 x > + Commit_1 x > + Commit_2 A > + Commit_2 B > + Commit_2 C > + Commit_2 D > + Commit_2 x > + Commit_2 E > + Commit_2 F > + Commit_2 G > + EOF > + > + git blame file.txt --minimal --diff-algorithm myers > output && > + sed -e "s/^[^ ]* (\([^ ]*\) [^)]*)/\1/g" output > actual && > + test_cmp expected actual > +' > + > +test_done