Re: [BUG] diff algorithm selection issue

[Junio C Hamano <gitster@pobox.com>]

ydirson@free.fr writes:

> When the config has diff.algorithm=patience set, "git diff --minimal" seems to
> be ignored, and does not give the same output as "git diff --diff-algorithm=minimal",
> but really the same as "git diff --diff-algorithm=patience".

Thanks for reporting.  The document on --diff-algorithm does make it
sound as if the --diff-algorithm=minimal option must operate as if
myers algorithm is used with the minumalization tweak, but that is
wrong from the point of view of the intent of the "minimal" option,
which was meant to be a secondary option that tweaks the base
algorithm (be it myers or patience or any other new algorithm we
might introduce in the future) by allowing it to spend more cycles
to come up with a smaller diff.

At least, it is what the "--minimal" option (not the value "minimal"
given to the "--diff-algorithm=<algo>" option), and the underlying
mechanism that supports the option, meant to do.

But the way the option is surfaced to the end-user facing UI (and
the documentation) with "--diff-algorithm=minimal", it does look
like 

	git -c diff.algorithm=patience cmd --diff-algorithm=minimal
	git -c diff.algorithm=patience cmd --diff-algorithm=myers --minimal

ought to be equivalent.

Also, I suspect that

	git -c diff.algorithm=patience cmd --diff-algorithm=myers

does not do what we expect, either.

I have not convinced myself that the attached is the best way to fix
the issue, but anyway, somebody seems to be OR'ing in diff_algorithm
to xdl_opts field after we see --diff-algorithm=minimal and replaced
XDF_DIFF_ALGORITHM_MASK bits in xdl_opts field in this function, so
the attached patch may defeat that code---the real bug is probably in
that code, but I haven't figured out where it is X-<.

 diff.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/diff.c b/diff.c
index 863da896c0..a6dba45cd6 100644
--- a/diff.c
+++ b/diff.c
@@ -5026,6 +5026,10 @@ static int diff_opt_diff_algorithm(const struct option *opt,
 		return error(_("option diff-algorithm accepts \"myers\", "
 			       "\"minimal\", \"patience\" and \"histogram\""));
 
+	/* we shouldn't have to do this... */
+	if (!(value & XDF_DIFF_ALGORITHM_MASK))
+		diff_algorithm &= ~XDF_DIFF_ALGORITHM_MASK;
+
 	/* clear out previous settings */
 	DIFF_XDL_CLR(options, NEED_MINIMAL);
 	options->xdl_opts &= ~XDF_DIFF_ALGORITHM_MASK;