git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Seija K. via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Seija K <pi1024e@github.com>
Subject: [PATCH v3] Simplify merge logic
Date: Tue, 17 Nov 2020 22:06:01 +0000	[thread overview]
Message-ID: <pull.911.v3.git.git.1605650762205.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.911.v2.git.git.1605650072908.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 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@github.com>
---
    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

      reply	other threads:[~2020-11-17 22:07 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-08 21:37 [PATCH] Simplified " Seija K. via GitGitGadget
2020-11-09 23:17 ` Junio C Hamano
2020-11-17 21:54 ` [PATCH v2] Simplify " Seija K. via GitGitGadget
2020-11-17 22:06   ` Seija K. via GitGitGadget [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=pull.911.v3.git.git.1605650762205.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=pi1024e@github.com \
    --subject='Re: [PATCH v3] Simplify merge logic' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).