| Message ID | 243e7b0c372c15cc907c838ee74794433aa79858.1645441854.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: make reflog messages independent of the backend | expand |
On Tue, Feb 22, 2022 at 6:34 AM Phillip Wood via GitGitGadget <gitgitgadget@gmail.com> 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. Nit: maybe "rebase --apply: reuse move_to_original_branch()" would be a better subject for this patch, as it's more specific. > 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. It looks like the removed call is using strbuf_reset(), not strbuf_release(). Actually a call to strbuf_release(&msg) is also removed inside the `if (oideq(&merge_base, &options.orig_head)) { ... }` clause, but it doesn't look like you are talking about this one. > 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); Yeah, it looks like `msg` isn't used in the function anymore so this call can be removed. > 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; > } Nice cleanup!
On Thu, Apr 7, 2022 at 1:30 PM Christian Couder <christian.couder@gmail.com> wrote: > > On Tue, Feb 22, 2022 at 6:34 AM Phillip Wood via GitGitGadget > <gitgitgadget@gmail.com> 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. > > Nit: maybe "rebase --apply: reuse move_to_original_branch()" would be > a better subject for this patch, as it's more specific. > > > 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. > > It looks like the removed call is using strbuf_reset(), not strbuf_release(). > > Actually a call to strbuf_release(&msg) is also removed inside the `if > (oideq(&merge_base, &options.orig_head)) { ... }` clause, but it > doesn't look like you are talking about this one. I think he is talking about that one and specifying why it's okay to drop that one, and omitted why it was okay to drop the strbuf_reset() because that one seemed more obvious. > > 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); > > Yeah, it looks like `msg` isn't used in the function anymore so this > call can be removed. > > > 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; > > } > > Nice cleanup! Agreed.
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; }