{"thread":{"id":"58339","subject":"[PATCH 0/2] Miscellaneous merge fixes","startedAt":"2022-08-21T04:38:38Z","lastAt":"2022-08-24T21:09:12Z","messageCount":15,"participants":["Elijah Newren via GitGitGadget","Eric Sunshine","Elijah Newren","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"461679","messageId":"pull.1331.git.1661056709.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":null,"subject":"[PATCH 0/2] Miscellaneous merge fixes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-21T04:38:27Z","receivedAt":"2022-08-21T04:38:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"While working on other things, I noticed a few miscellaneous issues in\nbuiltin/merge.c. Here's a few small fixes to address them.\n\nElijah Newren (2):\n merge: only apply autostash when appropriate\n merge: avoid searching for strategies with fewer than 0 conflicts\n\n builtin/merge.c | 19 ++++++++++++++-----\n t/t7600-merge.sh | 9 +++++++++\n 2 files changed, 23 insertions(+), 5 deletions(-)\n\n\nbase-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1331%2Fnewren%2Fmisc-merge-fixes-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1331/newren/misc-merge-fixes-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1331\n-- \ngitgitgadget\n"},{"id":"461680","messageId":"5657a05e7635ecadbb8d2e41ad97fe19f3633fdd.1661056709.git.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":"pull.1331.git.1661056709.gitgitgadget@gmail.com","subject":"[PATCH 2/2] merge: avoid searching for strategies with fewer than 0 conflicts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-21T04:38:29Z","receivedAt":"2022-08-21T04:38:40Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nbuiltin/merge.c has a loop over the specified strategies, where if\nthey all fail with conflicts, it picks the one with the least number\nof conflicts.\n\nIn the codepath that finds a successful merge, if an automatic commit\nwas wanted, the code breaks out of the above loop, which makes sense.\nHowever, if the user requested there be no automatic commit, the loop\nwould continue looking for a \"better\" strategy. Since it had just\nfound a strategy with 0 conflicts though, and it is not possible to\nhave fewer than 0 conflicts, the continuing search is guaranteed to be\nfutile.\n\nWhile searching additional strategies won't cause problems other than\nwasting energy, it is wasteful. Avoid searching for other strategies\nwith fewer than 0 conflicts.\n\nSigned-off-by: Elijah Newren \n---\n builtin/merge.c | 14 ++++++++++----\n 1 file changed, 10 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b4253710d19..f04100ce0da 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1718,12 +1718,18 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tif (ret < 2) {\n \t\t\tif (!ret) {\n-\t\t\t\tif (option_commit) {\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\telse\n+\t\t\t\t\t/* Merge good, but let user commit */\n+\t\t\t\t\tmerge_was_ok = 1;\n+\t\t\t\t/*\n+\t\t\t\t * This strategy worked; no point in trying\n+\t\t\t\t * another.\n+\t\t\t\t */\n+\t\t\t\tbest_strategy = wt_strategy;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t\tcnt = (use_strategies_nr > 1) ? evaluate_result() : 0;\n \t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n-- \ngitgitgadget\n"},{"id":"461681","messageId":"92840bf63782ca302e9d48a12faf7d4f5b3cc2d5.1661056709.git.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":"pull.1331.git.1661056709.gitgitgadget@gmail.com","subject":"[PATCH 1/2] merge: only apply autostash when appropriate","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-21T04:38:28Z","receivedAt":"2022-08-21T04:38:41Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf a merge failed and we are leaving conflicts in the working directory\nfor the user to resolve, we should not attempt to apply any autostash.\n\nFurther, if we fail to apply the autostash (because either the merge\nfailed, or the user requested --no-commit), then we should instruct the\nuser how to apply it later.\n\nAdd a testcase verifying we have corrected this behavior.\n\nSigned-off-by: Elijah Newren \n---\n builtin/merge.c | 5 ++++-\n t/t7600-merge.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f7c92c0e64f..b4253710d19 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -503,7 +503,8 @@ static void finish(struct commit *head_commit,\n \t/* Run a post-merge hook */\n \trun_hooks_l(\"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n-\tapply_autostash(git_path_merge_autostash(the_repository));\n+\tif (new_head)\n+\t\tapply_autostash(git_path_merge_autostash(the_repository));\n \tstrbuf_release(&reflog_message);\n }\n \n@@ -1781,6 +1782,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\"stopped before committing as requested\\n\"));\n \telse\n \t\tret = suggest_conflicts();\n+\tif (autostash)\n+\t\tprintf(_(\"When finished, apply stashed changes with `git stash pop`\\n\"));\n \n done:\n \tif (!automerge_was_ok) {\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex f0f6fda150b..6a39bc72122 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -255,6 +255,15 @@ test_expect_success 'merge --squash c3 with c7' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'merge --squash --autostash conflict does not attempt to play autostash' '\n+\tgit reset --hard c3 &&\n+\t>unrelated &&\n+\tgit add unrelated &&\n+\ttest_must_fail git merge --squash c7 --autostash >out 2>err &&\n+\t! grep \"Applying autostash resulted in conflicts.\" err &&\n+\tgrep \"When finished, apply stashed changes with \\`git stash pop\\`\" out\n+'\n+\n test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '\n \tgit config commit.cleanup scissors &&\n \tgit reset --hard c3 &&\n-- \ngitgitgadget\n\n"},{"id":"461682","messageId":"CAPig+cTMxj14gYGXgE3LAPFQRdDYXiH=Fd_iCwzoJp9uS1jCzg@mail.gmail.com","threadId":"58339","inReplyTo":"92840bf63782ca302e9d48a12faf7d4f5b3cc2d5.1661056709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 1/2] merge: only apply autostash when appropriate","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2022-08-21T04:52:53Z","receivedAt":"2022-08-21T04:53:08Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Aug 21, 2022 at 12:41 AM Elijah Newren via GitGitGadget\n wrote:\n> If a merge failed and we are leaving conflicts in the working directory\n> for the user to resolve, we should not attempt to apply any autostash.\n>\n> Further, if we fail to apply the autostash (because either the merge\n> failed, or the user requested --no-commit), then we should instruct the\n> user how to apply it later.\n>\n> Add a testcase verifying we have corrected this behavior.\n>\n> Signed-off-by: Elijah Newren \n> ---\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> @@ -255,6 +255,15 @@ test_expect_success 'merge --squash c3 with c7' '\n> +test_expect_success 'merge --squash --autostash conflict does not attempt to play autostash' '\n\nDid you mean s/play/apply/ ?\n"},{"id":"461683","messageId":"CABPp-BGYbdjXNXPp9rW02Ay72CWVkHKdpSDCs8V9aQA8Tu68Gg@mail.gmail.com","threadId":"58339","inReplyTo":"CAPig+cTMxj14gYGXgE3LAPFQRdDYXiH=Fd_iCwzoJp9uS1jCzg@mail.gmail.com","subject":"Re: [PATCH 1/2] merge: only apply autostash when appropriate","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-21T05:18:43Z","receivedAt":"2022-08-21T05:18:58Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Aug 20, 2022 at 9:53 PM Eric Sunshine wrote:\n>\n> On Sun, Aug 21, 2022 at 12:41 AM Elijah Newren via GitGitGadget\n> wrote:\n> > If a merge failed and we are leaving conflicts in the working directory\n> > for the user to resolve, we should not attempt to apply any autostash.\n> >\n> > Further, if we fail to apply the autostash (because either the merge\n> > failed, or the user requested --no-commit), then we should instruct the\n> > user how to apply it later.\n> >\n> > Add a testcase verifying we have corrected this behavior.\n> >\n> > Signed-off-by: Elijah Newren \n> > ---\n> > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> > @@ -255,6 +255,15 @@ test_expect_success 'merge --squash c3 with c7' '\n> > +test_expect_success 'merge --squash --autostash conflict does not attempt to play autostash' '\n>\n> Did you mean s/play/apply/ ?\n\nYep; will fix.\n"},{"id":"461739","messageId":"xmqqwnb1fp5n.fsf@gitster.g","threadId":"58339","inReplyTo":"5657a05e7635ecadbb8d2e41ad97fe19f3633fdd.1661056709.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 2/2] merge: avoid searching for strategies with fewer than 0 conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-21T18:05:24Z","receivedAt":"2022-08-21T18:05:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> builtin/merge.c has a loop over the specified strategies, where if\n> they all fail with conflicts, it picks the one with the least number\n> of conflicts.\n>\n> In the codepath that finds a successful merge, if an automatic commit\n> was wanted, the code breaks out of the above loop, which makes sense.\n> However, if the user requested there be no automatic commit, the loop\n> would continue looking for a \"better\" strategy. Since it had just\n> found a strategy with 0 conflicts though, and it is not possible to\n> have fewer than 0 conflicts, the continuing search is guaranteed to be\n> futile.\n\nLet's imagine \"git merge -s ours -s ort X\", both of which resolve\nthe merge cleanly but differently.\n\nThe \"best_cnt\" thing tries to find the last strategy with the lowest\nscore from evaluate_result(), so strictly speaking I think this\nchanges the behaviour. Am I mistaken?\n\nWhen two strategies 'ours' and 'ort' resolved a given merge cleanly\n(but differently), we would have taken the result from 'ort' in the\noriginal code, but now we stop at seeing that 'ours' resolved the\nmerge cleanly and use its result.\n\n> \t\t\tcnt = (use_strategies_nr > 1) ? evaluate_result() : 0;\n> \t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n\nWhile I think \"we see one clean merge, so it is OK to quit\" is more\nintuitive than the current behaviour, we need to update this\ncomparison to require the new candidate to have strictly better\nresult to take over the tentative best from the current candidate.\nThat will make things consistent with this change and makes it\neasier to sell.\n\nAs the userbase of Git is so wide, it is very plausible that\nsomebody already invented and publisized that prepending \"-s ours\"\nbefore the real strategy they want to use as an idiom to say \"fall\nback to pretend merge cleanly succeeded but did not affect the tree\"\nin a script that automate rebuilding tentative integration branch to\ntest, or something. They can \"fix\" their \"idiom\" to append instead\nof prepend \"-s ours\", and that would arguably make the resulting\ncommand line more intuitive and easier to understand, but the fact\nthat whatever that was working for them to their liking suddenly\nchanges the behaviour is a regression we have to ask them to accept.\n\nI do not mind writing something like this\n\n \"git merge\" with multiple strategies chooses to leave the least\n number of conflicted paths as the result. Among multiple strategies\n that give the same number of conflicted paths, it used to use the\n last such strategy with the smallest number of conflicted paths.\n The command now prefers the first such strategy instead.\n\n If you have been using \"git merge -s ours -s another X\" as a way to\n say \"we prefer 'another' strategy to merge, but use 'ours' strategy\n to make progress if it does not\", you now have to swap the order of\n strategies you list on the command line.\n\nin the \"Backward Incompatibility Notes\" section of the release\nnotes.\n\nThanks.\n"},{"id":"461759","messageId":"CABPp-BHy-Xs0S1-9kL7X4mi6JNfd420-vpVsswXNuXsc5C7AtA@mail.gmail.com","threadId":"58339","inReplyTo":"xmqqwnb1fp5n.fsf@gitster.g","subject":"Re: [PATCH 2/2] merge: avoid searching for strategies with fewer than 0 conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-22T15:00:00Z","receivedAt":"2022-08-22T15:01:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sun, Aug 21, 2022 at 11:05 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > builtin/merge.c has a loop over the specified strategies, where if\n> > they all fail with conflicts, it picks the one with the least number\n> > of conflicts.\n> >\n> > In the codepath that finds a successful merge, if an automatic commit\n> > was wanted, the code breaks out of the above loop, which makes sense.\n> > However, if the user requested there be no automatic commit, the loop\n> > would continue looking for a \"better\" strategy. Since it had just\n> > found a strategy with 0 conflicts though, and it is not possible to\n> > have fewer than 0 conflicts, the continuing search is guaranteed to be\n> > futile.\n>\n> Let's imagine \"git merge -s ours -s ort X\", both of which resolve\n> the merge cleanly but differently.\n>\n> The \"best_cnt\" thing tries to find the last strategy with the lowest\n> score from evaluate_result(), so strictly speaking I think this\n> changes the behaviour. Am I mistaken?\n\nYes, you are.\n\nThough, to be fair, my commit message is wrong too.\n\nI'll explain below...\n\n> When two strategies 'ours' and 'ort' resolved a given merge cleanly\n> (but differently), we would have taken the result from 'ort' in the\n> original code, but now we stop at seeing that 'ours' resolved the\n> merge cleanly and use its result.\n>\n> > cnt = (use_strategies_nr > 1) ? evaluate_result() : 0;\n> > if (best_cnt <= 0 || cnt <= best_cnt) {\n\nNo, the original code would not have taken 'ort'. You have overlooked\nthe part of the code immediately above these two lines:\n\n if (ret < 2) {\n if (!ret) {\n if (option_commit) {\n /* Automerge succeeded. */\n automerge_was_ok = 1;\n break;\n }\n merge_was_ok = 1;\n }\n\nIn particular, the \"break\" is key. If the first strategy succeeds\n(and the user did not specify --no-commit), then the loop will hit\nthat break statement and bail out of the loop, preventing any other\nmerge strategy from being tried. In contrast, if the user did specify\n--no-commit and the previous strategy succeeded, then we will continue\nthe loop. That seems rather inconsistent, since --no-commit should\nnot affect the choice of strategy.\n\nHowever, I missed two things in my reading. You are correct that I\nmissed the \"<=\" as opposed to \"<\" when I wrote my commit message,\nthough that turns out to not matter much due to the second thing. The\nsecond thing I missed was part of the code at the beginning of the\nloop:\n\n for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n\nIn particular, that \"!merge_was_ok\" check means that the code would\nbail early whenever ret was 0, regardless of whether --no-commit was\npassed. So, my code turns out to not only leave behavior alone, but\nalso doesn't count as an optimization either -- it's simply a\nhalf-baked cleanup. If I also get rid of the now-redundant\n\"!merge_was_ok\" check in the for loop and fix my commit message, then\nI think it'd be a fully-baked code cleanup.\n\nI'll submit an update.\n"},{"id":"461775","messageId":"xmqqr118dzek.fsf@gitster.g","threadId":"58339","inReplyTo":"CABPp-BHy-Xs0S1-9kL7X4mi6JNfd420-vpVsswXNuXsc5C7AtA@mail.gmail.com","subject":"Re: [PATCH 2/2] merge: avoid searching for strategies with fewer than 0 conflicts","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-22T16:19:15Z","receivedAt":"2022-08-22T16:19:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> No, the original code would not have taken 'ort'. You have overlooked\n> the part of the code immediately above these two lines:\n>\n> if (ret < 2) {\n> if (!ret) {\n> if (option_commit) {\n> /* Automerge succeeded. */\n> automerge_was_ok = 1;\n> break;\n> }\n> merge_was_ok = 1;\n> }\n>\n> In particular, the \"break\" is key.\n\nI noticed that much but then would \"git merge --no-commit -s A -s B\"\nhave the issue?\n\n> ... In contrast, if the user did specify\n> --no-commit and the previous strategy succeeded, then we will continue\n> the loop. That seems rather inconsistent, since --no-commit should\n> not affect the choice of strategy.\n\nYeah, exactly.\n\n> However, I missed two things in my reading. You are correct that I\n> missed the \"<=\" as opposed to \"<\" when I wrote my commit message,\n> though that turns out to not matter much due to the second thing. The\n> second thing I missed was part of the code at the beginning of the\n> loop:\n>\n> for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n\nAhhhh, that explains it. We leave as soon as we find merge_was_ok\nso this patch is not necessary. There is nothing to fix. The\noriginal was fine as-is.\n\nThanks.\n\n\n"},{"id":"461824","messageId":"CABPp-BHH6isv7iRKee34c1AEJ8JT5iUX7i7j6s9CVbmYN0Ervw@mail.gmail.com","threadId":"58339","inReplyTo":"xmqqr118dzek.fsf@gitster.g","subject":"Re: [PATCH 2/2] merge: avoid searching for strategies with fewer than 0 conflicts","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-23T01:18:00Z","receivedAt":"2022-08-23T01:18:50Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 22, 2022 at 9:19 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n[...]\n> > ... In contrast, if the user did specify\n> > --no-commit and the previous strategy succeeded, then we will continue\n> > the loop. That seems rather inconsistent, since --no-commit should\n> > not affect the choice of strategy.\n>\n> Yeah, exactly.\n>\n> > However, I missed two things in my reading. You are correct that I\n> > missed the \"<=\" as opposed to \"<\" when I wrote my commit message,\n> > though that turns out to not matter much due to the second thing. The\n> > second thing I missed was part of the code at the beginning of the\n> > loop:\n> >\n> > for (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n>\n> Ahhhh, that explains it. We leave as soon as we find merge_was_ok\n> so this patch is not necessary. There is nothing to fix. The\n> original was fine as-is.\n\nRight, there was no bug or even optimization opportunity in the\noriginal, it was just confusing...as evidenced by the fact that we\nboth misread the code.\n\nThe fact that we both misread the code, though, suggests there is\nsomething to fix: code readability.\n"},{"id":"461825","messageId":"pull.1331.v2.git.1661222541.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":"pull.1331.git.1661056709.gitgitgadget@gmail.com","subject":"[PATCH v2 0/3] Miscellaneous merge fixes","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T02:42:18Z","receivedAt":"2022-08-23T02:42:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"While working on other things, I noticed a few miscellaneous issues in\nbuiltin/merge.c. Here's a few small fixes to address them.\n\nElijah Newren (3):\n merge: only apply autostash when appropriate\n merge: cleanup confusing logic for handling successful merges\n merge: small code readability improvement\n\n builtin/merge.c | 25 +++++++++++++++----------\n t/t7600-merge.sh | 9 +++++++++\n 2 files changed, 24 insertions(+), 10 deletions(-)\n\n\nbase-commit: 9bf691b78cf906751e65d65ba0c6ffdcd9a5a12c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1331%2Fnewren%2Fmisc-merge-fixes-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1331/newren/misc-merge-fixes-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1331\n\nRange-diff vs v1:\n\n 1: 92840bf6378 ! 1: 610b8d089db merge: only apply autostash when appropriate\n @@ t/t7600-merge.sh: test_expect_success 'merge --squash c3 with c7' '\n \ttest_cmp expect actual\n '\n \n -+test_expect_success 'merge --squash --autostash conflict does not attempt to play autostash' '\n ++test_expect_success 'merge --squash --autostash conflict does not attempt to apply autostash' '\n +\tgit reset --hard c3 &&\n +\t>unrelated &&\n +\tgit add unrelated &&\n 2: 5657a05e763 ! 2: 9817d4b19bc merge: avoid searching for strategies with fewer than 0 conflicts\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - merge: avoid searching for strategies with fewer than 0 conflicts\n + merge: cleanup confusing logic for handling successful merges\n \n - builtin/merge.c has a loop over the specified strategies, where if\n - they all fail with conflicts, it picks the one with the least number\n - of conflicts.\n + builtin/merge.c has a loop over the specified strategies, where if they\n + all fail with conflicts, it picks the one with the least number of\n + conflicts.\n \n In the codepath that finds a successful merge, if an automatic commit\n was wanted, the code breaks out of the above loop, which makes sense.\n However, if the user requested there be no automatic commit, the loop\n - would continue looking for a \"better\" strategy. Since it had just\n - found a strategy with 0 conflicts though, and it is not possible to\n - have fewer than 0 conflicts, the continuing search is guaranteed to be\n - futile.\n + would continue. That seems weird; --no-commit should not affect the\n + choice of merge strategy, but the code as written makes one think it\n + does. However, since the loop itself embeds \"!merge_was_ok\" as a\n + condition on continuing to loop, it actually would also exit early if\n + --no-commit was specified, it just exited from a different location.\n \n - While searching additional strategies won't cause problems other than\n - wasting energy, it is wasteful. Avoid searching for other strategies\n - with fewer than 0 conflicts.\n + Restructure the code slightly to make it clear that the loop will\n + immediately exit whenever we find a merge strategy that is successful.\n \n Signed-off-by: Elijah Newren \n \n ## builtin/merge.c ##\n +@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)\n + \tif (save_state(&stash))\n + \t\toidclr(&stash);\n + \n +-\tfor (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n ++\tfor (i = 0; i < use_strategies_nr; i++) {\n + \t\tint ret, cnt;\n + \t\tif (i) {\n + \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n @@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\tif (ret < 2) {\n \t\t\tif (!ret) {\n -\t\t\t\tif (option_commit) {\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\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\telse\n -+\t\t\t\t\t/* Merge good, but let user commit */\n -+\t\t\t\t\tmerge_was_ok = 1;\n +\t\t\t\t/*\n +\t\t\t\t * This strategy worked; no point in trying\n +\t\t\t\t * another.\n +\t\t\t\t */\n -+\t\t\t\tbest_strategy = wt_strategy;\n + \t\t\t\tmerge_was_ok = 1;\n ++\t\t\t\tbest_strategy = use_strategies[i]->name;\n +\t\t\t\tbreak;\n \t\t\t}\n \t\t\tcnt = (use_strategies_nr > 1) ? evaluate_result() : 0;\n \t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n +@@ builtin/merge.c: int cmd_merge(int argc, const char **argv, const char *prefix)\n + \t * If we have a resulting tree, that means the strategy module\n + \t * auto resolved the merge cleanly.\n + \t */\n +-\tif (automerge_was_ok) {\n ++\tif (merge_was_ok && option_commit) {\n ++\t\tautomerge_was_ok = 1;\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 -: ----------- > 3: 88173eba0b9 merge: small code readability improvement\n\n-- \ngitgitgadget\n"},{"id":"461826","messageId":"610b8d089db97bf1a37dbf230f51ecafe3086254.1661222541.git.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":"pull.1331.v2.git.1661222541.gitgitgadget@gmail.com","subject":"[PATCH v2 1/3] merge: only apply autostash when appropriate","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T02:42:19Z","receivedAt":"2022-08-23T02:42:30Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIf a merge failed and we are leaving conflicts in the working directory\nfor the user to resolve, we should not attempt to apply any autostash.\n\nFurther, if we fail to apply the autostash (because either the merge\nfailed, or the user requested --no-commit), then we should instruct the\nuser how to apply it later.\n\nAdd a testcase verifying we have corrected this behavior.\n\nSigned-off-by: Elijah Newren \n---\n builtin/merge.c | 5 ++++-\n t/t7600-merge.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex f7c92c0e64f..b4253710d19 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -503,7 +503,8 @@ static void finish(struct commit *head_commit,\n \t/* Run a post-merge hook */\n \trun_hooks_l(\"post-merge\", squash ? \"1\" : \"0\", NULL);\n \n-\tapply_autostash(git_path_merge_autostash(the_repository));\n+\tif (new_head)\n+\t\tapply_autostash(git_path_merge_autostash(the_repository));\n \tstrbuf_release(&reflog_message);\n }\n \n@@ -1781,6 +1782,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\"stopped before committing as requested\\n\"));\n \telse\n \t\tret = suggest_conflicts();\n+\tif (autostash)\n+\t\tprintf(_(\"When finished, apply stashed changes with `git stash pop`\\n\"));\n \n done:\n \tif (!automerge_was_ok) {\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex f0f6fda150b..7c3f6ed9943 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -255,6 +255,15 @@ test_expect_success 'merge --squash c3 with c7' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'merge --squash --autostash conflict does not attempt to apply autostash' '\n+\tgit reset --hard c3 &&\n+\t>unrelated &&\n+\tgit add unrelated &&\n+\ttest_must_fail git merge --squash c7 --autostash >out 2>err &&\n+\t! grep \"Applying autostash resulted in conflicts.\" err &&\n+\tgrep \"When finished, apply stashed changes with \\`git stash pop\\`\" out\n+'\n+\n test_expect_success 'merge c3 with c7 with commit.cleanup = scissors' '\n \tgit config commit.cleanup scissors &&\n \tgit reset --hard c3 &&\n-- \ngitgitgadget\n\n"},{"id":"461827","messageId":"9817d4b19bc21e1b1581af24c7d89fe33b61638d.1661222541.git.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":"pull.1331.v2.git.1661222541.gitgitgadget@gmail.com","subject":"[PATCH v2 2/3] merge: cleanup confusing logic for handling successful merges","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T02:42:20Z","receivedAt":"2022-08-23T02:42:31Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nbuiltin/merge.c has a loop over the specified strategies, where if they\nall fail with conflicts, it picks the one with the least number of\nconflicts.\n\nIn the codepath that finds a successful merge, if an automatic commit\nwas wanted, the code breaks out of the above loop, which makes sense.\nHowever, if the user requested there be no automatic commit, the loop\nwould continue. That seems weird; --no-commit should not affect the\nchoice of merge strategy, but the code as written makes one think it\ndoes. However, since the loop itself embeds \"!merge_was_ok\" as a\ncondition on continuing to loop, it actually would also exit early if\n--no-commit was specified, it just exited from a different location.\n\nRestructure the code slightly to make it clear that the loop will\nimmediately exit whenever we find a merge strategy that is successful.\n\nSigned-off-by: Elijah Newren \n---\n builtin/merge.c | 16 +++++++++-------\n 1 file changed, 9 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b4253710d19..abf0567b20f 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1693,7 +1693,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (save_state(&stash))\n \t\toidclr(&stash);\n \n-\tfor (i = 0; !merge_was_ok && i < use_strategies_nr; i++) {\n+\tfor (i = 0; i < use_strategies_nr; i++) {\n \t\tint ret, cnt;\n \t\tif (i) {\n \t\t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n@@ -1718,12 +1718,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\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\t/*\n+\t\t\t\t * This strategy worked; no point in trying\n+\t\t\t\t * another.\n+\t\t\t\t */\n \t\t\t\tmerge_was_ok = 1;\n+\t\t\t\tbest_strategy = use_strategies[i]->name;\n+\t\t\t\tbreak;\n \t\t\t}\n \t\t\tcnt = (use_strategies_nr > 1) ? evaluate_result() : 0;\n \t\t\tif (best_cnt <= 0 || cnt <= best_cnt) {\n@@ -1737,7 +1738,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * If we have a resulting tree, that means the strategy module\n \t * auto resolved the merge cleanly.\n \t */\n-\tif (automerge_was_ok) {\n+\tif (merge_was_ok && option_commit) {\n+\t\tautomerge_was_ok = 1;\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-- \ngitgitgadget\n\n"},{"id":"461828","messageId":"88173eba0b9cdd861b9c8e3d229decae3b312681.1661222541.git.gitgitgadget@gmail.com","threadId":"58339","inReplyTo":"pull.1331.v2.git.1661222541.gitgitgadget@gmail.com","subject":"[PATCH v2 3/3] merge: small code readability improvement","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-08-23T02:42:21Z","receivedAt":"2022-08-23T02:42:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nAfter our loop through the selected strategies, we compare best_strategy\nto wt_strategy. This is fine, but the fact that the code setting\nbest_strategy sets it to use_strategies[i]->name requires a little bit\nof extra checking to determine that at the time of setting, that's the\nsame as wt_strategy. Just setting best_strategy to wt_strategy makes it\na little easier to verify what the loop is doing, at least for this\nreader.\n\nFurther, use_strategies[i]->name is used in a number of places, where we\ncould just use wt_strategy. The latter takes less time for this reader\nto parse (one variable name instead of three), so just use wt_strategy\nto make the code slightly faster for human readers to parse.\n\nSigned-off-by: Elijah Newren \n---\n builtin/merge.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex abf0567b20f..5900b81729d 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1708,7 +1708,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t */\n \t\twt_strategy = use_strategies[i]->name;\n \n-\t\tret = try_merge_strategy(use_strategies[i]->name,\n+\t\tret = try_merge_strategy(wt_strategy,\n \t\t\t\t\t common, remoteheads,\n \t\t\t\t\t head_commit);\n \t\t/*\n@@ -1723,12 +1723,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t * another.\n \t\t\t\t */\n \t\t\t\tmerge_was_ok = 1;\n-\t\t\t\tbest_strategy = use_strategies[i]->name;\n+\t\t\t\tbest_strategy = wt_strategy;\n \t\t\t\tbreak;\n \t\t\t}\n \t\t\tcnt = (use_strategies_nr > 1) ? evaluate_result() : 0;\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_strategy = wt_strategy;\n \t\t\t\tbest_cnt = cnt;\n \t\t\t}\n \t\t}\n-- \ngitgitgadget\n"},{"id":"461829","messageId":"CABPp-BHcRQatt9F2Hj4ygUsS9y8_BmfN3K=YFrLEnCn+2WLz9A@mail.gmail.com","threadId":"58339","inReplyTo":"pull.1331.v2.git.1661222541.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 0/3] Miscellaneous merge fixes","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2022-08-23T03:03:00Z","receivedAt":"2022-08-23T03:06:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Aug 22, 2022 at 7:42 PM Elijah Newren via GitGitGadget\n wrote:\n>\n> While working on other things, I noticed a few miscellaneous issues in\n> builtin/merge.c. Here's a few small fixes to address them.\n\nSorry, forgot to include a high level summary:\n\nChanges since v1:\n * Fix play/apply typo, spotted by Eric\n * Almost completely rewrote patch 2; it's now merely a \"code cleanup\"\n * Added a third patch, just a very minor code simplification\n"},{"id":"461913","messageId":"xmqqfshl5oy5.fsf@gitster.g","threadId":"58339","inReplyTo":"CABPp-BHcRQatt9F2Hj4ygUsS9y8_BmfN3K=YFrLEnCn+2WLz9A@mail.gmail.com","subject":"Re: [PATCH v2 0/3] Miscellaneous merge fixes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-08-24T21:09:06Z","receivedAt":"2022-08-24T21:09:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Mon, Aug 22, 2022 at 7:42 PM Elijah Newren via GitGitGadget\n> wrote:\n>>\n>> While working on other things, I noticed a few miscellaneous issues in\n>> builtin/merge.c. Here's a few small fixes to address them.\n>\n> Sorry, forgot to include a high level summary:\n>\n> Changes since v1:\n> * Fix play/apply typo, spotted by Eric\n> * Almost completely rewrote patch 2; it's now merely a \"code cleanup\"\n> * Added a third patch, just a very minor code simplification\n\nI was skeptical about patch 2 before I saw it, but removal of\n!merge_was_ok in the loop condition alone is very well worth it.\n\nAs to patch 3, I do not think it makes huge difference either way,\nbut I've queued it anyway.\n\nThanks.\n\n"}]}