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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Jun 8, 2022, 14:32 UTC
Message-ID: <ac135ece-a901-5868-f28c-2161d7a62e97@gmail.com>
In-Reply-To: <pull.1247.v2.git.1654634569.gitgitgadget@gmail.com>

Hi Stolee

On 07/06/2022 21:42, Derrick Stolee via GitGitGadget wrote:

Show 21 quoted lines

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

Show 5 quoted lines

>   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