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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Derrick Stolee <derrickstolee@github.com>,
	phillip.wood@dunelm.org.uk, Taylor Blau <me@ttaylorr.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, gitster@pobox.com, johannes.schindelin@gmx.de
Subject: Re: [PATCH 0/4] rebase: update branches in multi-part topic
Date: Tue, 7 Jun 2022 11:11:56 +0100	[thread overview]
Message-ID: <90ccd923-3552-fe88-5d6d-869def7f1aeb@gmail.com> (raw)
In-Reply-To: <3bb8baa2-e3e0-5cf1-aace-b0ba3ec6eb77@github.com>

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'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.

> 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).

> 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.

Best Wishes

Phillip

> Thanks,
> -Stolee