Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Victoria

On 07/11/2022 17:47, 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.
> 
> To fix this, delete the 'update-refs' state file when 'refs_to_oids' is
> empty. Additionally, add a tests covering "all update-ref lines removed"
> cases.

Thanks for re-rolling, unsurprisingly I prefer the unlink() approach to 
the previous version. As Stolee said the test coverage looks good too.

Best Wishes

Phillip


> Reported-by: herr.kaste <herr.kaste@gmail.com>
> Helped-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> Helped-by: Derrick Stolee <derrickstolee@github.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> Changes since v1:
> - Modified approach to handling empty 'refs_to_oids' from "optional force write
>    empty file" to "always unlink"
> - Added/updated tests
> 
>   sequencer.c                   |   9 ++-
>   t/t3404-rebase-interactive.sh | 107 ++++++++++++++++++++++++++++++++++
>   2 files changed, 113 insertions(+), 3 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e658df7e8ff..798a9702961 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4130,11 +4130,14 @@ static int write_update_refs_state(struct string_list *refs_to_oids)
>   	struct string_list_item *item;
>   	char *path;
> 
> -	if (!refs_to_oids->nr)
> -		return 0;
> -
>   	path = rebase_path_update_refs(the_repository->gitdir);
> 
> +	if (!refs_to_oids->nr) {
> +		if (unlink(path) && errno != ENOENT)
> +			result = error_errno(_("could not unlink: %s"), path);
> +		goto cleanup;
> +	}
> +
>   	if (safe_create_leading_directories(path)) {
>   		result = error(_("unable to create leading directories of %s"),
>   			       path);
> diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
> index 4f5abb5ad25..462cefd25df 100755
> --- a/t/t3404-rebase-interactive.sh
> +++ b/t/t3404-rebase-interactive.sh
> @@ -1964,6 +1964,113 @@ test_expect_success 'respect user edits to update-ref steps' '
>   	test_cmp_rev HEAD refs/heads/no-conflict-branch
>   '
> 
> +test_expect_success '--update-refs: all update-ref lines 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 &&
> +
> +	test_commit test-refs-not-removed &&
> +	git commit --amend --fixup first &&
> +
> +	git rev-parse first second third tip no-conflict-branch >expect-oids &&
> +
> +	(
> +		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
> +	) &&
> +
> +	# Ensure refs are not deleted and their OIDs have not changed
> +	git rev-parse first second third tip no-conflict-branch >actual-oids &&
> +	test_cmp expect-oids actual-oids
> +'
> +
> +test_expect_success '--update-refs: all update-ref lines removed, then some re-added' '
> +	git checkout -b test-refs-not-removed2 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 &&
> +
> +	test_commit test-refs-not-removed2 &&
> +	git commit --amend --fixup first &&
> +
> +	git rev-parse first second third >expect-oids &&
> +
> +	(
> +		set_cat_todo_editor &&
> +		test_must_fail git rebase -i \
> +			--autosquash --update-refs \
> +			base >todo.raw &&
> +		sed -e "/^update-ref/d" <todo.raw >todo
> +	) &&
> +
> +	# Add a break to the end of the todo so we can edit later
> +	echo "break" >>todo &&
> +
> +	(
> +		set_replace_editor todo &&
> +		git rebase -i --autosquash --update-refs base &&
> +		echo "update-ref refs/heads/tip" >todo &&
> +		git rebase --edit-todo &&
> +		git rebase --continue
> +	) &&
> +
> +	# Ensure first/second/third are unchanged, but tip is updated
> +	git rev-parse first second third >actual-oids &&
> +	test_cmp expect-oids actual-oids &&
> +	test_cmp_rev HEAD tip
> +'
> +
> +test_expect_success '--update-refs: --edit-todo with no update-ref lines' '
> +	git checkout -b test-refs-not-removed3 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 &&
> +
> +	test_commit test-refs-not-removed3 &&
> +	git commit --amend --fixup first &&
> +
> +	git rev-parse first second third tip no-conflict-branch >expect-oids &&
> +
> +	(
> +		set_cat_todo_editor &&
> +		test_must_fail git rebase -i \
> +			--autosquash --update-refs \
> +			base >todo.raw &&
> +		sed -e "/^update-ref/d" <todo.raw >todo
> +	) &&
> +
> +	# Add a break to the beginning of the todo so we can resume with no
> +	# update-ref lines
> +	echo "break" >todo.new &&
> +	cat todo >>todo.new &&
> +
> +	(
> +		set_replace_editor todo.new &&
> +		git rebase -i --autosquash --update-refs base &&
> +
> +		# Make no changes when editing so update-refs is still empty
> +		cat todo >todo.new &&
> +		git rebase --edit-todo &&
> +		git rebase --continue
> +	) &&
> +
> +	# Ensure refs are not deleted and their OIDs have not changed
> +	git rev-parse first second third tip no-conflict-branch >actual-oids &&
> +	test_cmp expect-oids actual-oids
> +'
> +
>   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
>