{"thread":{"id":"50995","subject":"[PATCH] rebase --abort: cleanup refs/rewritten","startedAt":"2019-04-26T10:32:48Z","lastAt":"2019-05-14T18:04:10Z","messageCount":15,"participants":["Phillip Wood","Johannes Schindelin","SZEDER Gábor","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"374551","messageId":"20190426103212.8097-1-phillip.wood123@gmail.com","threadId":"50995","inReplyTo":null,"subject":"[PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-26T10:32:12Z","receivedAt":"2019-04-26T10:32:48Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen `rebase -r` finishes it removes any refs under refs/rewritten\nthat it has created. However if the rebase is aborted these refs are\nnot removed. This can cause problems for future rebases. For example I\nrecently wanted to merge a updated version of a topic branch into an\nintegration branch so ran `rebase -ir` and removed the picks and label\nfor the topic branch from the todo list so that\n merge -C topic\nwould pick up the new version of topic. Unfortunately\nrefs/rewritten/topic already existed from a previous rebase that had\nbeen aborted so the rebase just used the old topic, not the new one.\n\nSigned-off-by: Phillip Wood \n---\n\nNotes:\n This is based on pw/rebase-i-internal, it would be nicer to base it on\n maint but there are function name clashes adding sequencer.h to rebase.c\n an maint. Those clashes are fixed in pw/rebase-i-internal\n\n builtin/rebase.c | 13 ++++++++++---\n t/t3430-rebase-merges.sh | 8 ++++++++\n 2 files changed, 18 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 82bd50a1b4..e2e49c8239 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -761,9 +761,16 @@ static int finish_rebase(struct rebase_options *opts)\n \t * user should see them.\n \t */\n \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n-\tstrbuf_addstr(&dir, opts->state_dir);\n-\tremove_dir_recursively(&dir, 0);\n-\tstrbuf_release(&dir);\n+\tif (opts->type == REBASE_INTERACTIVE) {\n+\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n+\n+\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n+\t\tsequencer_remove_state(&replay);\n+\t} else {\n+\t\tstrbuf_addstr(&dir, opts->state_dir);\n+\t\tremove_dir_recursively(&dir, 0);\n+\t\tstrbuf_release(&dir);\n+\t}\n \n \treturn 0;\n }\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 4c69255ee6..6ebebf7098 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -224,6 +224,14 @@ test_expect_success 'refs/rewritten/* is worktree-local' '\n \ttest_cmp_rev HEAD \"$(cat wt/b)\"\n '\n \n+test_expect_success '--abort cleans up refs/rewritten' '\n+\tgit checkout -b abort-cleans-refs-rewritten H &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >>\" git rebase -ir @^ &&\n+\tgit rev-parse --verify refs/rewritten/onto &&\n+\tgit rebase --abort &&\n+\ttest_must_fail git rev-parse --verify refs/rewritten/onto\n+'\n+\n test_expect_success 'post-rewrite hook and fixups work for merges' '\n \tgit checkout -b post-rewrite &&\n \ttest_commit same1 &&\n-- \n2.21.0\n\n"},{"id":"374645","messageId":"nycvar.QRO.7.76.6.1904291207040.45@tvgsbejvaqbjf.bet","threadId":"50995","inReplyTo":"20190426103212.8097-1-phillip.wood123@gmail.com","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-29T16:07:48Z","receivedAt":"2019-04-29T16:07:54Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Fri, 26 Apr 2019, Phillip Wood wrote:\n\n> From: Phillip Wood \n>\n> When `rebase -r` finishes it removes any refs under refs/rewritten\n> that it has created. However if the rebase is aborted these refs are\n> not removed. This can cause problems for future rebases. For example I\n> recently wanted to merge a updated version of a topic branch into an\n> integration branch so ran `rebase -ir` and removed the picks and label\n> for the topic branch from the todo list so that\n> merge -C topic\n> would pick up the new version of topic. Unfortunately\n> refs/rewritten/topic already existed from a previous rebase that had\n> been aborted so the rebase just used the old topic, not the new one.\n>\n> Signed-off-by: Phillip Wood \n> ---\n\nMakes a ton of sense, and I feel a bit embarrassed that I forgot about\nthat item on my TODO list. The patch looks obviously correct!\n\nThanks,\nDscho\n"},{"id":"374696","messageId":"a3d5dd8c-12a6-a1d8-a41f-ef75c15df024@gmail.com","threadId":"50995","inReplyTo":"nycvar.QRO.7.76.6.1904291207040.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-04-30T08:54:55Z","receivedAt":"2019-04-30T08:55:01Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 29/04/2019 17:07, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Fri, 26 Apr 2019, Phillip Wood wrote:\n> \n>> From: Phillip Wood \n>>\n>> When `rebase -r` finishes it removes any refs under refs/rewritten\n>> that it has created. However if the rebase is aborted these refs are\n>> not removed. This can cause problems for future rebases. For example I\n>> recently wanted to merge a updated version of a topic branch into an\n>> integration branch so ran `rebase -ir` and removed the picks and label\n>> for the topic branch from the todo list so that\n>> merge -C topic\n>> would pick up the new version of topic. Unfortunately\n>> refs/rewritten/topic already existed from a previous rebase that had\n>> been aborted so the rebase just used the old topic, not the new one.\n>>\n>> Signed-off-by: Phillip Wood \n>> ---\n> \n> Makes a ton of sense, and I feel a bit embarrassed that I forgot about\n> that item on my TODO list. The patch looks obviously correct!\n\nThanks, after I sent it I realized that --quit should probably clear \nrefs/rewritten as well, so I'll re-roll with that added. (One could \nargue that a user might want them after quitting the rebase but there is \nno way to clean them up safely once we've deleted the state files and I \nsuspect most users would be suprised if they were left laying around)\n\nBest Wishes\n\nPhillip\n\n> \n> Thanks,\n> Dscho\n> \n"},{"id":"374730","messageId":"nycvar.QRO.7.76.6.1904301848510.45@tvgsbejvaqbjf.bet","threadId":"50995","inReplyTo":"a3d5dd8c-12a6-a1d8-a41f-ef75c15df024@gmail.com","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-30T22:49:47Z","receivedAt":"2019-04-30T22:50:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Tue, 30 Apr 2019, Phillip Wood wrote:\n\n> On 29/04/2019 17:07, Johannes Schindelin wrote:\n> >\n> > On Fri, 26 Apr 2019, Phillip Wood wrote:\n> >\n> > > From: Phillip Wood \n> > >\n> > > When `rebase -r` finishes it removes any refs under refs/rewritten\n> > > that it has created. However if the rebase is aborted these refs are\n> > > not removed. This can cause problems for future rebases. For example I\n> > > recently wanted to merge a updated version of a topic branch into an\n> > > integration branch so ran `rebase -ir` and removed the picks and label\n> > > for the topic branch from the todo list so that\n> > > merge -C topic\n> > > would pick up the new version of topic. Unfortunately\n> > > refs/rewritten/topic already existed from a previous rebase that had\n> > > been aborted so the rebase just used the old topic, not the new one.\n> > >\n> > > Signed-off-by: Phillip Wood \n> > > ---\n> >\n> > Makes a ton of sense, and I feel a bit embarrassed that I forgot about\n> > that item on my TODO list. The patch looks obviously correct!\n>\n> Thanks, after I sent it I realized that --quit should probably clear\n> refs/rewritten as well, so I'll re-roll with that added. (One could argue that\n> a user might want them after quitting the rebase but there is no way to clean\n> them up safely once we've deleted the state files and I suspect most users\n> would be suprised if they were left laying around)\n\nI am not so sure. `--quit` is essentially all about \"leave the state\nas-is, but still abort the rebase\".\n\nSo if I were you, I would *not* remove the `refs/rewritten/` refs in the\n`--quit` case.\n\nCiao,\nDscho\n"},{"id":"374767","messageId":"4d486504-7f64-95fb-b7eb-51d424f3e6cc@gmail.com","threadId":"50995","inReplyTo":"nycvar.QRO.7.76.6.1904301848510.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-01T15:36:24Z","receivedAt":"2019-05-01T15:36:29Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 30/04/2019 23:49, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Tue, 30 Apr 2019, Phillip Wood wrote:\n> \n>> On 29/04/2019 17:07, Johannes Schindelin wrote:\n>>>\n>>> On Fri, 26 Apr 2019, Phillip Wood wrote:\n>>>\n>>>> From: Phillip Wood \n>>>>\n>>>> When `rebase -r` finishes it removes any refs under refs/rewritten\n>>>> that it has created. However if the rebase is aborted these refs are\n>>>> not removed. This can cause problems for future rebases. For example I\n>>>> recently wanted to merge a updated version of a topic branch into an\n>>>> integration branch so ran `rebase -ir` and removed the picks and label\n>>>> for the topic branch from the todo list so that\n>>>> merge -C topic\n>>>> would pick up the new version of topic. Unfortunately\n>>>> refs/rewritten/topic already existed from a previous rebase that had\n>>>> been aborted so the rebase just used the old topic, not the new one.\n>>>>\n>>>> Signed-off-by: Phillip Wood \n>>>> ---\n>>>\n>>> Makes a ton of sense, and I feel a bit embarrassed that I forgot about\n>>> that item on my TODO list. The patch looks obviously correct!\n>>\n>> Thanks, after I sent it I realized that --quit should probably clear\n>> refs/rewritten as well, so I'll re-roll with that added. (One could argue that\n>> a user might want them after quitting the rebase but there is no way to clean\n>> them up safely once we've deleted the state files and I suspect most users\n>> would be suprised if they were left laying around)\n> \n> I am not so sure. `--quit` is essentially all about \"leave the state\n> as-is, but still abort the rebase\".\n\nI think it depends on what you mean by \"state\" `--quit` is about \nremoving state specific to rebases while preserving HEAD, the index and \nworktree. When \"rebase --quit\" was introduced in 9512177b68 (\"rebase: \nadd --quit to cleanup rebase, leave everything else untouched\", \n2016-11-12) the start of the log message reads\n\n rebase: add --quit to cleanup rebase, leave everything else untouched\n\n There are occasions when you decide to abort an in-progress rebase and\n move on to do something else but you forget to do \"git rebase --abort\"\n first. Or the rebase has been in progress for so long you forgot about\n it. By the time you realize that (e.g. by starting another rebase)\n it's already too late to retrace your steps. The solution is normally\n\n rm -r .git/\n\n and continue with your life.\n\n\nSo `--quit` is used when the user has forgotten to run \"rebase --abort\". \nThey have moved onto something else and want to remove the rebase state \nwithout changing the current HEAD, index or worktree, they are not \nlooking to use the refs under refs/rewritten. I think the refs rebase \ncreates under refs/rewritten is an implementation detail of \"rebase -r\" \nand should be treated like files under .git/rebase-merge. I'm worried \nthat if we leave them lying around after --quit they will cause trouble \nfor future rebases in the same way they have after \"rebase --abort\"\n\nBest Wishes\n\nPhillip\n\n> So if I were you, I would *not* remove the `refs/rewritten/` refs in the\n> `--quit` case.\n> \n> Ciao,\n> Dscho\n> \n"},{"id":"374865","messageId":"nycvar.QRO.7.76.6.1905031117500.45@tvgsbejvaqbjf.bet","threadId":"50995","inReplyTo":"4d486504-7f64-95fb-b7eb-51d424f3e6cc@gmail.com","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-05-03T09:21:58Z","receivedAt":"2019-05-03T09:22:08Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Phillip,\n\nOn Wed, 1 May 2019, Phillip Wood wrote:\n\n> On 30/04/2019 23:49, Johannes Schindelin wrote:\n> >\n> > On Tue, 30 Apr 2019, Phillip Wood wrote:\n> >\n> > > On 29/04/2019 17:07, Johannes Schindelin wrote:\n> > > >\n> > > > On Fri, 26 Apr 2019, Phillip Wood wrote:\n> > > >\n> > > > > From: Phillip Wood \n> > > > >\n> > > > > When `rebase -r` finishes it removes any refs under refs/rewritten\n> > > > > that it has created. However if the rebase is aborted these refs are\n> > > > > not removed. This can cause problems for future rebases. For example I\n> > > > > recently wanted to merge a updated version of a topic branch into an\n> > > > > integration branch so ran `rebase -ir` and removed the picks and label\n> > > > > for the topic branch from the todo list so that\n> > > > > merge -C topic\n> > > > > would pick up the new version of topic. Unfortunately\n> > > > > refs/rewritten/topic already existed from a previous rebase that had\n> > > > > been aborted so the rebase just used the old topic, not the new one.\n> > > > >\n> > > > > Signed-off-by: Phillip Wood \n> > > > > ---\n> > > >\n> > > > Makes a ton of sense, and I feel a bit embarrassed that I forgot about\n> > > > that item on my TODO list. The patch looks obviously correct!\n> > >\n> > > Thanks, after I sent it I realized that --quit should probably clear\n> > > refs/rewritten as well, so I'll re-roll with that added. (One could argue\n> > > that\n> > > a user might want them after quitting the rebase but there is no way to\n> > > clean\n> > > them up safely once we've deleted the state files and I suspect most users\n> > > would be suprised if they were left laying around)\n> >\n> > I am not so sure. `--quit` is essentially all about \"leave the state\n> > as-is, but still abort the rebase\".\n>\n> I think it depends on what you mean by \"state\" `--quit` is about removing\n> state specific to rebases while preserving HEAD, the index and worktree.\n\nI guess the fault is mine for bleeding out internal rebase state into the\nrefs namespace.\n\nWhile I cannot really imagine any harm from this patch in practice, it is\nslightly worrisome that deleting refs also deletes their reflogs, which\nmakes it an unrecoverable problem *iff* any user runs into trouble with\nthis.\n\nCiao,\nDscho\n"},{"id":"374871","messageId":"e7adc1ec-3db3-654c-4ee1-e1a93ec84a99@gmail.com","threadId":"50995","inReplyTo":"nycvar.QRO.7.76.6.1905031117500.45@tvgsbejvaqbjf.bet","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-03T10:06:38Z","receivedAt":"2019-05-03T10:06:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Dscho\n\nOn 03/05/2019 10:21, Johannes Schindelin wrote:\n> Hi Phillip,\n> \n> On Wed, 1 May 2019, Phillip Wood wrote:\n> \n>> On 30/04/2019 23:49, Johannes Schindelin wrote:\n>>>\n>>> On Tue, 30 Apr 2019, Phillip Wood wrote:\n>>>\n>>>> On 29/04/2019 17:07, Johannes Schindelin wrote:\n>>>>>\n>>>>> On Fri, 26 Apr 2019, Phillip Wood wrote:\n>>>>>\n>>>>>> From: Phillip Wood \n>>>>>>\n>>>>>> When `rebase -r` finishes it removes any refs under refs/rewritten\n>>>>>> that it has created. However if the rebase is aborted these refs are\n>>>>>> not removed. This can cause problems for future rebases. For example I\n>>>>>> recently wanted to merge a updated version of a topic branch into an\n>>>>>> integration branch so ran `rebase -ir` and removed the picks and label\n>>>>>> for the topic branch from the todo list so that\n>>>>>> merge -C topic\n>>>>>> would pick up the new version of topic. Unfortunately\n>>>>>> refs/rewritten/topic already existed from a previous rebase that had\n>>>>>> been aborted so the rebase just used the old topic, not the new one.\n>>>>>>\n>>>>>> Signed-off-by: Phillip Wood \n>>>>>> ---\n>>>>>\n>>>>> Makes a ton of sense, and I feel a bit embarrassed that I forgot about\n>>>>> that item on my TODO list. The patch looks obviously correct!\n>>>>\n>>>> Thanks, after I sent it I realized that --quit should probably clear\n>>>> refs/rewritten as well, so I'll re-roll with that added. (One could argue\n>>>> that\n>>>> a user might want them after quitting the rebase but there is no way to\n>>>> clean\n>>>> them up safely once we've deleted the state files and I suspect most users\n>>>> would be suprised if they were left laying around)\n>>>\n>>> I am not so sure. `--quit` is essentially all about \"leave the state\n>>> as-is, but still abort the rebase\".\n>>\n>> I think it depends on what you mean by \"state\" `--quit` is about removing\n>> state specific to rebases while preserving HEAD, the index and worktree.\n> \n> I guess the fault is mine for bleeding out internal rebase state into the\n> refs namespace.\n\nI wouldn't feel bad about that, I guess it would be possible to get gc \nto read a list of objects not to collect from a file in \n.git/rebase-merge but creating a refs for labels seems like a sensible \nway to stop them from being collect by gc.\n\n> While I cannot really imagine any harm from this patch in practice, it is\n> slightly worrisome that deleting refs also deletes their reflogs,\n\nYes it's a shame there's no way to get a ref back once it's been deleted \n(though I'm not sure how long we'd want to keep any reflog of a deleted \nref before gc'ing the objects). In any case refs/rewritten only has a \nreflog if the user has explicitly enabled it.\n\n> which\n> makes it an unrecoverable problem *iff* any user runs into trouble with\n> this.\n\nI guess \"rebase --quit\" could print a warning listing the refs that are \nbeing deleted so the user can cut and paste if they need to. I'm not \nsure how likely they are to need that though.\n\nBest Wishes\n\nPhillip\n\n> Ciao,\n> Dscho\n> \n"},{"id":"375038","messageId":"20190507151530.GO14763@szeder.dev","threadId":"50995","inReplyTo":"20190426103212.8097-1-phillip.wood123@gmail.com","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-05-07T15:15:31Z","receivedAt":"2019-05-07T15:15:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Apr 26, 2019 at 11:32:12AM +0100, Phillip Wood wrote:\n> From: Phillip Wood \n> \n> When `rebase -r` finishes it removes any refs under refs/rewritten\n> that it has created. However if the rebase is aborted these refs are\n> not removed. This can cause problems for future rebases. For example I\n> recently wanted to merge a updated version of a topic branch into an\n> integration branch so ran `rebase -ir` and removed the picks and label\n> for the topic branch from the todo list so that\n> merge -C topic\n> would pick up the new version of topic. Unfortunately\n> refs/rewritten/topic already existed from a previous rebase that had\n> been aborted so the rebase just used the old topic, not the new one.\n> \n> Signed-off-by: Phillip Wood \n> ---\n> \n> Notes:\n> This is based on pw/rebase-i-internal, it would be nicer to base it on\n> maint but there are function name clashes adding sequencer.h to rebase.c\n> an maint. Those clashes are fixed in pw/rebase-i-internal\n> \n> builtin/rebase.c | 13 ++++++++++---\n> t/t3430-rebase-merges.sh | 8 ++++++++\n> 2 files changed, 18 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 82bd50a1b4..e2e49c8239 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -761,9 +761,16 @@ static int finish_rebase(struct rebase_options *opts)\n> \t * user should see them.\n> \t */\n> \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n> -\tstrbuf_addstr(&dir, opts->state_dir);\n> -\tremove_dir_recursively(&dir, 0);\n> -\tstrbuf_release(&dir);\n> +\tif (opts->type == REBASE_INTERACTIVE) {\n> +\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n\nThis patch and the topic 'pw/rebase-abort-clean-rewritten' can't be\ncompiled on its own, because it starts using 'struct replay_opts'\nhere, which is defined in 'sequencer.h', but 'builtin/rebase.c'\ndoesn't include that header yet. (Though 'pu' already builds fine,\nbecause commit 0609b741a4 (rebase -i: combine rebase--interactive.c\nwith rebase.c, 2019-04-17) in the parallel topic\n'pw/rebase-i-internal' adds the necessary #include.)\n\nSo, to keep future bisects from potentially tipping over the compiler\nerror, this patch should either #include \"sequencer.h\", or be applied\non top of 'pw/rebase-i-internal'.\n\n> +\n> +\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n> +\t\tsequencer_remove_state(&replay);\n> +\t} else {\n> +\t\tstrbuf_addstr(&dir, opts->state_dir);\n> +\t\tremove_dir_recursively(&dir, 0);\n> +\t\tstrbuf_release(&dir);\n> +\t}\n> \n> \treturn 0;\n> }\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index 4c69255ee6..6ebebf7098 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -224,6 +224,14 @@ test_expect_success 'refs/rewritten/* is worktree-local' '\n> \ttest_cmp_rev HEAD \"$(cat wt/b)\"\n> '\n> \n> +test_expect_success '--abort cleans up refs/rewritten' '\n> +\tgit checkout -b abort-cleans-refs-rewritten H &&\n> +\tGIT_SEQUENCE_EDITOR=\"echo break >>\" git rebase -ir @^ &&\n> +\tgit rev-parse --verify refs/rewritten/onto &&\n> +\tgit rebase --abort &&\n> +\ttest_must_fail git rev-parse --verify refs/rewritten/onto\n> +'\n> +\n> test_expect_success 'post-rewrite hook and fixups work for merges' '\n> \tgit checkout -b post-rewrite &&\n> \ttest_commit same1 &&\n> -- \n> 2.21.0\n> \n"},{"id":"375044","messageId":"xmqqpnoujlj4.fsf@gitster-ct.c.googlers.com","threadId":"50995","inReplyTo":"20190507151530.GO14763@szeder.dev","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-07T16:07:27Z","receivedAt":"2019-05-07T16:07:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> This patch and the topic 'pw/rebase-abort-clean-rewritten' can't be\n> compiled on its own, because it starts using 'struct replay_opts'\n> here, which is defined in 'sequencer.h', but 'builtin/rebase.c'\n> doesn't include that header yet. (Though 'pu' already builds fine,\n> because commit 0609b741a4 (rebase -i: combine rebase--interactive.c\n> with rebase.c, 2019-04-17) in the parallel topic\n> 'pw/rebase-i-internal' adds the necessary #include.)\n\nThanks; that's entirely my fault. I needed to find a good fork\npoint and failed to do so. FTR, when there are too many topics\nI need to queue on a given day, I may not have time to compile\ncheck individual topic branches before merging them to the\nintegration branches, testing the integration branches and pushing\nthem out. That was what happened here.\n\n> So, to keep future bisects from potentially tipping over the compiler\n> error, this patch should either #include \"sequencer.h\", or be applied\n> on top of 'pw/rebase-i-internal'.\n\nI suspect that the latter was how the patch originally was\ndeveloped.\n"},{"id":"375051","messageId":"d4d1dc5c-14ec-f354-4dac-d17faf533865@gmail.com","threadId":"50995","inReplyTo":"xmqqpnoujlj4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] rebase --abort: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-07T20:06:19Z","receivedAt":"2019-05-07T20:06:24Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 07/05/2019 17:07, Junio C Hamano wrote:\n> SZEDER Gábor writes:\n> \n>> This patch and the topic 'pw/rebase-abort-clean-rewritten' can't be\n>> compiled on its own, because it starts using 'struct replay_opts'\n>> here, which is defined in 'sequencer.h', but 'builtin/rebase.c'\n>> doesn't include that header yet. (Though 'pu' already builds fine,\n>> because commit 0609b741a4 (rebase -i: combine rebase--interactive.c\n>> with rebase.c, 2019-04-17) in the parallel topic\n>> 'pw/rebase-i-internal' adds the necessary #include.)\n> \n> Thanks; that's entirely my fault. I needed to find a good fork\n> point and failed to do so. FTR, when there are too many topics\n> I need to queue on a given day, I may not have time to compile\n> check individual topic branches before merging them to the\n> integration branches, testing the integration branches and pushing\n> them out. That was what happened here.\n> \n>> So, to keep future bisects from potentially tipping over the compiler\n>> error, this patch should either #include \"sequencer.h\", or be applied\n>> on top of 'pw/rebase-i-internal'.\n> \n> I suspect that the latter was how the patch originally was\n> developed.\n\nYes that's right, there is a note on the original patch [1] explaining \nwhy - you cannot just add '#include \"sequencer.h\"' as there are function \nname conflicts between a static function in builtin/rebase.c and a \nglobal function in sequencer.c which are fixed in pw/rebase-i-internal\n\nBest Wishes\n\nPhillip\n\n[1] \nhttps://public-inbox.org/git/xmqqpnoujlj4.fsf@gitster-ct.c.googlers.com/T/#mb431b81731798388e12f3852747c560f8ce7c6ec\n\n"},{"id":"375547","messageId":"20190514180349.17245-1-phillip.wood123@gmail.com","threadId":"50995","inReplyTo":"20190426103212.8097-1-phillip.wood123@gmail.com","subject":"[PATCH v2 0/4] rebase --abort/--quit: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-14T18:03:45Z","receivedAt":"2019-05-14T18:04:05Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nrefs/rewritten/ is now cleaned up on --quit as well as --abort. I've\nalso added a patch to make sequencer_remove_state() to return any\nerrors, so rebase now always reports any errors that occur when\ncleaning up the state directory.\n\nThese patches are still based on pw/rebase-i-internal, the first 3\ncould probably be applied to maint if required.\n\nI'm going to be off line for ten days or so from Thursday so there's\nno hurry to look at these (also we're in an rc phase at the moment)\n\nBest Wishes\n\nPhillip\n\n\nPhillip Wood (4):\n rebase: fix a memory leak\n rebase: warn if state directory cannot be removed\n sequencer: return errors from sequencer_remove_state()\n rebase --abort/--quit: cleanup refs/rewritten\n\n builtin/rebase.c | 39 +++++++++++++++++++++++++++++----------\n sequencer.c | 11 +++++++----\n t/t3430-rebase-merges.sh | 18 +++++++++++++++++-\n 3 files changed, 53 insertions(+), 15 deletions(-)\n\n-- \n2.21.0\n\n"},{"id":"375548","messageId":"20190514180349.17245-2-phillip.wood123@gmail.com","threadId":"50995","inReplyTo":"20190514180349.17245-1-phillip.wood123@gmail.com","subject":"[PATCH v2 1/4] rebase: fix a memory leak","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-14T18:03:46Z","receivedAt":"2019-05-14T18:04:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nbuf was never freed.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 82bd50a1b4..90037c9c45 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -2168,6 +2168,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tret = !!run_specific_rebase(&options, action);\n \n cleanup:\n+\tstrbuf_release(&buf);\n \tstrbuf_release(&revisions);\n \tfree(options.head_name);\n \tfree(options.gpg_sign_opt);\n-- \n2.21.0\n\n"},{"id":"375549","messageId":"20190514180349.17245-3-phillip.wood123@gmail.com","threadId":"50995","inReplyTo":"20190514180349.17245-1-phillip.wood123@gmail.com","subject":"[PATCH v2 2/4] rebase: warn if state directory cannot be removed","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-14T18:03:47Z","receivedAt":"2019-05-14T18:04:08Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf rebase --quit cannot remove the state directory then it dies. However\nwhen rebase finishes normally or the user runs rebase --abort any errors\nthat occur when removing the state directory are ignored. That is fixed\nby this commit.\n\nAll of the callers of finish_rebase() except the code\nthat handles --abort are careful to make sure they get a postive return\nvalue, do the same for --abort.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 8 +++++---\n 1 file changed, 5 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 90037c9c45..199cb5b81d 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -752,6 +752,7 @@ static int finish_rebase(struct rebase_options *opts)\n {\n \tstruct strbuf dir = STRBUF_INIT;\n \tconst char *argv_gc_auto[] = { \"gc\", \"--auto\", NULL };\n+\tint ret = 0;\n \n \tdelete_ref(NULL, \"REBASE_HEAD\", NULL, REF_NO_DEREF);\n \tapply_autostash(opts);\n@@ -762,10 +763,11 @@ static int finish_rebase(struct rebase_options *opts)\n \t */\n \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n \tstrbuf_addstr(&dir, opts->state_dir);\n-\tremove_dir_recursively(&dir, 0);\n+\tif (remove_dir_recursively(&dir, 0))\n+\t\tret = error(_(\"could not remove '%s'\"), opts->state_dir);\n \tstrbuf_release(&dir);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static struct commit *peel_committish(const char *name)\n@@ -1648,7 +1650,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"could not move back to %s\"),\n \t\t\t oid_to_hex(&options.orig_head));\n \t\tremove_branch_state(the_repository);\n-\t\tret = finish_rebase(&options);\n+\t\tret = !!finish_rebase(&options);\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_QUIT: {\n-- \n2.21.0\n\n"},{"id":"375550","messageId":"20190514180349.17245-5-phillip.wood123@gmail.com","threadId":"50995","inReplyTo":"20190514180349.17245-1-phillip.wood123@gmail.com","subject":"[PATCH v2 4/4] rebase --abort/--quit: cleanup refs/rewritten","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-14T18:03:49Z","receivedAt":"2019-05-14T18:04:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nWhen `rebase -r` finishes it removes any refs under refs/rewritten that\nit has created. However if the user aborts or quits the rebase refs are\nnot removed. This can cause problems for future rebases. For example I\nrecently wanted to merge a updated version of a topic branch into an\nintegration branch so ran `rebase -ir` and removed the picks and label\nfor the topic branch from the todo list so that\n\n merge -C topic\n\nwould pick up the new version of topic. Unfortunately\nrefs/rewritten/topic already existed from a previous rebase that had\nbeen aborted so the rebase just used the old topic, not the new one.\n\nThe logic for the non-interactive quit case is changed to ensure\n`buf` is always freed.\n\nSigned-off-by: Phillip Wood \n---\n builtin/rebase.c | 34 +++++++++++++++++++++++++---------\n t/t3430-rebase-merges.sh | 18 +++++++++++++++++-\n 2 files changed, 42 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 199cb5b81d..7c4cd1a733 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -762,10 +762,18 @@ static int finish_rebase(struct rebase_options *opts)\n \t * user should see them.\n \t */\n \trun_command_v_opt(argv_gc_auto, RUN_GIT_CMD);\n-\tstrbuf_addstr(&dir, opts->state_dir);\n-\tif (remove_dir_recursively(&dir, 0))\n-\t\tret = error(_(\"could not remove '%s'\"), opts->state_dir);\n-\tstrbuf_release(&dir);\n+\tif (opts->type == REBASE_INTERACTIVE) {\n+\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n+\n+\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n+\t\tret = sequencer_remove_state(&replay);\n+\t} else {\n+\t\tstrbuf_addstr(&dir, opts->state_dir);\n+\t\tif (remove_dir_recursively(&dir, 0))\n+\t\t\tret = error(_(\"could not remove '%s'\"),\n+\t\t\t\t opts->state_dir);\n+\t\tstrbuf_release(&dir);\n+\t}\n \n \treturn ret;\n }\n@@ -1654,11 +1662,19 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_QUIT: {\n-\t\tstrbuf_reset(&buf);\n-\t\tstrbuf_addstr(&buf, options.state_dir);\n-\t\tret = !!remove_dir_recursively(&buf, 0);\n-\t\tif (ret)\n-\t\t\tdie(_(\"could not remove '%s'\"), options.state_dir);\n+\t\tif (options.type == REBASE_INTERACTIVE) {\n+\t\t\tstruct replay_opts replay = REPLAY_OPTS_INIT;\n+\n+\t\t\treplay.action = REPLAY_INTERACTIVE_REBASE;\n+\t\t\tret = !!sequencer_remove_state(&replay);\n+\t\t} else {\n+\t\t\tstrbuf_reset(&buf);\n+\t\t\tstrbuf_addstr(&buf, options.state_dir);\n+\t\t\tret = !!remove_dir_recursively(&buf, 0);\n+\t\t\tif (ret)\n+\t\t\t\terror(_(\"could not remove '%s'\"),\n+\t\t\t\t options.state_dir);\n+\t\t}\n \t\tgoto cleanup;\n \t}\n \tcase ACTION_EDIT_TODO:\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex 4c69255ee6..e63576d334 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -224,8 +224,24 @@ test_expect_success 'refs/rewritten/* is worktree-local' '\n \ttest_cmp_rev HEAD \"$(cat wt/b)\"\n '\n \n+test_expect_success '--abort cleans up refs/rewritten' '\n+\tgit checkout -b abort-cleans-refs-rewritten H &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >>\" git rebase -ir @^ &&\n+\tgit rev-parse --verify refs/rewritten/onto &&\n+\tgit rebase --abort &&\n+\ttest_must_fail git rev-parse --verify refs/rewritten/onto\n+'\n+\n+test_expect_success '--quit cleans up refs/rewritten' '\n+\tgit checkout -b quit-cleans-refs-rewritten H &&\n+\tGIT_SEQUENCE_EDITOR=\"echo break >>\" git rebase -ir @^ &&\n+\tgit rev-parse --verify refs/rewritten/onto &&\n+\tgit rebase --quit &&\n+\ttest_must_fail git rev-parse --verify refs/rewritten/onto\n+'\n+\n test_expect_success 'post-rewrite hook and fixups work for merges' '\n-\tgit checkout -b post-rewrite &&\n+\tgit checkout -b post-rewrite H &&\n \ttest_commit same1 &&\n \tgit reset --hard HEAD^ &&\n \ttest_commit same2 &&\n-- \n2.21.0\n\n"},{"id":"375551","messageId":"20190514180349.17245-4-phillip.wood123@gmail.com","threadId":"50995","inReplyTo":"20190514180349.17245-1-phillip.wood123@gmail.com","subject":"[PATCH v2 3/4] sequencer: return errors from sequencer_remove_state()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-05-14T18:03:48Z","receivedAt":"2019-05-14T18:04:10Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"From: Phillip Wood \n\nIf there is an error when removing the state directory then we should\nreport it. This matches what the non-interactive rebase does.\n\nSigned-off-by: Phillip Wood \n---\n sequencer.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 610b7ece14..258e583156 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -274,7 +274,7 @@ static const char *gpg_sign_opt_quoted(struct replay_opts *opts)\n int sequencer_remove_state(struct replay_opts *opts)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n-\tint i;\n+\tint i, ret = 0;\n \n \tif (is_rebase_i(opts) &&\n \t strbuf_read_file(&buf, rebase_path_refs_to_delete(), 0) > 0) {\n@@ -283,8 +283,10 @@ int sequencer_remove_state(struct replay_opts *opts)\n \t\t\tchar *eol = strchr(p, '\\n');\n \t\t\tif (eol)\n \t\t\t\t*eol = '\\0';\n-\t\t\tif (delete_ref(\"(rebase -i) cleanup\", p, NULL, 0) < 0)\n+\t\t\tif (delete_ref(\"(rebase -i) cleanup\", p, NULL, 0) < 0) {\n \t\t\t\twarning(_(\"could not delete '%s'\"), p);\n+\t\t\t\tret = -1;\n+\t\t\t}\n \t\t\tif (!eol)\n \t\t\t\tbreak;\n \t\t\tp = eol + 1;\n@@ -300,10 +302,11 @@ int sequencer_remove_state(struct replay_opts *opts)\n \n \tstrbuf_reset(&buf);\n \tstrbuf_addstr(&buf, get_dir(opts));\n-\tremove_dir_recursively(&buf, 0);\n+\tif (remove_dir_recursively(&buf, 0))\n+\t\tret = error(_(\"could not remove '%s'\"), buf.buf);\n \tstrbuf_release(&buf);\n \n-\treturn 0;\n+\treturn ret;\n }\n \n static const char *action_name(const struct replay_opts *opts)\n-- \n2.21.0\n\n"}]}