Re: [PATCH v3 2/8] branch: consider refs under 'update-refs'
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Jun 30, 2022, 09:32 UTC
- Message-ID
- <f27d563b-d011-9983-8f7d-52232b052608@gmail.com>
- In-Reply-To
- <2bc647b6fcd6032a1e97e67739ced31117bfbfce.1656422759.git.gitgitgadget@gmail.com>
On 28/06/2022 14:25, Derrick Stolee via GitGitGadget wrote:
Show 28 quoted lines
> From: Derrick Stolee <derrickstolee@github.com> > > The branch_checked_out() helper helps commands like 'git branch' and > 'git fetch' from overwriting refs that are currently checked out in > other worktrees. > > A future update to 'git rebase' will introduce a new '--update-refs' > option which will update the local refs that point to commits that are > being rebased. To avoid collisions as the rebase completes, we want to > make the future data store for these refs to be considered by > branch_checked_out(). > > The data store is a plaintext file inside the 'rebase-merge' directory > for that worktree. The file alternates refnames and OIDs. The OIDs will > be used to store the to-be-written values as the rebase progresses, but > can be ignored at the moment. > > Create a new sequencer_get_update_refs_state() method that parses this > file and populates a struct string_list with the ref-OID pairs. We can > then use this list to add to the current_checked_out_branches strmap > used by branch_checked_out(). > > To properly navigate to the rebase directory for a given worktree, > extract the static strbuf_worktree_gitdir() method to a public API > method. > > We can test that this works without having Git write this file by > artificially creating one in our test script.
This looks good apart from a couple of small nits
Show 57 quoted lines
> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
> branch.c | 13 ++++++++++++
> sequencer.c | 42 +++++++++++++++++++++++++++++++++++++++
> sequencer.h | 9 +++++++++
> t/t2407-worktree-heads.sh | 22 ++++++++++++++++----
> 4 files changed, 82 insertions(+), 4 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 526e8237673..f252c4eef6c 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -365,6 +365,7 @@ static void prepare_checked_out_branches(void)
> char *old;
> struct wt_status_state state = { 0 };
> struct worktree *wt = worktrees[i++];
> + struct string_list update_refs = STRING_LIST_INIT_DUP;
>
> if (wt->is_bare)
> continue;
> @@ -400,6 +401,18 @@ static void prepare_checked_out_branches(void)
> strbuf_release(&ref);
> }
> wt_status_state_free_buffers(&state);
> +
> + if (!sequencer_get_update_refs_state(get_worktree_git_dir(wt),
> + &update_refs)) {
> + struct string_list_item *item;
> + for_each_string_list_item(item, &update_refs) {
> + old = strmap_put(¤t_checked_out_branches,
> + item->string,
> + xstrdup(wt->path));
> + free(old);
> + }
> + string_list_clear(&update_refs, 1);
> + }
> }
>
> free_worktrees(worktrees);
> diff --git a/sequencer.c b/sequencer.c
> index 8c3ed3532ac..1094e146b99 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -5901,3 +5901,45 @@ int sequencer_determine_whence(struct repository *r, enum commit_whence *whence)
>
> return 0;
> }
> +
> +int sequencer_get_update_refs_state(const char *wt_dir,
> + struct string_list *refs)
> +{
> + int result = 0;
> + struct stat st;
> + FILE *fp = NULL;
> + struct strbuf ref = STRBUF_INIT;
> + struct strbuf hash = STRBUF_INIT;
> + char *path = xstrfmt("%s/rebase-merge/update-refs", wt_dir);I think it would make sense to introduce rebase_path_update_refs() in this patch rather than later in the series
> + > + if (stat(path, &st)) > + goto cleanup;
What's the reason for stating the file first, rather than just letting fopen() fail if it does not exist?
Best Wishes
Phillip