{"thread":{"id":"24079","subject":"[RFC] ll-merge: Normalize files before merging","startedAt":"2010-06-10T20:48:14Z","lastAt":"2010-06-11T20:56:08Z","messageCount":8,"participants":["Eyvind Bernhardsen","Johannes Sixt","Finn Arne Gangstad","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"143477","messageId":"1276202894-11805-1-git-send-email-eyvind.bernhardsen@gmail.com","threadId":"24079","inReplyTo":null,"subject":"[RFC] ll-merge: Normalize files before merging","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-10T20:48:14Z","receivedAt":"2010-06-10T20:48:14Z","isPatch":false,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"Currently, merging across changes in line ending normalization is\npainful since all lines containing CRLF will conflict uselessly.\n\nFix ll-merge so that the \"base\", \"theirs\" and \"ours\" files are passed\nthrough convert_to_git() before a three-way merge.  This prevents\ndifferences that can be normalized away from blocking an automatic\nmerge.\n---\nI have a repository that has been normalized using \"* text=auto\" and\nneed to merge un-normalized changes (ie, branches based on commits from\nbefore the normalization) into it.  Unfortunately, this doesn't work:\nevery line containing a CRLF causes a conflict.  Fortunately, there is\nan easy fix in the merge code.\n\nThis patch has already been useful to me, but I'm not sure it is the\nbest possible solution to the problem (especially in terms of\nefficiency), hence the RFC.\n\nNote that clean and ident filters will also be run, which might be a\ngood thing.  Also, the tests require my crlf/text series from pu.\n-- \nEyvind\n\n\n\n ll-merge.c                 |   12 +++++++++\n t/t6038-merge-text-auto.sh |   54 ++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 66 insertions(+), 0 deletions(-)\n create mode 100755 t/t6038-merge-text-auto.sh\n\ndiff --git a/ll-merge.c b/ll-merge.c\nindex f9b3d85..63835af 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -321,6 +321,15 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]\n \treturn git_checkattr(path, 2, check);\n }\n \n+static void normalize_file(mmfile_t *mm, const char *path) {\n+\tstruct strbuf strbuf = STRBUF_INIT;\n+\tif (convert_to_git(path, mm->ptr, mm->size, &strbuf, 0)) {\n+\t\tfree(mm->ptr);\n+\t\tmm->size = strbuf.len;\n+\t\tmm->ptr = strbuf_detach(&strbuf, NULL);\n+\t}\n+}\n+\n int ll_merge(mmbuffer_t *result_buf,\n \t     const char *path,\n \t     mmfile_t *ancestor, const char *ancestor_label,\n@@ -334,6 +343,9 @@ int ll_merge(mmbuffer_t *result_buf,\n \tconst struct ll_merge_driver *driver;\n \tint virtual_ancestor = flag & 01;\n \n+\tnormalize_file(ancestor, path);\n+\tnormalize_file(ours, path);\n+\tnormalize_file(theirs, path);\n \tif (!git_path_check_merge(path, check)) {\n \t\tll_driver_name = check[0].value;\n \t\tif (check[1].value) {\ndiff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh\nnew file mode 100755\nindex 0000000..6af2c41\n--- /dev/null\n+++ b/t/t6038-merge-text-auto.sh\n@@ -0,0 +1,54 @@\n+#!/bin/sh\n+\n+test_description='CRLF merge conflict across text=auto change'\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\tgit config core.autocrlf false &&\n+\techo first line | append_cr >file &&\n+\tgit add file &&\n+\tgit commit -m \"Initial\" &&\n+\tgit tag initial &&\n+\tgit branch side &&\n+\techo \"* text=auto\" >.gitattributes &&\n+\tgit add .gitattributes &&\n+\techo same line | append_cr >>file &&\n+\tgit add file &&\n+\tgit commit -m \"add line from a\" &&\n+\tgit tag a &&\n+\tgit rm .gitattributes &&\n+\trm file &&\n+\tgit checkout file &&\n+\tgit commit -m \"remove .gitattributes\" &&\n+\tgit tag c &&\n+\tgit checkout side &&\n+\techo same line | append_cr >>file &&\n+\tgit commit -m \"add line from b\" file &&\n+\tgit tag b &&\n+\tgit checkout master\n+'\n+\n+test_expect_success 'Check merging after setting text=auto' '\n+\tgit reset --hard a &&\n+\tgit merge b &&\n+\tcat file | remove_cr >file.temp &&\n+\ttest_cmp file file.temp\n+'\n+\n+test_expect_success 'Check merging addition of text=auto' '\n+\tgit reset --hard b &&\n+\tgit merge a &&\n+\tcat file | remove_cr >file.temp &&\n+\ttest_cmp file file.temp\n+'\n+\n+# Not sure if this deserves to be fixed\n+test_expect_failure 'Check merging removal of text=auto' '\n+\tgit reset --hard b &&\n+\tgit merge c &&\n+\tcat file | remove_cr >file.temp &&\n+\ttest_cmp file file.temp\n+'\n+\n+test_done\n-- \n1.7.1.5.g0ed10.dirty\n"},{"id":"143486","messageId":"4C11CE75.7080706@viscovery.net","threadId":"24079","inReplyTo":"1276202894-11805-1-git-send-email-eyvind.bernhardsen@gmail.com","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-06-11T05:49:41Z","receivedAt":"2010-06-11T05:49:41Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/10/2010 22:48, schrieb Eyvind Bernhardsen:\n> Currently, merging across changes in line ending normalization is\n> painful since all lines containing CRLF will conflict uselessly.\n> \n> Fix ll-merge so that the \"base\", \"theirs\" and \"ours\" files are passed\n> through convert_to_git() before a three-way merge.  This prevents\n> differences that can be normalized away from blocking an automatic\n> merge.\n\nI think you are going overboard here. Normalization should only happen\nonly for data that moves from the worktree to the database. But during a\nmerge, at most one part can come from the worktree, methinks; you are\nnormalizing all three of them, though.\n\n> This patch has already been useful to me, but I'm not sure it is the\n> best possible solution to the problem (especially in terms of\n> efficiency), hence the RFC.\n> \n> Note that clean and ident filters will also be run, which might be a\n> good thing.  Also, the tests require my crlf/text series from pu.\n> --\n> Eyvind\n\nPlease do not put a dash-dash-blank line before the patch; Thunderbird\ntakes it as the beginning of the signature and truncates the message in\nthe reply.\n\n-- Hannes\n"},{"id":"143490","messageId":"4C11E717.4070508@gmail.com","threadId":"24079","inReplyTo":"4C11CE75.7080706@viscovery.net","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-11T07:34:47Z","receivedAt":"2010-06-11T07:34:47Z","isPatch":false,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 11. juni 2010 07:49, Johannes Sixt wrote:\n> Am 6/10/2010 22:48, schrieb Eyvind Bernhardsen:\n>> Currently, merging across changes in line ending normalization is\n>> painful since all lines containing CRLF will conflict uselessly.\n>>\n>> Fix ll-merge so that the \"base\", \"theirs\" and \"ours\" files are passed\n>> through convert_to_git() before a three-way merge.  This prevents\n>> differences that can be normalized away from blocking an automatic\n>> merge.\n>\n> I think you are going overboard here. Normalization should only happen\n> only for data that moves from the worktree to the database. But during a\n> merge, at most one part can come from the worktree, methinks; you are\n> normalizing all three of them, though.\n\nWell, that's sort of the point.  All three are normalized to (hopefully) \nminimize the differences between them, increasing the chance of a \nsuccessful merge.\n\nThe problem I'm trying to solve is that \"base\" and \"theirs\" were not \nnormalized when the files were added to the database, and this causes \nunnecessary conflicts with \"ours\".  Normalizing \"ours\" allows merging \nthe other way to work, too.\n\nIt's a brute-force method, and there may be a smarter way, but it works \nfor me.\n\n[...]\n\n> Please do not put a dash-dash-blank line before the patch; Thunderbird\n> takes it as the beginning of the signature and truncates the message in\n> the reply.\n\nOops, sorry!\n-- \nEyvind\n"},{"id":"143491","messageId":"4C11EB0D.20208@viscovery.net","threadId":"24079","inReplyTo":"4C11E717.4070508@gmail.com","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-06-11T07:51:41Z","receivedAt":"2010-06-11T07:51:41Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 6/11/2010 9:34, schrieb Eyvind Bernhardsen:\n> On 11. juni 2010 07:49, Johannes Sixt wrote:\n>> I think you are going overboard here. Normalization should only happen\n>> only for data that moves from the worktree to the database. But during a\n>> merge, at most one part can come from the worktree, methinks; you are\n>> normalizing all three of them, though.\n> \n> Well, that's sort of the point.  All three are normalized to (hopefully)\n> minimize the differences between them, increasing the chance of a\n> successful merge.\n\nI know what your point is. It is still inappropriate to call\nnormalize_file() on data that comes from the repository. It is not the\ntask of a merge procedure to blindly normalize data.\n\n-- Hannes\n"},{"id":"143492","messageId":"20100611083641.GB31109@pvv.org","threadId":"24079","inReplyTo":"4C11EB0D.20208@viscovery.net","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2010-06-11T08:36:41Z","receivedAt":"2010-06-11T08:36:41Z","isPatch":false,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Fri, Jun 11, 2010 at 09:51:41AM +0200, Johannes Sixt wrote:\n> Am 6/11/2010 9:34, schrieb Eyvind Bernhardsen:\n> > On 11. juni 2010 07:49, Johannes Sixt wrote:\n> >> I think you are going overboard here. Normalization should only happen\n> >> only for data that moves from the worktree to the database. But during a\n> >> merge, at most one part can come from the worktree, methinks; you are\n> >> normalizing all three of them, though.\n> > \n> > Well, that's sort of the point.  All three are normalized to (hopefully)\n> > minimize the differences between them, increasing the chance of a\n> > successful merge.\n> \n> I know what your point is. It is still inappropriate to call\n> normalize_file() on data that comes from the repository. It is not the\n> task of a merge procedure to blindly normalize data.\n\nI don't think this argument holds. If git doesn't call\nconvert_to_git() automatically and you get a merge conflict, git WILL\ncall convert_to_git() on the result anyway when you add it, so it\ncan't be that horrible that this happens automatically for you?\n\nIf you add something to .gitattributes that causes the repository\nrepresentation of a file to change, and then try to merge an older\nbranch, isn't it more helpful if it works than if it fails miserably?\n\nIt would probably be more correct to do\n\nconert_to_git(convert_to_work_tree(x)) for the parts taken from base\nand theirs, in theory I guess that this can be true:\n\nconvert_to_git(x) != convert_to_git(convert_to_work_tree(x))\n\n- Finn Arne\n"},{"id":"143493","messageId":"4C11F830.3050404@gmail.com","threadId":"24079","inReplyTo":"20100611083641.GB31109@pvv.org","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-11T08:47:44Z","receivedAt":"2010-06-11T08:47:44Z","isPatch":false,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 11. juni 2010 10:36, Finn Arne Gangstad wrote:\n\n> It would probably be more correct to do\n>\n> conert_to_git(convert_to_work_tree(x)) for the parts taken from base\n> and theirs, in theory I guess that this can be true:\n>\n> convert_to_git(x) != convert_to_git(convert_to_work_tree(x))\n\nInteresting idea, might be a bit slow perhaps?  I'll do some \nbenchmarketing.  Incidentally, it has to convert_to_work_tree() \"ours\" \nas well, since the merge code doesn't read anything from the worktree.\n\nI think convert_to_git(convert_to_work_tree(x)) is actually less likely \nto cause trouble than convert_to_git(convert_to_git(x)), which is what \nblind normalization would end up doing for at least one side of the merge.\n\n- Eyvind\n"},{"id":"143528","messageId":"7vd3vxicaw.fsf@alter.siamese.dyndns.org","threadId":"24079","inReplyTo":"4C11EB0D.20208@viscovery.net","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-11T19:44:55Z","receivedAt":"2010-06-11T19:44:55Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 6/11/2010 9:34, schrieb Eyvind Bernhardsen:\n>> On 11. juni 2010 07:49, Johannes Sixt wrote:\n>>> I think you are going overboard here. Normalization should only happen\n>>> only for data that moves from the worktree to the database. But during a\n>>> merge, at most one part can come from the worktree, methinks; you are\n>>> normalizing all three of them, though.\n>> \n>> Well, that's sort of the point.  All three are normalized to (hopefully)\n>> minimize the differences between them, increasing the chance of a\n>> successful merge.\n>\n> I know what your point is. It is still inappropriate to call\n> normalize_file() on data that comes from the repository. It is not the\n> task of a merge procedure to blindly normalize data.\n\nIt is not \"blindly\", but \"running normalization one _extra time_, as the\nrepository data is supposed to be canonical already\", which is utterly\nwrong.\n"},{"id":"143531","messageId":"FD073505-FFF4-40D7-B841-0EC1B902E32E@gmail.com","threadId":"24079","inReplyTo":"7vd3vxicaw.fsf@alter.siamese.dyndns.org","subject":"Re: [RFC] ll-merge: Normalize files before merging","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-11T20:56:08Z","receivedAt":"2010-06-11T20:56:08Z","isPatch":false,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 11. juni 2010, at 21.44, Junio C Hamano wrote:\n\n> Johannes Sixt <j.sixt@viscovery.net> writes:\n> \n>> I know what your point is. It is still inappropriate to call\n>> normalize_file() on data that comes from the repository. It is not the\n>> task of a merge procedure to blindly normalize data.\n> \n> It is not \"blindly\", but \"running normalization one _extra time_, as the\n> repository data is supposed to be canonical already\", which is utterly\n> wrong.\n\nI agree that double normalization is evil, but the repository data isn't necessarily canonical if the configuration has changed since the data was added.\n\nHow do you feel about Finn Arne's idea of first convert_to_work_tree()-ing the data, then convert_to_git()ing it back?  That gets rid of the double normalization at the cost of some performance and memory usage (especially with CRLF output enabled).  I'm going to do some benchmarks to go along with my next stab at this.\n\n- Eyvind\n"}]}