Message ID | 3ec2cc922f971af4e4a558188cf139cc0c0150d6.1657631226.git.gitgitgadget@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | rebase: update branches in multi-part topic | expand |
On Tue, Jul 12, 2022 at 6:07 AM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Derrick Stolee <derrickstolee@github.com> > [...] > > +--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. I think the second sentence here should be split. In particular, I don't think I understand the second half of the second sentence. Do you intend to say here that branches pointing to a `squash!` or `fixup!` will instead update the first `pick` in the ancestry of such a commit, rather than that such branches are entirely excluded from any updates? That's what I observed in my testing of your v3, at least, and that's the behavior I'd expect this feature to implement, but this documentation doesn't match. [...] > @@ -5660,6 +5764,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla > item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0; > } > > + if (update_refs && todo_list_add_update_ref_commands(todo_list)) > + return -1; > + As a tangent, I find it interesting that you add the update-ref commands as a post-processing step rather than as a part of sequencer_make_script(). I don't think you need to change anything, but I am curious due to my git-replay work if you find it advantageous to do it this way.
On Tue, Jul 12, 2022 at 01:07:00PM +0000, Derrick Stolee via GitGitGadget wrote: > From: Derrick Stolee <derrickstolee@github.com> > > 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. > > This change does _not_ include the actual behavior of tracking the > updated refs and writing the new ref values at the end of the rebase > process. That is deferred to a later change. > > Signed-off-by: Derrick Stolee <derrickstolee@github.com> > --- > Documentation/git-rebase.txt | 8 +++ > builtin/rebase.c | 5 ++ > sequencer.c | 107 ++++++++++++++++++++++++++++++++++ > sequencer.h | 1 + > t/t2407-worktree-heads.sh | 22 +++++++ > t/t3404-rebase-interactive.sh | 70 ++++++++++++++++++++++ > 6 files changed, 213 insertions(+) > > diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt > index 262fb01aec0..e7611b4089c 100644 > --- a/Documentation/git-rebase.txt > +++ b/Documentation/git-rebase.txt > @@ -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:: So the option is called '--update-refs', but ... > +--no-update-refs:: > + Automatically force-update any branches that point to commits that ... its description talks about "branches". > + 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 > -------------------- > > @@ -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 " And its short help talks about "local refs". I think at least the documentation and short help should use consistent terminology. > + "that are being rebased")), > { OPTION_STRING, 'S', "gpg-sign", &gpg_sign, N_("key-id"), > N_("GPG-sign commits"), > PARSE_OPT_OPTARG, NULL, (intptr_t) "" },
On 7/18/2022 5:05 AM, SZEDER Gábor wrote: > On Tue, Jul 12, 2022 at 01:07:00PM +0000, Derrick Stolee via GitGitGadget wrote: >> From: Derrick Stolee <derrickstolee@github.com> >> +--update-refs:: > > So the option is called '--update-refs', but ... > >> +--no-update-refs:: >> + Automatically force-update any branches that point to commits that > > ... its description talks about "branches". >> + OPT_BOOL(0, "update-refs", &options.update_refs, >> + N_("update local refs that point to commits " > > And its short help talks about "local refs". > > I think at least the documentation and short help should use > consistent terminology. Thanks for catching this. I think I should use "branches" here, but keep the name "--update-refs". The biggest reason is that it provides a nice parallel with the "update-ref" sequencer command. This command allows updating _any_ ref, such as lightweight tags in refs/tags/* or even refs in refs/my/namespace/*. The --update-refs option doesn't create the commands to update tags or refs in places other than refs/heads/*. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: > ... I think I should use "branches" here, but > keep the name "--update-refs". The biggest reason is that it provides > a nice parallel with the "update-ref" sequencer command. This command > allows updating _any_ ref, such as lightweight tags in refs/tags/* > or even refs in refs/my/namespace/*. > > The --update-refs option doesn't create the commands to update tags > or refs in places other than refs/heads/*. I guess it would make the choice of "branch" the most appropriate. I was hoping that we can repoint refs in private namespaces that are not branches with the option. But as long as the underlying "update-ref" instruction can be used by advanced users, that is OK.
On 7/16/2022 3:30 PM, Elijah Newren wrote: > On Tue, Jul 12, 2022 at 6:07 AM Derrick Stolee via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> From: Derrick Stolee <derrickstolee@github.com> >> > [...] >> >> +--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. > > I think the second sentence here should be split. In particular, I > don't think I understand the second half of the second sentence. Do > you intend to say here that branches pointing to a `squash!` or > `fixup!` will instead update the first `pick` in the ancestry of such > a commit, rather than that such branches are entirely excluded from > any updates? That's what I observed in my testing of your v3, at > least, and that's the behavior I'd expect this feature to implement, > but this documentation doesn't match. Good find. You're right that these don't match, and its in fact that I expected it to work this way, but it doesn't. I've added a branch to my tests that points to a fixup! that is not the tip commit and use it to demonstrate that it is not reordered, but _does_ appear in the 'update-ref <ref>' list. I'll update the documentation to match this behavior, too. This case is unlikely to happen much in practice, but I now believe it's better to include the ref than to ignore it. > [...] >> @@ -5660,6 +5764,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla >> item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0; >> } >> >> + if (update_refs && todo_list_add_update_ref_commands(todo_list)) >> + return -1; >> + > > As a tangent, I find it interesting that you add the update-ref > commands as a post-processing step rather than as a part of > sequencer_make_script(). I don't think you need to change anything, > but I am curious due to my git-replay work if you find it advantageous > to do it this way. I found it to be simple enough to do a scan of the steps directly instead of injecting extra logic into the _make_script() method. The simplest reason is that we would need to inject "update_refs" into that method. Thanks, -Stolee
On 7/18/2022 3:35 PM, Junio C Hamano wrote: > Derrick Stolee <derrickstolee@github.com> writes: > >> ... I think I should use "branches" here, but >> keep the name "--update-refs". The biggest reason is that it provides >> a nice parallel with the "update-ref" sequencer command. This command >> allows updating _any_ ref, such as lightweight tags in refs/tags/* >> or even refs in refs/my/namespace/*. >> >> The --update-refs option doesn't create the commands to update tags >> or refs in places other than refs/heads/*. > > I guess it would make the choice of "branch" the most appropriate. > > I was hoping that we can repoint refs in private namespaces that are > not branches with the option. But as long as the underlying > "update-ref" instruction can be used by advanced users, that is OK. I would like to keep the --update-refs name for a couple reasons: 1. 'update-ref' is the right name for the sequencer command. Having a parallel there is helpful for learning about the option. 2. We could extend the boolean '--update-refs' option into a more advanced multi-valued '--update-refs=<refspec>' option to allow advanced users to specify a ref namespace that they want included. Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: > On 7/18/2022 3:35 PM, Junio C Hamano wrote: >> Derrick Stolee <derrickstolee@github.com> writes: >> >>> ... I think I should use "branches" here, but >>> keep the name "--update-refs". The biggest reason is that it provides >>> a nice parallel with the "update-ref" sequencer command. This command >>> allows updating _any_ ref, such as lightweight tags in refs/tags/* >>> or even refs in refs/my/namespace/*. >>> >>> The --update-refs option doesn't create the commands to update tags >>> or refs in places other than refs/heads/*. >> >> I guess it would make the choice of "branch" the most appropriate. >> >> I was hoping that we can repoint refs in private namespaces that are >> not branches with the option. But as long as the underlying >> "update-ref" instruction can be used by advanced users, that is OK. > > I would like to keep the --update-refs name for a couple reasons: I do not think anybody proposed to change the name of that option. I was reacting to your "I should use branches here", with the understanding that "here" is this place where you used "local refs". >> + OPT_BOOL(0, "update-refs", &options.update_refs, >> + N_("update local refs that point to commits " If "rebase --update-refs" uses "update-ref" insn (which is capable of repointing non-branch refs) only for local branches, then the help text for the "--update-refs" option can safely say "update local branches" without being inaccurate. That is where my "branch is the most appropriate" comes from.
On 7/19/2022 12:44 PM, Junio C Hamano wrote: > Derrick Stolee <derrickstolee@github.com> writes: > >> On 7/18/2022 3:35 PM, Junio C Hamano wrote: >>> Derrick Stolee <derrickstolee@github.com> writes: >>> >>>> ... I think I should use "branches" here, but >>>> keep the name "--update-refs". The biggest reason is that it provides >>>> a nice parallel with the "update-ref" sequencer command. This command >>>> allows updating _any_ ref, such as lightweight tags in refs/tags/* >>>> or even refs in refs/my/namespace/*. >>>> >>>> The --update-refs option doesn't create the commands to update tags >>>> or refs in places other than refs/heads/*. >>> >>> I guess it would make the choice of "branch" the most appropriate. >>> >>> I was hoping that we can repoint refs in private namespaces that are >>> not branches with the option. But as long as the underlying >>> "update-ref" instruction can be used by advanced users, that is OK. >> >> I would like to keep the --update-refs name for a couple reasons: > > I do not think anybody proposed to change the name of that option. > > I was reacting to your "I should use branches here", with the > understanding that "here" is this place where you used "local refs". > >>> + OPT_BOOL(0, "update-refs", &options.update_refs, >>> + N_("update local refs that point to commits " > > If "rebase --update-refs" uses "update-ref" insn (which is capable > of repointing non-branch refs) only for local branches, then the > help text for the "--update-refs" option can safely say "update > local branches" without being inaccurate. That is where my "branch > is the most appropriate" comes from. Thanks for the clarification. Sorry for misunderstanding. Thanks, -Stolee
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt index 262fb01aec0..e7611b4089c 100644 --- a/Documentation/git-rebase.txt +++ b/Documentation/git-rebase.txt @@ -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: diff --git a/builtin/rebase.c b/builtin/rebase.c index 7ab50cda2ad..56d82a52106 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -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) "" }, diff --git a/sequencer.c b/sequencer.c index 1d6442c9639..e657862cda2 100644 --- a/sequencer.c +++ b/sequencer.c @@ -35,6 +35,7 @@ #include "commit-reach.h" #include "rebase-interactive.h" #include "reset.h" +#include "branch.h" #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION" @@ -5638,10 +5639,113 @@ static int skip_unnecessary_picks(struct repository *r, return 0; } +struct todo_add_branch_context { + struct todo_item *items; + size_t items_nr; + size_t items_alloc; + struct strbuf *buf; + struct commit *commit; + struct string_list refs_to_oids; +}; + +static int add_decorations_to_list(const struct commit *commit, + struct todo_add_branch_context *ctx) +{ + const struct name_decoration *decoration = get_name_decoration(&commit->object); + + while (decoration) { + struct todo_item *item; + const char *path; + size_t base_offset = ctx->buf->len; + + 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_insert(&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++; + + decoration = decoration->next; + } + + return 0; +} + +/* + * For each 'pick' command, find out if the commit has a decoration in + * refs/heads/. If so, then add a 'label for-update-refs/' command. + */ +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; + static struct string_list decorate_refs_exclude_config = STRING_LIST_INIT_NODUP; + static struct string_list decorate_refs_include = STRING_LIST_INIT_NODUP; + 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, + }; + 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; + 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]; + + /* insert ith item into new list */ + ALLOC_GROW(ctx.items, + ctx.items_nr + 1, + ctx.items_alloc); + + ctx.items[ctx.items_nr++] = todo_list->items[i++]; + + if (item->commit) { + ctx.commit = item->commit; + add_decorations_to_list(item->commit, &ctx); + } + } + + string_list_clear(&ctx.refs_to_oids, 1); + free(todo_list->items); + todo_list->items = ctx.items; + todo_list->nr = ctx.items_nr; + todo_list->alloc = ctx.items_alloc; + + return 0; +} + int complete_action(struct repository *r, struct replay_opts *opts, unsigned flags, 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) { char shortonto[GIT_MAX_HEXSZ + 1]; @@ -5660,6 +5764,9 @@ int complete_action(struct repository *r, struct replay_opts *opts, unsigned fla item->arg_len = item->arg_offset = item->flags = item->offset_in_buf = 0; } + if (update_refs && todo_list_add_update_ref_commands(todo_list)) + return -1; + if (autosquash && todo_list_rearrange_squash(todo_list)) return -1; diff --git a/sequencer.h b/sequencer.h index 2cf5c1b9a38..e671d7e0743 100644 --- a/sequencer.h +++ b/sequencer.h @@ -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); diff --git a/t/t2407-worktree-heads.sh b/t/t2407-worktree-heads.sh index 97f5c87f8c8..8a03f14df8d 100755 --- a/t/t2407-worktree-heads.sh +++ b/t/t2407-worktree-heads.sh @@ -164,4 +164,26 @@ 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 + ) +' + +# This must be the last test in this file +test_expect_success '$EDITOR and friends are unchanged' ' + test_editor_unchanged +' + test_done diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f31afd4a547..3cd20733bc8 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1743,6 +1743,76 @@ test_expect_success 'ORIG_HEAD is updated correctly' ' test_cmp_rev ORIG_HEAD test-orig-head@{1} ' +test_expect_success '--update-refs adds label and update-ref commands' ' + 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 && + git commit --allow-empty --fixup=third && + git branch -f shared-tip && + ( + set_cat_todo_editor && + + cat >expect <<-EOF && + pick $(git log -1 --format=%h J) J + 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 + update-ref refs/heads/third + pick $(git log -1 --format=%h M) M + 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 && + 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 + update-ref refs/heads/branch2 + label merge + reset onto + pick $(git log -1 --format=%h refs/heads/second) J + 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 + update-ref refs/heads/third + pick $(git log -1 --format=%h HEAD~2) M + update-ref refs/heads/no-conflict-branch + merge -C $(git log -1 --format=%h HEAD~1) merge # merge + update-ref refs/heads/merge-branch + EOF + + test_must_fail git rebase -i --autosquash \ + --rebase-merges=rebase-cousins \ + --update-refs primary >todo && + + test_cmp expect todo + ) +' + # This must be the last test in this file test_expect_success '$EDITOR and friends are unchanged' ' test_editor_unchanged