| Message ID | a4320f2fcf35f180e1c585be4105194f1555a874.1650448612.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: make reflog messages independent of the backend | expand |
On Wed, Apr 20 2022, Phillip Wood via GitGitGadget wrote: > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When we are reattaching HEAD after a fast-forward we can use > move_to_original_branch() rather than open coding a copy of that > code. The old code appears to handle the case where the rebase is > started from a detached HEAD but in fact in that case there is nothing > to do as we have already updated HEAD. > > Note that the removal of "strbuf_release(&msg)" is safe as there is an > identical call just above this hunk which can be seen by viewing the > diff with -U6. Instead of assuring the reader that this is OK, perhaps just squash this in: diff --git a/builtin/rebase.c b/builtin/rebase.c index 4a664964c30..5108679e816 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1029,7 +1029,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) int ret, flags, total_argc, in_progress = 0; int keep_base = 0; int ok_to_skip_pre_rebase = 0; - struct strbuf msg = STRBUF_INIT; struct strbuf revisions = STRBUF_INIT; struct strbuf buf = STRBUF_INIT; struct object_id merge_base; @@ -1769,17 +1768,22 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) printf(_("First, rewinding head to replay your work on top of " "it...\n")); - strbuf_addf(&msg, "%s: checkout %s", - getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options.onto_name); - ropts.oid = &options.onto->object.oid; - ropts.orig_head = &options.orig_head, - ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD | + { + struct strbuf msg = STRBUF_INIT; + + strbuf_addf(&msg, "%s: checkout %s", + getenv(GIT_REFLOG_ACTION_ENVIRONMENT), + options.onto_name); + ropts.oid = &options.onto->object.oid; + ropts.orig_head = &options.orig_head, + ropts.flags = RESET_HEAD_DETACH | RESET_ORIG_HEAD | RESET_HEAD_RUN_POST_CHECKOUT_HOOK; - ropts.head_msg = msg.buf; - ropts.default_reflog_action = DEFAULT_REFLOG_ACTION; - if (reset_head(the_repository, &ropts)) - die(_("Could not detach HEAD")); - strbuf_release(&msg); + ropts.head_msg = msg.buf; + ropts.default_reflog_action = DEFAULT_REFLOG_ACTION; + if (reset_head(the_repository, &ropts)) + die(_("Could not detach HEAD")); + strbuf_release(&msg); + } /* * If the onto is a proper descendant of the tip of the branch, then That's a 2-line change (aside from the scope addition) if viewed with -w. I.e. before your change below we needed &msg in two places, now that we only need it in one it seems better just to move the variable to be scoped with its only user. And maybe it's getting too much into unrelated cleanup, but this change also leaves the loose end that the "ropts" variable no longer needs to be shared. You added it in 6ae8086161d (reset_head(): take struct rebase_head_opts, 2022-01-26) along whith the memset() removed here, but (on top of the proposed squash) we could also do this: diff --git a/builtin/rebase.c b/builtin/rebase.c index 5108679e816..8e2dc74c834 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1043,7 +1043,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) int reschedule_failed_exec = -1; int allow_preemptive_ff = 1; int preserve_merges_selected = 0; - struct reset_head_opts ropts = { 0 }; struct option builtin_rebase_options[] = { OPT_STRING(0, "onto", &options.onto_name, N_("revision"), @@ -1275,7 +1274,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } case ACTION_SKIP: { struct string_list merge_rr = STRING_LIST_INIT_DUP; - + struct reset_head_opts ropts = { 0 }; + options.action = "skip"; set_reflog_action(&options); @@ -1291,6 +1291,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) } case ACTION_ABORT: { struct string_list merge_rr = STRING_LIST_INIT_DUP; + struct reset_head_opts ropts = { 0 }; + options.action = "abort"; set_reflog_action(&options); @@ -1770,6 +1772,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) { struct strbuf msg = STRBUF_INIT; + struct reset_head_opts ropts = { 0 }; strbuf_addf(&msg, "%s: checkout %s", getenv(GIT_REFLOG_ACTION_ENVIRONMENT), I.e. the "ropts" earlier in the function wasn't shared with the code below, we'd "goto" past it... > > Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> > --- > builtin/rebase.c | 11 +---------- > 1 file changed, 1 insertion(+), 10 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index e942c300f8c..4832f16e675 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) > * If the onto is a proper descendant of the tip of the branch, then > * we just fast-forwarded. > */ > - strbuf_reset(&msg); > if (oideq(&merge_base, &options.orig_head)) { > printf(_("Fast-forwarded %s to %s.\n"), > branch_name, options.onto_name); > - strbuf_addf(&msg, "rebase finished: %s onto %s", > - options.head_name ? options.head_name : "detached HEAD", > - oid_to_hex(&options.onto->object.oid)); > - memset(&ropts, 0, sizeof(ropts)); > - ropts.branch = options.head_name; > - ropts.flags = RESET_HEAD_REFS_ONLY; > - ropts.head_msg = msg.buf; > - reset_head(the_repository, &ropts); > - strbuf_release(&msg); > + move_to_original_branch(&options); > ret = finish_rebase(&options); > goto cleanup; > }
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > - strbuf_reset(&msg); This is unnecessary, because we have released immediately before. > From: Phillip Wood <phillip.wood@dunelm.org.uk> > > When we are reattaching HEAD after a fast-forward we can use > move_to_original_branch() rather than open coding a copy of that > code. The old code appears to handle the case where the rebase is > started from a detached HEAD but in fact in that case there is nothing > to do as we have already updated HEAD. At the beginning of move_to_original_branch(), there is an early return to do nothing when the head is detached. But the code before the pre-context of the hunk already detached the head at the "onto", and the fast-forward case wants to leave the detached head pointing at that "onto" commit, so the early return without doing anything is exactly what we want to see. Does that mean that the original code had left two reflog entries for HEAD, one to detach at the "onto" commit, and then in this block to say "rebase finished" here? With the new code, because we return early from move_to_original_branch(), we no longer leave the "rebase finished" record in the reflog of HEAD? Is it done deliberately as an improvement, or an oversight that led to a possible regression? Or do we still add the reflog entry for HEAD that says "rebase finished" somewhere else when we trigger the early return and I am misreading the code? > > if (oideq(&merge_base, &options.orig_head)) { > printf(_("Fast-forwarded %s to %s.\n"), > branch_name, options.onto_name); > - strbuf_addf(&msg, "rebase finished: %s onto %s", > - options.head_name ? options.head_name : "detached HEAD", > - oid_to_hex(&options.onto->object.oid)); > - memset(&ropts, 0, sizeof(ropts)); > - ropts.branch = options.head_name; > - ropts.flags = RESET_HEAD_REFS_ONLY; > - ropts.head_msg = msg.buf; > - reset_head(the_repository, &ropts); > - strbuf_release(&msg); > + move_to_original_branch(&options); > ret = finish_rebase(&options); > goto cleanup; > }
Junio C Hamano <gitster@pobox.com> writes: > "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> - strbuf_reset(&msg); > > This is unnecessary, because we have released immediately before. Sorry. I meant "reset is unnecessary and it is the right thing to remove it here", but I just realized it can be misread to say "this change is unnecessary".
diff with -U6. Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk> --- builtin/rebase.c | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index e942c300f8c..4832f16e675 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1782,19 +1782,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) * If the onto is a proper descendant of the tip of the branch, then * we just fast-forwarded. */ - strbuf_reset(&msg); if (oideq(&merge_base, &options.orig_head)) { printf(_("Fast-forwarded %s to %s.\n"), branch_name, options.onto_name); - strbuf_addf(&msg, "rebase finished: %s onto %s", - options.head_name ? options.head_name : "detached HEAD", - oid_to_hex(&options.onto->object.oid)); - memset(&ropts, 0, sizeof(ropts)); - ropts.branch = options.head_name; - ropts.flags = RESET_HEAD_REFS_ONLY; - ropts.head_msg = msg.buf; - reset_head(the_repository, &ropts); - strbuf_release(&msg); + move_to_original_branch(&options); ret = finish_rebase(&options); goto cleanup; }
From: Phillip Wood <phillip.wood@dunelm.org.uk> When we are reattaching HEAD after a fast-forward we can use move_to_original_branch() rather than open coding a copy of that code. The old code appears to handle the case where the rebase is started from a detached HEAD but in fact in that case there is nothing to do as we have already updated HEAD. Note that the removal of "strbuf_release(&msg)" is safe as there is an identical call just above this hunk which can be seen by viewing the