{"thread":{"id":"58660","subject":"rebase -i --update-refs can lead to deletion of branches","startedAt":"2022-10-20T17:01:51Z","lastAt":"2022-11-08T09:58:56Z","messageCount":17,"participants":["herr.kaste","Erik Cervin Edin","Phillip Wood","Victoria Dye","Taylor Blau","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"465304","messageId":"CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@mail.gmail.com","threadId":"58660","inReplyTo":null,"subject":"rebase -i --update-refs can lead to deletion of branches","fromName":"herr.kaste","fromEmail":"herr.kaste@gmail.com","sentAt":"2022-10-20T17:01:05Z","receivedAt":"2022-10-20T17:01:51Z","isPatch":false,"sender":{"key":"herr.kaste@gmail.com","avatar":null},"body":"Hi,\n\nI have the following:\n\nWhile doing a\n\n`$ git rebase --interactive --update-refs X`\n\nI *removed* the \"update-ref\" lines from the todo list. The rebase runs\nas expected and prints e.g.\n\n```\nSuccessfully rebased and updated refs/heads/test.\nUpdated the following refs with --update-refs:\nrefs/heads/master\nrefs/heads/permissive-interactive-rebase\nrefs/heads/variable-annotations-meta-block\n```\n\nAfter that all refs have been removed/deleted.\n\n```\n$ git branch --list\n* test\n```\n\nNow, I should just have not used `--update-refs` in the first place but anyway\nI decide late that I rather don't want to update \"master\" etc. and it should\nprobably not delete the local refs.\n\nActually, I so love the new feature that I switched it *on* by default, and just\nwanted to overwrite the behavior in the todo editor.\n\nRegards\nCaspar Duregger\n"},{"id":"465344","messageId":"CA+JQ7M-GbBTHZZ9xOLR=FitWFpUnkfuep9kSfNPxuSbJbKteGw@mail.gmail.com","threadId":"58660","inReplyTo":"CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@mail.gmail.com","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2022-10-20T20:49:27Z","receivedAt":"2022-10-20T20:50:31Z","isPatch":false,"sender":{"key":"erik@cervined.in","avatar":null},"body":"On Thu, Oct 20, 2022 at 7:04 PM herr.kaste wrote:\n>\n> After that all refs have been removed/deleted.\n>\n> ```\n> $ git branch --list\n> * test\n> ```\n\n:(\n\n> I decide late that I rather don't want to update \"master\" etc. and it should\n> probably not delete the local refs.\n\nDeleting refs if you remove from the rebase-todo seems undesirable in\nmy opinion. It's too easy to use that footgun. It may be a good\nfeature to be _able_ to delete branches during a rebase, in a similar\nmanner as updating them, but with a different explicit flag, like d\nregs/heads/ or something\n\n> Actually, I so love the new feature that I switched it *on* by default, and just\n> wanted to overwrite the behavior in the todo editor.\n\nI didn't know this feature had been added but I'm very very pleased as\nI've wanted it. It doesn't seem to update tags though. That would've\nbeen nice.\n"},{"id":"466386","messageId":"16a8a331-f012-7dae-de1e-f03da95ecb6e@dunelm.org.uk","threadId":"58660","inReplyTo":"CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@mail.gmail.com","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-03T09:32:19Z","receivedAt":"2022-11-03T09:32:25Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Caspar\n\nOn 20/10/2022 18:01, herr.kaste wrote:\n> Hi,\n> \n> I have the following:\n> \n> While doing a\n> \n> `$ git rebase --interactive --update-refs X`\n> \n> I *removed* the \"update-ref\" lines from the todo list. The rebase runs\n> as expected and prints e.g.\n> \n> ```\n> Successfully rebased and updated refs/heads/test.\n> Updated the following refs with --update-refs:\n> refs/heads/master\n> refs/heads/permissive-interactive-rebase\n> refs/heads/variable-annotations-meta-block\n> ```\n> \n> After that all refs have been removed/deleted.\n> \n> ```\n> $ git branch --list\n> * test\n> ```\n> \n> Now, I should just have not used `--update-refs` in the first place but anyway\n> I decide late that I rather don't want to update \"master\" etc. and it should\n> probably not delete the local refs.\n> \n> Actually, I so love the new feature that I switched it *on* by default, and just\n> wanted to overwrite the behavior in the todo editor.\n\nSorry for the slow reply, I'm afraid I still haven't found time to look \nat this. As far as I can remember deleting the \"update-ref\" lines should \nleave the ref unchanged. I've cc'd the author to see if they have any \ninsight into what is going on\n\nBest Wishes\n\nPhillip\n\n\n> Regards\n> Caspar Duregger\n"},{"id":"466409","messageId":"CAFzd1+58MCXC9XZ+R7QFUdtw99KV2mHUHGgQUYvoa0USuYSLog@mail.gmail.com","threadId":"58660","inReplyTo":"16a8a331-f012-7dae-de1e-f03da95ecb6e@dunelm.org.uk","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"herr.kaste","fromEmail":"herr.kaste@gmail.com","sentAt":"2022-11-03T15:25:57Z","receivedAt":"2022-11-03T15:26:31Z","isPatch":false,"sender":{"key":"herr.kaste@gmail.com","avatar":null},"body":"I have the following reproduction\n\n```\ngit init &&\ngit commit --allow-empty -m \"Init\" &&\ngit commit --allow-empty -m \"A\" &&\ngit checkout -b feature &&\ngit commit --allow-empty -m \"B\" &&\ngit commit --allow-empty -m \"C\" &&\nGIT_SEQUENCE_EDITOR=\"sed -i -e '/^update-ref/d'\" git rebase\n--update-refs master^ --interactive\n```\n\nAfter that\n\n```\n$ git branch -l\n* feature\n```\n\nand `master` is gone. Is that reproduction/test-case sane, even? I\n*think* that's what I originally described.\n\nRegards\nCaspar Duregger\n\n\nAm Do., 3. Nov. 2022 um 10:32 Uhr schrieb Phillip Wood\n:\n>\n> Hi Caspar\n>\n> On 20/10/2022 18:01, herr.kaste wrote:\n> > Hi,\n> >\n> > I have the following:\n> >\n> > While doing a\n> >\n> > `$ git rebase --interactive --update-refs X`\n> >\n> > I *removed* the \"update-ref\" lines from the todo list. The rebase runs\n> > as expected and prints e.g.\n> >\n> > ```\n> > Successfully rebased and updated refs/heads/test.\n> > Updated the following refs with --update-refs:\n> > refs/heads/master\n> > refs/heads/permissive-interactive-rebase\n> > refs/heads/variable-annotations-meta-block\n> > ```\n> >\n> > After that all refs have been removed/deleted.\n> >\n> > ```\n> > $ git branch --list\n> > * test\n> > ```\n> >\n> > Now, I should just have not used `--update-refs` in the first place but anyway\n> > I decide late that I rather don't want to update \"master\" etc. and it should\n> > probably not delete the local refs.\n> >\n> > Actually, I so love the new feature that I switched it *on* by default, and just\n> > wanted to overwrite the behavior in the todo editor.\n>\n> Sorry for the slow reply, I'm afraid I still haven't found time to look\n> at this. As far as I can remember deleting the \"update-ref\" lines should\n> leave the ref unchanged. I've cc'd the author to see if they have any\n> insight into what is going on\n>\n> Best Wishes\n>\n> Phillip\n>\n>\n> > Regards\n> > Caspar Duregger\n"},{"id":"466427","messageId":"CA+JQ7M-nmQAdOGERCFbhd6v4o-mxg7T5JeKAC=pAGs1SqAzC=Q@mail.gmail.com","threadId":"58660","inReplyTo":"CAFzd1+58MCXC9XZ+R7QFUdtw99KV2mHUHGgQUYvoa0USuYSLog@mail.gmail.com","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"Erik Cervin Edin","fromEmail":"erik@cervined.in","sentAt":"2022-11-03T16:52:15Z","receivedAt":"2022-11-03T16:52:58Z","isPatch":false,"sender":{"key":"erik@cervined.in","avatar":null},"body":"On Thu, Nov 3, 2022 at 4:34 PM herr.kaste wrote:\n>\n> I have the following reproduction\n>\n> ```\n> git init &&\n> git commit --allow-empty -m \"Init\" &&\n> git commit --allow-empty -m \"A\" &&\n> git checkout -b feature &&\n> git commit --allow-empty -m \"B\" &&\n> git commit --allow-empty -m \"C\" &&\n> GIT_SEQUENCE_EDITOR=\"sed -i -e '/^update-ref/d'\" git rebase\n> --update-refs master^ --interactive\n> ```\n\nSome minor changes\nBetter explicitly name the branch master\n git init -b master &&\n\nalso\n GIT_SEQUENCE_EDITOR=\"sed -i -e '/^u/d'\" git rebase --update-refs\nmaster^ --interactive\nin case of short-form interactive rebase\n\nBut yes, it deletes the master branch\n"},{"id":"466462","messageId":"123628cc-1410-aaa0-0151-2dff35bd1855@github.com","threadId":"58660","inReplyTo":"CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@mail.gmail.com","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-04T00:31:49Z","receivedAt":"2022-11-04T00:31:56Z","isPatch":false,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"herr.kaste wrote:\n> Now, I should just have not used `--update-refs` in the first place but anyway\n> I decide late that I rather don't want to update \"master\" etc. and it should\n> probably not delete the local refs.\n> \n\nAgreed, this doesn't seem like desired behavior - the opposite of \"update\nthe ref\" isn't \"delete the ref\". ;)\n\nThe reason it's happening is because, when '--update-refs' is used, the\nrebase starts by constructing a list of 'update_ref_record's for each of the\nrefs that *could* be updated. Each item in that list contains the\ncorresponding ref's \"before\" commit OID (i.e., what it currently points to)\nand initializes the \"after\" OID to null. When an 'update-ref' line is\nencountered in the 'rebase-todo', the \"after\" OID is updated with the\nnewly-rebased value. However, if an 'update-ref' line is removed from the\n'rebase-todo', the \"after\" value is never updated. Then, when the rebase\nfinishes and the ref state data is applied, all of the entries with null\n\"after\" OIDs are deleted.\n\nThe three options for a fix I can think of are:\n\n 1. initialize the \"after\" OID to the value of \"before\".\n 2. don't update refs with a null \"after\" OID.\n 3. initialize the \"after\" OID to the value of \"before\", don't update the\n ref if \"before\" == \"after\".\n\nI think #3 is the best option, since it avoids the unnecessary updates of #1\nand leaves a cleaner path to a 'delete-ref' option (like the one proposed\nelsewhere in the thread [1]) than #2. I'll send a patch shortly. \n\n[1] https://lore.kernel.org/git/CA+JQ7M-GbBTHZZ9xOLR=FitWFpUnkfuep9kSfNPxuSbJbKteGw@mail.gmail.com/\n\nThanks for reporting!\n- Victoria\n\n"},{"id":"466496","messageId":"c195b67c-4dbc-a8b8-8513-2664e1ca2404@dunelm.org.uk","threadId":"58660","inReplyTo":"123628cc-1410-aaa0-0151-2dff35bd1855@github.com","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-04T10:40:39Z","receivedAt":"2022-11-04T10:40:58Z","isPatch":false,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nOn 04/11/2022 00:31, Victoria Dye wrote:\n> herr.kaste wrote:\n>> Now, I should just have not used `--update-refs` in the first place but anyway\n>> I decide late that I rather don't want to update \"master\" etc. and it should\n>> probably not delete the local refs.\n>>\n> \n> Agreed, this doesn't seem like desired behavior - the opposite of \"update\n> the ref\" isn't \"delete the ref\". ;)\n> \n> The reason it's happening is because, when '--update-refs' is used, the\n> rebase starts by constructing a list of 'update_ref_record's for each of the\n> refs that *could* be updated. Each item in that list contains the\n> corresponding ref's \"before\" commit OID (i.e., what it currently points to)\n> and initializes the \"after\" OID to null. When an 'update-ref' line is\n> encountered in the 'rebase-todo', the \"after\" OID is updated with the\n> newly-rebased value. However, if an 'update-ref' line is removed from the\n> 'rebase-todo', the \"after\" value is never updated. Then, when the rebase\n> finishes and the ref state data is applied, all of the entries with null\n> \"after\" OIDs are deleted.\n> \n> The three options for a fix I can think of are:\n> \n> 1. initialize the \"after\" OID to the value of \"before\".\n> 2. don't update refs with a null \"after\" OID.\n> 3. initialize the \"after\" OID to the value of \"before\", don't update the\n> ref if \"before\" == \"after\".\n> \n> I think #3 is the best option, since it avoids the unnecessary updates of #1\n> and leaves a cleaner path to a 'delete-ref' option (like the one proposed\n> elsewhere in the thread [1]) than #2. I'll send a patch shortly.\n\nWe should be removing the entry entirely if the user removes it from the \ntodo-list see b3b1a21d1a (sequencer: rewrite update-refs as user edits \ntodo list, 2022-07-19) where the commit message says\n\n1. If a '//' triple in the update-refs file does not\n have a matching 'update-ref ' command in the todo-list _and_ the\n value is the null OID, then remove that triple. Here, the\n user removed the 'update-ref ' command before it was executed,\n since if it was executed then the value would store the\n commit at that position.\n\nI think that is the best approach but it seems the implementation isn't \nactually doing that.\n\nBest Wishes\n\nPhillip\n\n\n\n> [1] https://lore.kernel.org/git/CA+JQ7M-GbBTHZZ9xOLR=FitWFpUnkfuep9kSfNPxuSbJbKteGw@mail.gmail.com/\n> \n> Thanks for reporting!\n> - Victoria\n> \n"},{"id":"466540","messageId":"bf5bc739-cb88-61ff-ed6b-09b1316f2f35@github.com","threadId":"58660","inReplyTo":"c195b67c-4dbc-a8b8-8513-2664e1ca2404@dunelm.org.uk","subject":"Re: rebase -i --update-refs can lead to deletion of branches","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-04T15:28:32Z","receivedAt":"2022-11-04T15:28:57Z","isPatch":false,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"Phillip Wood wrote:\n> We should be removing the entry entirely if the user removes it from the\n> todo-list see b3b1a21d1a (sequencer: rewrite update-refs as user edits\n> todo list, 2022-07-19) where the commit message says\n> \n> 1. If a '//' triple in the update-refs file does not\n>    have a matching 'update-ref ' command in the todo-list _and_ the\n>    value is the null OID, then remove that triple. Here, the\n>    user removed the 'update-ref ' command before it was executed,\n>    since if it was executed then the value would store the\n>    commit at that position.\n> \n> I think that is the best approach but it seems the implementation isn't\n> actually doing that.\n\nThanks for pointing this out. This approach seems to have only been applied\nto 'git rebase --edit-todo', so ideally the fix will just be \"do the same\nthing in the initial rebase.\"\n\nI got sidetracked yesterday and didn't get as much time to work on this as\nI'd liked, but I should be able to send a patch today.\n"},{"id":"466543","messageId":"20221104165735.68899-1-vdye@github.com","threadId":"58660","inReplyTo":"bf5bc739-cb88-61ff-ed6b-09b1316f2f35@github.com","subject":"[PATCH] rebase --update-refs: avoid unintended ref deletion","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-04T16:57:36Z","receivedAt":"2022-11-04T16:58:10Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,\n2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle\nthe removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it\nremoves potential ref updates from the \"update refs state\" if a ref does not\nhave a corresponding 'update-ref' line.\n\nHowever, because 'write_update_refs_state()' will not update the state if\nthe 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\nresult in the state remaining unchanged from how it was initialized (with\nall refs' \"after\" OID being null). Then, when the ref update is applied, all\nrefs will be updated to null and consequently deleted.\n\nTo fix this, add a 'force_if_empty' flag to allow writing the update refs\nstate even if 'refs_to_oids' is empty. The three usages of\n'write_update_refs_state()' are updated as follows:\n\n- in 'todo_list_filter_update_refs()': force_if_empty is 1 because update\n ref entries are removed here. This setting fixes the ref deletion issue.\n- in 'do_update_ref()': force_if_empty is 0, since this method only modifies\n (does not add or delete) ref update entries.\n- in 'todo_list_add_update_ref_commands()': force_if_empty is 0, since this\n method strictly adds ref update entries.\n\nAdditionally, add a test covering the \"all update-ref lines removed\" case.\n\nReported-by: herr.kaste \nSigned-off-by: Victoria Dye \n---\nThis fixes the issue reported in [1]. I initially misinterpreted the root\ncause (thought that 'todo_list_filter_update_refs()' was only applied in the\ncase of '--edit-todo'). After looking into it a bit more, it appears that\nthe actual failure case is much narrower, occurring only when *all*\n'update-ref' lines were deleted from the 'rebase-todo'.\n\nThanks!\n- Victoria\n\n[1] https://lore.kernel.org/git/CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@mail.gmail.com/\n\n sequencer.c | 15 ++++++++++-----\n t/t3404-rebase-interactive.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 34 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..4d99a4fd6ca 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4122,7 +4122,7 @@ static int do_merge(struct repository *r,\n \treturn ret;\n }\n\n-static int write_update_refs_state(struct string_list *refs_to_oids)\n+static int write_update_refs_state(struct string_list *refs_to_oids, int force_if_empty)\n {\n \tint result = 0;\n \tstruct lock_file lock = LOCK_INIT;\n@@ -4130,7 +4130,12 @@ static int write_update_refs_state(struct string_list *refs_to_oids)\n \tstruct string_list_item *item;\n \tchar *path;\n\n-\tif (!refs_to_oids->nr)\n+\t/*\n+\t * If 'force' is specified, we want to write the updated refs even if\n+\t * the list is empty. This is only needed for callers that may have\n+\t * deleted items from 'refs_to_oids'.\n+\t */\n+\tif (!refs_to_oids->nr && !force_if_empty)\n \t\treturn 0;\n\n \tpath = rebase_path_update_refs(the_repository->gitdir);\n@@ -4260,7 +4265,7 @@ void todo_list_filter_update_refs(struct repository *r,\n \t}\n\n \tif (updated)\n-\t\twrite_update_refs_state(&update_refs);\n+\t\twrite_update_refs_state(&update_refs, 1);\n \tstring_list_clear(&update_refs, 1);\n }\n\n@@ -4281,7 +4286,7 @@ static int do_update_ref(struct repository *r, const char *refname)\n \t\t}\n \t}\n\n-\twrite_update_refs_state(&list);\n+\twrite_update_refs_state(&list, 0);\n \tstring_list_clear(&list, 1);\n \treturn 0;\n }\n@@ -6015,7 +6020,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)\n \t\t}\n \t}\n\n-\tres = write_update_refs_state(&ctx.refs_to_oids);\n+\tres = write_update_refs_state(&ctx.refs_to_oids, 0);\n\n \tstring_list_clear(&ctx.refs_to_oids, 1);\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 4f5abb5ad25..e7d3721ece8 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1964,6 +1964,30 @@ test_expect_success 'respect user edits to update-ref steps' '\n \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n '\n\n+test_expect_success '--update-refs: do not delete refs if all update-ref are removed' '\n+\tgit checkout -b test-refs-not-removed no-conflict-branch &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit branch -f tip &&\n+\t(\n+\t\tset_cat_todo_editor &&\n+\t\ttest_must_fail git rebase -i --update-refs base >todo.raw &&\n+\t\tsed -e \"/^update-ref/d\" todo\n+\t) &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --update-refs base\n+\t) &&\n+\n+\ttest_cmp_rev HEAD~3 refs/heads/first &&\n+\ttest_cmp_rev HEAD~3 refs/heads/second &&\n+\ttest_cmp_rev HEAD~1 refs/heads/third &&\n+\ttest_cmp_rev HEAD refs/heads/tip &&\n+\ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n+'\n+\n test_expect_success '--update-refs: check failed ref update' '\n \tgit checkout -B update-refs-error no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n--\n2.38.0\n\n"},{"id":"466548","messageId":"Y2VrhR6b0SzG1HEA@nand.local","threadId":"58660","inReplyTo":"20221104165735.68899-1-vdye@github.com","subject":"Re: [PATCH] rebase --update-refs: avoid unintended ref deletion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-04T19:44:05Z","receivedAt":"2022-11-04T19:44:11Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Fri, Nov 04, 2022 at 09:57:36AM -0700, Victoria Dye wrote:\n> However, because 'write_update_refs_state()' will not update the state if\n> the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\n> result in the state remaining unchanged from how it was initialized (with\n> all refs' \"after\" OID being null). Then, when the ref update is applied, all\n> refs will be updated to null and consequently deleted.\n\nGood catch.\n\nI wonder, though: should we only add pending ref updates to the\nupdate-refs state after we reach that point in the sequence?\n\nIOW: there is no world where deleting an update-refs command means to\ndrop the affected branch, right? So the initial state would be an empty\nlist, which would cause us to not update any references.\n\nThen as we proceed through the rebase, we accumulate update-refs\ncommands, and know their after_oid immediately. Then when we're done, we\ncan process the update-refs commands for the branches that we do have.\n\nThe more I think about this, the more that I am convinced that the bug\nis in how we initialize the pending list, not our treatment of it later\non.\n\nThe bug fix works as-is, but I can't help wonder if the above approach\nmight be more direct.\n\nThanks,\nTaylor\n"},{"id":"466549","messageId":"6c022318-afc0-2ad7-b29c-ccb87f2f2e94@dunelm.org.uk","threadId":"58660","inReplyTo":"20221104165735.68899-1-vdye@github.com","subject":"Re: [PATCH] rebase --update-refs: avoid unintended ref deletion","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-04T20:12:23Z","receivedAt":"2022-11-04T20:12:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nOn 04/11/2022 16:57, Victoria Dye wrote:\n> In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,\n> 2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle\n> the removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it\n> removes potential ref updates from the \"update refs state\" if a ref does not\n> have a corresponding 'update-ref' line.\n> \n> However, because 'write_update_refs_state()' will not update the state if\n> the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\n> result in the state remaining unchanged from how it was initialized (with\n> all refs' \"after\" OID being null). Then, when the ref update is applied, all\n> refs will be updated to null and consequently deleted.\n\nThanks for taking the time to track down the cause of this bug and fix it.\n\n> To fix this, add a 'force_if_empty' flag to allow writing the update refs\n> state even if 'refs_to_oids' is empty. The three usages of\n> 'write_update_refs_state()' are updated as follows:\n> \n> - in 'todo_list_filter_update_refs()': force_if_empty is 1 because update\n> ref entries are removed here. This setting fixes the ref deletion issue.\n> - in 'do_update_ref()': force_if_empty is 0, since this method only modifies\n> (does not add or delete) ref update entries.\n> - in 'todo_list_add_update_ref_commands()': force_if_empty is 0, since this\n> method strictly adds ref update entries.\n\nI think not writing the list if it is empty is just an optimization to \navoid creating an empty file. I wonder if it would be simpler to \nunlink() any existing file if write_update_refs_state() is called with \nan empty list rather than adding the force flag.\n\n> Additionally, add a test covering the \"all update-ref lines removed\" case.\n\nThat's great\n\nBest Wishes\n\nPhillip\n\n> Reported-by: herr.kaste \n> Signed-off-by: Victoria Dye \n> ---\n> This fixes the issue reported in [1]. I initially misinterpreted the root\n> cause (thought that 'todo_list_filter_update_refs()' was only applied in the\n> case of '--edit-todo'). After looking into it a bit more, it appears that\n> the actual failure case is much narrower, occurring only when *all*\n> 'update-ref' lines were deleted from the 'rebase-todo'.\n> \n> Thanks!\n> - Victoria\n> \n> [1] https://lore.kernel.org/git/CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@mail.gmail.com/\n> \n> sequencer.c | 15 ++++++++++-----\n> t/t3404-rebase-interactive.sh | 24 ++++++++++++++++++++++++\n> 2 files changed, 34 insertions(+), 5 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index e658df7e8ff..4d99a4fd6ca 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4122,7 +4122,7 @@ static int do_merge(struct repository *r,\n> \treturn ret;\n> }\n> \n> -static int write_update_refs_state(struct string_list *refs_to_oids)\n> +static int write_update_refs_state(struct string_list *refs_to_oids, int force_if_empty)\n> {\n> \tint result = 0;\n> \tstruct lock_file lock = LOCK_INIT;\n> @@ -4130,7 +4130,12 @@ static int write_update_refs_state(struct string_list *refs_to_oids)\n> \tstruct string_list_item *item;\n> \tchar *path;\n> \n> -\tif (!refs_to_oids->nr)\n> +\t/*\n> +\t * If 'force' is specified, we want to write the updated refs even if\n> +\t * the list is empty. This is only needed for callers that may have\n> +\t * deleted items from 'refs_to_oids'.\n> +\t */\n> +\tif (!refs_to_oids->nr && !force_if_empty)\n> \t\treturn 0;\n> \n> \tpath = rebase_path_update_refs(the_repository->gitdir);\n> @@ -4260,7 +4265,7 @@ void todo_list_filter_update_refs(struct repository *r,\n> \t}\n> \n> \tif (updated)\n> -\t\twrite_update_refs_state(&update_refs);\n> +\t\twrite_update_refs_state(&update_refs, 1);\n> \tstring_list_clear(&update_refs, 1);\n> }\n> \n> @@ -4281,7 +4286,7 @@ static int do_update_ref(struct repository *r, const char *refname)\n> \t\t}\n> \t}\n> \n> -\twrite_update_refs_state(&list);\n> +\twrite_update_refs_state(&list, 0);\n> \tstring_list_clear(&list, 1);\n> \treturn 0;\n> }\n> @@ -6015,7 +6020,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list)\n> \t\t}\n> \t}\n> \n> -\tres = write_update_refs_state(&ctx.refs_to_oids);\n> +\tres = write_update_refs_state(&ctx.refs_to_oids, 0);\n> \n> \tstring_list_clear(&ctx.refs_to_oids, 1);\n> \n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 4f5abb5ad25..e7d3721ece8 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1964,6 +1964,30 @@ test_expect_success 'respect user edits to update-ref steps' '\n> \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n> '\n> \n> +test_expect_success '--update-refs: do not delete refs if all update-ref are removed' '\n> +\tgit checkout -b test-refs-not-removed no-conflict-branch &&\n> +\tgit branch -f base HEAD~4 &&\n> +\tgit branch -f first HEAD~3 &&\n> +\tgit branch -f second HEAD~3 &&\n> +\tgit branch -f third HEAD~1 &&\n> +\tgit branch -f tip &&\n> +\t(\n> +\t\tset_cat_todo_editor &&\n> +\t\ttest_must_fail git rebase -i --update-refs base >todo.raw &&\n> +\t\tsed -e \"/^update-ref/d\" todo\n> +\t) &&\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\tgit rebase -i --update-refs base\n> +\t) &&\n> +\n> +\ttest_cmp_rev HEAD~3 refs/heads/first &&\n> +\ttest_cmp_rev HEAD~3 refs/heads/second &&\n> +\ttest_cmp_rev HEAD~1 refs/heads/third &&\n> +\ttest_cmp_rev HEAD refs/heads/tip &&\n> +\ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n> +'\n> +\n> test_expect_success '--update-refs: check failed ref update' '\n> \tgit checkout -B update-refs-error no-conflict-branch &&\n> \tgit branch -f base HEAD~4 &&\n> --\n> 2.38.0\n> \n"},{"id":"466550","messageId":"1ad5f905-f9f1-5a06-e419-476032a0d237@dunelm.org.uk","threadId":"58660","inReplyTo":"Y2VrhR6b0SzG1HEA@nand.local","subject":"Re: [PATCH] rebase --update-refs: avoid unintended ref deletion","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-04T20:17:53Z","receivedAt":"2022-11-04T20:17:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Taylor\n\nOn 04/11/2022 19:44, Taylor Blau wrote:\n> On Fri, Nov 04, 2022 at 09:57:36AM -0700, Victoria Dye wrote:\n>> However, because 'write_update_refs_state()' will not update the state if\n>> the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\n>> result in the state remaining unchanged from how it was initialized (with\n>> all refs' \"after\" OID being null). Then, when the ref update is applied, all\n>> refs will be updated to null and consequently deleted.\n> \n> Good catch.\n> \n> I wonder, though: should we only add pending ref updates to the\n> update-refs state after we reach that point in the sequence?\n\nIf I remember correctly the aim of the current behavior is to detect if \nanother process also updates the ref while we're rebasing. To do that we \nneed to record all the branch heads that have update-ref commands at the \nstart of the rebase.\n\nBest Wishes\n\nPhillip\n\n> IOW: there is no world where deleting an update-refs command means to\n> drop the affected branch, right? So the initial state would be an empty\n> list, which would cause us to not update any references.\n> \n> Then as we proceed through the rebase, we accumulate update-refs\n> commands, and know their after_oid immediately. Then when we're done, we\n> can process the update-refs commands for the branches that we do have.\n> \n> The more I think about this, the more that I am convinced that the bug\n> is in how we initialize the pending list, not our treatment of it later\n> on.\n> \n> The bug fix works as-is, but I can't help wonder if the above approach\n> might be more direct.\n> \n> Thanks,\n> Taylor\n"},{"id":"466640","messageId":"20221c91-e473-c66a-a0bd-b650a6626e31@github.com","threadId":"58660","inReplyTo":"6c022318-afc0-2ad7-b29c-ccb87f2f2e94@dunelm.org.uk","subject":"Re: [PATCH] rebase --update-refs: avoid unintended ref deletion","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T02:39:56Z","receivedAt":"2022-11-07T02:40:04Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/4/22 4:12 PM, Phillip Wood wrote:\n> Hi Victoria\n> \n> On 04/11/2022 16:57, Victoria Dye wrote:\n>> In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,\n>> 2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle\n>> the removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it\n>> removes potential ref updates from the \"update refs state\" if a ref does not\n>> have a corresponding 'update-ref' line.\n>>\n>> However, because 'write_update_refs_state()' will not update the state if\n>> the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\n>> result in the state remaining unchanged from how it was initialized (with\n>> all refs' \"after\" OID being null). Then, when the ref update is applied, all\n>> refs will be updated to null and consequently deleted.\n> \n> Thanks for taking the time to track down the cause of this bug and fix it.\n\nI will add my thanks, too. Thanks for jumping in when I could not!\n\n>> To fix this, add a 'force_if_empty' flag to allow writing the update refs\n>> state even if 'refs_to_oids' is empty. The three usages of\n>> 'write_update_refs_state()' are updated as follows:\n>>\n>> - in 'todo_list_filter_update_refs()': force_if_empty is 1 because update\n>>    ref entries are removed here. This setting fixes the ref deletion issue.\n>> - in 'do_update_ref()': force_if_empty is 0, since this method only modifies\n>>    (does not add or delete) ref update entries.\n>> - in 'todo_list_add_update_ref_commands()': force_if_empty is 0, since this\n>>    method strictly adds ref update entries.\n> \n> I think not writing the list if it is empty is just an optimization to avoid creating an empty file. I wonder if it would be simpler to unlink() any existing file if write_update_refs_state() is called with an empty list rather than adding the force flag.\n\nI agree that an unlink() is the best option, barring one point\n(that I will mention below).\n\n>> +test_expect_success '--update-refs: do not delete refs if all update-ref are removed' '\n>> +    git checkout -b test-refs-not-removed no-conflict-branch &&\n>> +    git branch -f base HEAD~4 &&\n>> +    git branch -f first HEAD~3 &&\n>> +    git branch -f second HEAD~3 &&\n>> +    git branch -f third HEAD~1 &&\n>> +    git branch -f tip &&\n>> +    (\n>> +        set_cat_todo_editor &&\n>> +        test_must_fail git rebase -i --update-refs base >todo.raw &&\n>> +        sed -e \"/^update-ref/d\" todo\n>> +    ) &&\n>> +    (\n>> +        set_replace_editor todo &&\n>> +        git rebase -i --update-refs base\n>> +    ) &&\n>> +\n>> +    test_cmp_rev HEAD~3 refs/heads/first &&\n>> +    test_cmp_rev HEAD~3 refs/heads/second &&\n>> +    test_cmp_rev HEAD~1 refs/heads/third &&\n>> +    test_cmp_rev HEAD refs/heads/tip &&\n>> +    test_cmp_rev HEAD refs/heads/no-conflict-branch\n>> +'\n>> +\n\nThis is a great test! I'm glad that it handles the existing\ncase. I think the only case that might be interesting is to\nmake the rebase actually create new commits and show that\nthe removed refs are no longer in the history of the new\nbranch, but are instead reachable from the older tip.\n\nFor this test, we could create a 'fixup!' to 'first' and\nuse --autosquash to generate new commits. At the end, we\ncan compare first, second, and third to different ancestors\nof refs/heads/no-conflict-branch _and_ guarantee that\nno-conflict-branch did not move. Or:\n\n git rev-parse first second third no-conflict-branch >expect-oids &&\n ...do the rebase...\n git rev-parse first second third no-conflict-branch >actual-oids &&\n test_cmp expect-oids actual-oids\n\nshould work to guarantee these refs were not updated.\n\nI wonder if anything interesting happens if after we remove\nthe update-ref commands we have a 'break' command and then\nre-insert some of the commands. Will things like the unlink()\ndirection cause a problem then?\n\nAnyway, here is a potential diff on top of your patch that\nadds these modifications to the test. They work with your\nimplementation changes, but I also didn't try the unlink()\nmodification.\n\n--- >8 ---\n\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex e7d3721ece8..4b09b73525a 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1971,21 +1971,60 @@ test_expect_success '--update-refs: do not delete refs if all update-ref are rem\n \tgit branch -f second HEAD~3 &&\n \tgit branch -f third HEAD~1 &&\n \tgit branch -f tip &&\n+\n+\ttest_commit test-refs-not-removed &&\n+\tgit commit --amend --fixup first &&\n+\n+\tgit rev-parse first second third tip no-conflict-branch >expect-oids &&\n+\n \t(\n \t\tset_cat_todo_editor &&\n-\t\ttest_must_fail git rebase -i --update-refs base >todo.raw &&\n+\t\ttest_must_fail git rebase -i \\\n+\t\t\t--autosquash --update-refs \\\n+\t\t\tbase >todo.raw &&\n \t\tsed -e \"/^update-ref/d\" todo\n \t) &&\n \t(\n \t\tset_replace_editor todo &&\n-\t\tgit rebase -i --update-refs base\n+\t\tgit rebase -i --autosquash --update-refs base\n \t) &&\n \n-\ttest_cmp_rev HEAD~3 refs/heads/first &&\n-\ttest_cmp_rev HEAD~3 refs/heads/second &&\n-\ttest_cmp_rev HEAD~1 refs/heads/third &&\n-\ttest_cmp_rev HEAD refs/heads/tip &&\n-\ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n+\tgit rev-parse first second third tip no-conflict-branch >actual-oids &&\n+\ttest_cmp expect-oids actual-oids\n+'\n+\n+test_expect_success '--update-refs: do not delete refs if all update-ref are removed and some re-added' '\n+\tgit checkout -b test-refs-not-removed2 no-conflict-branch &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit branch -f tip &&\n+\n+\ttest_commit test-refs-not-removed2 &&\n+\tgit commit --amend --fixup first &&\n+\n+\tgit rev-parse first second third >expect-oids &&\n+\n+\t(\n+\t\tset_cat_todo_editor &&\n+\t\ttest_must_fail git rebase -i \\\n+\t\t\t--autosquash --update-refs \\\n+\t\t\tbase >todo.raw &&\n+\t\tsed -e \"/^update-ref/d\" todo\n+\t) &&\n+\techo \"break\" >>todo &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --autosquash --update-refs base &&\n+\t\techo \"update-ref refs/heads/tip\" >todo &&\n+\t\tgit rebase --edit-todo &&\n+\t\tgit rebase --continue\n+\t) &&\n+\n+\tgit rev-parse first second third >actual-oids &&\n+\ttest_cmp expect-oids actual-oids &&\n+\ttest_cmp_rev HEAD tip\n '\n \n test_expect_success '--update-refs: check failed ref update' '\n\n--- >8 ---\n\nThanks,\n-Stolee\n"},{"id":"466681","messageId":"20221107174752.91186-1-vdye@github.com","threadId":"58660","inReplyTo":"20221104165735.68899-1-vdye@github.com","subject":"[PATCH v2] rebase --update-refs: avoid unintended ref deletion","fromName":"Victoria Dye","fromEmail":"vdye@github.com","sentAt":"2022-11-07T17:47:52Z","receivedAt":"2022-11-07T17:48:38Z","isPatch":true,"sender":{"key":"vdye@github.com","avatar":"https://avatars.githubusercontent.com/u/3619353?v=4"},"body":"In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,\n2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle\nthe removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it\nremoves potential ref updates from the \"update refs state\" if a ref does not\nhave a corresponding 'update-ref' line.\n\nHowever, because 'write_update_refs_state()' will not update the state if\nthe 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\nresult in the state remaining unchanged from how it was initialized (with\nall refs' \"after\" OID being null). Then, when the ref update is applied, all\nrefs will be updated to null and consequently deleted.\n\nTo fix this, delete the 'update-refs' state file when 'refs_to_oids' is\nempty. Additionally, add a tests covering \"all update-ref lines removed\"\ncases.\n\nReported-by: herr.kaste \nHelped-by: Phillip Wood \nHelped-by: Derrick Stolee \nSigned-off-by: Victoria Dye \n---\nChanges since v1:\n- Modified approach to handling empty 'refs_to_oids' from \"optional force write\n empty file\" to \"always unlink\"\n- Added/updated tests\n\n sequencer.c | 9 ++-\n t/t3404-rebase-interactive.sh | 107 ++++++++++++++++++++++++++++++++++\n 2 files changed, 113 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex e658df7e8ff..798a9702961 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -4130,11 +4130,14 @@ static int write_update_refs_state(struct string_list *refs_to_oids)\n \tstruct string_list_item *item;\n \tchar *path;\n\n-\tif (!refs_to_oids->nr)\n-\t\treturn 0;\n-\n \tpath = rebase_path_update_refs(the_repository->gitdir);\n\n+\tif (!refs_to_oids->nr) {\n+\t\tif (unlink(path) && errno != ENOENT)\n+\t\t\tresult = error_errno(_(\"could not unlink: %s\"), path);\n+\t\tgoto cleanup;\n+\t}\n+\n \tif (safe_create_leading_directories(path)) {\n \t\tresult = error(_(\"unable to create leading directories of %s\"),\n \t\t\t path);\ndiff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\nindex 4f5abb5ad25..462cefd25df 100755\n--- a/t/t3404-rebase-interactive.sh\n+++ b/t/t3404-rebase-interactive.sh\n@@ -1964,6 +1964,113 @@ test_expect_success 'respect user edits to update-ref steps' '\n \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n '\n\n+test_expect_success '--update-refs: all update-ref lines removed' '\n+\tgit checkout -b test-refs-not-removed no-conflict-branch &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit branch -f tip &&\n+\n+\ttest_commit test-refs-not-removed &&\n+\tgit commit --amend --fixup first &&\n+\n+\tgit rev-parse first second third tip no-conflict-branch >expect-oids &&\n+\n+\t(\n+\t\tset_cat_todo_editor &&\n+\t\ttest_must_fail git rebase -i --update-refs base >todo.raw &&\n+\t\tsed -e \"/^update-ref/d\" todo\n+\t) &&\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --update-refs base\n+\t) &&\n+\n+\t# Ensure refs are not deleted and their OIDs have not changed\n+\tgit rev-parse first second third tip no-conflict-branch >actual-oids &&\n+\ttest_cmp expect-oids actual-oids\n+'\n+\n+test_expect_success '--update-refs: all update-ref lines removed, then some re-added' '\n+\tgit checkout -b test-refs-not-removed2 no-conflict-branch &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit branch -f tip &&\n+\n+\ttest_commit test-refs-not-removed2 &&\n+\tgit commit --amend --fixup first &&\n+\n+\tgit rev-parse first second third >expect-oids &&\n+\n+\t(\n+\t\tset_cat_todo_editor &&\n+\t\ttest_must_fail git rebase -i \\\n+\t\t\t--autosquash --update-refs \\\n+\t\t\tbase >todo.raw &&\n+\t\tsed -e \"/^update-ref/d\" todo\n+\t) &&\n+\n+\t# Add a break to the end of the todo so we can edit later\n+\techo \"break\" >>todo &&\n+\n+\t(\n+\t\tset_replace_editor todo &&\n+\t\tgit rebase -i --autosquash --update-refs base &&\n+\t\techo \"update-ref refs/heads/tip\" >todo &&\n+\t\tgit rebase --edit-todo &&\n+\t\tgit rebase --continue\n+\t) &&\n+\n+\t# Ensure first/second/third are unchanged, but tip is updated\n+\tgit rev-parse first second third >actual-oids &&\n+\ttest_cmp expect-oids actual-oids &&\n+\ttest_cmp_rev HEAD tip\n+'\n+\n+test_expect_success '--update-refs: --edit-todo with no update-ref lines' '\n+\tgit checkout -b test-refs-not-removed3 no-conflict-branch &&\n+\tgit branch -f base HEAD~4 &&\n+\tgit branch -f first HEAD~3 &&\n+\tgit branch -f second HEAD~3 &&\n+\tgit branch -f third HEAD~1 &&\n+\tgit branch -f tip &&\n+\n+\ttest_commit test-refs-not-removed3 &&\n+\tgit commit --amend --fixup first &&\n+\n+\tgit rev-parse first second third tip no-conflict-branch >expect-oids &&\n+\n+\t(\n+\t\tset_cat_todo_editor &&\n+\t\ttest_must_fail git rebase -i \\\n+\t\t\t--autosquash --update-refs \\\n+\t\t\tbase >todo.raw &&\n+\t\tsed -e \"/^update-ref/d\" todo\n+\t) &&\n+\n+\t# Add a break to the beginning of the todo so we can resume with no\n+\t# update-ref lines\n+\techo \"break\" >todo.new &&\n+\tcat todo >>todo.new &&\n+\n+\t(\n+\t\tset_replace_editor todo.new &&\n+\t\tgit rebase -i --autosquash --update-refs base &&\n+\n+\t\t# Make no changes when editing so update-refs is still empty\n+\t\tcat todo >todo.new &&\n+\t\tgit rebase --edit-todo &&\n+\t\tgit rebase --continue\n+\t) &&\n+\n+\t# Ensure refs are not deleted and their OIDs have not changed\n+\tgit rev-parse first second third tip no-conflict-branch >actual-oids &&\n+\ttest_cmp expect-oids actual-oids\n+'\n+\n test_expect_success '--update-refs: check failed ref update' '\n \tgit checkout -B update-refs-error no-conflict-branch &&\n \tgit branch -f base HEAD~4 &&\n--\n2.38.0\n\n"},{"id":"466716","messageId":"Y2lZv7cgbn4Dx8Jb@nand.local","threadId":"58660","inReplyTo":"20221107174752.91186-1-vdye@github.com","subject":"Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2022-11-07T19:17:19Z","receivedAt":"2022-11-07T19:17:24Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Nov 07, 2022 at 09:47:52AM -0800, Victoria Dye wrote:\n> sequencer.c | 9 ++-\n> t/t3404-rebase-interactive.sh | 107 ++++++++++++++++++++++++++++++++++\n> 2 files changed, 113 insertions(+), 3 deletions(-)\n\nLooks great, thanks. Will queue.\n\nThanks,\nTaylor\n"},{"id":"466719","messageId":"6c19845a-b7e5-da85-34d5-0461960668bc@github.com","threadId":"58660","inReplyTo":"20221107174752.91186-1-vdye@github.com","subject":"Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2022-11-07T19:25:09Z","receivedAt":"2022-11-07T19:25:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 11/7/22 12:47 PM, Victoria Dye wrote:\n> In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,\n> 2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle\n> the removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it\n> removes potential ref updates from the \"update refs state\" if a ref does not\n> have a corresponding 'update-ref' line.\n> \n> However, because 'write_update_refs_state()' will not update the state if\n> the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\n> result in the state remaining unchanged from how it was initialized (with\n> all refs' \"after\" OID being null). Then, when the ref update is applied, all\n> refs will be updated to null and consequently deleted.\n> \n> To fix this, delete the 'update-refs' state file when 'refs_to_oids' is\n> empty. Additionally, add a tests covering \"all update-ref lines removed\"\n> cases.\n> \n> Reported-by: herr.kaste \n> Helped-by: Phillip Wood \n> Helped-by: Derrick Stolee \n> Signed-off-by: Victoria Dye \n> ---\n> Changes since v1:\n> - Modified approach to handling empty 'refs_to_oids' from \"optional force write\n> empty file\" to \"always unlink\"\n> - Added/updated tests\n\nThis \"always unlink\" version is much cleaner. Thanks!\n\nThe new tests look great and I'm confident that they\nare exercising the unlink() followed by a retry of\nparsing the update-refs steps.\n\nThis version LGTM.\n\nThanks,\n-Stolee\n"},{"id":"466802","messageId":"934a0862-2713-7705-9155-6584449397c7@dunelm.org.uk","threadId":"58660","inReplyTo":"20221107174752.91186-1-vdye@github.com","subject":"Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-11-08T09:58:49Z","receivedAt":"2022-11-08T09:58:56Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Victoria\n\nOn 07/11/2022 17:47, Victoria Dye wrote:\n> In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list,\n> 2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle\n> the removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it\n> removes potential ref updates from the \"update refs state\" if a ref does not\n> have a corresponding 'update-ref' line.\n> \n> However, because 'write_update_refs_state()' will not update the state if\n> the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will\n> result in the state remaining unchanged from how it was initialized (with\n> all refs' \"after\" OID being null). Then, when the ref update is applied, all\n> refs will be updated to null and consequently deleted.\n> \n> To fix this, delete the 'update-refs' state file when 'refs_to_oids' is\n> empty. Additionally, add a tests covering \"all update-ref lines removed\"\n> cases.\n\nThanks for re-rolling, unsurprisingly I prefer the unlink() approach to \nthe previous version. As Stolee said the test coverage looks good too.\n\nBest Wishes\n\nPhillip\n\n\n> Reported-by: herr.kaste \n> Helped-by: Phillip Wood \n> Helped-by: Derrick Stolee \n> Signed-off-by: Victoria Dye \n> ---\n> Changes since v1:\n> - Modified approach to handling empty 'refs_to_oids' from \"optional force write\n> empty file\" to \"always unlink\"\n> - Added/updated tests\n> \n> sequencer.c | 9 ++-\n> t/t3404-rebase-interactive.sh | 107 ++++++++++++++++++++++++++++++++++\n> 2 files changed, 113 insertions(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index e658df7e8ff..798a9702961 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -4130,11 +4130,14 @@ static int write_update_refs_state(struct string_list *refs_to_oids)\n> \tstruct string_list_item *item;\n> \tchar *path;\n> \n> -\tif (!refs_to_oids->nr)\n> -\t\treturn 0;\n> -\n> \tpath = rebase_path_update_refs(the_repository->gitdir);\n> \n> +\tif (!refs_to_oids->nr) {\n> +\t\tif (unlink(path) && errno != ENOENT)\n> +\t\t\tresult = error_errno(_(\"could not unlink: %s\"), path);\n> +\t\tgoto cleanup;\n> +\t}\n> +\n> \tif (safe_create_leading_directories(path)) {\n> \t\tresult = error(_(\"unable to create leading directories of %s\"),\n> \t\t\t path);\n> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh\n> index 4f5abb5ad25..462cefd25df 100755\n> --- a/t/t3404-rebase-interactive.sh\n> +++ b/t/t3404-rebase-interactive.sh\n> @@ -1964,6 +1964,113 @@ test_expect_success 'respect user edits to update-ref steps' '\n> \ttest_cmp_rev HEAD refs/heads/no-conflict-branch\n> '\n> \n> +test_expect_success '--update-refs: all update-ref lines removed' '\n> +\tgit checkout -b test-refs-not-removed no-conflict-branch &&\n> +\tgit branch -f base HEAD~4 &&\n> +\tgit branch -f first HEAD~3 &&\n> +\tgit branch -f second HEAD~3 &&\n> +\tgit branch -f third HEAD~1 &&\n> +\tgit branch -f tip &&\n> +\n> +\ttest_commit test-refs-not-removed &&\n> +\tgit commit --amend --fixup first &&\n> +\n> +\tgit rev-parse first second third tip no-conflict-branch >expect-oids &&\n> +\n> +\t(\n> +\t\tset_cat_todo_editor &&\n> +\t\ttest_must_fail git rebase -i --update-refs base >todo.raw &&\n> +\t\tsed -e \"/^update-ref/d\" todo\n> +\t) &&\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\tgit rebase -i --update-refs base\n> +\t) &&\n> +\n> +\t# Ensure refs are not deleted and their OIDs have not changed\n> +\tgit rev-parse first second third tip no-conflict-branch >actual-oids &&\n> +\ttest_cmp expect-oids actual-oids\n> +'\n> +\n> +test_expect_success '--update-refs: all update-ref lines removed, then some re-added' '\n> +\tgit checkout -b test-refs-not-removed2 no-conflict-branch &&\n> +\tgit branch -f base HEAD~4 &&\n> +\tgit branch -f first HEAD~3 &&\n> +\tgit branch -f second HEAD~3 &&\n> +\tgit branch -f third HEAD~1 &&\n> +\tgit branch -f tip &&\n> +\n> +\ttest_commit test-refs-not-removed2 &&\n> +\tgit commit --amend --fixup first &&\n> +\n> +\tgit rev-parse first second third >expect-oids &&\n> +\n> +\t(\n> +\t\tset_cat_todo_editor &&\n> +\t\ttest_must_fail git rebase -i \\\n> +\t\t\t--autosquash --update-refs \\\n> +\t\t\tbase >todo.raw &&\n> +\t\tsed -e \"/^update-ref/d\" todo\n> +\t) &&\n> +\n> +\t# Add a break to the end of the todo so we can edit later\n> +\techo \"break\" >>todo &&\n> +\n> +\t(\n> +\t\tset_replace_editor todo &&\n> +\t\tgit rebase -i --autosquash --update-refs base &&\n> +\t\techo \"update-ref refs/heads/tip\" >todo &&\n> +\t\tgit rebase --edit-todo &&\n> +\t\tgit rebase --continue\n> +\t) &&\n> +\n> +\t# Ensure first/second/third are unchanged, but tip is updated\n> +\tgit rev-parse first second third >actual-oids &&\n> +\ttest_cmp expect-oids actual-oids &&\n> +\ttest_cmp_rev HEAD tip\n> +'\n> +\n> +test_expect_success '--update-refs: --edit-todo with no update-ref lines' '\n> +\tgit checkout -b test-refs-not-removed3 no-conflict-branch &&\n> +\tgit branch -f base HEAD~4 &&\n> +\tgit branch -f first HEAD~3 &&\n> +\tgit branch -f second HEAD~3 &&\n> +\tgit branch -f third HEAD~1 &&\n> +\tgit branch -f tip &&\n> +\n> +\ttest_commit test-refs-not-removed3 &&\n> +\tgit commit --amend --fixup first &&\n> +\n> +\tgit rev-parse first second third tip no-conflict-branch >expect-oids &&\n> +\n> +\t(\n> +\t\tset_cat_todo_editor &&\n> +\t\ttest_must_fail git rebase -i \\\n> +\t\t\t--autosquash --update-refs \\\n> +\t\t\tbase >todo.raw &&\n> +\t\tsed -e \"/^update-ref/d\" todo\n> +\t) &&\n> +\n> +\t# Add a break to the beginning of the todo so we can resume with no\n> +\t# update-ref lines\n> +\techo \"break\" >todo.new &&\n> +\tcat todo >>todo.new &&\n> +\n> +\t(\n> +\t\tset_replace_editor todo.new &&\n> +\t\tgit rebase -i --autosquash --update-refs base &&\n> +\n> +\t\t# Make no changes when editing so update-refs is still empty\n> +\t\tcat todo >todo.new &&\n> +\t\tgit rebase --edit-todo &&\n> +\t\tgit rebase --continue\n> +\t) &&\n> +\n> +\t# Ensure refs are not deleted and their OIDs have not changed\n> +\tgit rev-parse first second third tip no-conflict-branch >actual-oids &&\n> +\ttest_cmp expect-oids actual-oids\n> +'\n> +\n> test_expect_success '--update-refs: check failed ref update' '\n> \tgit checkout -B update-refs-error no-conflict-branch &&\n> \tgit branch -f base HEAD~4 &&\n> --\n> 2.38.0\n> \n"}]}