threads / patch / 53341

patchrebase: save autostash entry into stash reflog on --quit

Subject: [PATCH] rebase: save autostash entry into stash reflog on --quit

## tl;dr

5 messages between Apr 28, 2020 and Apr 30, 2020. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Denton Liu· Apr 28, 2020, 09:31 UTC · lore

In a03b55530a (merge: teach --autostash option, 2020-04-07), the --autostash option was introduced for `git merge`. Notably, when `git merge --quit` is run with an autostash entry present, it is saved into the stash reflog. This is contrasted with the current behaviour of `git rebase --quit` where the autostash entry is simply just dropped out of existence.

Adopt the behaviour of `git merge --quit` in `git rebase --quit` and save the autostash entry into the stash reflog instead of just deleting it.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Notes:
    This patch is based on 'dl/merge-autostash'.
 Documentation/git-rebase.txt |  3 ++-
 builtin/rebase.c             |  1 +
 t/t3420-rebase-autostash.sh  | 20 ++++++++++++++++++++
 3 files changed, 23 insertions(+), 1 deletion(-)
Show changes to 3 files +23 −1

Documentation/git-rebase.txt, builtin/rebase.c, t/t3420-rebase-autostash.sh

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index f7a6033607..7d0c89a184 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.
 --quit::
 	Abort the rebase operation but HEAD is not reset back to the
 	original branch. The index and working tree are also left
-	unchanged as a result.
+	unchanged as a result. If a temporary stash entry was created
+	using --autostash, it will be saved to the stash reflog.
 
 --apply:
 	Use applying strategies to rebase (calling `git-am`
diff --git a/builtin/rebase.c b/builtin/rebase.c
index bc4fc69906..71aec532b1 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1556,6 +1556,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		goto cleanup;
 	}
 	case ACTION_QUIT: {
+		save_autostash(state_dir_path("autostash", &options));
 		if (options.type == REBASE_MERGE) {
 			struct replay_opts replay = REPLAY_OPTS_INIT;
 
diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index b97ea62363..ca331733fb 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -184,6 +184,26 @@ testrebase () {
 		git checkout feature-branch
 	'
 
+	test_expect_success "rebase$type: --quit" '
+		test_config rebase.autostash true &&
+		git reset --hard &&
+		git checkout -b rebased-feature-branch feature-branch &&
+		test_when_finished git branch -D rebased-feature-branch &&
+		echo dirty >>file3 &&
+		git diff >expect &&
+		test_must_fail git rebase$type related-onto-branch &&
+		test_path_is_file $dotest/autostash &&
+		test_path_is_missing file3 &&
+		git rebase --quit &&
+		test_when_finished git stash drop &&
+		test_path_is_missing $dotest/autostash &&
+		! grep dirty file3 &&
+		git stash show -p >actual &&
+		test_cmp expect actual &&
+		git reset --hard &&
+		git checkout feature-branch
+	'
+
 	test_expect_success "rebase$type: non-conflicting rebase, conflicting stash" '
 		test_config rebase.autostash true &&
 		git reset --hard &&

-- 
2.26.0.159.g23e2136ad0
Junio C Hamano· Apr 28, 2020, 19:35 UTC · re: Denton Liu · lore

Re: [PATCH] rebase: save autostash entry into stash reflog on --quit

Denton Liu <liu.denton@gmail.com> writes:
Show 10 quoted lines
> In a03b55530a (merge: teach --autostash option, 2020-04-07), the
> --autostash option was introduced for `git merge`. Notably, when
> `git merge --quit` is run with an autostash entry present, it is saved
> into the stash reflog. This is contrasted with the current behaviour of
> `git rebase --quit` where the autostash entry is simply just dropped out
> of existence.
>
> Adopt the behaviour of `git merge --quit` in `git rebase --quit` and
> save the autostash entry into the stash reflog instead of just deleting
> it.

The goal is wrothy, I would think, but I do not think we would explain it in terms of "stash reflog". It is true that what "stash list" shows, the list of stasn entries, happens to be implemented as reflog entries of a single ref, but the end users would not view them as reflog entries, I suspect. Do you know anybody who would do "git reflog stash@{now}"?

It is less bad to explain with "reflog of stash ref" in the log message, meant to be read by our future developers, but ...

Show 11 quoted lines
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f7a6033607..7d0c89a184 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.
>  --quit::
>  	Abort the rebase operation but HEAD is not reset back to the
>  	original branch. The index and working tree are also left
> -	unchanged as a result.
> +	unchanged as a result. If a temporary stash entry was created
> +	using --autostash, it will be saved to the stash reflog.

... let's not do so for end-user facing documentation. "..., it will be stashed away". Or we may not even want to say anything; any "--autostash" user would expect that the changes that were stashed before "rebase" started would not be discarded, and this change may just be a bugfix.

Show 11 quoted lines
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index bc4fc69906..71aec532b1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1556,6 +1556,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		goto cleanup;
>  	}
>  	case ACTION_QUIT: {
> +		save_autostash(state_dir_path("autostash", &options));
>  		if (options.type == REBASE_MERGE) {
>  			struct replay_opts replay = REPLAY_OPTS_INIT;
Nice to see a fix as simple a this one ;-)
Denton Liu· Apr 29, 2020, 00:23 UTC · re: Junio C Hamano · lore

Re: [PATCH] rebase: save autostash entry into stash reflog on --quit

Hi Junio,
On Tue, Apr 28, 2020 at 12:35:15PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> > index f7a6033607..7d0c89a184 100644
> > --- a/Documentation/git-rebase.txt
> > +++ b/Documentation/git-rebase.txt
> > @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.
> >  --quit::
> >  	Abort the rebase operation but HEAD is not reset back to the
> >  	original branch. The index and working tree are also left
> > -	unchanged as a result.
> > +	unchanged as a result. If a temporary stash entry was created
> > +	using --autostash, it will be saved to the stash reflog.
> 
> ... let's not do so for end-user facing documentation.  "..., it
> will be stashed away".  Or we may not even want to say anything; any
> "--autostash" user would expect that the changes that were stashed
> before "rebase" started would not be discarded, and this change may
> just be a bugfix.

Hmm, in this case, git-merge.txt may need an update as well. From 'dl/merge-autostash', 

	'git merge --abort' is equivalent to 'git reset --merge' when
	`MERGE_HEAD` is present unless `MERGE_AUTOSTASH` is also present in
	which case 'git merge --abort' applies the stash entry to the worktree
	whereas 'git reset --merge' will save the stashed changes in the stash
	reflog.
and
	--quit::
		Forget about the current merge in progress. Leave the index
		and the working tree as-is. If `MERGE_AUTOSTASH` is present, the
		stash entry will be saved to the stash reflog.
both need to be amended to remove the reference to the "stash reflog".

When I was writing this documentation, I wanted to distinguish between the temporary autostash entry and the actual stash since the autostash entry isn't pushed to the stash unless there are conflicts or it's explicitly saved. I'm not sure that something like "If a temporary stash entry was created using --autostash, it will be stashed away" works very well since the word "stash" is overloaded here to mean "a random stash commit" and "stashed away in _the_ stash". Unfortunately, I'm also having trouble coming up with a suitable phrasing of my own.

I dunno, perhaps I'm overthinking this too and your suggested rewording sounds good and I'm just being too picky.

Thanks,
Denton
Phillip Wood· Apr 30, 2020, 09:45 UTC · re: Denton Liu · lore

Re: [PATCH] rebase: save autostash entry into stash reflog on --quit

Hi Denton
On 29/04/2020 01:23, Denton Liu wrote:
Show 38 quoted lines
> Hi Junio,
> 
> On Tue, Apr 28, 2020 at 12:35:15PM -0700, Junio C Hamano wrote:
>>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
>>> index f7a6033607..7d0c89a184 100644
>>> --- a/Documentation/git-rebase.txt
>>> +++ b/Documentation/git-rebase.txt
>>> @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.
>>>  --quit::
>>>  	Abort the rebase operation but HEAD is not reset back to the
>>>  	original branch. The index and working tree are also left
>>> -	unchanged as a result.
>>> +	unchanged as a result. If a temporary stash entry was created
>>> +	using --autostash, it will be saved to the stash reflog.
>>
>> ... let's not do so for end-user facing documentation.  "..., it
>> will be stashed away".  Or we may not even want to say anything; any
>> "--autostash" user would expect that the changes that were stashed
>> before "rebase" started would not be discarded, and this change may
>> just be a bugfix.
> 
> Hmm, in this case, git-merge.txt may need an update as well. From
> 'dl/merge-autostash', 
> 
> 	'git merge --abort' is equivalent to 'git reset --merge' when
> 	`MERGE_HEAD` is present unless `MERGE_AUTOSTASH` is also present in
> 	which case 'git merge --abort' applies the stash entry to the worktree
> 	whereas 'git reset --merge' will save the stashed changes in the stash
> 	reflog.
> 
> and
> 
> 	--quit::
> 		Forget about the current merge in progress. Leave the index
> 		and the working tree as-is. If `MERGE_AUTOSTASH` is present, the
> 		stash entry will be saved to the stash reflog.
> 
> both need to be amended to remove the reference to the "stash reflog".

I agree that it would be good to have the documentation for rebase and merge match in this case

Show 8 quoted lines
> When I was writing this documentation, I wanted to distinguish between
> the temporary autostash entry and the actual stash since the autostash
> entry isn't pushed to the stash unless there are conflicts or it's
> explicitly saved. I'm not sure that something like "If a temporary stash
> entry was created using --autostash, it will be stashed away" works very
> well since the word "stash" is overloaded here to mean "a random stash
> commit" and "stashed away in _the_ stash". Unfortunately, I'm also
> having trouble coming up with a suitable phrasing of my own.

It's tricky to distinguish between the two, when I was reviewing the merge --autostash patches I was wary of the mention of the stash reflog but could not come up with anything better to distinguish between remembering the stash oid and saving it to the list of stashes. I'm not too bothered what we end up doing so long as it is consistent across the commands.

Best Wishes
Phillip
Show 7 quoted lines
> I dunno, perhaps I'm overthinking this too and your suggested rewording
> sounds good and I'm just being too picky.
> 
> Thanks,
> 
> Denton
>
Phillip Wood· Apr 30, 2020, 09:41 UTC · re: Denton Liu · lore

Re: [PATCH] rebase: save autostash entry into stash reflog on --quit

Hi Denton
Thanks for working on this, the implementation and test look fine
Best Wishes
Phillip
On 28/04/2020 10:31, Denton Liu wrote:
Show 78 quoted lines
> In a03b55530a (merge: teach --autostash option, 2020-04-07), the
> --autostash option was introduced for `git merge`. Notably, when
> `git merge --quit` is run with an autostash entry present, it is saved
> into the stash reflog. This is contrasted with the current behaviour of
> `git rebase --quit` where the autostash entry is simply just dropped out
> of existence.
> 
> Adopt the behaviour of `git merge --quit` in `git rebase --quit` and
> save the autostash entry into the stash reflog instead of just deleting
> it.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> 
> Notes:
>     This patch is based on 'dl/merge-autostash'.
> 
>  Documentation/git-rebase.txt |  3 ++-
>  builtin/rebase.c             |  1 +
>  t/t3420-rebase-autostash.sh  | 20 ++++++++++++++++++++
>  3 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index f7a6033607..7d0c89a184 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.
>  --quit::
>  	Abort the rebase operation but HEAD is not reset back to the
>  	original branch. The index and working tree are also left
> -	unchanged as a result.
> +	unchanged as a result. If a temporary stash entry was created
> +	using --autostash, it will be saved to the stash reflog.>  --apply:
>  	Use applying strategies to rebase (calling `git-am`
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index bc4fc69906..71aec532b1 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1556,6 +1556,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		goto cleanup;
>  	}
>  	case ACTION_QUIT: {
> +		save_autostash(state_dir_path("autostash", &options));
>  		if (options.type == REBASE_MERGE) {
>  			struct replay_opts replay = REPLAY_OPTS_INIT;
>  
> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
> index b97ea62363..ca331733fb 100755
> --- a/t/t3420-rebase-autostash.sh
> +++ b/t/t3420-rebase-autostash.sh
> @@ -184,6 +184,26 @@ testrebase () {
>  		git checkout feature-branch
>  	'
>  
> +	test_expect_success "rebase$type: --quit" '
> +		test_config rebase.autostash true &&
> +		git reset --hard &&
> +		git checkout -b rebased-feature-branch feature-branch &&
> +		test_when_finished git branch -D rebased-feature-branch &&
> +		echo dirty >>file3 &&
> +		git diff >expect &&
> +		test_must_fail git rebase$type related-onto-branch &&
> +		test_path_is_file $dotest/autostash &&
> +		test_path_is_missing file3 &&
> +		git rebase --quit &&
> +		test_when_finished git stash drop &&
> +		test_path_is_missing $dotest/autostash &&
> +		! grep dirty file3 &&
> +		git stash show -p >actual &&
> +		test_cmp expect actual &&
> +		git reset --hard &&
> +		git checkout feature-branch
> +	'
> +
>  	test_expect_success "rebase$type: non-conflicting rebase, conflicting stash" '
>  		test_config rebase.autostash true &&
>  		git reset --hard &&
>

← back to recent threads