| Message ID | cover.1577185374.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | merge: learn --autostash | expand |
Denton Liu <liu.denton@gmail.com> writes: > Alban reported[1] that he expected merge to have an --autostash option, > just like rebase. Since there's not really any reason why merge can't > have it, let's implement it in this patchset. Sigh. I would actually have preferred to remove the autostash from rebase if the goal is to have a consistent behaviour between the two, though ;-) But it is OK as long as it does not degrade the codepath with changes that wouldn't have been necessary if this weren't added. Let's see how it goes. Queued.
On 30/12/2019 21:49, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > >> Alban reported[1] that he expected merge to have an --autostash option, >> just like rebase. Since there's not really any reason why merge can't >> have it, let's implement it in this patchset. > > Sigh. > > I would actually have preferred to remove the autostash from rebase > if the goal is to have a consistent behaviour between the two, > though ;-) I find the --autostash option to rebase useful if I want to edit a commit and I've got uncommitted changes. I wish it was called --stash though as it's not automatic as one has to set a config setting or pass it on the commandline. I'm also not convinced the config setting is a good idea as it allows rebase to run when I think I've committed everything but I haven't. For `merge --autostash` I think we need to consider the interaction of `--no-commit` with `--autostash` as `stash pop` will refuse to run if the merge modified any of the stashed paths. Best Wishes Phillip > But it is OK as long as it does not degrade the codepath with > changes that wouldn't have been necessary if this weren't added. > > Let's see how it goes. Queued. >
Hi Junio, On Mon, Dec 30, 2019 at 01:49:05PM -0800, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > Alban reported[1] that he expected merge to have an --autostash option, > > just like rebase. Since there's not really any reason why merge can't > > have it, let's implement it in this patchset. > > Sigh. > > I would actually have preferred to remove the autostash from rebase > if the goal is to have a consistent behaviour between the two, > though ;-) > > But it is OK as long as it does not degrade the codepath with > changes that wouldn't have been necessary if this weren't added. > > Let's see how it goes. Queued. Sorry for the radio silence for the past week. I've been offline trying to recover from a particularly bad flu. Anyway, I have a reroll of this topic planned with the `flags` change you suggested and addressing Philip's concerns about --no-commit. Since this is a new topic that definitely won't be accepted for v2.25.0, I'll probably be taking my time with this ;) Thanks, Denton
Hi Phillip, Sorry for the late reply. I'm back in school so I've been pretty busy lately. On Tue, Dec 31, 2019 at 10:34:30AM +0000, Phillip Wood wrote: > For `merge --autostash` I think we need to consider the interaction of > `--no-commit` with `--autostash` as `stash pop` will refuse to run if the > merge modified any of the stashed paths. The only time when we run the `stash pop` is when we either commit the merge or abort it. With this in mind, what do you think of the following test cases? If they look good, I can squash them into the appropriate patch and send in a reroll. diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh index e0c8a0bad4..af5db58e16 100755 --- a/t/t7600-merge.sh +++ b/t/t7600-merge.sh @@ -727,6 +727,33 @@ test_expect_success 'conflicted merge with --autostash, --abort restores stash' 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 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 'merge with conflicted --autostash changes' ' git reset --hard c1 && git merge-file file file.orig file.9y && Thanks, Denton > > Best Wishes > > Phillip
Hi Denton On 08/01/2020 06:08, Denton Liu wrote: > Hi Phillip, > > Sorry for the late reply. I'm back in school so I've been pretty busy > lately. > > On Tue, Dec 31, 2019 at 10:34:30AM +0000, Phillip Wood wrote: >> For `merge --autostash` I think we need to consider the interaction of >> `--no-commit` with `--autostash` as `stash pop` will refuse to run if the >> merge modified any of the stashed paths. > > The only time when we run the `stash pop` is when we either commit the > merge or abort it. With this in mind, what do you think of the following > test cases? If they look good, I can squash them into the appropriate > patch and send in a reroll. Ah I misunderstood what happened with --autostash and --no-commit, the tests basically look fine (I've got one comment below). One other thing - if the user runs `git reset` or `git checkout <branch>` then cleanup_branch_state() removes MERGE_HEAD, MERGE_MSG etc. If we're not already doing so then I think we should remove MERGE_AUTOSTASH as well or you can get into a situation where the user does something like git merge --autostash <something> # results in conflicts git reset --hard <somewhere else> git merge <something> # succeeds and confusingly pops previous stash Running `git reset` doesn't make sense unless they want to discard the stashed changes as well. This is a difference with rebase where you cannot lose the stashed changes by running `git reset`, the only way they can get lost is by running `rebase --quit`. We could always add a warning about it throwing away the stashed changes in the future. I still not keen on the name `--autostash` as it's not automatic. `--stash` would make more sense to me. We'd have to deprecate `rebase --autostash` in favor of `rebase --stash` to change it but it if we want to change the name it would be better now before adding `--autostash` to merge and pull - what do you think? > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > index e0c8a0bad4..af5db58e16 100755 > --- a/t/t7600-merge.sh > +++ b/t/t7600-merge.sh > @@ -727,6 +727,33 @@ test_expect_success 'conflicted merge with --autostash, --abort restores stash' > 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 && Is this a complicated way of getting some unstaged changes so we can stash them or have I missed something? Best Wishes Phillip > + 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 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 'merge with conflicted --autostash changes' ' > git reset --hard c1 && > git merge-file file file.orig file.9y && > > Thanks, > > Denton > >> >> Best Wishes >> >> Phillip
On Fri, Jan 10, 2020 at 02:44:30PM +0000, Phillip Wood wrote: > Hi Denton > > On 08/01/2020 06:08, Denton Liu wrote: > > Hi Phillip, > > > > Sorry for the late reply. I'm back in school so I've been pretty busy > > lately. > > > > On Tue, Dec 31, 2019 at 10:34:30AM +0000, Phillip Wood wrote: > > > For `merge --autostash` I think we need to consider the interaction of > > > `--no-commit` with `--autostash` as `stash pop` will refuse to run if the > > > merge modified any of the stashed paths. > > > > The only time when we run the `stash pop` is when we either commit the > > merge or abort it. With this in mind, what do you think of the following > > test cases? If they look good, I can squash them into the appropriate > > patch and send in a reroll. > > Ah I misunderstood what happened with --autostash and --no-commit, the tests > basically look fine (I've got one comment below). > > One other thing - if the user runs `git reset` or `git checkout <branch>` > then cleanup_branch_state() removes MERGE_HEAD, MERGE_MSG etc. If we're not > already doing so then I think we should remove MERGE_AUTOSTASH as well or > you can get into a situation where the user does something like In cleanup_branch_state(), I insert `apply_autostash()` at the very end of the function so that the stash is popped whenever this is called. > git merge --autostash <something> # results in conflicts > git reset --hard <somewhere else> > git merge <something> # succeeds and confusingly pops previous stash > > Running `git reset` doesn't make sense unless they want to discard the > stashed changes as well. This is a difference with rebase where you cannot > lose the stashed changes by running `git reset`, the only way they can get > lost is by running `rebase --quit`. We could always add a warning about it > throwing away the stashed changes in the future. Currently, in rebase, if the stash cannot be popped cleanly, it is automatically pushed onto the stash stack so that the user can deal with it later. Do we want to do a similar thing where if we `reset --hard` with an autostash present, we store the stash and then leave the users with a clean worktree (as they'd expect)? > I still not keen on the name `--autostash` as it's not automatic. `--stash` > would make more sense to me. We'd have to deprecate `rebase --autostash` in > favor of `rebase --stash` to change it but it if we want to change the name > it would be better now before adding `--autostash` to merge and pull - what > do you think? Even though I agree with you that `--autostash` would be better named as `--stash`, I feel that it's worse than having argument names that perform the same functionality but with different names. So I'd be inclined to keep `--autostash`. > > > diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh > > index e0c8a0bad4..af5db58e16 100755 > > --- a/t/t7600-merge.sh > > +++ b/t/t7600-merge.sh > > @@ -727,6 +727,33 @@ test_expect_success 'conflicted merge with --autostash, --abort restores stash' > > 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 && > > Is this a complicated way of getting some unstaged changes so we can stash > them or have I missed something? Yes, that's exactly what this does. > > Best Wishes > > Phillip > > > + 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 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 'merge with conflicted --autostash changes' ' > > git reset --hard c1 && > > git merge-file file file.orig file.9y && > > > > Thanks, > > > > Denton > > > > > > > > Best Wishes > > > > > > Phillip