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

From: Phillip Wood <phillip.wood123@gmail.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Cc: gitster@pobox.com, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Jeff Hostetler <git@jeffhostetler.com>,
	Elijah Newren <newren@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH v2 0/7] rebase: update branches in multi-part topic
Date: Wed, 8 Jun 2022 15:32:46 +0100	[thread overview]
Message-ID: <ac135ece-a901-5868-f28c-2161d7a62e97@gmail.com> (raw)
In-Reply-To: <pull.1247.v2.git.1654634569.gitgitgadget@gmail.com>

Hi Stolee

On 07/06/2022 21:42, Derrick Stolee via GitGitGadget wrote:
> [...]
> As an example, here is my in-progress bundle URI RFC split into subtopics as
> they appear during the TODO list of a git rebase -i --update-refs:
> 
> pick 2d966282ff3 docs: document bundle URI standard
> pick 31396e9171a remote-curl: add 'get' capability
> pick 54c6ab70f67 bundle-uri: create basic file-copy logic
> pick 96cb2e35af1 bundle-uri: add support for http(s):// and file://
> pick 6adaf842684 fetch: add --bundle-uri option
> pick 6c5840ed77e fetch: add 'refs/bundle/' to log.excludeDecoration
> label for-update-refs/refs/heads/bundle-redo/fetch
> [...] 
> update-refs
> [...]
> Based on some of the discussion, it seemed like one way to do this would be
> to have an 'update-ref ' command that would take the place of these 'label'
> commands. However, this would require two things that make it a bit awkward:
> 
>   1. We would need to replicate the storage of those positions during the
>      rebase. 'label' already does this pretty well. I've added the
>      "for-update-refs/" label to help here.

I'm afraid I don't think that using a label with a name constructed from 
a magic prefix and the full refname is a good user interface.

(i) Having labels behave differently based on their name is confusing.

(ii) The length of the label string is cumbersome for users. Rebase 
already has a reputation for being unfriendly and hard to use, this will 
not improve that. "update-ref bundle-redo/fetch" is much simpler.

(iii) It means that we no longer store the oid of each branch when 
creating the todo list and so cannot check if it is safe to update it at 
the end of the rebase (just using the current value of the branch ref at 
the end of a long running operation like rebase is not sufficient to be 
safe).

>   2. If we want to close out all of the refs as the rebase is finishing, then
>      that "step" becomes invisible to the user (and a bit more complicated to
>      insert). Thus, the 'update-refs' step performs this action. If the user
>      wants to do things after that step, then they can do so by editing the
>      TODO list.

I'm not sure what the use case is for making the update-refs step 
visible to the user - it seems to be added complexity for them to deal 
with. We don't do that for the branch that is being rebased so why do we 
need to do it for the other branches? As for the implementation we could 
just update the branches at the end of the loop in pick_commits() where 
we update the branch and run the post-rewrite hook etc. there is no need 
for an entry in the todo list.

Best Wishes

Phillip