Re: [PATCH v4 03/12] branch: consider refs under 'update-refs'

[Phillip Wood <phillip.wood123@gmail.com>]

On 12/07/2022 14:06, Derrick Stolee via GitGitGadget wrote:
> 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 lists refnames followed by two OIDs, each on
> separate lines. The OIDs will be used to store the original values of
> the refs and 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, at least until 'git rebase
> --update-refs' is implemented and we can use it directly.

This looks good, I've left a couple of comments below but I'm not sure 
they're even worth reading let alone replying to!

Best Wishes

Phillip

> Signed-off-by: Derrick Stolee <derrickstolee@github.com>
> ---
>   branch.c                  | 13 +++++++
>   sequencer.c               | 74 +++++++++++++++++++++++++++++++++++++++
>   sequencer.h               |  9 +++++
>   t/t2407-worktree-heads.sh | 23 ++++++++++++
>   4 files changed, 119 insertions(+)
> 
> 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(&current_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..e93c61cbd25 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -147,6 +147,20 @@ static GIT_PATH_FUNC(rebase_path_squash_onto, "rebase-merge/squash-onto")
>    */
>   static GIT_PATH_FUNC(rebase_path_refs_to_delete, "rebase-merge/refs-to-delete")
>   
> +/*
> + * The update-refs file stores a list of refs that will be updated at the end
> + * of the rebase sequence. The 'update-ref <ref>' commands in the todo file
> + * update the OIDs for the refs in this file, but the refs are not updated
> + * until the end of the rebase sequence.
> + *
> + * rebase_path_update_refs() returns the path to this file for a given
> + * worktree directory. For the current worktree, pass the_repository->gitdir.
> + */
> +static char *rebase_path_update_refs(const char *wt_dir)

It is nice to see this function. The naming  of wt_dir confused me 
slightly, wt_git_dir would make it clear that this function takes the 
git_dir of the worktree and not the worktree directory. Theu 
documentation does a good job of clarifying that though.

> +{
> +	return xstrfmt("%s/rebase-merge/update-refs", wt_dir);
> +}
> +
>   /*
>    * The following files are written by git-rebase just after parsing the
>    * command-line.
> @@ -169,6 +183,15 @@ static GIT_PATH_FUNC(rebase_path_no_reschedule_failed_exec, "rebase-merge/no-res
>   static GIT_PATH_FUNC(rebase_path_drop_redundant_commits, "rebase-merge/drop_redundant_commits")
>   static GIT_PATH_FUNC(rebase_path_keep_redundant_commits, "rebase-merge/keep_redundant_commits")
>   
> +/**
> + * A 'struct update_refs_record' represents a value in the update-refs
> + * list. We use a string_list to map refs to these (before, after) pairs.
> + */
> +struct update_ref_record {
> +	struct object_id before;
> +	struct object_id after;
> +};
> +
>   static int git_sequencer_config(const char *k, const char *v, void *cb)
>   {
>   	struct replay_opts *opts = cb;
> @@ -5901,3 +5924,54 @@ 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;
> +	FILE *fp = NULL;
> +	struct strbuf ref = STRBUF_INIT;
> +	struct strbuf hash = STRBUF_INIT;
> +	struct update_ref_record *rec = NULL;
> +
> +	char *path = rebase_path_update_refs(wt_dir);
> +
> +	fp = fopen(path, "r");
> +	if (!fp)
> +		goto cleanup;
> +
> +	while (strbuf_getline(&ref, fp) != EOF) {
> +		struct string_list_item *item;
> +
> +		CALLOC_ARRAY(rec, 1);

It does not matter but later in the series we use rec = 
xmalloc(sizeof(*rec)) to do this. I'm not sure the project has a 
standard way of allocating a single instance of something.

> +
> +		if (strbuf_getline(&hash, fp) == EOF ||
> +		    get_oid_hex(hash.buf, &rec->before)) {
> +			warning(_("update-refs file at '%s' is invalid"),
> +				  path);
> +			result = -1;
> +			goto cleanup;
> +		}
> +
> +		if (strbuf_getline(&hash, fp) == EOF ||
> +		    get_oid_hex(hash.buf, &rec->after)) {
> +			warning(_("update-refs file at '%s' is invalid"),
> +				  path);
> +			result = -1;
> +			goto cleanup;
> +		}
> +
> +		item = string_list_insert(refs, ref.buf);
> +		item->util = rec;
> +		rec = NULL;
> +	}
> +
> +cleanup:
> +	if (fp)
> +		fclose(fp);
> +	free(path);
> +	free(rec);
> +	strbuf_release(&ref);
> +	strbuf_release(&hash);
> +	return result;
> +}
> diff --git a/sequencer.h b/sequencer.h
> index da64473636b..3ae541bb145 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -232,4 +232,13 @@ void sequencer_post_commit_cleanup(struct repository *r, int verbose);
>   int sequencer_get_last_command(struct repository* r,
>   			       enum replay_action *action);
>   int sequencer_determine_whence(struct repository *r, enum commit_whence *whence);
> +
> +/**
> + * Append the set of ref-OID pairs that are currently stored for the 'git
> + * rebase --update-refs' feature if such a rebase is currently happening.
> + *
> + * Localized to a worktree's git dir.
> + */
> +int sequencer_get_update_refs_state(const char *wt_dir, struct string_list *refs);
> +
>   #endif /* SEQUENCER_H */
> diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh
> index a67ce5fb003..97f5c87f8c8 100755
> --- a/t/t2407-worktree-heads.sh
> +++ b/t/t2407-worktree-heads.sh
> @@ -81,6 +81,29 @@ test_expect_success !SANITIZE_LEAK 'refuse to overwrite: worktree in rebase (mer
>   	grep "cannot force update the branch '\''wt-2'\'' checked out at.*wt-2" err
>   '
>   
> +test_expect_success 'refuse to overwrite: worktree in rebase with --update-refs' '
> +	test_when_finished rm -rf .git/worktrees/wt-3/rebase-merge &&
> +
> +	mkdir -p .git/worktrees/wt-3/rebase-merge &&
> +	touch .git/worktrees/wt-3/rebase-merge/interactive &&
> +
> +	cat >.git/worktrees/wt-3/rebase-merge/update-refs <<-EOF &&
> +	refs/heads/fake-3
> +	$(git rev-parse HEAD~1)
> +	$(git rev-parse HEAD)
> +	refs/heads/fake-4
> +	$(git rev-parse HEAD)
> +	$(git rev-parse HEAD)
> +	EOF
> +
> +	for i in 3 4
> +	do
> +		test_must_fail git branch -f fake-$i HEAD 2>err &&
> +		grep "cannot force update the branch '\''fake-$i'\'' checked out at.*wt-3" err ||
> +			return 1
> +	done
> +'
> +
>   test_expect_success !SANITIZE_LEAK 'refuse to fetch over ref: checked out' '
>   	test_must_fail git fetch server +refs/heads/wt-3:refs/heads/wt-3 2>err &&
>   	grep "refusing to fetch into branch '\''refs/heads/wt-3'\''" err &&