Re: [PATCH v4 09/12] sequencer: rewrite update-refs as user edits todo list

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 15, 2022, 13:13 UTC
Message-ID: <638f6b51-0045-f08e-fa63-d16a571d0099@github.com>
In-Reply-To: <7a471a98-09d1-78cb-d6dd-a7faaa9071ba@gmail.com>

On 7/15/2022 6:27 AM, Phillip Wood wrote:

Show 7 quoted lines

>> We can test that this works by rewriting the todo-list several times in
>> the course of a rebase. Check that each ref is locked or unlocked for
>> updates after each todo-list update. We an also verify that the ref
>> update fails if a concurrent process updates one of the refs after the
>> rebase process records the "locked" ref location.
> 
> Thanks for adding this and taking the time to write some good tests. When adding a new update-ref command to the todo list it would be nice to check if the ref is already checked. We could print a warning or force the user to re-edit the todo list as we do if they delete a pick and rebase.missingcommitscheck == error

I agree that that would be a helpful feature instead of the user
discovering the issue at the end of the rebase (as they do with
the message from patch 12).

> The checks below are quadratic but we don't expect that many refs so that should be fine. I don't think it is worth the complexity of building a hash table or whatever at this stage.

A hash table would be the natural approach if we see users having trouble from such high use of the feature.

I think both of these concerns would be excellent for a follow-up, since they would shave off some rough edges. I hesitate to add them to this series since it has been growing quite a bit already.

Thanks, -Stolee