From fa2364ec3457cc107e47d617779f1ffa23647ca4 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Dec 2012 15:08:01 -0800 Subject: Which merge_file() function do you mean? There are two different static functions and one global function, all of them called "merge_file()", with different signatures and purposes. Rename them all to reduce confusion in "git grep" output: * Rename the static one in merge-index to "merge_one_path(const char *path)" as that function is about asking an external command to resolve conflicts in one path. * Rename the global one in merge-file.c that is only used by merge-tree to "merge_blobs()", as the function takes three blobs and returns the merged result only in-core, without doing anything to the filesystem. * Rename the one in merge-recursive to "merge_one_file()", just to be fair. Also rename merge-file.[ch] to merge-blobs.[ch]. Signed-off-by: Junio C Hamano --- builtin/merge-index.c | 4 ++-- builtin/merge-tree.c | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'builtin') diff --git a/builtin/merge-index.c b/builtin/merge-index.c index 2338832587..be5e514324 100644 --- a/builtin/merge-index.c +++ b/builtin/merge-index.c @@ -42,7 +42,7 @@ static int merge_entry(int pos, const char *path) return found; } -static void merge_file(const char *path) +static void merge_one_path(const char *path) { int pos = cache_name_pos(path, strlen(path)); @@ -102,7 +102,7 @@ int cmd_merge_index(int argc, const char **argv, const char *prefix) } die("git merge-index: unknown option %s", arg); } - merge_file(arg); + merge_one_path(arg); } if (err && !quiet) die("merge program failed"); diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 897a563bc6..ebd0d25db6 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -3,7 +3,7 @@ #include "xdiff-interface.h" #include "blob.h" #include "exec_cmd.h" -#include "merge-file.h" +#include "merge-blobs.h" static const char merge_tree_usage[] = "git merge-tree "; static int resolve_directories = 1; @@ -76,7 +76,7 @@ static void *result(struct merge_list *entry, unsigned long *size) their = NULL; if (entry) their = entry->blob; - return merge_file(path, base, our, their, size); + return merge_blobs(path, base, our, their, size); } static void *origin(struct merge_list *entry, unsigned long *size) -- cgit v1.2.3 From b13112fa160f44fb0d795cdd992015c9f5a3d72a Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Dec 2012 15:41:04 -0800 Subject: merge-tree: lose unused "flags" from merge_list Drop the unused field from the structure. Signed-off-by: Junio C Hamano --- builtin/merge-tree.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'builtin') diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index ebd0d25db6..b61d811158 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -12,8 +12,7 @@ struct merge_list { struct merge_list *next; struct merge_list *link; /* other stages for this object */ - unsigned int stage : 2, - flags : 30; + unsigned int stage : 2; unsigned int mode; const char *path; struct blob *blob; -- cgit v1.2.3 From 3b8ff51b708e52e4ae2b8f266d25c187c3152820 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Dec 2012 15:53:41 -0800 Subject: merge-tree: lose unused "resolve_directories" This option is always set; simplify. Signed-off-by: Junio C Hamano --- builtin/merge-tree.c | 3 --- 1 file changed, 3 deletions(-) (limited to 'builtin') diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index b61d811158..95de162263 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -6,7 +6,6 @@ #include "merge-blobs.h" static const char merge_tree_usage[] = "git merge-tree "; -static int resolve_directories = 1; struct merge_list { struct merge_list *next; @@ -198,8 +197,6 @@ static int unresolved_directory(const struct traverse_info *info, struct name_en struct tree_desc t[3]; void *buf0, *buf1, *buf2; - if (!resolve_directories) - return 0; p = n; if (!p->mode) { p++; -- cgit v1.2.3 From 8dd15c6a909c059c6fd4f4dd914dbf932617ea57 Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 6 Dec 2012 16:15:45 -0800 Subject: merge-tree: add comments to clarify what these functions are doing Rename the "branch1" parameter given to resolve() to "ours", to clarify what is going on. Also, annotate the unresolved_directory() function with some comments to show what decisions are made in each step, and highlight two bugs that need to be fixed. Add two tests to t4300 to illustrate these bugs. Signed-off-by: Junio C Hamano --- builtin/merge-tree.c | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) (limited to 'builtin') diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 95de162263..5704d51050 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -172,17 +172,17 @@ static char *traverse_path(const struct traverse_info *info, const struct name_e return make_traverse_path(path, info, n); } -static void resolve(const struct traverse_info *info, struct name_entry *branch1, struct name_entry *result) +static void resolve(const struct traverse_info *info, struct name_entry *ours, struct name_entry *result) { struct merge_list *orig, *final; const char *path; - /* If it's already branch1, don't bother showing it */ - if (!branch1) + /* If it's already ours, don't bother showing it */ + if (!ours) return; path = traverse_path(info, result); - orig = create_entry(2, branch1->mode, branch1->sha1, path); + orig = create_entry(2, ours->mode, ours->sha1, path); final = create_entry(0, result->mode, result->sha1, path); final->link = orig; @@ -205,6 +205,15 @@ static int unresolved_directory(const struct traverse_info *info, struct name_en } if (!S_ISDIR(p->mode)) return 0; + /* + * NEEDSWORK: this is broken. The path can originally be a file + * and then one side may have turned it into a directory, in which + * case we return and let the three-way merge as if the tree were + * a regular file. If the path that was originally a tree is + * now a file in either branch, fill_tree_descriptor() below will + * die when fed a blob sha1. + */ + newbase = traverse_path(info, p); buf0 = fill_tree_descriptor(t+0, n[0].sha1); buf1 = fill_tree_descriptor(t+1, n[1].sha1); @@ -288,20 +297,29 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s /* Same in both? */ if (same_entry(entry+1, entry+2)) { if (entry[0].sha1) { + /* Modified identically */ resolve(info, NULL, entry+1); return mask; } + /* "Both added the same" is left unresolved */ } if (same_entry(entry+0, entry+1)) { if (entry[2].sha1 && !S_ISDIR(entry[2].mode)) { + /* We did not touch, they modified -- take theirs */ resolve(info, entry+1, entry+2); return mask; } + /* + * If we did not touch a directory but they made it + * into a file, we fall through and unresolved() + * recurses down. Likewise for the opposite case. + */ } if (same_entry(entry+0, entry+2)) { if (entry[1].sha1 && !S_ISDIR(entry[1].mode)) { + /* We modified, they did not touch -- take ours */ resolve(info, NULL, entry+1); return mask; } -- cgit v1.2.3 From 35ffe7583108ab236dcf81226690388491d9962f Mon Sep 17 00:00:00 2001 From: Junio C Hamano Date: Thu, 13 Dec 2012 15:51:29 -0800 Subject: merge-tree: fix d/f conflicts The previous commit documented two known breakages revolving around a case where one side flips a tree into a blob (or vice versa), where the original code simply gets confused and feeds a mixture of trees and blobs into either the recursive merge-tree (and recursing into the blob will fail) or three-way merge (and merging tree contents together with blobs will fail). Fix it by feeding trees (and only trees) into the recursive merge-tree machinery and blobs (and only blobs) into the three-way content level merge machinery separately; when this happens, the entire merge has to be marked as conflicting at the structure level. Signed-off-by: Junio C Hamano --- builtin/merge-tree.c | 72 +++++++++++++++++++++++++++++----------------------- 1 file changed, 40 insertions(+), 32 deletions(-) (limited to 'builtin') diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c index 5704d51050..e0d0b7d28b 100644 --- a/builtin/merge-tree.c +++ b/builtin/merge-tree.c @@ -25,7 +25,7 @@ static void add_merge_entry(struct merge_list *entry) merge_result_end = &entry->next; } -static void merge_trees(struct tree_desc t[3], const char *base); +static void merge_trees_recursive(struct tree_desc t[3], const char *base, int df_conflict); static const char *explanation(struct merge_list *entry) { @@ -190,41 +190,35 @@ static void resolve(const struct traverse_info *info, struct name_entry *ours, s add_merge_entry(final); } -static int unresolved_directory(const struct traverse_info *info, struct name_entry n[3]) +static void unresolved_directory(const struct traverse_info *info, struct name_entry n[3], + int df_conflict) { char *newbase; struct name_entry *p; struct tree_desc t[3]; void *buf0, *buf1, *buf2; - p = n; - if (!p->mode) { - p++; - if (!p->mode) - p++; + for (p = n; p < n + 3; p++) { + if (p->mode && S_ISDIR(p->mode)) + break; } - if (!S_ISDIR(p->mode)) - return 0; - /* - * NEEDSWORK: this is broken. The path can originally be a file - * and then one side may have turned it into a directory, in which - * case we return and let the three-way merge as if the tree were - * a regular file. If the path that was originally a tree is - * now a file in either branch, fill_tree_descriptor() below will - * die when fed a blob sha1. - */ + if (n + 3 <= p) + return; /* there is no tree here */ newbase = traverse_path(info, p); - buf0 = fill_tree_descriptor(t+0, n[0].sha1); - buf1 = fill_tree_descriptor(t+1, n[1].sha1); - buf2 = fill_tree_descriptor(t+2, n[2].sha1); - merge_trees(t, newbase); + +#define ENTRY_SHA1(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->sha1 : NULL) + buf0 = fill_tree_descriptor(t+0, ENTRY_SHA1(n + 0)); + buf1 = fill_tree_descriptor(t+1, ENTRY_SHA1(n + 1)); + buf2 = fill_tree_descriptor(t+2, ENTRY_SHA1(n + 2)); +#undef ENTRY_SHA1 + + merge_trees_recursive(t, newbase, df_conflict); free(buf0); free(buf1); free(buf2); free(newbase); - return 1; } @@ -247,18 +241,26 @@ static struct merge_list *link_entry(unsigned stage, const struct traverse_info static void unresolved(const struct traverse_info *info, struct name_entry n[3]) { struct merge_list *entry = NULL; + int i; + unsigned dirmask = 0, mask = 0; + + for (i = 0; i < 3; i++) { + mask |= (1 << 1); + if (n[i].mode && S_ISDIR(n[i].mode)) + dirmask |= (1 << i); + } + + unresolved_directory(info, n, dirmask && (dirmask != mask)); - if (unresolved_directory(info, n)) + if (dirmask == mask) return; - /* - * Do them in reverse order so that the resulting link - * list has the stages in order - link_entry adds new - * links at the front. - */ - entry = link_entry(3, info, n + 2, entry); - entry = link_entry(2, info, n + 1, entry); - entry = link_entry(1, info, n + 0, entry); + if (n[2].mode && !S_ISDIR(n[2].mode)) + entry = link_entry(3, info, n + 2, entry); + if (n[1].mode && !S_ISDIR(n[1].mode)) + entry = link_entry(2, info, n + 1, entry); + if (n[0].mode && !S_ISDIR(n[0].mode)) + entry = link_entry(1, info, n + 0, entry); add_merge_entry(entry); } @@ -329,15 +331,21 @@ static int threeway_callback(int n, unsigned long mask, unsigned long dirmask, s return mask; } -static void merge_trees(struct tree_desc t[3], const char *base) +static void merge_trees_recursive(struct tree_desc t[3], const char *base, int df_conflict) { struct traverse_info info; setup_traverse_info(&info, base); + info.data = &df_conflict; info.fn = threeway_callback; traverse_trees(3, t, &info); } +static void merge_trees(struct tree_desc t[3], const char *base) +{ + merge_trees_recursive(t, base, 0); +} + static void *get_tree_descriptor(struct tree_desc *desc, const char *rev) { unsigned char sha1[20]; -- cgit v1.2.3