Merge renormalization, config renamed
v6 cover Merge renormalization, config renamed
Here's a new version of the merge normalization series that renames the configuration variable. Since "merge.renormalize" got the best response, I went with that.
Junio, I hope you don't mind that I squashed your patch to introduce the config variable into the first patch. It seemed a bit out-of-place as a separate commit.
- 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
Documentation/gitattributes.txt | 34 ++++++++++++++++++++ Documentation/merge-config.txt | 10 ++++++ builtin/merge.c | 3 ++ cache.h | 2 + convert.c | 37 +++++++++++++++++++---- environment.c | 1 + ll-merge.c | 15 +++++++++ merge-recursive.c | 51 +++++++++++++++++++++++++++++- t/t6038-merge-text-auto.sh | 64 +++++++++++++++++++++++++++++++++++++++ 9 files changed, 209 insertions(+), 8 deletions(-) create mode 100755 t/t6038-merge-text-auto.sh
Signature · 1 line
1.7.1.575.g383de
PATCH v6 1/3 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.
This feature is optional for now since it changes a low-level mechanism and is not necessary for the majority of users. The "merge.renormalize" config variable enables it.
--- Documentation/gitattributes.txt | 34 ++++++++++++++++++++ Documentation/merge-config.txt | 10 ++++++ builtin/merge.c | 3 ++ cache.h | 2 + convert.c | 16 ++++++++- environment.c | 1 + ll-merge.c | 15 +++++++++ t/t6038-merge-text-auto.sh | 64 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 143 insertions(+), 2 deletions(-) create mode 100755 t/t6038-merge-text-auto.sh
Documentation/gitattributes.txt +34 -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,29 @@ 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 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 by setting the `merge.renormalize` +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. + +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 ~~~~~~~~~~~~~~~~~~~~
Documentation/merge-config.txt +10 -0
@@ -15,6 +15,16 @@ merge.renameLimit:: during a merge; if not specified, defaults to the value of diff.renameLimit. +merge.renormalize:: + 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]. + merge.stat:: Whether to print the diffstat between ORIG_HEAD and the merge result at the end of the merge. True by default.
builtin/merge.c +3 -0
@@ -503,6 +503,9 @@ static int git_merge_config(const char *k, const char *v, void *cb)return git_config_string (&pull_octopus, k, v); else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) option_log =git_config_bool (k, v); + else if (!strcmp(k, "merge.renormalize")) { + merge_renormalize =git_config_bool (k, v); + }return git_diff_ui_config (k, v, cb); }
cache.h +2 -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 merge_renormalize;enum safe_crlf {SAFE_CRLF_FALSE =0 ,
@@ -1054,6 +1055,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 ); +}
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 merge_renormalize;/* Parallel index stat data preload? */ int core_preload_index =0 ;
ll-merge.c +15 -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,11 @@ int ll_merge(mmbuffer_t *result_buf,const struct ll_merge_driver *driver;int virtual_ancestor = flag &01 ; +if (merge_renormalize) { +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 +64 -0
64 lines not on this page · raw message
Signature · 1 line
1.7.1.575.g383de
PATCH v6 2/3 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 (!merge_renormalize) +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
@@ -51,7 +51,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.1.575.g383de
PATCH v6 3/3 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.1.575.g383de
Quoted 7 lines · Eyvind Bernhardsen
Here's a new version of the merge normalization series that renames the configuration variable. Since "merge.renormalize" got the best response, I went with that.
Junio, I hope you don't mind that I squashed your patch to introduce the config variable into the first patch. It seemed a bit out-of-place as a separate commit.
It was meant to be squashed into the first one. Thanks.
cover merge -Xrenormalize
In reply to Eyvind BernhardsenHi Eyvind,
Quoted 3 lines · Eyvind Bernhardsen
Here's a new version of the merge normalization series that renames the configuration variable. Since "merge.renormalize" got the best response, I went with that.
I have no idea how the following branch happened in my local git tree, but it’s here now so maybe it’s worth something. Somehow I got the idea that the merge_renormalize variable was a nice short-term protection but long-term scary, and so a few patches emerged to banish it.
Please let me emphasize that in their current form I do not think these patches should be very usable.
But for later, they tell a nice story. The idea is that the "merge.renormalize" is not really about the behavior of the "git merge" command but about the merge-recursive driver; and so it should be usable everywhere else that driver is used, too.
So in an ideal world, you would be able to, e.g.:
git checkout -m -Xrenormalize otherbranch
or
git revert -Xrenormalize otherpatch
or
git pull --rebase -Xrenormalize
Of course, the plumbing is not all there yet; many commands that use merge drivers cannot pass through arbitrary options through yet.
Well, enough talking. Here are the patches; what do you think?
Patch 1 teaches blob_unchanged() to stop paying attention to the merge_recursive variable. It uses a parameter instead. So from then on, it can watch, bemused, a neutral party.
Patch 2 adds and respects a renormalize option in the merge_options struct. It defaults to the value of the merge_recursive global to save callers (in particular, "git merge") the pain of adjusting.
Patch 3 lets ll_merge merge callers decide whether to renormalize, too. Most callers never renormalize, since they are not "git merge" (and this is noted in new comments).
Patch 4 is a sort of a digression. It makes "git rerere" produce nice help output with the "git rerere -h" option.
Patch 5 lets rerere callers decide whether to renormalize, too. Most callers never renormalize. But looking at that patch reveals a serious problem: there are all kinds of other merge options (e.g., -Xsubtree) that rerere callers are unable to control.
Following the principle that scripted users should have about as much power as internal ones, patch 5 reluctantly exposes the renormalize option to rerere as a new --renormalize option. It should be -Xrenormalize instead.
Patch 6 eliminates the merge_recursive global once and for all, by teaching merge_recursive callers to set the renormalize merge option appropriately (and removing the backward-compatibility default). Callers that are not "git merge" still never use renormalize.
Following the principle that scripted users should have as much power as internal ones, it also exposes the nice interface of an -Xrenormalize option. Some redundancy between builtin/merge and builtin/merge-recursive is noticed but left for another topic.
Jonathan Nieder (6):
merge-trees: push choice to renormalize away from low level
merge-trees: let caller decide whether to renormalize
ll-merge: let caller decide whether to renormalize
rerere: migrate to parse-options API
rerere: let caller decide whether to renormalize
merge-recursive: add -Xrenormalize option
Documentation/merge-strategies.txt | 8 +++++ builtin/checkout.c | 13 ++++++++- builtin/commit.c | 4 ++ builtin/merge-recursive.c | 2 + builtin/merge.c | 24 +++++++++++---- builtin/rerere.c | 56 ++++++++++++++++++++--------------- builtin/revert.c | 18 ++++++++++- cache.h | 1 - environment.c | 1 - ll-merge.c | 4 +- ll-merge.h | 2 +- merge-file.c | 2 +- merge-recursive.c | 11 ++++-- merge-recursive.h | 1 + rerere.c | 20 ++++++++---- rerere.h | 1 + 16 files changed, 117 insertions(+), 51 deletions(-)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 1/6 merge-trees: push choice to renormalize away from low level
The merge machinery decides whether to resmudge and clean relevant entries based on the global merge_renormalize setting, which is set by "git merge" based on its configuration (and left alone by other commands).
A nicer interface would make that decision a parameter to merge_trees so callers would pass in a choice made on a call-by-call basis. Start by making blob_unchanged stop examining the merge_renormalize global.
In other words, this change is a trivial no-op, but it brings us closer to something good.
--- merge-recursive.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
merge-recursive.c +5 -4
@@ -1074,7 +1074,7 @@ static int read_sha1_strbuf(const unsigned char *sha1, struct strbuf *dst)static int blob_unchanged (const unsigned char *o_sha,const unsigned char *a_sha, -const char *path) +int renormalize,const char *path) {struct strbuf o =STRBUF_INIT ;struct strbuf a =STRBUF_INIT ;
@@ -1082,7 +1082,7 @@ static int blob_unchanged(const unsigned char *o_sha,if (sha_eq (o_sha, a_sha))return 1 ; -if (!merge_renormalize) +if (!renormalize)return 0 ;assert (o_sha && a_sha);
@@ -1112,6 +1112,7 @@ static int process_entry(struct merge_options *o,print_index_entry("\tpath: ", entry); */ int clean_merge =1 ; +int normalize = merge_renormalize;unsigned o_mode = entry->stages[1 ].mode;unsigned a_mode = entry->stages[2 ].mode;unsigned b_mode = entry->stages[3 ].mode;
@@ -1122,8 +1123,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) || - (!b_sha &&blob_unchanged (o_sha, a_sha, path)) || - (!a_sha &&blob_unchanged (o_sha, b_sha, path))) { + (!b_sha &&blob_unchanged (o_sha, a_sha, normalize, path)) || + (!a_sha &&blob_unchanged (o_sha, b_sha, normalize, path))) {/* Deleted in both or deleted in one and * unchanged in the other */ if (a_sha)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 2/6 merge-trees: let caller decide whether to renormalize
In reply to Jonathan NiederAdd a "renormalize" option to struct merge_options so callers can decide on a case-by-case basis whether the merge is likely to have overlapped with a change in smudge/clean rules. The option defaults to the global merge_renormalize setting for now.
No change in behavior intended.
--- merge-recursive.c | 3 ++- merge-recursive.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
merge-recursive.c +2 -1
@@ -1112,7 +1112,7 @@ static int process_entry(struct merge_options *o,print_index_entry("\tpath: ", entry); */ int clean_merge =1 ; -int normalize = merge_renormalize; +int normalize = o->renormalize;unsigned o_mode = entry->stages[1 ].mode;unsigned a_mode = entry->stages[2 ].mode;unsigned b_mode = entry->stages[3 ].mode;
@@ -1484,6 +1484,7 @@ void init_merge_options(struct merge_options *o) o->buffer_output =1 ; o->diff_rename_limit =-1 ; o->merge_rename_limit =-1 ; + o->renormalize = merge_renormalize;git_config (merge_recursive_config, o);if (getenv ("GIT_MERGE_VERBOSITY" )) o->verbosity =
merge-recursive.h +1 -0
@@ -17,6 +17,7 @@ struct merge_options {int verbosity;int diff_rename_limit;int merge_rename_limit; +int renormalize;int call_depth;struct strbuf obuf;struct string_list current_file_set;
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 3/6 ll-merge: let caller decide whether to renormalize
In reply to Jonathan NiederAdd a "renormalize" parameter to ll_merge so callers can decide on a case-by-case basis whether the merge is likely to have overlapped with a change in smudge/clean rules. This reveals a few commands that have not been taking that situation into account, though it does not fix them.
No change in behavior intended.
--- builtin/checkout.c | 6 +++++- ll-merge.c | 4 ++-- ll-merge.h | 2 +- merge-file.c | 2 +- merge-recursive.c | 1 + rerere.c | 17 +++++++++++------ 6 files changed, 21 insertions(+), 11 deletions(-)
builtin/checkout.c +5 -1
@@ -150,8 +150,12 @@ static int checkout_merged(int pos, struct checkout *state)read_mmblob (&ours, active_cache[pos+1 ]->sha1);read_mmblob (&theirs, active_cache[pos+2 ]->sha1); +/* +* NEEDSWORK: re-create conflicts from merges with +* merge.renormalize set, too +*/ status =ll_merge (&result_buf, path, &ancestor,"base" , - &ours,"ours" , &theirs,"theirs" ,0 ); + &ours,"ours" , &theirs,"theirs" ,0 ,0 );free (ancestor.ptr);free (ours.ptr);free (theirs.ptr);
ll-merge.c +2 -2
@@ -336,7 +336,7 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *ancestor, const char *ancestor_label, mmfile_t *ours, const char *our_label, mmfile_t *theirs, const char *their_label, - int flag) + int renormalize, int flag) { static struct git_attr_check check[2];const char *ll_driver_name =NULL ;
@@ -344,7 +344,7 @@ int ll_merge(mmbuffer_t *result_buf,const struct ll_merge_driver *driver;int virtual_ancestor = flag &01 ; -if (merge_renormalize) { +if (renormalize) {normalize_file (ancestor, path);normalize_file (ours, path);normalize_file (theirs, path);
ll-merge.h +1 -1
@@ -10,7 +10,7 @@ int ll_merge(mmbuffer_t *result_buf, mmfile_t *ancestor, const char *ancestor_label, mmfile_t *ours, const char *our_label, mmfile_t *theirs, const char *their_label, - int flag); + int renormalize, int flag);int ll_merge_marker_size (const char *path);
merge-file.c +1 -1
@@ -37,7 +37,7 @@ static void *three_way_filemerge(const char *path, mmfile_t *base, mmfile_t *our* common ancestor. */ merge_status =ll_merge (&res, path, base,NULL , - our,".our" , their,".their" ,0 ); + our,".our" , their,".their" ,0 ,0 );if (merge_status <0 )return NULL ;
merge-recursive.c +1 -0
@@ -647,6 +647,7 @@ static int merge_3way(struct merge_options *o, merge_status =ll_merge (result_buf, a->path, &orig, base_name, &src1, name1, &src2, name2, + o->renormalize, (!!o->call_depth) | (favor <<1 ));free (name1);
rerere.c +11 -6
@@ -319,9 +319,13 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpuif (!mmfile[i].ptr && !mmfile[i].size) mmfile[i].ptr =xstrdup ("" ); } +/* +* NEEDSWORK: handle conflicts from merges with +* merge.renormalize set, too +*/ ll_merge (&result, path, &mmfile[0 ],NULL , &mmfile[1 ],"ours" , - &mmfile[2 ],"theirs" ,0 ); + &mmfile[2 ],"theirs" ,0 ,0 );for (i =0 ; i <3 ; i++)free (mmfile[i].ptr);
@@ -361,7 +365,7 @@ static int find_conflict(struct string_list *conflict)return 0 ; } -static int merge (const char *name,const char *path) +static int merge (const char *name,int renormalize,const char *path) {int ret;mmfile_t cur = {NULL ,0 }, base = {NULL ,0 }, other = {NULL ,0 };
@@ -376,7 +380,8 @@ static int merge(const char *name, const char *path) ret =1 ; goto out; } - ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" ,0 ); + ret =ll_merge (&result, path, &base,NULL , + &cur,"" , &other,"" , renormalize,0 );if (!ret) {FILE *f =fopen (path,"w" );if (!f)
@@ -424,7 +429,7 @@ static int update_paths(struct string_list *update)return status; } -static int do_plain_rerere (struct string_list *rr,int fd) +static int do_plain_rerere (struct string_list *rr,int fd,int renormalize) {struct string_list conflict = {NULL ,0 ,0 ,1 };struct string_list update = {NULL ,0 ,0 ,1 };
@@ -469,7 +474,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)const char *name = (const char *)rr->items[i].util;if (has_rerere_resolution (name)) { -if (!merge (name, path)) { +if (!merge (name, renormalize, path)) {if (rerere_autoupdate)string_list_insert (path, &update); fprintf(stderr,
@@ -553,7 +558,7 @@ int rerere(int flags) fd =setup_rerere (&merge_rr, flags);if (fd <0 )return 0 ; -return do_plain_rerere (&merge_rr, fd); +return do_plain_rerere (&merge_rr, fd, merge_renormalize); }static int rerere_forget_one_path (const char *path,struct string_list *rr)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
Re: [PATCH 3/6] ll-merge: let caller decide whether to renormalize
Quoted 7 lines · Jonathan Nieder
Add a "renormalize" parameter to
ll_mergeso callers can decide on a case-by-case basis whether the merge is likely to have overlapped with a change in smudge/clean rules. This reveals a few commands that have not been taking that situation into account, though it does not fix them.No change in behavior intended.
Looking nice so far; I however think a separate "renormalize" parameter should be folded into "flag" word, perhaps with a few new macros to make it easier to access renormalize, virtual-ancestor, and favor-side parts it.
PATCH 4/6 rerere: migrate to parse-options API
In reply to Jonathan Nieder--- builtin/rerere.c | 52 ++++++++++++++++++++++++++++------------------------ 1 files changed, 28 insertions(+), 24 deletions(-)
builtin/rerere.c +28 -24
@@ -1,13 +1,16 @@#include "builtin.h" #include "cache.h" #include "dir.h" +#include "parse-options.h" #include "string-list.h" #include "rerere.h" #include "xdiff/xdiff.h" #include "xdiff-interface.h" -static const char git_rerere_usage[] = -"git rerere [clear | status | diff | gc]" ; +static const char *const rerere_usage[] = { +"git rerere [clear | status | diff | gc]" , +NULL , +};/* these values are days */ static int cutoff_noresolve =15 ;
@@ -103,25 +106,26 @@ static int diff_two(const char *file1, const char *label1,int cmd_rerere (int argc,const char **argv,const char *prefix) {struct string_list merge_rr = {NULL ,0 ,0 ,1 }; -int i, fd, flags =0 ; - -if (2 < argc) { -if (!strcmp (argv[1 ],"-h" )) -usage (git_rerere_usage); -if (!strcmp (argv[1 ],"--rerere-autoupdate" )) - flags =RERERE_AUTOUPDATE ; -else if (!strcmp (argv[1 ],"--no-rerere-autoupdate" )) - flags =RERERE_NOAUTOUPDATE ; -if (flags) { - argc--; - argv++; - } - } -if (argc <2 ) +int i, fd, autoupdate =-1 , flags =0 ; + +struct option options[] = { +OPT_SET_INT (0 ,"rerere-autoupdate" , &autoupdate, +"register clean resolutions in index" ,1 ), +OPT_END (), + }; + + argc =parse_options (argc, argv, prefix, options, rerere_usage,0 ); + +if (autoupdate ==1 ) + flags =RERERE_AUTOUPDATE ; +if (autoupdate ==0 ) + flags =RERERE_NOAUTOUPDATE ; + +if (argc <1 )return rerere (flags); -if (!strcmp (argv[1 ],"forget" )) { -const char **pathspec =get_pathspec (prefix, argv +2 ); +if (!strcmp (argv[0 ],"forget" )) { +const char **pathspec =get_pathspec (prefix, argv +1 );return rerere_forget (pathspec); }
@@ -129,26 +133,26 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)if (fd <0 )return 0 ; -if (!strcmp (argv[1 ],"clear" )) { +if (!strcmp (argv[0 ],"clear" )) {for (i =0 ; i < merge_rr.nr; i++) {const char *name = (const char *)merge_rr.items[i].util;if (!has_rerere_resolution (name))unlink_rr_item (name); }unlink_or_warn (git_path ("rr-cache/MERGE_RR" )); - }else if (!strcmp (argv[1 ],"gc" )) + }else if (!strcmp (argv[0 ],"gc" ))garbage_collect (&merge_rr); -else if (!strcmp (argv[1 ],"status" )) +else if (!strcmp (argv[0 ],"status" ))for (i =0 ; i < merge_rr.nr; i++)printf ("%s\n" , merge_rr.items[i].string); -else if (!strcmp (argv[1 ],"diff" )) +else if (!strcmp (argv[0 ],"diff" ))for (i =0 ; i < merge_rr.nr; i++) {const char *path = merge_rr.items[i].string;const char *name = (const char *)merge_rr.items[i].util;diff_two (rerere_path (name,"preimage" ), path, path, path); }else -usage (git_rerere_usage); +usage_with_options (rerere_usage, options);string_list_clear (&merge_rr,1 );return 0 ;
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 5/6 rerere: let caller decide whether to renormalize
In reply to Jonathan NiederAdd a RERERE_RENORMALIZE flag to rerere so callers can decide case-by-case whether the merge is likely to have overlapped with a change in smudge/clean rules.
This is only a change in internal plumbing. Many callers do not have a way to use that setting; this patch does not change that.
NEEDSWORK: this is a step in the wrong direction. rerere needs an -s option to use an arbitrary merge strategy and a -X option to pass arbitrary options to that driver. And maybe the options used in a merge should be recorded somewhere to help rerere repeat it.
This treats the renormalize option specially anyway, to support the existing "git merge" behavior of using -Xrenormalize in its rerere call when configured to do so. It reluctant exposes that option in the rerere command as --renormalize for the sake of experimentation, too, but it does not advertise it.
--- builtin/commit.c | 4 ++++ builtin/merge.c | 7 +++++-- builtin/rerere.c | 8 ++++++-- builtin/revert.c | 11 +++++++++-- rerere.c | 5 +++-- rerere.h | 1 + 6 files changed, 28 insertions(+), 8 deletions(-)
builtin/commit.c +4 -0
@@ -1377,6 +1377,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)"new_index file. Check that disk is not full or quota is\n" "not exceeded, and then \"git reset HEAD\" to recover."); +/* +* NEEDSWORK: use RERERE_RENORMALIZE after a merge with +* merge.renormalize set +*/ rerere (0 );run_hook (get_index_file (),"post-commit" ,NULL );if (amend && !no_post_rewrite) {
builtin/merge.c +5 -2
@@ -822,7 +822,7 @@ static int finish_automerge(struct commit_list *common,static int suggest_conflicts (void ) {FILE *fp; -int pos; +int pos, flag; fp =fopen (git_path ("MERGE_MSG" ),"a" );if (!fp)
@@ -841,7 +841,10 @@ static int suggest_conflicts(void) } }fclose (fp); -rerere (allow_rerere_auto); + flag = allow_rerere_auto; +if (merge_renormalize) + flag |=RERERE_RENORMALIZE ; +rerere (flag);printf ("Automatic merge failed; " "fix conflicts and then commit the result.\n" );return 1 ;
builtin/rerere.c +6 -2
@@ -111,15 +111,19 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)struct option options[] = {OPT_SET_INT (0 ,"rerere-autoupdate" , &autoupdate,"register clean resolutions in index" ,1 ), + {OPTION_BIT ,0 ,"renormalize-before-merge" , &flags,NULL , +"simplify conflict hunks for normalization changes" , +PARSE_OPT_NOARG |PARSE_OPT_HIDDEN ,NULL , +RERERE_RENORMALIZE },OPT_END (), }; argc =parse_options (argc, argv, prefix, options, rerere_usage,0 );if (autoupdate ==1 ) - flags =RERERE_AUTOUPDATE ; + flags |=RERERE_AUTOUPDATE ;if (autoupdate ==0 ) - flags =RERERE_NOAUTOUPDATE ; + flags |=RERERE_NOAUTOUPDATE ;if (argc <1 )return rerere (flags);
builtin/revert.c +9 -2
@@ -338,7 +338,7 @@ static void do_recursive_merge(struct commit *base, struct commit *next,rollback_lock_file (&index_lock);if (!clean) { -int i; +int i, flags;strbuf_addstr (msgbuf,"\nConflicts:\n\n" );for (i =0 ; i < active_nr;) {struct cache_entry *ce = active_cache[i++];
@@ -354,7 +354,10 @@ static void do_recursive_merge(struct commit *base, struct commit *next,write_message (msgbuf, defmsg);fprintf (stderr,"Automatic %s failed.%s\n" , me,help_msg ()); -rerere (allow_rerere_auto); + flags = allow_rerere_auto; +if (o.renormalize) + flags |=RERERE_RENORMALIZE ; +rerere (flags);exit (1 ); }write_message (msgbuf, defmsg);
@@ -481,6 +484,10 @@ static int do_pick_commit(void)if (res) {fprintf (stderr,"Automatic %s with strategy %s failed.%s\n" , me, strategy,help_msg ()); +/* +* NEEDSWORK: use RERERE_RENORMALIZE after +* a merge with merge.renormalize set +*/ rerere (allow_rerere_auto);exit (1 ); }
rerere.c +3 -2
@@ -553,12 +553,13 @@ int setup_rerere(struct string_list *merge_rr, int flags)int rerere (int flags) {struct string_list merge_rr = {NULL ,0 ,0 ,1 }; -int fd; +int fd, renormalize; fd =setup_rerere (&merge_rr, flags);if (fd <0 )return 0 ; -return do_plain_rerere (&merge_rr, fd, merge_renormalize); + renormalize = (flags &RERERE_RENORMALIZE ) ?1 :0 ; +return do_plain_rerere (&merge_rr, fd, renormalize); }static int rerere_forget_one_path (const char *path,struct string_list *rr)
rerere.h +1 -0
@@ -5,6 +5,7 @@#define RERERE_AUTOUPDATE 01#define RERERE_NOAUTOUPDATE 02 +#define RERERE_RENORMALIZE 04extern int setup_rerere (struct string_list *,int );extern int rerere (int );
Signature · 1 line
1.7.2.1.544.ga752d.dirty
Re: [PATCH 5/6] rerere: let caller decide whether to renormalize
Quoted 3 lines · Jonathan Nieder
NEEDSWORK: this is a step in the wrong direction. rerere needs an -s option to use an arbitrary merge strategy and a -X option to pass arbitrary options to that driver.
If you are talking about "rerere", I strongly disagree with that.
1. "-s"
A "merge strategy" deals with the shape of the history (e.g. common ancestor selection or synthesis for the purpose of 3-way merge) and the shape of the trees (e.g. rename detection, subtree shifting). Starting from two commits, it decides, based on the shape of the history, what three trees your tree-level 3-way merge would operate on, and then based on the shape of the trees, decides the pairing of blobs to run the 3-way merge at the file-content level.
When "rerere" is invoked, a strategy already has dealt with all of the above, and "rerere" only works on the (half-completed) result of that. It is purely a three-way merge at the file-contents level and there is no room for a "strategy" to get involved. It makes direct calls to ll_merge() exactly for this reason.
2. "-X"
In the "merge -X<opt>" syntax, "-X" does not stand for "low level details"; it just means "eXternal callout". It is there just to tell the "merge" front-end "You may not understand this yourself, but the program you call does, so just pass it along".
IOW, we want to be able to pass --<opt> through the frontend to the backend, without having to tell all the options any possible backends may know to the frontend. Just to make it easier to parse and tell which ones are the front-end options and which ones are not (for both machines and humans), we say -X<opt> to the frontend. Then "merge" turns that -X<opt> into "--<opt>" and give it to the strategy.
A command that natively knows about an option is correct to take an option as "--<option>", e.g. "merge--recursive --renormalize". You trigger it by saying "merge -Xrenormalize" from the frontend.
To recap: it is absolutely the right thing to do to introduce a new "rerere --renormalize" option, like your patch did. Doing anything else IS a step in the wrong direction.
[PATCH/RFC v2 0/12] Re: rerere: let caller decide whether to renormalize
Quoted 1 line · Junio C Hamano
1. "-s"
[...]
Quoted 1 line
When "rerere" is invoked,
[...]
Quoted 5 lines
It is purely a three-way merge at the file-contents level and there is no room for a "strategy" to get involved. It makes direct calls to
ll_merge()exactly for this reason.2. "-X"
[...]
Quoted 8 lines
A command that natively knows about an option is correct to take an option as "--<option>", e.g. "merge--recursive --renormalize". You trigger it by saying "merge -Xrenormalize" from the frontend.
To recap: it is absolutely the right thing to do to introduce a new "rerere --renormalize" option, like your patch did. Doing anything else IS a step in the wrong direction.
Once you say it like that, it makes sense. :)
Or rather, rerere should not take --renormalize at all. Patch 11 below has an explanation.
Jonathan Nieder (12):
t6038 (merge.renormalize): style nitpicks
t6038 (merge.renormalize): try checkout -m and cherry-pick t6038 (merge.renormalize): check that it can be turned off
merge-trees: push choice to renormalize away from low level
merge-trees: let caller decide whether to renormalize Documentation/technical: document ll_merge
ll-merge: make flag easier to populate
ll-merge: let caller decide whether to renormalize t4200 (rerere): modernize style
rerere: migrate to parse-options API
rerere: never renormalize
merge-recursive --renormalize
Documentation/merge-strategies.txt | 12 + Documentation/technical/api-merge.txt | 73 ++++++ builtin/checkout.c | 11 + builtin/merge-recursive.c | 4 + builtin/merge.c | 19 ++- builtin/rerere.c | 52 +++-- builtin/revert.c | 7 + cache.h | 1 - environment.c | 1 - ll-merge.c | 11 +- ll-merge.h | 15 ++ merge-recursive.c | 14 +- merge-recursive.h | 1 + rerere.c | 4 + t/t4200-rerere.sh | 394 ++++++++++++++++++++++----------- t/t6038-merge-text-auto.sh | 143 +++++++++++- 16 files changed, 588 insertions(+), 174 deletions(-) create mode 100644 Documentation/technical/api-merge.txt
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 1/12 t6038 (merge.renormalize): style nitpicks
Some tweaks to simplify adding and running tests.
- Use test_tick for predictable, sort of realistic commit dates;
- Use test_cmp as "test_cmp expected actual" --- some crazy content that was not expected should cause the test to fail;
- Remove and re-add all files at the start of each test so the worktree is easier to think about;
- Avoid using cat where not necessary for clarity.
--- t/t6038-merge-text-auto.sh | 52 +++++++++++++++++++++++++++++++++++++------ 1 files changed, 44 insertions(+), 8 deletions(-)
t/t6038-merge-text-auto.sh +44 -8
@@ -1,58 +1,94 @@ #!/bin/sh -test_description='CRLF merge conflict across text=auto change' +test_description='CRLF merge conflict across text=auto change + +* [master] remove .gitattributes + ! [side] add line from b +-- + + [side] add line from b +* [master] remove .gitattributes +* [master^] add line from a +* [master~2] normalize file +*+ [side^] Initial +' . ./test-lib.sh test_expect_success setup ' git config merge.renormalize true && git config core.autocrlf false && + echo first line | append_cr >file && echo first line >control_file && echo only line >inert_file && + git add file control_file inert_file && + test_tick && git commit -m "Initial" && git tag initial && git branch side && + echo "* text=auto" >.gitattributes && touch file && git add .gitattributes file && + test_tick && git commit -m "normalize file" && + echo same line | append_cr >>file && echo same line >>control_file && git add file control_file && + test_tick && git commit -m "add line from a" && git tag a && + git rm .gitattributes && rm file && git checkout file && + test_tick && git commit -m "remove .gitattributes" && git tag c && + git checkout side && echo same line | append_cr >>file && echo same line >>control_file && git add file control_file && + test_tick && git commit -m "add line from b" && git tag b && + git checkout master ' -test_expect_success 'Check merging after setting text=auto' ' +test_expect_success 'Merge after setting text=auto' ' + cat <<-\EOF >expected && + first line + same line + EOF + + git rm -fr . && + rm -f .gitattributes && git reset --hard a && git merge b && - cat file | remove_cr >file.temp && - test_cmp file file.temp + test_cmp expected file ' -test_expect_success 'Check merging addition of text=auto' ' +test_expect_success 'Merge addition of text=auto' ' + cat <<-\EOF >expected && + first line + same line + EOF + + git rm -fr . && + rm -f .gitattributes && git reset --hard b && git merge a && - cat file | remove_cr >file.temp && - test_cmp file file.temp + test_cmp expected file ' test_expect_success 'Test delete/normalize conflict' ' - git checkout side && + git checkout -f side && + git rm -fr . && + rm -f .gitattributes && git reset --hard initial && git rm file && git commit -m "remove file" &&
Signature · 1 line
1.7.2.1.544.ga752d.dirty
Re: [PATCH 01/12] t6038 (merge.renormalize): style nitpicks
Quoted 5 lines · Jonathan Nieder
Some tweaks to simplify adding and running tests.
- Use
test_tickfor predictable, sort of realistic commit dates;
Quoted 7 lines
git checkout side &&
echo same line |append_cr>>file &&
echo same line >>control_file &&
git add filecontrol_file&&
+test_tick&&
git commit -m "add line from b" &&
git tag b &&
FWIW this looks like it could use Dmitry's "test-lib.sh: introduce 4th argument to test_commit() specifying a tag name" patch. Maybe that goes for most of these git add/tick/commit/tag combos, i.e. they don't really need $commit_message != $tagname.
Re: [PATCH 01/12] t6038 (merge.renormalize): style nitpicks
Quoted 10 lines · Ævar Arnfjörð Bjarmason
Quoted 7 lines · Jonathan Nieder
git checkout side &&
echo same line |append_cr>>file &&
echo same line >>control_file &&
git add filecontrol_file&&
+test_tick&&
git commit -m "add line from b" &&
git tag b &&
Quoted 2 lines
FWIW this looks like it could use Dmitry's "
test-lib.sh: introduce 4th argument totest_commit()specifying a tag name" patch.
In this example I am not confident the file has content suitable for echo.
The discussion brings to mind something[1] I thought wise in a different context:
“I mentioned earlier that UNIX was not especially suited to applications involving vast quantities of data. The reason is this: files are limited in size to 64K bytes. The reason for this is not particularly defensible, but it has to do with the fact that the PDP-11 word size is 16 bits.
There are a couple of ways around this problem. One of them is simply to split one large logical file into several smaller actual files. This approach works for a while. The limitation here comes from the fact that directories are searched in a linear fashion. Thus if the are a vast number of files, it can become quite time-consuming tosearch directories to find the files they contain. We have not noticed this to be a problem, so far, it is only a worry.
Another way around the small file size is to use a disk as a special file. For various reasons, when an entire disk drive is accessed as a special file, the size limitation does not occur. Thus one can set up a program which manages its own data-- in effect is its own, special-purpose file system-- and expect reasonable results.
This again bears on the general versus special purpose
system: it probably is more efficient anyway to do your own data management, provided the extra labor is worth the cost.”
Of course the tradeoffs are completely different here but it is worth bearing in mind the underlying process: sometimes a too general facility only gets in the way unless all the facets of how it should be used have been carefully understood (i.e., good interfaces sometimes evolve by excluding the special cases until the missed benefit from not including them is overwhelming).
Sorry for the ramble. Another way to say it: I am happy to see test_commit be made more useful, but if extra-weird cases do not fit it, please do not take that as a failing.
PATCH 2/12 t6038 (merge.renormalize): try checkout -m and cherry-pick
In reply to Jonathan Niedercheckout -m and cherry-pick have not been wired up to respect merge.renormalize, but a naïve user would not know that.
---
This series does not change these to expect success, but they are part of the same thought process.
t/t6038-merge-text-auto.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 41 insertions(+), 0 deletions(-)
t/t6038-merge-text-auto.sh +41 -0
@@ -85,6 +85,47 @@ test_expect_success 'Merge addition of text=auto' ' test_cmp expected file ' +test_expect_failure 'checkout -m after setting text=auto' ' + cat <<-\EOF >expected && + first line + same line + EOF + + git rm -fr . && + rm -f .gitattributes && + git reset --hard initial && + git checkout a -- . && + git checkout -m b && + test_cmp expected file +' + +test_expect_failure 'checkout -m addition of text=auto' ' + cat <<-\EOF >expected && + first line + same line + EOF + + git rm -fr . && + rm -f .gitattributes file && + git reset --hard initial && + git checkout b -- . && + git checkout -m a && + test_cmp expected file +' + +test_expect_failure 'cherry-pick patch from after text=auto was added' ' + append_cr <<-\EOF >expected && + first line + same line + EOF + + git rm -fr . && + git reset --hard b && + test_must_fail git cherry-pick a >err 2>&1 && + grep "[Nn]othing added" err && + test_cmp expected file +' + test_expect_success 'Test delete/normalize conflict' ' git checkout -f side && git rm -fr . &&
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 3/12 t6038 (merge.renormalize): check that it can be turned off
In reply to Jonathan NiederAn unusual sort of person (not me) may even enjoy the conflicts from line-ending changes. But more importantly, it is useful to document that behavior so we can more easily notice if it changes in an uncontrolled way while no one is watching.
--- t/t6038-merge-text-auto.sh | 50 +++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 49 insertions(+), 1 deletions(-)
t/t6038-merge-text-auto.sh +49 -1
@@ -15,7 +15,6 @@ test_description='CRLF merge conflict across text=auto change . ./test-lib.sh test_expect_success setup ' - git config merge.renormalize true && git config core.autocrlf false && echo first line | append_cr >file &&
@@ -59,12 +58,19 @@ test_expect_success setup ' git checkout master ' +test_expect_success 'set up fuzz_conflict() helper' ' + fuzz_conflict() { + sed -e "s/^\([<>=]......\) .*/\1/" "$@" + } +' + test_expect_success 'Merge after setting text=auto' ' cat <<-\EOF >expected && first line same line EOF + git config merge.renormalize true && git rm -fr . && rm -f .gitattributes && git reset --hard a &&
@@ -78,6 +84,7 @@ test_expect_success 'Merge addition of text=auto' ' same line EOF + git config merge.renormalize true && git rm -fr . && rm -f .gitattributes && git reset --hard b &&
@@ -85,12 +92,51 @@ test_expect_success 'Merge addition of text=auto' ' test_cmp expected file ' +test_expect_success 'Detect CRLF/LF conflict after setting text=auto' ' + q_to_cr <<-\EOF >expected && + <<<<<<< + first line + same line + ======= + first lineQ + same lineQ + >>>>>>> + EOF + + git config merge.renormalize false && + rm -f .gitattributes && + git reset --hard a && + test_must_fail git merge b && + fuzz_conflict file >file.fuzzy && + test_cmp expected file.fuzzy +' + +test_expect_success 'Detect LF/CRLF conflict from addition of text=auto' ' + q_to_cr <<-\EOF >expected && + <<<<<<< + first lineQ + same lineQ + ======= + first line + same line + >>>>>>> + EOF + + git config merge.renormalize false && + rm -f .gitattributes && + git reset --hard b && + test_must_fail git merge a && + fuzz_conflict file >file.fuzzy && + test_cmp expected file.fuzzy +' + test_expect_failure 'checkout -m after setting text=auto' ' cat <<-\EOF >expected && first line same line EOF + git config merge.renormalize true && git rm -fr . && rm -f .gitattributes && git reset --hard initial &&
@@ -105,6 +151,7 @@ test_expect_failure 'checkout -m addition of text=auto' ' same line EOF + git config merge.renormalize true && git rm -fr . && rm -f .gitattributes file && git reset --hard initial &&
@@ -119,6 +166,7 @@ test_expect_failure 'cherry-pick patch from after text=auto was added' ' same line EOF + git config merge.renormalize true && git rm -fr . && git reset --hard b && test_must_fail git cherry-pick a >err 2>&1 &&
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 4/12 merge-trees: push choice to renormalize away from low level
In reply to Jonathan NiederThe merge machinery decides whether to resmudge and clean relevant entries based on the global merge_renormalize setting, which is set by "git merge" based on its configuration (and left alone by other commands).
A nicer interface would make that decision a parameter to merge_trees so callers would pass in a choice made on a call-by-call basis. Start by making blob_unchanged stop examining the merge_renormalize global.
In other words, this change is a trivial no-op, but it brings us closer to something good.
---
Unchanged from v1.
merge-recursive.c | 9 +++++---- 1 files changed, 5 insertions(+), 4 deletions(-)
merge-recursive.c +5 -4
@@ -1074,7 +1074,7 @@ static int read_sha1_strbuf(const unsigned char *sha1, struct strbuf *dst)static int blob_unchanged (const unsigned char *o_sha,const unsigned char *a_sha, -const char *path) +int renormalize,const char *path) {struct strbuf o =STRBUF_INIT ;struct strbuf a =STRBUF_INIT ;
@@ -1082,7 +1082,7 @@ static int blob_unchanged(const unsigned char *o_sha,if (sha_eq (o_sha, a_sha))return 1 ; -if (!merge_renormalize) +if (!renormalize)return 0 ;assert (o_sha && a_sha);
@@ -1112,6 +1112,7 @@ static int process_entry(struct merge_options *o,print_index_entry("\tpath: ", entry); */ int clean_merge =1 ; +int normalize = merge_renormalize;unsigned o_mode = entry->stages[1 ].mode;unsigned a_mode = entry->stages[2 ].mode;unsigned b_mode = entry->stages[3 ].mode;
@@ -1122,8 +1123,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) || - (!b_sha &&blob_unchanged (o_sha, a_sha, path)) || - (!a_sha &&blob_unchanged (o_sha, b_sha, path))) { + (!b_sha &&blob_unchanged (o_sha, a_sha, normalize, path)) || + (!a_sha &&blob_unchanged (o_sha, b_sha, normalize, path))) {/* Deleted in both or deleted in one and * unchanged in the other */ if (a_sha)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 5/12 merge-trees: let caller decide whether to renormalize
In reply to Jonathan NiederAdd a "renormalize" option to struct merge_options so callers can decide on a case-by-case basis whether the merge is likely to have overlapped with a change in smudge/clean rules. The option defaults to the global merge_renormalize setting for now.
No change in behavior intended.
---
The renormalize option now is advertized to only require one bit. Maybe some optimizer will take advantage of that.
merge-recursive.c | 3 ++- merge-recursive.h | 1 + 2 files changed, 3 insertions(+), 1 deletions(-)
merge-recursive.c +2 -1
@@ -1112,7 +1112,7 @@ static int process_entry(struct merge_options *o,print_index_entry("\tpath: ", entry); */ int clean_merge =1 ; -int normalize = merge_renormalize; +int normalize = o->renormalize;unsigned o_mode = entry->stages[1 ].mode;unsigned a_mode = entry->stages[2 ].mode;unsigned b_mode = entry->stages[3 ].mode;
@@ -1484,6 +1484,7 @@ void init_merge_options(struct merge_options *o) o->buffer_output =1 ; o->diff_rename_limit =-1 ; o->merge_rename_limit =-1 ; + o->renormalize = merge_renormalize;git_config (merge_recursive_config, o);if (getenv ("GIT_MERGE_VERBOSITY" )) o->verbosity =
merge-recursive.h +1 -0
@@ -14,6 +14,7 @@ struct merge_options { } recursive_variant;const char *subtree_shift; unsigned buffer_output : 1; + unsigned renormalize : 1;int verbosity;int diff_rename_limit;int merge_rename_limit;
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 6/12 Documentation/technical: document ll_merge
In reply to Jonathan Nieder---
Sane?
Documentation/technical/api-merge.txt | 70 +++++++++++++++++++++++++++++++++ 1 files changed, 70 insertions(+), 0 deletions(-) create mode 100644 Documentation/technical/api-merge.txt
Documentation/technical/api-merge.txt new file +70 -0
@@ -0,0 +1,70 @@ +merge API +========= + +The merge API helps a program to reconcile two competing sets of +improvements to some files (e.g., unregistered changes from the work +tree versus changes involved in switching to a new branch), reporting +conflicts if found. The library called through this API is +responsible for a few things. + + * determining which trees to merge (recursive ancestor consolidation); + + * lining up corresponding files in the trees to be merged (rename + detection, subtree shifting), reporting edge cases like add/add + and rename/rename conflicts to the user; + + * performing a three-way merge of corresponding files, taking + path-specific merge drivers (specified in `.gitattributes`) + into account. + +Low-level (single file) merge +----------------------------- + +`ll_merge`:: + + Perform a three-way single-file merge in core. This is + a thin wrapper around `xdl_merge` that takes the path and + any merge backend specified in `.gitattributes` or + `.git/info/attributes` into account. Returns 0 for a + clean merge. + +The caller: + +1. allocates an mmbuffer_t variable for the result; +2. allocates and fills variables with the file's original content + and two modified versions (using `read_mmfile`, for example); +3. calls ll_merge(); +4. reads the output from result_buf.ptr and result_buf.size; +5. releases buffers when finished (free(ancestor.ptr); free(ours.ptr); + free(theirs.ptr); free(result_buf.ptr);). + +If the modifications do not merge cleanly, `ll_merge` will return a +nonzero value and `result_buf` will generally include a description of +the conflict bracketed by markers such as the traditional `<<<<<<<` +and `>>>>>>>`. + +The `ancestor_label`, `our_label`, and `their_label` parameters are +used to label the different sides of a conflict if the merge driver +supports this. + +The `flag` parameter is a bitfield: + + - The least significant bit indicates whether this is an internal + merge to consolidate ancestors for a recursive merge. + + - The next two bits allow local conflicts to be automatically + resolved in favor of one side or the other (as in 'git merge-file' + `--ours`/`--theirs`/`--union` for 01, 10, and 11, respectively). + +Everything else +--------------- + +Talk about <merge-recursive.h> and merge_file(): + + - merge_trees() to merge with rename detection + - merge_recursive() for ancestor consolidation + - try_merge_command() for other strategies + - conflict format + - merge options + +(Daniel, Miklos, Stephan, JC)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 7/12 ll-merge: make flag easier to populate
In reply to Jonathan Niederll_merge() takes its options in a flag word, which has a few advantages:
- options flags can be cheaply passed around in registers, while an option struct passed by pointer cannot;
- callers can easily pass 0 without trouble for no options, while an option struct passed by value would not allow that.
The downside is that code to populate and access the flag word can be somewhat opaque. Mitigate that with a few macros.
--- Documentation/technical/api-merge.txt | 11 +++++++---- ll-merge.c | 9 +++++---- ll-merge.h | 14 ++++++++++++++ merge-recursive.c | 3 ++- 4 files changed, 28 insertions(+), 9 deletions(-)
Documentation/technical/api-merge.txt +7 -4
@@ -49,12 +49,15 @@ supports this. The `flag` parameter is a bitfield: - - The least significant bit indicates whether this is an internal - merge to consolidate ancestors for a recursive merge. + - The `LL_OPT_VIRTUAL_ANCESTOR` bit indicates whether this is an + internal merge to consolidate ancestors for a recursive merge. - - The next two bits allow local conflicts to be automatically + - The `LL_OPT_FAVOR_MASK` bits allow local conflicts to be automatically resolved in favor of one side or the other (as in 'git merge-file' - `--ours`/`--theirs`/`--union` for 01, 10, and 11, respectively). + `--ours`/`--theirs`/`--union`). + They can be populated by `create_ll_flag`, whose argument can be + `XDL_MERGE_FAVOR_OURS`, `XDL_MERGE_FAVOR_THEIRS`, or + `XDL_MERGE_FAVOR_UNION`. Everything else ---------------
ll-merge.c +5 -4
@@ -46,7 +46,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,* or common ancestor for an internal merge. Still return * "conflicted merge" status. */ -mmfile_t *stolen = (flag &01 ) ? orig : src1; +mmfile_t *stolen = (flag &LL_OPT_VIRTUAL_ANCESTOR ) ? orig : src1; result->ptr = stolen->ptr; result->size = stolen->size;
@@ -79,7 +79,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused,memset (&xmp,0 ,sizeof (xmp)); xmp.level =XDL_MERGE_ZEALOUS ; - xmp.favor= (flag >>1 ) &03 ; + xmp.favor =ll_opt_favor (flag);if (git_xmerge_style >=0 ) xmp.style = git_xmerge_style;if (marker_size >0 )
@@ -99,7 +99,8 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused, int flag, int marker_size) {/* Use union favor */ - flag = (flag &1 ) | (XDL_MERGE_FAVOR_UNION <<1 ); + flag = (flag &LL_OPT_VIRTUAL_ANCESTOR ) | +create_ll_flag (XDL_MERGE_FAVOR_UNION );return ll_xdl_merge (drv_unused, result, path_unused, orig,NULL , src1,NULL , src2,NULL , flag, marker_size);
@@ -342,7 +343,7 @@ int ll_merge(mmbuffer_t *result_buf,const char *ll_driver_name =NULL ;int marker_size =DEFAULT_CONFLICT_MARKER_SIZE ;const struct ll_merge_driver *driver; -int virtual_ancestor = flag &01 ; +int virtual_ancestor = flag &LL_OPT_VIRTUAL_ANCESTOR ;if (merge_renormalize) {normalize_file (ancestor, path);
ll-merge.h +14 -0
@@ -5,6 +5,20 @@#ifndef LL_MERGE_H #define LL_MERGE_H +#define LL_OPT_VIRTUAL_ANCESTOR (1 << 0) +#define LL_OPT_FAVOR_MASK ((1 << 1) | (1 << 2)) +#define LL_OPT_FAVOR_SHIFT 1 + +static inline int ll_opt_favor (int flag) +{ +return (flag &LL_OPT_FAVOR_MASK ) >>LL_OPT_FAVOR_SHIFT ; +} + +static inline int create_ll_flag (int favor) +{ +return ((favor <<LL_OPT_FAVOR_SHIFT ) &LL_OPT_FAVOR_MASK ); +} + int ll_merge(mmbuffer_t *result_buf, const char *path, mmfile_t *ancestor, const char *ancestor_label,
merge-recursive.c +2 -1
@@ -647,7 +647,8 @@ static int merge_3way(struct merge_options *o, merge_status =ll_merge (result_buf, a->path, &orig, base_name, &src1, name1, &src2, name2, - (!!o->call_depth) | (favor <<1 )); + ((o->call_depth ?LL_OPT_VIRTUAL_ANCESTOR :0 ) | +create_ll_flag (favor)));free (name1);free (name2);
Signature · 1 line
1.7.2.1.544.ga752d.dirty
Re: [PATCH 07/12] ll-merge: make flag easier to populate
Quoted 99 lines · Jonathan Nieder
ll_merge()takes its options in a flag word, which has a few advantages:- options flags can be cheaply passed around in registers, while an option struct passed by pointer cannot;
- callers can easily pass 0 without trouble for no options, while an option struct passed by value would not allow that.
The downside is that code to populate and access the flag word can be somewhat opaque. Mitigate that with a few macros.
Cc Avery Pennarun <[email protected]>Cc Bert Wesarg <[email protected]>Signed-off-by Jonathan Nieder <[email protected]>---
Documentation/technical/api-merge.txt | 11 +++++++----ll-merge.c| 9 +++++----ll-merge.h| 14 ++++++++++++++merge-recursive.c| 3 ++-4 files changed, 28 insertions(+), 9 deletions(-)Documentation/technical/api-merge.txt +0
-0@@ -49,12 +49,15 @@ supports this.The
flagparameter is a bitfield:- - The least significant bit indicates whether this is an internal
- merge to consolidate ancestors for a recursive merge.
+ - TheLL_OPT_VIRTUAL_ANCESTORbit indicates whether this is an
+ internal merge to consolidate ancestors for a recursive merge.- - The next two bits allow local conflicts to be automatically
+ - TheLL_OPT_FAVOR_MASKbits allow local conflicts to be automatically resolved in favor of one side or the other (as in 'git merge-file'
---ours/--theirs/--unionfor 01, 10, and 11, respectively).
+--ours/--theirs/--union).
+ They can be populated bycreate_ll_flag, whose argument can be
+XDL_MERGE_FAVOR_OURS,XDL_MERGE_FAVOR_THEIRS, or
+XDL_MERGE_FAVOR_UNION.Everything else
---------------ll-merge.c +5
-4@@ -46,7 +46,7 @@ static int ll_binary_merge(const struct ll_merge_driver *drv_unused,* or common ancestor for an internal merge. Still return * "conflicted merge" status. */ - mmfile_t *stolen = (flag & 01) ? orig : src1; + mmfile_t *stolen = (flag & LL_OPT_VIRTUAL_ANCESTOR) ? orig : src1; result->ptr = stolen->ptr; result->size = stolen->size;@@ -79,7 +79,7 @@ static int ll_xdl_merge(const struct ll_merge_driver *drv_unused, memset(&xmp, 0, sizeof(xmp)); xmp.level = XDL_MERGE_ZEALOUS; - xmp.favor= (flag >> 1) & 03; + xmp.favor = ll_opt_favor(flag); if (git_xmerge_style >= 0) xmp.style = git_xmerge_style; if (marker_size > 0)@@ -99,7 +99,8 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused, int flag, int marker_size) { /* Use union favor */ - flag = (flag & 1) | (XDL_MERGE_FAVOR_UNION << 1); + flag = (flag & LL_OPT_VIRTUAL_ANCESTOR) | + create_ll_flag(XDL_MERGE_FAVOR_UNION); return ll_xdl_merge(drv_unused, result, path_unused, orig, NULL, src1, NULL, src2, NULL, flag, marker_size);@@ -342,7 +343,7 @@ int ll_merge(mmbuffer_t *result_buf, const char *ll_driver_name = NULL; int marker_size = DEFAULT_CONFLICT_MARKER_SIZE; const struct ll_merge_driver *driver; - int virtual_ancestor = flag & 01; + int virtual_ancestor = flag & LL_OPT_VIRTUAL_ANCESTOR; if (merge_renormalize) { normalize_file(ancestor, path);ll-merge.h +4
-0@@ -5,6 +5,20 @@ #ifndef LL_MERGE_H #define LL_MERGE_H +#define LL_OPT_VIRTUAL_ANCESTOR (1 << 0) +#define LL_OPT_FAVOR_MASK ((1 << 1) | (1 << 2)) +#define LL_OPT_FAVOR_SHIFT 1 +
ll-merge.h line 21
Quoted 10 lines
ll-merge.h +10
-0+static inline int ll_opt_favor (int flag) +{ + return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT; +} + +static inline int create_ll_flag (int favor) +{ + return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK); +} +
These two function names do not suggests that these are symmetric. How about get_ll_flavor() and create_ll_flavor()? Or flavor_to_ll_flag() and ll_flag_to_flavor().
Regards,
Bert
Quoted 21 lines
ll-merge.h +0
-0int ll_merge(mmbuffer_t *result_buf, const char *path, mmfile_t *ancestor, const char *ancestor_label,merge-recursive.c +2
-2@@ -647,7 +647,8 @@ static int merge_3way(struct merge_options *o, merge_status = ll_merge(result_buf, a->path, &orig, base_name, &src1, name1, &src2, name2, - (!!o->call_depth) | (favor << 1)); + ((o->call_depth ? LL_OPT_VIRTUAL_ANCESTOR : 0) | + create_ll_flag(favor))); free(name1); free(name2); --1.7.2.1.544.ga752d.dirty
Re: [PATCH 07/12] ll-merge: make flag easier to populate
Quoted 12 lines · Bert Wesarg
Quoted 10 lines · Jonathan Nieder
ll-merge.h +10
-0+static inline int ll_opt_favor (int flag) +{ + return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT; +} + +static inline int create_ll_flag (int favor) +{ + return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK); +} +
[...]
Quoted 2 lines
Or
flavor_to_ll_flag()
andll_flag_to_flavor().
Sounds reasonable. (Well, except this is “favor” as in “favor our side” or “favor their side” rather than “flavor of the month”.) Patch?
Re: [PATCH 07/12] ll-merge: make flag easier to populate
Quoted 14 lines · Jonathan Nieder
Quoted 12 lines · Bert Wesarg
Quoted 10 lines · Jonathan Nieder
ll-merge.h +10
-0+static inline int ll_opt_favor (int flag) +{ + return (flag & LL_OPT_FAVOR_MASK) >> LL_OPT_FAVOR_SHIFT; +} + +static inline int create_ll_flag (int favor) +{ + return ((favor << LL_OPT_FAVOR_SHIFT) & LL_OPT_FAVOR_MASK); +} +[...]
Quoted 5 lines
Quoted 2 lines
Or
flavor_to_ll_flag()
andll_flag_to_flavor().Sounds reasonable. (Well, except this is “favor” as in “favor our side” or “favor their side” rather than “flavor of the month”.) Patch?
Sorry for this typo. Do you really want a squash patch for this renaming?
Bert
Quoted 1 line
Re: [PATCH 07/12] ll-merge: make flag easier to populate
Quoted 10 lines · Bert Wesarg
Quoted 7 lines · Jonathan Nieder
Quoted 2 lines · Bert Wesarg
Or
flavor_to_ll_flag()
andll_flag_to_flavor().Sounds reasonable. (Well, except this is “favor” as in “favor our side” or “favor their side” rather than “flavor of the month”.) Patch?
Sorry for this typo. Do you really want a squash patch for this renaming?
Yes, that would make my life easier. But if you are short on time, I can get to it myself later.
PATCH 8/12 ll-merge: let caller decide whether to renormalize
In reply to Jonathan NiederAdd a “renormalize” bit to the ll-merge options word so callers can decide on a case-by-case basis whether the merge is likely to have overlapped with a change in smudge/clean rules.
This reveals a few commands that have not been taking that situation into account, though it does not fix them.
No functional change intended.
---
Uses the flag word now.
The new option is not exposed through “git merge-file”, just like the virtual-ancestor option isn’t, but that is only from laziness. Exposing it would make tests this easier, too.
Some worries:
- "checkout -m" does not do convert_to_worktree() but it should;
- "rerere forget" has not been introduced to the wonderful world of smudge filters, either.
builtin/checkout.c | 4 ++++ ll-merge.c | 6 +++--- ll-merge.h | 1 + merge-recursive.c | 1 + rerere.c | 15 ++++++++++----- 5 files changed, 19 insertions(+), 8 deletions(-)
builtin/checkout.c +4 -0
@@ -150,6 +150,10 @@ static int checkout_merged(int pos, struct checkout *state)read_mmblob (&ours, active_cache[pos+1 ]->sha1);read_mmblob (&theirs, active_cache[pos+2 ]->sha1); +/* +* NEEDSWORK: re-create conflicts from merges with +* merge.renormalize set, too +*/ status =ll_merge (&result_buf, path, &ancestor,"base" , &ours,"ours" , &theirs,"theirs" ,0 );free (ancestor.ptr);
ll-merge.c +3 -3
@@ -99,8 +99,8 @@ static int ll_union_merge(const struct ll_merge_driver *drv_unused, int flag, int marker_size) {/* Use union favor */ - flag = (flag &LL_OPT_VIRTUAL_ANCESTOR ) | -create_ll_flag (XDL_MERGE_FAVOR_UNION ); + flag &= ~LL_OPT_FAVOR_MASK ; + flag |=create_ll_flag (XDL_MERGE_FAVOR_UNION );return ll_xdl_merge (drv_unused, result, path_unused, orig,NULL , src1,NULL , src2,NULL , flag, marker_size);
@@ -345,7 +345,7 @@ int ll_merge(mmbuffer_t *result_buf,const struct ll_merge_driver *driver;int virtual_ancestor = flag &LL_OPT_VIRTUAL_ANCESTOR ; -if (merge_renormalize) { +if (flag &LL_OPT_RENORMALIZE ) {normalize_file (ancestor, path);normalize_file (ours, path);normalize_file (theirs, path);
ll-merge.h +1 -0
@@ -8,6 +8,7 @@#define LL_OPT_VIRTUAL_ANCESTOR (1 << 0)#define LL_OPT_FAVOR_MASK ((1 << 1) | (1 << 2))#define LL_OPT_FAVOR_SHIFT 1 +#define LL_OPT_RENORMALIZE (1 << 3)static inline int ll_opt_favor (int flag) {
merge-recursive.c +1 -0
@@ -648,6 +648,7 @@ static int merge_3way(struct merge_options *o, merge_status =ll_merge (result_buf, a->path, &orig, base_name, &src1, name1, &src2, name2, ((o->call_depth ?LL_OPT_VIRTUAL_ANCESTOR :0 ) | + (o->renormalize ?LL_OPT_RENORMALIZE :0 ) |create_ll_flag (favor)));free (name1);
rerere.c +10 -5
@@ -319,6 +319,10 @@ static int handle_cache(const char *path, unsigned char *sha1, const char *outpuif (!mmfile[i].ptr && !mmfile[i].size) mmfile[i].ptr =xstrdup ("" ); } +/* +* NEEDSWORK: handle conflicts from merges with +* merge.renormalize set, too +*/ ll_merge (&result, path, &mmfile[0 ],NULL , &mmfile[1 ],"ours" , &mmfile[2 ],"theirs" ,0 );
@@ -361,7 +365,7 @@ static int find_conflict(struct string_list *conflict)return 0 ; } -static int merge (const char *name,const char *path) +static int merge (const char *name,int renormalize,const char *path) {int ret;mmfile_t cur = {NULL ,0 }, base = {NULL ,0 }, other = {NULL ,0 };
@@ -376,7 +380,8 @@ static int merge(const char *name, const char *path) ret =1 ; goto out; } - ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" ,0 ); + ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" , + renormalize ?LL_OPT_RENORMALIZE :0 );if (!ret) {FILE *f =fopen (path,"w" );if (!f)
@@ -424,7 +429,7 @@ static int update_paths(struct string_list *update)return status; } -static int do_plain_rerere (struct string_list *rr,int fd) +static int do_plain_rerere (struct string_list *rr,int fd,int renormalize) {struct string_list conflict = {NULL ,0 ,0 ,1 };struct string_list update = {NULL ,0 ,0 ,1 };
@@ -469,7 +474,7 @@ static int do_plain_rerere(struct string_list *rr, int fd)const char *name = (const char *)rr->items[i].util;if (has_rerere_resolution (name)) { -if (!merge (name, path)) { +if (!merge (name, renormalize, path)) {if (rerere_autoupdate)string_list_insert (path, &update); fprintf(stderr,
@@ -553,7 +558,7 @@ int rerere(int flags) fd =setup_rerere (&merge_rr, flags);if (fd <0 )return 0 ; -return do_plain_rerere (&merge_rr, fd); +return do_plain_rerere (&merge_rr, fd, merge_renormalize); }static int rerere_forget_one_path (const char *path,struct string_list *rr)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 9/12 t4200 (rerere): modernize style
In reply to Jonathan NiederGuard all test code with test_expect_success to make the script easier to follow. While at it, pick some other nits:
- use test_tick (more than we have to, to be realistic);
- 'single quotes' and \escaped HERE documents where possible simplify review for escaping problems;
- omit whitespace after >redirection operators for consistency with other tests;
- use "update-index --refresh" instead of testing that "ls-files -u" output is empty, since the former produces nicer output on failure;
- compare to expected nonempty "ls-files -u" output instead of counting lines when it is expected to be nonempty.
--- t/t4200-rerere.sh | 303 +++++++++++++++++++++++++++++++---------------------- 1 files changed, 179 insertions(+), 124 deletions(-)
t/t4200-rerere.sh +179 -124
@@ -4,237 +4,292 @@ # test_description='git rerere + +! [fifth] version1 + ! [first] first + ! [fourth] version1 + ! [master] initial + ! [second] prefer first over second + ! [third] version2 +------ + + [third] version2 ++ [fifth] version1 + + [fourth] version1 ++ + + [third^] third + - [second] prefer first over second + + + [first] first + + [second^] second +++++++ [master] initial ' . ./test-lib.sh -test_expect_success 'setup' " - cat > a1 <<- EOF && +test_expect_success 'setup' ' + cat >a1 <<-\EOF && Some title ========== - Whether 'tis nobler in the mind to suffer + Whether '\''tis nobler in the mind to suffer The slings and arrows of outrageous fortune, Or to take arms against a sea of troubles, And by opposing end them? To die: to sleep; No more; and by a sleep to say we end The heart-ache and the thousand natural shocks - That flesh is heir to, 'tis a consummation - Devoutly to be wish'd. + That flesh is heir to, '\''tis a consummation + Devoutly to be wish'\''d. EOF git add a1 && + test_tick && git commit -q -a -m initial && - git checkout -b first && - cat >> a1 <<- EOF && + cat >>a1 <<-\EOF && Some title ========== To die, to sleep; - To sleep: perchance to dream: ay, there's the rub; + To sleep: perchance to dream: ay, there'\''s the rub; For in that sleep of death what dreams may come When we have shuffled off this mortal coil, - Must give us pause: there's the respect + Must give us pause: there'\''s the respect That makes calamity of so long life; EOF + + git checkout -b first && + test_tick && git commit -q -a -m first && git checkout -b second master && git show first:a1 | - sed -e 's/To die, t/To die! T/' -e 's/Some title/Some Title/' > a1 && - echo '* END *' >>a1 && + sed -e "s/To die, t/To die! T/" -e "s/Some title/Some Title/" >a1 && + echo "* END *" >>a1 && + test_tick && git commit -q -a -m second -" +' test_expect_success 'nothing recorded without rerere' ' - (rm -rf .git/rr-cache; git config rerere.enabled false) && + rm -rf .git/rr-cache && + git config rerere.enabled false && test_must_fail git merge first && ! test -d .git/rr-cache ' -# activate rerere, old style -test_expect_success 'conflicting merge' ' +test_expect_success 'activate rerere, old style (conflicting merge)' ' git reset --hard && mkdir .git/rr-cache && - git config --unset rerere.enabled && - test_must_fail git merge first -' + test_might_fail git config --unset rerere.enabled && + test_must_fail git merge first && -sha1=$(perl -pe 's/ .*//' .git/MERGE_RR) -rr=.git/rr-cache/$sha1 -test_expect_success 'recorded preimage' "grep ^=======$ $rr/preimage" + sha1=$(perl -pe "s/ .*//" .git/MERGE_RR) && + rr=.git/rr-cache/$sha1 && + grep "^=======\$" $rr/preimage && + ! test -f $rr/postimage && + ! test -f $rr/thisimage +' test_expect_success 'rerere.enabled works, too' ' rm -rf .git/rr-cache && git config rerere.enabled true && git reset --hard && test_must_fail git merge first && + + sha1=$(perl -pe "s/ .*//" .git/MERGE_RR) && + rr=.git/rr-cache/$sha1 && grep ^=======$ $rr/preimage ' -test_expect_success 'no postimage or thisimage yet' \ - "test ! -f $rr/postimage -a ! -f $rr/thisimage" +test_expect_success 'set up rr-cache' ' + rm -rf .git/rr-cache && + git config rerere.enabled true && + git reset --hard && + test_must_fail git merge first && + sha1=$(perl -pe "s/ .*//" .git/MERGE_RR) && + rr=.git/rr-cache/$sha1 +' -test_expect_success 'preimage has right number of lines' ' +test_expect_success 'rr-cache looks sane' ' + # no postimage or thisimage yet + ! test -f $rr/postimage && + ! test -f $rr/thisimage && + # preimage has right number of lines cnt=$(sed -ne "/^<<<<<<</,/^>>>>>>>/p" $rr/preimage | wc -l) && + echo $cnt && test $cnt = 13 - ' -git show first:a1 > a1 - -cat > expect << EOF ---- a/a1 -+++ b/a1 -@@ -1,4 +1,4 @@ --Some Title -+Some title - ========== - Whether 'tis nobler in the mind to suffer - The slings and arrows of outrageous fortune, -@@ -8,21 +8,11 @@ - The heart-ache and the thousand natural shocks - That flesh is heir to, 'tis a consummation - Devoutly to be wish'd. --<<<<<<< --Some Title --========== --To die! To sleep; --======= - Some title - ========== - To die, to sleep; -->>>>>>> - To sleep: perchance to dream: ay, there's the rub; - For in that sleep of death what dreams may come - When we have shuffled off this mortal coil, - Must give us pause: there's the respect - That makes calamity of so long life; --<<<<<<< --======= --* END * -->>>>>>> -EOF -git rerere diff > out - -test_expect_success 'rerere diff' 'test_cmp expect out' - -cat > expect << EOF -a1 -EOF - -git rerere status > out +test_expect_success 'rerere diff' ' + git show first:a1 >a1 && + cat >expect <<-\EOF && + --- a/a1 + +++ b/a1 + @@ -1,4 +1,4 @@ + -Some Title + +Some title + ========== + Whether '\''tis nobler in the mind to suffer + The slings and arrows of outrageous fortune, + @@ -8,21 +8,11 @@ + The heart-ache and the thousand natural shocks + That flesh is heir to, '\''tis a consummation + Devoutly to be wish'\''d. + -<<<<<<< + -Some Title + -========== + -To die! To sleep; + -======= + Some title + ========== + To die, to sleep; + ->>>>>>> + To sleep: perchance to dream: ay, there'\''s the rub; + For in that sleep of death what dreams may come + When we have shuffled off this mortal coil, + Must give us pause: there'\''s the respect + That makes calamity of so long life; + -<<<<<<< + -======= + -* END * + ->>>>>>> + EOF + git rerere diff >out && + test_cmp expect out +' -test_expect_success 'rerere status' 'test_cmp expect out' +test_expect_success 'rerere status' ' + echo a1 >expect && + git rerere status >out && + test_cmp expect out +' -test_expect_success 'commit succeeds' \ - "git commit -q -a -m 'prefer first over second'" +test_expect_success 'first postimage wins' ' + git show first:a1 | sed "s/To die: t/To die! T/" >expect && -test_expect_success 'recorded postimage' "test -f $rr/postimage" + git commit -q -a -m "prefer first over second" && + test -f $rr/postimage && -test_expect_success 'another conflicting merge' ' git checkout -b third master && - git show second^:a1 | sed "s/To die: t/To die! T/" > a1 && + git show second^:a1 | sed "s/To die: t/To die! T/" >a1 && git commit -q -a -m third && - test_must_fail git pull . first -' - -git show first:a1 | sed 's/To die: t/To die! T/' > expect -test_expect_success 'rerere kicked in' "! grep ^=======$ a1" - -test_expect_success 'rerere prefers first change' 'test_cmp a1 expect' - -rm $rr/postimage -echo "$sha1 a1" | perl -pe 'y/\012/\000/' > .git/MERGE_RR - -test_expect_success 'rerere clear' 'git rerere clear' - -test_expect_success 'clear removed the directory' "test ! -d $rr" -mkdir $rr -echo Hello > $rr/preimage -echo World > $rr/postimage - -sha2=4000000000000000000000000000000000000000 -rr2=.git/rr-cache/$sha2 -mkdir $rr2 -echo Hello > $rr2/preimage + test_must_fail git pull . first && + # rerere kicked in + ! grep "^=======\$" a1 && + test_cmp expect a1 +' -almost_15_days_ago=$((60-15*86400)) -just_over_15_days_ago=$((-1-15*86400)) -almost_60_days_ago=$((60-60*86400)) -just_over_60_days_ago=$((-1-60*86400)) +test_expect_success 'rerere clear' ' + rm $rr/postimage && + echo "$sha1 a1" | perl -pe "y/\012/\000/" >.git/MERGE_RR && + git rerere clear && + ! test -d $rr +' -test-chmtime =$almost_60_days_ago $rr/preimage -test-chmtime =$almost_15_days_ago $rr2/preimage +test_expect_success 'set up for garbage collection tests' ' + mkdir -p $rr && + echo Hello >$rr/preimage && + echo World >$rr/postimage && -test_expect_success 'garbage collection (part1)' 'git rerere gc' + sha2=4000000000000000000000000000000000000000 && + rr2=.git/rr-cache/$sha2 && + mkdir $rr2 && + echo Hello >$rr2/preimage && -test_expect_success 'young records still live' \ - "test -f $rr/preimage && test -f $rr2/preimage" + almost_15_days_ago=$((60-15*86400)) && + just_over_15_days_ago=$((-1-15*86400)) && + almost_60_days_ago=$((60-60*86400)) && + just_over_60_days_ago=$((-1-60*86400)) && -test-chmtime =$just_over_60_days_ago $rr/preimage -test-chmtime =$just_over_15_days_ago $rr2/preimage + test-chmtime =$almost_60_days_ago $rr/preimage && + test-chmtime =$almost_15_days_ago $rr2/preimage +' -test_expect_success 'garbage collection (part2)' 'git rerere gc' +test_expect_success 'garbage collection preserves young records' ' + git rerere gc && + test -f $rr/preimage && + test -f $rr2/preimage +' -test_expect_success 'old records rest in peace' \ - "test ! -f $rr/preimage && test ! -f $rr2/preimage" +test_expect_success 'old records rest in peace' ' + test-chmtime =$just_over_60_days_ago $rr/preimage && + test-chmtime =$just_over_15_days_ago $rr2/preimage && + git rerere gc && + ! test -f $rr/preimage && + ! test -f $rr2/preimage +' -test_expect_success 'file2 added differently in two branches' ' +test_expect_success 'setup: file2 added differently in two branches' ' git reset --hard && + git checkout -b fourth && - echo Hallo > file2 && + echo Hallo >file2 && git add file2 && + test_tick && git commit -m version1 && + git checkout third && - echo Bello > file2 && + echo Bello >file2 && git add file2 && + test_tick && git commit -m version2 && + test_must_fail git merge fourth && - echo Cello > file2 && + echo Cello >file2 && git add file2 && git commit -m resolution ' test_expect_success 'resolution was recorded properly' ' + echo Cello >expected && + git reset --hard HEAD~2 && git checkout -b fifth && - echo Hallo > file3 && + + echo Hallo >file3 && git add file3 && + test_tick && git commit -m version1 && + git checkout third && - echo Bello > file3 && + echo Bello >file3 && git add file3 && + test_tick && git commit -m version2 && git tag version2 && + test_must_fail git merge fifth && - test Cello = "$(cat file3)" && - test 0 != $(git ls-files -u | wc -l) + test_cmp expected file3 && + test_must_fail git update-index --refresh ' test_expect_success 'rerere.autoupdate' ' - git config rerere.autoupdate true + git config rerere.autoupdate true && git reset --hard && git checkout version2 && test_must_fail git merge fifth && - test 0 = $(git ls-files -u | wc -l) + git update-index --refresh ' test_expect_success 'merge --rerere-autoupdate' ' - git config --unset rerere.autoupdate + test_might_fail git config --unset rerere.autoupdate && git reset --hard && git checkout version2 && test_must_fail git merge --rerere-autoupdate fifth && - test 0 = $(git ls-files -u | wc -l) + git update-index --refresh ' test_expect_success 'merge --no-rerere-autoupdate' ' - git config rerere.autoupdate true + headblob=$(git rev-parse version2:file3) && + mergeblob=$(git rev-parse fifth:file3) && + cat >expected <<-EOF && + 100644 $headblob 2 file3 + 100644 $mergeblob 3 file3 + EOF + + git config rerere.autoupdate true && git reset --hard && git checkout version2 && test_must_fail git merge --no-rerere-autoupdate fifth && - test 2 = $(git ls-files -u | wc -l) + git ls-files -u >actual && + test_cmp expected actual ' test_done
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 10/12 rerere: migrate to parse-options API
In reply to Jonathan Nieder---
The tests don’t pass for me with the old implementation; I think "2 < argc" should have been "2 <= argc". Well, no use dwelling in the past.
builtin/rerere.c | 52 ++++++++++++++++-------------- t/t4200-rerere.sh | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 119 insertions(+), 24 deletions(-)
builtin/rerere.c +28 -24
@@ -1,13 +1,16 @@#include "builtin.h" #include "cache.h" #include "dir.h" +#include "parse-options.h" #include "string-list.h" #include "rerere.h" #include "xdiff/xdiff.h" #include "xdiff-interface.h" -static const char git_rerere_usage[] = -"git rerere [clear | status | diff | gc]" ; +static const char *const rerere_usage[] = { +"git rerere [clear | status | diff | gc]" , +NULL , +};/* these values are days */ static int cutoff_noresolve =15 ;
@@ -103,25 +106,26 @@ static int diff_two(const char *file1, const char *label1,int cmd_rerere (int argc,const char **argv,const char *prefix) {struct string_list merge_rr = {NULL ,0 ,0 ,1 }; -int i, fd, flags =0 ; - -if (2 < argc) { -if (!strcmp (argv[1 ],"-h" )) -usage (git_rerere_usage); -if (!strcmp (argv[1 ],"--rerere-autoupdate" )) - flags =RERERE_AUTOUPDATE ; -else if (!strcmp (argv[1 ],"--no-rerere-autoupdate" )) - flags =RERERE_NOAUTOUPDATE ; -if (flags) { - argc--; - argv++; - } - } -if (argc <2 ) +int i, fd, autoupdate =-1 , flags =0 ; + +struct option options[] = { +OPT_SET_INT (0 ,"rerere-autoupdate" , &autoupdate, +"register clean resolutions in index" ,1 ), +OPT_END (), + }; + + argc =parse_options (argc, argv, prefix, options, rerere_usage,0 ); + +if (autoupdate ==1 ) + flags =RERERE_AUTOUPDATE ; +if (autoupdate ==0 ) + flags =RERERE_NOAUTOUPDATE ; + +if (argc <1 )return rerere (flags); -if (!strcmp (argv[1 ],"forget" )) { -const char **pathspec =get_pathspec (prefix, argv +2 ); +if (!strcmp (argv[0 ],"forget" )) { +const char **pathspec =get_pathspec (prefix, argv +1 );return rerere_forget (pathspec); }
@@ -129,26 +133,26 @@ int cmd_rerere(int argc, const char **argv, const char *prefix)if (fd <0 )return 0 ; -if (!strcmp (argv[1 ],"clear" )) { +if (!strcmp (argv[0 ],"clear" )) {for (i =0 ; i < merge_rr.nr; i++) {const char *name = (const char *)merge_rr.items[i].util;if (!has_rerere_resolution (name))unlink_rr_item (name); }unlink_or_warn (git_path ("rr-cache/MERGE_RR" )); - }else if (!strcmp (argv[1 ],"gc" )) + }else if (!strcmp (argv[0 ],"gc" ))garbage_collect (&merge_rr); -else if (!strcmp (argv[1 ],"status" )) +else if (!strcmp (argv[0 ],"status" ))for (i =0 ; i < merge_rr.nr; i++)printf ("%s\n" , merge_rr.items[i].string); -else if (!strcmp (argv[1 ],"diff" )) +else if (!strcmp (argv[0 ],"diff" ))for (i =0 ; i < merge_rr.nr; i++) {const char *path = merge_rr.items[i].string;const char *name = (const char *)merge_rr.items[i].util;diff_two (rerere_path (name,"preimage" ), path, path, path); }else -usage (git_rerere_usage); +usage_with_options (rerere_usage, options);string_list_clear (&merge_rr,1 );return 0 ;
t/t4200-rerere.sh +91 -0
95 lines not on this page · raw message
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 11/12 rerere: never renormalize
In reply to Jonathan Niederplain rerere performs three tasks; let us consider how the new merge.renormalize option should apply to each.
After an unsuccessful merge, rerere records conflict hunks from the work tree under .git/rr-cache. If the merge was performed with merge.renormalize enabled, both sides of the conflict hunk use the current work tree’s end-of-line and smudge rules; there is not really much of a choice.
After a successful manual resolution, rerere records the postimage. Here, also, the file will be in the current work tree’s canonical format and there is not much to do about it.
When encountering that conflict again, merge looks up the preimage and postimage using the conflict hunk as a key and runs a three-way merge to apply that resolution to the work tree. Since the conflict hunk used the current work tree’s canonical format, chances are the version in the work tree, the preimage, and the postimage will, too. In fact using the merge.renormalize machinery is exactly the wrong thing to do, since its result has been run through convert_to_git and therefore is not suitable for writing to the work tree.
The only affected caller is "git merge".
NEEDSWORK: lacks test
--- rerere.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
rerere.c +5 -6
@@ -365,7 +365,7 @@ static int find_conflict(struct string_list *conflict)return 0 ; } -static int merge (const char *name,int renormalize,const char *path) +static int merge (const char *name,const char *path) {int ret;mmfile_t cur = {NULL ,0 }, base = {NULL ,0 }, other = {NULL ,0 };
@@ -380,8 +380,7 @@ static int merge(const char *name, int renormalize, const char *path) ret =1 ; goto out; } - ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" , - renormalize ?LL_OPT_RENORMALIZE :0 ); + ret =ll_merge (&result, path, &base,NULL , &cur,"" , &other,"" ,0 );if (!ret) {FILE *f =fopen (path,"w" );if (!f)
@@ -429,7 +428,7 @@ static int update_paths(struct string_list *update)return status; } -static int do_plain_rerere (struct string_list *rr,int fd,int renormalize) +static int do_plain_rerere (struct string_list *rr,int fd) {struct string_list conflict = {NULL ,0 ,0 ,1 };struct string_list update = {NULL ,0 ,0 ,1 };
@@ -474,7 +473,7 @@ static int do_plain_rerere(struct string_list *rr, int fd, int renormalize)const char *name = (const char *)rr->items[i].util;if (has_rerere_resolution (name)) { -if (!merge (name, renormalize, path)) { +if (!merge (name, path)) {if (rerere_autoupdate)string_list_insert (path, &update); fprintf(stderr,
@@ -558,7 +557,7 @@ int rerere(int flags) fd =setup_rerere (&merge_rr, flags);if (fd <0 )return 0 ; -return do_plain_rerere (&merge_rr, fd, merge_renormalize); +return do_plain_rerere (&merge_rr, fd); }static int rerere_forget_one_path (const char *path,struct string_list *rr)
Signature · 1 line
1.7.2.1.544.ga752d.dirty
PATCH 12/12 merge-recursive --renormalize
In reply to Jonathan NiederTeach "git merge-recursive" a --renormalize option to enable the merge.renormalize configuration. The --no-renormalize option can be used to override it in the negative.
So in the future, you might be able to, e.g.:
git checkout -m -Xrenormalize otherbranch
or
git revert -Xrenormalize otherpatch
or
git pull --rebase -Xrenormalize
The bad part: merge.renormalize is still not honored for most commands. And it reveals lots of places that -X has not been plumbed in (so we get "git merge -Xrenormalize" but not much else).
NEEDSWORK: tests
---
- Commit message is rewritten
- Introduce --no-renormalize, too
- Adapt to rerere’s new lack of support for --renormalize
That’s it for this round. Thanks again.
Documentation/merge-strategies.txt | 12 ++++++++++++ builtin/checkout.c | 7 +++++++ builtin/merge-recursive.c | 4 ++++ builtin/merge.c | 19 ++++++++++++++----- builtin/revert.c | 7 +++++++ cache.h | 1 - environment.c | 1 - merge-recursive.c | 2 +- 8 files changed, 45 insertions(+), 8 deletions(-)
Documentation/merge-strategies.txt +12 -0
@@ -40,6 +40,18 @@ the other tree did, declaring 'our' history contains all that happened in it. theirs;; This is opposite of 'ours'. +renormalize;; + This runs a virtual check-out and check-in of all three stages + of a file when resolving a three-way merge. This option is + meant to be used when merging branches with different clean + filters or end-of-line normalization rules. See "Merging + branches with differing checkin/checkout attributes" in + linkgit:gitattributes[5] for details. + +no-renormalize;; + Disables the `renormalize` option. This overrides the + `merge.renormalize` configuration variable. + subtree[=path];; This option is a more advanced form of 'subtree' strategy, where the strategy makes a guess on how two trees must be shifted to
builtin/checkout.c +7 -0
@@ -437,6 +437,13 @@ static int merge_working_tree(struct checkout_opts *opts,*/ add_files_to_cache (NULL ,NULL ,0 ); +/* +* NEEDSWORK: carrying over local changes +* when branches have different end-of-line +* normalization (or clean+smudge rules) is +* a pain; plumb in an option to set +* o.renormalize? +*/ init_merge_options (&o); o.verbosity =0 ; work =write_tree_from_memory (&o);
builtin/merge-recursive.c +4 -0
@@ -45,6 +45,10 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) o.subtree_shift ="" ; else if (!prefixcmp(arg+2, "subtree=")) o.subtree_shift = arg +10 ; + else if (!strcmp(arg+2, "renormalize")) + o.renormalize =1 ; + else if (!strcmp(arg+2, "no-renormalize")) + o.renormalize =0 ; else die("Unknown option %s", arg);continue ;
builtin/merge.c +14 -5
@@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;static const char **xopts;static size_t xopts_nr, xopts_alloc;static const char *branch; +static int option_renormalize;static int verbosity;static int allow_rerere_auto;
@@ -503,9 +504,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)return git_config_string (&pull_octopus, k, v); else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) option_log =git_config_bool (k, v); - else if (!strcmp(k, "merge.renormalize")) { - merge_renormalize =git_config_bool (k, v); - } + else if (!strcmp(k, "merge.renormalize")) + option_renormalize =git_config_bool (k, v);return git_diff_ui_config (k, v, cb); }
@@ -627,6 +627,11 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,if (!strcmp (strategy,"subtree" )) o.subtree_shift ="" ; + o.renormalize = option_renormalize; + +/* +* NEEDSWORK: merge with table in builtin/merge-recursive +*/ for (x =0 ; x < xopts_nr; x++) {if (!strcmp (xopts[x],"ours" )) o.recursive_variant =MERGE_RECURSIVE_OURS ;
@@ -636,6 +641,10 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, o.subtree_shift ="" ; else if (!prefixcmp(xopts[x], "subtree=")) o.subtree_shift = xopts[x]+8; + else if (!strcmp(xopts[x], "renormalize")) + o.renormalize = 1; + else if (!strcmp(xopts[x], "no-renormalize")) + o.renormalize = 0; else die("Unknown option for merge-recursive: -X%s", xopts[x]); }
@@ -819,7 +828,7 @@ static int finish_automerge(struct commit_list *common,return 0 ; } -static int suggest_conflicts (void ) +static int suggest_conflicts (int renormalizing) {FILE *fp;int pos;
@@ -1304,5 +1313,5 @@ int cmd_merge(int argc, const char **argv, const char *prefix) "stopped before committing as requested\n");return 0 ; }else -return suggest_conflicts (); +return suggest_conflicts (option_renormalize); }
builtin/revert.c +7 -0
@@ -318,6 +318,13 @@ static void do_recursive_merge(struct commit *base, struct commit *next, index_fd =hold_locked_index (&index_lock,1 );read_cache (); + +/* +* NEEDSWORK: cherry-picking between branches with +* different end-of-line normalization is a pain; +* plumb in an option to set o.renormalize? +* (or better: arbitrary -X options) +*/ init_merge_options (&o); o.ancestor = base ? base_label :"(empty tree)" ; o.branch1 ="HEAD" ;
cache.h +0 -1
@@ -551,7 +551,6 @@ extern int read_replace_refs;extern int fsync_object_files;extern int core_preload_index;extern int core_apply_sparse_checkout; -extern int merge_renormalize;enum safe_crlf {SAFE_CRLF_FALSE =0 ,
environment.c +0 -1
@@ -53,7 +53,6 @@ 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 merge_renormalize;/* Parallel index stat data preload? */ int core_preload_index =0 ;
merge-recursive.c +1 -1
@@ -1486,7 +1486,7 @@ void init_merge_options(struct merge_options *o) o->buffer_output =1 ; o->diff_rename_limit =-1 ; o->merge_rename_limit =-1 ; - o->renormalize = merge_renormalize; + o->renormalize =0 ;git_config (merge_recursive_config, o);if (getenv ("GIT_MERGE_VERBOSITY" )) o->verbosity =
Signature · 1 line
1.7.2.1.544.ga752d.dirty
Eyvind Bernhardsen
Acked-byRe: [PATCH/RFC v2 0/12] Re: rerere: let caller decide whether to renormalize
In reply to Jonathan NiederHi Jonathan,
Sorry I'm late to respond, I've just caught up with work after a long summer holiday. It's back to working with complicated merges across normalization boundaries now, though. This series looks like it's taking my hack in a sane direction, so for what it's worth:
Meanwhile, I've hit an annoyance: currently, files that are introduced with CRLFs on a non-normalized branch are left alone in the merged branch, causing the classical core.autocrlf problem of marking every line in such a file as changed if it is so much as touched.
I think the renormalization setting makes much more sense if the end result of the merge is normalized, so I'm working on a patch to make it do that.
- Eyvind
PATCH 6/6 merge-recursive: add -Xrenormalize option
In reply to Jonathan NiederThis adds a -Xrenormalize option to override the merge.renormalize configuration. There is no way to override it in the negative yet, but hopefully this suggests how.
The good part: this destroys the global merge_renormalize variable.
The bad part: merge.renormalize is still not honored for most commands. And it reveals lots of places that -X has not been plumbed in.
---
There you have it. As I said, this is pretty rough. Places I would be happy to see this go:
- add tests and documentation
- -Xno-renormalize to override [merge] renormalize in the negative
- checkout -X and rerere -X
I probably will not have time to work on this topic again for a while, so I would be very happy if someone takes ideas from it and takes it somewhere else before I get a chance. If there are just minor cleanups to do, I can maintain this series for a short while, though.
Thoughts?
Documentation/merge-strategies.txt | 8 ++++++++ builtin/checkout.c | 7 +++++++ builtin/merge-recursive.c | 2 ++ builtin/merge.c | 19 +++++++++++++------ builtin/revert.c | 7 +++++++ cache.h | 1 - environment.c | 1 - merge-recursive.c | 2 +- 8 files changed, 38 insertions(+), 9 deletions(-)
Documentation/merge-strategies.txt +8 -0
@@ -40,6 +40,14 @@ the other tree did, declaring 'our' history contains all that happened in it. theirs;; This is opposite of 'ours'. +renormalize;; + This runs a virtual check-out and check-in of all three stages + of a file when resolving a three-way merge. This option is + meant to be used when merging branches with different clean + filters or end-of-line normalization rules. See "Merging + branches with differing checkin/checkout attributes" in + linkgit:gitattributes[5] for details. + subtree[=path];; This option is a more advanced form of 'subtree' strategy, where the strategy makes a guess on how two trees must be shifted to
builtin/checkout.c +7 -0
@@ -437,6 +437,13 @@ static int merge_working_tree(struct checkout_opts *opts,*/ add_files_to_cache (NULL ,NULL ,0 ); +/* +* NEEDSWORK: carrying over local changes +* when branches have different end-of-line +* normalization (or clean+smudge rules) is +* a pain; plumb in an option to set +* o.renormalize? +*/ init_merge_options (&o); o.verbosity =0 ; work =write_tree_from_memory (&o);
builtin/merge-recursive.c +2 -0
@@ -45,6 +45,8 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) o.subtree_shift ="" ; else if (!prefixcmp(arg+2, "subtree=")) o.subtree_shift = arg +10 ; + else if (!strcmp(arg+2, "renormalize")) + o.renormalize =1 ; else die("Unknown option %s", arg);continue ;
builtin/merge.c +13 -6
@@ -54,6 +54,7 @@ static size_t use_strategies_nr, use_strategies_alloc;static const char **xopts;static size_t xopts_nr, xopts_alloc;static const char *branch; +static int option_renormalize;static int verbosity;static int allow_rerere_auto;
@@ -503,9 +504,8 @@ static int git_merge_config(const char *k, const char *v, void *cb)return git_config_string (&pull_octopus, k, v); else if (!strcmp(k, "merge.log") || !strcmp(k, "merge.summary")) option_log =git_config_bool (k, v); - else if (!strcmp(k, "merge.renormalize")) { - merge_renormalize =git_config_bool (k, v); - } + else if (!strcmp(k, "merge.renormalize")) + option_renormalize =git_config_bool (k, v);return git_diff_ui_config (k, v, cb); }
@@ -627,6 +627,11 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,if (!strcmp (strategy,"subtree" )) o.subtree_shift ="" ; + o.renormalize = option_renormalize; + +/* +* NEEDSWORK: merge with table in builtin/merge-recursive +*/ for (x =0 ; x < xopts_nr; x++) {if (!strcmp (xopts[x],"ours" )) o.recursive_variant =MERGE_RECURSIVE_OURS ;
@@ -636,6 +641,8 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common, o.subtree_shift ="" ; else if (!prefixcmp(xopts[x], "subtree=")) o.subtree_shift = xopts[x]+8; + else if (!strcmp(xopts[x], "renormalize")) + o.renormalize = 1; else die("Unknown option for merge-recursive: -X%s", xopts[x]); }
@@ -819,7 +826,7 @@ static int finish_automerge(struct commit_list *common,return 0 ; } -static int suggest_conflicts (void ) +static int suggest_conflicts (int renormalizing) {FILE *fp;int pos, flag;
@@ -842,7 +849,7 @@ static int suggest_conflicts(void) }fclose (fp); flag = allow_rerere_auto; -if (merge_renormalize) +if (renormalizing) flag |=RERERE_RENORMALIZE ;rerere (flag); printf("Automatic merge failed; "
@@ -1307,5 +1314,5 @@ int cmd_merge(int argc, const char **argv, const char *prefix) "stopped before committing as requested\n");return 0 ; }else -return suggest_conflicts (); +return suggest_conflicts (option_renormalize); }
builtin/revert.c +7 -0
@@ -318,6 +318,13 @@ static void do_recursive_merge(struct commit *base, struct commit *next, index_fd =hold_locked_index (&index_lock,1 );read_cache (); + +/* +* NEEDSWORK: cherry-picking between branches with +* different end-of-line normalization is a pain; +* plumb in an option to set o.renormalize? +* (or better: arbitrary -X options) +*/ init_merge_options (&o); o.ancestor = base ? base_label :"(empty tree)" ; o.branch1 ="HEAD" ;
cache.h +0 -1
@@ -551,7 +551,6 @@ extern int read_replace_refs;extern int fsync_object_files;extern int core_preload_index;extern int core_apply_sparse_checkout; -extern int merge_renormalize;enum safe_crlf {SAFE_CRLF_FALSE =0 ,
environment.c +0 -1
@@ -53,7 +53,6 @@ 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 merge_renormalize;/* Parallel index stat data preload? */ int core_preload_index =0 ;
merge-recursive.c +1 -1
@@ -1485,7 +1485,7 @@ void init_merge_options(struct merge_options *o) o->buffer_output =1 ; o->diff_rename_limit =-1 ; o->merge_rename_limit =-1 ; - o->renormalize = merge_renormalize; + o->renormalize =0 ;git_config (merge_recursive_config, o);if (getenv ("GIT_MERGE_VERBOSITY" )) o->verbosity =
Signature · 1 line
1.7.2.1.544.ga752d.dirty
Re: [PATCH/RFC eb/double-convert-before-merge 0/6] merge -Xrenormalize
In reply to Jonathan NiederQuoted 15 lines · Jonathan Nieder
But for later, they tell a nice story. The idea is that the "merge.renormalize" is not really about the behavior of the "git merge" command but about the merge-recursive driver; and so it should be usable everywhere else that driver is used, too.
...
So in an ideal world, you would be able to, e.g.:git checkout -m -Xrenormalize otherbranch
or
git revert -Xrenormalize otherpatch
or
Quoted 7 lines
git pull --rebase -XrenormalizeOf course, the plumbing is not all there yet; many commands that use merge drivers cannot pass through arbitrary options through yet.
Well, enough talking. Here are the patches; what do you think?
Nicely summarized.