On 12/07/2022 14:06, Derrick Stolee via GitGitGadget wrote:
From: Derrick Stolee <derrickstolee@xxxxxxxxxx>
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@xxxxxxxxxx>
---
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(¤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..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 &&
