From d8523ca1b90641be4bddcdfc50fbf3a1be34adae Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 15 Aug 2019 14:40:30 -0700 Subject: merge-recursive: be consistent with assert In commit 8daec1df03de ("merge-recursive: switch from (oid,mode) pairs to a diff_filespec", 2019-04-05), an assertion on a->path && b->path was added for code readability to document that these both needed to be non-NULL at this point in the code. However, the subsequent lines also read o->path, so it should be included in the assert. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 6b812d6..1d960fa 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1043,7 +1043,7 @@ static int merge_3way(struct merge_options *opt, } } - assert(a->path && b->path); + assert(a->path && b->path && o->path); if (strcmp(a->path, b->path) || (opt->ancestor != NULL && strcmp(a->path, o->path) != 0)) { base_name = opt->ancestor == NULL ? NULL : -- cgit v0.10.2-6-g49f6 From 65c01c644250fb0a92f929c2fc61f33771bf480f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 15 Aug 2019 14:40:31 -0700 Subject: checkout: provide better conflict hunk description with detached HEAD When running 'git checkout -m' and using diff3 style conflict markers, we want all the conflict hunks (left-side, "common" or "merge base", and right-side) to have label markers letting the user know where each came from. The "common" hunk label (o.ancestor) came from old_branch_info->name, but that is NULL when HEAD is detached, which resulted in a blank label. Check for that case and provide an abbreviated commit hash instead. (Incidentally, this was the only case in the git codebase where merge_trees() was called with opt->ancestor being NULL. A subsequent commit will prevent similar problems by enforcing that merge_trees() always be called with opt->ancestor != NULL.) Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 6123f73..d5b946d 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -713,6 +713,7 @@ static int merge_working_tree(const struct checkout_opts *opts, struct tree *old_tree; struct merge_options o; struct strbuf sb = STRBUF_INIT; + struct strbuf old_commit_shortname = STRBUF_INIT; if (!opts->merge) return 1; @@ -768,6 +769,12 @@ static int merge_working_tree(const struct checkout_opts *opts, if (ret) return ret; o.ancestor = old_branch_info->name; + if (old_branch_info->name == NULL) { + strbuf_add_unique_abbrev(&old_commit_shortname, + &old_branch_info->commit->object.oid, + DEFAULT_ABBREV); + o.ancestor = old_commit_shortname.buf; + } o.branch1 = new_branch_info->name; o.branch2 = "local"; ret = merge_trees(&o, @@ -781,6 +788,7 @@ static int merge_working_tree(const struct checkout_opts *opts, opts, 0, writeout_error); strbuf_release(&o.obuf); + strbuf_release(&old_commit_shortname); if (ret) return ret; } -- cgit v0.10.2-6-g49f6 From 139ef37a2f4daf2071debeca8b2115b4e4b0a33f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Thu, 15 Aug 2019 14:40:32 -0700 Subject: merge-recursive: enforce opt->ancestor != NULL when calling merge_trees() We always want our conflict hunks to be labelled so that users can know where each came from. The previous commit fixed the one caller in the codebase which was not setting opt->ancestor (and thus not providing a label for the "merge base" conflict hunk in diff3-style conflict markers); add an assertion to prevent future codepaths from also overlooking this requirement. Enforcing this requirement also allows us to simplify the code for labelling the conflict hunks by no longer checking if the ancestor label is NULL. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 1d960fa..a67ea49 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1019,7 +1019,7 @@ static int merge_3way(struct merge_options *opt, { mmfile_t orig, src1, src2; struct ll_merge_options ll_opts = {0}; - char *base_name, *name1, *name2; + char *base, *name1, *name2; int merge_status; ll_opts.renormalize = opt->renormalize; @@ -1043,16 +1043,13 @@ static int merge_3way(struct merge_options *opt, } } - assert(a->path && b->path && o->path); - if (strcmp(a->path, b->path) || - (opt->ancestor != NULL && strcmp(a->path, o->path) != 0)) { - base_name = opt->ancestor == NULL ? NULL : - mkpathdup("%s:%s", opt->ancestor, o->path); + assert(a->path && b->path && o->path && opt->ancestor); + if (strcmp(a->path, b->path) || strcmp(a->path, o->path) != 0) { + base = mkpathdup("%s:%s", opt->ancestor, o->path); name1 = mkpathdup("%s:%s", branch1, a->path); name2 = mkpathdup("%s:%s", branch2, b->path); } else { - base_name = opt->ancestor == NULL ? NULL : - mkpathdup("%s", opt->ancestor); + base = mkpathdup("%s", opt->ancestor); name1 = mkpathdup("%s", branch1); name2 = mkpathdup("%s", branch2); } @@ -1061,11 +1058,11 @@ static int merge_3way(struct merge_options *opt, read_mmblob(&src1, &a->oid); read_mmblob(&src2, &b->oid); - merge_status = ll_merge(result_buf, a->path, &orig, base_name, + merge_status = ll_merge(result_buf, a->path, &orig, base, &src1, name1, &src2, name2, opt->repo->index, &ll_opts); - free(base_name); + free(base); free(name1); free(name2); free(orig.ptr); @@ -3390,6 +3387,8 @@ int merge_trees(struct merge_options *opt, int code, clean; struct strbuf sb = STRBUF_INIT; + assert(opt->ancestor != NULL); + if (!opt->call_depth && repo_index_has_changes(opt->repo, head, &sb)) { err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"), sb.buf); -- cgit v0.10.2-6-g49f6 From 743474cbfa8b65016dd02b4e56dd9f47f95652c3 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:24 -0700 Subject: merge-recursive: provide a better label for diff3 common ancestor In commit 7ca56aa07619 ("merge-recursive: add a label for ancestor", 2010-03-20), a label was added for the '||||||' line to make it have the more informative heading '|||||| merged common ancestors', with the statement: It would be nicer to use a more informative label. Perhaps someone will provide one some day. This chosen label was perfectly reasonable when recursiveness kicks in, i.e. when there are multiple merge bases. (I can't think of a better label in such cases.) But it is actually somewhat misleading when there is a unique merge base or no merge base. Change this based on the number of merge bases: >=2: "merged common ancestors" 1: 0: "" Tests have also been added to check that we get the right ancestor name for each of the three cases. Also, since merge_recursive() and merge_trees() have polar opposite pre-conditions for opt->ancestor, document merge_recursive()'s pre-condition with an assertion. (An assertion was added to merge_trees() already a few commits ago.) The differences in pre-conditions stem from two factors: (1) merge_trees() does not recurse and thus does not have multiple sub-merges to worry about -- each of which would require a different value for opt->ancestor, (2) merge_trees() is only passed trees rather than commits and thus cannot internally guess as good of a label. Thus, while external callers of merge_trees() are required to provide a non-NULL opt->ancestor, merge_recursive() expects to set this value itself. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index a67ea49..e6b84db 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3507,6 +3507,11 @@ int merge_recursive(struct merge_options *opt, struct commit *merged_common_ancestors; struct tree *mrtree; int clean; + const char *ancestor_name; + struct strbuf merge_base_abbrev = STRBUF_INIT; + + if (!opt->call_depth) + assert(opt->ancestor == NULL); if (show(opt, 4)) { output(opt, 4, _("Merging:")); @@ -3535,6 +3540,14 @@ int merge_recursive(struct merge_options *opt, tree = lookup_tree(opt->repo, opt->repo->hash_algo->empty_tree); merged_common_ancestors = make_virtual_commit(opt->repo, tree, "ancestor"); + ancestor_name = "empty tree"; + } else if (ca) { + ancestor_name = "merged common ancestors"; + } else { + strbuf_add_unique_abbrev(&merge_base_abbrev, + &merged_common_ancestors->object.oid, + DEFAULT_ABBREV); + ancestor_name = merge_base_abbrev.buf; } for (iter = ca; iter; iter = iter->next) { @@ -3568,10 +3581,11 @@ int merge_recursive(struct merge_options *opt, if (!opt->call_depth) repo_read_index(opt->repo); - opt->ancestor = "merged common ancestors"; + opt->ancestor = ancestor_name; clean = merge_trees(opt, get_commit_tree(h1), get_commit_tree(h2), get_commit_tree(merged_common_ancestors), &mrtree); + strbuf_release(&merge_base_abbrev); if (clean < 0) { flush_output(opt); return clean; diff --git a/t/t6036-recursive-corner-cases.sh b/t/t6036-recursive-corner-cases.sh index d23b948..7fddcc8 100755 --- a/t/t6036-recursive-corner-cases.sh +++ b/t/t6036-recursive-corner-cases.sh @@ -1562,6 +1562,7 @@ test_expect_success 'check nested conflicts' ' cd nested_conflicts && git clean -f && + MASTER=$(git rev-parse --short master) && git checkout L2^0 && # Merge must fail; there is a conflict @@ -1582,7 +1583,7 @@ test_expect_success 'check nested conflicts' ' git cat-file -p R1:a >theirs && test_must_fail git merge-file --diff3 \ -L "Temporary merge branch 1" \ - -L "merged common ancestors" \ + -L "$MASTER" \ -L "Temporary merge branch 2" \ ours \ base \ @@ -1594,7 +1595,7 @@ test_expect_success 'check nested conflicts' ' git cat-file -p R1:b >theirs && test_must_fail git merge-file --diff3 \ -L "Temporary merge branch 1" \ - -L "merged common ancestors" \ + -L "$MASTER" \ -L "Temporary merge branch 2" \ ours \ base \ @@ -1732,6 +1733,7 @@ test_expect_success 'check virtual merge base with nested conflicts' ' ( cd virtual_merge_base_has_nested_conflicts && + MASTER=$(git rev-parse --short master) && git checkout L3^0 && # Merge must fail; there is a conflict @@ -1760,7 +1762,7 @@ test_expect_success 'check virtual merge base with nested conflicts' ' cp left merged-once && test_must_fail git merge-file --diff3 \ -L "Temporary merge branch 1" \ - -L "merged common ancestors" \ + -L "$MASTER" \ -L "Temporary merge branch 2" \ merged-once \ base \ diff --git a/t/t6047-diff3-conflict-markers.sh b/t/t6047-diff3-conflict-markers.sh new file mode 100755 index 0000000..3fb68e0 --- /dev/null +++ b/t/t6047-diff3-conflict-markers.sh @@ -0,0 +1,189 @@ +#!/bin/sh + +test_description='recursive merge diff3 style conflict markers' + +. ./test-lib.sh + +# Setup: +# L1 +# \ +# ? +# / +# R1 +# +# Where: +# L1 and R1 both have a file named 'content' but have no common history +# + +test_expect_success 'setup no merge base' ' + test_create_repo no_merge_base && + ( + cd no_merge_base && + + git checkout -b L && + test_commit A content A && + + git checkout --orphan R && + test_commit B content B + ) +' + +test_expect_success 'check no merge base' ' + ( + cd no_merge_base && + + git checkout L^0 && + + test_must_fail git -c merge.conflictstyle=diff3 merge --allow-unrelated-histories -s recursive R^0 && + + grep "|||||| empty tree" content + ) +' + +# Setup: +# L1 +# / \ +# master ? +# \ / +# R1 +# +# Where: +# L1 and R1 have modified the same file ('content') in conflicting ways +# + +test_expect_success 'setup unique merge base' ' + test_create_repo unique_merge_base && + ( + cd unique_merge_base && + + test_commit base content "1 +2 +3 +4 +5 +" && + + git branch L && + git branch R && + + git checkout L && + test_commit L content "1 +2 +3 +4 +5 +7" && + + git checkout R && + git rm content && + test_commit R renamed "1 +2 +3 +4 +5 +six" + ) +' + +test_expect_success 'check unique merge base' ' + ( + cd unique_merge_base && + + git checkout L^0 && + MASTER=$(git rev-parse --short master) && + + test_must_fail git -c merge.conflictstyle=diff3 merge -s recursive R^0 && + + grep "|||||| $MASTER:content" renamed + ) +' + +# Setup: +# L1---L2--L3 +# / \ / \ +# master X1 ? +# \ / \ / +# R1---R2--R3 +# +# Where: +# commits L1 and R1 have modified the same file in non-conflicting ways +# X1 is an auto-generated merge-base used when merging L1 and R1 +# commits L2 and R2 are merges of R1 and L1 into L1 and R1, respectively +# commits L3 and R3 both modify 'content' in conflicting ways +# + +test_expect_success 'setup multiple merge bases' ' + test_create_repo multiple_merge_bases && + ( + cd multiple_merge_bases && + + test_commit initial content "1 +2 +3 +4 +5" && + + git branch L && + git branch R && + + # Create L1 + git checkout L && + test_commit L1 content "0 +1 +2 +3 +4 +5" && + + # Create R1 + git checkout R && + test_commit R1 content "1 +2 +3 +4 +5 +6" && + + # Create L2 + git checkout L && + git merge R1 && + + # Create R2 + git checkout R && + git merge L1 && + + # Create L3 + git checkout L && + test_commit L3 content "0 +1 +2 +3 +4 +5 +A" && + + # Create R3 + git checkout R && + git rm content && + test_commit R3 renamed "0 +2 +3 +4 +5 +six" + ) +' + +test_expect_success 'check multiple merge bases' ' + ( + cd multiple_merge_bases && + + git checkout L^0 && + + test_must_fail git -c merge.conflictstyle=diff3 merge -s recursive R^0 && + + grep "|||||| merged common ancestors:content" renamed + ) +' + +test_done -- cgit v0.10.2-6-g49f6 From 8e01251694fa277df53e5c52c137f0b4134d2cd5 Mon Sep 17 00:00:00 2001 From: Derrick Stolee Date: Sat, 17 Aug 2019 11:41:25 -0700 Subject: merge-recursive: introduce an enum for detect_directory_renames values Improve code readability by introducing an enum to replace the not-quite-boolean values taken on by detect_directory_renames. Signed-off-by: Derrick Stolee Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/am.c b/builtin/am.c index 1aea657..037e828 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -1538,7 +1538,7 @@ static int fall_back_threeway(const struct am_state *state, const char *index_pa o.branch1 = "HEAD"; their_tree_name = xstrfmt("%.*s", linelen(state->msg), state->msg); o.branch2 = their_tree_name; - o.detect_directory_renames = 0; + o.detect_directory_renames = MERGE_DIRECTORY_RENAMES_NONE; if (state->quiet) o.verbosity = 0; diff --git a/merge-recursive.c b/merge-recursive.c index e6b84db..9622781 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1375,7 +1375,8 @@ static int handle_rename_via_dir(struct merge_options *opt, const struct rename *ren = ci->ren1; const struct diff_filespec *dest = ren->pair->two; char *file_path = dest->path; - int mark_conflicted = (opt->detect_directory_renames == 1); + int mark_conflicted = (opt->detect_directory_renames == + MERGE_DIRECTORY_RENAMES_CONFLICT); assert(ren->dir_rename_original_dest); if (!opt->call_depth && would_lose_untracked(opt, dest->path)) { @@ -2860,8 +2861,9 @@ static int detect_and_process_renames(struct merge_options *opt, head_pairs = get_diffpairs(opt, common, head); merge_pairs = get_diffpairs(opt, common, merge); - if ((opt->detect_directory_renames == 2) || - (opt->detect_directory_renames == 1 && !opt->call_depth)) { + if ((opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE) || + (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT && + !opt->call_depth)) { dir_re_head = get_directory_renames(head_pairs); dir_re_merge = get_directory_renames(merge_pairs); @@ -3119,7 +3121,8 @@ static int handle_rename_normal(struct merge_options *opt, clean = handle_content_merge(&mfi, opt, path, was_dirty(opt, path), o, a, b, ci); - if (clean && opt->detect_directory_renames == 1 && + if (clean && + opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT && ren->dir_rename_original_dest) { if (update_stages(opt, path, NULL, @@ -3164,12 +3167,12 @@ static int warn_about_dir_renamed_entries(struct merge_options *opt, return clean; /* Sanity checks */ - assert(opt->detect_directory_renames > 0); + assert(opt->detect_directory_renames > MERGE_DIRECTORY_RENAMES_NONE); assert(ren->dir_rename_original_type == 'A' || ren->dir_rename_original_type == 'R'); /* Check whether to treat directory renames as a conflict */ - clean = (opt->detect_directory_renames == 2); + clean = (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE); is_add = (ren->dir_rename_original_type == 'A'); if (ren->dir_rename_original_type == 'A' && clean) { @@ -3679,9 +3682,12 @@ static void merge_recursive_config(struct merge_options *opt) if (!git_config_get_string("merge.directoryrenames", &value)) { int boolval = git_parse_maybe_bool(value); if (0 <= boolval) { - opt->detect_directory_renames = boolval ? 2 : 0; + opt->detect_directory_renames = boolval ? + MERGE_DIRECTORY_RENAMES_TRUE : + MERGE_DIRECTORY_RENAMES_NONE; } else if (!strcasecmp(value, "conflict")) { - opt->detect_directory_renames = 1; + opt->detect_directory_renames = + MERGE_DIRECTORY_RENAMES_CONFLICT; } /* avoid erroring on values from future versions of git */ free(value); } @@ -3701,7 +3707,7 @@ void init_merge_options(struct merge_options *opt, opt->renormalize = 0; opt->diff_detect_rename = -1; opt->merge_detect_rename = -1; - opt->detect_directory_renames = 1; + opt->detect_directory_renames = MERGE_DIRECTORY_RENAMES_CONFLICT; merge_recursive_config(opt); merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); if (merge_verbosity) diff --git a/merge-recursive.h b/merge-recursive.h index c2b7bb6..f1b6ef3 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -22,7 +22,11 @@ struct merge_options { unsigned renormalize : 1; long xdl_opts; int verbosity; - int detect_directory_renames; + enum { + MERGE_DIRECTORY_RENAMES_NONE = 0, + MERGE_DIRECTORY_RENAMES_CONFLICT = 1, + MERGE_DIRECTORY_RENAMES_TRUE = 2 + } detect_directory_renames; int diff_detect_rename; int merge_detect_rename; int diff_rename_limit; -- cgit v0.10.2-6-g49f6 From f836bf393731e141a289f6b82d549cf0a10a2bcc Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:26 -0700 Subject: merge-recursive: future-proof update_file_flags() against memory leaks There is a 'free_buf' label to which all but one of the error paths in update_file_flags() jump; that error case involves a NULL buf and is thus not a memory leak. However, make that error case execute the same deallocation code anyway so that if anyone adds any additional memory allocations or deallocations, then all error paths correctly deallocate resources. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 9622781..1d4df95 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -934,9 +934,11 @@ static int update_file_flags(struct merge_options *opt, } buf = read_object_file(&contents->oid, &type, &size); - if (!buf) - return err(opt, _("cannot read object %s '%s'"), - oid_to_hex(&contents->oid), path); + if (!buf) { + ret = err(opt, _("cannot read object %s '%s'"), + oid_to_hex(&contents->oid), path); + goto free_buf; + } if (type != OBJ_BLOB) { ret = err(opt, _("blob expected for %s '%s'"), oid_to_hex(&contents->oid), path); -- cgit v0.10.2-6-g49f6 From 10f751c06bb3dbc2afed0e80ecfbd6242e12d538 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:27 -0700 Subject: merge-recursive: remove another implicit dependency on the_repository Commit d7cf3a96e9a0 ("merge-recursive.c: remove implicit dependency on the_repository", 2019-01-12) and follow-ups like commit 34e7771bc644 ("Use the right 'struct repository' instead of the_repository", 2019-06-27), removed most implicit uses of the_repository. Convert calls to get_commit_tree() to instead use repo_get_commit_tree() to get rid of another. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 1d4df95..88a33e6 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3587,8 +3587,11 @@ int merge_recursive(struct merge_options *opt, repo_read_index(opt->repo); opt->ancestor = ancestor_name; - clean = merge_trees(opt, get_commit_tree(h1), get_commit_tree(h2), - get_commit_tree(merged_common_ancestors), + clean = merge_trees(opt, + repo_get_commit_tree(opt->repo, h1), + repo_get_commit_tree(opt->repo, h2), + repo_get_commit_tree(opt->repo, + merged_common_ancestors), &mrtree); strbuf_release(&merge_base_abbrev); if (clean < 0) { -- cgit v0.10.2-6-g49f6 From 9822175d2b349be1ed7a704d3a56dc912f5a7510 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:28 -0700 Subject: Ensure index matches head before invoking merge machinery, round N This is the bug that just won't die; there always seems to be another form of it somewhere. See the commit message of 55f39cf7551b ("merge: fix misleading pre-merge check documentation", 2018-06-30) for a more detailed explanation), but in short: builtin/merge.c contains this important requirement for merge strategies: ...the index must be in sync with the head commit. The strategies are responsible to ensure this. This condition is important to enforce because there are two likely failure cases when the index isn't in sync with the head commit: * we silently throw away changes the user had staged before the merge * we accidentally (and silently) include changes in the merge that were not part of either of the branches/trees being merged Discarding users' work and mis-merging are both bad outcomes, especially when done silently, so naturally this rule was stated sternly -- but, unfortunately totally ignored in practice unless and until actual bugs were found. But, fear not: the bugs from this were fixed in commit ee6566e8d70d ("[PATCH] Rewrite read-tree", 2005-09-05) through a rewrite of read-tree (again, commit 55f39cf7551b has a more detailed explanation of how this affected merge). And it was fixed again in commit 160252f81626 ("git-merge-ours: make sure our index matches HEAD", 2005-11-03) ...and it was fixed again in commit 3ec62ad9ffba ("merge-octopus: abort if index does not match HEAD", 2016-04-09) ...and again in commit 65170c07d466 ("merge-recursive: avoid incorporating uncommitted changes in a merge", 2017-12-21) ...and again in commit eddd1a411d93 ("merge-recursive: enforce rule that index matches head before merging", 2018-06-30) ...with multiple testcases added to the testsuite that could be enumerated in even more commits. Then, finally, in a patch in the same series as the last fix above, the documentation about this requirement was fixed in commit 55f39cf7551b ("merge: fix misleading pre-merge check documentation", 2018-06-30), and we all lived happily ever after... Unfortunately, "ever after" apparently denotes a limited time and it expired today. The merge-recursive rule to enforce that index matches head was at the beginning of merge_trees() and would only trigger when opt->call_depth was 0. Since merge_recursive() doesn't call merge_trees() until after returning from recursing, this meant that the check wasn't triggered by merge_recursive() until it had first finished all the intermediate merges to create virtual merge bases. That is a potentially HUGE amount of computation (and writing of intermediate merge results into the .git/objects directory) before it errors out and says, in effect, "Sorry, I can't do any merging because you have some local changes that would be overwritten." Trying to enforce that all of merge_trees(), merge_recursive(), and merge_recursive_generic() checked the index == head condition earlier resulted in a bunch of broken tests. It turns out that merge_recursive() has code to drop and reload the cache while recursing to create intermediate virtual merge bases, but unfortunately that code runs even when no recursion is necessary. This unconditional dropping and reloading of the cache masked a few bugs: * builtin/merge-recursive.c: didn't even bother loading the index. * builtin/stash.c: feels like a fake 'builtin' because it repeatedly invokes git subprocesses all over the place, mixed with other operations. In particular, invoking "git reset" will reset the index on disk, but the parent process that invoked it won't automatically have its in-memory index updated. * t3030-merge-recursive.h: this test has always been broken in that it didn't make sure to make index match head before running. But, it didn't care about the index or even the merge result, just the verbose output while running. While commit eddd1a411d93 ("merge-recursive: enforce rule that index matches head before merging", 2018-06-30) should have uncovered this broken test, it used a test_must_fail wrapper around the merge-recursive call because it was known that the merge resulted in a rename/rename conflict. Thus, that fix only made this test fail for a different reason, and since the index == head check didn't happen until after coming all the way back out of the recursion, the testcase had enough information to pass the one check that it did perform. So, load the index in builtin/merge-recursive.c, reload the in-memory index in builtin/stash.c, and modify the t3030 testcase to correctly setup the index and make sure that the test fails in the expected way (meaning it reports a rename/rename conflict). This makes sure that all callers actually make the index match head. The next commit will then enforce the condition that index matches head earlier so this problem doesn't return in the future. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c index 5b910e3..a4bfd8f 100644 --- a/builtin/merge-recursive.c +++ b/builtin/merge-recursive.c @@ -1,3 +1,4 @@ +#include "cache.h" #include "builtin.h" #include "commit.h" #include "tag.h" @@ -63,6 +64,9 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix) if (argc - i != 3) /* "--" "" "" */ die(_("not handling anything other than two heads merge.")); + if (repo_read_index_unmerged(the_repository)) + die_resolve_conflict("merge"); + o.branch1 = argv[++i]; o.branch2 = argv[++i]; diff --git a/builtin/stash.c b/builtin/stash.c index b5a301f..4aa4778 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -427,6 +427,8 @@ static int do_apply_stash(const char *prefix, struct stash_info *info, return error(_("could not save index tree")); reset_head(); + discard_cache(); + read_cache(); } } diff --git a/t/t3030-merge-recursive.sh b/t/t3030-merge-recursive.sh index ff641b3..a37bcc5 100755 --- a/t/t3030-merge-recursive.sh +++ b/t/t3030-merge-recursive.sh @@ -667,15 +667,22 @@ test_expect_success 'merging with triple rename across D/F conflict' ' test_expect_success 'merge-recursive remembers the names of all base trees' ' git reset --hard HEAD && + # make the index match $c1 so that merge-recursive below does not + # fail early + git diff --binary HEAD $c1 -- | git apply --cached && + # more trees than static slots used by oid_to_hex() for commit in $c0 $c2 $c4 $c5 $c6 $c7 do git rev-parse "$commit^{tree}" done >trees && - # ignore the return code -- it only fails because the input is weird + # ignore the return code; it only fails because the input is weird... test_must_fail git -c merge.verbosity=5 merge-recursive $(cat trees) -- $c1 $c3 >out && + # ...but make sure it fails in the expected way + test_i18ngrep CONFLICT.*rename/rename out && + # merge-recursive prints in reverse order, but we do not care sort expect && sed -n "s/^virtual //p" out | sort >actual && -- cgit v0.10.2-6-g49f6 From 98a1d3d88895a1cbc01b88627776bb4e2bfd6b84 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:29 -0700 Subject: merge-recursive: exit early if index != head We had a rule to enforce that the index matches head, but it was found at the beginning of merge_trees() and would only trigger when opt->call_depth was 0. Since merge_recursive() doesn't call merge_trees() until after returning from recursing, this meant that the check wasn't triggered by merge_recursive() until it had first finished all the intermediate merges to create virtual merge bases. That is a potentially huge amount of computation (and writing of intermediate merge results into the .git/objects directory) before it errors out and says, in effect, "Sorry, I can't do any merging because you have some local changes that would be overwritten." Further, not enforcing this requirement earlier allowed other bugs (such as an unintentional unconditional dropping and reloading of the index in merge_recursive() even when no recursion was necessary), to mask bugs in other callers (which were fixed in the commit prior to this one). Make sure we do the index == head check at the beginning of the merge, and error out immediately if it fails. While we're at it, fix a small leak in the show-the-error codepath. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 88a33e6..2a254d5 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3382,23 +3382,14 @@ static int process_entry(struct merge_options *opt, return clean_merge; } -int merge_trees(struct merge_options *opt, - struct tree *head, - struct tree *merge, - struct tree *common, - struct tree **result) +static int merge_trees_internal(struct merge_options *opt, + struct tree *head, + struct tree *merge, + struct tree *common, + struct tree **result) { struct index_state *istate = opt->repo->index; int code, clean; - struct strbuf sb = STRBUF_INIT; - - assert(opt->ancestor != NULL); - - if (!opt->call_depth && repo_index_has_changes(opt->repo, head, &sb)) { - err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"), - sb.buf); - return -1; - } if (opt->subtree_shift) { merge = shift_tree_object(opt->repo, head, merge, opt->subtree_shift); @@ -3502,11 +3493,11 @@ static struct commit_list *reverse_commit_list(struct commit_list *list) * Merge the commits h1 and h2, return the resulting virtual * commit object and a flag indicating the cleanness of the merge. */ -int merge_recursive(struct merge_options *opt, - struct commit *h1, - struct commit *h2, - struct commit_list *ca, - struct commit **result) +static int merge_recursive_internal(struct merge_options *opt, + struct commit *h1, + struct commit *h2, + struct commit_list *ca, + struct commit **result) { struct commit_list *iter; struct commit *merged_common_ancestors; @@ -3515,9 +3506,6 @@ int merge_recursive(struct merge_options *opt, const char *ancestor_name; struct strbuf merge_base_abbrev = STRBUF_INIT; - if (!opt->call_depth) - assert(opt->ancestor == NULL); - if (show(opt, 4)) { output(opt, 4, _("Merging:")); output_commit_title(opt, h1); @@ -3571,7 +3559,7 @@ int merge_recursive(struct merge_options *opt, saved_b2 = opt->branch2; opt->branch1 = "Temporary merge branch 1"; opt->branch2 = "Temporary merge branch 2"; - if (merge_recursive(opt, merged_common_ancestors, iter->item, + if (merge_recursive_internal(opt, merged_common_ancestors, iter->item, NULL, &merged_common_ancestors) < 0) return -1; opt->branch1 = saved_b1; @@ -3587,12 +3575,12 @@ int merge_recursive(struct merge_options *opt, repo_read_index(opt->repo); opt->ancestor = ancestor_name; - clean = merge_trees(opt, - repo_get_commit_tree(opt->repo, h1), - repo_get_commit_tree(opt->repo, h2), - repo_get_commit_tree(opt->repo, - merged_common_ancestors), - &mrtree); + clean = merge_trees_internal(opt, + repo_get_commit_tree(opt->repo, h1), + repo_get_commit_tree(opt->repo, h2), + repo_get_commit_tree(opt->repo, + merged_common_ancestors), + &mrtree); strbuf_release(&merge_base_abbrev); if (clean < 0) { flush_output(opt); @@ -3613,6 +3601,61 @@ int merge_recursive(struct merge_options *opt, return clean; } +static int merge_start(struct merge_options *opt, struct tree *head) +{ + struct strbuf sb = STRBUF_INIT; + + if (repo_index_has_changes(opt->repo, head, &sb)) { + err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"), + sb.buf); + strbuf_release(&sb); + return -1; + } + + return 0; +} + +static void merge_finalize(struct merge_options *opt) +{ + /* Common code for wrapping up merges will be added here later */ +} + +int merge_trees(struct merge_options *opt, + struct tree *head, + struct tree *merge, + struct tree *common, + struct tree **result) +{ + int clean; + + assert(opt->ancestor != NULL); + + if (merge_start(opt, head)) + return -1; + clean = merge_trees_internal(opt, head, merge, common, result); + merge_finalize(opt); + + return clean; +} + +int merge_recursive(struct merge_options *opt, + struct commit *h1, + struct commit *h2, + struct commit_list *ca, + struct commit **result) +{ + int clean; + + assert(opt->ancestor == NULL); + + if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) + return -1; + clean = merge_recursive_internal(opt, h1, h2, ca, result); + merge_finalize(opt); + + return clean; +} + static struct commit *get_ref(struct repository *repo, const struct object_id *oid, const char *name) { -- cgit v0.10.2-6-g49f6 From b4db8a2b768742f4f43d4a6cdb1db39c2ffc9f7f Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:30 -0700 Subject: merge-recursive: remove useless parameter in merge_trees() merge_trees() took a results parameter that would only be written when opt->call_depth was positive, which is never the case now that merge_trees_internal() has been split from merge_trees(). Remove the misleading and unused parameter from merge_trees(). While at it, add some comments explaining how the output of merge_trees() and merge_recursive() differ. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index d5b946d..90e0eaf 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -708,7 +708,6 @@ static int merge_working_tree(const struct checkout_opts *opts, * give up or do a real merge, depending on * whether the merge flag was used. */ - struct tree *result; struct tree *work; struct tree *old_tree; struct merge_options o; @@ -780,8 +779,7 @@ static int merge_working_tree(const struct checkout_opts *opts, ret = merge_trees(&o, new_tree, work, - old_tree, - &result); + old_tree); if (ret < 0) exit(128); ret = reset_tree(new_tree, diff --git a/merge-recursive.c b/merge-recursive.c index 2a254d5..4ce783d 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3623,16 +3623,16 @@ static void merge_finalize(struct merge_options *opt) int merge_trees(struct merge_options *opt, struct tree *head, struct tree *merge, - struct tree *common, - struct tree **result) + struct tree *common) { int clean; + struct tree *ignored; assert(opt->ancestor != NULL); if (merge_start(opt, head)) return -1; - clean = merge_trees_internal(opt, head, merge, common, result); + clean = merge_trees_internal(opt, head, merge, common, &ignored); merge_finalize(opt); return clean; diff --git a/merge-recursive.h b/merge-recursive.h index f1b6ef3..18012ff 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -74,19 +74,31 @@ static inline int merge_detect_rename(struct merge_options *o) o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1; } -/* merge_trees() but with recursive ancestor consolidation */ +/* + * merge_recursive is like merge_trees() but with recursive ancestor + * consolidation, and when successful, it creates an actual commit + * and writes its address to *result. + * + * NOTE: empirically, about a decade ago it was determined that with more + * than two merge bases, optimal behavior was found when the + * ancestors were passed in the order of oldest merge base to newest + * one. Also, ancestors will be consumed (emptied) so make a copy if + * you need it. + */ int merge_recursive(struct merge_options *o, struct commit *h1, struct commit *h2, struct commit_list *ancestors, struct commit **result); -/* rename-detecting three-way merge, no recursion */ +/* + * rename-detecting three-way merge, no recursion; result of merge is written + * to opt->repo->index. + */ int merge_trees(struct merge_options *o, struct tree *head, struct tree *merge, - struct tree *common, - struct tree **result); + struct tree *common); /* * "git-merge-recursive" can be fed trees; wrap them into diff --git a/sequencer.c b/sequencer.c index 34ebf8e..c4ed30f 100644 --- a/sequencer.c +++ b/sequencer.c @@ -586,7 +586,7 @@ static int do_recursive_merge(struct repository *r, struct replay_opts *opts) { struct merge_options o; - struct tree *result, *next_tree, *base_tree, *head_tree; + struct tree *next_tree, *base_tree, *head_tree; int clean; char **xopt; struct lock_file index_lock = LOCK_INIT; @@ -613,7 +613,7 @@ static int do_recursive_merge(struct repository *r, clean = merge_trees(&o, head_tree, - next_tree, base_tree, &result); + next_tree, base_tree); if (is_rebase_i(opts) && clean <= 0) fputs(o.obuf.buf, stdout); strbuf_release(&o.obuf); -- cgit v0.10.2-6-g49f6 From 345480d1ed462135d98e99cb5b5a426da27257c8 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:31 -0700 Subject: merge-recursive: don't force external callers to do our logging Alternatively, you can view this as "make the merge functions behave more similarly." merge-recursive has three different entry points: merge_trees(), merge_recursive(), and merge_recursive_generic(). Two of these would call diff_warn_rename_limit(), but merge_trees() didn't. This lead to callers of merge_trees() needing to manually call diff_warn_rename_limit() themselves. Move this to the new merge_finalize() function to make sure that all three entry points run this function. Note that there are two external callers of merge_trees(), one in sequencer.c and one in builtin/checkout.c. The one in sequencer.c is cleaned up by this patch and just transfers where the call to diff_warn_rename_limit() is made; the one in builtin/checkout.c is for switching to a different commit and in the very rare case where the warning might be triggered, it would probably be helpful to include (e.g. if someone is modifying a file that has been renamed in moving to the other commit, but there are so many renames between the commits that the limit kicks in and none are detected, it may help to have an explanation about why they got a delete/modify conflict instead of a proper content merge in a renamed file). Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 4ce783d..fda67dd 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3595,9 +3595,6 @@ static int merge_recursive_internal(struct merge_options *opt, flush_output(opt); if (!opt->call_depth && opt->buffer_output < 2) strbuf_release(&opt->obuf); - if (show(opt, 2)) - diff_warn_rename_limit("merge.renamelimit", - opt->needed_rename_limit, 0); return clean; } @@ -3617,7 +3614,9 @@ static int merge_start(struct merge_options *opt, struct tree *head) static void merge_finalize(struct merge_options *opt) { - /* Common code for wrapping up merges will be added here later */ + if (show(opt, 2)) + diff_warn_rename_limit("merge.renamelimit", + opt->needed_rename_limit, 0); } int merge_trees(struct merge_options *opt, diff --git a/sequencer.c b/sequencer.c index c4ed30f..094a4dd 100644 --- a/sequencer.c +++ b/sequencer.c @@ -617,7 +617,6 @@ static int do_recursive_merge(struct repository *r, if (is_rebase_i(opts) && clean <= 0) fputs(o.obuf.buf, stdout); strbuf_release(&o.obuf); - diff_warn_rename_limit("merge.renamelimit", o.needed_rename_limit, 0); if (clean < 0) { rollback_lock_file(&index_lock); return clean; -- cgit v0.10.2-6-g49f6 From 724dd767b245db588840d7e9dbd46687ee84020b Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:32 -0700 Subject: cache-tree: share code between functions writing an index as a tree write_tree_from_memory() appeared to be a merge-recursive special that basically duplicated write_index_as_tree(). The two have a different signature, but the bigger difference was just that write_index_as_tree() would always unconditionally read the index off of disk instead of working on the current in-memory index. So: * split out common code into write_index_as_tree_internal() * rename write_tree_from_memory() to write_inmemory_index_as_tree(), make it call write_index_as_tree_internal(), and move it to cache-tree.c Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/builtin/checkout.c b/builtin/checkout.c index 90e0eaf..5e41fc1 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -760,7 +760,7 @@ static int merge_working_tree(const struct checkout_opts *opts, */ init_merge_options(&o, the_repository); o.verbosity = 0; - work = write_tree_from_memory(&o); + work = write_in_core_index_as_tree(the_repository); ret = reset_tree(new_tree, opts, 1, diff --git a/cache-tree.c b/cache-tree.c index 706ffcf..fbb5252 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -608,11 +608,66 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat return it; } +static int write_index_as_tree_internal(struct object_id *oid, + struct index_state *index_state, + int cache_tree_valid, + int flags, + const char *prefix) +{ + if (flags & WRITE_TREE_IGNORE_CACHE_TREE) { + cache_tree_free(&index_state->cache_tree); + cache_tree_valid = 0; + } + + if (!index_state->cache_tree) + index_state->cache_tree = cache_tree(); + + if (!cache_tree_valid && cache_tree_update(index_state, flags) < 0) + return WRITE_TREE_UNMERGED_INDEX; + + if (prefix) { + struct cache_tree *subtree; + subtree = cache_tree_find(index_state->cache_tree, prefix); + if (!subtree) + return WRITE_TREE_PREFIX_ERROR; + oidcpy(oid, &subtree->oid); + } + else + oidcpy(oid, &index_state->cache_tree->oid); + + return 0; +} + +struct tree* write_in_core_index_as_tree(struct repository *repo) { + struct object_id o; + int was_valid, ret; + + struct index_state *index_state = repo->index; + was_valid = index_state->cache_tree && + cache_tree_fully_valid(index_state->cache_tree); + + ret = write_index_as_tree_internal(&o, index_state, was_valid, 0, NULL); + if (ret == WRITE_TREE_UNMERGED_INDEX) { + int i; + fprintf(stderr, "BUG: There are unmerged index entries:\n"); + for (i = 0; i < index_state->cache_nr; i++) { + const struct cache_entry *ce = index_state->cache[i]; + if (ce_stage(ce)) + fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce), + (int)ce_namelen(ce), ce->name); + } + BUG("unmerged index entries when writing inmemory index"); + } + + return lookup_tree(repo, &index_state->cache_tree->oid); +} + + int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix) { int entries, was_valid; struct lock_file lock_file = LOCK_INIT; - int ret = 0; + int ret; hold_lock_file_for_update(&lock_file, index_path, LOCK_DIE_ON_ERROR); @@ -621,18 +676,14 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, ret = WRITE_TREE_UNREADABLE_INDEX; goto out; } - if (flags & WRITE_TREE_IGNORE_CACHE_TREE) - cache_tree_free(&index_state->cache_tree); - if (!index_state->cache_tree) - index_state->cache_tree = cache_tree(); + was_valid = !(flags & WRITE_TREE_IGNORE_CACHE_TREE) && + index_state->cache_tree && + cache_tree_fully_valid(index_state->cache_tree); - was_valid = cache_tree_fully_valid(index_state->cache_tree); - if (!was_valid) { - if (cache_tree_update(index_state, flags) < 0) { - ret = WRITE_TREE_UNMERGED_INDEX; - goto out; - } + ret = write_index_as_tree_internal(oid, index_state, was_valid, flags, + prefix); + if (!ret && !was_valid) { write_locked_index(index_state, &lock_file, COMMIT_LOCK); /* Not being able to write is fine -- we are only interested * in updating the cache-tree part, and if the next caller @@ -642,18 +693,6 @@ int write_index_as_tree(struct object_id *oid, struct index_state *index_state, */ } - if (prefix) { - struct cache_tree *subtree; - subtree = cache_tree_find(index_state->cache_tree, prefix); - if (!subtree) { - ret = WRITE_TREE_PREFIX_ERROR; - goto out; - } - oidcpy(oid, &subtree->oid); - } - else - oidcpy(oid, &index_state->cache_tree->oid); - out: rollback_lock_file(&lock_file); return ret; diff --git a/cache-tree.h b/cache-tree.h index 757bbc4..639bfa5 100644 --- a/cache-tree.h +++ b/cache-tree.h @@ -34,7 +34,7 @@ int cache_tree_fully_valid(struct cache_tree *); int cache_tree_update(struct index_state *, int); void cache_tree_verify(struct repository *, struct index_state *); -/* bitmasks to write_cache_as_tree flags */ +/* bitmasks to write_index_as_tree flags */ #define WRITE_TREE_MISSING_OK 1 #define WRITE_TREE_IGNORE_CACHE_TREE 2 #define WRITE_TREE_DRY_RUN 4 @@ -46,6 +46,7 @@ void cache_tree_verify(struct repository *, struct index_state *); #define WRITE_TREE_UNMERGED_INDEX (-2) #define WRITE_TREE_PREFIX_ERROR (-3) +struct tree* write_in_core_index_as_tree(struct repository *repo); int write_index_as_tree(struct object_id *oid, struct index_state *index_state, const char *index_path, int flags, const char *prefix); void prime_cache_tree(struct repository *, struct index_state *, struct tree *); diff --git a/merge-recursive.c b/merge-recursive.c index fda67dd..ae50935 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -412,37 +412,6 @@ static void unpack_trees_finish(struct merge_options *opt) clear_unpack_trees_porcelain(&opt->unpack_opts); } -struct tree *write_tree_from_memory(struct merge_options *opt) -{ - struct tree *result = NULL; - struct index_state *istate = opt->repo->index; - - if (unmerged_index(istate)) { - int i; - fprintf(stderr, "BUG: There are unmerged index entries:\n"); - for (i = 0; i < istate->cache_nr; i++) { - const struct cache_entry *ce = istate->cache[i]; - if (ce_stage(ce)) - fprintf(stderr, "BUG: %d %.*s\n", ce_stage(ce), - (int)ce_namelen(ce), ce->name); - } - BUG("unmerged index entries in merge-recursive.c"); - } - - if (!istate->cache_tree) - istate->cache_tree = cache_tree(); - - if (!cache_tree_fully_valid(istate->cache_tree) && - cache_tree_update(istate, 0) < 0) { - err(opt, _("error building trees")); - return NULL; - } - - result = lookup_tree(opt->repo, &istate->cache_tree->oid); - - return result; -} - static int save_files_dirs(const struct object_id *oid, struct strbuf *base, const char *path, unsigned int mode, int stage, void *context) @@ -3472,7 +3441,8 @@ static int merge_trees_internal(struct merge_options *opt, unpack_trees_finish(opt); - if (opt->call_depth && !(*result = write_tree_from_memory(opt))) + if (opt->call_depth && + !(*result = write_in_core_index_as_tree(opt->repo))) return -1; return clean; diff --git a/merge-recursive.h b/merge-recursive.h index 18012ff..0a3033b 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -113,7 +113,6 @@ int merge_recursive_generic(struct merge_options *o, void init_merge_options(struct merge_options *o, struct repository *repo); -struct tree *write_tree_from_memory(struct merge_options *o); int parse_merge_opt(struct merge_options *out, const char *s); -- cgit v0.10.2-6-g49f6 From 4d7101e25cfd5fba24ddc310f10c3962edba8b4d Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:33 -0700 Subject: merge-recursive: fix some overly long lines No substantive code change, just add some line breaks to fix lines that have grown in length due to various refactorings. Most remaining lines of excessive length in merge-recursive include error messages and it's not clear that splitting those improves things. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index ae50935..720678c 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -681,7 +681,9 @@ static void add_flattened_path(struct strbuf *out, const char *s) out->buf[i] = '_'; } -static char *unique_path(struct merge_options *opt, const char *path, const char *branch) +static char *unique_path(struct merge_options *opt, + const char *path, + const char *branch) { struct path_hashmap_entry *entry; struct strbuf newpath = STRBUF_INIT; @@ -915,7 +917,8 @@ static int update_file_flags(struct merge_options *opt, } if (S_ISREG(contents->mode)) { struct strbuf strbuf = STRBUF_INIT; - if (convert_to_working_tree(opt->repo->index, path, buf, size, &strbuf)) { + if (convert_to_working_tree(opt->repo->index, + path, buf, size, &strbuf)) { free(buf); size = strbuf.len; buf = strbuf_detach(&strbuf, NULL); @@ -3393,7 +3396,8 @@ static int merge_trees_internal(struct merge_options *opt, * opposed to decaring a local hashmap is for convenience * so that we don't have to pass it to around. */ - hashmap_init(&opt->current_file_dir_set, path_hashmap_cmp, NULL, 512); + hashmap_init(&opt->current_file_dir_set, path_hashmap_cmp, + NULL, 512); get_files_dirs(opt, head); get_files_dirs(opt, merge); @@ -3502,7 +3506,8 @@ static int merge_recursive_internal(struct merge_options *opt, struct tree *tree; tree = lookup_tree(opt->repo, opt->repo->hash_algo->empty_tree); - merged_common_ancestors = make_virtual_commit(opt->repo, tree, "ancestor"); + merged_common_ancestors = make_virtual_commit(opt->repo, + tree, "ancestor"); ancestor_name = "empty tree"; } else if (ca) { ancestor_name = "merged common ancestors"; @@ -3625,7 +3630,8 @@ int merge_recursive(struct merge_options *opt, return clean; } -static struct commit *get_ref(struct repository *repo, const struct object_id *oid, +static struct commit *get_ref(struct repository *repo, + const struct object_id *oid, const char *name) { struct object *object; @@ -3660,7 +3666,8 @@ int merge_recursive_generic(struct merge_options *opt, int i; for (i = 0; i < num_base_list; ++i) { struct commit *base; - if (!(base = get_ref(opt->repo, base_list[i], oid_to_hex(base_list[i])))) + if (!(base = get_ref(opt->repo, base_list[i], + oid_to_hex(base_list[i])))) return err(opt, _("Could not parse object '%s'"), oid_to_hex(base_list[i])); commit_list_insert(base, &ca); -- cgit v0.10.2-6-g49f6 From ff1bfa2cd5693900a84750dc47cda3cee3cf6ba7 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:34 -0700 Subject: merge-recursive: use common name for ancestors/common/base_list merge_trees(), merge_recursive(), and merge_recursive_generic() in their function headers used four different names for the merge base or list of merge bases they were passed: * 'common' * 'ancestors' * 'ca' * 'base_list' They were able to refer to it four different ways instead of only three by using a different name in the signature for the .c file than the .h file. Change all of these to 'merge_base' or 'merge_bases'. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 720678c..3287113 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3357,24 +3357,26 @@ static int process_entry(struct merge_options *opt, static int merge_trees_internal(struct merge_options *opt, struct tree *head, struct tree *merge, - struct tree *common, + struct tree *merge_base, struct tree **result) { struct index_state *istate = opt->repo->index; int code, clean; if (opt->subtree_shift) { - merge = shift_tree_object(opt->repo, head, merge, opt->subtree_shift); - common = shift_tree_object(opt->repo, head, common, opt->subtree_shift); + merge = shift_tree_object(opt->repo, head, merge, + opt->subtree_shift); + merge_base = shift_tree_object(opt->repo, head, merge_base, + opt->subtree_shift); } - if (oid_eq(&common->object.oid, &merge->object.oid)) { + if (oid_eq(&merge_base->object.oid, &merge->object.oid)) { output(opt, 0, _("Already up to date!")); *result = head; return 1; } - code = unpack_trees_start(opt, common, head, merge); + code = unpack_trees_start(opt, merge_base, head, merge); if (code != 0) { if (show(opt, 4) || opt->call_depth) @@ -3402,7 +3404,7 @@ static int merge_trees_internal(struct merge_options *opt, get_files_dirs(opt, merge); entries = get_unmerged(opt->repo->index); - clean = detect_and_process_renames(opt, common, head, merge, + clean = detect_and_process_renames(opt, merge_base, head, merge, entries, &re_info); record_df_conflict_files(opt, entries); if (clean < 0) @@ -3470,11 +3472,11 @@ static struct commit_list *reverse_commit_list(struct commit_list *list) static int merge_recursive_internal(struct merge_options *opt, struct commit *h1, struct commit *h2, - struct commit_list *ca, + struct commit_list *merge_bases, struct commit **result) { struct commit_list *iter; - struct commit *merged_common_ancestors; + struct commit *merged_merge_bases; struct tree *mrtree; int clean; const char *ancestor_name; @@ -3486,39 +3488,39 @@ static int merge_recursive_internal(struct merge_options *opt, output_commit_title(opt, h2); } - if (!ca) { - ca = get_merge_bases(h1, h2); - ca = reverse_commit_list(ca); + if (!merge_bases) { + merge_bases = get_merge_bases(h1, h2); + merge_bases = reverse_commit_list(merge_bases); } if (show(opt, 5)) { - unsigned cnt = commit_list_count(ca); + unsigned cnt = commit_list_count(merge_bases); output(opt, 5, Q_("found %u common ancestor:", "found %u common ancestors:", cnt), cnt); - for (iter = ca; iter; iter = iter->next) + for (iter = merge_bases; iter; iter = iter->next) output_commit_title(opt, iter->item); } - merged_common_ancestors = pop_commit(&ca); - if (merged_common_ancestors == NULL) { + merged_merge_bases = pop_commit(&merge_bases); + if (merged_merge_bases == NULL) { /* if there is no common ancestor, use an empty tree */ struct tree *tree; tree = lookup_tree(opt->repo, opt->repo->hash_algo->empty_tree); - merged_common_ancestors = make_virtual_commit(opt->repo, - tree, "ancestor"); + merged_merge_bases = make_virtual_commit(opt->repo, tree, + "ancestor"); ancestor_name = "empty tree"; - } else if (ca) { + } else if (merge_bases) { ancestor_name = "merged common ancestors"; } else { strbuf_add_unique_abbrev(&merge_base_abbrev, - &merged_common_ancestors->object.oid, + &merged_merge_bases->object.oid, DEFAULT_ABBREV); ancestor_name = merge_base_abbrev.buf; } - for (iter = ca; iter; iter = iter->next) { + for (iter = merge_bases; iter; iter = iter->next) { const char *saved_b1, *saved_b2; opt->call_depth++; /* @@ -3534,14 +3536,14 @@ static int merge_recursive_internal(struct merge_options *opt, saved_b2 = opt->branch2; opt->branch1 = "Temporary merge branch 1"; opt->branch2 = "Temporary merge branch 2"; - if (merge_recursive_internal(opt, merged_common_ancestors, iter->item, - NULL, &merged_common_ancestors) < 0) + if (merge_recursive_internal(opt, merged_merge_bases, iter->item, + NULL, &merged_merge_bases) < 0) return -1; opt->branch1 = saved_b1; opt->branch2 = saved_b2; opt->call_depth--; - if (!merged_common_ancestors) + if (!merged_merge_bases) return err(opt, _("merge returned no commit")); } @@ -3554,7 +3556,7 @@ static int merge_recursive_internal(struct merge_options *opt, repo_get_commit_tree(opt->repo, h1), repo_get_commit_tree(opt->repo, h2), repo_get_commit_tree(opt->repo, - merged_common_ancestors), + merged_merge_bases), &mrtree); strbuf_release(&merge_base_abbrev); if (clean < 0) { @@ -3597,7 +3599,7 @@ static void merge_finalize(struct merge_options *opt) int merge_trees(struct merge_options *opt, struct tree *head, struct tree *merge, - struct tree *common) + struct tree *merge_base) { int clean; struct tree *ignored; @@ -3606,7 +3608,7 @@ int merge_trees(struct merge_options *opt, if (merge_start(opt, head)) return -1; - clean = merge_trees_internal(opt, head, merge, common, &ignored); + clean = merge_trees_internal(opt, head, merge, merge_base, &ignored); merge_finalize(opt); return clean; @@ -3615,7 +3617,7 @@ int merge_trees(struct merge_options *opt, int merge_recursive(struct merge_options *opt, struct commit *h1, struct commit *h2, - struct commit_list *ca, + struct commit_list *merge_bases, struct commit **result) { int clean; @@ -3624,7 +3626,7 @@ int merge_recursive(struct merge_options *opt, if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) return -1; - clean = merge_recursive_internal(opt, h1, h2, ca, result); + clean = merge_recursive_internal(opt, h1, h2, merge_bases, result); merge_finalize(opt); return clean; @@ -3652,8 +3654,8 @@ static struct commit *get_ref(struct repository *repo, int merge_recursive_generic(struct merge_options *opt, const struct object_id *head, const struct object_id *merge, - int num_base_list, - const struct object_id **base_list, + int num_merge_bases, + const struct object_id **merge_bases, struct commit **result) { int clean; @@ -3662,14 +3664,14 @@ int merge_recursive_generic(struct merge_options *opt, struct commit *next_commit = get_ref(opt->repo, merge, opt->branch2); struct commit_list *ca = NULL; - if (base_list) { + if (merge_bases) { int i; - for (i = 0; i < num_base_list; ++i) { + for (i = 0; i < num_merge_bases; ++i) { struct commit *base; - if (!(base = get_ref(opt->repo, base_list[i], - oid_to_hex(base_list[i])))) + if (!(base = get_ref(opt->repo, merge_bases[i], + oid_to_hex(merge_bases[i])))) return err(opt, _("Could not parse object '%s'"), - oid_to_hex(base_list[i])); + oid_to_hex(merge_bases[i])); commit_list_insert(base, &ca); } } diff --git a/merge-recursive.h b/merge-recursive.h index 0a3033b..6f35109 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -81,14 +81,14 @@ static inline int merge_detect_rename(struct merge_options *o) * * NOTE: empirically, about a decade ago it was determined that with more * than two merge bases, optimal behavior was found when the - * ancestors were passed in the order of oldest merge base to newest - * one. Also, ancestors will be consumed (emptied) so make a copy if - * you need it. + * merge_bases were passed in the order of oldest commit to newest + * commit. Also, merge_bases will be consumed (emptied) so make a + * copy if you need it. */ int merge_recursive(struct merge_options *o, struct commit *h1, struct commit *h2, - struct commit_list *ancestors, + struct commit_list *merge_bases, struct commit **result); /* @@ -98,7 +98,7 @@ int merge_recursive(struct merge_options *o, int merge_trees(struct merge_options *o, struct tree *head, struct tree *merge, - struct tree *common); + struct tree *merge_base); /* * "git-merge-recursive" can be fed trees; wrap them into @@ -107,8 +107,8 @@ int merge_trees(struct merge_options *o, int merge_recursive_generic(struct merge_options *o, const struct object_id *head, const struct object_id *merge, - int num_ca, - const struct object_id **ca, + int num_merge_bases, + const struct object_id **merge_bases, struct commit **result); void init_merge_options(struct merge_options *o, -- cgit v0.10.2-6-g49f6 From bab56877e04efcf1bf9154398131dd57d73725d4 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:35 -0700 Subject: merge-recursive: rename 'mrtree' to 'result_tree', for clarity It is not at all clear what 'mr' was supposed to stand for, at least not to me. Pick a clearer name for this variable. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 3287113..1823a87 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3477,7 +3477,7 @@ static int merge_recursive_internal(struct merge_options *opt, { struct commit_list *iter; struct commit *merged_merge_bases; - struct tree *mrtree; + struct tree *result_tree; int clean; const char *ancestor_name; struct strbuf merge_base_abbrev = STRBUF_INIT; @@ -3557,7 +3557,7 @@ static int merge_recursive_internal(struct merge_options *opt, repo_get_commit_tree(opt->repo, h2), repo_get_commit_tree(opt->repo, merged_merge_bases), - &mrtree); + &result_tree); strbuf_release(&merge_base_abbrev); if (clean < 0) { flush_output(opt); @@ -3565,7 +3565,8 @@ static int merge_recursive_internal(struct merge_options *opt, } if (opt->call_depth) { - *result = make_virtual_commit(opt->repo, mrtree, "merged tree"); + *result = make_virtual_commit(opt->repo, result_tree, + "merged tree"); commit_list_insert(h1, &(*result)->parents); commit_list_insert(h2, &(*result)->parents->next); } -- cgit v0.10.2-6-g49f6 From c749ab1da812cdb1263f868c63fe1808a12dcff2 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:36 -0700 Subject: merge-recursive: rename merge_options argument to opt in header In commit 259ccb6cc324 ("merge-recursive: rename merge_options argument from 'o' to 'opt'", 2019-04-05), I renamed a bunch of function arguments in merge-recursive.c, but forgot to make that same change to merge-recursive.h. Make the two match. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.h b/merge-recursive.h index 6f35109..2cb3844 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -68,10 +68,10 @@ struct collision_entry { unsigned reported_already:1; }; -static inline int merge_detect_rename(struct merge_options *o) +static inline int merge_detect_rename(struct merge_options *opt) { - return o->merge_detect_rename >= 0 ? o->merge_detect_rename : - o->diff_detect_rename >= 0 ? o->diff_detect_rename : 1; + return opt->merge_detect_rename >= 0 ? opt->merge_detect_rename : + opt->diff_detect_rename >= 0 ? opt->diff_detect_rename : 1; } /* @@ -85,7 +85,7 @@ static inline int merge_detect_rename(struct merge_options *o) * commit. Also, merge_bases will be consumed (emptied) so make a * copy if you need it. */ -int merge_recursive(struct merge_options *o, +int merge_recursive(struct merge_options *opt, struct commit *h1, struct commit *h2, struct commit_list *merge_bases, @@ -95,7 +95,7 @@ int merge_recursive(struct merge_options *o, * rename-detecting three-way merge, no recursion; result of merge is written * to opt->repo->index. */ -int merge_trees(struct merge_options *o, +int merge_trees(struct merge_options *opt, struct tree *head, struct tree *merge, struct tree *merge_base); @@ -104,16 +104,16 @@ int merge_trees(struct merge_options *o, * "git-merge-recursive" can be fed trees; wrap them into * virtual commits and call merge_recursive() proper. */ -int merge_recursive_generic(struct merge_options *o, +int merge_recursive_generic(struct merge_options *opt, const struct object_id *head, const struct object_id *merge, int num_merge_bases, const struct object_id **merge_bases, struct commit **result); -void init_merge_options(struct merge_options *o, +void init_merge_options(struct merge_options *opt, struct repository *repo); -int parse_merge_opt(struct merge_options *out, const char *s); +int parse_merge_opt(struct merge_options *opt, const char *s); #endif -- cgit v0.10.2-6-g49f6 From 7c0a6c8e477bd9009001590f49274e892e92fce8 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:37 -0700 Subject: merge-recursive: move some definitions around to clean up the header No substantive code changes (view this with diff --color-moved), but a few small code cleanups: * Move structs and an inline function only used by merge-recursive.c into merge-recursive.c * Re-order function declarations to be more logical * Add or fix some explanatory comments Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 1823a87..9807b24 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -54,6 +54,24 @@ static unsigned int path_hash(const char *path) return ignore_case ? strihash(path) : strhash(path); } +/* + * For dir_rename_entry, directory names are stored as a full path from the + * toplevel of the repository and do not include a trailing '/'. Also: + * + * dir: original name of directory being renamed + * non_unique_new_dir: if true, could not determine new_dir + * new_dir: final name of directory being renamed + * possible_new_dirs: temporary used to help determine new_dir; see comments + * in get_directory_renames() for details + */ +struct dir_rename_entry { + struct hashmap_entry ent; /* must be the first member! */ + char *dir; + unsigned non_unique_new_dir:1; + struct strbuf new_dir; + struct string_list possible_new_dirs; +}; + static struct dir_rename_entry *dir_rename_find_entry(struct hashmap *hashmap, char *dir) { @@ -92,6 +110,13 @@ static void dir_rename_entry_init(struct dir_rename_entry *entry, string_list_init(&entry->possible_new_dirs, 0); } +struct collision_entry { + struct hashmap_entry ent; /* must be the first member! */ + char *target_file; + struct string_list source_files; + unsigned reported_already:1; +}; + static struct collision_entry *collision_find_entry(struct hashmap *hashmap, char *target_file) { @@ -358,6 +383,12 @@ static int add_cacheinfo(struct merge_options *opt, return ret; } +static inline int merge_detect_rename(struct merge_options *opt) +{ + return opt->merge_detect_rename >= 0 ? opt->merge_detect_rename : + opt->diff_detect_rename >= 0 ? opt->diff_detect_rename : 1; +} + static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree) { parse_tree(tree); diff --git a/merge-recursive.h b/merge-recursive.h index 2cb3844..0fdae90 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -43,47 +43,50 @@ struct merge_options { struct repository *repo; }; +void init_merge_options(struct merge_options *opt, struct repository *repo); + +/* parse the option in s and update the relevant field of opt */ +int parse_merge_opt(struct merge_options *opt, const char *s); + /* - * For dir_rename_entry, directory names are stored as a full path from the - * toplevel of the repository and do not include a trailing '/'. Also: - * - * dir: original name of directory being renamed - * non_unique_new_dir: if true, could not determine new_dir - * new_dir: final name of directory being renamed - * possible_new_dirs: temporary used to help determine new_dir; see comments - * in get_directory_renames() for details + * RETURN VALUES: All the merge_* functions below return a value as follows: + * > 0 Merge was clean + * = 0 Merge had conflicts + * < 0 Merge hit an unexpected and unrecoverable problem (e.g. disk + * full) and aborted merge part-way through. */ -struct dir_rename_entry { - struct hashmap_entry ent; /* must be the first member! */ - char *dir; - unsigned non_unique_new_dir:1; - struct strbuf new_dir; - struct string_list possible_new_dirs; -}; - -struct collision_entry { - struct hashmap_entry ent; /* must be the first member! */ - char *target_file; - struct string_list source_files; - unsigned reported_already:1; -}; -static inline int merge_detect_rename(struct merge_options *opt) -{ - return opt->merge_detect_rename >= 0 ? opt->merge_detect_rename : - opt->diff_detect_rename >= 0 ? opt->diff_detect_rename : 1; -} +/* + * rename-detecting three-way merge, no recursion. + * + * Outputs: + * - See RETURN VALUES above + * - No commit is created + * - opt->repo->index has the new index + * - $GIT_INDEX_FILE is not updated + * - The working tree is updated with results of the merge + */ +int merge_trees(struct merge_options *opt, + struct tree *head, + struct tree *merge, + struct tree *merge_base); /* * merge_recursive is like merge_trees() but with recursive ancestor - * consolidation, and when successful, it creates an actual commit - * and writes its address to *result. + * consolidation and, if the commit is clean, creation of a commit. * * NOTE: empirically, about a decade ago it was determined that with more * than two merge bases, optimal behavior was found when the * merge_bases were passed in the order of oldest commit to newest * commit. Also, merge_bases will be consumed (emptied) so make a * copy if you need it. + * + * Outputs: + * - See RETURN VALUES above + * - If merge is clean, a commit is created and its address written to *result + * - opt->repo->index has the new index + * - $GIT_INDEX_FILE is not updated + * - The working tree is updated with results of the merge */ int merge_recursive(struct merge_options *opt, struct commit *h1, @@ -92,17 +95,16 @@ int merge_recursive(struct merge_options *opt, struct commit **result); /* - * rename-detecting three-way merge, no recursion; result of merge is written - * to opt->repo->index. - */ -int merge_trees(struct merge_options *opt, - struct tree *head, - struct tree *merge, - struct tree *merge_base); - -/* - * "git-merge-recursive" can be fed trees; wrap them into - * virtual commits and call merge_recursive() proper. + * merge_recursive_generic can operate on trees instead of commits, by + * wrapping the trees into virtual commits, and calling merge_recursive(). + * It also writes out the in-memory index to disk if the merge is successful. + * + * Outputs: + * - See RETURN VALUES above + * - If merge is clean, a commit is created and its address written to *result + * - opt->repo->index has the new index + * - $GIT_INDEX_FILE is updated + * - The working tree is updated with results of the merge */ int merge_recursive_generic(struct merge_options *opt, const struct object_id *head, @@ -111,9 +113,4 @@ int merge_recursive_generic(struct merge_options *opt, const struct object_id **merge_bases, struct commit **result); -void init_merge_options(struct merge_options *opt, - struct repository *repo); - -int parse_merge_opt(struct merge_options *opt, const char *s); - #endif -- cgit v0.10.2-6-g49f6 From 8599ab4574ce3a8b2fc894c7cfdac8fd61450b7b Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:38 -0700 Subject: merge-recursive: consolidate unnecessary fields in merge_options We provided users with the ability to state whether they wanted rename detection, and to put a limit on how much CPU would be spent. Both of these fields had multiple configuration parameters for setting them, with one being a fallback and the other being an override. However, instead of implementing the logic for how to combine the multiple source locations into the appropriate setting at config loading time, we loaded and tracked both values and then made the code combine them every time it wanted to check the overall value. This had a few minor drawbacks: * it seems more complicated than necessary * it runs the risk of people using the independent settings in the future and breaking the intent of how the options are used together * it makes merge_options more complicated than necessary for other potential users of the API Fix these problems by moving the logic for combining the pairs of options into a single value; make it apply at time-of-config-loading instead of each-time-of-use. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 9807b24..0f0b952 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -385,8 +385,7 @@ static int add_cacheinfo(struct merge_options *opt, static inline int merge_detect_rename(struct merge_options *opt) { - return opt->merge_detect_rename >= 0 ? opt->merge_detect_rename : - opt->diff_detect_rename >= 0 ? opt->diff_detect_rename : 1; + return (opt->detect_renames >= 0) ? opt->detect_renames : 1; } static void init_tree_desc_from_tree(struct tree_desc *desc, struct tree *tree) @@ -1883,9 +1882,7 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt, */ if (opts.detect_rename > DIFF_DETECT_RENAME) opts.detect_rename = DIFF_DETECT_RENAME; - opts.rename_limit = opt->merge_rename_limit >= 0 ? opt->merge_rename_limit : - opt->diff_rename_limit >= 0 ? opt->diff_rename_limit : - 1000; + opts.rename_limit = (opt->rename_limit >= 0) ? opt->rename_limit : 1000; opts.rename_score = opt->rename_score; opts.show_rename_progress = opt->show_rename_progress; opts.output_format = DIFF_FORMAT_NO_OUTPUT; @@ -3727,14 +3724,14 @@ static void merge_recursive_config(struct merge_options *opt) { char *value = NULL; git_config_get_int("merge.verbosity", &opt->verbosity); - git_config_get_int("diff.renamelimit", &opt->diff_rename_limit); - git_config_get_int("merge.renamelimit", &opt->merge_rename_limit); + git_config_get_int("diff.renamelimit", &opt->rename_limit); + git_config_get_int("merge.renamelimit", &opt->rename_limit); if (!git_config_get_string("diff.renames", &value)) { - opt->diff_detect_rename = git_config_rename("diff.renames", value); + opt->detect_renames = git_config_rename("diff.renames", value); free(value); } if (!git_config_get_string("merge.renames", &value)) { - opt->merge_detect_rename = git_config_rename("merge.renames", value); + opt->detect_renames = git_config_rename("merge.renames", value); free(value); } if (!git_config_get_string("merge.directoryrenames", &value)) { @@ -3760,11 +3757,9 @@ void init_merge_options(struct merge_options *opt, opt->repo = repo; opt->verbosity = 2; opt->buffer_output = 1; - opt->diff_rename_limit = -1; - opt->merge_rename_limit = -1; + opt->rename_limit = -1; opt->renormalize = 0; - opt->diff_detect_rename = -1; - opt->merge_detect_rename = -1; + opt->detect_renames = -1; opt->detect_directory_renames = MERGE_DIRECTORY_RENAMES_CONFLICT; merge_recursive_config(opt); merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); @@ -3816,16 +3811,16 @@ int parse_merge_opt(struct merge_options *opt, const char *s) else if (!strcmp(s, "no-renormalize")) opt->renormalize = 0; else if (!strcmp(s, "no-renames")) - opt->merge_detect_rename = 0; + opt->detect_renames = 0; else if (!strcmp(s, "find-renames")) { - opt->merge_detect_rename = 1; + opt->detect_renames = 1; opt->rename_score = 0; } else if (skip_prefix(s, "find-renames=", &arg) || skip_prefix(s, "rename-threshold=", &arg)) { if ((opt->rename_score = parse_rename_score(&arg)) == -1 || *arg != 0) return -1; - opt->merge_detect_rename = 1; + opt->detect_renames = 1; } /* * Please update $__git_merge_strategy_options in diff --git a/merge-recursive.h b/merge-recursive.h index 0fdae90..f4bdfbc 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -27,10 +27,8 @@ struct merge_options { MERGE_DIRECTORY_RENAMES_CONFLICT = 1, MERGE_DIRECTORY_RENAMES_TRUE = 2 } detect_directory_renames; - int diff_detect_rename; - int merge_detect_rename; - int diff_rename_limit; - int merge_rename_limit; + int detect_renames; + int rename_limit; int rename_score; int needed_rename_limit; int show_rename_progress; -- cgit v0.10.2-6-g49f6 From a779fb829bf6160cb519500ac1e15d8ab8a247a4 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:39 -0700 Subject: merge-recursive: comment and reorder the merge_options fields The merge_options struct had lots of fields, making it a little imposing, but the options naturally fall into multiple different groups. Grouping similar options and adding a comment or two makes it easier to read, easier for new folks to figure out which options are related, and thus easier for them to find the options they need. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 0f0b952..43dec33 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3754,21 +3754,27 @@ void init_merge_options(struct merge_options *opt, { const char *merge_verbosity; memset(opt, 0, sizeof(struct merge_options)); + opt->repo = repo; + + opt->detect_renames = -1; + opt->detect_directory_renames = MERGE_DIRECTORY_RENAMES_CONFLICT; + opt->rename_limit = -1; + opt->verbosity = 2; opt->buffer_output = 1; - opt->rename_limit = -1; + strbuf_init(&opt->obuf, 0); + opt->renormalize = 0; - opt->detect_renames = -1; - opt->detect_directory_renames = MERGE_DIRECTORY_RENAMES_CONFLICT; + + string_list_init(&opt->df_conflict_file_set, 1); + merge_recursive_config(opt); merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); if (merge_verbosity) opt->verbosity = strtol(merge_verbosity, NULL, 10); if (opt->verbosity >= 5) opt->buffer_output = 0; - strbuf_init(&opt->obuf, 0); - string_list_init(&opt->df_conflict_file_set, 1); } int parse_merge_opt(struct merge_options *opt, const char *s) diff --git a/merge-recursive.h b/merge-recursive.h index f4bdfbc..9e04060 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -9,36 +9,48 @@ struct commit; struct repository; struct merge_options { + struct repository *repo; + + /* ref names used in console messages and conflict markers */ const char *ancestor; const char *branch1; const char *branch2; - enum { - MERGE_RECURSIVE_NORMAL = 0, - MERGE_RECURSIVE_OURS, - MERGE_RECURSIVE_THEIRS - } recursive_variant; - const char *subtree_shift; - unsigned buffer_output; /* 1: output at end, 2: keep buffered */ - unsigned renormalize : 1; - long xdl_opts; - int verbosity; + + /* rename related options */ + int detect_renames; enum { MERGE_DIRECTORY_RENAMES_NONE = 0, MERGE_DIRECTORY_RENAMES_CONFLICT = 1, MERGE_DIRECTORY_RENAMES_TRUE = 2 } detect_directory_renames; - int detect_renames; int rename_limit; int rename_score; - int needed_rename_limit; int show_rename_progress; + + /* xdiff-related options (patience, ignore whitespace, ours/theirs) */ + long xdl_opts; + enum { + MERGE_RECURSIVE_NORMAL = 0, + MERGE_RECURSIVE_OURS, + MERGE_RECURSIVE_THEIRS + } recursive_variant; + + /* console output related options */ + int verbosity; + unsigned buffer_output; /* 1: output at end, 2: keep buffered */ + struct strbuf obuf; /* output buffer */ + + /* miscellaneous control options */ + const char *subtree_shift; + unsigned renormalize : 1; + + /* internal fields used by the implementation (do NOT set these) */ int call_depth; - struct strbuf obuf; + int needed_rename_limit; struct hashmap current_file_dir_set; struct string_list df_conflict_file_set; struct unpack_trees_options unpack_opts; struct index_state orig_index; - struct repository *repo; }; void init_merge_options(struct merge_options *opt, struct repository *repo); -- cgit v0.10.2-6-g49f6 From e95e481f9e6e4dfe62380ccc8a47acf446403a94 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:40 -0700 Subject: merge-recursive: avoid losing output and leaking memory holding that output If opt->buffer_output is less than 2, then merge_trees(), merge_recursive(), and merge_recursive_generic() are all supposed to flush the opt->obuf output buffer to stdout and release any memory it holds. merge_trees() did not do this. Move the logic that handles this for merge_recursive_internal() to merge_finalize() so that all three methods handle this requirement. Note that this bug didn't cause any problems currently, because there are only two callers of merge_trees() right now (a git grep for 'merge_trees(' is misleading because builtin/merge-tree.c also defines a 'merge_tree' function that is unrelated), and only one of those is called with buffer_output less than 2 (builtin/checkout.c), but it set opt->verbosity to 0, for which there is only currently one non-error message that would be shown: "Already up to date!". However, that one message can only occur when the merge is utterly trivial (the merge base tree exactly matches the merge tree), and builtin/checkout.c already attempts a trivial merge via unpack_trees() before falling back to merge_trees(). Also, if opt->buffer_output is 2, then the caller is responsible to handle showing any output in opt->obuf and for free'ing it. This requirement might be easy to overlook, so add a comment to merge-recursive.h pointing it out. (There are currently two callers that set buffer_output to 2, both in sequencer.c, and both of which handle this correctly.) Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 43dec33..262db8b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3598,9 +3598,6 @@ static int merge_recursive_internal(struct merge_options *opt, commit_list_insert(h1, &(*result)->parents); commit_list_insert(h2, &(*result)->parents->next); } - flush_output(opt); - if (!opt->call_depth && opt->buffer_output < 2) - strbuf_release(&opt->obuf); return clean; } @@ -3620,6 +3617,9 @@ static int merge_start(struct merge_options *opt, struct tree *head) static void merge_finalize(struct merge_options *opt) { + flush_output(opt); + if (!opt->call_depth && opt->buffer_output < 2) + strbuf_release(&opt->obuf); if (show(opt, 2)) diff_warn_rename_limit("merge.renamelimit", opt->needed_rename_limit, 0); diff --git a/merge-recursive.h b/merge-recursive.h index 9e04060..933d6e7 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -38,7 +38,8 @@ struct merge_options { /* console output related options */ int verbosity; unsigned buffer_output; /* 1: output at end, 2: keep buffered */ - struct strbuf obuf; /* output buffer */ + struct strbuf obuf; /* output buffer; if buffer_output == 2, caller + * must handle and call strbuf_release */ /* miscellaneous control options */ const char *subtree_shift; -- cgit v0.10.2-6-g49f6 From 5bf7e5779ec6d5293b3135554d49e6fcee88d399 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:41 -0700 Subject: merge-recursive: split internal fields into a separate struct merge_options has several internal fields that should not be set or read by external callers. This just complicates the API. Move them into an opaque merge_options_internal struct that is defined only in merge-recursive.c and keep these out of merge-recursive.h. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 262db8b..c92993e 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -29,6 +29,15 @@ #include "revision.h" #include "commit-reach.h" +struct merge_options_internal { + int call_depth; + int needed_rename_limit; + struct hashmap current_file_dir_set; + struct string_list df_conflict_file_set; + struct unpack_trees_options unpack_opts; + struct index_state orig_index; +}; + struct path_hashmap_entry { struct hashmap_entry e; char path[FLEX_ARRAY]; @@ -309,7 +318,8 @@ static inline void setup_rename_conflict_info(enum rename_type rename_type, static int show(struct merge_options *opt, int v) { - return (!opt->call_depth && opt->verbosity >= v) || opt->verbosity >= 5; + return (!opt->priv->call_depth && opt->verbosity >= v) || + opt->verbosity >= 5; } __attribute__((format (printf, 3, 4))) @@ -320,7 +330,7 @@ static void output(struct merge_options *opt, int v, const char *fmt, ...) if (!show(opt, v)) return; - strbuf_addchars(&opt->obuf, ' ', opt->call_depth * 2); + strbuf_addchars(&opt->obuf, ' ', opt->priv->call_depth * 2); va_start(ap, fmt); strbuf_vaddf(&opt->obuf, fmt, ap); @@ -335,7 +345,7 @@ static void output_commit_title(struct merge_options *opt, struct commit *commit { struct merge_remote_desc *desc; - strbuf_addchars(&opt->obuf, ' ', opt->call_depth * 2); + strbuf_addchars(&opt->obuf, ' ', opt->priv->call_depth * 2); desc = merge_remote_util(commit); if (desc) strbuf_addf(&opt->obuf, "virtual %s\n", desc->name); @@ -403,43 +413,43 @@ static int unpack_trees_start(struct merge_options *opt, struct tree_desc t[3]; struct index_state tmp_index = { NULL }; - memset(&opt->unpack_opts, 0, sizeof(opt->unpack_opts)); - if (opt->call_depth) - opt->unpack_opts.index_only = 1; + memset(&opt->priv->unpack_opts, 0, sizeof(opt->priv->unpack_opts)); + if (opt->priv->call_depth) + opt->priv->unpack_opts.index_only = 1; else - opt->unpack_opts.update = 1; - opt->unpack_opts.merge = 1; - opt->unpack_opts.head_idx = 2; - opt->unpack_opts.fn = threeway_merge; - opt->unpack_opts.src_index = opt->repo->index; - opt->unpack_opts.dst_index = &tmp_index; - opt->unpack_opts.aggressive = !merge_detect_rename(opt); - setup_unpack_trees_porcelain(&opt->unpack_opts, "merge"); + opt->priv->unpack_opts.update = 1; + opt->priv->unpack_opts.merge = 1; + opt->priv->unpack_opts.head_idx = 2; + opt->priv->unpack_opts.fn = threeway_merge; + opt->priv->unpack_opts.src_index = opt->repo->index; + opt->priv->unpack_opts.dst_index = &tmp_index; + opt->priv->unpack_opts.aggressive = !merge_detect_rename(opt); + setup_unpack_trees_porcelain(&opt->priv->unpack_opts, "merge"); init_tree_desc_from_tree(t+0, common); init_tree_desc_from_tree(t+1, head); init_tree_desc_from_tree(t+2, merge); - rc = unpack_trees(3, t, &opt->unpack_opts); + rc = unpack_trees(3, t, &opt->priv->unpack_opts); cache_tree_free(&opt->repo->index->cache_tree); /* - * Update opt->repo->index to match the new results, AFTER saving a copy - * in opt->orig_index. Update src_index to point to the saved copy. - * (verify_uptodate() checks src_index, and the original index is - * the one that had the necessary modification timestamps.) + * Update opt->repo->index to match the new results, AFTER saving a + * copy in opt->priv->orig_index. Update src_index to point to the + * saved copy. (verify_uptodate() checks src_index, and the original + * index is the one that had the necessary modification timestamps.) */ - opt->orig_index = *opt->repo->index; + opt->priv->orig_index = *opt->repo->index; *opt->repo->index = tmp_index; - opt->unpack_opts.src_index = &opt->orig_index; + opt->priv->unpack_opts.src_index = &opt->priv->orig_index; return rc; } static void unpack_trees_finish(struct merge_options *opt) { - discard_index(&opt->orig_index); - clear_unpack_trees_porcelain(&opt->unpack_opts); + discard_index(&opt->priv->orig_index); + clear_unpack_trees_porcelain(&opt->priv->unpack_opts); } static int save_files_dirs(const struct object_id *oid, @@ -454,7 +464,7 @@ static int save_files_dirs(const struct object_id *oid, FLEX_ALLOC_MEM(entry, path, base->buf, base->len); hashmap_entry_init(entry, path_hash(entry->path)); - hashmap_add(&opt->current_file_dir_set, entry); + hashmap_add(&opt->priv->current_file_dir_set, entry); strbuf_setlen(base, baselen); return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0); @@ -585,7 +595,7 @@ static void record_df_conflict_files(struct merge_options *opt, * If we're merging merge-bases, we don't want to bother with * any working directory changes. */ - if (opt->call_depth) + if (opt->priv->call_depth) return; /* Ensure D/F conflicts are adjacent in the entries list. */ @@ -597,7 +607,7 @@ static void record_df_conflict_files(struct merge_options *opt, df_sorted_entries.cmp = string_list_df_name_compare; string_list_sort(&df_sorted_entries); - string_list_clear(&opt->df_conflict_file_set, 1); + string_list_clear(&opt->priv->df_conflict_file_set, 1); for (i = 0; i < df_sorted_entries.nr; i++) { const char *path = df_sorted_entries.items[i].string; int len = strlen(path); @@ -613,7 +623,7 @@ static void record_df_conflict_files(struct merge_options *opt, len > last_len && memcmp(path, last_file, last_len) == 0 && path[last_len] == '/') { - string_list_insert(&opt->df_conflict_file_set, last_file); + string_list_insert(&opt->priv->df_conflict_file_set, last_file); } /* @@ -680,8 +690,8 @@ static void update_entry(struct stage_data *entry, static int remove_file(struct merge_options *opt, int clean, const char *path, int no_wd) { - int update_cache = opt->call_depth || clean; - int update_working_directory = !opt->call_depth && !no_wd; + int update_cache = opt->priv->call_depth || clean; + int update_working_directory = !opt->priv->call_depth && !no_wd; if (update_cache) { if (remove_file_from_index(opt->repo->index, path)) @@ -724,16 +734,16 @@ static char *unique_path(struct merge_options *opt, add_flattened_path(&newpath, branch); base_len = newpath.len; - while (hashmap_get_from_hash(&opt->current_file_dir_set, + while (hashmap_get_from_hash(&opt->priv->current_file_dir_set, path_hash(newpath.buf), newpath.buf) || - (!opt->call_depth && file_exists(newpath.buf))) { + (!opt->priv->call_depth && file_exists(newpath.buf))) { strbuf_setlen(&newpath, base_len); strbuf_addf(&newpath, "_%d", suffix++); } FLEX_ALLOC_MEM(entry, path, newpath.buf, newpath.len); hashmap_entry_init(entry, path_hash(entry->path)); - hashmap_add(&opt->current_file_dir_set, entry); + hashmap_add(&opt->priv->current_file_dir_set, entry); return strbuf_detach(&newpath, NULL); } @@ -775,7 +785,7 @@ static int dir_in_way(struct index_state *istate, const char *path, static int was_tracked_and_matches(struct merge_options *opt, const char *path, const struct diff_filespec *blob) { - int pos = index_name_pos(&opt->orig_index, path, strlen(path)); + int pos = index_name_pos(&opt->priv->orig_index, path, strlen(path)); struct cache_entry *ce; if (0 > pos) @@ -783,7 +793,7 @@ static int was_tracked_and_matches(struct merge_options *opt, const char *path, return 0; /* See if the file we were tracking before matches */ - ce = opt->orig_index.cache[pos]; + ce = opt->priv->orig_index.cache[pos]; return (oid_eq(&ce->oid, &blob->oid) && ce->ce_mode == blob->mode); } @@ -792,7 +802,7 @@ static int was_tracked_and_matches(struct merge_options *opt, const char *path, */ static int was_tracked(struct merge_options *opt, const char *path) { - int pos = index_name_pos(&opt->orig_index, path, strlen(path)); + int pos = index_name_pos(&opt->priv->orig_index, path, strlen(path)); if (0 <= pos) /* we were tracking this path before the merge */ @@ -849,12 +859,12 @@ static int was_dirty(struct merge_options *opt, const char *path) struct cache_entry *ce; int dirty = 1; - if (opt->call_depth || !was_tracked(opt, path)) + if (opt->priv->call_depth || !was_tracked(opt, path)) return !dirty; - ce = index_file_exists(opt->unpack_opts.src_index, + ce = index_file_exists(opt->priv->unpack_opts.src_index, path, strlen(path), ignore_case); - dirty = verify_uptodate(ce, &opt->unpack_opts) != 0; + dirty = verify_uptodate(ce, &opt->priv->unpack_opts) != 0; return dirty; } @@ -864,8 +874,8 @@ static int make_room_for_path(struct merge_options *opt, const char *path) const char *msg = _("failed to create path '%s'%s"); /* Unlink any D/F conflict files that are in the way */ - for (i = 0; i < opt->df_conflict_file_set.nr; i++) { - const char *df_path = opt->df_conflict_file_set.items[i].string; + for (i = 0; i < opt->priv->df_conflict_file_set.nr; i++) { + const char *df_path = opt->priv->df_conflict_file_set.items[i].string; size_t pathlen = strlen(path); size_t df_pathlen = strlen(df_path); if (df_pathlen < pathlen && @@ -875,7 +885,7 @@ static int make_room_for_path(struct merge_options *opt, const char *path) _("Removing %s to make room for subdirectory\n"), df_path); unlink(df_path); - unsorted_string_list_delete_item(&opt->df_conflict_file_set, + unsorted_string_list_delete_item(&opt->priv->df_conflict_file_set, i, 0); break; } @@ -916,7 +926,7 @@ static int update_file_flags(struct merge_options *opt, { int ret = 0; - if (opt->call_depth) + if (opt->priv->call_depth) update_wd = 0; if (update_wd) { @@ -1001,7 +1011,7 @@ static int update_file(struct merge_options *opt, const char *path) { return update_file_flags(opt, contents, path, - opt->call_depth || clean, !opt->call_depth); + opt->priv->call_depth || clean, !opt->priv->call_depth); } /* Low level file merging, update and removal */ @@ -1030,7 +1040,7 @@ static int merge_3way(struct merge_options *opt, ll_opts.extra_marker_size = extra_marker_size; ll_opts.xdl_opts = opt->xdl_opts; - if (opt->call_depth) { + if (opt->priv->call_depth) { ll_opts.virtual_ancestor = 1; ll_opts.variant = 0; } else { @@ -1161,7 +1171,7 @@ static int merge_submodule(struct merge_options *opt, struct object_array merges; int i; - int search = !opt->call_depth; + int search = !opt->priv->call_depth; /* store a in result in case we fail */ oidcpy(result, a); @@ -1383,7 +1393,7 @@ static int handle_rename_via_dir(struct merge_options *opt, MERGE_DIRECTORY_RENAMES_CONFLICT); assert(ren->dir_rename_original_dest); - if (!opt->call_depth && would_lose_untracked(opt, dest->path)) { + if (!opt->priv->call_depth && would_lose_untracked(opt, dest->path)) { mark_conflicted = 1; file_path = unique_path(opt, dest->path, ren->branch); output(opt, 1, _("Error: Refusing to lose untracked file at %s; " @@ -1426,12 +1436,12 @@ static int handle_change_delete(struct merge_options *opt, const char *update_path = path; int ret = 0; - if (dir_in_way(opt->repo->index, path, !opt->call_depth, 0) || - (!opt->call_depth && would_lose_untracked(opt, path))) { + if (dir_in_way(opt->repo->index, path, !opt->priv->call_depth, 0) || + (!opt->priv->call_depth && would_lose_untracked(opt, path))) { update_path = alt_path = unique_path(opt, path, change_branch); } - if (opt->call_depth) { + if (opt->priv->call_depth) { /* * We cannot arbitrarily accept either a_sha or b_sha as * correct; since there is no true "middle point" between @@ -1506,14 +1516,14 @@ static int handle_rename_delete(struct merge_options *opt, opt->branch2 : opt->branch1); if (handle_change_delete(opt, - opt->call_depth ? orig->path : dest->path, - opt->call_depth ? NULL : orig->path, + opt->priv->call_depth ? orig->path : dest->path, + opt->priv->call_depth ? NULL : orig->path, orig, dest, rename_branch, delete_branch, _("rename"), _("renamed"))) return -1; - if (opt->call_depth) + if (opt->priv->call_depth) return remove_file_from_index(opt->repo->index, dest->path); else return update_stages(opt, dest->path, NULL, @@ -1550,7 +1560,7 @@ static int handle_file_collision(struct merge_options *opt, /* * In the recursive case, we just opt to undo renames */ - if (opt->call_depth && (prev_path1 || prev_path2)) { + if (opt->priv->call_depth && (prev_path1 || prev_path2)) { /* Put first file (a->oid, a->mode) in its original spot */ if (prev_path1) { if (update_file(opt, 1, a, prev_path1)) @@ -1579,10 +1589,10 @@ static int handle_file_collision(struct merge_options *opt, /* Remove rename sources if rename/add or rename/rename(2to1) */ if (prev_path1) remove_file(opt, 1, prev_path1, - opt->call_depth || would_lose_untracked(opt, prev_path1)); + opt->priv->call_depth || would_lose_untracked(opt, prev_path1)); if (prev_path2) remove_file(opt, 1, prev_path2, - opt->call_depth || would_lose_untracked(opt, prev_path2)); + opt->priv->call_depth || would_lose_untracked(opt, prev_path2)); /* * Remove the collision path, if it wouldn't cause dirty contents @@ -1624,12 +1634,12 @@ static int handle_file_collision(struct merge_options *opt, null.mode = 0; if (merge_mode_and_contents(opt, &null, a, b, collide_path, - branch1, branch2, opt->call_depth * 2, &mfi)) + branch1, branch2, opt->priv->call_depth * 2, &mfi)) return -1; mfi.clean &= !alt_path; if (update_file(opt, mfi.clean, &mfi.blob, update_path)) return -1; - if (!mfi.clean && !opt->call_depth && + if (!mfi.clean && !opt->priv->call_depth && update_stages(opt, collide_path, NULL, a, b)) return -1; free(alt_path); @@ -1669,7 +1679,7 @@ static int handle_rename_add(struct merge_options *opt, &ci->ren1->src_entry->stages[other_stage], prev_path_desc, opt->branch1, opt->branch2, - 1 + opt->call_depth * 2, &mfi)) + 1 + opt->priv->call_depth * 2, &mfi)) return -1; free(prev_path_desc); @@ -1687,7 +1697,7 @@ static char *find_path_for_conflict(struct merge_options *opt, const char *branch2) { char *new_path = NULL; - if (dir_in_way(opt->repo->index, path, !opt->call_depth, 0)) { + if (dir_in_way(opt->repo->index, path, !opt->priv->call_depth, 0)) { new_path = unique_path(opt, path, branch1); output(opt, 1, _("%s is a directory in %s adding " "as %s instead"), @@ -1718,17 +1728,17 @@ static int handle_rename_rename_1to2(struct merge_options *opt, "rename \"%s\"->\"%s\" in \"%s\"%s"), o->path, a->path, ci->ren1->branch, o->path, b->path, ci->ren2->branch, - opt->call_depth ? _(" (left unresolved)") : ""); + opt->priv->call_depth ? _(" (left unresolved)") : ""); path_desc = xstrfmt("%s and %s, both renamed from %s", a->path, b->path, o->path); if (merge_mode_and_contents(opt, o, a, b, path_desc, ci->ren1->branch, ci->ren2->branch, - opt->call_depth * 2, &mfi)) + opt->priv->call_depth * 2, &mfi)) return -1; free(path_desc); - if (opt->call_depth) { + if (opt->priv->call_depth) { /* * FIXME: For rename/add-source conflicts (if we could detect * such), this is wrong. We should instead find a unique @@ -1843,12 +1853,12 @@ static int handle_rename_rename_2to1(struct merge_options *opt, &ci->ren1->src_entry->stages[ostage1], path_side_1_desc, opt->branch1, opt->branch2, - 1 + opt->call_depth * 2, &mfi_c1) || + 1 + opt->priv->call_depth * 2, &mfi_c1) || merge_mode_and_contents(opt, b, &ci->ren2->src_entry->stages[ostage2], c2, path_side_2_desc, opt->branch1, opt->branch2, - 1 + opt->call_depth * 2, &mfi_c2)) + 1 + opt->priv->call_depth * 2, &mfi_c2)) return -1; free(path_side_1_desc); free(path_side_2_desc); @@ -1889,8 +1899,8 @@ static struct diff_queue_struct *get_diffpairs(struct merge_options *opt, diff_setup_done(&opts); diff_tree_oid(&o_tree->object.oid, &tree->object.oid, "", &opts); diffcore_std(&opts); - if (opts.needed_rename_limit > opt->needed_rename_limit) - opt->needed_rename_limit = opts.needed_rename_limit; + if (opts.needed_rename_limit > opt->priv->needed_rename_limit) + opt->priv->needed_rename_limit = opts.needed_rename_limit; ret = xmalloc(sizeof(*ret)); *ret = diff_queued_diff; @@ -2865,7 +2875,7 @@ static int detect_and_process_renames(struct merge_options *opt, if ((opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_TRUE) || (opt->detect_directory_renames == MERGE_DIRECTORY_RENAMES_CONFLICT && - !opt->call_depth)) { + !opt->priv->call_depth)) { dir_re_head = get_directory_renames(head_pairs); dir_re_merge = get_directory_renames(merge_pairs); @@ -3022,13 +3032,13 @@ static int handle_content_merge(struct merge_file_info *mfi, reason = _("add/add"); assert(o->path && a->path && b->path); - if (ci && dir_in_way(opt->repo->index, path, !opt->call_depth, + if (ci && dir_in_way(opt->repo->index, path, !opt->priv->call_depth, S_ISGITLINK(ci->ren1->pair->two->mode))) df_conflict_remains = 1; if (merge_mode_and_contents(opt, o, a, b, path, opt->branch1, opt->branch2, - opt->call_depth * 2, mfi)) + opt->priv->call_depth * 2, mfi)) return -1; /* @@ -3044,7 +3054,7 @@ static int handle_content_merge(struct merge_file_info *mfi, output(opt, 3, _("Skipped %s (merged same as existing)"), path); if (add_cacheinfo(opt, &mfi->blob, path, - 0, (!opt->call_depth && !is_dirty), 0)) + 0, (!opt->priv->call_depth && !is_dirty), 0)) return -1; /* * However, add_cacheinfo() will delete the old cache entry @@ -3052,8 +3062,8 @@ static int handle_content_merge(struct merge_file_info *mfi, * flag to avoid making the file appear as if it were * deleted by the user. */ - pos = index_name_pos(&opt->orig_index, path, strlen(path)); - ce = opt->orig_index.cache[pos]; + pos = index_name_pos(&opt->priv->orig_index, path, strlen(path)); + ce = opt->priv->orig_index.cache[pos]; if (ce_skip_worktree(ce)) { pos = index_name_pos(opt->repo->index, path, strlen(path)); ce = opt->repo->index->cache[pos]; @@ -3074,7 +3084,7 @@ static int handle_content_merge(struct merge_file_info *mfi, if (df_conflict_remains || is_dirty) { char *new_path; - if (opt->call_depth) { + if (opt->priv->call_depth) { remove_file_from_index(opt->repo->index, path); } else { if (!mfi->clean) { @@ -3333,7 +3343,7 @@ static int process_entry(struct merge_options *opt, conf = _("directory/file"); } if (dir_in_way(opt->repo->index, path, - !opt->call_depth && !S_ISGITLINK(a->mode), + !opt->priv->call_depth && !S_ISGITLINK(a->mode), 0)) { char *new_path = unique_path(opt, path, add_branch); clean_merge = 0; @@ -3342,7 +3352,7 @@ static int process_entry(struct merge_options *opt, conf, path, other_branch, path, new_path); if (update_file(opt, 0, contents, new_path)) clean_merge = -1; - else if (opt->call_depth) + else if (opt->priv->call_depth) remove_file_from_index(opt->repo->index, path); free(new_path); } else { @@ -3407,7 +3417,7 @@ static int merge_trees_internal(struct merge_options *opt, code = unpack_trees_start(opt, merge_base, head, merge); if (code != 0) { - if (show(opt, 4) || opt->call_depth) + if (show(opt, 4) || opt->priv->call_depth) err(opt, _("merging of trees %s and %s failed"), oid_to_hex(&head->object.oid), oid_to_hex(&merge->object.oid)); @@ -3426,7 +3436,7 @@ static int merge_trees_internal(struct merge_options *opt, * opposed to decaring a local hashmap is for convenience * so that we don't have to pass it to around. */ - hashmap_init(&opt->current_file_dir_set, path_hashmap_cmp, + hashmap_init(&opt->priv->current_file_dir_set, path_hashmap_cmp, NULL, 512); get_files_dirs(opt, head); get_files_dirs(opt, merge); @@ -3463,7 +3473,7 @@ static int merge_trees_internal(struct merge_options *opt, string_list_clear(entries, 1); free(entries); - hashmap_free(&opt->current_file_dir_set, 1); + hashmap_free(&opt->priv->current_file_dir_set, 1); if (clean < 0) { unpack_trees_finish(opt); @@ -3475,7 +3485,7 @@ static int merge_trees_internal(struct merge_options *opt, unpack_trees_finish(opt); - if (opt->call_depth && + if (opt->priv->call_depth && !(*result = write_in_core_index_as_tree(opt->repo))) return -1; @@ -3550,7 +3560,7 @@ static int merge_recursive_internal(struct merge_options *opt, for (iter = merge_bases; iter; iter = iter->next) { const char *saved_b1, *saved_b2; - opt->call_depth++; + opt->priv->call_depth++; /* * When the merge fails, the result contains files * with conflict markers. The cleanness flag is @@ -3569,14 +3579,14 @@ static int merge_recursive_internal(struct merge_options *opt, return -1; opt->branch1 = saved_b1; opt->branch2 = saved_b2; - opt->call_depth--; + opt->priv->call_depth--; if (!merged_merge_bases) return err(opt, _("merge returned no commit")); } discard_index(opt->repo->index); - if (!opt->call_depth) + if (!opt->priv->call_depth) repo_read_index(opt->repo); opt->ancestor = ancestor_name; @@ -3592,7 +3602,7 @@ static int merge_recursive_internal(struct merge_options *opt, return clean; } - if (opt->call_depth) { + if (opt->priv->call_depth) { *result = make_virtual_commit(opt->repo, result_tree, "merged tree"); commit_list_insert(h1, &(*result)->parents); @@ -3612,17 +3622,20 @@ static int merge_start(struct merge_options *opt, struct tree *head) return -1; } + opt->priv = xcalloc(1, sizeof(*opt->priv)); + string_list_init(&opt->priv->df_conflict_file_set, 1); return 0; } static void merge_finalize(struct merge_options *opt) { flush_output(opt); - if (!opt->call_depth && opt->buffer_output < 2) + if (!opt->priv->call_depth && opt->buffer_output < 2) strbuf_release(&opt->obuf); if (show(opt, 2)) diff_warn_rename_limit("merge.renamelimit", - opt->needed_rename_limit, 0); + opt->priv->needed_rename_limit, 0); + FREE_AND_NULL(opt->priv); } int merge_trees(struct merge_options *opt, @@ -3767,8 +3780,6 @@ void init_merge_options(struct merge_options *opt, opt->renormalize = 0; - string_list_init(&opt->df_conflict_file_set, 1); - merge_recursive_config(opt); merge_verbosity = getenv("GIT_MERGE_VERBOSITY"); if (merge_verbosity) diff --git a/merge-recursive.h b/merge-recursive.h index 933d6e7..58a4c52 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -1,13 +1,15 @@ #ifndef MERGE_RECURSIVE_H #define MERGE_RECURSIVE_H -#include "string-list.h" -#include "unpack-trees.h" +#include "strbuf.h" struct commit; - +struct commit_list; +struct object_id; struct repository; +struct tree; +struct merge_options_internal; struct merge_options { struct repository *repo; @@ -45,13 +47,8 @@ struct merge_options { const char *subtree_shift; unsigned renormalize : 1; - /* internal fields used by the implementation (do NOT set these) */ - int call_depth; - int needed_rename_limit; - struct hashmap current_file_dir_set; - struct string_list df_conflict_file_set; - struct unpack_trees_options unpack_opts; - struct index_state orig_index; + /* internal fields used by the implementation */ + struct merge_options_internal *priv; }; void init_merge_options(struct merge_options *opt, struct repository *repo); -- cgit v0.10.2-6-g49f6 From f3081dae014e08ece75cfb28a9844beb01b6d9f2 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:42 -0700 Subject: merge-recursive: rename MERGE_RECURSIVE_* to MERGE_VARIANT_* I want to implement the same outward facing API as found within merge-recursive.h in a different merge strategy. However, that makes names like MERGE_RECURSIVE_{NORMAL,OURS,THEIRS} look a little funny; rename to MERGE_VARIANT_{NORMAL,OURS,THEIRS}. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index c92993e..fa3f8eb 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -1045,10 +1045,10 @@ static int merge_3way(struct merge_options *opt, ll_opts.variant = 0; } else { switch (opt->recursive_variant) { - case MERGE_RECURSIVE_OURS: + case MERGE_VARIANT_OURS: ll_opts.variant = XDL_MERGE_FAVOR_OURS; break; - case MERGE_RECURSIVE_THEIRS: + case MERGE_VARIANT_THEIRS: ll_opts.variant = XDL_MERGE_FAVOR_THEIRS; break; default: @@ -1355,15 +1355,15 @@ static int merge_mode_and_contents(struct merge_options *opt, &b->oid); } else if (S_ISLNK(a->mode)) { switch (opt->recursive_variant) { - case MERGE_RECURSIVE_NORMAL: + case MERGE_VARIANT_NORMAL: oidcpy(&result->blob.oid, &a->oid); if (!oid_eq(&a->oid, &b->oid)) result->clean = 0; break; - case MERGE_RECURSIVE_OURS: + case MERGE_VARIANT_OURS: oidcpy(&result->blob.oid, &a->oid); break; - case MERGE_RECURSIVE_THEIRS: + case MERGE_VARIANT_THEIRS: oidcpy(&result->blob.oid, &b->oid); break; } @@ -3795,9 +3795,9 @@ int parse_merge_opt(struct merge_options *opt, const char *s) if (!s || !*s) return -1; if (!strcmp(s, "ours")) - opt->recursive_variant = MERGE_RECURSIVE_OURS; + opt->recursive_variant = MERGE_VARIANT_OURS; else if (!strcmp(s, "theirs")) - opt->recursive_variant = MERGE_RECURSIVE_THEIRS; + opt->recursive_variant = MERGE_VARIANT_THEIRS; else if (!strcmp(s, "subtree")) opt->subtree_shift = ""; else if (skip_prefix(s, "subtree=", &arg)) diff --git a/merge-recursive.h b/merge-recursive.h index 58a4c52..978847e 100644 --- a/merge-recursive.h +++ b/merge-recursive.h @@ -32,9 +32,9 @@ struct merge_options { /* xdiff-related options (patience, ignore whitespace, ours/theirs) */ long xdl_opts; enum { - MERGE_RECURSIVE_NORMAL = 0, - MERGE_RECURSIVE_OURS, - MERGE_RECURSIVE_THEIRS + MERGE_VARIANT_NORMAL = 0, + MERGE_VARIANT_OURS, + MERGE_VARIANT_THEIRS } recursive_variant; /* console output related options */ -- cgit v0.10.2-6-g49f6 From 45ef16f77ae5a14aa751d03c9c9fc2fa91804425 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:43 -0700 Subject: merge-recursive: add sanity checks for relevant merge_options There are lots of options that callers can set, yet most have a limited range of valid values, some options are meant for output (e.g. opt->obuf, which is expected to start empty), and callers are expected to not set opt->priv. Add several sanity checks to ensure callers provide sane values. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index fa3f8eb..0231d7b 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3615,6 +3615,30 @@ static int merge_start(struct merge_options *opt, struct tree *head) { struct strbuf sb = STRBUF_INIT; + /* Sanity checks on opt */ + assert(opt->repo); + + assert(opt->branch1 && opt->branch2); + + assert(opt->detect_renames >= -1 && + opt->detect_renames <= DIFF_DETECT_COPY); + assert(opt->detect_directory_renames >= MERGE_DIRECTORY_RENAMES_NONE && + opt->detect_directory_renames <= MERGE_DIRECTORY_RENAMES_TRUE); + assert(opt->rename_limit >= -1); + assert(opt->rename_score >= 0 && opt->rename_score <= MAX_SCORE); + assert(opt->show_rename_progress >= 0 && opt->show_rename_progress <= 1); + + assert(opt->xdl_opts >= 0); + assert(opt->recursive_variant >= MERGE_VARIANT_NORMAL && + opt->recursive_variant <= MERGE_VARIANT_THEIRS); + + assert(opt->verbosity >= 0 && opt->verbosity <= 5); + assert(opt->buffer_output <= 2); + assert(opt->obuf.len == 0); + + assert(opt->priv == NULL); + + /* Sanity check on repo state; index must match head */ if (repo_index_has_changes(opt->repo, head, &sb)) { err(opt, _("Your local changes to the following files would be overwritten by merge:\n %s"), sb.buf); -- cgit v0.10.2-6-g49f6 From 4615a8cb5b3a8d4959c30338925b1fa3b948ae52 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Sat, 17 Aug 2019 11:41:44 -0700 Subject: merge-recursive: alphabetize include list Other than cache.h which needs to appear first, and merge-recursive.h which I want to be second so that we are more likely to notice if merge-recursive.h has any missing includes, the rest of the list is long and easier to look through if it's alphabetical. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index 0231d7b..b058741 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -4,30 +4,31 @@ * The thieves were Alex Riesen and Johannes Schindelin, in June/July 2006 */ #include "cache.h" -#include "config.h" +#include "merge-recursive.h" + #include "advice.h" -#include "lockfile.h" -#include "cache-tree.h" -#include "object-store.h" -#include "repository.h" -#include "commit.h" +#include "alloc.h" +#include "attr.h" #include "blob.h" #include "builtin.h" -#include "tree-walk.h" +#include "cache-tree.h" +#include "commit.h" +#include "commit-reach.h" +#include "config.h" #include "diff.h" #include "diffcore.h" +#include "dir.h" +#include "ll-merge.h" +#include "lockfile.h" +#include "object-store.h" +#include "repository.h" +#include "revision.h" +#include "string-list.h" +#include "submodule.h" #include "tag.h" -#include "alloc.h" +#include "tree-walk.h" #include "unpack-trees.h" -#include "string-list.h" #include "xdiff-interface.h" -#include "ll-merge.h" -#include "attr.h" -#include "merge-recursive.h" -#include "dir.h" -#include "submodule.h" -#include "revision.h" -#include "commit-reach.h" struct merge_options_internal { int call_depth; -- cgit v0.10.2-6-g49f6 From 8e4ec3376e9d73bd471336cc7c11b35f5bc5dc87 Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Tue, 1 Oct 2019 11:17:27 -0700 Subject: merge-recursive: fix the diff3 common ancestor label for virtual commits In commit 743474cbfa8b ("merge-recursive: provide a better label for diff3 common ancestor", 2019-08-17), the label for the common ancestor was changed from always being "merged common ancestors" to instead be based on the number of merge bases: >=2: "merged common ancestors" 1: 0: "" Unfortunately, this did not take into account that when we have a single merge base, that merge base could be fake or constructed. In such cases, this resulted in a label of "00000000". Of course, the previous label of "merged common ancestors" was also misleading for this case. Since we have an API that is explicitly about creating fake merge base commits in merge_recursive_generic(), we should provide a better label when using that API with one merge base. So, when merge_recursive_generic() is called with one merge base, set the label to: "constructed merge base" Note that callers of merge_recursive_generic() include the builtin commands git-am (in combination with git apply --build-fake-ancestor), git-merge-recursive, and git-stash. Helped-by: Jeff King Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index b058741..e12d91f 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3550,6 +3550,8 @@ static int merge_recursive_internal(struct merge_options *opt, merged_merge_bases = make_virtual_commit(opt->repo, tree, "ancestor"); ancestor_name = "empty tree"; + } else if (opt->ancestor) { + ancestor_name = opt->ancestor; } else if (merge_bases) { ancestor_name = "merged common ancestors"; } else { @@ -3689,7 +3691,8 @@ int merge_recursive(struct merge_options *opt, { int clean; - assert(opt->ancestor == NULL); + assert(opt->ancestor == NULL || + !strcmp(opt->ancestor, "constructed merge base")); if (merge_start(opt, repo_get_commit_tree(opt->repo, h1))) return -1; @@ -3741,6 +3744,8 @@ int merge_recursive_generic(struct merge_options *opt, oid_to_hex(merge_bases[i])); commit_list_insert(base, &ca); } + if (num_merge_bases == 1) + opt->ancestor = "constructed merge base"; } repo_hold_locked_index(opt->repo, &lock, LOCK_DIE_ON_ERROR); diff --git a/t/t6047-diff3-conflict-markers.sh b/t/t6047-diff3-conflict-markers.sh index 3fb68e0..860542a 100755 --- a/t/t6047-diff3-conflict-markers.sh +++ b/t/t6047-diff3-conflict-markers.sh @@ -186,4 +186,17 @@ test_expect_success 'check multiple merge bases' ' ) ' +test_expect_success 'rebase describes fake ancestor base' ' + test_create_repo rebase && + ( + cd rebase && + test_commit base file && + test_commit master file && + git checkout -b side HEAD^ && + test_commit side file && + test_must_fail git -c merge.conflictstyle=diff3 rebase master && + grep "||||||| constructed merge base" file + ) +' + test_done -- cgit v0.10.2-6-g49f6 From b6570477193b8cf75ce625b8d540e28f71ece3fe Mon Sep 17 00:00:00 2001 From: Elijah Newren Date: Mon, 7 Oct 2019 08:52:11 -0700 Subject: merge-recursive: fix the fix to the diff3 common ancestor label In commit 8e4ec337 ("merge-recursive: fix the diff3 common ancestor label for virtual commits", 2019-10-01), which was a fix to commit 743474cbfa8b ("merge-recursive: provide a better label for diff3 common ancestor", 2019-08-17), the label for the common ancestor was changed from always being "merged common ancestors" to instead be based on the number of merge bases and whether the merge base was a real commit or a virtual one: >=2: "merged common ancestors" 1, via merge_recursive_generic: "constructed merge base" 1, otherwise: 0: "" The handling for "constructed merge base" worked by allowing opt->ancestor to be set in merge_recursive_generic(), so we paid attention to the setting of that variable in merge_recursive_internal(). Now, for the outer merge, the code flow was simply the following: ancestor_name = "merged merge bases" loop over merge_bases: merge_recursive_internal() The first merge base not needing recursion would determine its own ancestor_name however necessary and thus run ancestor_name = $SOMETHING empty loop over merge_bases... opt->ancestor = ancestor_name merge_trees_internal() Now, the next set of merge_bases that would need to be merged after this particular merge had completed would note that opt->ancestor has been set to something (to a local ancestor_name variable that has since been popped off the stack), and thus it would run: ... else if (opt->ancestor) { ancestor_name = opt->ancestor; /* OOPS! */ loop over merge_bases: merge_recursive_internal() opt->ancestor = ancestor_name merge_trees_internal() This resulted in garbage strings being printed for the virtual merge bases, which was visible in git.git by just merging commit b744c3af07 into commit 6d8cb22a4f. There are two ways to fix this: set opt->ancestor to NULL after using it to avoid re-use, or add a !opt->priv->call_depth check to the if block for using a pre-defined opt->ancestor. Apply both fixes. Signed-off-by: Elijah Newren Signed-off-by: Junio C Hamano diff --git a/merge-recursive.c b/merge-recursive.c index e12d91f..2653ba9 100644 --- a/merge-recursive.c +++ b/merge-recursive.c @@ -3550,7 +3550,7 @@ static int merge_recursive_internal(struct merge_options *opt, merged_merge_bases = make_virtual_commit(opt->repo, tree, "ancestor"); ancestor_name = "empty tree"; - } else if (opt->ancestor) { + } else if (opt->ancestor && !opt->priv->call_depth) { ancestor_name = opt->ancestor; } else if (merge_bases) { ancestor_name = "merged common ancestors"; @@ -3600,6 +3600,7 @@ static int merge_recursive_internal(struct merge_options *opt, merged_merge_bases), &result_tree); strbuf_release(&merge_base_abbrev); + opt->ancestor = NULL; /* avoid accidental re-use of opt->ancestor */ if (clean < 0) { flush_output(opt); return clean; -- cgit v0.10.2-6-g49f6