{"thread":{"id":"53341","subject":"[PATCH] rebase: save autostash entry into stash reflog on --quit","startedAt":"2020-04-28T09:31:49Z","lastAt":"2020-04-30T09:45:21Z","messageCount":5,"participants":["Denton Liu","Junio C Hamano","Phillip Wood"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"396449","messageId":"353a67567a90aea8a90bce1de05d005c61b3b670.1588066252.git.liu.denton@gmail.com","threadId":"53341","inReplyTo":null,"subject":"[PATCH] rebase: save autostash entry into stash reflog on --quit","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-28T09:31:31Z","receivedAt":"2020-04-28T09:31:49Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In a03b55530a (merge: teach --autostash option, 2020-04-07), the\n--autostash option was introduced for `git merge`. Notably, when\n`git merge --quit` is run with an autostash entry present, it is saved\ninto the stash reflog. This is contrasted with the current behaviour of\n`git rebase --quit` where the autostash entry is simply just dropped out\nof existence.\n\nAdopt the behaviour of `git merge --quit` in `git rebase --quit` and\nsave the autostash entry into the stash reflog instead of just deleting\nit.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n\nNotes:\n    This patch is based on 'dl/merge-autostash'.\n\n Documentation/git-rebase.txt |  3 ++-\n builtin/rebase.c             |  1 +\n t/t3420-rebase-autostash.sh  | 20 ++++++++++++++++++++\n 3 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex f7a6033607..7d0c89a184 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.\n --quit::\n \tAbort the rebase operation but HEAD is not reset back to the\n \toriginal branch. The index and working tree are also left\n-\tunchanged as a result.\n+\tunchanged as a result. If a temporary stash entry was created\n+\tusing --autostash, it will be saved to the stash reflog.\n \n --apply:\n \tUse applying strategies to rebase (calling `git-am`\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex bc4fc69906..71aec532b1 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1556,6 +1556,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_QUIT: {\n+\t\tsave_autostash(state_dir_path(\"autostash\", &options));\n \t\tif (options.type == REBASE_MERGE) {\n \t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n \ndiff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\nindex b97ea62363..ca331733fb 100755\n--- a/t/t3420-rebase-autostash.sh\n+++ b/t/t3420-rebase-autostash.sh\n@@ -184,6 +184,26 @@ testrebase () {\n \t\tgit checkout feature-branch\n \t'\n \n+\ttest_expect_success \"rebase$type: --quit\" '\n+\t\ttest_config rebase.autostash true &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout -b rebased-feature-branch feature-branch &&\n+\t\ttest_when_finished git branch -D rebased-feature-branch &&\n+\t\techo dirty >>file3 &&\n+\t\tgit diff >expect &&\n+\t\ttest_must_fail git rebase$type related-onto-branch &&\n+\t\ttest_path_is_file $dotest/autostash &&\n+\t\ttest_path_is_missing file3 &&\n+\t\tgit rebase --quit &&\n+\t\ttest_when_finished git stash drop &&\n+\t\ttest_path_is_missing $dotest/autostash &&\n+\t\t! grep dirty file3 &&\n+\t\tgit stash show -p >actual &&\n+\t\ttest_cmp expect actual &&\n+\t\tgit reset --hard &&\n+\t\tgit checkout feature-branch\n+\t'\n+\n \ttest_expect_success \"rebase$type: non-conflicting rebase, conflicting stash\" '\n \t\ttest_config rebase.autostash true &&\n \t\tgit reset --hard &&\n-- \n2.26.0.159.g23e2136ad0\n\n"},{"id":"396493","messageId":"xmqqv9lj1k0c.fsf@gitster.c.googlers.com","threadId":"53341","inReplyTo":"353a67567a90aea8a90bce1de05d005c61b3b670.1588066252.git.liu.denton@gmail.com","subject":"Re: [PATCH] rebase: save autostash entry into stash reflog on --quit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-28T19:35:15Z","receivedAt":"2020-04-28T19:35:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> In a03b55530a (merge: teach --autostash option, 2020-04-07), the\n> --autostash option was introduced for `git merge`. Notably, when\n> `git merge --quit` is run with an autostash entry present, it is saved\n> into the stash reflog. This is contrasted with the current behaviour of\n> `git rebase --quit` where the autostash entry is simply just dropped out\n> of existence.\n>\n> Adopt the behaviour of `git merge --quit` in `git rebase --quit` and\n> save the autostash entry into the stash reflog instead of just deleting\n> it.\n\nThe goal is wrothy, I would think, but I do not think we would\nexplain it in terms of \"stash reflog\".  It is true that what \"stash\nlist\" shows, the list of stasn entries, happens to be implemented as\nreflog entries of a single ref, but the end users would not view\nthem as reflog entries, I suspect.  Do you know anybody who would do\n\"git reflog stash@{now}\"?\n\nIt is less bad to explain with \"reflog of stash ref\" in the log\nmessage, meant to be read by our future developers, but ...\n\n\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index f7a6033607..7d0c89a184 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.\n>  --quit::\n>  \tAbort the rebase operation but HEAD is not reset back to the\n>  \toriginal branch. The index and working tree are also left\n> -\tunchanged as a result.\n> +\tunchanged as a result. If a temporary stash entry was created\n> +\tusing --autostash, it will be saved to the stash reflog.\n\n... let's not do so for end-user facing documentation.  \"..., it\nwill be stashed away\".  Or we may not even want to say anything; any\n\"--autostash\" user would expect that the changes that were stashed\nbefore \"rebase\" started would not be discarded, and this change may\njust be a bugfix.\n\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index bc4fc69906..71aec532b1 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1556,6 +1556,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\tgoto cleanup;\n>  \t}\n>  \tcase ACTION_QUIT: {\n> +\t\tsave_autostash(state_dir_path(\"autostash\", &options));\n>  \t\tif (options.type == REBASE_MERGE) {\n>  \t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n\nNice to see a fix as simple a this one ;-)\n"},{"id":"396520","messageId":"20200429002323.GB2570028@generichostname","threadId":"53341","inReplyTo":"xmqqv9lj1k0c.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] rebase: save autostash entry into stash reflog on --quit","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-04-29T00:23:23Z","receivedAt":"2020-04-29T00:23:29Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Junio,\n\nOn Tue, Apr 28, 2020 at 12:35:15PM -0700, Junio C Hamano wrote:\n> > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> > index f7a6033607..7d0c89a184 100644\n> > --- a/Documentation/git-rebase.txt\n> > +++ b/Documentation/git-rebase.txt\n> > @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.\n> >  --quit::\n> >  \tAbort the rebase operation but HEAD is not reset back to the\n> >  \toriginal branch. The index and working tree are also left\n> > -\tunchanged as a result.\n> > +\tunchanged as a result. If a temporary stash entry was created\n> > +\tusing --autostash, it will be saved to the stash reflog.\n> \n> ... let's not do so for end-user facing documentation.  \"..., it\n> will be stashed away\".  Or we may not even want to say anything; any\n> \"--autostash\" user would expect that the changes that were stashed\n> before \"rebase\" started would not be discarded, and this change may\n> just be a bugfix.\n\nHmm, in this case, git-merge.txt may need an update as well. From\n'dl/merge-autostash', \n\n\t'git merge --abort' is equivalent to 'git reset --merge' when\n\t`MERGE_HEAD` is present unless `MERGE_AUTOSTASH` is also present in\n\twhich case 'git merge --abort' applies the stash entry to the worktree\n\twhereas 'git reset --merge' will save the stashed changes in the stash\n\treflog.\n\nand\n\n\t--quit::\n\t\tForget about the current merge in progress. Leave the index\n\t\tand the working tree as-is. If `MERGE_AUTOSTASH` is present, the\n\t\tstash entry will be saved to the stash reflog.\n\nboth need to be amended to remove the reference to the \"stash reflog\".\n\nWhen I was writing this documentation, I wanted to distinguish between\nthe temporary autostash entry and the actual stash since the autostash\nentry isn't pushed to the stash unless there are conflicts or it's\nexplicitly saved. I'm not sure that something like \"If a temporary stash\nentry was created using --autostash, it will be stashed away\" works very\nwell since the word \"stash\" is overloaded here to mean \"a random stash\ncommit\" and \"stashed away in _the_ stash\". Unfortunately, I'm also\nhaving trouble coming up with a suitable phrasing of my own.\n\nI dunno, perhaps I'm overthinking this too and your suggested rewording\nsounds good and I'm just being too picky.\n\nThanks,\n\nDenton\n"},{"id":"396655","messageId":"c2babcad-d14d-0e06-3897-a3653e72edf6@gmail.com","threadId":"53341","inReplyTo":"353a67567a90aea8a90bce1de05d005c61b3b670.1588066252.git.liu.denton@gmail.com","subject":"Re: [PATCH] rebase: save autostash entry into stash reflog on --quit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-30T09:41:44Z","receivedAt":"2020-04-30T09:41:54Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Denton\n\nThanks for working on this, the implementation and test look fine\n\nBest Wishes\n\nPhillip\n\nOn 28/04/2020 10:31, Denton Liu wrote:\n> In a03b55530a (merge: teach --autostash option, 2020-04-07), the\n> --autostash option was introduced for `git merge`. Notably, when\n> `git merge --quit` is run with an autostash entry present, it is saved\n> into the stash reflog. This is contrasted with the current behaviour of\n> `git rebase --quit` where the autostash entry is simply just dropped out\n> of existence.\n> \n> Adopt the behaviour of `git merge --quit` in `git rebase --quit` and\n> save the autostash entry into the stash reflog instead of just deleting\n> it.\n> \n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n> \n> Notes:\n>     This patch is based on 'dl/merge-autostash'.\n> \n>  Documentation/git-rebase.txt |  3 ++-\n>  builtin/rebase.c             |  1 +\n>  t/t3420-rebase-autostash.sh  | 20 ++++++++++++++++++++\n>  3 files changed, 23 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index f7a6033607..7d0c89a184 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.\n>  --quit::\n>  \tAbort the rebase operation but HEAD is not reset back to the\n>  \toriginal branch. The index and working tree are also left\n> -\tunchanged as a result.\n> +\tunchanged as a result. If a temporary stash entry was created\n> +\tusing --autostash, it will be saved to the stash reflog.>  --apply:\n>  \tUse applying strategies to rebase (calling `git-am`\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index bc4fc69906..71aec532b1 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1556,6 +1556,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\tgoto cleanup;\n>  \t}\n>  \tcase ACTION_QUIT: {\n> +\t\tsave_autostash(state_dir_path(\"autostash\", &options));\n>  \t\tif (options.type == REBASE_MERGE) {\n>  \t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n>  \n> diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh\n> index b97ea62363..ca331733fb 100755\n> --- a/t/t3420-rebase-autostash.sh\n> +++ b/t/t3420-rebase-autostash.sh\n> @@ -184,6 +184,26 @@ testrebase () {\n>  \t\tgit checkout feature-branch\n>  \t'\n>  \n> +\ttest_expect_success \"rebase$type: --quit\" '\n> +\t\ttest_config rebase.autostash true &&\n> +\t\tgit reset --hard &&\n> +\t\tgit checkout -b rebased-feature-branch feature-branch &&\n> +\t\ttest_when_finished git branch -D rebased-feature-branch &&\n> +\t\techo dirty >>file3 &&\n> +\t\tgit diff >expect &&\n> +\t\ttest_must_fail git rebase$type related-onto-branch &&\n> +\t\ttest_path_is_file $dotest/autostash &&\n> +\t\ttest_path_is_missing file3 &&\n> +\t\tgit rebase --quit &&\n> +\t\ttest_when_finished git stash drop &&\n> +\t\ttest_path_is_missing $dotest/autostash &&\n> +\t\t! grep dirty file3 &&\n> +\t\tgit stash show -p >actual &&\n> +\t\ttest_cmp expect actual &&\n> +\t\tgit reset --hard &&\n> +\t\tgit checkout feature-branch\n> +\t'\n> +\n>  \ttest_expect_success \"rebase$type: non-conflicting rebase, conflicting stash\" '\n>  \t\ttest_config rebase.autostash true &&\n>  \t\tgit reset --hard &&\n> \n\n"},{"id":"396656","messageId":"a349bc81-2098-b2ae-76be-09b18b9229ef@gmail.com","threadId":"53341","inReplyTo":"20200429002323.GB2570028@generichostname","subject":"Re: [PATCH] rebase: save autostash entry into stash reflog on --quit","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2020-04-30T09:45:15Z","receivedAt":"2020-04-30T09:45:21Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Denton\n\nOn 29/04/2020 01:23, Denton Liu wrote:\n> Hi Junio,\n> \n> On Tue, Apr 28, 2020 at 12:35:15PM -0700, Junio C Hamano wrote:\n>>> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n>>> index f7a6033607..7d0c89a184 100644\n>>> --- a/Documentation/git-rebase.txt\n>>> +++ b/Documentation/git-rebase.txt\n>>> @@ -256,7 +256,8 @@ See also INCOMPATIBLE OPTIONS below.\n>>>  --quit::\n>>>  \tAbort the rebase operation but HEAD is not reset back to the\n>>>  \toriginal branch. The index and working tree are also left\n>>> -\tunchanged as a result.\n>>> +\tunchanged as a result. If a temporary stash entry was created\n>>> +\tusing --autostash, it will be saved to the stash reflog.\n>>\n>> ... let's not do so for end-user facing documentation.  \"..., it\n>> will be stashed away\".  Or we may not even want to say anything; any\n>> \"--autostash\" user would expect that the changes that were stashed\n>> before \"rebase\" started would not be discarded, and this change may\n>> just be a bugfix.\n> \n> Hmm, in this case, git-merge.txt may need an update as well. From\n> 'dl/merge-autostash', \n> \n> \t'git merge --abort' is equivalent to 'git reset --merge' when\n> \t`MERGE_HEAD` is present unless `MERGE_AUTOSTASH` is also present in\n> \twhich case 'git merge --abort' applies the stash entry to the worktree\n> \twhereas 'git reset --merge' will save the stashed changes in the stash\n> \treflog.\n> \n> and\n> \n> \t--quit::\n> \t\tForget about the current merge in progress. Leave the index\n> \t\tand the working tree as-is. If `MERGE_AUTOSTASH` is present, the\n> \t\tstash entry will be saved to the stash reflog.\n> \n> both need to be amended to remove the reference to the \"stash reflog\".\n\nI agree that it would be good to have the documentation for rebase and\nmerge match in this case\n\n> When I was writing this documentation, I wanted to distinguish between\n> the temporary autostash entry and the actual stash since the autostash\n> entry isn't pushed to the stash unless there are conflicts or it's\n> explicitly saved. I'm not sure that something like \"If a temporary stash\n> entry was created using --autostash, it will be stashed away\" works very\n> well since the word \"stash\" is overloaded here to mean \"a random stash\n> commit\" and \"stashed away in _the_ stash\". Unfortunately, I'm also\n> having trouble coming up with a suitable phrasing of my own.\n\nIt's tricky to distinguish between the two, when I was reviewing the\nmerge --autostash patches I was wary of the mention of the stash reflog\nbut could not come up with anything better to distinguish between\nremembering the stash oid and saving it to the list of stashes. I'm not\ntoo bothered what we end up doing so long as it is consistent across the\ncommands.\n\nBest Wishes\n\nPhillip\n\n> I dunno, perhaps I'm overthinking this too and your suggested rewording\n> sounds good and I'm just being too picky.\n> \n> Thanks,\n> \n> Denton\n> \n\n"}]}