On 7/16/2022 3:30 PM, Elijah Newren wrote: > On Tue, Jul 12, 2022 at 6:07 AM Derrick Stolee via GitGitGadget > <gitgitgadget@xxxxxxxxx> wrote: >> >> From: Derrick Stolee <derrickstolee@xxxxxxxxxx> >> > [...] >> >> +--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. > [...] >> @@ -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
