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 <eyvind.bernhardsen@xxxxxxxxx> 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). [...] > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html