{"thread":{"id":"54602","subject":"[PATCH] Simplified merge logic","startedAt":"2020-11-08T21:39:04Z","lastAt":"2020-11-17T22:06:30Z","messageCount":4,"participants":["Seija K. via GitGitGadget","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"409338","messageId":"pull.911.git.git.1604871456715.gitgitgadget@gmail.com","threadId":"54602","inReplyTo":null,"subject":"[PATCH] Simplified merge logic","fromName":"Seija K. via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-08T21:37:36Z","receivedAt":"2020-11-08T21:39:04Z","isPatch":true,"sender":{"key":"name:Seija K.","avatar":null},"body":"From: pi1024e <pi1024e@github.com>\n\ncommit: Avoid extraneous boolean checking by simplifying the if statements.\nSigned-off-by: Seija <pi1024e@github.com>\n---\n    Simplified merge logic\n    \n    The logic for the merging is somewhat confusing. So I simplified it to\n    be equivalent. I tested all my changes to ensure in practice they work.\n    \n    The first thing I did was test out which branch would occur for every\n    possible value of \n    \n    remoteheads->nextcommon->nextoption_commitBranchTTTATTFATFTATFFAFTTCFTFC\n    FFTBFFFAUsing this truth table, I was able to deduce that if the second\n    branch ran, the if statement for the first branch was false. Taking the\n    inverse, it was then found many of the checks were redundant, so the if\n    statement was rewritten to have each branch run under the same exact\n    conditions, except each value is evaluated as little as possible.\n    \n    I hope you can approve of this and show me how to send it.\n    \n    Thank you.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-911%2Fpi1024e%2Fmerge-cleanup-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-911/pi1024e/merge-cleanup-v1\nPull-Request: https://github.com/git/git/pull/911\n\n builtin/merge.c | 85 ++++++++++++++++++++++---------------------------\n 1 file changed, 38 insertions(+), 47 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4c133402a6..9664da6031 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -853,9 +853,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n \t\t\t    git_path_merge_msg(the_repository), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n-\tif (0 < option_edit) {\n-\t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n-\t\t\tabort_commit(remoteheads, NULL);\n+\tif (0 < option_edit && launch_editor(git_path_merge_msg(the_repository), NULL, NULL)) {\n+\t\tabort_commit(remoteheads, NULL);\n \t}\n \n \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n@@ -1213,7 +1212,7 @@ static int merging_a_throwaway_tag(struct commit *commit)\n \tif (!merge_remote_util(commit) ||\n \t    !merge_remote_util(commit)->obj ||\n \t    merge_remote_util(commit)->obj->type != OBJ_TAG)\n-\t\treturn is_throwaway_tag;\n+\t\treturn 0;\n \n \t/*\n \t * Now we know we are merging a tag object.  Are we downstream\n@@ -1459,13 +1458,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tfast_forward = FF_NO;\n \t}\n \n-\tif (!use_strategies) {\n-\t\tif (!remoteheads)\n-\t\t\t; /* already up-to-date */\n-\t\telse if (!remoteheads->next)\n-\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n-\t\telse\n+\tif (!use_strategies && remoteheads) {\n+\t\t/* not up-to-date */\n+\t\tif (remoteheads->next)\n \t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n+\t\telse\n+\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n \t}\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n@@ -1475,15 +1473,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tallow_trivial = 0;\n \t}\n \n-\tif (!remoteheads)\n-\t\t; /* already up-to-date */\n-\telse if (!remoteheads->next)\n-\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n-\telse {\n-\t\tstruct commit_list *list = remoteheads;\n-\t\tcommit_list_insert(head_commit, &list);\n-\t\tcommon = get_octopus_merge_bases(list);\n-\t\tfree(list);\n+\tif (remoteheads) {\n+\t\t/* not up-to-date */\n+\t\tif (remoteheads->next) {\n+\t\t\tstruct commit_list *list = remoteheads;\n+\t\t\tcommit_list_insert(head_commit, &list);\n+\t\t\tcommon = get_octopus_merge_bases(list);\n+\t\t\tfree(list);\n+\t\t} else\n+\t\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n \t}\n \n \tupdate_ref(\"updating ORIG_HEAD\", \"ORIG_HEAD\",\n@@ -1542,31 +1540,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tfinish(head_commit, remoteheads, &commit->object.oid, msg.buf);\n \t\tremove_merge_branch_state(the_repository);\n \t\tgoto done;\n-\t} else if (!remoteheads->next && common->next)\n-\t\t;\n-\t\t/*\n-\t\t * We are not doing octopus and not fast-forward.  Need\n-\t\t * a real merge.\n-\t\t */\n-\telse if (!remoteheads->next && !common->next && option_commit) {\n-\t\t/*\n-\t\t * We are not doing octopus, not fast-forward, and have\n-\t\t * only one common.\n-\t\t */\n-\t\trefresh_cache(REFRESH_QUIET);\n-\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n-\t\t\t/* See if it is really trivial. */\n-\t\t\tgit_committer_info(IDENT_STRICT);\n-\t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n-\t\t\tif (!read_tree_trivial(&common->item->object.oid,\n-\t\t\t\t\t       &head_commit->object.oid,\n-\t\t\t\t\t       &remoteheads->item->object.oid)) {\n-\t\t\t\tret = merge_trivial(head_commit, remoteheads);\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t\tprintf(_(\"Nope.\\n\"));\n-\t\t}\n-\t} else {\n+\t} else if (remoteheads->next || (!common->next && !option_commit)) {\n \t\t/*\n \t\t * An octopus.  If we can reach all the remote we are up\n \t\t * to date.\n@@ -1592,6 +1566,24 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tfinish_up_to_date(_(\"Already up to date. Yeeah!\"));\n \t\t\tgoto done;\n \t\t}\n+\t} else if (!common->next) {\n+\t\t/*\n+\t\t * We are not doing octopus, not fast-forward, and have\n+\t\t * only one common.\n+\t\t */\n+\t\trefresh_cache(REFRESH_QUIET);\n+\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n+\t\t\t/* See if it is really trivial. */\n+\t\t\tgit_committer_info(IDENT_STRICT);\n+\t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n+\t\t\tif (!read_tree_trivial(&common->item->object.oid,\n+\t\t\t\t\t       &head_commit->object.oid,\n+\t\t\t\t\t       &remoteheads->item->object.oid)) {\n+\t\t\t\tret = merge_trivial(head_commit, remoteheads);\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t\tprintf(_(\"Nope.\\n\"));\n+\t\t}\n \t}\n \n \tif (fast_forward == FF_ONLY)\n@@ -1685,9 +1677,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tuse_strategies[0]->name);\n \t\tret = 2;\n \t\tgoto done;\n-\t} else if (best_strategy == wt_strategy)\n-\t\t; /* We already have its result in the working tree. */\n-\telse {\n+\t} else if (best_strategy != wt_strategy) {\n+\t\t/* We do not have its result in the working tree. */\n \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n \t\trestore_state(&head_commit->object.oid, &stash);\n \t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n\nbase-commit: 7f7ebe054af6d831b999d6c2241b9227c4e4e08d\n-- \ngitgitgadget\n"},{"id":"409435","messageId":"xmqqo8k69l3e.fsf@gitster.c.googlers.com","threadId":"54602","inReplyTo":"pull.911.git.git.1604871456715.gitgitgadget@gmail.com","subject":"Re: [PATCH] Simplified merge logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-11-09T23:17:09Z","receivedAt":"2020-11-09T23:17:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Seija K. via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: pi1024e <pi1024e@github.com>\n>\n> commit: Avoid extraneous boolean checking by simplifying the if statements.\n> Signed-off-by: Seija <pi1024e@github.com>\n> ---\n\nMeh.\n\nAdmittedly, readability is somewhat subjective, but a rewrite like\n\n        if (condition)                  if (!condition)\n                do_when_true();                 do_when_false();\n        else                       ==>  else\n                do_when_false();                do_when_true();\n\nwhile it may not be incorrect per-se needs more than a subjective \"I\nthink this is more readable\" to justify the code churn.\n\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 4c133402a6..9664da6031 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -853,9 +853,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n>  \tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n>  \t\t\t    git_path_merge_msg(the_repository), \"merge\", NULL))\n>  \t\tabort_commit(remoteheads, NULL);\n> -\tif (0 < option_edit) {\n> -\t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n> -\t\t\tabort_commit(remoteheads, NULL);\n> +\tif (0 < option_edit && launch_editor(git_path_merge_msg(the_repository), NULL, NULL)) {\n> +\t\tabort_commit(remoteheads, NULL);\n>  \t}\n\nThis may reduce the number of lines, but personally I find that\n\n\tif (are we editing?) {\n\t\tif (run editor---did we fail?)\n\t\t\tabort();\n\t}\n\nis much easier to read.\n\nAnd much more importantly, it would be much easier to extend later\nwhat hwppens when we decide to edit, than the new code proposed by\nthis patch.\n\n> @@ -1213,7 +1212,7 @@ static int merging_a_throwaway_tag(struct commit *commit)\n>  \tif (!merge_remote_util(commit) ||\n>  \t    !merge_remote_util(commit)->obj ||\n>  \t    merge_remote_util(commit)->obj->type != OBJ_TAG)\n> -\t\treturn is_throwaway_tag;\n> +\t\treturn 0;\n\nLikewise.  If somebody _must_ touch this function to gain commit\ncount without making the code harder to maintain, it may be an\noption to use \"goto leave;\" here and then create a \"leave:\" label\nbefore the other \"return\"---at least that may be worth considering,\nbut not this---when everybody else in the function wants to maintain\nthat the value in this variable is what is returned from the\nfunction.\n\n> @@ -1459,13 +1458,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\tfast_forward = FF_NO;\n>  \t}\n>  \n> -\tif (!use_strategies) {\n> -\t\tif (!remoteheads)\n> -\t\t\t; /* already up-to-date */\n> -\t\telse if (!remoteheads->next)\n> -\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n> -\t\telse\n> +\tif (!use_strategies && remoteheads) {\n> +\t\t/* not up-to-date */\n> +\t\tif (remoteheads->next)\n>  \t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n> +\t\telse\n> +\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n>  \t}\n\nLikewise.\n\n\tif (do we have to choose strategies ourselves?) {\n\t\t... depending on the case, choose strategy ...\n\t}\n\nis much easier to reason about than\n\n\tif (do we have to choose strategies ourselves?, oh by the\n    \t    way, don't forget that already up-to-date case we do not\n\t    have to choose) {\n\t\t... the remainder ...\n\t}\n\nand extend when we need to add something to do in the up-to-date\ncase as long as the end-user did not specify which strategy to use\nin the future.  The reduced line count alone is not a good yardstick\nto use when talking about code restructuring for readability and\nmaintainability.\n\nThanks.\n"},{"id":"410168","messageId":"pull.911.v2.git.git.1605650072908.gitgitgadget@gmail.com","threadId":"54602","inReplyTo":"pull.911.git.git.1604871456715.gitgitgadget@gmail.com","subject":"[PATCH v2] Simplify merge logic","fromName":"Seija K. via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-17T21:54:32Z","receivedAt":"2020-11-17T21:54:37Z","isPatch":true,"sender":{"key":"name:Seija K.","avatar":null},"body":"From: Seija K <pi1024e@github.com>\n\ncommit: Avoid extraneous boolean checking by simplifying the if statements.\nThis is so that the code can be clearer and avoid duplicate boolean checks.\n\nChanges since v1: Undid if statement combos as suggested by Junio C Hamano.\n\nThe current logic for the merging is somewhat confusing.\nSo I simplified said logic to be equivalent, but simpler.\nI tested all my changes to ensure in practice they work as well.\n\nFirst, I tested out which branch would occur for every possible value below:\n\nremoteheads->next | common->next | option_commit | Branch\n-- | -- | -- | --\nT | T | T | A\nT | T | F | A\nT | F | T | A\nT | F | F | A\nF | T | T | C\nF | T | F | C\nF | F | T | B\nF | F | F | A\n\nThen, using this truth table, I was able to deduce that if the second branch ran,\nthe if statement for the first branch would be false.\n\nThen, taking the inverse, I found that many of the checks were redundant,\nso I rewrote the if statements to have each branch run under the same exact conditions,\nexcept each value is evaluated as little as possible.\n\nI hope you find value in these changes.\n\nThank you!\n\nSigned-off-by: Seija K. <pi1024e@github.com>\n---\n    Simplify merge logic\n    \n    This is so that the code can be clearer and avoid duplicate boolean\n    checks.\n    \n    Changes since v1: Undid if statement combos as suggested by Junio C\n    Hamano.\n    \n    The current logic for the merging is somewhat confusing. So I simplified\n    said logic to be equivalent, but simpler. I tested all my changes to\n    ensure in practice they work as well.\n    \n    First, I tested out which branch would occur for every possible value\n    below:\n    \n    remoteheads->nextcommon->nextoption_commitBranchTTTATTFATFTATFFAFTTCFTFC\n    FFTBFFFAThen, using this truth table, I was able to deduce that if the\n    second branch ran, the if statement for the first branch would be false.\n    \n    Then, taking the inverse, I found that many of the checks were\n    redundant, so I rewrote the if statements to have each branch run under\n    the same exact conditions, except each value is evaluated as little as\n    possible.\n    \n    I hope you find value in these changes.\n    \n    Thank you!\n    \n    Signed-off-by: Seija K. pi1024e@github.com [pi1024e@github.com]\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-911%2Fpi1024e%2Fmerge-cleanup-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-911/pi1024e/merge-cleanup-v2\nPull-Request: https://github.com/git/git/pull/911\n\nRange-diff vs v1:\n\n 1:  9b333c9872 ! 1:  dd1c4dd678 Simplified merge logic\n     @@\n       ## Metadata ##\n     -Author: pi1024e <pi1024e@github.com>\n     +Author: Seija K <pi1024e@github.com>\n      \n       ## Commit message ##\n     -    Simplified merge logic\n     +    Simplify merge logic\n      \n          commit: Avoid extraneous boolean checking by simplifying the if statements.\n     -    Signed-off-by: Seija <pi1024e@github.com>\n     +    This is so that the code can be clearer and avoid duplicate boolean checks.\n     +\n     +    Changes since v1: Undid if statement combos as suggested by Junio C Hamano.\n     +\n     +    The current logic for the merging is somewhat confusing.\n     +    So I simplified said logic to be equivalent, but simpler.\n     +    I tested all my changes to ensure in practice they work as well.\n     +\n     +    First, I tested out which branch would occur for every possible value below:\n     +\n     +    remoteheads->next | common->next | option_commit | Branch\n     +    -- | -- | -- | --\n     +    T | T | T | A\n     +    T | T | F | A\n     +    T | F | T | A\n     +    T | F | F | A\n     +    F | T | T | C\n     +    F | T | F | C\n     +    F | F | T | B\n     +    F | F | F | A\n     +\n     +    Then, using this truth table, I was able to deduce that if the second branch ran,\n     +    the if statement for the first branch would be false.\n     +\n     +    Then, taking the inverse, I found that many of the checks were redundant,\n     +    so I rewrote the if statements to have each branch run under the same exact conditions,\n     +    except each value is evaluated as little as possible.\n     +\n     +    I hope you find value in these changes.\n     +\n     +    Thank you!\n     +\n     +    Signed-off-by: Seija K. <pi1024e@github.com>\n      \n       ## builtin/merge.c ##\n     -@@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)\n     - \tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n     - \t\t\t    git_path_merge_msg(the_repository), \"merge\", NULL))\n     - \t\tabort_commit(remoteheads, NULL);\n     --\tif (0 < option_edit) {\n     --\t\tif (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))\n     --\t\t\tabort_commit(remoteheads, NULL);\n     -+\tif (0 < option_edit && launch_editor(git_path_merge_msg(the_repository), NULL, NULL)) {\n     -+\t\tabort_commit(remoteheads, NULL);\n     - \t}\n     - \n     - \tif (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),\n      @@ builtin/merge.c: static int merging_a_throwaway_tag(struct commit *commit)\n       \tif (!merge_remote_util(commit) ||\n       \t    !merge_remote_util(commit)->obj ||\n     @@ builtin/merge.c: static int merging_a_throwaway_tag(struct commit *commit)\n       \t/*\n       \t * Now we know we are merging a tag object.  Are we downstream\n      @@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)\n     - \t\t\tfast_forward = FF_NO;\n       \t}\n       \n     --\tif (!use_strategies) {\n     + \tif (!use_strategies) {\n      -\t\tif (!remoteheads)\n      -\t\t\t; /* already up-to-date */\n      -\t\telse if (!remoteheads->next)\n      -\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n      -\t\telse\n     -+\tif (!use_strategies && remoteheads) {\n     -+\t\t/* not up-to-date */\n     -+\t\tif (remoteheads->next)\n     - \t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n     -+\t\telse\n     -+\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n     +-\t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n     ++\t\tif (remoteheads) {\n     ++\t\t\t/* not up-to-date */\n     ++\t\t\tif (remoteheads->next)\n     ++\t\t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n     ++\t\t\telse\n     ++\t\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n     ++\t\t}\n       \t}\n       \n       \tfor (i = 0; i < use_strategies_nr; i++) {\n\n\n builtin/merge.c | 82 +++++++++++++++++++++++--------------------------\n 1 file changed, 38 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4c133402a6..148d08f8db 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1213,7 +1213,7 @@ static int merging_a_throwaway_tag(struct commit *commit)\n \tif (!merge_remote_util(commit) ||\n \t    !merge_remote_util(commit)->obj ||\n \t    merge_remote_util(commit)->obj->type != OBJ_TAG)\n-\t\treturn is_throwaway_tag;\n+\t\treturn 0;\n \n \t/*\n \t * Now we know we are merging a tag object.  Are we downstream\n@@ -1460,12 +1460,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!use_strategies) {\n-\t\tif (!remoteheads)\n-\t\t\t; /* already up-to-date */\n-\t\telse if (!remoteheads->next)\n-\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n-\t\telse\n-\t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n+\t\tif (remoteheads) {\n+\t\t\t/* not up-to-date */\n+\t\t\tif (remoteheads->next)\n+\t\t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n+\t\t\telse\n+\t\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n+\t\t}\n \t}\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n@@ -1475,15 +1476,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tallow_trivial = 0;\n \t}\n \n-\tif (!remoteheads)\n-\t\t; /* already up-to-date */\n-\telse if (!remoteheads->next)\n-\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n-\telse {\n-\t\tstruct commit_list *list = remoteheads;\n-\t\tcommit_list_insert(head_commit, &list);\n-\t\tcommon = get_octopus_merge_bases(list);\n-\t\tfree(list);\n+\tif (remoteheads) {\n+\t\t/* not up-to-date */\n+\t\tif (remoteheads->next) {\n+\t\t\tstruct commit_list *list = remoteheads;\n+\t\t\tcommit_list_insert(head_commit, &list);\n+\t\t\tcommon = get_octopus_merge_bases(list);\n+\t\t\tfree(list);\n+\t\t} else\n+\t\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n \t}\n \n \tupdate_ref(\"updating ORIG_HEAD\", \"ORIG_HEAD\",\n@@ -1542,31 +1543,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tfinish(head_commit, remoteheads, &commit->object.oid, msg.buf);\n \t\tremove_merge_branch_state(the_repository);\n \t\tgoto done;\n-\t} else if (!remoteheads->next && common->next)\n-\t\t;\n-\t\t/*\n-\t\t * We are not doing octopus and not fast-forward.  Need\n-\t\t * a real merge.\n-\t\t */\n-\telse if (!remoteheads->next && !common->next && option_commit) {\n-\t\t/*\n-\t\t * We are not doing octopus, not fast-forward, and have\n-\t\t * only one common.\n-\t\t */\n-\t\trefresh_cache(REFRESH_QUIET);\n-\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n-\t\t\t/* See if it is really trivial. */\n-\t\t\tgit_committer_info(IDENT_STRICT);\n-\t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n-\t\t\tif (!read_tree_trivial(&common->item->object.oid,\n-\t\t\t\t\t       &head_commit->object.oid,\n-\t\t\t\t\t       &remoteheads->item->object.oid)) {\n-\t\t\t\tret = merge_trivial(head_commit, remoteheads);\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t\tprintf(_(\"Nope.\\n\"));\n-\t\t}\n-\t} else {\n+\t} else if (remoteheads->next || (!common->next && !option_commit)) {\n \t\t/*\n \t\t * An octopus.  If we can reach all the remote we are up\n \t\t * to date.\n@@ -1592,6 +1569,24 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tfinish_up_to_date(_(\"Already up to date. Yeeah!\"));\n \t\t\tgoto done;\n \t\t}\n+\t} else if (!common->next) {\n+\t\t/*\n+\t\t * We are not doing octopus, not fast-forward, and have\n+\t\t * only one common.\n+\t\t */\n+\t\trefresh_cache(REFRESH_QUIET);\n+\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n+\t\t\t/* See if it is really trivial. */\n+\t\t\tgit_committer_info(IDENT_STRICT);\n+\t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n+\t\t\tif (!read_tree_trivial(&common->item->object.oid,\n+\t\t\t\t\t       &head_commit->object.oid,\n+\t\t\t\t\t       &remoteheads->item->object.oid)) {\n+\t\t\t\tret = merge_trivial(head_commit, remoteheads);\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t\tprintf(_(\"Nope.\\n\"));\n+\t\t}\n \t}\n \n \tif (fast_forward == FF_ONLY)\n@@ -1685,9 +1680,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tuse_strategies[0]->name);\n \t\tret = 2;\n \t\tgoto done;\n-\t} else if (best_strategy == wt_strategy)\n-\t\t; /* We already have its result in the working tree. */\n-\telse {\n+\t} else if (best_strategy != wt_strategy) {\n+\t\t/* We do not have its result in the working tree. */\n \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n \t\trestore_state(&head_commit->object.oid, &stash);\n \t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n\nbase-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\n-- \ngitgitgadget\n"},{"id":"410170","messageId":"pull.911.v3.git.git.1605650762205.gitgitgadget@gmail.com","threadId":"54602","inReplyTo":"pull.911.v2.git.git.1605650072908.gitgitgadget@gmail.com","subject":"[PATCH v3] Simplify merge logic","fromName":"Seija K. via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-17T22:06:01Z","receivedAt":"2020-11-17T22:06:30Z","isPatch":true,"sender":{"key":"name:Seija K.","avatar":null},"body":"From: Seija K <pi1024e@github.com>\n\ncommit: Avoid extraneous boolean checking by simplifying the if statements.\nThis is so that the code can be clearer and we can avoid duplicate checks.\n\nChanges since v1: Undid if statement combos as suggested by Junio C Hamano.\n\nCurrently, the logic for merging is somewhat confusing.\nSo I simplified said logic to be equivalent, but simpler.\nI tested all my changes to ensure in practice they work as well.\n\nFirst, I tested out which branch would occur for every possible value below:\n\nremoteheads->next | common->next | option_commit | Branch\n-- | -- | -- | --\nT | T | T | A\nT | T | F | A\nT | F | T | A\nT | F | F | A\nF | T | T | C\nF | T | F | C\nF | F | T | B\nF | F | F | A\n\nThen, using this truth table, I was able to deduce that if the second branch ran,\nthe if statement for the first branch would be false.\n\nThen, taking the inverse, I found that many of the checks were redundant,\nso I rewrote the if statements to have each branch run under the same exact conditions,\nexcept each value is evaluated as little as possible.\n\nI hope you find value in these changes.\n\nThank you!\n\nSigned-off-by: Seija K. <pi1024e@github.com>\n---\n    Simplify merge logic\n    \n    commit: Avoid extraneous boolean checking by simplifying the if\n    statements. This is so that the code can be clearer and we can avoid\n    duplicate checks.\n    \n    Changes since v1: Undid if statement combos as suggested by Junio C\n    Hamano.\n    \n    Currently, the logic for merging is somewhat confusing. So I simplified\n    said logic to be equivalent, but simpler. I tested all my changes to\n    ensure in practice they work as well.\n    \n    First, I tested out which branch would occur for every possible value\n    below:\n    \n    remoteheads->nextcommon->nextoption_commitBranchTTTATTFATFTATFFAFTTCFTFC\n    FFTBFFFAThen, using this truth table, I was able to deduce that if the\n    second branch ran, the if statement for the first branch would be false.\n    \n    Then, taking the inverse, I found that many of the checks were\n    redundant, so I rewrote the if statements to have each branch run under\n    the same exact conditions, except each value is evaluated as little as\n    possible.\n    \n    I hope you find value in these changes.\n    \n    Thank you!\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-911%2Fpi1024e%2Fmerge-cleanup-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-911/pi1024e/merge-cleanup-v3\nPull-Request: https://github.com/git/git/pull/911\n\nRange-diff vs v2:\n\n 1:  dd1c4dd678 ! 1:  92e0b7ec32 Simplify merge logic\n     @@ Commit message\n          Simplify merge logic\n      \n          commit: Avoid extraneous boolean checking by simplifying the if statements.\n     -    This is so that the code can be clearer and avoid duplicate boolean checks.\n     +    This is so that the code can be clearer and we can avoid duplicate checks.\n      \n          Changes since v1: Undid if statement combos as suggested by Junio C Hamano.\n      \n     -    The current logic for the merging is somewhat confusing.\n     +    Currently, the logic for merging is somewhat confusing.\n          So I simplified said logic to be equivalent, but simpler.\n          I tested all my changes to ensure in practice they work as well.\n      \n\n\n builtin/merge.c | 82 +++++++++++++++++++++++--------------------------\n 1 file changed, 38 insertions(+), 44 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 4c133402a6..148d08f8db 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1213,7 +1213,7 @@ static int merging_a_throwaway_tag(struct commit *commit)\n \tif (!merge_remote_util(commit) ||\n \t    !merge_remote_util(commit)->obj ||\n \t    merge_remote_util(commit)->obj->type != OBJ_TAG)\n-\t\treturn is_throwaway_tag;\n+\t\treturn 0;\n \n \t/*\n \t * Now we know we are merging a tag object.  Are we downstream\n@@ -1460,12 +1460,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (!use_strategies) {\n-\t\tif (!remoteheads)\n-\t\t\t; /* already up-to-date */\n-\t\telse if (!remoteheads->next)\n-\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n-\t\telse\n-\t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n+\t\tif (remoteheads) {\n+\t\t\t/* not up-to-date */\n+\t\t\tif (remoteheads->next)\n+\t\t\t\tadd_strategies(pull_octopus, DEFAULT_OCTOPUS);\n+\t\t\telse\n+\t\t\t\tadd_strategies(pull_twohead, DEFAULT_TWOHEAD);\n+\t\t}\n \t}\n \n \tfor (i = 0; i < use_strategies_nr; i++) {\n@@ -1475,15 +1476,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tallow_trivial = 0;\n \t}\n \n-\tif (!remoteheads)\n-\t\t; /* already up-to-date */\n-\telse if (!remoteheads->next)\n-\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n-\telse {\n-\t\tstruct commit_list *list = remoteheads;\n-\t\tcommit_list_insert(head_commit, &list);\n-\t\tcommon = get_octopus_merge_bases(list);\n-\t\tfree(list);\n+\tif (remoteheads) {\n+\t\t/* not up-to-date */\n+\t\tif (remoteheads->next) {\n+\t\t\tstruct commit_list *list = remoteheads;\n+\t\t\tcommit_list_insert(head_commit, &list);\n+\t\t\tcommon = get_octopus_merge_bases(list);\n+\t\t\tfree(list);\n+\t\t} else\n+\t\t\tcommon = get_merge_bases(head_commit, remoteheads->item);\n \t}\n \n \tupdate_ref(\"updating ORIG_HEAD\", \"ORIG_HEAD\",\n@@ -1542,31 +1543,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tfinish(head_commit, remoteheads, &commit->object.oid, msg.buf);\n \t\tremove_merge_branch_state(the_repository);\n \t\tgoto done;\n-\t} else if (!remoteheads->next && common->next)\n-\t\t;\n-\t\t/*\n-\t\t * We are not doing octopus and not fast-forward.  Need\n-\t\t * a real merge.\n-\t\t */\n-\telse if (!remoteheads->next && !common->next && option_commit) {\n-\t\t/*\n-\t\t * We are not doing octopus, not fast-forward, and have\n-\t\t * only one common.\n-\t\t */\n-\t\trefresh_cache(REFRESH_QUIET);\n-\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n-\t\t\t/* See if it is really trivial. */\n-\t\t\tgit_committer_info(IDENT_STRICT);\n-\t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n-\t\t\tif (!read_tree_trivial(&common->item->object.oid,\n-\t\t\t\t\t       &head_commit->object.oid,\n-\t\t\t\t\t       &remoteheads->item->object.oid)) {\n-\t\t\t\tret = merge_trivial(head_commit, remoteheads);\n-\t\t\t\tgoto done;\n-\t\t\t}\n-\t\t\tprintf(_(\"Nope.\\n\"));\n-\t\t}\n-\t} else {\n+\t} else if (remoteheads->next || (!common->next && !option_commit)) {\n \t\t/*\n \t\t * An octopus.  If we can reach all the remote we are up\n \t\t * to date.\n@@ -1592,6 +1569,24 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tfinish_up_to_date(_(\"Already up to date. Yeeah!\"));\n \t\t\tgoto done;\n \t\t}\n+\t} else if (!common->next) {\n+\t\t/*\n+\t\t * We are not doing octopus, not fast-forward, and have\n+\t\t * only one common.\n+\t\t */\n+\t\trefresh_cache(REFRESH_QUIET);\n+\t\tif (allow_trivial && fast_forward != FF_ONLY) {\n+\t\t\t/* See if it is really trivial. */\n+\t\t\tgit_committer_info(IDENT_STRICT);\n+\t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n+\t\t\tif (!read_tree_trivial(&common->item->object.oid,\n+\t\t\t\t\t       &head_commit->object.oid,\n+\t\t\t\t\t       &remoteheads->item->object.oid)) {\n+\t\t\t\tret = merge_trivial(head_commit, remoteheads);\n+\t\t\t\tgoto done;\n+\t\t\t}\n+\t\t\tprintf(_(\"Nope.\\n\"));\n+\t\t}\n \t}\n \n \tif (fast_forward == FF_ONLY)\n@@ -1685,9 +1680,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\tuse_strategies[0]->name);\n \t\tret = 2;\n \t\tgoto done;\n-\t} else if (best_strategy == wt_strategy)\n-\t\t; /* We already have its result in the working tree. */\n-\telse {\n+\t} else if (best_strategy != wt_strategy) {\n+\t\t/* We do not have its result in the working tree. */\n \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n \t\trestore_state(&head_commit->object.oid, &stash);\n \t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n\nbase-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500\n-- \ngitgitgadget\n"}]}