Hi Victoria On 04/11/2022 16:57, Victoria Dye wrote:
In b3b1a21d1a5 (sequencer: rewrite update-refs as user edits todo list, 2022-07-19), the 'todo_list_filter_update_refs()' step was added to handle the removal of 'update-ref' lines from a 'rebase-todo'. Specifically, it removes potential ref updates from the "update refs state" if a ref does not have a corresponding 'update-ref' line. However, because 'write_update_refs_state()' will not update the state if the 'refs_to_oids' list was empty, removing *all* 'update-ref' lines will result in the state remaining unchanged from how it was initialized (with all refs' "after" OID being null). Then, when the ref update is applied, all refs will be updated to null and consequently deleted.
Thanks for taking the time to track down the cause of this bug and fix it.
To fix this, add a 'force_if_empty' flag to allow writing the update refs state even if 'refs_to_oids' is empty. The three usages of 'write_update_refs_state()' are updated as follows: - in 'todo_list_filter_update_refs()': force_if_empty is 1 because update ref entries are removed here. This setting fixes the ref deletion issue. - in 'do_update_ref()': force_if_empty is 0, since this method only modifies (does not add or delete) ref update entries. - in 'todo_list_add_update_ref_commands()': force_if_empty is 0, since this method strictly adds ref update entries.
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.
Additionally, add a test covering the "all update-ref lines removed" case.
That's great Best Wishes Phillip
Reported-by: herr.kaste <herr.kaste@xxxxxxxxx> Signed-off-by: Victoria Dye <vdye@xxxxxxxxxx> --- This fixes the issue reported in [1]. I initially misinterpreted the root cause (thought that 'todo_list_filter_update_refs()' was only applied in the case of '--edit-todo'). After looking into it a bit more, it appears that the actual failure case is much narrower, occurring only when *all* 'update-ref' lines were deleted from the 'rebase-todo'. Thanks! - Victoria [1] https://lore.kernel.org/git/CAFzd1+5F4zqQ1CNeY2xaaf0r__JmE4ECiBt5h5OdiJHbaE78VA@xxxxxxxxxxxxxx/ sequencer.c | 15 ++++++++++----- t/t3404-rebase-interactive.sh | 24 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 5 deletions(-) diff --git a/sequencer.c b/sequencer.c index e658df7e8ff..4d99a4fd6ca 100644 --- a/sequencer.c +++ b/sequencer.c @@ -4122,7 +4122,7 @@ static int do_merge(struct repository *r, return ret; } -static int write_update_refs_state(struct string_list *refs_to_oids) +static int write_update_refs_state(struct string_list *refs_to_oids, int force_if_empty) { int result = 0; struct lock_file lock = LOCK_INIT; @@ -4130,7 +4130,12 @@ static int write_update_refs_state(struct string_list *refs_to_oids) struct string_list_item *item; char *path; - if (!refs_to_oids->nr) + /* + * If 'force' is specified, we want to write the updated refs even if + * the list is empty. This is only needed for callers that may have + * deleted items from 'refs_to_oids'. + */ + if (!refs_to_oids->nr && !force_if_empty) return 0; path = rebase_path_update_refs(the_repository->gitdir); @@ -4260,7 +4265,7 @@ void todo_list_filter_update_refs(struct repository *r, } if (updated) - write_update_refs_state(&update_refs); + write_update_refs_state(&update_refs, 1); string_list_clear(&update_refs, 1); } @@ -4281,7 +4286,7 @@ static int do_update_ref(struct repository *r, const char *refname) } } - write_update_refs_state(&list); + write_update_refs_state(&list, 0); string_list_clear(&list, 1); return 0; } @@ -6015,7 +6020,7 @@ static int todo_list_add_update_ref_commands(struct todo_list *todo_list) } } - res = write_update_refs_state(&ctx.refs_to_oids); + res = write_update_refs_state(&ctx.refs_to_oids, 0); string_list_clear(&ctx.refs_to_oids, 1); diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index 4f5abb5ad25..e7d3721ece8 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1964,6 +1964,30 @@ test_expect_success 'respect user edits to update-ref steps' ' test_cmp_rev HEAD refs/heads/no-conflict-branch ' +test_expect_success '--update-refs: do not delete refs if all update-ref are removed' ' + git checkout -b test-refs-not-removed no-conflict-branch && + git branch -f base HEAD~4 && + git branch -f first HEAD~3 && + git branch -f second HEAD~3 && + git branch -f third HEAD~1 && + git branch -f tip && + ( + set_cat_todo_editor && + test_must_fail git rebase -i --update-refs base >todo.raw && + sed -e "/^update-ref/d" <todo.raw >todo + ) && + ( + set_replace_editor todo && + git rebase -i --update-refs base + ) && + + test_cmp_rev HEAD~3 refs/heads/first && + test_cmp_rev HEAD~3 refs/heads/second && + test_cmp_rev HEAD~1 refs/heads/third && + test_cmp_rev HEAD refs/heads/tip && + test_cmp_rev HEAD refs/heads/no-conflict-branch +' + test_expect_success '--update-refs: check failed ref update' ' git checkout -B update-refs-error no-conflict-branch && git branch -f base HEAD~4 && -- 2.38.0
