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

Eyvind Bernhardsen [off-list ref] 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.

Key points:

 - Your "normalized_eq()" is called only from the codepath that wants to
   know if "one is deleted and the other is unchanged" to implement the
   latter half of that check.  Let's name that function blob_unchanged().
   Also let's move the "how to compare two blobs when we are not doing
   double conversion" (i.e. sha_eq()) logic from its caller to it.  These
   changes would make it a lot easier to read the caller.

 - Your read_sha1_strbuf() took "path" as input but didn't do anything
   with it.  Let's drop it.

 - It didn't diagnose any errors.  Let's make sure that it follows the
   usual "0 for success, negative for error" convention and make sure its
   only caller knows about it.

 - "blob_unchanged()" (aka "normalized_eq()") is called only when we have
   two blobs to compare.  It is a programming error to pass NULL in either
   o_sha or a_sha; let's assert() it.

 - Your error path in the function made the caller assume that two input
   matched and it is Ok to continue; it should instead force the caller to
   stop so that the end user can notice.

 - Return values from functions in convert.c are NOT error signals.  A
   true value is to let the caller know that the callee had to convert
   (and a false value is that the callee didn't convert), and the next
   transformation needs to be done on the result in the strbuf (as opposed
   to the src buffer that was left inact).

   In your use of renormalize_buffer(), your input "src" points at the
   same strbuf as your output "dst", so your caller does not care if
   renormalize didn't have to do anything.  Either way, you would pick the
   result up from the strbuf.

   But using the return value to skip the comparison as if the call failed
   is _wrong_.  Even if there is no need for conversion for the given path
   (i.e. 0 return from convert.c functions), you would need to pick up the
   result and perform the comparison.

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?

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.

Thanks.

 merge-recursive.c          |   45 ++++++++++++++++++++++++++++++++++++++++++-
 t/t6038-merge-text-auto.sh |    2 +-
 2 files changed, 44 insertions(+), 3 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 206c103..3c63e5e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c

@@ -1056,6 +1056,47 @@ static unsigned char *stage_sha(const unsigned char *sha, unsigned mode)
 	return (is_null_sha1(sha) || mode == 0) ? NULL: (unsigned char *)sha;
 }
 
+static int read_sha1_strbuf(const unsigned char *sha1, struct strbuf *dst)
+{
+	void *buf;
+	enum object_type type;
+	unsigned long size;
+	buf = read_sha1_file(sha1, &type, &size);
+	if (!buf)
+		return error("cannot read object %s", sha1_to_hex(sha1));
+	if (type != OBJ_BLOB) {
+		free(buf);
+		return error("object %s is not a blob", sha1_to_hex(sha1));
+	}
+	strbuf_attach(dst, buf, size, size + 1);
+	return 0;
+}
+
+static int blob_unchanged(const unsigned char *o_sha,
+			  const unsigned char *a_sha,
+			  const char *path)
+{
+	struct strbuf o = STRBUF_INIT;
+	struct strbuf a = STRBUF_INIT;
+	int ret;
+
+	if (!core_ll_merge_double_convert)
+		return sha_eq(o_sha, a_sha);
+
+	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));
+
+error_return:
+	strbuf_release(&o);
+	strbuf_release(&a);
+	return ret;
+}
+
 /* Per entry merge function */
 static int process_entry(struct merge_options *o,
 			 const char *path, struct stage_data *entry)

@@ -1075,8 +1116,8 @@ static int process_entry(struct merge_options *o,
 	if (o_sha && (!a_sha || !b_sha)) {
 		/* Case A: Deleted in one */
 		if ((!a_sha && !b_sha) ||
-		    (sha_eq(a_sha, o_sha) && !b_sha) ||
-		    (!a_sha && sha_eq(b_sha, o_sha))) {
+		    (!b_sha && blob_unchanged(o_sha, a_sha, path)) ||
+		    (!a_sha && blob_unchanged(o_sha, b_sha, path))) {
 			/* Deleted in both or deleted in one and
 			 * unchanged in the other */
 			if (a_sha)

diff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh
index 2caebb9..4a7bc48 100755
--- a/t/t6038-merge-text-auto.sh
+++ b/t/t6038-merge-text-auto.sh

@@ -46,7 +46,7 @@ test_expect_success 'Check merging addition of text=auto' '
 	test_cmp file file.temp
 '
 
-test_expect_failure 'Test delete/normalize conflict' '
+test_expect_success 'Test delete/normalize conflict' '
 	git checkout side &&
 	git reset --hard initial &&
 	git rm file &&

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