Simplified merge logic

Commit Message

Seija K Nov. 8, 2020, 9:37 p.m. UTC

From: pi1024e <[e-mail]>

commit: Avoid extraneous boolean checking by simplifying the if statements.
Signed-off-by: Seija <[e-mail]>
---
    Simplified merge logic
    
    The logic for the merging is somewhat confusing. So I simplified it to
    be equivalent. I tested all my changes to ensure in practice they work.
    
    The first thing I did was test out which branch would occur for every
    possible value of 
    
    remoteheads->nextcommon->nextoption_commitBranchTTTATTFATFTATFFAFTTCFTFC
    FFTBFFFAUsing this truth table, I was able to deduce that if the second
    branch ran, the if statement for the first branch was false. Taking the
    inverse, it was then found many of the checks were redundant, so the if
    statement was rewritten to have each branch run under the same exact
    conditions, except each value is evaluated as little as possible.
    
    I hope you can approve of this and show me how to send it.
    
    Thank you.

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

 builtin/merge.c | 85 ++++++++++++++++++++++---------------------------
 1 file changed, 38 insertions(+), 47 deletions(-)


base-commit: 7f7ebe054af6d831b999d6c2241b9227c4e4e08d

Comments

Junio C Hamano Nov. 9, 2020, 11:17 p.m. UTC | #1

"Seija K. via GitGitGadget" <[e-mail]> writes:

> From: pi1024e <[e-mail]>
>
> commit: Avoid extraneous boolean checking by simplifying the if statements.
> Signed-off-by: Seija <[e-mail]>
> ---

Meh.

Admittedly, readability is somewhat subjective, but a rewrite like

        if (condition)                  if (!condition)
                do_when_true();                 do_when_false();
        else                       ==>  else
                do_when_false();                do_when_true();

while it may not be incorrect per-se needs more than a subjective "I
think this is more readable" to justify the code churn.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4c133402a6..9664da6031 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -853,9 +853,8 @@ 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);
>  	}

This may reduce the number of lines, but personally I find that

	if (are we editing?) {
		if (run editor---did we fail?)
			abort();
	}

is much easier to read.

And much more importantly, it would be much easier to extend later
what hwppens when we decide to edit, than the new code proposed by
this patch.

> @@ -1213,7 +1212,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;

Likewise.  If somebody _must_ touch this function to gain commit
count without making the code harder to maintain, it may be an
option to use "goto leave;" here and then create a "leave:" label
before the other "return"---at least that may be worth considering,
but not this---when everybody else in the function wants to maintain
that the value in this variable is what is returned from the
function.

> @@ -1459,13 +1458,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  			fast_forward = FF_NO;
>  	}
>  
> -	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);
>  	}

Likewise.

	if (do we have to choose strategies ourselves?) {
		... depending on the case, choose strategy ...
	}

is much easier to reason about than

	if (do we have to choose strategies ourselves?, oh by the
    	    way, don't forget that already up-to-date case we do not
	    have to choose) {
		... the remainder ...
	}

and extend when we need to add something to do in the up-to-date
case as long as the end-user did not specify which strategy to use
in the future.  The reduced line count alone is not a good yardstick
to use when talking about code restructuring for readability and
maintainability.

Thanks.

Patch

diff --git a/builtin/merge.c b/builtin/merge.c
index 4c133402a6..9664da6031 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -853,9 +853,8 @@  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(),
@@ -1213,7 +1212,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
@@ -1459,13 +1458,12 @@  int cmd_merge(int argc, const char **argv, const char *prefix)
 			fast_forward = FF_NO;
 	}
 
-	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);
 	}
 
 	for (i = 0; i < use_strategies_nr; i++) {
@@ -1475,15 +1473,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 +1540,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 +1566,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 +1677,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"),