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

From: Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>
Date: Jun 30, 2010, 21:32 UTC
Message-ID: <0CF2361E-D010-4042-B481-6918BE2A9341@gmail.com>
In-Reply-To: <7v4ogk76sb.fsf@alter.siamese.dyndns.org>

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:

Show 6 quoted lines

> Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:
> 
>> 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).

[...]

Show 5 quoted lines

> 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).

Show 7 quoted lines

> +	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