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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jan 10, 2020, 14:44 UTC
Message-ID: <f545a13f-4b3c-9603-7511-ff80ad950ee9@gmail.com>
In-Reply-To: <20200108060842.GA51465@generichostname>

Hi Denton

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

Show 14 quoted lines

> 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?

Show 11 quoted lines

> 	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

Show 36 quoted lines

> 	+	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