diff mbox series

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

Message ID 20221107174752.91186-1-vdye@github.com (mailing list archive)
State New, archived
Headers show
Series [v2] rebase --update-refs: avoid unintended ref deletion | expand

Commit Message

Victoria Dye Nov. 7, 2022, 5:47 p.m. UTC 
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.

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(-)

--
2.38.0

Comments

Taylor Blau Nov. 7, 2022, 7:17 p.m. UTC | #1 
On Mon, Nov 07, 2022 at 09:47:52AM -0800, Victoria Dye wrote:
>  sequencer.c                   |   9 ++-
>  t/t3404-rebase-interactive.sh | 107 ++++++++++++++++++++++++++++++++++
>  2 files changed, 113 insertions(+), 3 deletions(-)

Looks great, thanks. Will queue.

Thanks,
Taylor
Derrick Stolee Nov. 7, 2022, 7:25 p.m. UTC | #2 
On 11/7/22 12:47 PM, 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.
> 
> 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

This "always unlink" version is much cleaner. Thanks!

The new tests look great and I'm confident that they
are exercising the unlink() followed by a retry of
parsing the update-refs steps.

This version LGTM.

Thanks,
-Stolee
Phillip Wood Nov. 8, 2022, 9:58 a.m. UTC | #3 
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
>
diff mbox series

Patch

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 &&