{"thread":{"id":"51439","subject":"[PATCH v1] builtin/merge.c - cleanup of code in for-cycle that tests strategies","startedAt":"2019-07-07T00:01:03Z","lastAt":"2019-07-08T20:52:32Z","messageCount":4,"participants":["Edmundo Carmona Antoranz","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"378661","messageId":"20190707000052.26057-1-eantoranz@gmail.com","threadId":"51439","inReplyTo":null,"subject":"[PATCH v1] builtin/merge.c - cleanup of code in for-cycle that tests strategies","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-07T00:00:52Z","receivedAt":"2019-07-07T00:01:03Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"Previous code was a little convoluted to follow logic\nNew code is shorter and logic is easier to follow\n\n- Easier to see what happens when merge is successful\n\tand how --no-commit affects result\n- Simpler to see that for-cycle will stop when merge_was_ok is set\n- Easier to spot what logic will run through best_strategy\n- Easier to see that in case of ret being 2, cycle will continue\n- Keep a single break case (when automerge succedes and a revision will\n be created)\n- Put together closing actions when automerge succedes if a revision\n will be created\n\nSigned-off-by: Edmundo Carmona Antoranz \n---\n builtin/merge.c | 51 ++++++++++++++++++-------------------------------\n 1 file changed, 19 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 6e99aead46..94f2713bea 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1586,7 +1586,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t save_state(&stash))\n \t\toidclr(&stash);\n \n-\tfor (i = 0; i < use_strategies_nr; i++) {\n+\tfor (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n \t\tint ret;\n \t\tif (i) {\n \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n@@ -1604,40 +1604,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = try_merge_strategy(use_strategies[i]->name,\n \t\t\t\t\t common, remoteheads,\n \t\t\t\t\t head_commit);\n-\t\tif (!option_commit && !ret) {\n-\t\t\tmerge_was_ok = 1;\n-\t\t\t/*\n-\t\t\t * This is necessary here just to avoid writing\n-\t\t\t * the tree, but later we will *not* exit with\n-\t\t\t * status code 1 because merge_was_ok is set.\n-\t\t\t */\n-\t\t\tret = 1;\n-\t\t}\n-\n-\t\tif (ret) {\n-\t\t\t/*\n-\t\t\t * The backend exits with 1 when conflicts are\n-\t\t\t * left to be resolved, with 2 when it does not\n-\t\t\t * handle the given merge at all.\n-\t\t\t */\n-\t\t\tif (ret == 1) {\n-\t\t\t\tint cnt = evaluate_result();\n-\n-\t\t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n-\t\t\t\t\tbest_strategy = use_strategies[i]->name;\n-\t\t\t\t\tbest_cnt = cnt;\n+\t\t/*\n+\t\t * The backend exits with 1 when conflicts are\n+\t\t * left to be resolved, with 2 when it does not\n+\t\t * handle the given merge at all.\n+\t\t */\n+\t\tif (ret < 2) {\n+\t\t\tif (!ret) {\n+\t\t\t\tif (option_commit) {\n+\t\t\t\t\t/* Automerge succeeded. */\n+\t\t\t\t\tautomerge_was_ok = 1;\n+\t\t\t\t\tbreak;\n \t\t\t\t}\n+\t\t\t\tmerge_was_ok = 1;\n+\t\t\t}\n+\t\t\tint cnt = evaluate_result();\n+\t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n+\t\t\t\tbest_strategy = use_strategies[i]->name;\n+\t\t\t\tbest_cnt = cnt;\n \t\t\t}\n-\t\t\tif (merge_was_ok)\n-\t\t\t\tbreak;\n-\t\t\telse\n-\t\t\t\tcontinue;\n \t\t}\n-\n-\t\t/* Automerge succeeded. */\n-\t\twrite_tree_trivial(&result_tree);\n-\t\tautomerge_was_ok = 1;\n-\t\tbreak;\n \t}\n \n \t/*\n@@ -1645,6 +1631,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * auto resolved the merge cleanly.\n \t */\n \tif (automerge_was_ok) {\n+\t\twrite_tree_trivial(&result_tree);\n \t\tret = finish_automerge(head_commit, head_subsumed,\n \t\t\t\t common, remoteheads,\n \t\t\t\t &result_tree, wt_strategy);\n-- \n2.20.1\n\n"},{"id":"378662","messageId":"CAOc6etYGA_rHhrDgMbf4x_GxdWwgWy7W2Kqvv6PsMMHFP0U_vA@mail.gmail.com","threadId":"51439","inReplyTo":"20190707000052.26057-1-eantoranz@gmail.com","subject":"Re: [PATCH v1] builtin/merge.c - cleanup of code in for-cycle that tests strategies","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-07T03:15:01Z","receivedAt":"2019-07-07T03:15:15Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Sat, Jul 6, 2019 at 6:00 PM Edmundo Carmona Antoranz\n wrote:\n> @@ -1645,6 +1631,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> * auto resolved the merge cleanly.\n> */\n> if (automerge_was_ok) {\n> + write_tree_trivial(&result_tree);\n> ret = finish_automerge(head_commit, head_subsumed,\n> common, remoteheads,\n> &result_tree, wt_strategy);\n\nI realized later that the call to write_tree_trivial could be included\nin finish_automerge. Will include this change on v2 of the patch (if\nthere's a v2, depending on feedback).\n"},{"id":"378705","messageId":"xmqqh87wxoza.fsf@gitster-ct.c.googlers.com","threadId":"51439","inReplyTo":"20190707000052.26057-1-eantoranz@gmail.com","subject":"Re: [PATCH v1] builtin/merge.c - cleanup of code in for-cycle that tests strategies","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-07-08T20:15:05Z","receivedAt":"2019-07-08T20:15:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Edmundo Carmona Antoranz writes:\n\n> Previous code was a little convoluted to follow logic\n> New code is shorter and logic is easier to follow\n\nIn the body of the proposed commit log message, please finish each\nsentence with a full-stop.\n\n> - Easier to see what happens when merge is successful\n> \tand how --no-commit affects result\n> - Simpler to see that for-cycle will stop when merge_was_ok is set\n> - Easier to spot what logic will run through best_strategy\n> - Easier to see that in case of ret being 2, cycle will continue\n\nThese bullets are all subjective, and do not add any value to what\nyou already said in the second sentence.\n\n> - Keep a single break case (when automerge succedes and a revision will\n> be created)\n> - Put together closing actions when automerge succedes if a revision\n> will be created\n\nThese are facts that readers can see for themselves and agree with.\n\nSomething like...\n\n\tThe cmd_merge() function has a loop that tries different\n\tmerge strategies in turn, and stops when a strategy gets a\n\tclean merge, while keeping the \"best\" conflicted merge so\n\tfar.\n\n\tMake the loop easier to follow by moving the code around,\n\tensuring that there is only one \"break\" in the loop where\n\tan automerge succeeds. Also group the actions that are\n\tperformed after an automerge succeeds together to a single\n\tlocation, outside and after the loop.\n\nperhaps?\n\n> Signed-off-by: Edmundo Carmona Antoranz \n> ---\n> builtin/merge.c | 51 ++++++++++++++++++-------------------------------\n> 1 file changed, 19 insertions(+), 32 deletions(-)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 6e99aead46..94f2713bea 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1586,7 +1586,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \t save_state(&stash))\n> \t\toidclr(&stash);\n> \n> -\tfor (i = 0; i < use_strategies_nr; i++) {\n> +\tfor (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n> \t\tint ret;\n> \t\tif (i) {\n> \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n> @@ -1604,40 +1604,26 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \t\tret = try_merge_strategy(use_strategies[i]->name,\n> \t\t\t\t\t common, remoteheads,\n> \t\t\t\t\t head_commit);\n> -\t\tif (!option_commit && !ret) {\n> -\t\t\tmerge_was_ok = 1;\n> -\t\t\t/*\n> -\t\t\t * This is necessary here just to avoid writing\n> -\t\t\t * the tree, but later we will *not* exit with\n> -\t\t\t * status code 1 because merge_was_ok is set.\n> -\t\t\t */\n> -\t\t\tret = 1;\n> -\t\t}\n> -\n> -\t\tif (ret) {\n> -\t\t\t/*\n> -\t\t\t * The backend exits with 1 when conflicts are\n> -\t\t\t * left to be resolved, with 2 when it does not\n> -\t\t\t * handle the given merge at all.\n> -\t\t\t */\n> -\t\t\tif (ret == 1) {\n> -\t\t\t\tint cnt = evaluate_result();\n> -\n> -\t\t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n> -\t\t\t\t\tbest_strategy = use_strategies[i]->name;\n> -\t\t\t\t\tbest_cnt = cnt;\n> +\t\t/*\n> +\t\t * The backend exits with 1 when conflicts are\n> +\t\t * left to be resolved, with 2 when it does not\n> +\t\t * handle the given merge at all.\n> +\t\t */\n> +\t\tif (ret < 2) {\n> +\t\t\tif (!ret) {\n> +\t\t\t\tif (option_commit) {\n> +\t\t\t\t\t/* Automerge succeeded. */\n> +\t\t\t\t\tautomerge_was_ok = 1;\n> +\t\t\t\t\tbreak;\n> \t\t\t\t}\n> +\t\t\t\tmerge_was_ok = 1;\n> +\t\t\t}\n> +\t\t\tint cnt = evaluate_result();\n\nThis introduces -Wdeclaration-after-statement, doesn't it?\nPerhaps just declare the variable at the top of the for loop, next\nto where the local 'ret' is declared?\n\nOther than this single glitch, I think the code with this patch does\nbecome easier to follow.\n\n> +\t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n> +\t\t\t\tbest_strategy = use_strategies[i]->name;\n> +\t\t\t\tbest_cnt = cnt;\n> \t\t\t}\n> -\t\t\tif (merge_was_ok)\n> -\t\t\t\tbreak;\n> -\t\t\telse\n> -\t\t\t\tcontinue;\n> \t\t}\n> -\n> -\t\t/* Automerge succeeded. */\n> -\t\twrite_tree_trivial(&result_tree);\n> -\t\tautomerge_was_ok = 1;\n> -\t\tbreak;\n> \t}\n> \n> \t/*\n> @@ -1645,6 +1631,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n> \t * auto resolved the merge cleanly.\n> \t */\n> \tif (automerge_was_ok) {\n> +\t\twrite_tree_trivial(&result_tree);\n> \t\tret = finish_automerge(head_commit, head_subsumed,\n> \t\t\t\t common, remoteheads,\n> \t\t\t\t &result_tree, wt_strategy);\n"},{"id":"378708","messageId":"CAOc6etbwHAAFSM-GwuD8BaC76EQNv+pQ5TgNCWN65DhNfV3KEw@mail.gmail.com","threadId":"51439","inReplyTo":"xmqqh87wxoza.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v1] builtin/merge.c - cleanup of code in for-cycle that tests strategies","fromName":"Edmundo Carmona Antoranz","fromEmail":"eantoranz@gmail.com","sentAt":"2019-07-08T20:52:19Z","receivedAt":"2019-07-08T20:52:32Z","isPatch":true,"sender":{"key":"eantoranz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1491018?v=4"},"body":"On Mon, Jul 8, 2019 at 2:15 PM Junio C Hamano wrote:\n> In the body of the proposed commit log message, please finish each\n> sentence with a full-stop.\n\nOk\n\n>\n> These bullets are all subjective, and do not add any value to what\n> you already said in the second sentence.\n\nOk\n\n\n>\n> These are facts that readers can see for themselves and agree with.\n>\n> Something like...\n>\n> The cmd_merge() function has a loop that tries different\n> merge strategies in turn, and stops when a strategy gets a\n> clean merge, while keeping the \"best\" conflicted merge so\n> far.\n>\n> Make the loop easier to follow by moving the code around,\n> ensuring that there is only one \"break\" in the loop where\n> an automerge succeeds. Also group the actions that are\n> performed after an automerge succeeds together to a single\n> location, outside and after the loop.\n>\n> perhaps?\n\nI like it.\n\n> > + int cnt = evaluate_result();\n> This introduces -Wdeclaration-after-statement, doesn't it?\n\nIt does! Let me take care of it. (Didn't know about DEVELOPER\nenvironment variable up until... 5 minutes ago?)\n\n> Perhaps just declare the variable at the top of the for loop, next\n> to where the local 'ret' is declared?\n\nYep, make sense\n\n>\n> Other than this single glitch, I think the code with this patch does\n> become easier to follow.\n\nGreat!\n\nLet me come back with v2 during the day.\n\nThanks!\n"}]}