{"thread":{"id":"24211","subject":"[PATCH v4 0/3] CRLF merge conflict reduction, take 4","startedAt":"2010-06-27T19:43:04Z","lastAt":"2010-07-01T03:33:08Z","messageCount":14,"participants":["Eyvind Bernhardsen","Finn Arne Gangstad","Junio C Hamano"],"isPatch":true,"patchVersion":4,"patchTotal":3},"messages":[{"id":"144358","messageId":"cover.1277667177.git.eyvind.bernhardsen@gmail.com","threadId":"24211","inReplyTo":null,"subject":"[PATCH v4 0/3] CRLF merge conflict reduction, take 4","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-27T19:43:04Z","receivedAt":"2010-06-27T19:43:04Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"Only a few changes since the last round:\n\n- added some paragraphs to the filter documentation in gitattributes to\n  explain why having normalizing filters is a good thing (comments\n  welcome)\n\n- fixed the problem Johannes spotted in the eol=crlf optimization by\n  expanding CRLFs (ie not optimizing) if a smudge filter is configured\n\n- moved the opening brace in my function definitions to the next line\n\nThanks for your input.  I'm pretty happy with this now.\n-- \nEyvind\n\nEyvind Bernhardsen (3):\n  Avoid conflicts when merging branches with mixed normalization\n  Try normalizing files to avoid delete/modify conflicts when merging\n  Don't expand CRLFs when normalizing text during merge\n\n Documentation/gitattributes.txt |   27 ++++++++++++++++++\n cache.h                         |    1 +\n convert.c                       |   37 +++++++++++++++++++++----\n ll-merge.c                      |   13 +++++++++\n merge-recursive.c               |   44 ++++++++++++++++++++++++++++-\n t/t6038-merge-text-auto.sh      |   58 +++++++++++++++++++++++++++++++++++++++\n 6 files changed, 172 insertions(+), 8 deletions(-)\n create mode 100755 t/t6038-merge-text-auto.sh\n\n-- \n1.7.1.575.g383de\n"},{"id":"144361","messageId":"07a766a7972671b9e39dbca55719d024c30c7a28.1277667177.git.eyvind.bernhardsen@gmail.com","threadId":"24211","inReplyTo":"cover.1277667177.git.eyvind.bernhardsen@gmail.com","subject":"[PATCH v4 1/3] Avoid conflicts when merging branches with mixed normalization","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-27T19:43:05Z","receivedAt":"2010-06-27T19:43:05Z","isPatch":true,"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 files containing CRLF will conflict with normalized files,\neven if the only difference between the two versions is the line\nendings.  Additionally, any \"real\" merge conflicts that exist are\nobscured because every line in the file has a conflict.\n\nAssume you start out with a repo that has a lot of text files with CRLF\nchecked in (A):\n\n      o---C\n     /     \\\n    A---B---D\n\nB: Add \"* text=auto\" to .gitattributes and normalize all files to\n   LF-only\n\nC: Modify some of the text files\n\nD: Try to merge C\n\nYou will get a ridiculous number of LF/CRLF conflicts when trying to\nmerge C into D, since the repository contents for C are \"wrong\" wrt the\nnew .gitattributes file.\n\nFix ll-merge so that the \"base\", \"theirs\" and \"ours\" stages are passed\nthrough convert_to_worktree() and convert_to_git() before a three-way\nmerge.  This ensures that all three stages are normalized in the same\nway, removing from consideration differences that are only due to\nnormalization.\n\nSigned-off-by: Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>\n---\n Documentation/gitattributes.txt |   27 ++++++++++++++++++\n cache.h                         |    1 +\n convert.c                       |   16 +++++++++-\n ll-merge.c                      |   13 +++++++++\n t/t6038-merge-text-auto.sh      |   58 +++++++++++++++++++++++++++++++++++++++\n 5 files changed, 113 insertions(+), 2 deletions(-)\n create mode 100755 t/t6038-merge-text-auto.sh\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 564586b..b110082 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -317,6 +317,18 @@ command is \"cat\").\n \tsmudge = cat\n ------------------------\n \n+For best results, `clean` and `smudge` commands should produce output\n+that is not dependent on the corresponding command having been run.\n+That is, `clean` should produce identical output whether its input has\n+been run through `smudge` or not, and `smudge` should not rely on its\n+input having been run through `clean`.  See the section on merging\n+below for a rationale.\n+\n+The example \"indent\" filter is well-behaved in this regard: it will\n+accept input that is already correctly indented without modifying it.\n+In this case, the lack of a smudge filter means that the clean filter\n+_must_ accept its own output without modifying it.\n+\n \n Interaction between checkin/checkout attributes\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n@@ -331,6 +343,21 @@ In the check-out codepath, the blob content is first converted\n with `text`, and then `ident` and fed to `filter`.\n \n \n+Merging branches with differing checkin/checkout attributes\n+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n+\n+To prevent unnecessary merge conflicts, git runs a virtual check-out\n+and check-in of all three stages of a file when resolving a three-way\n+merge.  This prevents changes caused by check-in conversion from\n+causing spurious merge conflicts when a converted file is merged with\n+an unconverted file.\n+\n+This strategy will break down if a `smudge` filter relies on its input\n+having been processed by the corresponding `clean` filter or vice\n+versa.  Such filters may otherwise work well, but will prevent\n+automatic merging.\n+\n+\n Generating diff text\n ~~~~~~~~~~~~~~~~~~~~\n \ndiff --git a/cache.h b/cache.h\nindex ff4a7c2..5db89f9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1043,6 +1043,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);\n extern int convert_to_git(const char *path, const char *src, size_t len,\n                           struct strbuf *dst, enum safe_crlf checksafe);\n extern int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst);\n+extern int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst);\n \n /* add */\n /*\ndiff --git a/convert.c b/convert.c\nindex e41a31e..0203be8 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -93,7 +93,8 @@ static int is_binary(unsigned long size, struct text_stat *stats)\n \treturn 0;\n }\n \n-static enum eol determine_output_conversion(enum action action) {\n+static enum eol determine_output_conversion(enum action action)\n+{\n \tswitch (action) {\n \tcase CRLF_BINARY:\n \t\treturn EOL_UNSET;\n@@ -693,7 +694,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)\n \treturn !!ATTR_TRUE(value);\n }\n \n-enum action determine_action(enum action text_attr, enum eol eol_attr) {\n+static enum action determine_action(enum action text_attr, enum eol eol_attr)\n+{\n \tif (text_attr == CRLF_BINARY)\n \t\treturn CRLF_BINARY;\n \tif (eol_attr == EOL_LF)\n@@ -773,3 +775,13 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc\n \t}\n \treturn ret | apply_filter(path, src, len, dst, filter);\n }\n+\n+int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst)\n+{\n+\tint ret = convert_to_working_tree(path, src, len, dst);\n+\tif (ret) {\n+\t\tsrc = dst->buf;\n+\t\tlen = dst->len;\n+\t}\n+\treturn ret | convert_to_git(path, src, len, dst, 0);\n+}\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 3764a1a..28c6f54 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -321,6 +321,16 @@ 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+{\n+\tstruct strbuf strbuf = STRBUF_INIT;\n+\tif (renormalize_buffer(path, mm->ptr, mm->size, &strbuf)) {\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 +344,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..44e6003\n--- /dev/null\n+++ b/t/t6038-merge-text-auto.sh\n@@ -0,0 +1,58 @@\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+\ttouch file &&\n+\tgit add .gitattributes file &&\n+\tgit commit -m \"normalize file\" &&\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+test_expect_failure 'Test delete/normalize conflict' '\n+\tgit checkout side &&\n+\tgit reset --hard initial &&\n+\tgit rm file &&\n+\tgit commit -m \"remove file\" &&\n+\tgit checkout master &&\n+\tgit reset --hard a^ &&\n+\tgit merge side\n+'\n+\n+test_done\n-- \n1.7.1.575.g383de\n"},{"id":"144360","messageId":"0497dd2d68e65e9d2ec1d40f82231557ff95c04c.1277667177.git.eyvind.bernhardsen@gmail.com","threadId":"24211","inReplyTo":"cover.1277667177.git.eyvind.bernhardsen@gmail.com","subject":"[PATCH v4 2/3] Try normalizing files to avoid delete/modify conflicts when merging","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-27T19:43:06Z","receivedAt":"2010-06-27T19:43:06Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"If a file is modified due to normalization on one branch, and deleted on\nanother, a merge of the two branches will result in a delete/modify\nconflict for that file even if it is otherwise unchanged.\n\nTry to avoid the conflict by normalizing and comparing the \"base\" file\nand the modified file when their sha1s differ.  If they compare equal,\nthe file is considered unmodified and is deleted.\n\nSigned-off-by: Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>\n---\n merge-recursive.c          |   44 ++++++++++++++++++++++++++++++++++++++++++--\n t/t6038-merge-text-auto.sh |    2 +-\n 2 files changed, 43 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 206c103..f4f09a2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1056,6 +1056,44 @@ static unsigned char *stage_sha(const unsigned char *sha, unsigned mode)\n \treturn (is_null_sha1(sha) || mode == 0) ? NULL: (unsigned char *)sha;\n }\n \n+static int read_sha1_strbuf(const unsigned char *sha, const char *path,\n+\t\t\t    struct strbuf *dst)\n+{\n+\tvoid *buf;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tbuf = read_sha1_file(sha, &type, &size);\n+\tif (!buf)\n+\t\treturn 0;\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn 0;\n+\t}\n+\tstrbuf_attach(dst, buf, size, size + 1);\n+\treturn 1;\n+}\n+\n+static int normalized_eq(const unsigned char *a_sha,\n+\t\t\t const unsigned char *b_sha,\n+\t\t\t const char *path)\n+{\n+\tstruct strbuf a = STRBUF_INIT;\n+\tstruct strbuf b = STRBUF_INIT;\n+\tint ret = 0;\n+\tif (a_sha && b_sha &&\n+\t    read_sha1_strbuf(a_sha, path, &a) &&\n+\t    read_sha1_strbuf(b_sha, path, &b)) {\n+\t\t/* Both files must be normalized, so we can't use || */\n+\t\tif ((renormalize_buffer(path, a.buf, a.len, &a) |\n+\t\t     renormalize_buffer(path, b.buf, b.len, &b)) &&\n+\t\t    (a.len == b.len))\n+\t\t\tret = memcmp(a.buf, b.buf, a.len) == 0;\n+\t}\n+\tstrbuf_release(&a);\n+\tstrbuf_release(&b);\n+\treturn ret;\n+}\n+\n /* Per entry merge function */\n static int process_entry(struct merge_options *o,\n \t\t\t const char *path, struct stage_data *entry)\n@@ -1075,8 +1113,10 @@ static int process_entry(struct merge_options *o,\n \tif (o_sha && (!a_sha || !b_sha)) {\n \t\t/* Case A: Deleted in one */\n \t\tif ((!a_sha && !b_sha) ||\n-\t\t    (sha_eq(a_sha, o_sha) && !b_sha) ||\n-\t\t    (!a_sha && sha_eq(b_sha, o_sha))) {\n+\t\t    (!b_sha && sha_eq(a_sha, o_sha) ||\n+\t\t     normalized_eq(a_sha, o_sha, path)) ||\n+\t\t    (!a_sha && sha_eq(b_sha, o_sha) ||\n+\t\t     normalized_eq(b_sha, o_sha, path))) {\n \t\t\t/* Deleted in both or deleted in one and\n \t\t\t * unchanged in the other */\n \t\t\tif (a_sha)\ndiff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh\nindex 44e6003..5e45256 100755\n--- a/t/t6038-merge-text-auto.sh\n+++ b/t/t6038-merge-text-auto.sh\n@@ -45,7 +45,7 @@ test_expect_success 'Check merging addition of text=auto' '\n \ttest_cmp file file.temp\n '\n \n-test_expect_failure 'Test delete/normalize conflict' '\n+test_expect_success 'Test delete/normalize conflict' '\n \tgit checkout side &&\n \tgit reset --hard initial &&\n \tgit rm file &&\n-- \n1.7.1.575.g383de\n"},{"id":"144359","messageId":"1f8eb89768d01fbd0624b740c6698cf333b9d3a0.1277667177.git.eyvind.bernhardsen@gmail.com","threadId":"24211","inReplyTo":"cover.1277667177.git.eyvind.bernhardsen@gmail.com","subject":"[PATCH v4 3/3] Don't expand CRLFs when normalizing text during merge","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-27T19:43:07Z","receivedAt":"2010-06-27T19:43:07Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"Disable CRLF expansion when convert_to_working_tree() is called from\nnormalize_buffer().  This improves performance when merging branches\nwith conflicting line endings when core.eol=crlf or core.autocrlf=true\nby making the normalization act as if core.eol=lf.\n\nSigned-off-by: Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>\n---\n convert.c |   27 ++++++++++++++++++++-------\n 1 files changed, 20 insertions(+), 7 deletions(-)\n\ndiff --git a/convert.c b/convert.c\nindex 0203be8..01de9a8 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -741,7 +741,9 @@ int convert_to_git(const char *path, const char *src, size_t len,\n \treturn ret | ident_to_git(path, src, len, dst, ident);\n }\n \n-int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst)\n+static int convert_to_working_tree_internal(const char *path, const char *src,\n+\t\t\t\t\t    size_t len, struct strbuf *dst,\n+\t\t\t\t\t    int normalizing)\n {\n \tstruct git_attr_check check[5];\n \tenum action action = CRLF_GUESS;\n@@ -767,18 +769,29 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n \t}\n-\taction = determine_action(action, eol_attr);\n-\tret |= crlf_to_worktree(path, src, len, dst, action);\n-\tif (ret) {\n-\t\tsrc = dst->buf;\n-\t\tlen = dst->len;\n+\t/*\n+\t * CRLF conversion can be skipped if normalizing, unless there\n+\t * is a smudge filter.  The filter might expect CRLFs.\n+\t */\n+\tif (filter || !normalizing) {\n+\t\taction = determine_action(action, eol_attr);\n+\t\tret |= crlf_to_worktree(path, src, len, dst, action);\n+\t\tif (ret) {\n+\t\t\tsrc = dst->buf;\n+\t\t\tlen = dst->len;\n+\t\t}\n \t}\n \treturn ret | apply_filter(path, src, len, dst, filter);\n }\n \n+int convert_to_working_tree(const char *path, const char *src, size_t len, struct strbuf *dst)\n+{\n+\treturn convert_to_working_tree_internal(path, src, len, dst, 0);\n+}\n+\n int renormalize_buffer(const char *path, const char *src, size_t len, struct strbuf *dst)\n {\n-\tint ret = convert_to_working_tree(path, src, len, dst);\n+\tint ret = convert_to_working_tree_internal(path, src, len, dst, 1);\n \tif (ret) {\n \t\tsrc = dst->buf;\n \t\tlen = dst->len;\n-- \n1.7.1.575.g383de\n"},{"id":"144375","messageId":"20100628080234.GA7134@pvv.org","threadId":"24211","inReplyTo":"07a766a7972671b9e39dbca55719d024c30c7a28.1277667177.git.eyvind.bernhardsen@gmail.com","subject":"Re: [PATCH v4 1/3] Avoid conflicts when merging branches with mixed normalization","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2010-06-28T08:02:34Z","receivedAt":"2010-06-28T08:02:34Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"> --- a/Documentation/gitattributes.txt\n> +++ b/Documentation/gitattributes.txt\n> @@ -317,6 +317,18 @@ command is \"cat\").\n>  \tsmudge = cat\n>  ------------------------\n>  \n> +For best results, `clean` and `smudge` commands should produce output\n> +that is not dependent on the corresponding command having been run.\n> +That is, `clean` should produce identical output whether its input has\n> +been run through `smudge` or not, and `smudge` should not rely on its\n> +input having been run through `clean`.  See the section on merging\n> +below for a rationale.\n\nI think this is marginally unclear, what about:\n\n  Clean should not alter its output further if run again\n  clean(x) == clean(clean(x))\n\n  Smudge should not alter the output of clean\n  clean(x) == clean(smudge(x))\n\nIt should not matter that smudge will do something weird if clean\nhasn't been run, as long as clean(x) == clean(smudge(x)) still\nholds. I also think it is worth mentioning explicitly that clean can\nbe run multiple times, you should not have to infer this.\n\n> [...]\n> +Merging branches with differing checkin/checkout attributes\n> +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n\nMaybe something about when this happens, or even put it in the header\ninstead? Something like\n\n  If you have added attributes to a file that cause the canonical\n  repository format for that file to change, such as adding a\n  clean/smudge filter or text/eol/ident attributes, merging anything based\n  on a point in time where the attribute was not in place would normally\n  cause merge conflicts.\n\n> +\n> +To prevent unnecessary merge conflicts, git runs a virtual check-out\n> +and check-in of all three stages of a file when resolving a three-way\n> +merge.  This prevents changes caused by check-in conversion from\n> +causing spurious merge conflicts when a converted file is merged with\n> +an unconverted file.\n\n\n- Finn Arne\n"},{"id":"144410","messageId":"0cd82ad22a6f240ebcde0c2f3a437a805dae5668.1277753114.git.eyvind.bernhardsen@gmail.com","threadId":"24211","inReplyTo":"20100628080234.GA7134@pvv.org","subject":"[PATCH] Clarify text filter merge conflict reduction docs","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-28T19:32:50Z","receivedAt":"2010-06-28T19:32:50Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"Signed-off-by: Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>\n---\nHow does this look?\n\nThis commit should really be squashed into the first one in the series.\nLive and learn, next time I'll add new changes in a new commit and put\nit _last_.  I'll submit a fixed series once we're happy with the\ndocumentation.\n\n- Eyvind\n\n Documentation/gitattributes.txt |   46 ++++++++++++++++++++++-----------------\n 1 files changed, 26 insertions(+), 20 deletions(-)\n\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex b110082..22400c1 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -317,17 +317,16 @@ command is \"cat\").\n \tsmudge = cat\n ------------------------\n \n-For best results, `clean` and `smudge` commands should produce output\n-that is not dependent on the corresponding command having been run.\n-That is, `clean` should produce identical output whether its input has\n-been run through `smudge` or not, and `smudge` should not rely on its\n-input having been run through `clean`.  See the section on merging\n-below for a rationale.\n+For best results, `clean` should not alter its output further if it is\n+run twice (\"clean->clean\" should be equivalent to \"clean\"), and\n+multiple `smudge` commands should not alter `clean`'s output\n+(\"smudge->smudge->clean\" should be equivalent to \"clean\").  See the\n+section on merging below.\n \n-The example \"indent\" filter is well-behaved in this regard: it will\n-accept input that is already correctly indented without modifying it.\n-In this case, the lack of a smudge filter means that the clean filter\n-_must_ accept its own output without modifying it.\n+The \"indent\" filter is well-behaved in this regard: it will not modify\n+input that is already correctly indented.  In this case, the lack of a\n+smudge filter means that the clean filter _must_ accept its own output\n+without modifying it.\n \n \n Interaction between checkin/checkout attributes\n@@ -346,16 +345,23 @@ with `text`, and then `ident` and fed to `filter`.\n Merging branches with differing checkin/checkout attributes\n ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^\n \n-To prevent unnecessary merge conflicts, git runs a virtual check-out\n-and check-in of all three stages of a file when resolving a three-way\n-merge.  This prevents changes caused by check-in conversion from\n-causing spurious merge conflicts when a converted file is merged with\n-an unconverted file.\n-\n-This strategy will break down if a `smudge` filter relies on its input\n-having been processed by the corresponding `clean` filter or vice\n-versa.  Such filters may otherwise work well, but will prevent\n-automatic merging.\n+If you have added attributes to a file that cause the canonical\n+repository format for that file to change, such as adding a\n+clean/smudge filter or text/eol/ident attributes, merging anything\n+where the attribute is not in place would normally cause merge\n+conflicts.\n+\n+To prevent these unnecessary merge conflicts, git runs a virtual\n+check-out and check-in of all three stages of a file when resolving a\n+three-way merge.  This prevents changes caused by check-in conversion\n+from causing spurious merge conflicts when a converted file is merged\n+with an unconverted file.\n+\n+As long as a \"smudge->clean\" results in the same output as a \"clean\"\n+even on files that are already smudged, this strategy will\n+automatically resolve all filter-related conflicts.  Filters that do\n+not act in this way may cause additional merge conflicts that must be\n+resolved manually.\n \n \n Generating diff text\n-- \n1.7.1.575.g383de\n"},{"id":"144414","messageId":"20100628203128.GA10890@pvv.org","threadId":"24211","inReplyTo":"0cd82ad22a6f240ebcde0c2f3a437a805dae5668.1277753114.git.eyvind.bernhardsen@gmail.com","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Finn Arne Gangstad","fromEmail":"finnag@pvv.org","sentAt":"2010-06-28T20:31:29Z","receivedAt":"2010-06-28T20:31:29Z","isPatch":true,"sender":{"key":"finnag@pvv.org","avatar":"https://gravatar.com/avatar/b421ddd58c3f0f93aa473e17b98bb8d53c221fef741746bc8cb59fae4ec6d95e?d=mp&s=160"},"body":"On Mon, Jun 28, 2010 at 09:32:50PM +0200, Eyvind Bernhardsen wrote:\n\n> How does this look?\n> \n> This commit should really be squashed into the first one in the series.\n> Live and learn, next time I'll add new changes in a new commit and put\n> it _last_.  I'll submit a fixed series once we're happy with the\n> documentation.\n\nLooks good I think!\n\n- Finn Arne\n"},{"id":"144469","messageId":"7vk4phbyl5.fsf@alter.siamese.dyndns.org","threadId":"24211","inReplyTo":"0cd82ad22a6f240ebcde0c2f3a437a805dae5668.1277753114.git.eyvind.bernhardsen@gmail.com","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-29T16:19:50Z","receivedAt":"2010-06-29T16:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:\n\n> Signed-off-by: Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com>\n> ---\n> How does this look?\n\nLooks Ok (I didn't read _this_ patch but read a squashed-in result),\nthanks.\n\n> +If you have added attributes to a file that cause...\n> +...To prevent these unnecessary merge conflicts,\n\nThis naturally calls for an optimization idea, doesn't it?\n\nI wonder if ll_merge should gain another flag bit to disable the calls to\nnormalize_file(), so that the whole thing can be skipped when the caller\nsomehow knows .gitattributes files that govern the path didn't change.\n\nThat won't be a trivial optimization and my gut feeling is that it\nshouldn't be part of this series.\n\nI do however wonder if this should be initially introduced as an\nexperimental feature, guarded with a configuration option for brave souls\nto try it out, and flip the feature on by default after we gain confidence\nin it, both in performance and in correctness.\n\n-- >8 --\nIntroduce \"double conversion during merge\" more gradually\n\nThis marks the recent improvement to the merge machinery that helps people\nwho changed their mind between CRLF/LF an opt in feature, so that we can\nmore easily release it early to everybody, without fear of breaking the\nmajority of users (read: on POSIX) that don't need it.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/config.txt        |   10 ++++++++++\n Documentation/gitattributes.txt |    5 +++--\n cache.h                         |    1 +\n config.c                        |    5 +++++\n environment.c                   |    1 +\n ll-merge.c                      |    8 +++++---\n 6 files changed, 25 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 4c49104..ad2a27e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -538,6 +538,16 @@ core.sparseCheckout::\n \tEnable \"sparse checkout\" feature. See section \"Sparse checkout\" in\n \tlinkgit:git-read-tree[1] for more information.\n \n+core.doubleConvert::\n+\tTell git that canonical representation of files in the repository\n+\thas changed over time (e.g. earlier commits record text files\n+\twith CRLF line endings, but recent ones use LF line endings).  In\n+\tsuch a repository, git is forced to convert the data recorded in\n+\tcommits twice before performing a merge to reduce unnecessary\n+\tconflicts.  For more information, see section\n+\t\"Merging branches with differing checkin/checkout attributes\" in\n+\tlinkgit:gitattributes[5].\n+\n add.ignore-errors::\n \tTells 'git add' to continue adding files when some files cannot be\n \tadded due to indexing errors. Equivalent to the '--ignore-errors'\ndiff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt\nindex 22400c1..504d5ca 100644\n--- a/Documentation/gitattributes.txt\n+++ b/Documentation/gitattributes.txt\n@@ -351,9 +351,10 @@ clean/smudge filter or text/eol/ident attributes, merging anything\n where the attribute is not in place would normally cause merge\n conflicts.\n \n-To prevent these unnecessary merge conflicts, git runs a virtual\n+To prevent these unnecessary merge conflicts, git can be told to run a virtual\n check-out and check-in of all three stages of a file when resolving a\n-three-way merge.  This prevents changes caused by check-in conversion\n+three-way merge by setting `core.doubleConvert` configuration variable.\n+This prevents changes caused by check-in conversion\n from causing spurious merge conflicts when a converted file is merged\n with an unconverted file.\n \ndiff --git a/cache.h b/cache.h\nindex aa725b0..217f1e9 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -551,6 +551,7 @@ extern int read_replace_refs;\n extern int fsync_object_files;\n extern int core_preload_index;\n extern int core_apply_sparse_checkout;\n+extern int core_ll_merge_double_convert;\n \n enum safe_crlf {\n \tSAFE_CRLF_FALSE = 0,\ndiff --git a/config.c b/config.c\nindex cdcf583..bea054c 100644\n--- a/config.c\n+++ b/config.c\n@@ -595,6 +595,11 @@ static int git_default_core_config(const char *var, const char *value)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"core.doubleconvert\")) {\n+\t\tcore_ll_merge_double_convert = git_config_bool(var, value);\n+\t\treturn 0;\n+\t}\n+\n \t/* Add other config variables here and to Documentation/config.txt. */\n \treturn 0;\n }\ndiff --git a/environment.c b/environment.c\nindex 83d38d3..a8f04e7 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -53,6 +53,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;\n char *notes_ref_name;\n int grafts_replace_parents = 1;\n int core_apply_sparse_checkout;\n+int core_ll_merge_double_convert;\n \n /* Parallel index stat data preload? */\n int core_preload_index = 0;\ndiff --git a/ll-merge.c b/ll-merge.c\nindex 28c6f54..8831631 100644\n--- a/ll-merge.c\n+++ b/ll-merge.c\n@@ -344,9 +344,11 @@ 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 (core_ll_merge_double_convert) {\n+\t\tnormalize_file(ancestor, path);\n+\t\tnormalize_file(ours, path);\n+\t\tnormalize_file(theirs, path);\n+\t}\n \tif (!git_path_check_merge(path, check)) {\n \t\tll_driver_name = check[0].value;\n \t\tif (check[1].value) {\n"},{"id":"144483","messageId":"BE5ECD39-0A80-410B-87C9-5C86F082773C@gmail.com","threadId":"24211","inReplyTo":"7vk4phbyl5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-29T21:18:37Z","receivedAt":"2010-06-29T21:18:37Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 29. juni 2010, at 18.19, Junio C Hamano wrote:\n\n> I do however wonder if this should be initially introduced as an\n> experimental feature, guarded with a configuration option for brave souls\n> to try it out, and flip the feature on by default after we gain confidence\n> in it, both in performance and in correctness.\n\nAgreed, and thanks.  Messing with the merge machinery unnerves me a little.\n\nShouldn't the normalization in merge-recursive be conditional too?\n\nSomething like this squashed into the delete/modify patch:\n\n--8<--\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex a2c174f..49bd3d2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1079,6 +1079,8 @@ static int normalized_eq(const unsigned char *a_sha,\n {\n        struct strbuf a = STRBUF_INIT;\n        struct strbuf b = STRBUF_INIT;\n+       if (!core_ll_merge_double_convert)\n+               return 0;\n        int ret = 0;\n        if (a_sha && b_sha &&\n            read_sha1_strbuf(a_sha, path, &a) &&\n--8<--\n\nSorry about the whitespace-damaged inline diff, I'll redo the series if necessary.\n-- \nEyvind\n"},{"id":"144508","messageId":"4718B1FE-4525-41C2-A4D3-27E99C5A6973@gmail.com","threadId":"24211","inReplyTo":"7vk4phbyl5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-30T08:20:14Z","receivedAt":"2010-06-30T08:20:14Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"On 29. juni 2010, at 18.19, Junio C Hamano wrote:\n\n> Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:\n\n[...]\n\n>> +If you have added attributes to a file that cause...\n>> +...To prevent these unnecessary merge conflicts,\n> \n> This naturally calls for an optimization idea, doesn't it?\n> \n> I wonder if ll_merge should gain another flag bit to disable the calls to\n> normalize_file(), so that the whole thing can be skipped when the caller\n> somehow knows .gitattributes files that govern the path didn't change.\n> \n> That won't be a trivial optimization and my gut feeling is that it\n> shouldn't be part of this series.\n\nAre you thinking that we could check changes in .gitattributes during a merge and only turn on normalization for those files where relevant attributes have changed?  I like it, but I agree with your gut, especially since normalization has to be enabled manually.\n\n> I do however wonder if this should be initially introduced as an\n> experimental feature, guarded with a configuration option for brave souls\n> to try it out, and flip the feature on by default after we gain confidence\n> in it, both in performance and in correctness.\n\nMy fix to add the configuration option to the delete/modify patch yesterday was pretty bad, sorry.  My only excuse is that I was in a hurry, I'll resend the series tonight with a better fix.\n-- \nEyvind Bernhardsen\n"},{"id":"144518","messageId":"7vbpas8scr.fsf@alter.siamese.dyndns.org","threadId":"24211","inReplyTo":"4718B1FE-4525-41C2-A4D3-27E99C5A6973@gmail.com","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-30T15:15:00Z","receivedAt":"2010-06-30T15:15:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:\n\n> Are you thinking that we could check changes in .gitattributes during a\n> merge and only turn on normalization for those files where relevant\n> attributes have changed?\n\nNothing that elaborate.\n\nI was envisioning that we would compare object names of the .gitattributes\nfiles in directories that lead to the path being merged in three trees,\nand we use the new slowpath unless all three match.  You could look _into_\nthe actual contents of .gitattributes and decide that a particular change\ndoes not affect the path you are merging, but I don't think it is worth\nit; sane people are expected not to flip CRLF/LF around many times a day\nanyway, so changes to .gitattributes should already be rare events.\n\nWe will be walking the trees in parallel while merging anyway, so when you\nhave to merge a/b/c.txt, we would already have opened the top-level tree,\ntree \"a\", and tree \"a/b\" already, and we should be able pick up the object\nname of .gitattributes, a/.gitattributes and b/.gitattributes cheaply\nwithout opening any extra object.\n"},{"id":"144537","messageId":"7v4ogk76sb.fsf@alter.siamese.dyndns.org","threadId":"24211","inReplyTo":"BE5ECD39-0A80-410B-87C9-5C86F082773C@gmail.com","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-06-30T17:46:12Z","receivedAt":"2010-06-30T17:46:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:\n\n> Shouldn't the normalization in merge-recursive be conditional too?\n\nTrue, but your patch to merge-recursive is broken, I think.  It should at\nleast look like the attached rewrite.\n\nKey points:\n\n - Your \"normalized_eq()\" is called only from the codepath that wants to\n   know if \"one is deleted and the other is unchanged\" to implement the\n   latter half of that check.  Let's name that function blob_unchanged().\n   Also let's move the \"how to compare two blobs when we are not doing\n   double conversion\" (i.e. sha_eq()) logic from its caller to it.  These\n   changes would make it a lot easier to read the caller.\n\n - Your read_sha1_strbuf() took \"path\" as input but didn't do anything\n   with it.  Let's drop it.\n\n - It didn't diagnose any errors.  Let's make sure that it follows the\n   usual \"0 for success, negative for error\" convention and make sure its\n   only caller knows about it.\n\n - \"blob_unchanged()\" (aka \"normalized_eq()\") is called only when we have\n   two blobs to compare.  It is a programming error to pass NULL in either\n   o_sha or a_sha; let's assert() it.\n\n - Your error path in the function made the caller assume that two input\n   matched and it is Ok to continue; it should instead force the caller to\n   stop so that the end user can notice.\n\n - Return values from functions in convert.c are NOT error signals.  A\n   true value is to let the caller know that the callee had to convert\n   (and a false value is that the callee didn't convert), and the next\n   transformation needs to be done on the result in the strbuf (as opposed\n   to the src buffer that was left inact).\n\n   In your use of renormalize_buffer(), your input \"src\" points at the\n   same strbuf as your output \"dst\", so your caller does not care if\n   renormalize didn't have to do anything.  Either way, you would pick the\n   result up from the strbuf.\n\n   But using the return value to skip the comparison as if the call failed\n   is _wrong_.  Even if there is no need for conversion for the given path\n   (i.e. 0 return from convert.c functions), you would need to pick up the\n   result and perform the comparison.\n\nIf you had a test that made sure that the merge works for paths that do\nnot need double-conversion, you might have caught the last issue.  I\nsuspect that your new tests _only_ checked what happens to paths that\nactually triggers these double conversion, without making sure that the\nnew code would not affect the cases where it shouldn't be involved, no?\n\nIt is a common trap to fall into not testing the negative case, when you\nare working on your own shiny new toy.  Let's be more careful when writing\nnew tests.\n\nThanks.\n\n merge-recursive.c          |   45 ++++++++++++++++++++++++++++++++++++++++++-\n t/t6038-merge-text-auto.sh |    2 +-\n 2 files changed, 44 insertions(+), 3 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 206c103..3c63e5e 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1056,6 +1056,47 @@ static unsigned char *stage_sha(const unsigned char *sha, unsigned mode)\n \treturn (is_null_sha1(sha) || mode == 0) ? NULL: (unsigned char *)sha;\n }\n \n+static int read_sha1_strbuf(const unsigned char *sha1, struct strbuf *dst)\n+{\n+\tvoid *buf;\n+\tenum object_type type;\n+\tunsigned long size;\n+\tbuf = read_sha1_file(sha1, &type, &size);\n+\tif (!buf)\n+\t\treturn error(\"cannot read object %s\", sha1_to_hex(sha1));\n+\tif (type != OBJ_BLOB) {\n+\t\tfree(buf);\n+\t\treturn error(\"object %s is not a blob\", sha1_to_hex(sha1));\n+\t}\n+\tstrbuf_attach(dst, buf, size, size + 1);\n+\treturn 0;\n+}\n+\n+static int blob_unchanged(const unsigned char *o_sha,\n+\t\t\t  const unsigned char *a_sha,\n+\t\t\t  const char *path)\n+{\n+\tstruct strbuf o = STRBUF_INIT;\n+\tstruct strbuf a = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!core_ll_merge_double_convert)\n+\t\treturn sha_eq(o_sha, a_sha);\n+\n+\tret = 0; /* assume changed for safety */\n+\tassert(o_sha && a_sha);\n+\tif (read_sha1_strbuf(o_sha, &o) || read_sha1_strbuf(a_sha, &a))\n+\t\tgoto error_return;\n+\trenormalize_buffer(path, o.buf, o.len, &o);\n+\trenormalize_buffer(path, a.buf, o.len, &a);\n+\tret = (o.len == a.len && !memcmp(o.buf, a.buf, o.len));\n+\n+error_return:\n+\tstrbuf_release(&o);\n+\tstrbuf_release(&a);\n+\treturn ret;\n+}\n+\n /* Per entry merge function */\n static int process_entry(struct merge_options *o,\n \t\t\t const char *path, struct stage_data *entry)\n@@ -1075,8 +1116,8 @@ static int process_entry(struct merge_options *o,\n \tif (o_sha && (!a_sha || !b_sha)) {\n \t\t/* Case A: Deleted in one */\n \t\tif ((!a_sha && !b_sha) ||\n-\t\t    (sha_eq(a_sha, o_sha) && !b_sha) ||\n-\t\t    (!a_sha && sha_eq(b_sha, o_sha))) {\n+\t\t    (!b_sha && blob_unchanged(o_sha, a_sha, path)) ||\n+\t\t    (!a_sha && blob_unchanged(o_sha, b_sha, path))) {\n \t\t\t/* Deleted in both or deleted in one and\n \t\t\t * unchanged in the other */\n \t\t\tif (a_sha)\ndiff --git a/t/t6038-merge-text-auto.sh b/t/t6038-merge-text-auto.sh\nindex 2caebb9..4a7bc48 100755\n--- a/t/t6038-merge-text-auto.sh\n+++ b/t/t6038-merge-text-auto.sh\n@@ -46,7 +46,7 @@ test_expect_success 'Check merging addition of text=auto' '\n \ttest_cmp file file.temp\n '\n \n-test_expect_failure 'Test delete/normalize conflict' '\n+test_expect_success 'Test delete/normalize conflict' '\n \tgit checkout side &&\n \tgit reset --hard initial &&\n \tgit rm file &&\n"},{"id":"144555","messageId":"0CF2361E-D010-4042-B481-6918BE2A9341@gmail.com","threadId":"24211","inReplyTo":"7v4ogk76sb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Eyvind Bernhardsen","fromEmail":"eyvind.bernhardsen@gmail.com","sentAt":"2010-06-30T21:32:49Z","receivedAt":"2010-06-30T21:32:49Z","isPatch":true,"sender":{"key":"eyvind.bernhardsen@gmail.com","avatar":"https://avatars.githubusercontent.com/u/106762?v=4"},"body":"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.\n\nOn 30. juni 2010, at 19.46, Junio C Hamano wrote:\n\n> Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:\n> \n>> Shouldn't the normalization in merge-recursive be conditional too?\n> \n> True, but your patch to merge-recursive is broken, I think.  It should at\n> least look like the attached rewrite.\n\nYour 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).\n\n[...]\n\n> If you had a test that made sure that the merge works for paths that do\n> not need double-conversion, you might have caught the last issue.  I\n> suspect that your new tests _only_ checked what happens to paths that\n> actually triggers these double conversion, without making sure that the\n> new code would not affect the cases where it shouldn't be involved, no?\n\nThe 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.\n\n> It is a common trap to fall into not testing the negative case, when you\n> are working on your own shiny new toy.  Let's be more careful when writing\n> new tests.\n\nYes, 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 :)\n\nEven though I like your version better than mine, I have a few comments, only some of which are defensive:\n\n> +\tif (!core_ll_merge_double_convert)\n> +\t\treturn sha_eq(o_sha, a_sha);\n\nI would rather do this:\n\n\tif (sha_eq(o_sha, a_sha))\n\t\treturn 1;\n\tif (!core_ll_merge_double_convert)\n\t\treturn 0;\n\nIf the sha1s are equal before normalization the files will be equal after normalization, so there's no sense in going through the rigmarole.\n\nBikeshed 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).\n\n> +\tret = 0; /* assume changed for safety */\n> +\tassert(o_sha && a_sha);\n> +\tif (read_sha1_strbuf(o_sha, &o) || read_sha1_strbuf(a_sha, &a))\n> +\t\tgoto error_return;\n> +\trenormalize_buffer(path, o.buf, o.len, &o);\n> +\trenormalize_buffer(path, a.buf, o.len, &a);\n> +\tret = (o.len == a.len && !memcmp(o.buf, a.buf, o.len));\n\nThis 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).\n\nIf 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.\n\n> +error_return:\n> +\tstrbuf_release(&o);\n> +\tstrbuf_release(&a);\n> +\treturn ret;\n\nI'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?\n\nThanks again for taking the time to improve my patch.\n-- \nEyvind\n"},{"id":"144561","messageId":"7vd3v7511n.fsf@alter.siamese.dyndns.org","threadId":"24211","inReplyTo":"0CF2361E-D010-4042-B481-6918BE2A9341@gmail.com","subject":"Re: [PATCH] Clarify text filter merge conflict reduction docs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-07-01T03:33:08Z","receivedAt":"2010-07-01T03:33:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eyvind Bernhardsen <eyvind.bernhardsen@gmail.com> writes:\n\n>> +\tif (!core_ll_merge_double_convert)\n>> +\t\treturn sha_eq(o_sha, a_sha);\n>\n> I would rather do this:\n>\n> \tif (sha_eq(o_sha, a_sha))\n> \t\treturn 1;\n> \tif (!core_ll_merge_double_convert)\n> \t\treturn 0;\n\nYou are absolutely right.  This part of my patch was an unnecessary\npessimization.\n\nWill fix.\n"}]}