{"thread":{"id":"56151","subject":"[PATCH 0/4] [RFC] merge --autostash: apply autostash in more cases","startedAt":"2021-07-23T12:14:35Z","lastAt":"2021-07-23T19:22:37Z","messageCount":11,"participants":["Philippe Blain via GitGitGadget","Felipe Contreras","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"431023","messageId":"pull.1002.git.1627042470.gitgitgadget@gmail.com","threadId":"56151","inReplyTo":null,"subject":"[PATCH 0/4] [RFC] merge --autostash: apply autostash in more cases","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-23T12:14:26Z","receivedAt":"2021-07-23T12:14:35Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"This series aims to apply the stash created by 'git merge --autostash' in 2\nsituations that were not covered by the code:\n\n 1. If the merge is fast-forward but the fast-forward operation fails -\n    PATCH 3/4\n 2. If the merge strategy completely fails to handle the merge (exit code 2)\n    - PATCH 4/4\n\nThe first 2 commits are small improvements that I noticed while implementing\nthe other two.\n\nI'm marking it [RFC] because I'm not 100% sure that trying to apply the\nautostash in 3/4 and 4/4 is actually the best course of action (or if it\nwould be better to call 'save_autostash' instead). That's because:\n\nFor 3/4 (fast-forward fails): I'm not sure if 'unpack_trees' (called by\n'checkout_fast_forward') is guaranteed to fail atomically, or it might fail\nmid-way and leave the worktree unclean, in which case it might be better not\nto apply the autostash, but just save it instead (and tell the user). In the\ntest case I'm adding, it does fail before starting to update the working\ntree, but I'm not sure if it's always the case.\n\nFor 4/4 (merge strategy fails): Same reasoning: I'm not sure if all\nstrategies (and even less user-supplied ones, which are not documented but\nkind of supported) are guaranteed to 'exit 2' before messing up the working\ntree.\n\nPhilippe Blain (4):\n  merge: add missing word \"strategy\" to a message\n  Documentation: define 'MERGE_AUTOSTASH'\n  merge: apply autostash if fast-forward fails\n  merge: apply autostash if merge strategy fails\n\n Documentation/merge-options.txt |  3 ++-\n builtin/merge.c                 |  4 +++-\n t/t7600-merge.sh                | 19 +++++++++++++++++++\n 3 files changed, 24 insertions(+), 2 deletions(-)\n\n\nbase-commit: d486ca60a51c9cb1fe068803c3f540724e95e83a\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1002%2Fphil-blain%2Fmerge-autostash-ff-fails-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1002/phil-blain/merge-autostash-ff-fails-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1002\n-- \ngitgitgadget\n"},{"id":"431024","messageId":"85016e679eb77cbb17edc9b6849251893a5e9ec2.1627042470.git.gitgitgadget@gmail.com","threadId":"56151","inReplyTo":"pull.1002.git.1627042470.gitgitgadget@gmail.com","subject":"[PATCH 1/4] merge: add missing word \"strategy\" to a message","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-23T12:14:27Z","receivedAt":"2021-07-23T12:14:37Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThe variable 'best_strategy' holds the name of the merge strategy that\nresulted in fewer conflicts, if several strategies were tried. When\nthat's the case but the best strategy was not the first one tried, we\ninform the user which strategy was the \"best\" one before recreating the\nmerge and leaving the conflicted files in the tree.\n\nThis informational message is missing the word \"strategy\", so it shows\nsomething like:\n\n    Using the recursive to prepare resolving by hand.\n\nFix that.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n builtin/merge.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex a8a843b1f54..74797b6c7a6 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1715,7 +1715,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \telse {\n \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n \t\trestore_state(&head_commit->object.oid, &stash);\n-\t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n+\t\tprintf(_(\"Using the %s strategy to prepare resolving by hand.\\n\"),\n \t\t\tbest_strategy);\n \t\ttry_merge_strategy(best_strategy, common, remoteheads,\n \t\t\t\t   head_commit);\n-- \ngitgitgadget\n\n"},{"id":"431025","messageId":"86becf764243f129c980f073127ec3f08fd4477c.1627042470.git.gitgitgadget@gmail.com","threadId":"56151","inReplyTo":"pull.1002.git.1627042470.gitgitgadget@gmail.com","subject":"[PATCH 3/4] merge: apply autostash if fast-forward fails","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-23T12:14:29Z","receivedAt":"2021-07-23T12:14:38Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince 'git merge' learned '--autostash' in a03b55530a (merge: teach\n--autostash option, 2020-04-07), 'cmd_merge', in the fast-forward case,\ncalls 'create_autostash' before calling 'checkout_fast_forward' if\n'--autostash' is given.\n\nHowever, if 'checkout_fast_forward' fails, the autostash is not applied\nto the working tree, nor saved in the stash list, since the code simply\ncalls 'goto done'.\n\nBe more helpful to the user by applying the autostash in that case.\n\nAn easy way to test a failing fast-forward is when we are merging a\nbranch that has a tracked file that conflicts with an untracked file in\nthe working tree.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n builtin/merge.c  |  1 +\n t/t7600-merge.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 74797b6c7a6..788a6b0cd55 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t  &head_commit->object.oid,\n \t\t\t\t\t  &commit->object.oid,\n \t\t\t\t\t  overwrite_ignore)) {\n+\t\t\tapply_autostash(git_path_merge_autostash(the_repository));\n \t\t\tret = 1;\n \t\t\tgoto done;\n \t\t}\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 1cbc9715a81..216113d3537 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -122,6 +122,8 @@ test_expect_success 'setup' '\n \tc0=$(git rev-parse HEAD) &&\n \tcp file.1 file &&\n \tgit add file &&\n+\tcp file.1 other &&\n+\tgit add other &&\n \ttest_tick &&\n \tgit commit -m \"commit 1\" &&\n \tgit tag c1 &&\n@@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '\n \ttest_cmp result.1-5 file\n '\n \n+test_expect_success 'failed fast-forward merge with --autostash' '\n+\tgit reset --hard c0 &&\n+\tgit merge-file file file.orig file.5 &&\n+\tcp file.5 other &&\n+\ttest_must_fail git merge --autostash c1 2>err &&\n+\ttest_i18ngrep \"Applied autostash.\" err &&\n+\ttest_cmp file.5 file\n+'\n+\n test_expect_success 'octopus merge with --autostash' '\n \tgit reset --hard c1 &&\n \tgit merge-file file file.orig file.3 &&\n-- \ngitgitgadget\n\n"},{"id":"431026","messageId":"5d3d95c9cb978ee02d9e39c5eb70b49ecdbd218f.1627042470.git.gitgitgadget@gmail.com","threadId":"56151","inReplyTo":"pull.1002.git.1627042470.gitgitgadget@gmail.com","subject":"[PATCH 2/4] Documentation: define 'MERGE_AUTOSTASH'","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-23T12:14:28Z","receivedAt":"2021-07-23T12:14:39Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nThe documentation for 'git merge --abort' and 'git merge --quit' both\nmention the special ref 'MERGE_AUTOSTASH', but this ref is not formally\ndefined anywhere. Mention it in the description of the '--autostash'\noption for 'git merge'.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n Documentation/merge-options.txt | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt\nindex eb0aabd396f..52565014c15 100644\n--- a/Documentation/merge-options.txt\n+++ b/Documentation/merge-options.txt\n@@ -154,7 +154,8 @@ endif::git-pull[]\n --autostash::\n --no-autostash::\n \tAutomatically create a temporary stash entry before the operation\n-\tbegins, and apply it after the operation ends.  This means\n+\tbegins, record it in the special ref `MERGE_AUTOSTASH`\n+\tand apply it after the operation ends.  This means\n \tthat you can run the operation on a dirty worktree.  However, use\n \twith care: the final stash application after a successful\n \tmerge might result in non-trivial conflicts.\n-- \ngitgitgadget\n\n"},{"id":"431027","messageId":"e02b945ce8595a8d7b1b4ad9c382a1beeb040ed4.1627042470.git.gitgitgadget@gmail.com","threadId":"56151","inReplyTo":"pull.1002.git.1627042470.gitgitgadget@gmail.com","subject":"[PATCH 4/4] merge: apply autostash if merge strategy fails","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2021-07-23T12:14:30Z","receivedAt":"2021-07-23T12:14:39Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain <levraiphilippeblain@gmail.com>\n\nSince 'git merge' learned '--autostash' in a03b55530a (merge: teach\n--autostash option, 2020-04-07), 'cmd_merge', once it is determined that\nwe have to create a merge commit, calls 'create_autostash' if\n'--autostash' is given.\n\nAs explained in a03b55530a, and made more abvious by the tests added in\nthat commit, the autostash is then applied if the merge succeeds, either\ndirectly or by committing (after conflict resolution or if '--no-commit'\nwas given), or if the merge is aborted with 'git merge --abort'. In some\nother cases, like the user calling 'git reset --merge' or 'git merge\n--quit', the autostash is not applied, but saved in the stash list.\n\nHowever, there exists a scenario that creates an autostash but does not\napply nor save it to the stash list: if the chosen merge strategy\ncompletely fails to handle the merge, i.e. 'try_merge_strategy' returns\n2.\n\nApply the autostash in that case also. An easy way to test that is to\ntry to merge more than two commits but explicitely ask for the 'recursive'\nmerge strategy.\n\nSigned-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n---\n builtin/merge.c  | 1 +\n t/t7600-merge.sh | 8 ++++++++\n 2 files changed, 9 insertions(+)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 788a6b0cd55..d44c14a21a3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1709,6 +1709,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tfprintf(stderr, _(\"Merge with strategy %s failed.\\n\"),\n \t\t\t\tuse_strategies[0]->name);\n+\t\tapply_autostash(git_path_merge_autostash(the_repository));\n \t\tret = 2;\n \t\tgoto done;\n \t} else if (best_strategy == wt_strategy)\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex 216113d3537..2ef39d3088e 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -732,6 +732,14 @@ test_expect_success 'octopus merge with --autostash' '\n \ttest_cmp result.1-3-5-9 file\n '\n \n+test_expect_success 'failed merge (exit 2) with --autostash' '\n+\tgit reset --hard c1 &&\n+\tgit merge-file file file.orig file.5 &&\n+\ttest_must_fail git merge -s recursive --autostash c2 c3 2>err &&\n+\ttest_i18ngrep \"Applied autostash.\" err &&\n+\ttest_cmp result.1-5 file\n+'\n+\n test_expect_success 'conflicted merge with --autostash, --abort restores stash' '\n \tgit reset --hard c3 &&\n \tcp file.1 file &&\n-- \ngitgitgadget\n"},{"id":"431042","messageId":"60fae6cdadd2_defb20872@natae.notmuch","threadId":"56151","inReplyTo":"85016e679eb77cbb17edc9b6849251893a5e9ec2.1627042470.git.gitgitgadget@gmail.com","subject":"RE: [PATCH 1/4] merge: add missing word \"strategy\" to a message","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-23T15:57:01Z","receivedAt":"2021-07-23T15:57:06Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Philippe Blain via GitGitGadget wrote:\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1715,7 +1715,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \telse {\n>  \t\tprintf(_(\"Rewinding the tree to pristine...\\n\"));\n>  \t\trestore_state(&head_commit->object.oid, &stash);\n> -\t\tprintf(_(\"Using the %s to prepare resolving by hand.\\n\"),\n> +\t\tprintf(_(\"Using the %s strategy to prepare resolving by hand.\\n\"),\n\nObviously correct.\n\n-- \nFelipe Contreras\n"},{"id":"431044","messageId":"60fae7982f7b0_defb208c7@natae.notmuch","threadId":"56151","inReplyTo":"5d3d95c9cb978ee02d9e39c5eb70b49ecdbd218f.1627042470.git.gitgitgadget@gmail.com","subject":"RE: [PATCH 2/4] Documentation: define 'MERGE_AUTOSTASH'","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-23T16:00:24Z","receivedAt":"2021-07-23T16:00:31Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Philippe Blain via GitGitGadget wrote:\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n\n> --- a/Documentation/merge-options.txt\n> +++ b/Documentation/merge-options.txt\n> @@ -154,7 +154,8 @@ endif::git-pull[]\n>  --autostash::\n>  --no-autostash::\n>  \tAutomatically create a temporary stash entry before the operation\n> -\tbegins, and apply it after the operation ends.  This means\n> +\tbegins, record it in the special ref `MERGE_AUTOSTASH`\n> +\tand apply it after the operation ends.  This means\n>  \tthat you can run the operation on a dirty worktree.  However, use\n>  \twith care: the final stash application after a successful\n>  \tmerge might result in non-trivial conflicts.\n\nAlthough normally I'm against mentioning low-level technical details in\nhigh-level documentation, there doesn't seem to be a better location to\nexplain MERGE_AUTOSTASH, and it's just a simple sentence.\n\nFine by me.\n\n-- \nFelipe Contreras\n"},{"id":"431046","messageId":"60fae7f8747c_defb20888@natae.notmuch","threadId":"56151","inReplyTo":"86becf764243f129c980f073127ec3f08fd4477c.1627042470.git.gitgitgadget@gmail.com","subject":"RE: [PATCH 3/4] merge: apply autostash if fast-forward fails","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-23T16:02:00Z","receivedAt":"2021-07-23T16:02:04Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Philippe Blain via GitGitGadget wrote:\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\t  &head_commit->object.oid,\n>  \t\t\t\t\t  &commit->object.oid,\n>  \t\t\t\t\t  overwrite_ignore)) {\n> +\t\t\tapply_autostash(git_path_merge_autostash(the_repository));\n>  \t\t\tret = 1;\n>  \t\t\tgoto done;\n>  \t\t}\n\nI can verify that this fixes my simple test.\n\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 1cbc9715a81..216113d3537 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -122,6 +122,8 @@ test_expect_success 'setup' '\n>  \tc0=$(git rev-parse HEAD) &&\n>  \tcp file.1 file &&\n>  \tgit add file &&\n> +\tcp file.1 other &&\n> +\tgit add other &&\n>  \ttest_tick &&\n>  \tgit commit -m \"commit 1\" &&\n>  \tgit tag c1 &&\n> @@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '\n>  \ttest_cmp result.1-5 file\n>  '\n>  \n> +test_expect_success 'failed fast-forward merge with --autostash' '\n> +\tgit reset --hard c0 &&\n> +\tgit merge-file file file.orig file.5 &&\n> +\tcp file.5 other &&\n> +\ttest_must_fail git merge --autostash c1 2>err &&\n> +\ttest_i18ngrep \"Applied autostash.\" err &&\n\nI've heard others test we are moving away from test_i18ngrep in favor of\ngrep.\n\n> +\ttest_cmp file.5 file\n> +'\n> +\n>  test_expect_success 'octopus merge with --autostash' '\n>  \tgit reset --hard c1 &&\n>  \tgit merge-file file file.orig file.3 &&\n\n-- \nFelipe Contreras\n"},{"id":"431047","messageId":"60faea007b6ce_defb20871@natae.notmuch","threadId":"56151","inReplyTo":"pull.1002.git.1627042470.gitgitgadget@gmail.com","subject":"RE: [PATCH 0/4] [RFC] merge --autostash: apply autostash in more cases","fromName":"Felipe Contreras","fromEmail":"felipe.contreras@gmail.com","sentAt":"2021-07-23T16:10:40Z","receivedAt":"2021-07-23T16:10:45Z","isPatch":true,"sender":{"key":"felipe.contreras@gmail.com","avatar":"https://avatars.githubusercontent.com/u/8358?v=4"},"body":"Philippe Blain via GitGitGadget wrote:\n> This series aims to apply the stash created by 'git merge --autostash' in 2\n> situations that were not covered by the code:\n> \n>  1. If the merge is fast-forward but the fast-forward operation fails -\n>     PATCH 3/4\n>  2. If the merge strategy completely fails to handle the merge (exit code 2)\n>     - PATCH 4/4\n> \n> The first 2 commits are small improvements that I noticed while implementing\n> the other two.\n> \n> I'm marking it [RFC] because I'm not 100% sure that trying to apply the\n> autostash in 3/4 and 4/4 is actually the best course of action (or if it\n> would be better to call 'save_autostash' instead). That's because:\n> \n> For 3/4 (fast-forward fails): I'm not sure if 'unpack_trees' (called by\n> 'checkout_fast_forward') is guaranteed to fail atomically, or it might fail\n> mid-way and leave the worktree unclean, in which case it might be better not\n> to apply the autostash, but just save it instead (and tell the user). In the\n> test case I'm adding, it does fail before starting to update the working\n> tree, but I'm not sure if it's always the case.\n\nI'm not familiar with unpack_trees, but sans that possibility the whole\nseries looks fine to me.\n\n-- \nFelipe Contreras\n"},{"id":"431084","messageId":"xmqqo8asn8s4.fsf@gitster.g","threadId":"56151","inReplyTo":"86becf764243f129c980f073127ec3f08fd4477c.1627042470.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 3/4] merge: apply autostash if fast-forward fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-23T19:13:15Z","receivedAt":"2021-07-23T19:13:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Philippe Blain <levraiphilippeblain@gmail.com>\n>\n> Since 'git merge' learned '--autostash' in a03b55530a (merge: teach\n> --autostash option, 2020-04-07), 'cmd_merge', in the fast-forward case,\n> calls 'create_autostash' before calling 'checkout_fast_forward' if\n> '--autostash' is given.\n>\n> However, if 'checkout_fast_forward' fails, the autostash is not applied\n> to the working tree, nor saved in the stash list, since the code simply\n> calls 'goto done'.\n>\n> Be more helpful to the user by applying the autostash in that case.\n>\n> An easy way to test a failing fast-forward is when we are merging a\n> branch that has a tracked file that conflicts with an untracked file in\n> the working tree.\n\nI think this one makes sense, as the case that is tested I know\nfast-forward (aka two-tree switching) would be atomic. If your\nworking tree is already broken (e.g. triggers I/O errors in the\nmiddle, some directories having wrong permissions to make them\nunwritable by you, etc.), you would also see an error from\nfast-forward and you probably cannot tell these two cases apart,\nand trying to unstash the local changes may make things even worse,\nbut I suspect that there isn't much you can do about it.\n\nThanks.\n\n>\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  builtin/merge.c  |  1 +\n>  t/t7600-merge.sh | 11 +++++++++++\n>  2 files changed, 12 insertions(+)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 74797b6c7a6..788a6b0cd55 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1560,6 +1560,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\t  &head_commit->object.oid,\n>  \t\t\t\t\t  &commit->object.oid,\n>  \t\t\t\t\t  overwrite_ignore)) {\n> +\t\t\tapply_autostash(git_path_merge_autostash(the_repository));\n>  \t\t\tret = 1;\n>  \t\t\tgoto done;\n>  \t\t}\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 1cbc9715a81..216113d3537 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -122,6 +122,8 @@ test_expect_success 'setup' '\n>  \tc0=$(git rev-parse HEAD) &&\n>  \tcp file.1 file &&\n>  \tgit add file &&\n> +\tcp file.1 other &&\n> +\tgit add other &&\n>  \ttest_tick &&\n>  \tgit commit -m \"commit 1\" &&\n>  \tgit tag c1 &&\n> @@ -711,6 +713,15 @@ test_expect_success 'fast-forward merge with --autostash' '\n>  \ttest_cmp result.1-5 file\n>  '\n>  \n> +test_expect_success 'failed fast-forward merge with --autostash' '\n> +\tgit reset --hard c0 &&\n> +\tgit merge-file file file.orig file.5 &&\n> +\tcp file.5 other &&\n> +\ttest_must_fail git merge --autostash c1 2>err &&\n> +\ttest_i18ngrep \"Applied autostash.\" err &&\n> +\ttest_cmp file.5 file\n> +'\n> +\n>  test_expect_success 'octopus merge with --autostash' '\n>  \tgit reset --hard c1 &&\n>  \tgit merge-file file file.orig file.3 &&\n"},{"id":"431086","messageId":"xmqqk0lgn8cl.fsf@gitster.g","threadId":"56151","inReplyTo":"e02b945ce8595a8d7b1b4ad9c382a1beeb040ed4.1627042470.git.gitgitgadget@gmail.com","subject":"Re: [PATCH 4/4] merge: apply autostash if merge strategy fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-23T19:22:34Z","receivedAt":"2021-07-23T19:22:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Philippe Blain via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> As explained in a03b55530a, and made more abvious by the tests added in\n> that commit, the autostash is then applied if the merge succeeds, either\n> directly or by committing (after conflict resolution or if '--no-commit'\n> was given), or if the merge is aborted with 'git merge --abort'. In some\n> other cases, like the user calling 'git reset --merge' or 'git merge\n> --quit', the autostash is not applied, but saved in the stash list.\n\nOK.\n\n> However, there exists a scenario that creates an autostash but does not\n> apply nor save it to the stash list: if the chosen merge strategy\n> completely fails to handle the merge, i.e. 'try_merge_strategy' returns\n> 2.\n>\n> Apply the autostash in that case also. An easy way to test that is to\n> try to merge more than two commits but explicitely ask for the 'recursive'\n> merge strategy.\n\nWe know that the recursive that barfs when asked to merge more than\none \"other\" histories will fail before touching the index or the\nworking tree, but I do not think the merge-strategy protocol makes\nany guarantee as to what half-modified state the strategy would\nleave the index and the working tree in.\n\nSo this change at the first glance looks risky, but at least we have\na call to restore_state() before you added an apply_autostash().\nThe restore_state() call is the same as the one is used between\napplication of a strategy that has failed the same way in the loop,\nso we should be able to trust it to bring the state back to the\npristine (i.e. after autostash removed the local changes).\n\nI think this change makes sense, too.\n\n(I didn't look at 1/4 and 2/4)\n\nThanks.\n\n\n\n> Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> ---\n>  builtin/merge.c  | 1 +\n>  t/t7600-merge.sh | 8 ++++++++\n>  2 files changed, 9 insertions(+)\n>\n> diff --git a/builtin/merge.c b/builtin/merge.c\n> index 788a6b0cd55..d44c14a21a3 100644\n> --- a/builtin/merge.c\n> +++ b/builtin/merge.c\n> @@ -1709,6 +1709,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n>  \t\telse\n>  \t\t\tfprintf(stderr, _(\"Merge with strategy %s failed.\\n\"),\n>  \t\t\t\tuse_strategies[0]->name);\n> +\t\tapply_autostash(git_path_merge_autostash(the_repository));\n>  \t\tret = 2;\n>  \t\tgoto done;\n>  \t} else if (best_strategy == wt_strategy)\n> diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\n> index 216113d3537..2ef39d3088e 100755\n> --- a/t/t7600-merge.sh\n> +++ b/t/t7600-merge.sh\n> @@ -732,6 +732,14 @@ test_expect_success 'octopus merge with --autostash' '\n>  \ttest_cmp result.1-3-5-9 file\n>  '\n>  \n> +test_expect_success 'failed merge (exit 2) with --autostash' '\n> +\tgit reset --hard c1 &&\n> +\tgit merge-file file file.orig file.5 &&\n> +\ttest_must_fail git merge -s recursive --autostash c2 c3 2>err &&\n> +\ttest_i18ngrep \"Applied autostash.\" err &&\n> +\ttest_cmp result.1-5 file\n> +'\n> +\n>  test_expect_success 'conflicted merge with --autostash, --abort restores stash' '\n>  \tgit reset --hard c3 &&\n>  \tcp file.1 file &&\n"}]}