On Fri, Jan 10, 2020 at 02:44:30PM +0000, Phillip Wood wrote:

Hi Denton

On 08/01/2020 06:08, Denton Liu wrote:

quoted

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:

quoted

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`.

quoted

	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

quoted

	+	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

quoted

Best Wishes

Phillip