Re: [PATCH 0/4] rebase: update branches in multi-part topic

[Derrick Stolee <derrickstolee@github.com>]

On 6/7/2022 6:11 AM, Phillip Wood wrote:
> On 06/06/2022 16:12, Derrick Stolee wrote:
>> On 6/4/2022 11:28 AM, Phillip Wood wrote:
>>> On 03/06/2022 19:27, Taylor Blau wrote:
>>>> On Fri, Jun 03, 2022 at 01:37:48PM +0000, Derrick Stolee via GitGitGadget wrote:
>>>>> This is a feature I've wanted for quite a while. When working on the sparse
>>>>> index topic, I created a long RFC that actually broke into three topics for
>>>>> full review upstream. These topics were sequential, so any feedback on an
>>>>> earlier one required updates to the later ones. I would work on the full
>>>>> feature and use interactive rebase to update the full list of commits.
>>>>> However, I would need to update the branches pointing to those sub-topics.
>>>>
>>>> This is really exciting. I'm glad that you're working on it, because I
>>>> have also wanted this feature a handful of times over the years.
>>>
>>> Yes, thank you Stolee. I agree this will be useful, but I wonder if users
>>> will be confused that --update-refs only updates the branch heads that happen
>>> to be in the todo list, rather than updating all the branches that contain a
>>> rewritten commit. I think the latter is something we should try to add in the
>>> future and so we should take care to design this topic so that is possible.
>>
>> At the moment, the design adds a comment to the TODO list, showing which
>> branches are not possible to move because they are checked out at another
>> worktree (or is currently checked out and will be updated by the rebase
>> itself). That seems like a good place to insert alternative logic in the
>> future if we see a need for better behavior here.
> 
> I think the question of whether to update branches that are checked out in
> another worktree is a question of whether it is less inconvenient to the user
> to skip the ref update and leave the user to manually update the branch or to> update the ref and leave the worktree in a potentially awkward state if the
> user was half way through building a commit. The answer probably depends on
> the preferences of the user.

I think that their 'git status' will look strange no matter what: their
working directory and index could look significantly different from what
the branch at HEAD is reporting. For this situation, I would rather continue
preventing these ref updates from underneath worktrees.
 
> I've been using a script that updates the refs for all the branches being
> rewritten for a while and have found it preferable to always update the ref
> rather than have to do it manually. My script also updates the worktree
> checkout to the new HEAD if there are no uncommitted changes which I have
> found very convenient. My preference is probably because I tend not to have
> uncommitted changes lying around in the worktrees whose branches get updated.

Actually updating the worktree to match seems like an interesting twist, which
we would want to consider if we go this route in the future.
 
>> Unless: am I misunderstanding something about your concern here? Are you
>> worried about refs outside of refs/heads/*? Are you concerned about it
>> being _all_ refs/heads/* that we find?
> 
> My concerns are primarily about being able to extend the --update-ref
> option in a backwards compatible way.
> 
> I'd like to be able to add functionality to rebase all refs/heads/* that
> are descended from the commits that a simple rebase would rewrite. Say I want
> to edit $commit then I want the rebase to rewrite all the commits and update
> all the branches in in
> 
>     git rev-list $(git for-each-ref --contains $commit refs/heads/) ^$commit^@
> 
> Ideally we'd avoid adding a new commandline option when adding that. I think
> we could use an optional argument as you suggest below (though it would not be
> a refspec).

This is sort of the opposite of what my series is doing: you want to find all
refs that contain the current commits and make sure that they are _also_
rebased as we rebase the current set of commits. That seems like an interesting
direction, with a very different set of complexities (ignoring other worktrees
seems like much more of a blocker here).

I would consider this to be _yet another mode_ that is separate from
--update-refs, though it could share some underlying logic. It gets more
complicated if there are merge commits involved (do we have a --rebase-merges
option?).

This makes me think of earlier discussions around a "git evolve" command
(based on Mercurial's evolve command). The idea is to have a different
workflow than I am presenting: instead of creating `fixup!` commits at the
top of a multi-part topic, you could check out a branch on a middle part
and work forward from there. Then, you could "rebase every branch with a
rewritten commit" to get things back into a nice line.

In conclusion: I don't think we should go down this rabbit hole right now.
I think that --update-refs would be something we want to enable by default
if we have such a mode, though.
 
>> One potential way to extend this (in the future) is to make --update-refs
>> take an optional string argument containing a refspec. This would replace
>> the default refspec of refs/heads/*.
>> [...] 
>>> Instead of using 'label' and 'exec' I'd prefer a new todo list command
>>> ('update-ref' or 'update-branch'?) used in place of 'label' that takes a
>>> branch name and updates the branch ref at the end of the rebase. That
>>> would make it easy to do all the updates in a single transaction as you
>>> suggested. Adding exec lines to do this makes the todo list messy and we
>>> have been trying to stop rebase forking all the time.
>>
>> Thanks. Yes, a new 'update-refs' step at the end would be good to make
>> it clear we want to rewrite the refs in one go without a possible
>> interruption from the user.
> 
> That's great, it is a bit more work for you but I think it gives a much
> nicer UI.

Thanks. After some hurdles on my end (and some additional complexities
discovered in the process) I will have v2 ready soon.

Thanks,
-Stolee