Re-rolled merge normalization
v5 cover Re-rolled merge normalization
Hi Junio,
I took the liberty of re-rolling my series with your improved d/m patch. I re-added some optimizations to your patch and renamed the config variable to something a little more typeable.
Signature · 24 lines
Eyvind Eyvind Bernhardsen (3): Avoid conflicts when merging branches with mixed normalization Try normalizing files to avoid delete/modify conflicts when merging Don't expand CRLFs when normalizing text during merge Junio C Hamano (1): Introduce "double conversion during merge" more gradually Documentation/config.txt | 10 ++++++ Documentation/gitattributes.txt | 34 ++++++++++++++++++++++ cache.h | 2 + config.c | 5 +++ convert.c | 37 ++++++++++++++++++++---- environment.c | 1 + ll-merge.c | 15 ++++++++++ merge-recursive.c | 51 ++++++++++++++++++++++++++++++++- t/t6038-merge-text-auto.sh | 59 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 206 insertions(+), 8 deletions(-) create mode 100755 t/t6038-merge-text-auto.sh -- 1.7.2.rc1.4.g09d06
PATCH v5 1/4 Avoid conflicts when merging branches with mixed normalization
Currently, merging across changes in line ending normalization is painful since files containing CRLF will conflict with normalized files, even if the only difference between the two versions is the line endings. Additionally, any "real" merge conflicts that exist are obscured because every line in the file has a conflict.
Assume you start out with a repo that has a lot of text files with CRLF checked in (A):
o---C
/ \
A---B---DB: Add "* text=auto" to .gitattributes and normalize all files to
LF-only
C: Modify some of the text files
D: Try to merge C
You will get a ridiculous number of LF/CRLF conflicts when trying to merge C into D, since the repository contents for C are "wrong" wrt the new .gitattributes file.
Fix ll-merge so that the "base", "theirs" and "ours" stages are passed through convert_to_worktree() and convert_to_git() before a three-way merge. This ensures that all three stages are normalized in the same way, removing from consideration differences that are only due to normalization.
--- Documentation/gitattributes.txt | 33 ++++++++++++++++++++++ cache.h | 1 + convert.c | 16 +++++++++- ll-merge.c | 13 +++++++++ t/t6038-merge-text-auto.sh | 58 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 119 insertions(+), 2 deletions(-) create mode 100755 t/t6038-merge-text-auto.sh
Documentation/gitattributes.txt +33 -0
@@ -317,6 +317,17 @@ command is "cat"). smudge = cat ------------------------ +For best results, `clean` should not alter its output further if it is +run twice ("clean->clean" should be equivalent to "clean"), and +multiple `smudge` commands should not alter `clean`'s output +("smudge->smudge->clean" should be equivalent to "clean"). See the +section on merging below. + +The "indent" filter is well-behaved in this regard: it will not modify +input that is already correctly indented. In this case, the lack of a +smudge filter means that the clean filter _must_ accept its own output +without modifying it. + Interaction between checkin/checkout attributes ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
@@ -331,6 +342,28 @@ In the check-out codepath, the blob content is first converted with `text`, and then `ident` and fed to `filter`. +Merging branches with differing checkin/checkout attributes +^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +If you have added attributes to a file that cause the canonical +repository format for that file to change, such as adding a +clean/smudge filter or text/eol/ident attributes, merging anything +where the attribute is not in place would normally cause merge +conflicts. + +To prevent these unnecessary merge conflicts, git runs a virtual +check-out and check-in of all three stages of a file when resolving a +three-way merge. This prevents changes caused by check-in conversion +from causing spurious merge conflicts when a converted file is merged +with an unconverted file. + +As long as a "smudge->clean" results in the same output as a "clean" +even on files that are already smudged, this strategy will +automatically resolve all filter-related conflicts. Filters that do +not act in this way may cause additional merge conflicts that must be +resolved manually. + + Generating diff text ~~~~~~~~~~~~~~~~~~~~
cache.h +1 -0
@@ -1054,6 +1054,7 @@ extern void trace_argv_printf(const char **argv, const char *format, ...);extern int convert_to_git (const char *path,const char *src,size_t len,struct strbuf *dst,enum safe_crlf checksafe);extern int convert_to_working_tree (const char *path,const char *src,size_t len,struct strbuf *dst); +extern int renormalize_buffer (const char *path,const char *src,size_t len,struct strbuf *dst);/* add */ /*
convert.c +14 -2
@@ -93,7 +93,8 @@ static int is_binary(unsigned long size, struct text_stat *stats)return 0 ; } -static enum eol determine_output_conversion (enum action action) { +static enum eol determine_output_conversion (enum action action) +{switch (action) {case CRLF_BINARY :return EOL_UNSET ;
@@ -693,7 +694,8 @@ static int git_path_check_ident(const char *path, struct git_attr_check *check)return !!ATTR_TRUE (value); } -enum action determine_action (enum action text_attr,enum eol eol_attr) { +static enum action determine_action (enum action text_attr,enum eol eol_attr) +{if (text_attr ==CRLF_BINARY )return CRLF_BINARY ;if (eol_attr ==EOL_LF )
@@ -773,3 +775,13 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc }return ret |apply_filter (path, src, len, dst, filter); } + +int renormalize_buffer (const char *path,const char *src,size_t len,struct strbuf *dst) +{ +int ret =convert_to_working_tree (path, src, len, dst); +if (ret) { + src = dst->buf; + len = dst->len; + } +return ret |convert_to_git (path, src, len, dst,0 ); +}
ll-merge.c +13 -0
@@ -321,6 +321,16 @@ static int git_path_check_merge(const char *path, struct git_attr_check check[2]return git_checkattr (path,2 , check); } +static void normalize_file (mmfile_t *mm,const char *path) +{ +struct strbuf strbuf =STRBUF_INIT ; +if (renormalize_buffer (path, mm->ptr, mm->size, &strbuf)) { +free (mm->ptr); + mm->size = strbuf.len; + mm->ptr =strbuf_detach (&strbuf,NULL ); + } +} + int ll_merge(mmbuffer_t *result_buf, const char *path, mmfile_t *ancestor, const char *ancestor_label,
@@ -334,6 +344,9 @@ int ll_merge(mmbuffer_t *result_buf,const struct ll_merge_driver *driver;int virtual_ancestor = flag &01 ; +normalize_file (ancestor, path); +normalize_file (ours, path); +normalize_file (theirs, path);if (!git_path_check_merge (path, check)) { ll_driver_name = check[0 ].value;if (check[1 ].value) {
t/t6038-merge-text-auto.sh new file +58 -0
58 lines not on this page · raw message
Signature · 1 line
1.7.2.rc1.4.g09d06
PATCH v5 2/4 Introduce "double conversion during merge" more gradually
In reply to Eyvind BernhardsenFrom: Junio C Hamano <[email protected]>
This marks the recent improvement to the merge machinery that helps people who changed their mind between CRLF/LF an opt in feature, so that we can more easily release it early to everybody, without fear of breaking the majority of users (read: on POSIX) that don't need it.
--- Documentation/config.txt | 10 ++++++++++ Documentation/gitattributes.txt | 5 +++-- cache.h | 1 + config.c | 5 +++++ environment.c | 1 + ll-merge.c | 8 +++++--- t/t6038-merge-text-auto.sh | 1 + 7 files changed, 26 insertions(+), 5 deletions(-)
Documentation/config.txt +10 -0
@@ -538,6 +538,16 @@ core.sparseCheckout:: Enable "sparse checkout" feature. See section "Sparse checkout" in linkgit:git-read-tree[1] for more information. +core.mergePrefilter:: + Tell git that canonical representation of files in the repository + has changed over time (e.g. earlier commits record text files + with CRLF line endings, but recent ones use LF line endings). In + such a repository, git can convert the data recorded in commits to + a canonical form before performing a merge to reduce unnecessary + conflicts. For more information, see section + "Merging branches with differing checkin/checkout attributes" in + linkgit:gitattributes[5]. + add.ignore-errors:: Tells 'git add' to continue adding files when some files cannot be added due to indexing errors. Equivalent to the '--ignore-errors'
Documentation/gitattributes.txt +3 -2
@@ -351,9 +351,10 @@ clean/smudge filter or text/eol/ident attributes, merging anything where the attribute is not in place would normally cause merge conflicts. -To prevent these unnecessary merge conflicts, git runs a virtual +To prevent these unnecessary merge conflicts, git can be told to run a virtual check-out and check-in of all three stages of a file when resolving a -three-way merge. This prevents changes caused by check-in conversion +three-way merge by setting the `core.mergePrefilter` configuration variable. +This prevents changes caused by check-in conversion from causing spurious merge conflicts when a converted file is merged with an unconverted file.
cache.h +1 -0
@@ -551,6 +551,7 @@ extern int read_replace_refs;extern int fsync_object_files;extern int core_preload_index;extern int core_apply_sparse_checkout; +extern int core_merge_prefilter;enum safe_crlf {SAFE_CRLF_FALSE =0 ,
config.c +5 -0
@@ -595,6 +595,11 @@ static int git_default_core_config(const char *var, const char *value)return 0 ; } +if (!strcmp (var,"core.mergeprefilter" )) { + core_merge_prefilter =git_config_bool (var, value); +return 0 ; + } +/* Add other config variables here and to Documentation/config.txt. */ return 0 ; }
environment.c +1 -0
@@ -53,6 +53,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;char *notes_ref_name;int grafts_replace_parents =1 ;int core_apply_sparse_checkout; +int core_merge_prefilter;/* Parallel index stat data preload? */ int core_preload_index =0 ;
ll-merge.c +5 -3
@@ -344,9 +344,11 @@ int ll_merge(mmbuffer_t *result_buf,const struct ll_merge_driver *driver;int virtual_ancestor = flag &01 ; -normalize_file (ancestor, path); -normalize_file (ours, path); -normalize_file (theirs, path); +if (core_merge_prefilter) { +normalize_file (ancestor, path); +normalize_file (ours, path); +normalize_file (theirs, path); + }if (!git_path_check_merge (path, check)) { ll_driver_name = check[0 ].value;if (check[1 ].value) {
t/t6038-merge-text-auto.sh +1 -0
@@ -5,6 +5,7 @@ test_description='CRLF merge conflict across text=auto change' . ./test-lib.sh test_expect_success setup ' + git config core.mergeprefilter true && git config core.autocrlf false && echo first line | append_cr >file && git add file &&
Signature · 1 line
1.7.2.rc1.4.g09d06
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Am 7/1/2010 11:09, schrieb Eyvind Bernhardsen:
Quoted 1 line
+core.mergePrefilter::
BTW, any particular reason that this is in the core namespace rather than merge namespace? It could be merge.prefilter.
-- Hannes
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Quoted 5 lines · Johannes Sixt
Am 7/1/2010 11:09, schrieb Eyvind Bernhardsen:
Quoted 1 line
+core.mergePrefilter::
BTW, any particular reason that this is in the core namespace rather than merge namespace? It could be merge.prefilter.
Good point.
Somehow to me "prefilter" does not sound to convey what really is going on here, though.
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Quoted 5 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt
Am 7/1/2010 11:09, schrieb Eyvind Bernhardsen:
Quoted 1 line
+core.mergePrefilter::
Quoted 7 lines
Quoted 2 lines
BTW, any particular reason that this is in the core namespace rather than merge namespace? It could be merge.prefilter.
Good point.
Somehow to me "prefilter" does not sound to convey what really is going on here, though.
"Doubleconvert" doesn't really mean anything either though, and "convert" and "normalise" are too generic. I think the problem is that there's no existing name for what convert.c does.
I chose "filter" because of the filter property; the crlf and ident things can be regarded as built-in filters.
Signature · 1 line
Eyvind
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Quoted 14 lines · Eyvind Bernhardsen
Quoted 5 lines · Junio C Hamano
Quoted 4 lines · Johannes Sixt
Am 7/1/2010 11:09, schrieb Eyvind Bernhardsen:
Quoted 1 line
+core.mergePrefilter::
Quoted 7 lines
Quoted 2 lines
BTW, any particular reason that this is in the core namespace rather than merge namespace? It could be merge.prefilter.
Good point.
Somehow to me "prefilter" does not sound to convey what really is going on here, though.
Quoted 6 lines
"Doubleconvert" doesn't really mean anything either though, and "convert" and "normalise" are too generic. I think the problem is that there's no existing name for what
convert.cdoes.I chose "filter" because of the filter property; the crlf and ident things can be regarded as built-in filters. -- Eyvind
What about `merge.renormalize' ;-) ?
Signature · 3 lines
Jakub Narebski Poland ShadeHawk on #git
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Quoted 11 lines · Jakub Narebski
Quoted 5 lines · Eyvind Bernhardsen
Quoted 4 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt
Am 7/1/2010 11:09, schrieb Eyvind Bernhardsen:
Quoted 1 line
+core.mergePrefilter::
[...]
Quoted 4 lines
Quoted 3 lines
Somehow to me "prefilter" does not sound to convey what really is going on here, though.
Quoted 8 lines
Quoted 6 lines
"Doubleconvert" doesn't really mean anything either though, and "convert" and "normalise" are too generic. I think the problem is that there's no existing name for what
convert.cdoes.I chose "filter" because of the filter property; the crlf and ident things can be regarded as built-in filters. -- Eyvind
What about `merge.renormalize' ;-) ?
Best so far! Or what about "merge.canonicalize"? Sorry for bikeshedding :)
- Finn Arne
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Quoted 16 lines · Finn Arne Gangstad
Quoted 15 lines · Jakub Narebski
Quoted 5 lines · Eyvind Bernhardsen
Quoted 4 lines · Junio C Hamano
Quoted 3 lines · Johannes Sixt
Am 7/1/2010 11:09, schrieb Eyvind Bernhardsen:
Quoted 1 line
+core.mergePrefilter::
[...]
Quoted 8 lines
Quoted 3 lines
Somehow to me "prefilter" does not sound to convey what really is going on here, though.
"Doubleconvert" doesn't really mean anything either though, and "convert" and "normalise" are too generic. I think the problem is that there's no existing name for what
convert.cdoes.
Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
I chose "filter" because of the filter property; the crlf and ident things can be regarded as built-in filters. -- Eyvind
What about `merge.renormalize' ;-) ?
Best so far! Or what about "merge.canonicalize"? Sorry for bikeshedding :)
Perhaps merge.regularize'? Or merge.normalizeToWorkTree'? It is about converting to worktree version according to current settings, IIUC...
Signature · 3 lines
Jakub Narebski Poland ShadeHawk on #git
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
Quoted 9 lines · Jakub Narebski
Quoted 4 lines · Finn Arne Gangstad
Quoted 1 line · Jakub Narebski
What about `merge.renormalize' ;-) ?
Best so far! Or what about "merge.canonicalize"? Sorry for bikeshedding :)
Perhaps
merge.regularize'? Ormerge.normalizeToWorkTree'? It is about converting to worktree version according to current settings, IIUC...
Almost, it's about converting to the _repository_ version according to current (that is, merged) settings. Since the content is already in repository format it needs to be converted to the worktree version before it is converted back, hence Junio's "doubleConvert".
"merge.renormalizeUsingMergedGitattributes"?
Signature · 1 line
Eyvind
Re: [PATCH v5 2/4] Introduce "double conversion during merge" more gradually
In reply to Jakub NarebskiQuoted 5 lines · Jakub Narebski
Quoted 4 lines · Eyvind Bernhardsen
Quoted 2 lines · Junio C Hamano
Somehow to me "prefilter" does not sound to convey what really is going on here, though.
Quoted 8 lines
Quoted 6 lines
"Doubleconvert" doesn't really mean anything either though, and "convert" and "normalise" are too generic. I think the problem is that there's no existing name for what
convert.cdoes.I chose "filter" because of the filter property; the crlf and ident things can be regarded as built-in filters. -- Eyvind
What about `merge.renormalize' ;-) ?
I like it, but it's still a bit generic. "merge.renormalizeContent", perhaps?
Signature · 1 line
Eyvind
PATCH v5 3/4 Try normalizing files to avoid delete/modify conflicts when merging
In reply to Eyvind BernhardsenIf a file is modified due to normalization on one branch, and deleted on another, a merge of the two branches will result in a delete/modify conflict for that file even if it is otherwise unchanged.
Try to avoid the conflict by normalizing and comparing the "base" file and the modified file when their sha1s differ. If they compare equal, the file is considered unmodified and is deleted.
--- merge-recursive.c | 51 ++++++++++++++++++++++++++++++++++++++++++- t/t6038-merge-text-auto.sh | 2 +- 2 files changed, 50 insertions(+), 3 deletions(-)
merge-recursive.c +49 -2
@@ -1056,6 +1056,53 @@ 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 =0 ;/* assume changed for safety */ + +if (sha_eq (o_sha, a_sha)) +return 1 ; +if (!core_merge_prefilter) +return 0 ; + +assert (o_sha && a_sha); +if (read_sha1_strbuf (o_sha, &o) ||read_sha1_strbuf (a_sha, &a)) + goto error_return; +/* +* Note: binary | is used so that both renormalizations are +* performed. Comparison can be skipped if both files are +* unchanged since their sha1s have already been compared. +*/ +if (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 +1122,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)
t/t6038-merge-text-auto.sh +1 -1
@@ -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 &&
Signature · 1 line
1.7.2.rc1.4.g09d06
PATCH v5 4/4 Don't expand CRLFs when normalizing text during merge
In reply to Eyvind BernhardsenDisable CRLF expansion when convert_to_working_tree() is called from normalize_buffer(). This improves performance when merging branches with conflicting line endings when core.eol=crlf or core.autocrlf=true by making the normalization act as if core.eol=lf.
--- convert.c | 27 ++++++++++++++++++++------- 1 files changed, 20 insertions(+), 7 deletions(-)
convert.c +20 -7
@@ -741,7 +741,9 @@ int convert_to_git(const char *path, const char *src, size_t len,return ret |ident_to_git (path, src, len, dst, ident); } -int convert_to_working_tree (const char *path,const char *src,size_t len,struct strbuf *dst) +static int convert_to_working_tree_internal (const char *path,const char *src, +size_t len,struct strbuf *dst, +int normalizing) {struct git_attr_check check[5 ];enum action action =CRLF_GUESS ;
@@ -767,18 +769,29 @@ int convert_to_working_tree(const char *path, const char *src, size_t len, struc src = dst->buf; len = dst->len; } - action =determine_action (action, eol_attr); - ret |=crlf_to_worktree (path, src, len, dst, action); -if (ret) { - src = dst->buf; - len = dst->len; +/* +* CRLF conversion can be skipped if normalizing, unless there +* is a smudge filter. The filter might expect CRLFs. +*/ +if (filter || !normalizing) { + action =determine_action (action, eol_attr); + ret |=crlf_to_worktree (path, src, len, dst, action); +if (ret) { + src = dst->buf; + len = dst->len; + } }return ret |apply_filter (path, src, len, dst, filter); } +int convert_to_working_tree (const char *path,const char *src,size_t len,struct strbuf *dst) +{ +return convert_to_working_tree_internal (path, src, len, dst,0 ); +} +int renormalize_buffer (const char *path,const char *src,size_t len,struct strbuf *dst) { -int ret =convert_to_working_tree (path, src, len, dst); +int ret =convert_to_working_tree_internal (path, src, len, dst,1 );if (ret) { src = dst->buf; len = dst->len;
Signature · 1 line
1.7.2.rc1.4.g09d06