Re: [PATCH] Clarify text filter merge conflict reduction docs

Thanks for the detailed review and rewrite!  I unexpectedly had to spend the evening working on non-git related stuff, so I haven't even had time to send my promised re-roll.

On 30. juni 2010, at 19.46, Junio C Hamano wrote:

Eyvind Bernhardsen [off-list ref] writes:

quoted

Shouldn't the normalization in merge-recursive be conditional too?

True, but your patch to merge-recursive is broken, I think.  It should at
least look like the attached rewrite.

Your patch is better than mine, but in my defense I think you misdiagnosed the big problems.  The error path in normalized_eq returned 0 in case of problems, making the caller assume that the files differ and generate a merge conflict, and the return code from normalize_buffer was used correctly (see below).

[...]

If you had a test that made sure that the merge works for paths that do
not need double-conversion, you might have caught the last issue.  I
suspect that your new tests _only_ checked what happens to paths that
actually triggers these double conversion, without making sure that the
new code would not affect the cases where it shouldn't be involved, no?

The real tests I ran were a couple of huge merges, admittedly across a "text=auto" change, but most paths were not touched by either branch, so I would definitely have hit that issue.

It is a common trap to fall into not testing the negative case, when you
are working on your own shiny new toy.  Let's be more careful when writing
new tests.

Yes, absolutely.  The tests are pretty threadbare for such a potentially dangerous change, so I'll cop to a "shiny new toy" charge.  I'll also cop to writing hard-to-read code, but I still think it worked :)

Even though I like your version better than mine, I have a few comments, only some of which are defensive:

+	if (!core_ll_merge_double_convert)
+		return sha_eq(o_sha, a_sha);

I would rather do this:

	if (sha_eq(o_sha, a_sha))
		return 1;
	if (!core_ll_merge_double_convert)
		return 0;

If the sha1s are equal before normalization the files will be equal after normalization, so there's no sense in going through the rigmarole.

Bikeshed colour, I know, but core_ll_merge_double_convert is unwieldy and also a bit inaccurate since it's not just for ll_merge.  How about core_merge_prefilter, with a corresponding change to the config setting?  (I had this change as part of my unsent re-roll).

+	ret = 0; /* assume changed for safety */
+	assert(o_sha && a_sha);
+	if (read_sha1_strbuf(o_sha, &o) || read_sha1_strbuf(a_sha, &a))
+		goto error_return;
+	renormalize_buffer(path, o.buf, o.len, &o);
+	renormalize_buffer(path, a.buf, o.len, &a);
+	ret = (o.len == a.len && !memcmp(o.buf, a.buf, o.len));

This was one of your points, but I deliberately skipped the memcmp here if _neither_ of the renormalize_buffers did any work (binary "|" instead of boolean "||" to ensure both sides are evaluated, but the comment was perhaps a little obscure).

If the files had been equal without any normalization the sha_eq() would have caught it, so we know the files are different without having to compare them.

+error_return:
+	strbuf_release(&o);
+	strbuf_release(&a);
+	return ret;

I'm not a goto objector, just curious: what's the advantage of "goto error_return" here vs. having the renormalizing code inside the if and inverting the test?

Thanks again for taking the time to improve my patch.
-- 
Eyvind