[PATCH v2] Simplify merge logic
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(-)
Show changes to builtin/merge.c +38 −44
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
