From: Seija K <pi1024e@xxxxxxxxxx> commit: Avoid extraneous boolean checking by simplifying the if statements. This is so that the code can be clearer and we can avoid duplicate checks. Changes since v1: Undid if statement combos as suggested by Junio C Hamano. Currently, the logic for merging is somewhat confusing. So I simplified said logic to be equivalent, but simpler. I tested all my changes to ensure in practice they work as well. First, I tested out which branch would occur for every possible value below: remoteheads->next | common->next | option_commit | Branch -- | -- | -- | -- T | T | T | A T | T | F | A T | F | T | A T | F | F | A F | T | T | C F | T | F | C F | F | T | B F | F | F | A Then, using this truth table, I was able to deduce that if the second branch ran, the if statement for the first branch would be false. Then, taking the inverse, I found that many of the checks were redundant, so I rewrote the if statements to have each branch run under the same exact conditions, except each value is evaluated as little as possible. I hope you find value in these changes. Thank you! Signed-off-by: Seija K. <pi1024e@xxxxxxxxxx> --- Simplify merge logic commit: Avoid extraneous boolean checking by simplifying the if statements. This is so that the code can be clearer and we can avoid duplicate checks. Changes since v1: Undid if statement combos as suggested by Junio C Hamano. Currently, the logic for merging is somewhat confusing. So I simplified said logic to be equivalent, but simpler. I tested all my changes to ensure in practice they work as well. First, I tested out which branch would occur for every possible value below: remoteheads->nextcommon->nextoption_commitBranchTTTATTFATFTATFFAFTTCFTFC FFTBFFFAThen, using this truth table, I was able to deduce that if the second branch ran, the if statement for the first branch would be false. Then, taking the inverse, I found that many of the checks were redundant, so I rewrote the if statements to have each branch run under the same exact conditions, except each value is evaluated as little as possible. I hope you find value in these changes. Thank you! Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-911%2Fpi1024e%2Fmerge-cleanup-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-911/pi1024e/merge-cleanup-v3 Pull-Request: https://github.com/git/git/pull/911 Range-diff vs v2: 1: dd1c4dd678 ! 1: 92e0b7ec32 Simplify merge logic @@ Commit message Simplify merge logic commit: Avoid extraneous boolean checking by simplifying the if statements. - This is so that the code can be clearer and avoid duplicate boolean checks. + This is so that the code can be clearer and we can avoid duplicate checks. Changes since v1: Undid if statement combos as suggested by Junio C Hamano. - The current logic for the merging is somewhat confusing. + Currently, the logic for merging is somewhat confusing. So I simplified said logic to be equivalent, but simpler. I tested all my changes to ensure in practice they work as well. builtin/merge.c | 82 +++++++++++++++++++++++-------------------------- 1 file changed, 38 insertions(+), 44 deletions(-) diff --git a/builtin/merge.c b/builtin/merge.c index 4c133402a6..148d08f8db 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1213,7 +1213,7 @@ static int merging_a_throwaway_tag(struct commit *commit) if (!merge_remote_util(commit) || !merge_remote_util(commit)->obj || merge_remote_util(commit)->obj->type != OBJ_TAG) - return is_throwaway_tag; + return 0; /* * Now we know we are merging a tag object. Are we downstream @@ -1460,12 +1460,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix) } if (!use_strategies) { - if (!remoteheads) - ; /* already up-to-date */ - else if (!remoteheads->next) - add_strategies(pull_twohead, DEFAULT_TWOHEAD); - else - add_strategies(pull_octopus, DEFAULT_OCTOPUS); + if (remoteheads) { + /* not up-to-date */ + if (remoteheads->next) + add_strategies(pull_octopus, DEFAULT_OCTOPUS); + else + add_strategies(pull_twohead, DEFAULT_TWOHEAD); + } } for (i = 0; i < use_strategies_nr; i++) { @@ -1475,15 +1476,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix) allow_trivial = 0; } - if (!remoteheads) - ; /* already up-to-date */ - else if (!remoteheads->next) - common = get_merge_bases(head_commit, remoteheads->item); - else { - struct commit_list *list = remoteheads; - commit_list_insert(head_commit, &list); - common = get_octopus_merge_bases(list); - free(list); + if (remoteheads) { + /* not up-to-date */ + if (remoteheads->next) { + struct commit_list *list = remoteheads; + commit_list_insert(head_commit, &list); + common = get_octopus_merge_bases(list); + free(list); + } else + common = get_merge_bases(head_commit, remoteheads->item); } update_ref("updating ORIG_HEAD", "ORIG_HEAD", @@ -1542,31 +1543,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) finish(head_commit, remoteheads, &commit->object.oid, msg.buf); remove_merge_branch_state(the_repository); goto done; - } else if (!remoteheads->next && common->next) - ; - /* - * We are not doing octopus and not fast-forward. Need - * a real merge. - */ - else if (!remoteheads->next && !common->next && option_commit) { - /* - * We are not doing octopus, not fast-forward, and have - * only one common. - */ - refresh_cache(REFRESH_QUIET); - if (allow_trivial && fast_forward != FF_ONLY) { - /* See if it is really trivial. */ - git_committer_info(IDENT_STRICT); - printf(_("Trying really trivial in-index merge...\n")); - if (!read_tree_trivial(&common->item->object.oid, - &head_commit->object.oid, - &remoteheads->item->object.oid)) { - ret = merge_trivial(head_commit, remoteheads); - goto done; - } - printf(_("Nope.\n")); - } - } else { + } else if (remoteheads->next || (!common->next && !option_commit)) { /* * An octopus. If we can reach all the remote we are up * to date. @@ -1592,6 +1569,24 @@ int cmd_merge(int argc, const char **argv, const char *prefix) finish_up_to_date(_("Already up to date. Yeeah!")); goto done; } + } else if (!common->next) { + /* + * We are not doing octopus, not fast-forward, and have + * only one common. + */ + refresh_cache(REFRESH_QUIET); + if (allow_trivial && fast_forward != FF_ONLY) { + /* See if it is really trivial. */ + git_committer_info(IDENT_STRICT); + printf(_("Trying really trivial in-index merge...\n")); + if (!read_tree_trivial(&common->item->object.oid, + &head_commit->object.oid, + &remoteheads->item->object.oid)) { + ret = merge_trivial(head_commit, remoteheads); + goto done; + } + printf(_("Nope.\n")); + } } if (fast_forward == FF_ONLY) @@ -1685,9 +1680,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix) use_strategies[0]->name); ret = 2; goto done; - } else if (best_strategy == wt_strategy) - ; /* We already have its result in the working tree. */ - else { + } else if (best_strategy != wt_strategy) { + /* We do not have its result in the working tree. */ printf(_("Rewinding the tree to pristine...\n")); restore_state(&head_commit->object.oid, &stash); printf(_("Using the %s to prepare resolving by hand.\n"), base-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500 -- gitgitgadget