[v3] Simplify merge logic

Commit Message

Seija K Nov. 17, 2020, 10:06 p.m. UTC

From: Seija K <[e-mail]>

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. <[e-mail]>
---
    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(-)


base-commit: e31aba42fb12bdeb0f850829e008e1e3f43af500

Patch

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"),