Re: [PATCH v4 07/12] rebase: add --update-refs option

From: Derrick Stolee <derrickstolee@github.com>
Date: Jul 19, 2022, 15:50 UTC
Message-ID: <01247261-dbf0-f834-965b-a4c02c4ef256@github.com>
In-Reply-To: <CABPp-BENsp3uc7J_xS0gr_yjWQ25NVuJbdHzCMEcaaGYvLEiKA@mail.gmail.com>

On 7/16/2022 3:30 PM, Elijah Newren wrote:

Show 22 quoted lines

> On Tue, Jul 12, 2022 at 6:07 AM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>>
>> From: Derrick Stolee <derrickstolee@github.com>
>>
> [...]
>>
>> +--update-refs::
>> +--no-update-refs::
>> +       Automatically force-update any branches that point to commits that
>> +       are being rebased. Any branches that are checked out in a worktree
>> +       or point to a `squash! ...` or `fixup! ...` commit are not updated
>> +       in this way.
> 
> I think the second sentence here should be split.  In particular, I
> don't think I understand the second half of the second sentence.  Do
> you intend to say here that branches pointing to a `squash!` or
> `fixup!` will instead update the first `pick` in the ancestry of such
> a commit, rather than that such branches are entirely excluded from
> any updates?  That's what I observed in my testing of your v3, at
> least, and that's the behavior I'd expect this feature to implement,
> but this documentation doesn't match.

Good find. You're right that these don't match, and its in fact that I expected it to work this way, but it doesn't.

I've added a branch to my tests that points to a fixup! that is not the tip commit and use it to demonstrate that it is not reordered, but _does_ appear in the 'update-ref <ref>' list.

I'll update the documentation to match this behavior, too. This case
is unlikely to happen much in practice, but I now believe it's better
to include the ref than to ignore it.

Show 14 quoted lines

> [...]
>> @@ -5660,6 +5764,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
>>                 item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0;
>>         }
>>
>> +       if (update_refs && todo_list_add_update_ref_commands(todo_list))
>> +               return -1;
>> +
> 
> As a tangent, I find it interesting that you add the update-ref
> commands as a post-processing step rather than as a part of
> sequencer_make_script().  I don't think you need to change anything,
> but I am curious due to my git-replay work if you find it advantageous
> to do it this way.

I found it to be simple enough to do a scan of the steps directly instead of injecting extra logic into the _make_script() method. The simplest reason is that we would need to inject "update_refs" into that method.

Thanks, -Stolee