[PATCH v2] Simplify merge logic

["Seija K. via GitGitGadget" <gitgitgadget@gmail.com>]

From: Seija K <pi1024e@github.com>

commit: Avoid extraneous boolean checking by simplifying the if statements.
This is so that the code can be clearer and avoid duplicate boolean checks.

Changes since v1: Undid if statement combos as suggested by Junio C Hamano.

The current logic for the 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@github.com>
---
    Simplify merge logic
    
    This is so that the code can be clearer and avoid duplicate boolean
    checks.
    
    Changes since v1: Undid if statement combos as suggested by Junio C
    Hamano.
    
    The current logic for the 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!
    
    Signed-off-by: Seija K. pi1024e@github.com [pi1024e@github.com]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-911%2Fpi1024e%2Fmerge-cleanup-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-911/pi1024e/merge-cleanup-v2
Pull-Request: https://github.com/git/git/pull/911

Range-diff vs v1:

 1:  9b333c9872 ! 1:  dd1c4dd678 Simplified merge logic
     @@
       ## Metadata ##
     -Author: pi1024e <pi1024e@github.com>
     +Author: Seija K <pi1024e@github.com>
      
       ## Commit message ##
     -    Simplified merge logic
     +    Simplify merge logic
      
          commit: Avoid extraneous boolean checking by simplifying the if statements.
     -    Signed-off-by: Seija <pi1024e@github.com>
     +    This is so that the code can be clearer and avoid duplicate boolean checks.
     +
     +    Changes since v1: Undid if statement combos as suggested by Junio C Hamano.
     +
     +    The current logic for the 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@github.com>
      
       ## builtin/merge.c ##
     -@@ builtin/merge.c: static void prepare_to_commit(struct commit_list *remoteheads)
     - 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
     - 			    git_path_merge_msg(the_repository), "merge", NULL))
     - 		abort_commit(remoteheads, NULL);
     --	if (0 < option_edit) {
     --		if (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))
     --			abort_commit(remoteheads, NULL);
     -+	if (0 < option_edit && launch_editor(git_path_merge_msg(the_repository), NULL, NULL)) {
     -+		abort_commit(remoteheads, NULL);
     - 	}
     - 
     - 	if (!no_verify && run_commit_hook(0 < option_edit, get_index_file(),
      @@ builtin/merge.c: static int merging_a_throwaway_tag(struct commit *commit)
       	if (!merge_remote_util(commit) ||
       	    !merge_remote_util(commit)->obj ||
     @@ builtin/merge.c: static int merging_a_throwaway_tag(struct commit *commit)
       	/*
       	 * Now we know we are merging a tag object.  Are we downstream
      @@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)
     - 			fast_forward = FF_NO;
       	}
       
     --	if (!use_strategies) {
     + 	if (!use_strategies) {
      -		if (!remoteheads)
      -			; /* already up-to-date */
      -		else if (!remoteheads->next)
      -			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
      -		else
     -+	if (!use_strategies && remoteheads) {
     -+		/* not up-to-date */
     -+		if (remoteheads->next)
     - 			add_strategies(pull_octopus, DEFAULT_OCTOPUS);
     -+		else
     -+			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
     +-			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++) {


 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