Re: [PATCH v2 00/17] merge: learn --autostash

[Denton Liu <liu.denton@gmail.com>]

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