rebase: update branches in multi-part topic

This is a feature I've wanted for quite a while. When working on the sparse index topic, I created a long RFC that actually broke into three topics for full review upstream. These topics were sequential, so any feedback on an earlier one required updates to the later ones. I would work on the full feature and use interactive rebase to update the full list of commits. However, I would need to update the branches pointing to those sub-topics.

This series adds a new --update-refs option to 'git rebase' (along with a rebase.updateRefs config option) that adds 'git update-ref' commands into the TODO list. This is powered by the commit decoration machinery.

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
exec git update-ref refs/heads/bundle-redo/fetch HEAD 6c5840ed77e1bc41c1fe6fb7c894ceede1b8d730

pick 1e3f6546632 clone: add --bundle-uri option
pick 9e4a6fe9b68 clone: --bundle-uri cannot be combined with --depth
exec git update-ref refs/heads/bundle-redo/clone HEAD 9e4a6fe9b68a8455b427c9ac8cdbff30c96653b4

pick 5451cb6599c bundle-uri: create bundle_list struct and helpers
pick 3029c3aca15 bundle-uri: create base key-value pair parsing
pick a8b2de79ce8 bundle-uri: create "key=value" line parsing
pick 92625a47673 bundle-uri: unit test "key=value" parsing
pick a8616af4dc2 bundle-uri: limit recursion depth for bundle lists
pick 9d6809a8d53 bundle-uri: parse bundle list in config format
pick 287a732b54c bundle-uri: fetch a list of bundles
exec git update-ref refs/heads/bundle-redo/list HEAD 287a732b54c4d95e7f410b3b36ef90d8a19cd346

pick b09f8226185 protocol v2: add server-side "bundle-uri" skeleton pick 520204dcd1c bundle-uri client: add minimal NOOP client
pick 62e8b457b48 bundle-uri client: add "git ls-remote-bundle-uri" pick 00eae925043 bundle-uri: serve URI advertisement from bundle.* config pick 4277440a250 bundle-uri client: add boolean transfer.bundleURI setting pick caf4599a81d bundle-uri: allow relative URLs in bundle lists pick df255000b7e bundle-uri: download bundles from an advertised list pick d71beabf199 clone: unbundle the advertised bundles
pick c9578391976 t5601: basic bundle URI tests
# Ref refs/heads/bundle-redo/rfc-3 checked out at '/home/stolee/_git/git-bundles'

exec git update-ref refs/heads/bundle-redo/advertise HEAD c9578391976ab9899c4e4f9b5fa2827650097305

The first two patches are helpers that are needed, but the full logic of the --update-refs option is introduced in patch 3. The config option is available in patch 4.

Thanks, -Stolee

Derrick Stolee (4):
log-tree: create for_each_decoration()
branch: add branch_checked_out() helper
rebase: add --update-refs option
rebase: add rebase.updateRefs config option

 Documentation/config/rebase.txt |   3 +
 Documentation/git-rebase.txt    |  11 ++++
 branch.c                        |  24 ++++---
 branch.h                        |   8 +++
 builtin/rebase.c                |  10 +++
 log-tree.c                      | 111 ++++++++++++++++++++++----------
 log-tree.h                      |   4 ++
 sequencer.c                     |  99 ++++++++++++++++++++++++++++
 sequencer.h                     |   1 +
 t/t3404-rebase-interactive.sh   |  34 ++++++++++
 10 files changed, 262 insertions(+), 43 deletions(-)

base-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1247%2Fderrickstolee%2Frebase-keep-decorations-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1247/derrickstolee/rebase-keep-decorations-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1247

gitgitgadget

From: Derrick Stolee <[email protected]>

It can be helpful to iterate through all the decorations on a commit without necessarily writing them to a stream. Implement for_each_decoration() and reimplement format_decorations_extended() to use that iterator.

---
 log-tree.c | 111 ++++++++++++++++++++++++++++++++++++-----------------
 log-tree.h |   4 ++
 2 files changed, 80 insertions(+), 35 deletions(-)

index d0ac0a6327a..b15a7c9db22 100644
@@ -282,6 +282,54 @@ static void show_name(struct strbuf *sb, const struct name_decoration *decoratio
 		strbuf_addstr(sb, decoration->name);
 }
 
+struct format_decorations_context {
+	struct strbuf *sb;
+	int use_color;
+	const char *prefix;
+	const char *separator;
+	const char *suffix;
+	const char *color_commit;
+	const char *color_reset;
+	const struct name_decoration *current_and_HEAD;
+};
+
+static int append_decoration(const struct name_decoration *d,
+			     void *data)
+{
+	struct format_decorations_context *ctx = data;
+	/*
+	 * When both current and HEAD are there, only
+	 * show HEAD->current where HEAD would have
+	 * appeared, skipping the entry for current.
+	 */
+	if (d != ctx->current_and_HEAD) {
+		strbuf_addstr(ctx->sb, ctx->color_commit);
+		strbuf_addstr(ctx->sb, ctx->prefix);
+		strbuf_addstr(ctx->sb, ctx->color_reset);
+		strbuf_addstr(ctx->sb, decorate_get_color(ctx->use_color, d->type));
+		if (d->type == DECORATION_REF_TAG)
+			strbuf_addstr(ctx->sb, "tag: ");
+
+		show_name(ctx->sb, d);
+
+		if (ctx->current_and_HEAD &&
+		    d->type == DECORATION_REF_HEAD) {
+			strbuf_addstr(ctx->sb, " -> ");
+			strbuf_addstr(ctx->sb, ctx->color_reset);
+			strbuf_addstr(ctx->sb,
+				      decorate_get_color(
+						ctx->use_color,
+						ctx->current_and_HEAD->type));
+			show_name(ctx->sb, ctx->current_and_HEAD);
+		}
+		strbuf_addstr(ctx->sb, ctx->color_reset);
+
+		ctx->prefix = ctx->separator;
+	}
+
+	return 0;
+}
+
 /*
  * The caller makes sure there is no funny color before calling.
  * format_decorations_extended makes sure the same after return.

@@ -294,49 +342,42 @@ void format_decorations_extended(struct strbuf *sb,
 			const char *suffix)
 {
 	const struct name_decoration *decoration;
-	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	struct format_decorations_context ctx = {
+		.sb = sb,
+		.use_color = use_color,
+		.prefix = prefix,
+		.separator = separator,
+		.suffix = suffix,
+		.color_commit =	diff_get_color(use_color, DIFF_COMMIT),
+		.color_reset = decorate_get_color(use_color, DECORATION_NONE),
+	};
 
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
-	current_and_HEAD = current_pointed_by_HEAD(decoration);
-	while (decoration) {
-		/*
-		 * When both current and HEAD are there, only
-		 * show HEAD->current where HEAD would have
-		 * appeared, skipping the entry for current.
-		 */
-		if (decoration != current_and_HEAD) {
-			strbuf_addstr(sb, color_commit);
-			strbuf_addstr(sb, prefix);
-			strbuf_addstr(sb, color_reset);
-			strbuf_addstr(sb, decorate_get_color(use_color, decoration->type));
-			if (decoration->type == DECORATION_REF_TAG)
-				strbuf_addstr(sb, "tag: ");
-
-			show_name(sb, decoration);
-
-			if (current_and_HEAD &&
-			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, " -> ");
-				strbuf_addstr(sb, color_reset);
-				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
-				show_name(sb, current_and_HEAD);
-			}
-			strbuf_addstr(sb, color_reset);
+	ctx.current_and_HEAD = current_pointed_by_HEAD(decoration);
 
-			prefix = separator;
-		}
+	for_each_decoration(commit, append_decoration, &ctx);
+
+	strbuf_addstr(sb, ctx.color_commit);
+	strbuf_addstr(sb, ctx.suffix);
+	strbuf_addstr(sb, ctx.color_reset);
+}
+
+int for_each_decoration(const struct commit *c, decoration_fn fn, void *data)
+{
+	const struct name_decoration *decoration;
+
+	decoration = get_name_decoration(&c->object);
+	while (decoration) {
+		int res;
+		if ((res = fn(decoration, data)))
+			return res;
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+
+	return 0;
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)

index e7e4641cf83..ea07da2625b 100644
@@ -35,4 +35,8 @@ void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 void fmt_output_email_subject(struct strbuf *, struct rev_info *);
 
+typedef int decoration_fn(const struct name_decoration *d,
+			  void *data);
+int for_each_decoration(const struct commit *c, decoration_fn fn, void *data);
+
 #endif

gitgitgadget

This is more an issue with this particular caller since it needs that current_pointed_by_HEAD() information. I had considered it as something to include in the prototype of decoration_fn so it could be incorporated into for_each_decoration(), but it ended up being overly clunky for this one consumer. I'm open to other ideas, though.

The goal of this method was to make it super simple to iterate without doing any prep work: just load the commit and go.

The iterator returns quickly because the while loop notices a NULL decoration. This use in format_decorations_extended() needs that check because of the current_pointed_by_HEAD(decoration) call based on the result being non-NULL.

You are right that this would work, but it's a bit messier. We could go this route to avoid the duplicate get_name_decoration() calls in format_decorations_extended().

Thanks,
-Stolee

Having a convenience function that takes a commit and let you walk over its decoration is OK, but calling it for_each_decoration() bothers me somewhat (which is where my comment started from after all).

Hopefully it will stay unambiguous, and it will stay to be unnecessary to name it for_each_decoration_for_commit(), as I do not think we'd add "decoration" to things that are not commits (or if we added one, we probably will call it differently from "decoration").

So, OK.

From: Derrick Stolee <[email protected]>

The validate_new_branchname() method contains a check to see if a branch is checked out in any non-bare worktree. This is intended to prevent a force push that will mess up an existing checkout. This helper is not suitable to performing just that check, because the method will die() when the branch is checked out instead of returning an error code.

Extract branch_checked_out() and use it within validate_new_branchname(). Another caller will be added in a coming change.

---
 branch.c | 24 ++++++++++++++++--------
 branch.h |  8 ++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

index 2d6569b0c62..2e6419cdfa5 100644
@@ -369,6 +369,19 @@ int validate_branchname(const char *name, struct strbuf *ref)
 	return ref_exists(ref->buf);
 }
 
+int branch_checked_out(const char *refname, char **path)
+{
+	struct worktree **worktrees = get_worktrees();
+	const struct worktree *wt = find_shared_symref(worktrees, "HEAD", refname);
+	int result = wt && !wt->is_bare;
+
+	if (result && path)
+		*path = xstrdup(wt->path);
+
+	free_worktrees(worktrees);
+	return result;
+}
+
 /*
  * Check if a branch 'name' can be created as a new branch; die otherwise.
  * 'force' can be used when it is OK for the named branch already exists.

@@ -377,9 +390,7 @@ int validate_branchname(const char *name, struct strbuf *ref)
  */
 int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 {
-	struct worktree **worktrees;
-	const struct worktree *wt;
-
+	char *path;
 	if (!validate_branchname(name, ref))
 		return 0;
 

@@ -387,13 +398,10 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 		die(_("a branch named '%s' already exists"),
 		    ref->buf + strlen("refs/heads/"));
 
-	worktrees = get_worktrees();
-	wt = find_shared_symref(worktrees, "HEAD", ref->buf);
-	if (wt && !wt->is_bare)
+	if (branch_checked_out(ref->buf, &path))
 		die(_("cannot force update the branch '%s' "
 		      "checked out at '%s'"),
-		    ref->buf + strlen("refs/heads/"), wt->path);
-	free_worktrees(worktrees);
+		    ref->buf + strlen("refs/heads/"), path);
 
 	return 1;
 }

index 560b6b96a8f..5ea93d217b1 100644
@@ -101,6 +101,14 @@ void create_branches_recursively(struct repository *r, const char *name,
 				 const char *tracking_name, int force,
 				 int reflog, int quiet, enum branch_track track,
 				 int dry_run);
+
+/*
+ * Returns true if the branch at 'refname' is checked out at any
+ * non-bare worktree. The path of the worktree is stored in the
+ * given 'path', if provided.
+ */
+int branch_checked_out(const char *refname, char **path);
+
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
  * Return 1 if the named branch already exists; return 0 otherwise.

gitgitgadget

From: Derrick Stolee <[email protected]>

When working on a large feature, it can be helpful to break that feature into multiple smaller parts that become reviewed in sequence. During development or during review, a change to one part of the feature could affect multiple of these parts. An interactive rebase can help adjust the multi-part "story" of the branch.

However, if there are branches tracking the different parts of the feature, then rebasing the entire list of commits can create commits not reachable from those "sub branches". It can take a manual step to update those branches.

Add a new --update-refs option to 'git rebase -i' that adds 'git update-ref' exec steps to the todo file whenever a commit that is being rebased is decorated with that <ref>. This allows the user to rebase a long list of commits in a multi-part feature and keep all of their pointers to those parts.

Use the new for_each_decoration() while iterating over the existing todo list. Be sure to iterate after any squashing or fixups are placed. Update the branch only after those squashes and fixups are complete. This allows a --fixup commit at the tip of the feature to apply correctly to the sub branch, even if it is fixing up the most-recent commit in that part.

One potential problem here is that refs decorating commits that are already marked as "fixup!" or "squash!" will not be included in this list. Generally, the reordering of the "fixup!" and "squash!" is likely to change the relative order of these refs, so it is not recommended. The workflow here is intended to allow these kinds of commits at the tip of the rebased branch while the other sub branches come along for the ride without intervention.

Be careful to not attempt updating any branch that is checked out. The most common example is the branch being rebased is checked out and decorates the tip commit. If the user is rebasing commits reachable from a different branch that is checked out in a different worktree, then they may be surprised to not see that ref update. However, it's probably best to not optimize for this scenario and do the safest thing that will result in a successful rebase. A comment is left in the TODO list that signals that these refs are currently checked out.

---
 Documentation/git-rebase.txt  |  7 +++
 builtin/rebase.c              |  5 ++
 sequencer.c                   | 99 +++++++++++++++++++++++++++++++++++
 sequencer.h                   |  1 +
 t/t3404-rebase-interactive.sh | 29 ++++++++++
 5 files changed, 141 insertions(+)

index 262fb01aec0..866554fc978 100644
@@ -609,6 +609,13 @@ provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
 start would be overridden by the presence of
 `rebase.rescheduleFailedExec=true` configuration.
 
+--update-refs::
+--no-update-refs::
+	Automatically force-update any branches that point to commits that
+	are being rebased. Any branches that are checked out in a worktree
+	or point to a `squash! ...` or `fixup! ...` commit are not updated
+	in this way.
+
 INCOMPATIBLE OPTIONS
 --------------------
 

index 7ab50cda2ad..56d82a52106 100644
@@ -102,6 +102,7 @@ struct rebase_options {
 	int reschedule_failed_exec;
 	int reapply_cherry_picks;
 	int fork_point;
+	int update_refs;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\

@@ -298,6 +299,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
 			&opts->orig_head, &commands, opts->autosquash,
+			opts->update_refs,
 			&todo_list);
 	}
 

@@ -1124,6 +1126,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "autosquash", &options.autosquash,
 			 N_("move commits that begin with "
 			    "squash!/fixup! under -i")),
+		OPT_BOOL(0, "update-refs", &options.update_refs,
+			 N_("update local refs that point to commits "
+			    "that are being rebased")),
 		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },

index da64473636b..1add3c7119f 100644
@@ -166,6 +166,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const struct object_id *orig_head,
 		    struct string_list *commands, unsigned autosquash,
+		    unsigned keep_decorations,
 		    struct todo_list *todo_list);
 int todo_list_rearrange_squash(struct todo_list *todo_list);
 

index f31afd4a547..38c7ef95e0e 100755
@@ -1743,6 +1743,35 @@ test_expect_success 'ORIG_HEAD is updated correctly' '
 	test_cmp_rev ORIG_HEAD test-orig-head@{1}
 '
 
+test_expect_success '--update-refs adds git branch commands' '
+	git checkout -b update-refs no-conflict-branch &&
+	test_commit extra fileX &&
+	git commit --amend --fixup=L &&
+	(
+		set_cat_todo_editor &&
+		git branch -f base A &&
+		git branch -f first J &&
+		git branch -f second J &&
+		git branch -f third L &&
+
+		test_must_fail git rebase -i --autosquash --update-refs primary >todo &&
+
+		cat >expect <<-EOF &&
+		pick $(git log -1 --format=%h J) J
+		exec git update-ref refs/heads/second HEAD $(git rev-parse J)
+		exec git update-ref refs/heads/first HEAD $(git rev-parse  J)
+		pick $(git log -1 --format=%h K) K
+		pick $(git log -1 --format=%h L) L
+		fixup $(git log -1 --format=%h update-refs) fixup! L
+		exec git update-ref refs/heads/third HEAD $(git rev-parse L)
+		pick $(git log -1 --format=%h M) M
+		exec git update-ref refs/heads/no-conflict-branch HEAD $(git rev-parse M)
+		EOF
+
+		test_cmp expect todo
+	)
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged

gitgitgadget

Hi Stolee

Just a couple of minor comments.

	struct todo_list_item *item;
	size_t nr, alloc;

	ctx.new_list.items[ctx.new_list.nr++] = todo_list->items[i++];

From: Derrick Stolee <[email protected]>

The previous change added the --update-refs command-line option. For users who always want this mode, create the rebase.updateRefs config option which behaves the same way as rebase.autoSquash does with the --autosquash option.

---
 Documentation/config/rebase.txt | 3 +++
 Documentation/git-rebase.txt    | 4 ++++
 builtin/rebase.c                | 5 +++++
 t/t3404-rebase-interactive.sh   | 5 +++++
 4 files changed, 17 insertions(+)

index 8c979cb20f2..f19bd0e0407 100644
@@ -21,6 +21,9 @@ rebase.autoStash::
 	`--autostash` options of linkgit:git-rebase[1].
 	Defaults to false.
 
+rebase.updateRefs::
+	If set to true enable `--update-refs` option by default.
+
 rebase.missingCommitsCheck::
 	If set to "warn", git rebase -i will print a warning if some
 	commits are removed (e.g. a line was deleted), however the

index 866554fc978..cb1b498bd99 100644
@@ -615,6 +615,10 @@ start would be overridden by the presence of
 	are being rebased. Any branches that are checked out in a worktree
 	or point to a `squash! ...` or `fixup! ...` commit are not updated
 	in this way.
++
+If the `--update-refs` option is enabled by default using the
+configuration variable `rebase.updateRefs`, this option can be
+used to override and disable this setting.
 
 INCOMPATIBLE OPTIONS
 --------------------

index 56d82a52106..8ebc98ea505 100644
@@ -802,6 +802,11 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "rebase.updaterefs")) {
+		opts->update_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "rebase.reschedulefailedexec")) {
 		opts->reschedule_failed_exec = git_config_bool(var, value);
 		return 0;

index 38c7ef95e0e..1fa9f78d40d 100755
@@ -1768,6 +1768,11 @@ test_expect_success '--update-refs adds git branch commands' '
 		exec git update-ref refs/heads/no-conflict-branch HEAD $(git rev-parse M)
 		EOF
 
+		test_cmp expect todo &&
+
+		test_must_fail git -c rebase.autosquash=true \
+				   -c rebase.updaterefs=true \
+				   rebase -i primary >todo &&
 		test_cmp expect todo
 	)
 '

gitgitgadget

;-)

This is really exciting. I'm glad that you're working on it, because I have also wanted this feature a handful of times over the years.

This definitely would have come in handy when writing MIDX bitmaps, where I was often editing a local branch pointing at the final topic, and then trying to reconstruct all of the intermediate branches after each rebase. Not ever having to do that again sounds like a delight ;-).

But I wonder if we can or should delay these update-refs as long as possible. In particular: what happens if I get past this "exec" line (so that I've already updated my bundle-redo/fetch branch to point at the new thing), but decide at some later point to abort the rebase?

I think users will expect us to restore bundle-redo/fetch to where it was before if we end up in that case. Recovering from it manually sounds like kind of a headache.

What if instead we created labels here, and then delayed all ref updates to the end by replacing this with:

label bundle-redo/fetch

and then at the end of the todo list we'd add:

exec git update-ref refs/heads/bundle-redo/fetch refs/rewritten/bundle-redo/fetch

If we do all of those ref updates in a single transaction at the end, it should be much easier to roll back from if desired, and we'd avoid the aborted-rebase problem entirely.

Thanks,
Taylor

That is a very good safety and usability concern. I am glad somebody thought of it.

;-)

I agree. I could have really used this while juggling all of the parts of FSMonitor recently. And yes, it should write the updates at the bottom in case of an abort.

Should this take a branch pattern/regex to limit the set of branches that are updated (or offered to be updated)? For example, if I have an intermediate commit in the series that has 2 branch names pointing at it, do we want to offer to update both of them or only the one that matches some pattern related to the tip? Or is it sufficient to just enumerate them at the bottom of the todo list and let the user delete the lines they don't want?

Should we actually do the update-ref's or should we write a script that lets the user do it later? The latter would let us also write out the commands to force update the remote refs if that would be helpful.

Thanks,
Jeff

That could be a useful feature to layer on top. I don't think it's required here, though, since (as you note) users can remove lines from the update-ref invocation(s) at the bottom that they don't want to perform.

That seems like it would be outside the bounds of what rebase should provide, but others may feel differently.

Thanks,
Taylor

The latter sounds sufficient (starting from something simpler should work well in this case).

Aren't we writing "a script" already by implementing it as an additional "exec git update-ref" in the todo list already? I found that, combined with the idea to use the "label" Taylor mentioned, was the most brilliant part of this proposal.

Yes, thank you Stolee. I agree this will be useful, but I wonder if users will be confused that --update-refs only updates the branch heads that happen to be in the todo list, rather than updating all the branches that contain a rewritten commit. I think the latter is something we should try to add in the future and so we should take care to design this topic so that is possible.

Absolutely! There is also the question of what to do if a user skips a commit that is a branch head. It is not obvious if they just want to drop that commit from the branch or if they want to skip updating the ref as well (I guess they can edit the todo list for the latter case).

Instead of using 'label' and 'exec' I'd prefer a new todo list command ('update-ref' or 'update-branch'?) used in place of 'label' that takes a branch name and updates the branch ref at the end of the rebase. That would make it easy to do all the updates in a single transaction as you suggested. Adding exec lines to do this makes the todo list messy and we have been trying to stop rebase forking all the time.

Best Wishes

Phillip

At the moment, the design adds a comment to the TODO list, showing which branches are not possible to move because they are checked out at another worktree (or is currently checked out and will be updated by the rebase itself). That seems like a good place to insert alternative logic in the future if we see a need for better behavior here.

Unless: am I misunderstanding something about your concern here? Are you worried about refs outside of refs/heads/*? Are you concerned about it being _all_ refs/heads/* that we find?

One potential way to extend this (in the future) is to make --update-refs take an optional string argument containing a refspec. This would replace the default refspec of refs/heads/*.

Thanks. Yes, a new 'update-refs' step at the end would be good to make it clear we want to rewrite the refs in one go without a possible interruption from the user.

Thanks,
-Stolee

I think the question of whether to update branches that are checked out in another worktree is a question of whether it is less inconvenient to the user to skip the ref update and leave the user to manually update the branch or to update the ref and leave the worktree in a potentially awkward state if the user was half way through building a commit. The answer probably depends on the preferences of the user.

I've been using a script that updates the refs for all the branches being rewritten for a while and have found it preferable to always update the ref rather than have to do it manually. My script also updates the worktree checkout to the new HEAD if there are no uncommitted changes which I have found very convenient. My preference is probably because I tend not to have uncommitted changes lying around in the worktrees whose branches get updated.

My concerns are primarily about being able to extend the --update-ref option in a backwards compatible way.

I'd like to be able to add functionality to rebase all refs/heads/* that are descended from the commits that a simple rebase would rewrite. Say I want to edit $commit then I want the rebase to rewrite all the commits and update all the branches in in

    git rev-list $(git for-each-ref --contains $commit refs/heads/) 

^$commit^@

Ideally we'd avoid adding a new commandline option when adding that. I think we could use an optional argument as you suggest below (though it would not be a refspec).

That's great, it is a bit more work for you but I think it gives a much nicer UI.

Best Wishes

Phillip

I think that their 'git status' will look strange no matter what: their working directory and index could look significantly different from what the branch at HEAD is reporting. For this situation, I would rather continue preventing these ref updates from underneath worktrees.

Actually updating the worktree to match seems like an interesting twist, which we would want to consider if we go this route in the future.

This is sort of the opposite of what my series is doing: you want to find all refs that contain the current commits and make sure that they are _also_ rebased as we rebase the current set of commits. That seems like an interesting direction, with a very different set of complexities (ignoring other worktrees seems like much more of a blocker here).

I would consider this to be _yet another mode_ that is separate from --update-refs, though it could share some underlying logic. It gets more complicated if there are merge commits involved (do we have a --rebase-merges option?).

This makes me think of earlier discussions around a "git evolve" command (based on Mercurial's evolve command). The idea is to have a different workflow than I am presenting: instead of creating fixup! commits at the top of a multi-part topic, you could check out a branch on a middle part and work forward from there. Then, you could "rebase every branch with a rewritten commit" to get things back into a nice line.

In conclusion: I don't think we should go down this rabbit hole right now. I think that --update-refs would be something we want to enable by default if we have such a mode, though.

Thanks. After some hurdles on my end (and some additional complexities discovered in the process) I will have v2 ready soon.

Thanks,
-Stolee

To some degree, it might be true, but I think the recent thinking is that by default any refs being worked on by the user must be kept intact. "switch" does not let you check out a branch that is already checked out elsewhere, "fetch" does not let you overwrite the branch that is currently checked out without "--update-head-ok", etc.

Usually I caution against adding "features" that can complicate the end-user experience like this, but in this case, the potential for extra conflicts coming from such updates to the working trees may not be too bad.

It all depends on why the user has these intermediate branches and how they are used, but in order to see any conflicts happen after rebasing a branch here, before you start that rebase, you need to have in different working trees that had these intermediate branches checked out and had local modifications on top of them.

I would say that you deserve it if conflicts resulting from such a set-up hurts you ;-) If you are using these intermediate branches so that you can work on steps in the middle of the larger whole, then the proposed "rebase --update-refs" is not for you. Having local modification in these separate working trees that check out the intermediate branches is OK, but once you create a commit on such a branch, the commit that is in the larger topic is no longer at the tip, so "rebase --update-refs" would not notice it anyway.

Rather, if you have the intermediate branches, you'd probably want to work on each of them to make them stable, and rebase them on top of each other in an appropriate order. "rebase --update-refs" that runs on the larger topic does not have enough information to rebuild it with tips of intermediate branches that are updated elsewhere.

Which means that anybody sane who uses "rebase --update-refs" would not modify these intermediate branches outside the context of the larger topic, and those who do not follow this rule can keep both halves of their history ;-)

FWIW a complementary tool that would work in the other direction, to expect the user to have worked on smaller branches and rebuild the larger branch that contains them, is also missing from our tool set. Those who check out these smaller branches in separate working trees would want to use such a tool, not "rebase --update-ref".

Thanks.

Sounds like a good approach.

Very interesting; nice to see that others are thinking on a similar wavelength. I've got some related work in git-replay, and it heavily uses update-refs style handling.

Not sure if there are any differences worth affecting your design or mine, but perhaps it's worth mentioning my current work and plans in git-replay related to the use of update-refs:

Since git-replay doesn't touch the working tree or index, I had the natural question of what the output should be and whether ref-updates should automatically happen. I decided that like git-merge-tree, ref updates should not be automatic (and thus it behaves somewhat like a "dry run" version of rebase/cherry-pick). Since I also want to intrinsically handle replaying multiple branches simultaneously, the update-ref style output seemed like a natural fit to me (meaning users would pipe the output from git-reply to git-update-ref). Thus, one can do:

    $ git replay --onto main startpoint..mytopic

and see git-update-ref style output:

    update refs/heads/mytopic ${NEW_mytopic_HASH} ${OLD_mytopic_HASH}

where ${NEW_mytopic_HASH} is the commit at the tip of a chain of rebased mytopic commits. (Sidenote: There's the question of whether to update 'main' (i.e. cherry-picking) or 'mytopic' (i.e. rebasing); thus, users have to choose --advance vs. --onto to specify which of these should happen, except in cases where it can be implicitly determined.)

Or, if you have several branches to update:

    $ git replay --onto main ^startpoint mytopic1 mytopic2 mytopic3

and see

    update refs/heads/mytopic1 ${NEW_mytopic1_HASH} ${OLD_mytopic1_HASH}
    update refs/heads/mytopic2 ${NEW_mytopic2_HASH} ${OLD_mytopic2_HASH}
    update refs/heads/mytopic3 ${NEW_mytopic3_HASH} ${OLD_mytopic3_HASH}

(and yes, common commits from before will be replayed and then shared by the new branch versions.)

If the topics are totally contained within each other, you can do this more simply as

    $ git replay --contained --onto main main..my_tip_topic

and still see all the update commands:

    update refs/heads/mytopic1 ${NEW_mytopic1_HASH} ${OLD_mytopic1_HASH}
    update refs/heads/mytopic2 ${NEW_mytopic2_HASH} ${OLD_mytopic2_HASH}
    update refs/heads/mytopic3 ${NEW_mytopic3_HASH} ${OLD_mytopic3_HASH}
    update refs/heads/my_tip_topic ${NEW_my_tip_topic_HASH}

${OLD_my_tip_topic_HASH}

The option to list several refs provides a natural way of allowing users to control what gets updated without updating all contained branches, though rebase's implicit ranges doesn't really have a nice analogue for you due to its implicit range handling.

Right now, git-replay doesn't handle conflicts (it just die()s). That and a few other thing are still TODO.

However, git-replay does already handle basic replaying of merges, including ones with evil changes (so long as none of the merges involved have conflicts). So, I'm hoping git-replay will also useful for stuff like:

   $ git replay --first-parent --interactive --onto next ^next
special_topic seen

where special_topic is one of the topics previously merged into seen. This last commit would allow seen to be rebuilt with some changes to special_topic. Basically, it makes whatever interactive changes are specified to special_topic followed by replaying all the merges in next..seen (including any necessary conflict resolutions or other semantic changes recorded in the merge commits being replayed), unless those . However, I do need to figure out some syntax for keeping special_topic from being transplanted onto next here...

This is a feature I've wanted for quite a while. When working on the sparse index topic, I created a long RFC that actually broke into three topics for full review upstream. These topics were sequential, so any feedback on an earlier one required updates to the later ones. I would work on the full feature and use interactive rebase to update the full list of commits. However, I would need to update the branches pointing to those sub-topics.

This series adds a new --update-refs option to 'git rebase' (along with a rebase.updateRefs config option) that adds 'label' and 'update-refs' commands into the TODO list. This is powered by the commit decoration machinery.

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

pick 1e3f6546632 clone: add --bundle-uri option
pick 9e4a6fe9b68 clone: --bundle-uri cannot be combined with --depth label for-update-refs/refs/heads/bundle-redo/clone

pick 5451cb6599c bundle-uri: create bundle_list struct and helpers pick 3029c3aca15 bundle-uri: create base key-value pair parsing pick a8b2de79ce8 bundle-uri: create "key=value" line parsing pick 92625a47673 bundle-uri: unit test "key=value" parsing pick a8616af4dc2 bundle-uri: limit recursion depth for bundle lists pick 9d6809a8d53 bundle-uri: parse bundle list in config format pick 287a732b54c bundle-uri: fetch a list of bundles label for-update-refs/refs/heads/bundle-redo/list

pick b09f8226185 protocol v2: add server-side "bundle-uri" skeleton pick 520204dcd1c bundle-uri client: add minimal NOOP client
pick 62e8b457b48 bundle-uri client: add "git ls-remote-bundle-uri" pick 00eae925043 bundle-uri: serve URI advertisement from bundle.* config pick 4277440a250 bundle-uri client: add boolean transfer.bundleURI setting pick caf4599a81d bundle-uri: allow relative URLs in bundle lists pick df255000b7e bundle-uri: download bundles from an advertised list pick d71beabf199 clone: unbundle the advertised bundles
pick c9578391976 t5601: basic bundle URI tests
# Ref refs/heads/bundle-redo/rfc-3 checked out at '/home/stolee/_git/git-bundles'

label for-update-refs/refs/heads/bundle-redo/advertise

update-refs

* Patches 1-2 are basic refactors that we will need for our implementation.
* Patch 3 is a cleanup of an array with indexes given by the todo_command enum.
* Patches 4-6 implement the feature incrementally. For instance, we can test the edits to the TODO file in patch 5, but we don't actually do anything when seeing the 'update-refs' command until patch 6.
* Patch 7 adds the rebase.updateRefs config option similar to rebase.autoSquash.

Updates in v2
=============

As recommended by the excellent feedback, I have removed the 'exec' commands in favor of the 'label' commands and a new 'update-refs' command at the very end. This way, there is only one step that updates all of the refs at the end instead of updating refs during the rebase. If a user runs 'git rebase --abort' in the middle, then their refs are still where they need to be.

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.
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.

Other updates:

* The 'keep_decorations' parameter was renamed to 'update_refs'.
* I added tests for --rebase-merges=rebase-cousins to show how these labels interact with other labels and merge commands.
* I changed the order of the insertion of these update-refs labels to be before the fixups are rearranged. This fixes a bug where the tip commit is a fixup! so its decorations are never inspected (and they would be in the wrong place even if they were). The fixup! commands are properly inserted between a pick and its following label command. Tests demonstrate this is correct.
* Numerous style choices are updated based on feedback.

Thank you for all of the detailed review and ideas in this space. I appreciate any more ideas that can make this feature as effective as it can be.

Thanks, -Stolee

Derrick Stolee (7):
log-tree: create for_each_decoration()
branch: add branch_checked_out() helper
sequencer: define array with enum values
sequencer: add update-refs command
rebase: add --update-refs option
sequencer: implement 'update-refs' command
rebase: add rebase.updateRefs config option

 Documentation/config/rebase.txt |   3 +
 Documentation/git-rebase.txt    |  12 ++
 branch.c                        |  24 ++--
 branch.h                        |   8 ++
 builtin/rebase.c                |  10 ++
 log-tree.c                      | 111 +++++++++++------
 log-tree.h                      |   4 +
 sequencer.c                     | 206 +++++++++++++++++++++++++++++---
 sequencer.h                     |   2 +
 t/t3404-rebase-interactive.sh   | 110 +++++++++++++++++
 10 files changed, 432 insertions(+), 58 deletions(-)

base-commit: 2668e3608e47494f2f10ef2b6e69f08a84816bcb
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1247%2Fderrickstolee%2Frebase-keep-decorations-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1247/derrickstolee/rebase-keep-decorations-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1247

Range-diff vs v1:

 1:  4f9f3487641 = 1:  4f9f3487641 log-tree: create for_each_decoration()
 2:  5f54766e103 = 2:  5f54766e103 branch: add branch_checked_out() helper
 -:  ----------- > 3:  9f261c7df2c sequencer: define array with enum values
 -:  ----------- > 4:  842b2186d25 sequencer: add update-refs command
 3:  23fa6638864 ! 5:  0a4c110127b rebase: add --update-refs option
     @@ Commit message
          reachable from those "sub branches". It can take a manual step to update
          those branches.
      
     -    Add a new --update-refs option to 'git rebase -i' that adds 'git
     -    update-ref' exec steps to the todo file whenever a commit that is being
     -    rebased is decorated with that <ref>. This allows the user to rebase a
     -    long list of commits in a multi-part feature and keep all of their
     -    pointers to those parts.
     +    Add a new --update-refs option to 'git rebase -i' that adds 'label
     +    for-update-refs/*' steps to the todo file whenever a commit that is
     +    being rebased is decorated with that <ref>. At the very end, the
     +    'update-refs' step is added to update all of the branches referenced by
     +    the 'label' steps. This allows the user to rebase a long list of commits
     +    in a multi-part feature and keep all of their pointers to those parts.
     +
     +    NOTE: This change only introduce the --update-refs option and implements
     +    the changes to the todo file. It does _not_ yet implement the action
     +    taken by the 'update-refs' todo step, which will be implemented and
     +    tested in a later change.
      
          Use the new for_each_decoration() while iterating over the existing todo
          list. Be sure to iterate after any squashing or fixups are placed.
     @@ Documentation/git-rebase.txt: provided. Otherwise an explicit `--no-reschedule-f
       INCOMPATIBLE OPTIONS
       --------------------
       
     +@@ Documentation/git-rebase.txt: are incompatible with the following options:
     +  * --empty=
     +  * --reapply-cherry-picks
     +  * --edit-todo
     ++ * --update-refs
     +  * --root when used in combination with --onto
     + 
     + In addition, the following pairs of options are incompatible:
      
       ## builtin/rebase.c ##
      @@ builtin/rebase.c: struct rebase_options {
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
       }
       
      +struct todo_add_branch_context {
     -+	struct todo_list new_list;
     ++	struct todo_item *items;
     ++	size_t items_nr;
     ++	size_t items_alloc;
      +	struct strbuf *buf;
      +	struct commit *commit;
      +};
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
      +{
      +	struct todo_add_branch_context *ctx = data;
      +	size_t base_offset = ctx->buf->len;
     -+	int i = ctx->new_list.nr;
      +	struct todo_item *item;
      +	char *path;
      +
     -+	ALLOC_GROW(ctx->new_list.items,
     -+		   ctx->new_list.nr + 1,
     -+		   ctx->new_list.alloc);
     -+	item = &ctx->new_list.items[i];
     ++	ALLOC_GROW(ctx->items,
     ++		   ctx->items_nr + 1,
     ++		   ctx->items_alloc);
     ++	item = &ctx->items[ctx->items_nr];
     ++	memset(item, 0, sizeof(*item));
      +
      +	/* If the branch is checked out, then leave a comment instead. */
      +	if (branch_checked_out(d->name, &path)) {
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
      +			    d->name, path);
      +		free(path);
      +	} else {
     -+		item->command = TODO_EXEC;
     -+		strbuf_addf(ctx->buf, "git update-ref %s HEAD %s\n",
     -+			    d->name, oid_to_hex(&ctx->commit->object.oid));
     ++		item->command = TODO_LABEL;
     ++		strbuf_addf(ctx->buf, "for-update-refs/%s\n", d->name);
      +	}
      +
     -+	item->commit = NULL;
      +	item->offset_in_buf = base_offset;
      +	item->arg_offset = base_offset;
      +	item->arg_len = ctx->buf->len - base_offset;
     -+	ctx->new_list.nr++;
     ++	ctx->items_nr++;
      +
      +	return 0;
      +}
      +
      +/*
      + * For each 'pick' command, find out if the commit has a decoration in
     -+ * refs/heads/. If so, then add a 'git branch -f' exec command after
     -+ * that 'pick' (plus any following 'squash' or 'fixup' commands).
     ++ * refs/heads/. If so, then add a 'label for-update-refs/' command.
      + */
     -+static int todo_list_add_branch_commands(struct todo_list *todo_list)
     ++static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
      +{
      +	int i;
      +	static struct string_list decorate_refs_exclude = STRING_LIST_INIT_NODUP;
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
      +		.exclude_ref_config_pattern = &decorate_refs_exclude_config
      +	};
      +	struct todo_add_branch_context ctx = {
     -+		.new_list = TODO_LIST_INIT,
      +		.buf = &todo_list->buf,
      +	};
      +
     ++	ctx.items_alloc = 2 * todo_list->nr + 1;
     ++	ALLOC_ARRAY(ctx.items, ctx.items_alloc);
     ++
      +	string_list_append(&decorate_refs_include, "refs/heads/");
      +	load_ref_decorations(&decoration_filter, 0);
      +
      +	for (i = 0; i < todo_list->nr; ) {
      +		struct todo_item *item = &todo_list->items[i];
      +
     -+		do {
     -+			/* insert ith item into new list */
     -+			ALLOC_GROW(ctx.new_list.items,
     -+				   ctx.new_list.nr + 1,
     -+				   ctx.new_list.alloc);
     ++		/* insert ith item into new list */
     ++		ALLOC_GROW(ctx.items,
     ++			   ctx.items_nr + 1,
     ++			   ctx.items_alloc);
      +
     -+			memcpy(&ctx.new_list.items[ctx.new_list.nr++],
     -+			       &todo_list->items[i],
     -+			       sizeof(struct todo_item));
     ++		ctx.items[ctx.items_nr++] = todo_list->items[i++];
      +
     -+			i++;
     -+		} while (i < todo_list->nr &&
     -+			 todo_list->items[i].command != TODO_PICK);
     -+
     -+		ctx.commit = item->commit;
     -+		for_each_decoration(item->commit, add_branch_for_decoration, &ctx);
     ++		if (item->commit) {
     ++			ctx.commit = item->commit;
     ++			for_each_decoration(item->commit,
     ++					    add_branch_for_decoration,
     ++					    &ctx);
     ++		}
      +	}
      +
     ++	/* Add the "update-refs" step. */
     ++	ALLOC_GROW(ctx.items,
     ++		   ctx.items_nr + 1,
     ++		   ctx.items_alloc);
     ++	memset(&ctx.items[ctx.items_nr], 0, sizeof(struct todo_item));
     ++	ctx.items[ctx.items_nr].command = TODO_UPDATE_REFS;
     ++	ctx.items_nr++;
     ++
      +	free(todo_list->items);
     -+	todo_list->items = ctx.new_list.items;
     -+	todo_list->nr = ctx.new_list.nr;
     -+	todo_list->alloc = ctx.new_list.alloc;
     ++	todo_list->items = ctx.items;
     ++	todo_list->nr = ctx.items_nr;
     ++	todo_list->alloc = ctx.items_alloc;
      +
      +	return 0;
      +}
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
       		    const char *shortrevisions, const char *onto_name,
       		    struct commit *onto, const struct object_id *orig_head,
       		    struct string_list *commands, unsigned autosquash,
     -+		    unsigned keep_decorations,
     ++		    unsigned update_refs,
       		    struct todo_list *todo_list)
       {
       	char shortonto[GIT_MAX_HEXSZ + 1];
      @@ sequencer.c: int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
     - 	if (autosquash && todo_list_rearrange_squash(todo_list))
     - 		return -1;
     + 		item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0;
     + 	}
       
     -+	if (keep_decorations && todo_list_add_branch_commands(todo_list))
     ++	if (update_refs && todo_list_add_update_ref_commands(todo_list))
      +		return -1;
      +
     - 	if (commands->nr)
     - 		todo_list_add_exec_commands(todo_list, commands);
     + 	if (autosquash && todo_list_rearrange_squash(todo_list))
     + 		return -1;
       
      
       ## sequencer.h ##
     @@ sequencer.h: int complete_action(struct repository *r, struct replay_opts *opts,
       		    const char *shortrevisions, const char *onto_name,
       		    struct commit *onto, const struct object_id *orig_head,
       		    struct string_list *commands, unsigned autosquash,
     -+		    unsigned keep_decorations,
     ++		    unsigned update_refs,
       		    struct todo_list *todo_list);
       int todo_list_rearrange_squash(struct todo_list *todo_list);
       
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'ORIG_HEAD is updated correct
       	test_cmp_rev ORIG_HEAD test-orig-head@{1}
       '
       
     -+test_expect_success '--update-refs adds git branch commands' '
     ++test_expect_success '--update-refs adds label and update-ref commands' '
      +	git checkout -b update-refs no-conflict-branch &&
     -+	test_commit extra fileX &&
     -+	git commit --amend --fixup=L &&
     ++	git branch -f base HEAD~4 &&
     ++	git branch -f first HEAD~3 &&
     ++	git branch -f second HEAD~3 &&
     ++	git branch -f third HEAD~1 &&
     ++	git commit --allow-empty --fixup=third &&
     ++	git branch -f shared-tip &&
      +	(
      +		set_cat_todo_editor &&
     -+		git branch -f base A &&
     -+		git branch -f first J &&
     -+		git branch -f second J &&
     -+		git branch -f third L &&
     -+
     -+		test_must_fail git rebase -i --autosquash --update-refs primary >todo &&
      +
      +		cat >expect <<-EOF &&
      +		pick $(git log -1 --format=%h J) J
     -+		exec git update-ref refs/heads/second HEAD $(git rev-parse J)
     -+		exec git update-ref refs/heads/first HEAD $(git rev-parse  J)
     ++		label for-update-refs/refs/heads/second
     ++		label for-update-refs/refs/heads/first
      +		pick $(git log -1 --format=%h K) K
      +		pick $(git log -1 --format=%h L) L
     -+		fixup $(git log -1 --format=%h update-refs) fixup! L
     -+		exec git update-ref refs/heads/third HEAD $(git rev-parse L)
     ++		fixup $(git log -1 --format=%h update-refs) fixup! L # empty
     ++		label for-update-refs/refs/heads/third
      +		pick $(git log -1 --format=%h M) M
     -+		exec git update-ref refs/heads/no-conflict-branch HEAD $(git rev-parse M)
     ++		label for-update-refs/refs/heads/no-conflict-branch
     ++		label for-update-refs/refs/heads/shared-tip
     ++		update-refs
      +		EOF
      +
     ++		test_must_fail git rebase -i --autosquash --update-refs primary >todo &&
     ++		test_cmp expect todo
     ++	)
     ++'
     ++
     ++test_expect_success '--update-refs adds commands with --rebase-merges' '
     ++	git checkout -b update-refs-with-merge no-conflict-branch &&
     ++	git branch -f base HEAD~4 &&
     ++	git branch -f first HEAD~3 &&
     ++	git branch -f second HEAD~3 &&
     ++	git branch -f third HEAD~1 &&
     ++	git merge -m merge branch2 &&
     ++	git branch -f merge-branch &&
     ++	git commit --fixup=third --allow-empty &&
     ++	(
     ++		set_cat_todo_editor &&
     ++
     ++		cat >expect <<-EOF &&
     ++		label onto
     ++		reset onto
     ++		pick $(git log -1 --format=%h branch2~1) F
     ++		pick $(git log -1 --format=%h branch2) I
     ++		label for-update-refs/refs/heads/branch2
     ++		label merge
     ++		reset onto
     ++		pick $(git log -1 --format=%h refs/heads/second) J
     ++		label for-update-refs/refs/heads/second
     ++		label for-update-refs/refs/heads/first
     ++		pick $(git log -1 --format=%h refs/heads/third~1) K
     ++		pick $(git log -1 --format=%h refs/heads/third) L
     ++		fixup $(git log -1 --format=%h update-refs-with-merge) fixup! L # empty
     ++		label for-update-refs/refs/heads/third
     ++		pick $(git log -1 --format=%h HEAD~2) M
     ++		label for-update-refs/refs/heads/no-conflict-branch
     ++		merge -C $(git log -1 --format=%h HEAD~1) merge # merge
     ++		label for-update-refs/refs/heads/merge-branch
     ++		update-refs
     ++		EOF
     ++
     ++		test_must_fail git rebase -i --autosquash \
     ++				   --rebase-merges=rebase-cousins \
     ++				   --update-refs primary >todo &&
     ++
      +		test_cmp expect todo
      +	)
      +'
 -:  ----------- > 6:  68f8e51b19c sequencer: implement 'update-refs' command
 4:  b99c5bf34ef ! 7:  3d7d3f656b4 rebase: add rebase.updateRefs config option
     @@ builtin/rebase.c: static int rebase_config(const char *var, const char *value, v
       		return 0;
      
       ## t/t3404-rebase-interactive.sh ##
     -@@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs adds git branch commands' '
     - 		exec git update-ref refs/heads/no-conflict-branch HEAD $(git rev-parse M)
     +@@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs adds label and update-ref commands' '
       		EOF
       
     + 		test_must_fail git rebase -i --autosquash --update-refs primary >todo &&
      +		test_cmp expect todo &&
      +
      +		test_must_fail git -c rebase.autosquash=true \
      +				   -c rebase.updaterefs=true \
      +				   rebase -i primary >todo &&
     ++
     + 		test_cmp expect todo
     + 	)
     + '
     +@@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs adds commands with --rebase-merges' '
     + 				   --rebase-merges=rebase-cousins \
     + 				   --update-refs primary >todo &&
     + 
     ++		test_cmp expect todo &&
     ++
     ++		test_must_fail git -c rebase.autosquash=true \
     ++				   -c rebase.updaterefs=true \
     ++				   rebase -i \
     ++				   --rebase-merges=rebase-cousins \
     ++				   primary >todo &&
     ++
       		test_cmp expect todo
       	)
       '

gitgitgadget

From: Derrick Stolee <[email protected]>

It can be helpful to iterate through all the decorations on a commit without necessarily writing them to a stream. Implement for_each_decoration() and reimplement format_decorations_extended() to use that iterator.

---
 log-tree.c | 111 ++++++++++++++++++++++++++++++++++++-----------------
 log-tree.h |   4 ++
 2 files changed, 80 insertions(+), 35 deletions(-)

index d0ac0a6327a..b15a7c9db22 100644
@@ -282,6 +282,54 @@ static void show_name(struct strbuf *sb, const struct name_decoration *decoratio
 		strbuf_addstr(sb, decoration->name);
 }
 
+struct format_decorations_context {
+	struct strbuf *sb;
+	int use_color;
+	const char *prefix;
+	const char *separator;
+	const char *suffix;
+	const char *color_commit;
+	const char *color_reset;
+	const struct name_decoration *current_and_HEAD;
+};
+
+static int append_decoration(const struct name_decoration *d,
+			     void *data)
+{
+	struct format_decorations_context *ctx = data;
+	/*
+	 * When both current and HEAD are there, only
+	 * show HEAD->current where HEAD would have
+	 * appeared, skipping the entry for current.
+	 */
+	if (d != ctx->current_and_HEAD) {
+		strbuf_addstr(ctx->sb, ctx->color_commit);
+		strbuf_addstr(ctx->sb, ctx->prefix);
+		strbuf_addstr(ctx->sb, ctx->color_reset);
+		strbuf_addstr(ctx->sb, decorate_get_color(ctx->use_color, d->type));
+		if (d->type == DECORATION_REF_TAG)
+			strbuf_addstr(ctx->sb, "tag: ");
+
+		show_name(ctx->sb, d);
+
+		if (ctx->current_and_HEAD &&
+		    d->type == DECORATION_REF_HEAD) {
+			strbuf_addstr(ctx->sb, " -> ");
+			strbuf_addstr(ctx->sb, ctx->color_reset);
+			strbuf_addstr(ctx->sb,
+				      decorate_get_color(
+						ctx->use_color,
+						ctx->current_and_HEAD->type));
+			show_name(ctx->sb, ctx->current_and_HEAD);
+		}
+		strbuf_addstr(ctx->sb, ctx->color_reset);
+
+		ctx->prefix = ctx->separator;
+	}
+
+	return 0;
+}
+
 /*
  * The caller makes sure there is no funny color before calling.
  * format_decorations_extended makes sure the same after return.

@@ -294,49 +342,42 @@ void format_decorations_extended(struct strbuf *sb,
 			const char *suffix)
 {
 	const struct name_decoration *decoration;
-	const struct name_decoration *current_and_HEAD;
-	const char *color_commit =
-		diff_get_color(use_color, DIFF_COMMIT);
-	const char *color_reset =
-		decorate_get_color(use_color, DECORATION_NONE);
+	struct format_decorations_context ctx = {
+		.sb = sb,
+		.use_color = use_color,
+		.prefix = prefix,
+		.separator = separator,
+		.suffix = suffix,
+		.color_commit =	diff_get_color(use_color, DIFF_COMMIT),
+		.color_reset = decorate_get_color(use_color, DECORATION_NONE),
+	};
 
 	decoration = get_name_decoration(&commit->object);
 	if (!decoration)
 		return;
 
-	current_and_HEAD = current_pointed_by_HEAD(decoration);
-	while (decoration) {
-		/*
-		 * When both current and HEAD are there, only
-		 * show HEAD->current where HEAD would have
-		 * appeared, skipping the entry for current.
-		 */
-		if (decoration != current_and_HEAD) {
-			strbuf_addstr(sb, color_commit);
-			strbuf_addstr(sb, prefix);
-			strbuf_addstr(sb, color_reset);
-			strbuf_addstr(sb, decorate_get_color(use_color, decoration->type));
-			if (decoration->type == DECORATION_REF_TAG)
-				strbuf_addstr(sb, "tag: ");
-
-			show_name(sb, decoration);
-
-			if (current_and_HEAD &&
-			    decoration->type == DECORATION_REF_HEAD) {
-				strbuf_addstr(sb, " -> ");
-				strbuf_addstr(sb, color_reset);
-				strbuf_addstr(sb, decorate_get_color(use_color, current_and_HEAD->type));
-				show_name(sb, current_and_HEAD);
-			}
-			strbuf_addstr(sb, color_reset);
+	ctx.current_and_HEAD = current_pointed_by_HEAD(decoration);
 
-			prefix = separator;
-		}
+	for_each_decoration(commit, append_decoration, &ctx);
+
+	strbuf_addstr(sb, ctx.color_commit);
+	strbuf_addstr(sb, ctx.suffix);
+	strbuf_addstr(sb, ctx.color_reset);
+}
+
+int for_each_decoration(const struct commit *c, decoration_fn fn, void *data)
+{
+	const struct name_decoration *decoration;
+
+	decoration = get_name_decoration(&c->object);
+	while (decoration) {
+		int res;
+		if ((res = fn(decoration, data)))
+			return res;
 		decoration = decoration->next;
 	}
-	strbuf_addstr(sb, color_commit);
-	strbuf_addstr(sb, suffix);
-	strbuf_addstr(sb, color_reset);
+
+	return 0;
 }
 
 void show_decorations(struct rev_info *opt, struct commit *commit)

index e7e4641cf83..ea07da2625b 100644
@@ -35,4 +35,8 @@ void fmt_output_commit(struct strbuf *, struct commit *, struct rev_info *);
 void fmt_output_subject(struct strbuf *, const char *subject, struct rev_info *);
 void fmt_output_email_subject(struct strbuf *, struct rev_info *);
 
+typedef int decoration_fn(const struct name_decoration *d,
+			  void *data);
+int for_each_decoration(const struct commit *c, decoration_fn fn, void *data);
+
 #endif

gitgitgadget

From: Derrick Stolee <[email protected]>

The validate_new_branchname() method contains a check to see if a branch is checked out in any non-bare worktree. This is intended to prevent a force push that will mess up an existing checkout. This helper is not suitable to performing just that check, because the method will die() when the branch is checked out instead of returning an error code.

Extract branch_checked_out() and use it within validate_new_branchname(). Another caller will be added in a coming change.

---
 branch.c | 24 ++++++++++++++++--------
 branch.h |  8 ++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

index 2d6569b0c62..2e6419cdfa5 100644
@@ -369,6 +369,19 @@ int validate_branchname(const char *name, struct strbuf *ref)
 	return ref_exists(ref->buf);
 }
 
+int branch_checked_out(const char *refname, char **path)
+{
+	struct worktree **worktrees = get_worktrees();
+	const struct worktree *wt = find_shared_symref(worktrees, "HEAD", refname);
+	int result = wt && !wt->is_bare;
+
+	if (result && path)
+		*path = xstrdup(wt->path);
+
+	free_worktrees(worktrees);
+	return result;
+}
+
 /*
  * Check if a branch 'name' can be created as a new branch; die otherwise.
  * 'force' can be used when it is OK for the named branch already exists.

@@ -377,9 +390,7 @@ int validate_branchname(const char *name, struct strbuf *ref)
  */
 int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 {
-	struct worktree **worktrees;
-	const struct worktree *wt;
-
+	char *path;
 	if (!validate_branchname(name, ref))
 		return 0;
 

@@ -387,13 +398,10 @@ int validate_new_branchname(const char *name, struct strbuf *ref, int force)
 		die(_("a branch named '%s' already exists"),
 		    ref->buf + strlen("refs/heads/"));
 
-	worktrees = get_worktrees();
-	wt = find_shared_symref(worktrees, "HEAD", ref->buf);
-	if (wt && !wt->is_bare)
+	if (branch_checked_out(ref->buf, &path))
 		die(_("cannot force update the branch '%s' "
 		      "checked out at '%s'"),
-		    ref->buf + strlen("refs/heads/"), wt->path);
-	free_worktrees(worktrees);
+		    ref->buf + strlen("refs/heads/"), path);
 
 	return 1;
 }

index 560b6b96a8f..5ea93d217b1 100644
@@ -101,6 +101,14 @@ void create_branches_recursively(struct repository *r, const char *name,
 				 const char *tracking_name, int force,
 				 int reflog, int quiet, enum branch_track track,
 				 int dry_run);
+
+/*
+ * Returns true if the branch at 'refname' is checked out at any
+ * non-bare worktree. The path of the worktree is stored in the
+ * given 'path', if provided.
+ */
+int branch_checked_out(const char *refname, char **path);
+
 /*
  * Check if 'name' can be a valid name for a branch; die otherwise.
  * Return 1 if the named branch already exists; return 0 otherwise.

gitgitgadget

Sorry, you had mentioned this in v1 and I just forgot to fix this.

Yes, that's a good plan. I'll take a look.

Thanks,
-Stolee

Here is a fixup for this patch that should work. I'll put it in v3.

index 2e6419cdfa5..514212f5619 100644
@@ -10,6 +10,7 @@
 #include "worktree.h"
 #include "submodule-config.h"
 #include "run-command.h"
+#include "strmap.h"
 
 struct tracking {
 	struct refspec_item spec;

@@ -369,17 +370,44 @@ int validate_branchname(const char *name, struct strbuf *ref)
 	return ref_exists(ref->buf);
 }
 
-int branch_checked_out(const char *refname, char **path)
+static int initialized_checked_out_branches;
+static struct strmap current_checked_out_branches = STRMAP_INIT;
+
+static void prepare_checked_out_branches(void)
 {
-	struct worktree **worktrees = get_worktrees();
-	const struct worktree *wt = find_shared_symref(worktrees, "HEAD", refname);
-	int result = wt && !wt->is_bare;
+	int i = 0;
+	struct worktree **worktrees;
+
+	if (initialized_checked_out_branches)
+		return;
+	initialized_checked_out_branches = 1;
+
+	worktrees = get_worktrees();
+
+	while (worktrees[i]) {
+		struct worktree *wt = worktrees[i];
 
-	if (result && path)
-		*path = xstrdup(wt->path);
+		if (!wt->is_bare && wt->head_ref)
+			strmap_put(&current_checked_out_branches,
+				   wt->head_ref,
+				   wt->path);
+
+		i++;
+	}
 
 	free_worktrees(worktrees);
-	return result;
+}
+
+int branch_checked_out(const char *refname, char **path)
+{
+	const char *path_in_set;
+	prepare_checked_out_branches();
+
+	path_in_set = strmap_get(&current_checked_out_branches, refname);
+	if (path_in_set && path)
+		*path = xstrdup(path_in_set);
+
+	return !!path_in_set;
 }
 
 /*

The patch there was this:

index e3791f09ed5..eeee5ac8f15 100644
@@ -1440,6 +1440,7 @@ static void check_not_current_branch(struct ref *ref_map,
 	const struct worktree *wt;
 	for (; ref_map; ref_map = ref_map->next)
 		if (ref_map->peer_ref &&
+		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
 		    (wt = find_shared_symref(worktrees, "HEAD",
 					     ref_map->peer_ref->name)) &&
 		    !wt->is_bare)

And there is another use of find_shared_symref() in the same file, allowing us to do the following:

index ac29c2b1ae3..3933c482839 100644
@@ -885,7 +885,7 @@ static int update_local_ref(struct ref *ref,
 			    struct worktree **worktrees)
 {
 	struct commit *current = NULL, *updated;
-	const struct worktree *wt;
+	char *path = NULL;
 	const char *pretty_ref = prettify_refname(ref->name);
 	int fast_forward = 0;
 

@@ -900,17 +900,17 @@ static int update_local_ref(struct ref *ref,
 	}
 
 	if (!update_head_ok &&
-	    (wt = find_shared_symref(worktrees, "HEAD", ref->name)) &&
-	    !wt->is_bare && !is_null_oid(&ref->old_oid)) {
+	    !is_null_oid(&ref->old_oid) &&
+	    branch_checked_out(ref->name, &path)) {
 		/*
 		 * If this is the head, and it's not okay to update
 		 * the head, and the old value of the head isn't empty...
 		 */
 		format_display(display, '!', _("[rejected]"),
-			       wt->is_current ?
-				       _("can't fetch in current branch") :
-				       _("checked out in another worktree"),
+			       path ? _("can't fetch in current branch") :
+				      _("checked out in another worktree"),
 			       remote, pretty_ref, summary_width);
+		free(path);
 		return 1;
 	}
 

@@ -1437,16 +1437,14 @@ static int prune_refs(struct refspec *rs,
 static void check_not_current_branch(struct ref *ref_map,
 				     struct worktree **worktrees)
 {
-	const struct worktree *wt;
+	char *path;
 	for (; ref_map; ref_map = ref_map->next)
 		if (ref_map->peer_ref &&
 		    starts_with(ref_map->peer_ref->name, "refs/heads/") &&
-		    (wt = find_shared_symref(worktrees, "HEAD",
-					     ref_map->peer_ref->name)) &&
-		    !wt->is_bare)
+		    branch_checked_out(ref_map->peer_ref->name, &path))
 			die(_("refusing to fetch into branch '%s' "
 			      "checked out at '%s'"),
-			    ref_map->peer_ref->name, wt->path);
+			    ref_map->peer_ref->name, path);
 }
 
 static int truncate_fetch_head(void)

I can extract these as patches in their own series if we think that's something we worth fast-tracking.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

The todo_command_info array defines which strings match with which todo_command enum values. The array is defined in the same order as the enum values, but if one changed without the other, then we would have unexpected results.

Make it easier to see changes to the enum and this array by using the enum values as the indices of the array.

---
 sequencer.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

index 8c3ed3532ac..8e26c9a6261 100644
@@ -1685,20 +1685,20 @@ static struct {
 	char c;
 	const char *str;
 } todo_command_info[] = {
-	{ 'p', "pick" },
-	{ 0,   "revert" },
-	{ 'e', "edit" },
-	{ 'r', "reword" },
-	{ 'f', "fixup" },
-	{ 's', "squash" },
-	{ 'x', "exec" },
-	{ 'b', "break" },
-	{ 'l', "label" },
-	{ 't', "reset" },
-	{ 'm', "merge" },
-	{ 0,   "noop" },
-	{ 'd', "drop" },
-	{ 0,   NULL }
+	[TODO_PICK] = { 'p', "pick" },
+	[TODO_REVERT] = { 0,   "revert" },
+	[TODO_EDIT] = { 'e', "edit" },
+	[TODO_REWORD] = { 'r', "reword" },
+	[TODO_FIXUP] = { 'f', "fixup" },
+	[TODO_SQUASH] = { 's', "squash" },
+	[TODO_EXEC] = { 'x', "exec" },
+	[TODO_BREAK] = { 'b', "break" },
+	[TODO_LABEL] = { 'l', "label" },
+	[TODO_RESET] = { 't', "reset" },
+	[TODO_MERGE] = { 'm', "merge" },
+	[TODO_NOOP] = { 0,   "noop" },
+	[TODO_DROP] = { 'd', "drop" },
+	[TODO_COMMENT] = { 0,   NULL },
 };
 
 static const char *command_to_string(const enum todo_command command)

gitgitgadget

Good thinking.

I love seeing a future-proofing change like this.

Thanks.

From: Derrick Stolee <[email protected]>

Add the boilerplat for an "update-refs" command in the sequencer. This connects to the current no-op do_update_refs() which will be filled in after more connections are created.

---
 sequencer.c | 13 ++++++++++++-
 sequencer.h |  1 +
 2 files changed, 13 insertions(+), 1 deletion(-)

index 8e26c9a6261..68f7c76e896 100644
@@ -1696,6 +1696,7 @@ static struct {
 	[TODO_LABEL] = { 'l', "label" },
 	[TODO_RESET] = { 't', "reset" },
 	[TODO_MERGE] = { 'm', "merge" },
+	[TODO_UPDATE_REFS] = { 'u', "update-refs" },
 	[TODO_NOOP] = { 0,   "noop" },
 	[TODO_DROP] = { 'd', "drop" },
 	[TODO_COMMENT] = { 0,   NULL },

@@ -2442,7 +2443,9 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 	padding = strspn(bol, " \t");
 	bol += padding;
 
-	if (item->command == TODO_NOOP || item->command == TODO_BREAK) {
+	if (item->command == TODO_NOOP ||
+	    item->command == TODO_BREAK ||
+	    item->command == TODO_UPDATE_REFS) {
 		if (bol != eol)
 			return error(_("%s does not accept arguments: '%s'"),
 				     command_to_string(item->command), bol);

@@ -4056,6 +4059,11 @@ leave_merge:
 	return ret;
 }
 
+static int do_update_refs(struct repository *r)
+{
+	return 0;
+}
+
 static int is_final_fixup(struct todo_list *todo_list)
 {
 	int i = todo_list->current;

@@ -4431,6 +4439,9 @@ static int pick_commits(struct repository *r,
 				return error_with_patch(r, item->commit,
 							arg, item->arg_len,
 							opts, res, 0);
+		} else if (item->command == TODO_UPDATE_REFS) {
+			if ((res = do_update_refs(r)))
+				reschedule = 1;
 		} else if (!is_noop(item->command))
 			return error(_("unknown command %d"), item->command);
 

index da64473636b..c2b4e148d8f 100644
@@ -95,6 +95,7 @@ enum todo_command {
 	TODO_LABEL,
 	TODO_RESET,
 	TODO_MERGE,
+	TODO_UPDATE_REFS,
 	/* commands that do nothing but are counted for reporting progress */
 	TODO_NOOP,
 	TODO_DROP,

gitgitgadget

From: Derrick Stolee <[email protected]>

When working on a large feature, it can be helpful to break that feature into multiple smaller parts that become reviewed in sequence. During development or during review, a change to one part of the feature could affect multiple of these parts. An interactive rebase can help adjust the multi-part "story" of the branch.

However, if there are branches tracking the different parts of the feature, then rebasing the entire list of commits can create commits not reachable from those "sub branches". It can take a manual step to update those branches.

Add a new --update-refs option to 'git rebase -i' that adds 'label for-update-refs/*' steps to the todo file whenever a commit that is being rebased is decorated with that <ref>. At the very end, the 'update-refs' step is added to update all of the branches referenced by the 'label' steps. This allows the user to rebase a long list of commits in a multi-part feature and keep all of their pointers to those parts.

NOTE: This change only introduce the --update-refs option and implements the changes to the todo file. It does _not_ yet implement the action taken by the 'update-refs' todo step, which will be implemented and tested in a later change.

Use the new for_each_decoration() while iterating over the existing todo list. Be sure to iterate after any squashing or fixups are placed. Update the branch only after those squashes and fixups are complete. This allows a --fixup commit at the tip of the feature to apply correctly to the sub branch, even if it is fixing up the most-recent commit in that part.

One potential problem here is that refs decorating commits that are already marked as "fixup!" or "squash!" will not be included in this list. Generally, the reordering of the "fixup!" and "squash!" is likely to change the relative order of these refs, so it is not recommended. The workflow here is intended to allow these kinds of commits at the tip of the rebased branch while the other sub branches come along for the ride without intervention.

Be careful to not attempt updating any branch that is checked out. The most common example is the branch being rebased is checked out and decorates the tip commit. If the user is rebasing commits reachable from a different branch that is checked out in a different worktree, then they may be surprised to not see that ref update. However, it's probably best to not optimize for this scenario and do the safest thing that will result in a successful rebase. A comment is left in the TODO list that signals that these refs are currently checked out.

---
 Documentation/git-rebase.txt  |   8 +++
 builtin/rebase.c              |   5 ++
 sequencer.c                   | 105 ++++++++++++++++++++++++++++++++++
 sequencer.h                   |   1 +
 t/t3404-rebase-interactive.sh |  72 +++++++++++++++++++++++
 5 files changed, 191 insertions(+)

index 262fb01aec0..e7611b4089c 100644
@@ -609,6 +609,13 @@ provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
 start would be overridden by the presence of
 `rebase.rescheduleFailedExec=true` configuration.
 
+--update-refs::
+--no-update-refs::
+	Automatically force-update any branches that point to commits that
+	are being rebased. Any branches that are checked out in a worktree
+	or point to a `squash! ...` or `fixup! ...` commit are not updated
+	in this way.
+
 INCOMPATIBLE OPTIONS
 --------------------
 

@@ -632,6 +639,7 @@ are incompatible with the following options:
  * --empty=
  * --reapply-cherry-picks
  * --edit-todo
+ * --update-refs
  * --root when used in combination with --onto
 
 In addition, the following pairs of options are incompatible:

index 7ab50cda2ad..56d82a52106 100644
@@ -102,6 +102,7 @@ struct rebase_options {
 	int reschedule_failed_exec;
 	int reapply_cherry_picks;
 	int fork_point;
+	int update_refs;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\

@@ -298,6 +299,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
 			&opts->orig_head, &commands, opts->autosquash,
+			opts->update_refs,
 			&todo_list);
 	}
 

@@ -1124,6 +1126,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "autosquash", &options.autosquash,
 			 N_("move commits that begin with "
 			    "squash!/fixup! under -i")),
+		OPT_BOOL(0, "update-refs", &options.update_refs,
+			 N_("update local refs that point to commits "
+			    "that are being rebased")),
 		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },

index c2b4e148d8f..e268208b315 100644
@@ -167,6 +167,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const struct object_id *orig_head,
 		    struct string_list *commands, unsigned autosquash,
+		    unsigned update_refs,
 		    struct todo_list *todo_list);
 int todo_list_rearrange_squash(struct todo_list *todo_list);
 

gitgitgadget

From: Derrick Stolee <[email protected]>

The previous change allowed 'git rebase --update-refs' to create 'label' commands for each branch among the commits being rewritten and add an 'update-refs' command at the end of the todo list. Now, teach Git to update the refs during that final 'update-refs' command.

We need to create an array of new and old OIDs for each ref by iterating over the refs/rewritten/for-update-refs/ namespace. We cannot update the refs in-place since this will confuse the refs iterator.

---
 sequencer.c                   | 62 ++++++++++++++++++++++++++++++++++-
 t/t3404-rebase-interactive.sh | 24 ++++++++++++++
 2 files changed, 85 insertions(+), 1 deletion(-)

index 94f8d52e041..a8f62ce8e5f 100644
@@ -4061,11 +4061,71 @@ leave_merge:
 	return ret;
 }
 
-static int do_update_refs(struct repository *r)
+struct update_refs_context {
+	struct ref_store *refs;
+	char **ref_names;
+	struct object_id *old;
+	struct object_id *new;
+	size_t nr;
+	size_t alloc;
+};
+
+static int add_ref_to_context(const char *refname,
+			      const struct object_id *oid,
+			      int flags,
+			      void *data)
 {
+	int f = 0;
+	const char *name;
+	struct update_refs_context *ctx = data;
+
+	ALLOC_GROW(ctx->ref_names, ctx->nr + 1, ctx->alloc);
+	ALLOC_GROW(ctx->old, ctx->nr + 1, ctx->alloc);
+	ALLOC_GROW(ctx->new, ctx->nr + 1, ctx->alloc);
+
+	if (!skip_prefix(refname, "refs/rewritten/for-update-refs/", &name))
+		return 1;
+
+	ctx->ref_names[ctx->nr] = xstrdup(name);
+	oidcpy(&ctx->new[ctx->nr], oid);
+	if (!refs_resolve_ref_unsafe(ctx->refs, name, 0,
+				     &ctx->old[ctx->nr], &f))
+		return 1;
+
+	ctx->nr++;
 	return 0;
 }
 
+static int do_update_refs(struct repository *r)
+{
+	int i, res;
+	struct update_refs_context ctx = {
+		.refs = get_main_ref_store(r),
+		.alloc = 16,
+	};
+	ALLOC_ARRAY(ctx.ref_names, ctx.alloc);
+	ALLOC_ARRAY(ctx.old, ctx.alloc);
+	ALLOC_ARRAY(ctx.new, ctx.alloc);
+
+	res = refs_for_each_fullref_in(ctx.refs,
+				       "refs/rewritten/for-update-refs/",
+				       add_ref_to_context,
+				       &ctx);
+
+	for (i = 0; !res && i < ctx.nr; i++)
+		res = refs_update_ref(ctx.refs, "rewritten during rebase",
+				ctx.ref_names[i],
+				&ctx.new[i], &ctx.old[i],
+				0, UPDATE_REFS_MSG_ON_ERR);
+
+	for (i = 0; i < ctx.nr; i++)
+		free(ctx.ref_names[i]);
+	free(ctx.ref_names);
+	free(ctx.old);
+	free(ctx.new);
+	return res;
+}
+
 static int is_final_fixup(struct todo_list *todo_list)
 {
 	int i = todo_list->current;

index 5e99ad7f3b6..72711efec28 100755
@@ -1815,6 +1815,30 @@ test_expect_success '--update-refs adds commands with --rebase-merges' '
 	)
 '
 
+compare_two_refs () {
+	git rev-parse $1 >expect &&
+	git rev-parse $2 >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success '--update-refs updates refs correctly' '
+	git checkout -B update-refs no-conflict-branch &&
+	git branch -f base HEAD~4 &&
+	git branch -f first HEAD~3 &&
+	git branch -f second HEAD~3 &&
+	git branch -f third HEAD~1 &&
+	test_commit extra2 fileX &&
+	git commit --amend --fixup=L &&
+	(
+		git rebase -i --autosquash --update-refs primary &&
+
+		compare_two_refs HEAD~3 refs/heads/first &&
+		compare_two_refs HEAD~3 refs/heads/second &&
+		compare_two_refs HEAD~1 refs/heads/third &&
+		compare_two_refs HEAD refs/heads/no-conflict-branch
+	)
+'
+
 # This must be the last test in this file
 test_expect_success '$EDITOR and friends are unchanged' '
 	test_editor_unchanged

gitgitgadget

In other words, grab everything we need to do and then use that in-core information to tell refs API what refs to change to what values? That dounds like a quite reasonable thing to do.

Looking at the patch text, the only thing that stands out at me is that this does not seem to perform the updates in a single transaction, which may often not matter in practice, but may be a prudent thing to do anyway at philosophical level.

Thanks, will queue.

From: Derrick Stolee <[email protected]>

The previous change added the --update-refs command-line option. For users who always want this mode, create the rebase.updateRefs config option which behaves the same way as rebase.autoSquash does with the --autosquash option.

---
 Documentation/config/rebase.txt |  3 +++
 Documentation/git-rebase.txt    |  4 ++++
 builtin/rebase.c                |  5 +++++
 t/t3404-rebase-interactive.sh   | 14 ++++++++++++++
 4 files changed, 26 insertions(+)

index 8c979cb20f2..f19bd0e0407 100644
@@ -21,6 +21,9 @@ rebase.autoStash::
 	`--autostash` options of linkgit:git-rebase[1].
 	Defaults to false.
 
+rebase.updateRefs::
+	If set to true enable `--update-refs` option by default.
+
 rebase.missingCommitsCheck::
 	If set to "warn", git rebase -i will print a warning if some
 	commits are removed (e.g. a line was deleted), however the

index e7611b4089c..7a56dbb9bec 100644
@@ -615,6 +615,10 @@ start would be overridden by the presence of
 	are being rebased. Any branches that are checked out in a worktree
 	or point to a `squash! ...` or `fixup! ...` commit are not updated
 	in this way.
++
+If the `--update-refs` option is enabled by default using the
+configuration variable `rebase.updateRefs`, this option can be
+used to override and disable this setting.
 
 INCOMPATIBLE OPTIONS
 --------------------

index 56d82a52106..8ebc98ea505 100644
@@ -802,6 +802,11 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "rebase.updaterefs")) {
+		opts->update_refs = git_config_bool(var, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "rebase.reschedulefailedexec")) {
 		opts->reschedule_failed_exec = git_config_bool(var, value);
 		return 0;

index 72711efec28..f580afd8723 100755
@@ -1769,6 +1769,12 @@ test_expect_success '--update-refs adds label and update-ref commands' '
 		EOF
 
 		test_must_fail git rebase -i --autosquash --update-refs primary >todo &&
+		test_cmp expect todo &&
+
+		test_must_fail git -c rebase.autosquash=true \
+				   -c rebase.updaterefs=true \
+				   rebase -i primary >todo &&
+
 		test_cmp expect todo
 	)
 '

@@ -1811,6 +1817,14 @@ test_expect_success '--update-refs adds commands with --rebase-merges' '
 				   --rebase-merges=rebase-cousins \
 				   --update-refs primary >todo &&
 
+		test_cmp expect todo &&
+
+		test_must_fail git -c rebase.autosquash=true \
+				   -c rebase.updaterefs=true \
+				   rebase -i \
+				   --rebase-merges=rebase-cousins \
+				   primary >todo &&
+
 		test_cmp expect todo
 	)
 '

gitgitgadget

Hi Stolee

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

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

The label commands do as advertised, but the 'update-refs' command does something with the set of refs based on their names, yes. We would need to store this information during the rebase _somewhere_, and refs/rewritten/ seems natural.

I agree that your proposed replacement for "label for-update-refs/..." would look cleaner. I don't think that that is enough to justify hiding information from the user.

The operation we are doing necessarily requires a force-update, since we are modifying history. The safety you are caring about here is about the case where the user modifies one of these refs between the initial 'git rebase --update-refs' and the final 'git rebase --continue' that actually writes the refs. You want to prevent that final update from succeeding because another change to those refs could be lost.

I'm less concerned about this case, because the user is requesting that we update the refs pointing to this set of commits. But, I'm glad you brought it up.

One way to prevent this situation would be to extend the idea of "what branch is being rebased?" (REBASE_HEAD) to "which branches are being rewritten?" (REBASE_HEADS?). If the worktree kept the full list somewhere in the $GIT_DIR, then other Git commands could notice that those refs are currently being overwritten and those updates should fail (similarly to how we currently fail to update a branch being rebased). This ties into your later point:

I concede that allowing the user to move the 'update-refs' command around is not super important.

The biggest concern I originally had with this idea was that there was no connection between the "--update-refs" option in the first "git rebase" command and the final "git rebase --continue" command. That is, other than which refs are in refs/rewritten/*.

HOWEVER, using refs/rewritten/* is likely buggy: if we had two rebases happening at the same time (operating on a disjoint set of refs), then how do we differentiate which refs make sense for us to update as we complete this rebase?

So, I'm coming to the conclusion that using refs/rewritten/* is problematic and I should use something closer to REBASE_HEAD as a way to describe which refs are being updated. In that context, your 'update-ref <ref>' command makes a lot more sense. The user can decide to move those around _or_ remove them; it won't change their protection under "REBASE_HEADS" but would prevent them from being rewritten.

While I think on this, I'll send my branch_checked_out() patches in a separate thread, since those have grown in complexity since this version.

Thanks,
-Stolee

Hi Stolee

In general I think it is better not to burden the user with unnecessary information - it makes the ui more complicated and exposes implementation details which makes it harder to change the implementation.

At the end of the rebase we pass the oid of the branch that we store at the start of the rebase when updating the ref to avoid nasty surprises. I think it makes sense to do the same for the other branches being rewritten - it is easy to get stuck on a conflict resolution and go and do something else for a while and forget a branch is being rebased. If we cannot update the ref at the end of the rebase we should print the new oid with instructions to run "git reset --hard" or "git update-ref" if the user wants to update the branch.

Just to clarify REBASE_HEAD points to the commit we're currently trying to pick, the branch ref is stored it .git/rebase-merge/head-name and its oid in .git/rebase-merge/orig-head

That's a good point and one that I should have remembered as I implemented that check in my script. I agree that we should stop the user starting a rebase in a worktree whose branch is being updated elsewhere. If we write a list of the refs we want to update under .git/rebase-merge before walking the other worktrees to see what's being rebased elsewhere that avoids a race where two processes start that try to rebase the same branch in different worktrees at the same time. It would be easy to store the original oids with the ref names.

An added complexity is that we would have to check the todo list for any new update-ref commands and check those refs are not being rebased elsewhere each time the list is edited. In the future we should update "git status" to read that file but I don't think that needs to be part of the initial implementation.

Whenever one adds a new option to rebase it almost always involves writing more state to .git/rebase-merge, in this case I think we want to store the branches from the update-ref commands and their original oids there. I'm not sure that we want to expose the file to the user though so we can change the implementation in the future if we need to (e.g. we may decide it is better to store this information for all worktrees under a single file in $GIT_COMMON_DIR).

If you want to discuss any ideas face-to-face drop me a email and we can arrange a time to chat if that would be useful.

Best Wishes

Phillip

(Update: This is now based on ds/branch-checked-out.)

This is a feature I've wanted for quite a while. When working on the sparse index topic, I created a long RFC that actually broke into three topics for full review upstream. These topics were sequential, so any feedback on an earlier one required updates to the later ones. I would work on the full feature and use interactive rebase to update the full list of commits. However, I would need to update the branches pointing to those sub-topics.

This series adds a new --update-refs option to 'git rebase' (along with a rebase.updateRefs config option) that adds 'update-ref' commands into the TODO list. This is powered by the commit decoration machinery.

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 update-ref refs/heads/bundle-redo/fetch

pick 1e3f6546632 clone: add --bundle-uri option
pick 9e4a6fe9b68 clone: --bundle-uri cannot be combined with --depth update-ref refs/heads/bundle-redo/clone

pick 5451cb6599c bundle-uri: create bundle_list struct and helpers pick 3029c3aca15 bundle-uri: create base key-value pair parsing pick a8b2de79ce8 bundle-uri: create "key=value" line parsing pick 92625a47673 bundle-uri: unit test "key=value" parsing pick a8616af4dc2 bundle-uri: limit recursion depth for bundle lists pick 9d6809a8d53 bundle-uri: parse bundle list in config format pick 287a732b54c bundle-uri: fetch a list of bundles update-ref refs/heads/bundle-redo/list

pick b09f8226185 protocol v2: add server-side "bundle-uri" skeleton pick 520204dcd1c bundle-uri client: add minimal NOOP client
pick 62e8b457b48 bundle-uri client: add "git ls-remote-bundle-uri" pick 00eae925043 bundle-uri: serve URI advertisement from bundle.* config pick 4277440a250 bundle-uri client: add boolean transfer.bundleURI setting pick caf4599a81d bundle-uri: allow relative URLs in bundle lists pick df255000b7e bundle-uri: download bundles from an advertised list pick d71beabf199 clone: unbundle the advertised bundles
pick c9578391976 t5601: basic bundle URI tests
# Ref refs/heads/bundle-redo/rfc-3 checked out at '/home/stolee/_git/git-bundles'

update-ref refs/heads/bundle-redo/advertise

Here is an outline of the series:

* Patch 1 updates some tests for branch_checked_out() for the 'apply' backend.
* Patch 2 updates branch_checked_out() to parse the rebase-merge/update-refs file to block concurrent ref updates and checkouts on branches selected by --update-refs.
* Patch 3 updates the todo list documentation to remove some unnecessary dots in the 'merge' command. This makes it consistent with the 'fixup' command before we document the 'update-ref' command.
* Patch 4 updates the definition of todo_command_info to use enum values as array indices.
* Patches 5-7 implement the --update-refs logic itself.
* Patch 8 adds the rebase.updateRefs config option similar to rebase.autoSquash.

Updates in v3
=============

* The branch_checked_out() API was extracted to its own topic and is now the ds/branch-checked-out branch. This series is now based on that one.
* The for_each_decoration() API was removed, since it became trivial once it did not take a commit directly.
* The branch_checked_out() tests did not verify the rebase-apply data (for the apply backend), so that is fixed.
* Instead of using the 'label' command and a final 'update-refs' command in the todo list, use a new 'update-ref ' command. This command updates the rebase-merge/update-refs file with the OID of HEAD at these steps. At the very end of the rebase sequence, those refs are updated to the stored OID values (assuming that they were not removed by the user, in which case we notice that the OID is the null OID and we do nothing).
* New tests are added.
* The todo-list comment documentation has some new formatting updates, but also includes a description of 'update-refs' in this version.

Updates in v2
=============

As recommended by the excellent feedback, I have removed the 'exec' commands in favor of the 'label' commands and a new 'update-refs' command at the very end. This way, there is only one step that updates all of the refs at the end instead of updating refs during the rebase. If a user runs 'git rebase --abort' in the middle, then their refs are still where they need to be.

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.
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.

Other updates:

* The 'keep_decorations' parameter was renamed to 'update_refs'.
* I added tests for --rebase-merges=rebase-cousins to show how these labels interact with other labels and merge commands.
* I changed the order of the insertion of these update-refs labels to be before the fixups are rearranged. This fixes a bug where the tip commit is a fixup! so its decorations are never inspected (and they would be in the wrong place even if they were). The fixup! commands are properly inserted between a pick and its following label command. Tests demonstrate this is correct.
* Numerous style choices are updated based on feedback.

Thank you for all of the detailed review and ideas in this space. I appreciate any more ideas that can make this feature as effective as it can be.

Thanks, -Stolee

Derrick Stolee (8):
t2407: test branches currently using apply backend
branch: consider refs under 'update-refs'
rebase-interactive: update 'merge' description
sequencer: define array with enum values
sequencer: add update-ref command
rebase: add --update-refs option
rebase: update refs from 'update-ref' commands
rebase: add rebase.updateRefs config option

 Documentation/config/rebase.txt |   3 +
 Documentation/git-rebase.txt    |  11 ++
 branch.c                        |  13 ++
 builtin/rebase.c                |  10 ++
 rebase-interactive.c            |   9 +-
 sequencer.c                     | 303 ++++++++++++++++++++++++++++++--
 sequencer.h                     |  11 ++
 t/t2407-worktree-heads.sh       |  52 +++++-
 t/t3404-rebase-interactive.sh   | 107 +++++++++++
 9 files changed, 496 insertions(+), 23 deletions(-)

base-commit: 9bef0b1e6ec371e786c2fba3edcc06ad040a536c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1247%2Fderrickstolee%2Frebase-keep-decorations-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1247/derrickstolee/rebase-keep-decorations-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1247

Range-diff vs v2:

 1:  4f9f3487641 < -:  ----------- log-tree: create for_each_decoration()
 2:  5f54766e103 < -:  ----------- branch: add branch_checked_out() helper
 -:  ----------- > 1:  fbaedc7f1f0 t2407: test branches currently using apply backend
 -:  ----------- > 2:  2bc647b6fcd branch: consider refs under 'update-refs'
 -:  ----------- > 3:  669f4abd59e rebase-interactive: update 'merge' description
 3:  9f261c7df2c = 4:  6528a50343f sequencer: define array with enum values
 4:  842b2186d25 ! 5:  e95ad41d355 sequencer: add update-refs command
     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    sequencer: add update-refs command
     +    sequencer: add update-ref command
      
     -    Add the boilerplat for an "update-refs" command in the sequencer. This
     -    connects to the current no-op do_update_refs() which will be filled in
     +    Add the boilerplate for an "update-ref" command in the sequencer. This
     +    connects to the current no-op do_update_ref() which will be filled in
          after more connections are created.
      
     +    The syntax in the todo list will be "update-ref <ref-name>" to signal
     +    that we should store the current commit as the value for updating
     +    <ref-name> at the end of the rebase.
     +
          Signed-off-by: Derrick Stolee <[email protected]>
      
     + ## rebase-interactive.c ##
     +@@ rebase-interactive.c: void append_todo_help(int command_count,
     + "        create a merge commit using the original merge commit's\n"
     + "        message (or the oneline, if no original merge commit was\n"
     + "        specified); use -c <commit> to reword the commit message\n"
     ++"u, update-ref <ref> = track a placeholder for the <ref> to be updated\n"
     ++"                      to this position in the new commits. The <ref> is\n"
     ++"                      updated at the end of the rebase\n"
     + "\n"
     + "These lines can be re-ordered; they are executed from top to bottom.\n");
     + 	unsigned edit_todo = !(shortrevisions && shortonto);
     +
       ## sequencer.c ##
      @@ sequencer.c: static struct {
       	[TODO_LABEL] = { 'l', "label" },
       	[TODO_RESET] = { 't', "reset" },
       	[TODO_MERGE] = { 'm', "merge" },
     -+	[TODO_UPDATE_REFS] = { 'u', "update-refs" },
     ++	[TODO_UPDATE_REF] = { 'u', "update-ref" },
       	[TODO_NOOP] = { 0,   "noop" },
       	[TODO_DROP] = { 'd', "drop" },
       	[TODO_COMMENT] = { 0,   NULL },
      @@ sequencer.c: static int parse_insn_line(struct repository *r, struct todo_item *item,
     - 	padding = strspn(bol, " \t");
     - 	bol += padding;
     + 			     command_to_string(item->command));
       
     --	if (item->command == TODO_NOOP || item->command == TODO_BREAK) {
     -+	if (item->command == TODO_NOOP ||
     -+	    item->command == TODO_BREAK ||
     -+	    item->command == TODO_UPDATE_REFS) {
     - 		if (bol != eol)
     - 			return error(_("%s does not accept arguments: '%s'"),
     - 				     command_to_string(item->command), bol);
     + 	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
     +-	    item->command == TODO_RESET) {
     ++	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
     + 		item->commit = NULL;
     + 		item->arg_offset = bol - buf;
     + 		item->arg_len = (int)(eol - bol);
      @@ sequencer.c: leave_merge:
       	return ret;
       }
       
     -+static int do_update_refs(struct repository *r)
     ++static int do_update_ref(struct repository *r, const char *ref_name)
      +{
      +	return 0;
      +}
     @@ sequencer.c: static int pick_commits(struct repository *r,
       				return error_with_patch(r, item->commit,
       							arg, item->arg_len,
       							opts, res, 0);
     -+		} else if (item->command == TODO_UPDATE_REFS) {
     -+			if ((res = do_update_refs(r)))
     ++		} else if (item->command == TODO_UPDATE_REF) {
     ++			struct strbuf ref = STRBUF_INIT;
     ++			strbuf_add(&ref, arg, item->arg_len);
     ++			if ((res = do_update_ref(r, ref.buf)))
      +				reschedule = 1;
     ++			strbuf_release(&ref);
       		} else if (!is_noop(item->command))
       			return error(_("unknown command %d"), item->command);
       
     @@ sequencer.h: enum todo_command {
       	TODO_LABEL,
       	TODO_RESET,
       	TODO_MERGE,
     -+	TODO_UPDATE_REFS,
     ++	TODO_UPDATE_REF,
       	/* commands that do nothing but are counted for reporting progress */
       	TODO_NOOP,
       	TODO_DROP,
 5:  0a4c110127b ! 6:  918b398d6a2 rebase: add --update-refs option
     @@ Commit message
          reachable from those "sub branches". It can take a manual step to update
          those branches.
      
     -    Add a new --update-refs option to 'git rebase -i' that adds 'label
     -    for-update-refs/*' steps to the todo file whenever a commit that is
     -    being rebased is decorated with that <ref>. At the very end, the
     -    'update-refs' step is added to update all of the branches referenced by
     -    the 'label' steps. This allows the user to rebase a long list of commits
     -    in a multi-part feature and keep all of their pointers to those parts.
     +    Add a new --update-refs option to 'git rebase -i' that adds 'update-ref
     +    <ref>' steps to the todo file whenever a commit that is being rebased is
     +    decorated with that <ref>. At the very end, the rebase process updates
     +    all of the listed refs to the values stored during the rebase operation.
      
     -    NOTE: This change only introduce the --update-refs option and implements
     -    the changes to the todo file. It does _not_ yet implement the action
     -    taken by the 'update-refs' todo step, which will be implemented and
     -    tested in a later change.
     -
     -    Use the new for_each_decoration() while iterating over the existing todo
     -    list. Be sure to iterate after any squashing or fixups are placed.
     -    Update the branch only after those squashes and fixups are complete.
     -    This allows a --fixup commit at the tip of the feature to apply
     -    correctly to the sub branch, even if it is fixing up the most-recent
     -    commit in that part.
     +    Be sure to iterate after any squashing or fixups are placed. Update the
     +    branch only after those squashes and fixups are complete. This allows a
     +    --fixup commit at the tip of the feature to apply correctly to the sub
     +    branch, even if it is fixing up the most-recent commit in that part.
      
          One potential problem here is that refs decorating commits that are
          already marked as "fixup!" or "squash!" will not be included in this
     @@ Commit message
          of the rebased branch while the other sub branches come along for the
          ride without intervention.
      
     -    Be careful to not attempt updating any branch that is checked out. The
     -    most common example is the branch being rebased is checked out and
     -    decorates the tip commit. If the user is rebasing commits reachable from
     -    a different branch that is checked out in a different worktree, then
     -    they may be surprised to not see that ref update. However, it's probably
     -    best to not optimize for this scenario and do the safest thing that will
     -    result in a successful rebase. A comment is left in the TODO list that
     -    signals that these refs are currently checked out.
     +    This change update the documentation and builtin to accept the
     +    --update-refs option as well as updating the todo file with the
     +    'update-ref' commands. Tests are added to ensure that these todo
     +    commands are added in the correct locations.
     +
     +    A future change will update the behavior to actually update the refs
     +    at the end of the rebase sequence.
      
          Signed-off-by: Derrick Stolee <[email protected]>
      
     @@ sequencer.c
       #include "rebase-interactive.h"
       #include "reset.h"
      +#include "branch.h"
     -+#include "log-tree.h"
       
       #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
       
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
      +	size_t items_alloc;
      +	struct strbuf *buf;
      +	struct commit *commit;
     ++	struct string_list refs_to_oids;
      +};
      +
     -+static int add_branch_for_decoration(const struct name_decoration *d, void *data)
     ++static int add_decorations_to_list(const struct commit *commit,
     ++				   struct todo_add_branch_context *ctx)
      +{
     -+	struct todo_add_branch_context *ctx = data;
     -+	size_t base_offset = ctx->buf->len;
     -+	struct todo_item *item;
     -+	char *path;
     ++	const struct name_decoration *decoration = get_name_decoration(&commit->object);
      +
     -+	ALLOC_GROW(ctx->items,
     -+		   ctx->items_nr + 1,
     -+		   ctx->items_alloc);
     -+	item = &ctx->items[ctx->items_nr];
     -+	memset(item, 0, sizeof(*item));
     ++	while (decoration) {
     ++		struct todo_item *item;
     ++		const char *path;
     ++		size_t base_offset = ctx->buf->len;
      +
     -+	/* If the branch is checked out, then leave a comment instead. */
     -+	if (branch_checked_out(d->name, &path)) {
     -+		item->command = TODO_COMMENT;
     -+		strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
     -+			    d->name, path);
     -+		free(path);
     -+	} else {
     -+		item->command = TODO_LABEL;
     -+		strbuf_addf(ctx->buf, "for-update-refs/%s\n", d->name);
     -+	}
     ++		ALLOC_GROW(ctx->items,
     ++			ctx->items_nr + 1,
     ++			ctx->items_alloc);
     ++		item = &ctx->items[ctx->items_nr];
     ++		memset(item, 0, sizeof(*item));
     ++
     ++		/* If the branch is checked out, then leave a comment instead. */
     ++		if ((path = branch_checked_out(decoration->name))) {
     ++			item->command = TODO_COMMENT;
     ++			strbuf_addf(ctx->buf, "# Ref %s checked out at '%s'\n",
     ++				    decoration->name, path);
     ++		} else {
     ++			struct string_list_item *sti;
     ++			item->command = TODO_UPDATE_REF;
     ++			strbuf_addf(ctx->buf, "%s\n", decoration->name);
     ++
     ++			sti = string_list_append(&ctx->refs_to_oids,
     ++						 decoration->name);
     ++			sti->util = oiddup(the_hash_algo->null_oid);
     ++		}
     ++
     ++		item->offset_in_buf = base_offset;
     ++		item->arg_offset = base_offset;
     ++		item->arg_len = ctx->buf->len - base_offset;
     ++		ctx->items_nr++;
      +
     -+	item->offset_in_buf = base_offset;
     -+	item->arg_offset = base_offset;
     -+	item->arg_len = ctx->buf->len - base_offset;
     -+	ctx->items_nr++;
     ++		decoration = decoration->next;
     ++	}
      +
      +	return 0;
      +}
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
      +	struct decoration_filter decoration_filter = {
      +		.include_ref_pattern = &decorate_refs_include,
      +		.exclude_ref_pattern = &decorate_refs_exclude,
     -+		.exclude_ref_config_pattern = &decorate_refs_exclude_config
     ++		.exclude_ref_config_pattern = &decorate_refs_exclude_config,
      +	};
      +	struct todo_add_branch_context ctx = {
      +		.buf = &todo_list->buf,
     ++		.refs_to_oids = STRING_LIST_INIT_DUP,
      +	};
      +
      +	ctx.items_alloc = 2 * todo_list->nr + 1;
     @@ sequencer.c: static int skip_unnecessary_picks(struct repository *r,
      +
      +		if (item->commit) {
      +			ctx.commit = item->commit;
     -+			for_each_decoration(item->commit,
     -+					    add_branch_for_decoration,
     -+					    &ctx);
     ++			add_decorations_to_list(item->commit, &ctx);
      +		}
      +	}
      +
     -+	/* Add the "update-refs" step. */
     -+	ALLOC_GROW(ctx.items,
     -+		   ctx.items_nr + 1,
     -+		   ctx.items_alloc);
     -+	memset(&ctx.items[ctx.items_nr], 0, sizeof(struct todo_item));
     -+	ctx.items[ctx.items_nr].command = TODO_UPDATE_REFS;
     -+	ctx.items_nr++;
     -+
     ++	string_list_clear(&ctx.refs_to_oids, 1);
      +	free(todo_list->items);
      +	todo_list->items = ctx.items;
      +	todo_list->nr = ctx.items_nr;
     @@ sequencer.h: int complete_action(struct repository *r, struct replay_opts *opts,
       int todo_list_rearrange_squash(struct todo_list *todo_list);
       
      
     + ## t/t2407-worktree-heads.sh ##
     +@@ t/t2407-worktree-heads.sh: test_expect_success 'refuse to overwrite when in error states' '
     + 	done
     + '
     + 
     ++. "$TEST_DIRECTORY"/lib-rebase.sh
     ++
     ++test_expect_success !SANITIZE_LEAK 'refuse to overwrite during rebase with --update-refs' '
     ++	git commit --fixup HEAD~2 --allow-empty &&
     ++	(
     ++		set_cat_todo_editor &&
     ++		test_must_fail git rebase -i --update-refs HEAD~3 >todo &&
     ++		! grep "update-refs" todo
     ++	) &&
     ++	git branch -f allow-update HEAD~2 &&
     ++	(
     ++		set_cat_todo_editor &&
     ++		test_must_fail git rebase -i --update-refs HEAD~3 >todo &&
     ++		grep "update-ref refs/heads/allow-update" todo
     ++	)
     ++'
     ++
     + test_done
     +
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success 'ORIG_HEAD is updated correctly' '
       	test_cmp_rev ORIG_HEAD test-orig-head@{1}
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'ORIG_HEAD is updated correct
      +
      +		cat >expect <<-EOF &&
      +		pick $(git log -1 --format=%h J) J
     -+		label for-update-refs/refs/heads/second
     -+		label for-update-refs/refs/heads/first
     ++		update-ref refs/heads/second
     ++		update-ref refs/heads/first
      +		pick $(git log -1 --format=%h K) K
      +		pick $(git log -1 --format=%h L) L
      +		fixup $(git log -1 --format=%h update-refs) fixup! L # empty
     -+		label for-update-refs/refs/heads/third
     ++		update-ref refs/heads/third
      +		pick $(git log -1 --format=%h M) M
     -+		label for-update-refs/refs/heads/no-conflict-branch
     -+		label for-update-refs/refs/heads/shared-tip
     -+		update-refs
     ++		update-ref refs/heads/no-conflict-branch
     ++		update-ref refs/heads/shared-tip
      +		EOF
      +
      +		test_must_fail git rebase -i --autosquash --update-refs primary >todo &&
     @@ t/t3404-rebase-interactive.sh: test_expect_success 'ORIG_HEAD is updated correct
      +		reset onto
      +		pick $(git log -1 --format=%h branch2~1) F
      +		pick $(git log -1 --format=%h branch2) I
     -+		label for-update-refs/refs/heads/branch2
     ++		update-ref refs/heads/branch2
      +		label merge
      +		reset onto
      +		pick $(git log -1 --format=%h refs/heads/second) J
     -+		label for-update-refs/refs/heads/second
     -+		label for-update-refs/refs/heads/first
     ++		update-ref refs/heads/second
     ++		update-ref refs/heads/first
      +		pick $(git log -1 --format=%h refs/heads/third~1) K
      +		pick $(git log -1 --format=%h refs/heads/third) L
      +		fixup $(git log -1 --format=%h update-refs-with-merge) fixup! L # empty
     -+		label for-update-refs/refs/heads/third
     ++		update-ref refs/heads/third
      +		pick $(git log -1 --format=%h HEAD~2) M
     -+		label for-update-refs/refs/heads/no-conflict-branch
     ++		update-ref refs/heads/no-conflict-branch
      +		merge -C $(git log -1 --format=%h HEAD~1) merge # merge
     -+		label for-update-refs/refs/heads/merge-branch
     -+		update-refs
     ++		update-ref refs/heads/merge-branch
      +		EOF
      +
      +		test_must_fail git rebase -i --autosquash \
 6:  68f8e51b19c ! 7:  72e0481b643 sequencer: implement 'update-refs' command
     @@ Metadata
      Author: Derrick Stolee <[email protected]>
      
       ## Commit message ##
     -    sequencer: implement 'update-refs' command
     +    rebase: update refs from 'update-ref' commands
      
     -    The previous change allowed 'git rebase --update-refs' to create 'label'
     -    commands for each branch  among the commits being rewritten and add an
     -    'update-refs' command at the end of the todo list. Now, teach Git to
     -    update the refs during that final 'update-refs' command.
     +    The previous change introduced the 'git rebase --update-refs' option
     +    which added 'update-ref <ref>' commands to the todo list of an
     +    interactive rebase.
      
     -    We need to create an array of new and old OIDs for each ref by iterating
     -    over the refs/rewritten/for-update-refs/ namespace. We cannot update the
     -    refs in-place since this will confuse the refs iterator.
     +    Teach Git to record the HEAD position when reaching these 'update-ref'
     +    commands. The ref/OID pair is stored in the
     +    $GIT_DIR/rebase-merge/update-refs file. A previous change parsed this
     +    file to avoid having other processes updating the refs in that file
     +    while the rebase is in progress.
     +
     +    Not only do we update the file when the sequencer reaches these
     +    'update-ref' commands, we then update the refs themselves at the end of
     +    the rebase sequence. If the rebase is aborted before this final step,
     +    then the refs are not updated.
      
          Signed-off-by: Derrick Stolee <[email protected]>
      
       ## sequencer.c ##
     +@@
     + #include "rebase-interactive.h"
     + #include "reset.h"
     + #include "branch.h"
     ++#include "log-tree.h"
     + 
     + #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
     + 
     +@@ sequencer.c: 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.
     ++ */
     ++static GIT_PATH_FUNC(rebase_path_update_refs, "rebase-merge/update-refs")
     ++
     + /*
     +  * The following files are written by git-rebase just after parsing the
     +  * command-line.
      @@ sequencer.c: leave_merge:
       	return ret;
       }
       
     --static int do_update_refs(struct repository *r)
     -+struct update_refs_context {
     -+	struct ref_store *refs;
     -+	char **ref_names;
     -+	struct object_id *old;
     -+	struct object_id *new;
     -+	size_t nr;
     -+	size_t alloc;
     -+};
     -+
     -+static int add_ref_to_context(const char *refname,
     -+			      const struct object_id *oid,
     -+			      int flags,
     -+			      void *data)
     +-static int do_update_ref(struct repository *r, const char *ref_name)
     ++static int write_update_refs_state(struct string_list *refs_to_oids)
     ++{
     ++	int result = 0;
     ++	FILE *fp = NULL;
     ++	struct string_list_item *item;
     ++	char *path = xstrdup(rebase_path_update_refs());
     ++
     ++	if (safe_create_leading_directories(path)) {
     ++		result = error(_("unable to create leading directories of %s"),
     ++			       path);
     ++		goto cleanup;
     ++	}
     ++
     ++	fp = fopen(path, "w");
     ++	if (!fp) {
     ++		result = error_errno(_("could not open '%s' for writing"), path);
     ++		goto cleanup;
     ++	}
     ++
     ++	for_each_string_list_item(item, refs_to_oids)
     ++		fprintf(fp, "%s\n%s\n", item->string, oid_to_hex(item->util));
     ++
     ++cleanup:
     ++	if (fp)
     ++		fclose(fp);
     ++	return result;
     ++}
     ++
     ++static int do_update_ref(struct repository *r, const char *refname)
       {
     -+	int f = 0;
     -+	const char *name;
     -+	struct update_refs_context *ctx = data;
     ++	struct string_list_item *item;
     ++	struct string_list list = STRING_LIST_INIT_DUP;
     ++	int found = 0;
     ++
     ++	sequencer_get_update_refs_state(r->gitdir, &list);
      +
     -+	ALLOC_GROW(ctx->ref_names, ctx->nr + 1, ctx->alloc);
     -+	ALLOC_GROW(ctx->old, ctx->nr + 1, ctx->alloc);
     -+	ALLOC_GROW(ctx->new, ctx->nr + 1, ctx->alloc);
     ++	for_each_string_list_item(item, &list) {
     ++		if (!strcmp(item->string, refname)) {
     ++			struct object_id oid;
     ++			free(item->util);
     ++			found = 1;
      +
     -+	if (!skip_prefix(refname, "refs/rewritten/for-update-refs/", &name))
     -+		return 1;
     ++			if (!read_ref("HEAD", &oid)) {
     ++				item->util = oiddup(&oid);
     ++				break;
     ++			}
     ++		}
     ++	}
      +
     -+	ctx->ref_names[ctx->nr] = xstrdup(name);
     -+	oidcpy(&ctx->new[ctx->nr], oid);
     -+	if (!refs_resolve_ref_unsafe(ctx->refs, name, 0,
     -+				     &ctx->old[ctx->nr], &f))
     -+		return 1;
     ++	if (!found) {
     ++		struct object_id oid;
     ++		item = string_list_append(&list, refname);
      +
     -+	ctx->nr++;
     ++		if (!read_ref("HEAD", &oid))
     ++			item->util = oiddup(&oid);
     ++		else
     ++			item->util = oiddup(the_hash_algo->null_oid);
     ++	}
     ++
     ++	write_update_refs_state(&list);
     ++	string_list_clear(&list, 1);
       	return 0;
       }
       
      +static int do_update_refs(struct repository *r)
      +{
     -+	int i, res;
     -+	struct update_refs_context ctx = {
     -+		.refs = get_main_ref_store(r),
     -+		.alloc = 16,
     -+	};
     -+	ALLOC_ARRAY(ctx.ref_names, ctx.alloc);
     -+	ALLOC_ARRAY(ctx.old, ctx.alloc);
     -+	ALLOC_ARRAY(ctx.new, ctx.alloc);
     -+
     -+	res = refs_for_each_fullref_in(ctx.refs,
     -+				       "refs/rewritten/for-update-refs/",
     -+				       add_ref_to_context,
     -+				       &ctx);
     -+
     -+	for (i = 0; !res && i < ctx.nr; i++)
     -+		res = refs_update_ref(ctx.refs, "rewritten during rebase",
     -+				ctx.ref_names[i],
     -+				&ctx.new[i], &ctx.old[i],
     ++	int res = 0;
     ++	struct string_list_item *item;
     ++	struct string_list refs_to_oids = STRING_LIST_INIT_DUP;
     ++	struct ref_store *refs = get_main_ref_store(r);
     ++
     ++	sequencer_get_update_refs_state(r->gitdir, &refs_to_oids);
     ++
     ++	for_each_string_list_item(item, &refs_to_oids) {
     ++		struct object_id *oid_to = item->util;
     ++		struct object_id oid_from;
     ++
     ++		if (oideq(oid_to, the_hash_algo->null_oid)) {
     ++			/*
     ++			 * Ref was not updated. User may have deleted the
     ++			 * 'update-ref' step.
     ++			 */
     ++			continue;
     ++		}
     ++
     ++		if (read_ref(item->string, &oid_from)) {
     ++			/*
     ++			 * The ref does not exist. The user probably
     ++			 * inserted a new 'update-ref' step with a new
     ++			 * branch name.
     ++			 */
     ++			oidcpy(&oid_from, the_hash_algo->null_oid);
     ++		}
     ++
     ++		res |= refs_update_ref(refs, "rewritten during rebase",
     ++				item->string,
     ++				oid_to, &oid_from,
      +				0, UPDATE_REFS_MSG_ON_ERR);
     ++	}
      +
     -+	for (i = 0; i < ctx.nr; i++)
     -+		free(ctx.ref_names[i]);
     -+	free(ctx.ref_names);
     -+	free(ctx.old);
     -+	free(ctx.new);
     ++	string_list_clear(&refs_to_oids, 1);
      +	return res;
      +}
      +
       static int is_final_fixup(struct todo_list *todo_list)
       {
       	int i = todo_list->current;
     +@@ sequencer.c: cleanup_head_ref:
     + 		strbuf_release(&head_ref);
     + 	}
     + 
     ++	do_update_refs(r);
     ++
     + 	/*
     + 	 * Sequence of picks finished successfully; cleanup by
     + 	 * removing the .git/sequencer directory
     +@@ sequencer.c: static int todo_list_add_update_ref_commands(struct todo_list *todo_list)
     + 		}
     + 	}
     + 
     ++	write_update_refs_state(&ctx.refs_to_oids);
     ++
     + 	string_list_clear(&ctx.refs_to_oids, 1);
     + 	free(todo_list->items);
     + 	todo_list->items = ctx.items;
      
       ## t/t3404-rebase-interactive.sh ##
      @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs adds commands with --rebase-merges' '
     @@ t/t3404-rebase-interactive.sh: test_expect_success '--update-refs adds commands
      +	git branch -f third HEAD~1 &&
      +	test_commit extra2 fileX &&
      +	git commit --amend --fixup=L &&
     -+	(
     -+		git rebase -i --autosquash --update-refs primary &&
     -+
     -+		compare_two_refs HEAD~3 refs/heads/first &&
     -+		compare_two_refs HEAD~3 refs/heads/second &&
     -+		compare_two_refs HEAD~1 refs/heads/third &&
     -+		compare_two_refs HEAD refs/heads/no-conflict-branch
     -+	)
     ++
     ++	git rebase -i --autosquash --update-refs primary &&
     ++
     ++	compare_two_refs HEAD~3 refs/heads/first &&
     ++	compare_two_refs HEAD~3 refs/heads/second &&
     ++	compare_two_refs HEAD~1 refs/heads/third &&
     ++	compare_two_refs HEAD refs/heads/no-conflict-branch
      +'
      +
       # This must be the last test in this file

7: 3d7d3f656b4 ! 8: d2cfdbfc431 rebase: add rebase.updateRefs config option
@@ Documentation/git-rebase.txt: start would be overridden by the presence of
or point to a squash! ... or fixup! ... commit are not updated in this way.
++
-+If the --update-refs option is enabled by default using the -+configuration variable rebase.updateRefs, this option can be -+used to override and disable this setting.
++If the configuration variable rebase.updateRefs is set, then this option ++can be used to override and disable this setting.

INCOMPATIBLE OPTIONS
--------------------

gitgitgadget

From: Derrick Stolee <[email protected]>

The tests in t2407 that verify the branch_checked_out() helper in the case of bisects and rebases were added by 9347303db89 (branch: check for bisects and rebases, 2022-06-08). However, that commit failed to check for rebases that are using the 'apply' backend.

Add a test that checks the apply backend. The implementation was already correct here, but it is good to have regression tests before modifying the implementation further.

---
 t/t2407-worktree-heads.sh | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

index b6be42f74a2..4f59bc21303 100755
@@ -54,7 +54,18 @@ test_expect_success 'refuse to overwrite: worktree in bisect' '
 	grep "cannot force update the branch '\''fake-2'\'' checked out at.*wt-4" err
 '
 
-test_expect_success 'refuse to overwrite: worktree in rebase' '
+test_expect_success 'refuse to overwrite: worktree in rebase (apply)' '
+	test_when_finished rm -rf .git/worktrees/wt-*/rebase-apply &&
+
+	mkdir -p .git/worktrees/wt-3/rebase-apply &&
+	echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-apply/head-name &&
+	echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-apply/onto &&
+
+	test_must_fail git branch -f fake-1 HEAD 2>err &&
+	grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
+'
+
+test_expect_success 'refuse to overwrite: worktree in rebase (merge)' '
 	test_when_finished rm -rf .git/worktrees/wt-*/rebase-merge &&
 
 	mkdir -p .git/worktrees/wt-3/rebase-merge &&

gitgitgadget

This is true that it is based on how it's currently implemented. I was confused that none of these rebase storage mechanisms are documented anywhere (so I couldn't extend that documentation with the new update-refs file added in this series). But perhaps that's by
design: this is intentionally an internal implementation detail.

I do wonder where that leaves third-party implementations like libgit2 to handle any changes we make in this space, or how much we care about supporting someone rebasing in a worktree with an older Git client while also checking out a branch in another worktree using a newer one.

This, and the equivalent "pause in the middle of a bisect" would be ways to hide these internal details.

While I was thinking that this provides a low-weight mechanism for testing the implementation of branch_checked_out(), it is only testing the implementation but not the _intention_. If 'git rebase' changed its backend, then these tests would not start failing as we need them to.

I can either re-roll that series or create a new forward-fix that includes the functionality of this test. Both are the same amount of work for me, so let me know which you prefer.

Thanks,
-Stolee

Either is fine by me. Other than this small glitch in the test, the remainder of the "should we allow this branch to be touched?" topic looked really cleanly done and ready to be unleashed to the public.

Thanks.

If you don't mind, I've got some forward-fixes prepped for the next version of this topic. They are test-only changes and do not change the implementation of branch_checked_out().

One thing I noticed from looking back on the series is that I wrote a test that specifically tests a case that is impossible to construct without modifying the .git directory directly (having two worktrees "reserving" a branch for different reasons). That test will need to stay as one that knows about the internals of this storage, but the others can be replaced with more opaque steps.

In patch 2 of this series, I test the 'update-refs' storage using a similar mechanism. I can replace that with the opaque version that calls 'git rebase --update-refs', but only as part of patch 7, since writing the file is not implemented yet.

I was going to ask if I should reorder the patches, but there is a possibility that we will change the storage of these "reserved" refs yet again, so I won't focus on this point quite yet.

Thanks,
-Stolee

From: Derrick Stolee <[email protected]>

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.

---
 branch.c                  | 13 ++++++++++++
 sequencer.c               | 42 +++++++++++++++++++++++++++++++++++++++
 sequencer.h               |  9 +++++++++
 t/t2407-worktree-heads.sh | 22 ++++++++++++++++----
 4 files changed, 82 insertions(+), 4 deletions(-)

index 526e8237673..f252c4eef6c 100644
@@ -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);

index da64473636b..3ae541bb145 100644
@@ -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 */

index 4f59bc21303..f1e9e172a0c 100755
@@ -7,8 +7,11 @@ TEST_PASSES_SANITIZE_LEAK=true
 
 test_expect_success 'setup' '
 	test_commit init &&
-	git branch -f fake-1 &&
-	git branch -f fake-2 &&
+
+	for i in 1 2 3 4
+	do
+		git branch -f fake-$i || return 1
+	done &&
 
 	for i in 1 2 3 4
 	do

@@ -73,8 +76,19 @@ test_expect_success 'refuse to overwrite: worktree in rebase (merge)' '
 	echo refs/heads/fake-1 >.git/worktrees/wt-3/rebase-merge/head-name &&
 	echo refs/heads/fake-2 >.git/worktrees/wt-3/rebase-merge/onto &&
 
-	test_must_fail git branch -f fake-1 HEAD 2>err &&
-	grep "cannot force update the branch '\''fake-1'\'' checked out at.*wt-3" err
+	cat >.git/worktrees/wt-3/rebase-merge/update-refs <<-EOF &&
+	refs/heads/fake-3
+	$(git rev-parse HEAD~1)
+	refs/heads/fake-4
+	$(git rev-parse HEAD)
+	EOF
+
+	for i in 1 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' '

gitgitgadget

Hmph, I am not thrilled to see an ad-hoc text file for things like this. Do the objects that appear in this file otherwise already anchored by existing refs, or can some of them be "floating" without any refs pointing at them, i.e. making the "connected" and "fsck" machinery also being aware of them to protect them from collected as garbage and reported as dangling/unreachable?

You're right that we could have this sequence of events in a todo file:

pick deadbeef Here is a commit that will become unreachable update-ref will-be-lost

squash 1234567 make will-be-lost unreachable
...

So if a GC runs after this 'update-ref' step but before the rebase completes, then that commit can be lost. The ref-update at the end of the rebase process would fail.

I wonder how important such a situation is, though. But I'm willing to add the extra lookups in 'git gc' and 'git fsck' to make this a non-issue.

I'll take a look to see where the other refs in rebase-<backend>/* are handled by these processes.

Thanks,
-Stolee

The commit is protected by HEAD's reflog for a while because we update HEAD after each pick but it is not permanently safe.

I've taken a look at the first few patches and they're looking good, I hope to look at the rest tomorrow

Best Wishes

Phillip

Ah, I forgot about that one. Thanks for pointing it out. It does sound like HEAD's reflog makes it safe enough.

What I was hinting at was if some of this can be done by using a ref, so that we do not have to touch "gc" or "fsck" at all.

As to the importance, I would say it is about the same importance as being able to prevent the branches involved in the rebase from touched in other worktrees. We expect it to take sufficiently large wallclock time from the beginning of a rebase and its finish to make such a protection necessary, so an auto-gc may even be started from other worktrees without our knowledge.

Thanks.

This looks good apart from a couple of small nits

The biggest difference is that rebase_path_update_refs() only gives the path for the current worktree, while this method needs to read the file for any worktree.

There is likely some opportunity to create rebase_path_update_refs() in a different way that serves both purposes.

Not sure what I was looking at that gave this pattern, but you're right that it isn't necessary.

Thanks,
-Stolee

Hi Stolee

That's an important distinction that I'd failed to notice

That would be nice, even just having a #define for "rebase-merge/update-refs" and using that here and in the other patch would mean we're not hard coding the filename in two different places.

Best Wishes

Phillip

From: Derrick Stolee <[email protected]>

The 'merge' command description for the todo list documentation in an interactive rebase has multiple lines. The lines other than the first one start with dots ('.') while the similar multi-line documentation for 'fixup' does not.

The 'merge' command was documented when interactive rebase was first ported to C in 145e05ac44b (rebase -i: rewrite append_todo_help() in C, 2018-08-10). These dots might have been carried over from the previous shell implementation.

The 'fixup' command was documented more recently in 9e3cebd97cb (rebase -i: add fixup [-C | -c] command, 2021-01-29).

Looking at the output in an editor, my personal opinion is that the dots are unnecessary and noisy. Remove them now before adding more commands with multi-line documentation.

---
 rebase-interactive.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

index 87649d0c016..22394224faa 100644
@@ -54,9 +54,9 @@ void append_todo_help(int command_count,
 "l, label <label> = label current HEAD with a name\n"
 "t, reset <label> = reset HEAD to a label\n"
 "m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]\n"
-".       create a merge commit using the original merge commit's\n"
-".       message (or the oneline, if no original merge commit was\n"
-".       specified); use -c <commit> to reword the commit message\n"
+"        create a merge commit using the original merge commit's\n"
+"        message (or the oneline, if no original merge commit was\n"
+"        specified); use -c <commit> to reword the commit message\n"
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 	unsigned edit_todo = !(shortrevisions && shortonto);

gitgitgadget

The text indeed does come literally from the block removed by that commit. I wondered if the shell "gettext" in git-i18n.sh had some magic in it, but it does not seem to have anything, and po/git.pot around that time has these lines with leading dots, so I suspect that they were quite deliberately added, not for the reasons of formatting machinery (e.g. preventing somebody in the dataflow from losing leading indentation), but to show them to the end users.

Unfortunately, the offending commit 4c68e7dd (sequencer: introduce the merge command, 2018-04-25) does not justify them X-<.

I personally do not mind having them in the UI, but I can also be happy to see them go. It is unlikely that any program is consuming these strings, so I would say this is a fairly safe clean-up.

Perhaps I should be a bit clearer that these are appearing in the comment section of the todo-file when presented to the user for editing:

# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message # e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit # f, fixup [-C | -c] <commit> = like "squash" but keep only the previous

#                    commit's log message, unless -C is used, in which case
#                    keep only this commit's message; -c is same as -C but
#                    opens the editor

# x, exec <command> = run command (the rest of the line) using shell # b, break = stop here (continue rebase later with 'git rebase --continue') # d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]

# .       create a merge commit using the original merge commit's
# .       message (or the oneline, if no original merge commit was
# .       specified); use -c <commit> to reword the commit message
#
# These lines can be re-ordered; they are executed from top to bottom.
#

This does not appear to be used anywhere else.

Thanks,
-Stolee

Yup, it is good to spell it out for others.

Nowhere in our codebase. I do not think we have a way to know if users (e.g. hooks and scripts) depend on them, but I am reasonably sure that nobody would complain ;-)

I agree, this is a nice cleanup

Thanks

Phillip

From: Derrick Stolee <[email protected]>

The todo_command_info array defines which strings match with which todo_command enum values. The array is defined in the same order as the enum values, but if one changed without the other, then we would have unexpected results.

Make it easier to see changes to the enum and this array by using the enum values as the indices of the array.

---
 sequencer.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

index 1094e146b99..2ffee79619c 100644
@@ -1685,20 +1685,20 @@ static struct {
 	char c;
 	const char *str;
 } todo_command_info[] = {
-	{ 'p', "pick" },
-	{ 0,   "revert" },
-	{ 'e', "edit" },
-	{ 'r', "reword" },
-	{ 'f', "fixup" },
-	{ 's', "squash" },
-	{ 'x', "exec" },
-	{ 'b', "break" },
-	{ 'l', "label" },
-	{ 't', "reset" },
-	{ 'm', "merge" },
-	{ 0,   "noop" },
-	{ 'd', "drop" },
-	{ 0,   NULL }
+	[TODO_PICK] = { 'p', "pick" },
+	[TODO_REVERT] = { 0,   "revert" },
+	[TODO_EDIT] = { 'e', "edit" },
+	[TODO_REWORD] = { 'r', "reword" },
+	[TODO_FIXUP] = { 'f', "fixup" },
+	[TODO_SQUASH] = { 's', "squash" },
+	[TODO_EXEC] = { 'x', "exec" },
+	[TODO_BREAK] = { 'b', "break" },
+	[TODO_LABEL] = { 'l', "label" },
+	[TODO_RESET] = { 't', "reset" },
+	[TODO_MERGE] = { 'm', "merge" },
+	[TODO_NOOP] = { 0,   "noop" },
+	[TODO_DROP] = { 'd', "drop" },
+	[TODO_COMMENT] = { 0,   NULL },
 };
 
 static const char *command_to_string(const enum todo_command command)

gitgitgadget

OK. It is a bit of shame that we cannot define this array and enum at the same time in a single construct, but this is much safer than the status quo. Thanks.

From: Derrick Stolee <[email protected]>

Add the boilerplate for an "update-ref" command in the sequencer. This connects to the current no-op do_update_ref() which will be filled in after more connections are created.

The syntax in the todo list will be "update-ref <ref-name>" to signal that we should store the current commit as the value for updating <ref-name> at the end of the rebase.

---
 rebase-interactive.c |  3 +++
 sequencer.c          | 14 +++++++++++++-
 sequencer.h          |  1 +
 3 files changed, 17 insertions(+), 1 deletion(-)

index 22394224faa..1ff07647af3 100644
@@ -57,6 +57,9 @@ void append_todo_help(int command_count,
 "        create a merge commit using the original merge commit's\n"
 "        message (or the oneline, if no original merge commit was\n"
 "        specified); use -c <commit> to reword the commit message\n"
+"u, update-ref <ref> = track a placeholder for the <ref> to be updated\n"
+"                      to this position in the new commits. The <ref> is\n"
+"                      updated at the end of the rebase\n"
 "\n"
 "These lines can be re-ordered; they are executed from top to bottom.\n");
 	unsigned edit_todo = !(shortrevisions && shortonto);

index 2ffee79619c..0b61835d441 100644
@@ -1696,6 +1696,7 @@ static struct {
 	[TODO_LABEL] = { 'l', "label" },
 	[TODO_RESET] = { 't', "reset" },
 	[TODO_MERGE] = { 'm', "merge" },
+	[TODO_UPDATE_REF] = { 'u', "update-ref" },
 	[TODO_NOOP] = { 0,   "noop" },
 	[TODO_DROP] = { 'd', "drop" },
 	[TODO_COMMENT] = { 0,   NULL },

@@ -2457,7 +2458,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
 			     command_to_string(item->command));
 
 	if (item->command == TODO_EXEC || item->command == TODO_LABEL ||
-	    item->command == TODO_RESET) {
+	    item->command == TODO_RESET || item->command == TODO_UPDATE_REF) {
 		item->commit = NULL;
 		item->arg_offset = bol - buf;
 		item->arg_len = (int)(eol - bol);

@@ -4056,6 +4057,11 @@ leave_merge:
 	return ret;
 }
 
+static int do_update_ref(struct repository *r, const char *ref_name)
+{
+	return 0;
+}
+
 static int is_final_fixup(struct todo_list *todo_list)
 {
 	int i = todo_list->current;

@@ -4431,6 +4437,12 @@ static int pick_commits(struct repository *r,
 				return error_with_patch(r, item->commit,
 							arg, item->arg_len,
 							opts, res, 0);
+		} else if (item->command == TODO_UPDATE_REF) {
+			struct strbuf ref = STRBUF_INIT;
+			strbuf_add(&ref, arg, item->arg_len);
+			if ((res = do_update_ref(r, ref.buf)))
+				reschedule = 1;
+			strbuf_release(&ref);
 		} else if (!is_noop(item->command))
 			return error(_("unknown command %d"), item->command);
 

index 3ae541bb145..2cf5c1b9a38 100644
@@ -95,6 +95,7 @@ enum todo_command {
 	TODO_LABEL,
 	TODO_RESET,
 	TODO_MERGE,
+	TODO_UPDATE_REF,
 	/* commands that do nothing but are counted for reporting progress */
 	TODO_NOOP,
 	TODO_DROP,

gitgitgadget

From: Derrick Stolee <[email protected]>

When working on a large feature, it can be helpful to break that feature into multiple smaller parts that become reviewed in sequence. During development or during review, a change to one part of the feature could affect multiple of these parts. An interactive rebase can help adjust the multi-part "story" of the branch.

However, if there are branches tracking the different parts of the feature, then rebasing the entire list of commits can create commits not reachable from those "sub branches". It can take a manual step to update those branches.

Add a new --update-refs option to 'git rebase -i' that adds 'update-ref <ref>' steps to the todo file whenever a commit that is being rebased is decorated with that <ref>. At the very end, the rebase process updates all of the listed refs to the values stored during the rebase operation.

Be sure to iterate after any squashing or fixups are placed. Update the branch only after those squashes and fixups are complete. This allows a --fixup commit at the tip of the feature to apply correctly to the sub branch, even if it is fixing up the most-recent commit in that part.

One potential problem here is that refs decorating commits that are already marked as "fixup!" or "squash!" will not be included in this list. Generally, the reordering of the "fixup!" and "squash!" is likely to change the relative order of these refs, so it is not recommended. The workflow here is intended to allow these kinds of commits at the tip of the rebased branch while the other sub branches come along for the ride without intervention.

This change update the documentation and builtin to accept the --update-refs option as well as updating the todo file with the 'update-ref' commands. Tests are added to ensure that these todo commands are added in the correct locations.

A future change will update the behavior to actually update the refs at the end of the rebase sequence.

---
 Documentation/git-rebase.txt  |   8 +++
 builtin/rebase.c              |   5 ++
 sequencer.c                   | 107 ++++++++++++++++++++++++++++++++++
 sequencer.h                   |   1 +
 t/t2407-worktree-heads.sh     |  17 ++++++
 t/t3404-rebase-interactive.sh |  70 ++++++++++++++++++++++
 6 files changed, 208 insertions(+)

index 262fb01aec0..e7611b4089c 100644
@@ -609,6 +609,13 @@ provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
 start would be overridden by the presence of
 `rebase.rescheduleFailedExec=true` configuration.
 
+--update-refs::
+--no-update-refs::
+	Automatically force-update any branches that point to commits that
+	are being rebased. Any branches that are checked out in a worktree
+	or point to a `squash! ...` or `fixup! ...` commit are not updated
+	in this way.
+
 INCOMPATIBLE OPTIONS
 --------------------
 

@@ -632,6 +639,7 @@ are incompatible with the following options:
  * --empty=
  * --reapply-cherry-picks
  * --edit-todo
+ * --update-refs
  * --root when used in combination with --onto
 
 In addition, the following pairs of options are incompatible:

index 7ab50cda2ad..56d82a52106 100644
@@ -102,6 +102,7 @@ struct rebase_options {
 	int reschedule_failed_exec;
 	int reapply_cherry_picks;
 	int fork_point;
+	int update_refs;
 };
 
 #define REBASE_OPTIONS_INIT {			  	\

@@ -298,6 +299,7 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags)
 		ret = complete_action(the_repository, &replay, flags,
 			shortrevisions, opts->onto_name, opts->onto,
 			&opts->orig_head, &commands, opts->autosquash,
+			opts->update_refs,
 			&todo_list);
 	}
 

@@ -1124,6 +1126,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		OPT_BOOL(0, "autosquash", &options.autosquash,
 			 N_("move commits that begin with "
 			    "squash!/fixup! under -i")),
+		OPT_BOOL(0, "update-refs", &options.update_refs,
+			 N_("update local refs that point to commits "
+			    "that are being rebased")),
 		{ OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"),
 			N_("GPG-sign commits"),
 			PARSE_OPT_OPTARG, NULL, (intptr_t) "" },

index 2cf5c1b9a38..e671d7e0743 100644
@@ -167,6 +167,7 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla
 		    const char *shortrevisions, const char *onto_name,
 		    struct commit *onto, const struct object_id *orig_head,
 		    struct string_list *commands, unsigned autosquash,
+		    unsigned update_refs,
 		    struct todo_list *todo_list);
 int todo_list_rearrange_squash(struct todo_list *todo_list);
 

index f1e9e172a0c..203997d92c6 100755
@@ -151,4 +151,21 @@ test_expect_success 'refuse to overwrite when in error states' '
 	done
 '
 
+. "$TEST_DIRECTORY"/lib-rebase.sh
+
+test_expect_success !SANITIZE_LEAK 'refuse to overwrite during rebase with --update-refs' '
+	git commit --fixup HEAD~2 --allow-empty &&
+	(
+		set_cat_todo_editor &&
+		test_must_fail git rebase -i --update-refs HEAD~3 >todo &&
+		! grep "update-refs" todo
+	) &&
+	git branch -f allow-update HEAD~2 &&
+	(
+		set_cat_todo_editor &&
+		test_must_fail git rebase -i --update-refs HEAD~3 >todo &&
+		grep "update-ref refs/heads/allow-update" todo
+	)
+'
+
 test_done

gitgitgadget

OK, so command line option is --update-refs because the range being rebased could contain more than one branch tips, and the option causes one update-ref insn per such ref left in the todo list? I had to read the above twice to remind me what was going on.

The design looks OK here but has "rebase-merge/update-refs" already came into the picture, or will it be in future steps? I lost track.

Let's read on.

Thanks.

...

Sorry, we have the ability to read the file (and tests that read an artificial version of the file for the branch_checked_out() logic) but have not written it from within Git yet.

I can make it clearer what is _not_ in this patch and coming in a future change.

Thanks,
-Stolee

This looks quite good to me, my main concern is that we don't check what the user did when they edited the todo list which leaves a potentially long window when another process could update a branch that is being rebased. I've left some comments below, they're mostly me talking to myself while reading the code.