From: "Seija K. via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Seija K <pi1024e@github.com>
Subject: [PATCH v2] Simplify merge logic
Date: Tue, 17 Nov 2020 21:54:32 +0000 [thread overview]
Message-ID: <pull.911.v2.git.git.1605650072908.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.911.git.git.1604871456715.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
next prev parent reply other threads:[~2020-11-17 21:55 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-08 21:37 [PATCH] Simplified merge logic Seija K. via GitGitGadget
2020-11-09 23:17 ` Junio C Hamano
2020-11-17 21:54 ` Seija K. via GitGitGadget [this message]
2020-11-17 22:06 ` [PATCH v3] Simplify " Seija K. via GitGitGadget
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.v2.git.git.1605650072908.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=git@vger.kernel.org \
--cc=pi1024e@github.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public 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).