On Thu, Apr 7, 2022 at 1:30 PM Christian Couder
<christian.couder@xxxxxxxxx> wrote:
>
> On Tue, Feb 22, 2022 at 6:34 AM Phillip Wood via GitGitGadget
> <gitgitgadget@xxxxxxxxx> wrote:
> >
> > From: Phillip Wood <phillip.wood@xxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxx>
> > ---
> > 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.
