* Re: [PATCH] rebase --update-refs: avoid unintended ref deletion
2022-11-04 16:57 ` [PATCH] rebase --update-refs: avoid unintended ref deletion Victoria Dye
@ 2022-11-04 19:44 ` Taylor Blau
2022-11-04 20:17 ` Phillip Wood
2022-11-04 20:12 ` Phillip Wood
2022-11-07 17:47 ` [PATCH v2] " Victoria Dye
2 siblings, 1 reply; 17+ messages in thread
From: Taylor Blau @ 2022-11-04 19:44 UTC (permalink / raw)
To: Victoria Dye; +Cc: git, Phillip Wood, herr . kaste, Derrick Stolee
On Fri, Nov 04, 2022 at 09:57:36AM -0700, Victoria Dye wrote:
> 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.
Good catch.
I wonder, though: should we only add pending ref updates to the
update-refs state after we reach that point in the sequence?
IOW: there is no world where deleting an update-refs command means to
drop the affected branch, right? So the initial state would be an empty
list, which would cause us to not update any references.
Then as we proceed through the rebase, we accumulate update-refs
commands, and know their after_oid immediately. Then when we're done, we
can process the update-refs commands for the branches that we do have.
The more I think about this, the more that I am convinced that the bug
is in how we initialize the pending list, not our treatment of it later
on.
The bug fix works as-is, but I can't help wonder if the above approach
might be more direct.
Thanks,
Taylor
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] rebase --update-refs: avoid unintended ref deletion
2022-11-04 19:44 ` Taylor Blau
@ 2022-11-04 20:17 ` Phillip Wood
0 siblings, 0 replies; 17+ messages in thread
From: Phillip Wood @ 2022-11-04 20:17 UTC (permalink / raw)
To: Taylor Blau, Victoria Dye; +Cc: git, Phillip Wood, herr . kaste, Derrick Stolee
Hi Taylor
On 04/11/2022 19:44, Taylor Blau wrote:
> On Fri, Nov 04, 2022 at 09:57:36AM -0700, Victoria Dye wrote:
>> 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.
>
> Good catch.
>
> I wonder, though: should we only add pending ref updates to the
> update-refs state after we reach that point in the sequence?
If I remember correctly the aim of the current behavior is to detect if
another process also updates the ref while we're rebasing. To do that we
need to record all the branch heads that have update-ref commands at the
start of the rebase.
Best Wishes
Phillip
> IOW: there is no world where deleting an update-refs command means to
> drop the affected branch, right? So the initial state would be an empty
> list, which would cause us to not update any references.
>
> Then as we proceed through the rebase, we accumulate update-refs
> commands, and know their after_oid immediately. Then when we're done, we
> can process the update-refs commands for the branches that we do have.
>
> The more I think about this, the more that I am convinced that the bug
> is in how we initialize the pending list, not our treatment of it later
> on.
>
> The bug fix works as-is, but I can't help wonder if the above approach
> might be more direct.
>
> Thanks,
> Taylor
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] rebase --update-refs: avoid unintended ref deletion
2022-11-04 16:57 ` [PATCH] rebase --update-refs: avoid unintended ref deletion Victoria Dye
2022-11-04 19:44 ` Taylor Blau
@ 2022-11-04 20:12 ` Phillip Wood
2022-11-07 2:39 ` Derrick Stolee
2022-11-07 17:47 ` [PATCH v2] " Victoria Dye
2 siblings, 1 reply; 17+ messages in thread
From: Phillip Wood @ 2022-11-04 20:12 UTC (permalink / raw)
To: Victoria Dye, git; +Cc: Phillip Wood, herr . kaste, Derrick Stolee, Taylor Blau
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@gmail.com>
> Signed-off-by: Victoria Dye <vdye@github.com>
> ---
> 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@mail.gmail.com/
>
> 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
>
^ permalink raw reply [flat|nested] 17+ messages in thread* Re: [PATCH] rebase --update-refs: avoid unintended ref deletion
2022-11-04 20:12 ` Phillip Wood
@ 2022-11-07 2:39 ` Derrick Stolee
0 siblings, 0 replies; 17+ messages in thread
From: Derrick Stolee @ 2022-11-07 2:39 UTC (permalink / raw)
To: phillip.wood, Victoria Dye, git; +Cc: Phillip Wood, herr . kaste, Taylor Blau
On 11/4/22 4:12 PM, Phillip Wood wrote:
> 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.
I will add my thanks, too. Thanks for jumping in when I could not!
>> 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.
I agree that an unlink() is the best option, barring one point
(that I will mention below).
>> +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
>> +'
>> +
This is a great test! I'm glad that it handles the existing
case. I think the only case that might be interesting is to
make the rebase actually create new commits and show that
the removed refs are no longer in the history of the new
branch, but are instead reachable from the older tip.
For this test, we could create a 'fixup!' to 'first' and
use --autosquash to generate new commits. At the end, we
can compare first, second, and third to different ancestors
of refs/heads/no-conflict-branch _and_ guarantee that
no-conflict-branch did not move. Or:
git rev-parse first second third no-conflict-branch >expect-oids &&
...do the rebase...
git rev-parse first second third no-conflict-branch >actual-oids &&
test_cmp expect-oids actual-oids
should work to guarantee these refs were not updated.
I wonder if anything interesting happens if after we remove
the update-ref commands we have a 'break' command and then
re-insert some of the commands. Will things like the unlink()
direction cause a problem then?
Anyway, here is a potential diff on top of your patch that
adds these modifications to the test. They work with your
implementation changes, but I also didn't try the unlink()
modification.
--- >8 ---
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index e7d3721ece8..4b09b73525a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1971,21 +1971,60 @@ test_expect_success '--update-refs: do not delete refs if all update-ref are rem
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 &&
+ test_must_fail git rebase -i \
+ --autosquash --update-refs \
+ base >todo.raw &&
sed -e "/^update-ref/d" <todo.raw >todo
) &&
(
set_replace_editor todo &&
- git rebase -i --update-refs base
+ git rebase -i --autosquash --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
+ git rev-parse first second third tip no-conflict-branch >actual-oids &&
+ test_cmp expect-oids actual-oids
+'
+
+test_expect_success '--update-refs: do not delete refs if all update-ref are removed and 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
+ ) &&
+ 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
+ ) &&
+
+ git rev-parse first second third >actual-oids &&
+ test_cmp expect-oids actual-oids &&
+ test_cmp_rev HEAD tip
'
test_expect_success '--update-refs: check failed ref update' '
--- >8 ---
Thanks,
-Stolee
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2] rebase --update-refs: avoid unintended ref deletion
2022-11-04 16:57 ` [PATCH] rebase --update-refs: avoid unintended ref deletion Victoria Dye
2022-11-04 19:44 ` Taylor Blau
2022-11-04 20:12 ` Phillip Wood
@ 2022-11-07 17:47 ` Victoria Dye
2022-11-07 19:17 ` Taylor Blau
` (2 more replies)
2 siblings, 3 replies; 17+ messages in thread
From: Victoria Dye @ 2022-11-07 17:47 UTC (permalink / raw)
To: git; +Cc: Victoria Dye, Phillip Wood, herr.kaste, Derrick Stolee, Phillip Wood
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(-)
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
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion
2022-11-07 17:47 ` [PATCH v2] " Victoria Dye
@ 2022-11-07 19:17 ` Taylor Blau
2022-11-07 19:25 ` Derrick Stolee
2022-11-08 9:58 ` Phillip Wood
2 siblings, 0 replies; 17+ messages in thread
From: Taylor Blau @ 2022-11-07 19:17 UTC (permalink / raw)
To: Victoria Dye; +Cc: git, Phillip Wood, herr.kaste, Derrick Stolee, Phillip Wood
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion
2022-11-07 17:47 ` [PATCH v2] " Victoria Dye
2022-11-07 19:17 ` Taylor Blau
@ 2022-11-07 19:25 ` Derrick Stolee
2022-11-08 9:58 ` Phillip Wood
2 siblings, 0 replies; 17+ messages in thread
From: Derrick Stolee @ 2022-11-07 19:25 UTC (permalink / raw)
To: Victoria Dye, git; +Cc: Phillip Wood, herr.kaste, Phillip Wood
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2] rebase --update-refs: avoid unintended ref deletion
2022-11-07 17:47 ` [PATCH v2] " Victoria Dye
2022-11-07 19:17 ` Taylor Blau
2022-11-07 19:25 ` Derrick Stolee
@ 2022-11-08 9:58 ` Phillip Wood
2 siblings, 0 replies; 17+ messages in thread
From: Phillip Wood @ 2022-11-08 9:58 UTC (permalink / raw)
To: Victoria Dye, git; +Cc: Phillip Wood, herr.kaste, Derrick Stolee
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
>
^ permalink raw reply [flat|nested] 17+ messages in thread