merge: avoid "safer crlf" during recording of merge results

When merge_recursive() decides what the correct blob object merge result for a path should be, it uses update_file_flags() helper function to write it out to a working tree file and then calls add_cacheinfo(). The add_cacheinfo() function in turn calls make_cache_entry() to create a new cache entry to replace the higher-stage entries for the path that represents the conflict. The make_cache_entry() function calls refresh_cache_entry() to fill in the cached stat information. To mark a cache entry as up-to-date, the data is re-read from the file in the working tree, and goes through convert_to_git() conversion to be compared with the blob object name the new cache entry records. It is important to note that this happens while the higher-stage entries, which are going to be replaced with the new entry, are still in the index. Unfortunately, the convert_to_git() conversion has a misguided "safer crlf" mechanism baked in, and looks at the existing cache entry for the path to decide how to convert the contents in the working tree file. If our side (i.e. stage#2) records a text blob with CRLF in it, even when the system is configured to record LF in blobs and convert them to CRLF upon checkout (and back to LF upon checkin), the "safer crlf" mechanism stops us doing so. This especially poses a problem during a renormalizing merge, where the merge result for the path is computed by first "normalizing" the blobs involved in the merge by using convert_to_working_tree() followed by convert_to_git() with "safer crlf" disabled. The merge result that is computed correctly and fed to add_cacheinfo() via update_file_flags() does _not_ match what refresh_cache_entry() sees by converting the working tree file via convert_to_git(). We can work this around by not refreshing the new cache entry in make_cache_entry() called by add_cacheinfo(). After add_cacheinfo() adds the new entry, we can call refresh_cache_entry() on that, knowing that addition of this new cache entry would have removed the stale cache entries that had CRLF in stage #2 that were carried over before the renormalizing merge started and will not interfere with the correct recording of the result. The test update was taken from a series by Torsten Bögershausen that attempted to fix this with a different approach. Signed-off-by: Torsten Bögershausen <tboegi@web.de> Signed-off-by: Junio C Hamano <gitster@pobox.com> Reviewed-by: Torsten Bögershausen <tboegi@web.de>

Junio C Hamano committed Jul 8, 2016 at 10:59 UTC 1335d76e4569fa84e52dc24c88c04daeae6e160e
4 files changed +43 -31
cache.h
+1
@@ -632,6 +632,7 @@ extern void fill_stat_cache_info(struct cache_entry *ce, struct stat *st);
632 #define REFRESH_IGNORE_SUBMODULES 0x0010 /* ignore submodules */
633 #define REFRESH_IN_PORCELAIN 0x0020 /* user friendly output, not "needs update" */
634 extern int refresh_index(struct index_state *, unsigned int flags, const struct pathspec *pathspec, char *seen, const char *header_msg);
635 +extern struct cache_entry *refresh_cache_entry(struct cache_entry *, unsigned int);
636
637 extern void update_index_if_able(struct index_state *, struct lock_file *);
638
merge-recursive.c
+13 -4
@@ -202,12 +202,21 @@ static int add_cacheinfo(unsigned int mode, const unsigned char *sha1,
202 const char *path, int stage, int refresh, int options)
203 {
204 struct cache_entry *ce;
205 - ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage,
206 - (refresh ? (CE_MATCH_REFRESH |
207 - CE_MATCH_IGNORE_MISSING) : 0 ));
205 + int ret;
206 +
207 + ce = make_cache_entry(mode, sha1 ? sha1 : null_sha1, path, stage, 0);
208 if (!ce)
209 return error(_("addinfo_cache failed for path '%s'"), path);
210 - return add_cache_entry(ce, options);
210 +
211 + ret = add_cache_entry(ce, options);
212 + if (refresh) {
213 + struct cache_entry *nce;
214 +
215 + nce = refresh_cache_entry(ce, CE_MATCH_REFRESH | CE_MATCH_IGNORE_MISSING);
216 + if (nce != ce)
217 + ret = add_cache_entry(nce, options);
218 + }
219 + return ret;
220 }
221
222 static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree)
read-cache.c
+1 -4
@@ -19,9 +19,6 @@
19 #include "split-index.h"
20 #include "utf8.h"
21
22 -static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
23 - unsigned int options);
24 -
22 /* Mask for the name length in ce_flags in the on-disk index */
23
24 #define CE_NAMEMASK (0x0fff)
@@ -1254,7 +1251,7 @@ int refresh_index(struct index_state *istate, unsigned int flags,
1251 return has_errors;
1252 }
1253
1257 -static struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
1254 +struct cache_entry *refresh_cache_entry(struct cache_entry *ce,
1255 unsigned int options)
1256 {
1257 return refresh_cache_ent(&the_index, ce, options, NULL, NULL);
t/t6038-merge-text-auto.sh
+28 -23
@@ -91,16 +91,13 @@ test_expect_success 'Merge after setting text=auto' '
91 compare_files expected file
92 '
93
94 -test_expect_success 'Merge addition of text=auto' '
94 +test_expect_success 'Merge addition of text=auto eol=LF' '
95 + git config core.eol lf &&
96 cat <<-\EOF >expected &&
97 first line
98 same line
99 EOF
100
100 - if test_have_prereq NATIVE_CRLF; then
101 - append_cr <expected >expected.temp &&
102 - mv expected.temp expected
103 - fi &&
101 git config merge.renormalize true &&
102 git rm -fr . &&
103 rm -f .gitattributes &&
@@ -109,17 +106,31 @@ test_expect_success 'Merge addition of text=auto' '
106 compare_files expected file
107 '
108
109 +test_expect_success 'Merge addition of text=auto eol=CRLF' '
110 + git config core.eol crlf &&
111 + cat <<-\EOF >expected &&
112 + first line
113 + same line
114 + EOF
115 +
116 + append_cr <expected >expected.temp &&
117 + mv expected.temp expected &&
118 + git config merge.renormalize true &&
119 + git rm -fr . &&
120 + rm -f .gitattributes &&
121 + git reset --hard b &&
122 + echo >&2 "After git reset --hard b" &&
123 + git ls-files -s --eol >&2 &&
124 + git merge a &&
125 + compare_files expected file
126 +'
127 +
128 test_expect_success 'Detect CRLF/LF conflict after setting text=auto' '
129 + git config core.eol native &&
130 echo "<<<<<<<" >expected &&
114 - if test_have_prereq NATIVE_CRLF; then
115 - echo first line | append_cr >>expected &&
116 - echo same line | append_cr >>expected &&
117 - echo ======= | append_cr >>expected
118 - else
119 - echo first line >>expected &&
120 - echo same line >>expected &&
121 - echo ======= >>expected
122 - fi &&
131 + echo first line >>expected &&
132 + echo same line >>expected &&
133 + echo ======= >>expected &&
134 echo first line | append_cr >>expected &&
135 echo same line | append_cr >>expected &&
136 echo ">>>>>>>" >>expected &&
@@ -135,15 +146,9 @@ test_expect_success 'Detect LF/CRLF conflict from addition of text=auto' '
146 echo "<<<<<<<" >expected &&
147 echo first line | append_cr >>expected &&
148 echo same line | append_cr >>expected &&
138 - if test_have_prereq NATIVE_CRLF; then
139 - echo ======= | append_cr >>expected &&
140 - echo first line | append_cr >>expected &&
141 - echo same line | append_cr >>expected
142 - else
143 - echo ======= >>expected &&
144 - echo first line >>expected &&
145 - echo same line >>expected
146 - fi &&
149 + echo ======= >>expected &&
150 + echo first line >>expected &&
151 + echo same line >>expected &&
152 echo ">>>>>>>" >>expected &&
153 git config merge.renormalize false &&
154 rm -f .gitattributes &&