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

From: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: gitster@pobox.com, johannes.schindelin@gmx.de, me@ttaylorr.com,
	Jeff Hostetler <git@jeffhostetler.com>,
	Phillip Wood <phillip.wood123@gmail.com>,
	Elijah Newren <newren@gmail.com>,
	Derrick Stolee <derrickstolee@github.com>
Subject: [PATCH v2 0/7] rebase: update branches in multi-part topic
Date: Tue, 07 Jun 2022 20:42:42 +0000	[thread overview]
Message-ID: <pull.1247.v2.git.1654634569.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1247.git.1654263472.gitgitgadget@gmail.com>

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