| Message ID | 9e3d4393cae8813cc4718c6ffcc28231b1344fbe.1584782450.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | merge: learn --autostash | expand |
Hi Denton On 21/03/2020 09:21, Denton Liu wrote: > In rebase, one can pass the `--autostash` option to cause the worktree > to be automatically stashed before continuing with the rebase. This > option is missing in merge, however. > > Implement the `--autostash` option and corresponding `merge.autoStash` > option in merge which stashes before merging and then pops after. > > This option is useful when a developer has some local changes on a topic > branch but they realize that their work depends on another branch. > Previously, they had to run something like > > git fetch ... > git stash push > git merge FETCH_HEAD > git stash pop > > but now, that is reduced to > > git fetch ... > git merge --autostash FETCH_HEAD > > When `git reset --hard` is run to abort a merge, the working tree will > be left in a clean state, as expected, with the autostash pushed onto > the stash stack. > > Suggested-by: Alban Gruin <alban.gruin@gmail.com> > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > Documentation/config/merge.txt | 10 +++ > Documentation/merge-options.txt | 8 +++ > branch.c | 1 + > builtin/commit.c | 2 + > builtin/merge.c | 17 ++++++ > builtin/reset.c | 7 ++- > path.c | 1 + > path.h | 4 +- > t/t3033-merge-toplevel.sh | 22 +++++++ > t/t7600-merge.sh | 104 ++++++++++++++++++++++++++++++++ > 10 files changed, 174 insertions(+), 2 deletions(-) > > diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt > index 6a313937f8..88b29127bf 100644 > --- a/Documentation/config/merge.txt > +++ b/Documentation/config/merge.txt > @@ -70,6 +70,16 @@ merge.stat:: > Whether to print the diffstat between ORIG_HEAD and the merge result > at the end of the merge. True by default. > > +merge.autoStash:: > + When set to true, automatically create a temporary stash entry > + before the operation begins, and apply it after the operation > + ends. This means that you can run rebase copy and paste error - s/rebase/merge/ > on a dirty worktree. > + However, use with care: the final stash application after a > + successful rebase might result in non-trivial conflicts. > + This option can be overridden by the `--no-autostash` and > + `--autostash` options of linkgit:git-merge[1]. > + Defaults to false. > + > merge.tool:: > Controls which merge tool is used by linkgit:git-mergetool[1]. > The list below shows the valid built-in values. > diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt > index 40dc4f5e8c..34493eb58b 100644 > --- a/Documentation/merge-options.txt > +++ b/Documentation/merge-options.txt > @@ -155,6 +155,14 @@ ifndef::git-pull[] > Note that not all merge strategies may support progress > reporting. > > +--autostash:: > +--no-autostash:: > + Automatically create a temporary stash entry before the operation > + begins, and apply it after the operation ends. This means > + that you can run rebase on a dirty worktree. However, use > + with care: the final stash application after a successful > + rebase might result in non-trivial conflicts. s/rebase/merge/ > + > endif::git-pull[] > > --allow-unrelated-histories:: > diff --git a/branch.c b/branch.c > index 579494738a..bf2536c70d 100644 > --- a/branch.c > +++ b/branch.c > @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r) > unlink(git_path_merge_rr(r)); > unlink(git_path_merge_msg(r)); > unlink(git_path_merge_mode(r)); > + apply_autostash(git_path_merge_autostash(r)); > } > > void remove_branch_state(struct repository *r, int verbose) > diff --git a/builtin/commit.c b/builtin/commit.c > index 7ba33a3bec..c11894423a 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > unlink(git_path_merge_mode(the_repository)); > unlink(git_path_squash_msg(the_repository)); > > + apply_autostash(git_path_merge_autostash(the_repository)); > + It's hard to tell from the limited context but do we want to run commit_index_files() before applying the autostash? I wonder if this should be using remove_merge_branch_state() instead of duplicating it as well. > if (commit_index_files()) > die(_("repository has been updated, but unable to write\n" > "new_index file. Check that disk is not full and quota is\n" > diff --git a/builtin/merge.c b/builtin/merge.c > index d127d2225f..e038bef5ad 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -82,6 +82,7 @@ static int show_progress = -1; > static int default_to_upstream = 1; > static int signoff; > static const char *sign_commit; > +static int autostash; > static int no_verify; > > static struct strategy all_strategy[] = { > @@ -286,6 +287,8 @@ static struct option builtin_merge_options[] = { > OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1), > { OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"), > N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, > + OPT_BOOL(0, "autostash", &autostash, > + N_("automatically stash/stash pop before and after")), If we're adding this option to several commands (rebase, merge, pull) then it might be worth defining it in parse-options.h > OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")), > OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")), > OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")), > @@ -441,6 +444,7 @@ static void finish(struct commit *head_commit, > strbuf_addf(&reflog_message, "%s: %s", > getenv("GIT_REFLOG_ACTION"), msg); > } > + apply_autostash(git_path_merge_autostash(the_repository)); > if (squash) { > squash_message(head_commit, remoteheads); > } else { > @@ -634,6 +638,9 @@ static int git_merge_config(const char *k, const char *v, void *cb) > return 0; > } else if (!strcmp(k, "gpg.mintrustlevel")) { > check_trust_level = 0; > + } else if (!strcmp(k, "merge.autostash")) { > + autostash = git_config_bool(k, v); > + return 0; > } > > status = fmt_merge_msg_config(k, v, cb); > @@ -1291,6 +1298,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > > /* Invoke 'git reset --merge' */ > ret = cmd_reset(nargc, nargv, prefix); > + apply_autostash(git_path_merge_autostash(the_repository)); > goto done; > } > > @@ -1513,6 +1521,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > goto done; > } > > + if (autostash) > + create_autostash(the_repository, > + git_path_merge_autostash(the_repository), > + "merge"); > if (checkout_fast_forward(the_repository, > &head_commit->object.oid, > &commit->object.oid, > @@ -1579,6 +1591,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > if (fast_forward == FF_ONLY) > die(_("Not possible to fast-forward, aborting.")); > > + if (autostash) > + create_autostash(the_repository, > + git_path_merge_autostash(the_repository), > + "merge"); > + > /* We are going to make a new commit. */ > git_committer_info(IDENT_STRICT); > > diff --git a/builtin/reset.c b/builtin/reset.c > index 18228c312e..038c8532eb 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -25,6 +25,7 @@ > #include "cache-tree.h" > #include "submodule.h" > #include "submodule-config.h" > +#include "sequencer.h" > > #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) > > @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > if (reset_type == HARD && !update_ref_status && !quiet) > print_new_head_line(lookup_commit_reference(the_repository, &oid)); > } > - if (!pathspec.nr) > + if (!pathspec.nr) { > + if (reset_type == HARD) > + save_autostash(git_path_merge_autostash(the_repository)); > + > remove_branch_state(the_repository, 0); This removes the autostash file for all reset types but we only keep the stash in the case of 'reset --hard' which is confusing. Best Wishes Phillip > + } > > return update_ref_status; > } > diff --git a/path.c b/path.c > index 88cf593007..d764738146 100644 > --- a/path.c > +++ b/path.c > @@ -1535,5 +1535,6 @@ REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG") > REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR") > REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE") > REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD") > +REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH") > REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD") > REPO_GIT_PATH_FUNC(shallow, "shallow") > diff --git a/path.h b/path.h > index 14d6dcad16..1f1bf8f87a 100644 > --- a/path.h > +++ b/path.h > @@ -177,11 +177,12 @@ struct path_cache { > const char *merge_rr; > const char *merge_mode; > const char *merge_head; > + const char *merge_autostash; > const char *fetch_head; > const char *shallow; > }; > > -#define PATH_CACHE_INIT { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL } > +#define PATH_CACHE_INIT { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL } > > const char *git_path_cherry_pick_head(struct repository *r); > const char *git_path_revert_head(struct repository *r); > @@ -190,6 +191,7 @@ const char *git_path_merge_msg(struct repository *r); > const char *git_path_merge_rr(struct repository *r); > const char *git_path_merge_mode(struct repository *r); > const char *git_path_merge_head(struct repository *r); > +const char *git_path_merge_autostash(struct repository *r); > const char *git_path_fetch_head(struct repository *r); > const char *git_path_shallow(struct repository *r); > > diff --git a/t/t3033-merge-toplevel.sh b/t/t3033-merge-toplevel.sh > index d314599428..e29c284b9b 100755 > --- a/t/t3033-merge-toplevel.sh > +++ b/t/t3033-merge-toplevel.sh > @@ -142,6 +142,17 @@ test_expect_success 'refuse two-project merge by default' ' > test_must_fail git merge five > ' > > +test_expect_success 'refuse two-project merge by default, quit before --autostash happens' ' > + t3033_reset && > + git reset --hard four && > + echo change >>one.t && > + git diff >expect && > + test_must_fail git merge --autostash five 2>err && > + test_i18ngrep ! "stash" err && > + git diff >actual && > + test_cmp expect actual > +' > + > test_expect_success 'two-project merge with --allow-unrelated-histories' ' > t3033_reset && > git reset --hard four && > @@ -149,4 +160,15 @@ test_expect_success 'two-project merge with --allow-unrelated-histories' ' > git diff --exit-code five > ' > > +test_expect_success 'two-project merge with --allow-unrelated-histories with --autostash' ' > + t3033_reset && > + git reset --hard four && > + echo change >>one.t && > + git diff one.t >expect && > + git merge --allow-unrelated-histories --autostash five 2>err && > + test_i18ngrep "Applied autostash." err && > + git diff one.t >actual && > + test_cmp expect actual > +' > + > test_done > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > index 4fa0ef8e3b..c08e4315f4 100755 > --- a/t/t7600-merge.sh > +++ b/t/t7600-merge.sh > @@ -30,13 +30,17 @@ Testing basic merge operations/option parsing. > . "$TEST_DIRECTORY"/lib-gpg.sh > > test_write_lines 1 2 3 4 5 6 7 8 9 >file > +cp file file.orig > test_write_lines '1 X' 2 3 4 5 6 7 8 9 >file.1 > +test_write_lines 1 2 '3 X' 4 5 6 7 8 9 >file.3 > test_write_lines 1 2 3 4 '5 X' 6 7 8 9 >file.5 > test_write_lines 1 2 3 4 5 6 7 8 '9 X' >file.9 > test_write_lines 1 2 3 4 5 6 7 8 '9 Y' >file.9y > test_write_lines '1 X' 2 3 4 5 6 7 8 9 >result.1 > test_write_lines '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5 > +test_write_lines '1 X' 2 3 4 5 6 7 8 '9 X' >result.1-9 > test_write_lines '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9 > +test_write_lines '1 X' 2 '3 X' 4 '5 X' 6 7 8 '9 X' >result.1-3-5-9 > test_write_lines 1 2 3 4 5 6 7 8 '9 Z' >result.9z > > create_merge_msgs () { > @@ -675,6 +679,106 @@ test_expect_success 'refresh the index before merging' ' > git merge c3 > ' > > +test_expect_success 'merge with --autostash' ' > + git reset --hard c1 && > + git merge-file file file.orig file.9 && > + git merge --autostash c2 2>err && > + test_i18ngrep "Applied autostash." err && > + git show HEAD:file >merge-result && > + test_cmp result.1-5 merge-result && > + test_cmp result.1-5-9 file > +' > + > +test_expect_success 'merge with merge.autoStash' ' > + test_config merge.autoStash true && > + git reset --hard c1 && > + git merge-file file file.orig file.9 && > + git merge c2 2>err && > + test_i18ngrep "Applied autostash." err && > + git show HEAD:file >merge-result && > + test_cmp result.1-5 merge-result && > + test_cmp result.1-5-9 file > +' > + > +test_expect_success 'fast-forward merge with --autostash' ' > + git reset --hard c0 && > + git merge-file file file.orig file.5 && > + git merge --autostash c1 2>err && > + test_i18ngrep "Applied autostash." err && > + test_cmp result.1-5 file > +' > + > +test_expect_success 'octopus merge with --autostash' ' > + git reset --hard c1 && > + git merge-file file file.orig file.3 && > + git merge --autostash c2 c3 2>err && > + test_i18ngrep "Applied autostash." err && > + git show HEAD:file >merge-result && > + test_cmp result.1-5-9 merge-result && > + test_cmp result.1-3-5-9 file > +' > + > +test_expect_success 'conflicted merge with --autostash, --abort restores stash' ' > + git reset --hard c3 && > + cp file.1 file && > + test_must_fail git merge --autostash c7 && > + git merge --abort 2>err && > + test_i18ngrep "Applied autostash." err && > + test_cmp file.1 file > +' > + > +test_expect_success 'completed merge with --no-commit and --autostash' ' > + git reset --hard c1 && > + git merge-file file file.orig file.9 && > + git diff >expect && > + git merge --no-commit --autostash c2 && > + git stash show -p MERGE_AUTOSTASH >actual && > + test_cmp expect actual && > + git commit 2>err && > + test_i18ngrep "Applied autostash." err && > + git show HEAD:file >merge-result && > + test_cmp result.1-5 merge-result && > + test_cmp result.1-5-9 file > +' > + > +test_expect_success 'aborted merge (merge --abort) with --no-commit and --autostash' ' > + git reset --hard c1 && > + git merge-file file file.orig file.9 && > + git diff >expect && > + git merge --no-commit --autostash c2 && > + git stash show -p MERGE_AUTOSTASH >actual && > + test_cmp expect actual && > + git merge --abort 2>err && > + test_i18ngrep "Applied autostash." err && > + git diff >actual && > + test_cmp expect actual > +' > + > +test_expect_success 'aborted merge (reset --hard) with --no-commit and --autostash' ' > + git reset --hard c1 && > + git merge-file file file.orig file.9 && > + git diff >expect && > + git merge --no-commit --autostash c2 && > + git stash show -p MERGE_AUTOSTASH >actual && > + test_cmp expect actual && > + git reset --hard 2>err && > + test_i18ngrep "Autostash exists; creating a new stash entry." err && > + git diff --exit-code > +' > + > +test_expect_success 'merge with conflicted --autostash changes' ' > + git reset --hard c1 && > + git merge-file file file.orig file.9y && > + git diff >expect && > + test_when_finished "test_might_fail git stash drop" && > + git merge --autostash c3 2>err && > + test_i18ngrep "Applying autostash resulted in conflicts." err && > + git show HEAD:file >merge-result && > + test_cmp result.1-9 merge-result && > + git stash show -p >actual && > + test_cmp expect actual > +' > + > cat >expected.branch <<\EOF > Merge branch 'c5-branch' (early part) > EOF >
Hi Phillip, Thanks for your detailed review on the series. I've updated everything according to your comments except for the parts below: On Thu, Apr 02, 2020 at 04:24:54PM +0100, Phillip Wood wrote: > > diff --git a/branch.c b/branch.c > > index 579494738a..bf2536c70d 100644 > > --- a/branch.c > > +++ b/branch.c > > @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r) > > unlink(git_path_merge_rr(r)); > > unlink(git_path_merge_msg(r)); > > unlink(git_path_merge_mode(r)); > > + apply_autostash(git_path_merge_autostash(r)); > > } > > void remove_branch_state(struct repository *r, int verbose) > > diff --git a/builtin/commit.c b/builtin/commit.c > > index 7ba33a3bec..c11894423a 100644 > > --- a/builtin/commit.c > > +++ b/builtin/commit.c > > @@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > > unlink(git_path_merge_mode(the_repository)); > > unlink(git_path_squash_msg(the_repository)); > > + apply_autostash(git_path_merge_autostash(the_repository)); > > + > > It's hard to tell from the limited context but do we want to run > commit_index_files() before applying the autostash? I don't think it really matters which order we run it in. When we run apply_autostash(), we only ever touch the working tree, not the index so it doesn't matter if it's run before or after. I'd prefer to keep it here because if we ever refactor this to use remove_merge_branch_state(), the person working on this will be able to perform the refactor more easily without having to worry about implicit ordering dependencies. > I wonder if this should > be using remove_merge_branch_state() instead of duplicating it as well. We can _almost_ use remove_branch_state() even! Unfortunately, remove_merge_branch_state() calls `unlink(git_path_merge_rr(r))` which we cannot do since repo_rerere() relies on it later. Perhaps we can can move repo_rerere() earlier? > > diff --git a/builtin/reset.c b/builtin/reset.c > > index 18228c312e..038c8532eb 100644 > > --- a/builtin/reset.c > > +++ b/builtin/reset.c > > @@ -25,6 +25,7 @@ > > #include "cache-tree.h" > > #include "submodule.h" > > #include "submodule-config.h" > > +#include "sequencer.h" > > #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) > > @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > > if (reset_type == HARD && !update_ref_status && !quiet) > > print_new_head_line(lookup_commit_reference(the_repository, &oid)); > > } > > - if (!pathspec.nr) > > + if (!pathspec.nr) { > > + if (reset_type == HARD) > > + save_autostash(git_path_merge_autostash(the_repository)); > > + > > remove_branch_state(the_repository, 0); > > This removes the autostash file for all reset types but we only keep the > stash in the case of 'reset --hard' which is confusing. I was worried that this change would be controversial... The rationale behind this change was that with `reset --hard`, we want to leave a clean working tree behind so we save it into the stash reflog. In all other cases, remove_branch_state() will apply the saved stash entry which should be fine since users don't expect a clean worktree with the other reset types. I considered saving the autostash in all cases of reset but `git merge --abort` invokes `git reset --merge` behind the scenes so we'd have to consider that. Perhaps we can make all resets save the stash entry and, in the case of `merge --abort`, we can add some extra logic to subvert this so that the stash entry is applied? I'm not sure about what the most intuitive behaviour is. I thought that this implementation would be the best but I'm not entirely sure. I'd appreciate some more discussion on this. Thanks, Denton > Best Wishes > > Phillip > > > + } > > return update_ref_status; > > }
On Fri, Apr 03, 2020 at 06:31:26AM -0400, Denton Liu wrote: > > > diff --git a/builtin/reset.c b/builtin/reset.c > > > index 18228c312e..038c8532eb 100644 > > > --- a/builtin/reset.c > > > +++ b/builtin/reset.c > > > @@ -25,6 +25,7 @@ > > > #include "cache-tree.h" > > > #include "submodule.h" > > > #include "submodule-config.h" > > > +#include "sequencer.h" > > > #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) > > > @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > > > if (reset_type == HARD && !update_ref_status && !quiet) > > > print_new_head_line(lookup_commit_reference(the_repository, &oid)); > > > } > > > - if (!pathspec.nr) > > > + if (!pathspec.nr) { > > > + if (reset_type == HARD) > > > + save_autostash(git_path_merge_autostash(the_repository)); > > > + > > > remove_branch_state(the_repository, 0); > > > > This removes the autostash file for all reset types but we only keep the > > stash in the case of 'reset --hard' which is confusing. > > I was worried that this change would be controversial... The rationale > behind this change was that with `reset --hard`, we want to leave a > clean working tree behind so we save it into the stash reflog. In all > other cases, remove_branch_state() will apply the saved stash entry > which should be fine since users don't expect a clean worktree with the > other reset types. > > I considered saving the autostash in all cases of reset but > `git merge --abort` invokes `git reset --merge` behind the scenes so > we'd have to consider that. Perhaps we can make all resets save the > stash entry and, in the case of `merge --abort`, we can add some extra > logic to subvert this so that the stash entry is applied? Perhaps something like this? -- >8 -- commit 14d0b569cb7675f00d32d3d7fad7564fcaeca458 Author: Denton Liu <liu.denton@gmail.com> Date: Fri Apr 3 06:50:34 2020 -0400 squash! merge: teach --autostash option Stash is saved when any reset is run, when git merge --abort is run, stash is applied. diff --git a/builtin/merge.c b/builtin/merge.c index 9573d77096..31b82d614c 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -1242,6 +1242,8 @@ static int merging_a_throwaway_tag(struct commit *commit) return is_throwaway_tag; } +static GIT_PATH_FUNC(git_path_merge_autostash_saved, "MERGE_AUTOSTASH_SAVED") + int cmd_merge(int argc, const char **argv, const char *prefix) { struct object_id result_tree, stash, head_oid; @@ -1295,9 +1297,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (!file_exists(git_path_merge_head(the_repository))) die(_("There is no merge to abort (MERGE_HEAD missing).")); + if (file_exists(git_path_merge_autostash(the_repository))) { + if (rename(git_path_merge_autostash(the_repository), + git_path_merge_autostash_saved())) + die_errno(_("failed to rename autostash")); + } + /* Invoke 'git reset --merge' */ ret = cmd_reset(nargc, nargv, prefix); - apply_autostash(git_path_merge_autostash(the_repository)); + + apply_autostash(git_path_merge_autostash_saved()); goto done; } diff --git a/builtin/reset.c b/builtin/reset.c index 038c8532eb..060470c455 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -439,9 +439,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) print_new_head_line(lookup_commit_reference(the_repository, &oid)); } if (!pathspec.nr) { - if (reset_type == HARD) - save_autostash(git_path_merge_autostash(the_repository)); - + save_autostash(git_path_merge_autostash(the_repository)); remove_branch_state(the_repository, 0); } > > I'm not sure about what the most intuitive behaviour is. I thought that > this implementation would be the best but I'm not entirely sure. I'd > appreciate some more discussion on this. > > Thanks, > > Denton > > > Best Wishes > > > > Phillip > > > > > + } > > > return update_ref_status; > > > }
Hi Denton On 03/04/2020 11:56, Denton Liu wrote: > On Fri, Apr 03, 2020 at 06:31:26AM -0400, Denton Liu wrote: >>>> diff --git a/builtin/reset.c b/builtin/reset.c >>>> index 18228c312e..038c8532eb 100644 >>>> --- a/builtin/reset.c >>>> +++ b/builtin/reset.c >>>> @@ -25,6 +25,7 @@ >>>> #include "cache-tree.h" >>>> #include "submodule.h" >>>> #include "submodule-config.h" >>>> +#include "sequencer.h" >>>> #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) >>>> @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) >>>> if (reset_type == HARD && !update_ref_status && !quiet) >>>> print_new_head_line(lookup_commit_reference(the_repository, &oid)); >>>> } >>>> - if (!pathspec.nr) >>>> + if (!pathspec.nr) { >>>> + if (reset_type == HARD) >>>> + save_autostash(git_path_merge_autostash(the_repository)); >>>> + >>>> remove_branch_state(the_repository, 0); >>> >>> This removes the autostash file for all reset types but we only keep the >>> stash in the case of 'reset --hard' which is confusing. >> >> I was worried that this change would be controversial... The rationale >> behind this change was that with `reset --hard`, we want to leave a >> clean working tree behind so we save it into the stash reflog. In all >> other cases, remove_branch_state() will apply the saved stash entry >> which should be fine since users don't expect a clean worktree with the >> other reset types. >> >> I considered saving the autostash in all cases of reset but >> `git merge --abort` invokes `git reset --merge` behind the scenes so >> we'd have to consider that. Perhaps we can make all resets save the >> stash entry and, in the case of `merge --abort`, we can add some extra >> logic to subvert this so that the stash entry is applied? > > Perhaps something like this? > > -- >8 -- > commit 14d0b569cb7675f00d32d3d7fad7564fcaeca458 > Author: Denton Liu <liu.denton@gmail.com> > Date: Fri Apr 3 06:50:34 2020 -0400 > > squash! merge: teach --autostash option > > Stash is saved when any reset is run, when git merge --abort is run, > stash is applied. I think this is the easiest behavior to understand, it avoids changing the behavior of reset, in particular it stops 'reset --mixed/--soft' from suddenly starting to touch the working tree. > diff --git a/builtin/merge.c b/builtin/merge.c > index 9573d77096..31b82d614c 100644 > --- a/builtin/merge.c > +++ b/builtin/merge.c > @@ -1242,6 +1242,8 @@ static int merging_a_throwaway_tag(struct commit *commit) > return is_throwaway_tag; > } > > +static GIT_PATH_FUNC(git_path_merge_autostash_saved, "MERGE_AUTOSTASH_SAVED") > + > int cmd_merge(int argc, const char **argv, const char *prefix) > { > struct object_id result_tree, stash, head_oid; > @@ -1295,9 +1297,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > if (!file_exists(git_path_merge_head(the_repository))) > die(_("There is no merge to abort (MERGE_HEAD missing).")); > > + if (file_exists(git_path_merge_autostash(the_repository))) { > + if (rename(git_path_merge_autostash(the_repository), > + git_path_merge_autostash_saved())) > + die_errno(_("failed to rename autostash")); This is a bit of a performance, can't we just remember the stash oid in a variable and tweak the apply code? > + } > + > /* Invoke 'git reset --merge' */ > ret = cmd_reset(nargc, nargv, prefix); > - apply_autostash(git_path_merge_autostash(the_repository)); > + > + apply_autostash(git_path_merge_autostash_saved()); Calling cmd_reset() was already a bit dodgy by our normal rules, now we're calling other functions after it though I guess given the current autostash implementation it's mostly in a separate process. I think this is a good direction to go in BTW what message gets printed when the stash is saved? Best Wishes Phillip > goto done; > } > > diff --git a/builtin/reset.c b/builtin/reset.c > index 038c8532eb..060470c455 100644 > --- a/builtin/reset.c > +++ b/builtin/reset.c > @@ -439,9 +439,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > print_new_head_line(lookup_commit_reference(the_repository, &oid)); > } > if (!pathspec.nr) { > - if (reset_type == HARD) > - save_autostash(git_path_merge_autostash(the_repository)); > - > + save_autostash(git_path_merge_autostash(the_repository)); > remove_branch_state(the_repository, 0); > } > >> >> I'm not sure about what the most intuitive behaviour is. I thought that >> this implementation would be the best but I'm not entirely sure. I'd >> appreciate some more discussion on this. >> >> Thanks, >> >> Denton >> >>> Best Wishes >>> >>> Phillip >>> >>>> + } >>>> return update_ref_status; >>>> }
Hi Denton On 03/04/2020 11:31, Denton Liu wrote: > Hi Phillip, > > Thanks for your detailed review on the series. I've updated everything > according to your comments except for the parts below: That's great, I've added a few more comments below > On Thu, Apr 02, 2020 at 04:24:54PM +0100, Phillip Wood wrote: >>> diff --git a/branch.c b/branch.c >>> index 579494738a..bf2536c70d 100644 >>> --- a/branch.c >>> +++ b/branch.c >>> @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r) >>> unlink(git_path_merge_rr(r)); >>> unlink(git_path_merge_msg(r)); >>> unlink(git_path_merge_mode(r)); >>> + apply_autostash(git_path_merge_autostash(r)); >>> } >>> void remove_branch_state(struct repository *r, int verbose) >>> diff --git a/builtin/commit.c b/builtin/commit.c >>> index 7ba33a3bec..c11894423a 100644 >>> --- a/builtin/commit.c >>> +++ b/builtin/commit.c >>> @@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) >>> unlink(git_path_merge_mode(the_repository)); >>> unlink(git_path_squash_msg(the_repository)); >>> + apply_autostash(git_path_merge_autostash(the_repository)); >>> + >> >> It's hard to tell from the limited context but do we want to run >> commit_index_files() before applying the autostash? > > I don't think it really matters which order we run it in. When we run > apply_autostash(), we only ever touch the working tree, not the index so > it doesn't matter if it's run before or after. I'd prefer to keep it > here because if we ever refactor this to use > remove_merge_branch_state(), the person working on this will be able to > perform the refactor more easily without having to worry about implicit > ordering dependencies. Thinking a bit more about this we definitely want to commit the index files before applying the stash as that is done in a separate process so we want our index written to disk first. 'git stash apply' does touch the index while it's merging and then tries to reset it to the pre-merge state if the merge has no conflicts. If the user runs git add new-file git merge --autostash branch and new-file is not in branch then 'git stash apply' will add new-file to the index >> I wonder if this should >> be using remove_merge_branch_state() instead of duplicating it as well. > > We can _almost_ use remove_branch_state() even! Unfortunately, > remove_merge_branch_state() calls `unlink(git_path_merge_rr(r))` which > we cannot do since repo_rerere() relies on it later. Perhaps we can > can move repo_rerere() earlier? I think we should move apply_autostash() down so that repo_rerere() is called with the index that we've committed before we apply the stash which might add conflicts or new files. We should probably run the post-commit and post-rewrite hooks before we apply the autostash as well to avoid changing the existing behaviour. If we aim for something like commit_index_files() repo_rerere() stash_oid = read_autostash_oid() cleanup_branch_state() run_post_commit_hook() run_post_rewrite_hook() print_commit_summary() apply_autostash(stash_oid) Then we keep the existing behaviour for rerere and the hooks, and the commit summary is printed before all the status junk that apply_autostash() spews out We probably need to check the where 'git merge' applies the autostash as well to make sure it is writing the index to disk, calling the post-merge hook and printing any messages before applying the stash Best Wishes Phillip >>> diff --git a/builtin/reset.c b/builtin/reset.c >>> index 18228c312e..038c8532eb 100644 >>> --- a/builtin/reset.c >>> +++ b/builtin/reset.c >>> @@ -25,6 +25,7 @@ >>> #include "cache-tree.h" >>> #include "submodule.h" >>> #include "submodule-config.h" >>> +#include "sequencer.h" >>> #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) >>> @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) >>> if (reset_type == HARD && !update_ref_status && !quiet) >>> print_new_head_line(lookup_commit_reference(the_repository, &oid)); >>> } >>> - if (!pathspec.nr) >>> + if (!pathspec.nr) { >>> + if (reset_type == HARD) >>> + save_autostash(git_path_merge_autostash(the_repository)); >>> + >>> remove_branch_state(the_repository, 0); >> >> This removes the autostash file for all reset types but we only keep the >> stash in the case of 'reset --hard' which is confusing. > > I was worried that this change would be controversial... The rationale > behind this change was that with `reset --hard`, we want to leave a > clean working tree behind so we save it into the stash reflog. In all > other cases, remove_branch_state() will apply the saved stash entry > which should be fine since users don't expect a clean worktree with the > other reset types. > > I considered saving the autostash in all cases of reset but > `git merge --abort` invokes `git reset --merge` behind the scenes so > we'd have to consider that. Perhaps we can make all resets save the > stash entry and, in the case of `merge --abort`, we can add some extra > logic to subvert this so that the stash entry is applied? > > I'm not sure about what the most intuitive behaviour is. I thought that > this implementation would be the best but I'm not entirely sure. I'd > appreciate some more discussion on this. > > Thanks, > > Denton > >> Best Wishes >> >> Phillip >> >>> + } >>> return update_ref_status; >>> }
Hi Phillip, On Fri, Apr 03, 2020 at 02:09:26PM +0100, Phillip Wood wrote: > Hi Denton > > On 03/04/2020 11:56, Denton Liu wrote: > > On Fri, Apr 03, 2020 at 06:31:26AM -0400, Denton Liu wrote: > > > > > diff --git a/builtin/reset.c b/builtin/reset.c > > > > > index 18228c312e..038c8532eb 100644 > > > > > --- a/builtin/reset.c > > > > > +++ b/builtin/reset.c > > > > > @@ -25,6 +25,7 @@ > > > > > #include "cache-tree.h" > > > > > #include "submodule.h" > > > > > #include "submodule-config.h" > > > > > +#include "sequencer.h" > > > > > #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) > > > > > @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > > > > > if (reset_type == HARD && !update_ref_status && !quiet) > > > > > print_new_head_line(lookup_commit_reference(the_repository, &oid)); > > > > > } > > > > > - if (!pathspec.nr) > > > > > + if (!pathspec.nr) { > > > > > + if (reset_type == HARD) > > > > > + save_autostash(git_path_merge_autostash(the_repository)); > > > > > + > > > > > remove_branch_state(the_repository, 0); > > > > > > > > This removes the autostash file for all reset types but we only keep the > > > > stash in the case of 'reset --hard' which is confusing. > > > > > > I was worried that this change would be controversial... The rationale > > > behind this change was that with `reset --hard`, we want to leave a > > > clean working tree behind so we save it into the stash reflog. In all > > > other cases, remove_branch_state() will apply the saved stash entry > > > which should be fine since users don't expect a clean worktree with the > > > other reset types. > > > > > > I considered saving the autostash in all cases of reset but > > > `git merge --abort` invokes `git reset --merge` behind the scenes so > > > we'd have to consider that. Perhaps we can make all resets save the > > > stash entry and, in the case of `merge --abort`, we can add some extra > > > logic to subvert this so that the stash entry is applied? > > > > Perhaps something like this? > > > > -- >8 -- > > commit 14d0b569cb7675f00d32d3d7fad7564fcaeca458 > > Author: Denton Liu <liu.denton@gmail.com> > > Date: Fri Apr 3 06:50:34 2020 -0400 > > > > squash! merge: teach --autostash option > > Stash is saved when any reset is run, when git merge --abort is run, > > stash is applied. > > I think this is the easiest behavior to understand, it avoids changing the > behavior of reset, in particular it stops 'reset --mixed/--soft' from > suddenly starting to touch the working tree. Great, I'll use this approach then. > > diff --git a/builtin/merge.c b/builtin/merge.c > > index 9573d77096..31b82d614c 100644 > > --- a/builtin/merge.c > > +++ b/builtin/merge.c > > @@ -1242,6 +1242,8 @@ static int merging_a_throwaway_tag(struct commit *commit) > > return is_throwaway_tag; > > } > > +static GIT_PATH_FUNC(git_path_merge_autostash_saved, "MERGE_AUTOSTASH_SAVED") > > + > > int cmd_merge(int argc, const char **argv, const char *prefix) > > { > > struct object_id result_tree, stash, head_oid; > > @@ -1295,9 +1297,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix) > > if (!file_exists(git_path_merge_head(the_repository))) > > die(_("There is no merge to abort (MERGE_HEAD missing).")); > > + if (file_exists(git_path_merge_autostash(the_repository))) { > > + if (rename(git_path_merge_autostash(the_repository), > > + git_path_merge_autostash_saved())) > > + die_errno(_("failed to rename autostash")); > > This is a bit of a performance, can't we just remember the stash oid in a > variable and tweak the apply code? This makes me a little bit sad because we basically have to duplicate the file-handling portion of apply_save_autostash() but I agree that doing your way should be better. > > + } > > + > > /* Invoke 'git reset --merge' */ > > ret = cmd_reset(nargc, nargv, prefix); > > - apply_autostash(git_path_merge_autostash(the_repository)); > > + > > + apply_autostash(git_path_merge_autostash_saved()); > > Calling cmd_reset() was already a bit dodgy by our normal rules, now we're > calling other functions after it though I guess given the current autostash > implementation it's mostly in a separate process. > > I think this is a good direction to go in > BTW what message gets printed when the stash is saved? This is the message that is printed: $ git reset --hard HEAD is now at c4c4222 commit 1 Autostash exists; creating a new stash entry. Your changes are safe in the stash. You can run "git stash pop" or "git stash drop" at any time. Thanks, Denton > Best Wishes > > Phillip > > > goto done; > > } > > diff --git a/builtin/reset.c b/builtin/reset.c > > index 038c8532eb..060470c455 100644 > > --- a/builtin/reset.c > > +++ b/builtin/reset.c > > @@ -439,9 +439,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix) > > print_new_head_line(lookup_commit_reference(the_repository, &oid)); > > } > > if (!pathspec.nr) { > > - if (reset_type == HARD) > > - save_autostash(git_path_merge_autostash(the_repository)); > > - > > + save_autostash(git_path_merge_autostash(the_repository)); > > remove_branch_state(the_repository, 0); > > } > > > > > > I'm not sure about what the most intuitive behaviour is. I thought that > > > this implementation would be the best but I'm not entirely sure. I'd > > > appreciate some more discussion on this. > > > > > > Thanks, > > > > > > Denton > > > > > > > Best Wishes > > > > > > > > Phillip > > > > > > > > > + } > > > > > return update_ref_status; > > > > > }
Hi Phillip, On Fri, Apr 03, 2020 at 02:34:30PM +0100, Phillip Wood wrote: > > On Thu, Apr 02, 2020 at 04:24:54PM +0100, Phillip Wood wrote: > > > > diff --git a/branch.c b/branch.c > > > > index 579494738a..bf2536c70d 100644 > > > > --- a/branch.c > > > > +++ b/branch.c > > > > @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r) > > > > unlink(git_path_merge_rr(r)); > > > > unlink(git_path_merge_msg(r)); > > > > unlink(git_path_merge_mode(r)); > > > > + apply_autostash(git_path_merge_autostash(r)); > > > > } > > > > void remove_branch_state(struct repository *r, int verbose) > > > > diff --git a/builtin/commit.c b/builtin/commit.c > > > > index 7ba33a3bec..c11894423a 100644 > > > > --- a/builtin/commit.c > > > > +++ b/builtin/commit.c > > > > @@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) > > > > unlink(git_path_merge_mode(the_repository)); > > > > unlink(git_path_squash_msg(the_repository)); > > > > + apply_autostash(git_path_merge_autostash(the_repository)); > > > > + > > > > > > It's hard to tell from the limited context but do we want to run > > > commit_index_files() before applying the autostash? > > > > I don't think it really matters which order we run it in. When we run > > apply_autostash(), we only ever touch the working tree, not the index so > > it doesn't matter if it's run before or after. I'd prefer to keep it > > here because if we ever refactor this to use > > remove_merge_branch_state(), the person working on this will be able to > > perform the refactor more easily without having to worry about implicit > > ordering dependencies. > > Thinking a bit more about this we definitely want to commit the index files > before applying the stash as that is done in a separate process so we want > our index written to disk first. > > 'git stash apply' does touch the index while it's merging and then tries to > reset it to the pre-merge state if the merge has no conflicts. If the user > runs > git add new-file > git merge --autostash branch > and new-file is not in branch then 'git stash apply' will add new-file to > the index > > > > > I wonder if this should > > > be using remove_merge_branch_state() instead of duplicating it as well. > > > > We can _almost_ use remove_branch_state() even! Unfortunately, > > remove_merge_branch_state() calls `unlink(git_path_merge_rr(r))` which > > we cannot do since repo_rerere() relies on it later. Perhaps we can > > can move repo_rerere() earlier? > > I think we should move apply_autostash() down so that repo_rerere() is > called with the index that we've committed before we apply the stash which > might add conflicts or new files. We should probably run the post-commit and > post-rewrite hooks before we apply the autostash as well to avoid changing > the existing behaviour. > > If we aim for something like > commit_index_files() > repo_rerere() > stash_oid = read_autostash_oid() > cleanup_branch_state() > run_post_commit_hook() > run_post_rewrite_hook() > print_commit_summary() > apply_autostash(stash_oid) > > Then we keep the existing behaviour for rerere and the hooks, and the commit > summary is printed before all the status junk that apply_autostash() spews > out Makes sense, I'll make this change. > We probably need to check the where 'git merge' applies the autostash as > well to make sure it is writing the index to disk, calling the post-merge > hook and printing any messages before applying the stash It currently applies the autostash in finish(). After reviewing it, I've moved the apply_autostash() call to the end of that function. Actually, going back to review some more callsites of apply_autostash(), the one in remove_merge_branch_state() doesn't really make sense. We should really be calling save_autostash() in there so that we don't accidentally dirty the worktree unexpectedly. The bonus to this is that I've removed the save_autostash() call in `git reset` so it should just be able to call remove_branch_state() and have everything just work. As a result of these changes, we now only introduce three callsites of apply_autostash(), which are all explicit: 1. At the end of `git commit` 2. At the end of finish() in `git merge` 3. At the end of `git merge --abort` I'll send up a new version later today. Thanks for helping shape this topic up, Denton > Best Wishes > > Phillip
diff --git a/Documentation/config/merge.txt b/Documentation/config/merge.txt index 6a313937f8..88b29127bf 100644 --- a/Documentation/config/merge.txt +++ b/Documentation/config/merge.txt @@ -70,6 +70,16 @@ merge.stat:: Whether to print the diffstat between ORIG_HEAD and the merge result at the end of the merge. True by default. +merge.autoStash:: + When set to true, automatically create a temporary stash entry + before the operation begins, and apply it after the operation + ends. This means that you can run rebase on a dirty worktree. + However, use with care: the final stash application after a + successful rebase might result in non-trivial conflicts. + This option can be overridden by the `--no-autostash` and + `--autostash` options of linkgit:git-merge[1]. + Defaults to false. + merge.tool:: Controls which merge tool is used by linkgit:git-mergetool[1]. The list below shows the valid built-in values. diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt index 40dc4f5e8c..34493eb58b 100644 --- a/Documentation/merge-options.txt +++ b/Documentation/merge-options.txt @@ -155,6 +155,14 @@ ifndef::git-pull[] Note that not all merge strategies may support progress reporting. +--autostash:: +--no-autostash:: + Automatically create a temporary stash entry before the operation + begins, and apply it after the operation ends. This means + that you can run rebase on a dirty worktree. However, use + with care: the final stash application after a successful + rebase might result in non-trivial conflicts. + endif::git-pull[] --allow-unrelated-histories:: diff --git a/branch.c b/branch.c index 579494738a..bf2536c70d 100644 --- a/branch.c +++ b/branch.c @@ -344,6 +344,7 @@ void remove_merge_branch_state(struct repository *r) unlink(git_path_merge_rr(r)); unlink(git_path_merge_msg(r)); unlink(git_path_merge_mode(r)); + apply_autostash(git_path_merge_autostash(r)); } void remove_branch_state(struct repository *r, int verbose) diff --git a/builtin/commit.c b/builtin/commit.c index 7ba33a3bec..c11894423a 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1687,6 +1687,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) unlink(git_path_merge_mode(the_repository)); unlink(git_path_squash_msg(the_repository)); + apply_autostash(git_path_merge_autostash(the_repository)); + if (commit_index_files()) die(_("repository has been updated, but unable to write\n" "new_index file. Check that disk is not full and quota is\n" diff --git a/builtin/merge.c b/builtin/merge.c index d127d2225f..e038bef5ad 100644 --- a/builtin/merge.c +++ b/builtin/merge.c @@ -82,6 +82,7 @@ static int show_progress = -1; static int default_to_upstream = 1; static int signoff; static const char *sign_commit; +static int autostash; static int no_verify; static struct strategy all_strategy[] = { @@ -286,6 +287,8 @@ static struct option builtin_merge_options[] = { OPT_SET_INT(0, "progress", &show_progress, N_("force progress reporting"), 1), { OPTION_STRING, 'S', "gpg-sign", &sign_commit, N_("key-id"), N_("GPG sign commit"), PARSE_OPT_OPTARG, NULL, (intptr_t) "" }, + OPT_BOOL(0, "autostash", &autostash, + N_("automatically stash/stash pop before and after")), OPT_BOOL(0, "overwrite-ignore", &overwrite_ignore, N_("update ignored files (default)")), OPT_BOOL(0, "signoff", &signoff, N_("add Signed-off-by:")), OPT_BOOL(0, "no-verify", &no_verify, N_("bypass pre-merge-commit and commit-msg hooks")), @@ -441,6 +444,7 @@ static void finish(struct commit *head_commit, strbuf_addf(&reflog_message, "%s: %s", getenv("GIT_REFLOG_ACTION"), msg); } + apply_autostash(git_path_merge_autostash(the_repository)); if (squash) { squash_message(head_commit, remoteheads); } else { @@ -634,6 +638,9 @@ static int git_merge_config(const char *k, const char *v, void *cb) return 0; } else if (!strcmp(k, "gpg.mintrustlevel")) { check_trust_level = 0; + } else if (!strcmp(k, "merge.autostash")) { + autostash = git_config_bool(k, v); + return 0; } status = fmt_merge_msg_config(k, v, cb); @@ -1291,6 +1298,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix) /* Invoke 'git reset --merge' */ ret = cmd_reset(nargc, nargv, prefix); + apply_autostash(git_path_merge_autostash(the_repository)); goto done; } @@ -1513,6 +1521,10 @@ int cmd_merge(int argc, const char **argv, const char *prefix) goto done; } + if (autostash) + create_autostash(the_repository, + git_path_merge_autostash(the_repository), + "merge"); if (checkout_fast_forward(the_repository, &head_commit->object.oid, &commit->object.oid, @@ -1579,6 +1591,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix) if (fast_forward == FF_ONLY) die(_("Not possible to fast-forward, aborting.")); + if (autostash) + create_autostash(the_repository, + git_path_merge_autostash(the_repository), + "merge"); + /* We are going to make a new commit. */ git_committer_info(IDENT_STRICT); diff --git a/builtin/reset.c b/builtin/reset.c index 18228c312e..038c8532eb 100644 --- a/builtin/reset.c +++ b/builtin/reset.c @@ -25,6 +25,7 @@ #include "cache-tree.h" #include "submodule.h" #include "submodule-config.h" +#include "sequencer.h" #define REFRESH_INDEX_DELAY_WARNING_IN_MS (2 * 1000) @@ -437,8 +438,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) if (reset_type == HARD && !update_ref_status && !quiet) print_new_head_line(lookup_commit_reference(the_repository, &oid)); } - if (!pathspec.nr) + if (!pathspec.nr) { + if (reset_type == HARD) + save_autostash(git_path_merge_autostash(the_repository)); + remove_branch_state(the_repository, 0); + } return update_ref_status; } diff --git a/path.c b/path.c index 88cf593007..d764738146 100644 --- a/path.c +++ b/path.c @@ -1535,5 +1535,6 @@ REPO_GIT_PATH_FUNC(merge_msg, "MERGE_MSG") REPO_GIT_PATH_FUNC(merge_rr, "MERGE_RR") REPO_GIT_PATH_FUNC(merge_mode, "MERGE_MODE") REPO_GIT_PATH_FUNC(merge_head, "MERGE_HEAD") +REPO_GIT_PATH_FUNC(merge_autostash, "MERGE_AUTOSTASH") REPO_GIT_PATH_FUNC(fetch_head, "FETCH_HEAD") REPO_GIT_PATH_FUNC(shallow, "shallow") diff --git a/path.h b/path.h index 14d6dcad16..1f1bf8f87a 100644 --- a/path.h +++ b/path.h @@ -177,11 +177,12 @@ struct path_cache { const char *merge_rr; const char *merge_mode; const char *merge_head; + const char *merge_autostash; const char *fetch_head; const char *shallow; }; -#define PATH_CACHE_INIT { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL } +#define PATH_CACHE_INIT { NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL, NULL } const char *git_path_cherry_pick_head(struct repository *r); const char *git_path_revert_head(struct repository *r); @@ -190,6 +191,7 @@ const char *git_path_merge_msg(struct repository *r); const char *git_path_merge_rr(struct repository *r); const char *git_path_merge_mode(struct repository *r); const char *git_path_merge_head(struct repository *r); +const char *git_path_merge_autostash(struct repository *r); const char *git_path_fetch_head(struct repository *r); const char *git_path_shallow(struct repository *r); diff --git a/t/t3033-merge-toplevel.sh b/t/t3033-merge-toplevel.sh index d314599428..e29c284b9b 100755 --- a/t/t3033-merge-toplevel.sh +++ b/t/t3033-merge-toplevel.sh @@ -142,6 +142,17 @@ test_expect_success 'refuse two-project merge by default' ' test_must_fail git merge five ' +test_expect_success 'refuse two-project merge by default, quit before --autostash happens' ' + t3033_reset && + git reset --hard four && + echo change >>one.t && + git diff >expect && + test_must_fail git merge --autostash five 2>err && + test_i18ngrep ! "stash" err && + git diff >actual && + test_cmp expect actual +' + test_expect_success 'two-project merge with --allow-unrelated-histories' ' t3033_reset && git reset --hard four && @@ -149,4 +160,15 @@ test_expect_success 'two-project merge with --allow-unrelated-histories' ' git diff --exit-code five ' +test_expect_success 'two-project merge with --allow-unrelated-histories with --autostash' ' + t3033_reset && + git reset --hard four && + echo change >>one.t && + git diff one.t >expect && + git merge --allow-unrelated-histories --autostash five 2>err && + test_i18ngrep "Applied autostash." err && + git diff one.t >actual && + test_cmp expect actual +' + test_done diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index 4fa0ef8e3b..c08e4315f4 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -30,13 +30,17 @@ Testing basic merge operations/option parsing. . "$TEST_DIRECTORY"/lib-gpg.sh test_write_lines 1 2 3 4 5 6 7 8 9 >file +cp file file.orig test_write_lines '1 X' 2 3 4 5 6 7 8 9 >file.1 +test_write_lines 1 2 '3 X' 4 5 6 7 8 9 >file.3 test_write_lines 1 2 3 4 '5 X' 6 7 8 9 >file.5 test_write_lines 1 2 3 4 5 6 7 8 '9 X' >file.9 test_write_lines 1 2 3 4 5 6 7 8 '9 Y' >file.9y test_write_lines '1 X' 2 3 4 5 6 7 8 9 >result.1 test_write_lines '1 X' 2 3 4 '5 X' 6 7 8 9 >result.1-5 +test_write_lines '1 X' 2 3 4 5 6 7 8 '9 X' >result.1-9 test_write_lines '1 X' 2 3 4 '5 X' 6 7 8 '9 X' >result.1-5-9 +test_write_lines '1 X' 2 '3 X' 4 '5 X' 6 7 8 '9 X' >result.1-3-5-9 test_write_lines 1 2 3 4 5 6 7 8 '9 Z' >result.9z create_merge_msgs () { @@ -675,6 +679,106 @@ test_expect_success 'refresh the index before merging' ' git merge c3 ' +test_expect_success 'merge with --autostash' ' + git reset --hard c1 && + git merge-file file file.orig file.9 && + git merge --autostash c2 2>err && + test_i18ngrep "Applied autostash." err && + git show HEAD:file >merge-result && + test_cmp result.1-5 merge-result && + test_cmp result.1-5-9 file +' + +test_expect_success 'merge with merge.autoStash' ' + test_config merge.autoStash true && + git reset --hard c1 && + git merge-file file file.orig file.9 && + git merge c2 2>err && + test_i18ngrep "Applied autostash." err && + git show HEAD:file >merge-result && + test_cmp result.1-5 merge-result && + test_cmp result.1-5-9 file +' + +test_expect_success 'fast-forward merge with --autostash' ' + git reset --hard c0 && + git merge-file file file.orig file.5 && + git merge --autostash c1 2>err && + test_i18ngrep "Applied autostash." err && + test_cmp result.1-5 file +' + +test_expect_success 'octopus merge with --autostash' ' + git reset --hard c1 && + git merge-file file file.orig file.3 && + git merge --autostash c2 c3 2>err && + test_i18ngrep "Applied autostash." err && + git show HEAD:file >merge-result && + test_cmp result.1-5-9 merge-result && + test_cmp result.1-3-5-9 file +' + +test_expect_success 'conflicted merge with --autostash, --abort restores stash' ' + git reset --hard c3 && + cp file.1 file && + test_must_fail git merge --autostash c7 && + git merge --abort 2>err && + test_i18ngrep "Applied autostash." err && + test_cmp file.1 file +' + +test_expect_success 'completed merge with --no-commit and --autostash' ' + git reset --hard c1 && + git merge-file file file.orig file.9 && + git diff >expect && + git merge --no-commit --autostash c2 && + git stash show -p MERGE_AUTOSTASH >actual && + test_cmp expect actual && + git commit 2>err && + test_i18ngrep "Applied autostash." err && + git show HEAD:file >merge-result && + test_cmp result.1-5 merge-result && + test_cmp result.1-5-9 file +' + +test_expect_success 'aborted merge (merge --abort) with --no-commit and --autostash' ' + git reset --hard c1 && + git merge-file file file.orig file.9 && + git diff >expect && + git merge --no-commit --autostash c2 && + git stash show -p MERGE_AUTOSTASH >actual && + test_cmp expect actual && + git merge --abort 2>err && + test_i18ngrep "Applied autostash." err && + git diff >actual && + test_cmp expect actual +' + +test_expect_success 'aborted merge (reset --hard) with --no-commit and --autostash' ' + git reset --hard c1 && + git merge-file file file.orig file.9 && + git diff >expect && + git merge --no-commit --autostash c2 && + git stash show -p MERGE_AUTOSTASH >actual && + test_cmp expect actual && + git reset --hard 2>err && + test_i18ngrep "Autostash exists; creating a new stash entry." err && + git diff --exit-code +' + +test_expect_success 'merge with conflicted --autostash changes' ' + git reset --hard c1 && + git merge-file file file.orig file.9y && + git diff >expect && + test_when_finished "test_might_fail git stash drop" && + git merge --autostash c3 2>err && + test_i18ngrep "Applying autostash resulted in conflicts." err && + git show HEAD:file >merge-result && + test_cmp result.1-9 merge-result && + git stash show -p >actual && + test_cmp expect actual +' + cat >expected.branch <<\EOF Merge branch 'c5-branch' (early part) EOF
In rebase, one can pass the `--autostash` option to cause the worktree to be automatically stashed before continuing with the rebase. This option is missing in merge, however. Implement the `--autostash` option and corresponding `merge.autoStash` option in merge which stashes before merging and then pops after. This option is useful when a developer has some local changes on a topic branch but they realize that their work depends on another branch. Previously, they had to run something like git fetch ... git stash push git merge FETCH_HEAD git stash pop but now, that is reduced to git fetch ... git merge --autostash FETCH_HEAD When `git reset --hard` is run to abort a merge, the working tree will be left in a clean state, as expected, with the autostash pushed onto the stash stack. Suggested-by: Alban Gruin <alban.gruin@gmail.com> Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/config/merge.txt | 10 +++ Documentation/merge-options.txt | 8 +++ branch.c | 1 + builtin/commit.c | 2 + builtin/merge.c | 17 ++++++ builtin/reset.c | 7 ++- path.c | 1 + path.h | 4 +- t/t3033-merge-toplevel.sh | 22 +++++++ t/t7600-merge.sh | 104 ++++++++++++++++++++++++++++++++ 10 files changed, 174 insertions(+), 2 deletions(-)