Re: [PATCH 1/7] rebase --apply: remove duplicated code
- From
Elijah Newren <newren@gmail.com>
- Date
- Apr 17, 2022, 01:56 UTC
- Message-ID
- <CABPp-BF2p54rcR9xLyZ34-HBPDw6ip08=0rGs3uH=qqSsn0cRA@mail.gmail.com>
- In-Reply-To
- <CAP8UFD25KjNYuMoVb72N2kGmo3yJ0qGgfJn78axc562Z+YhAGQ@mail.gmail.com>
On Thu, Apr 7, 2022 at 1:30 PM Christian Couder <christian.couder@gmail.com> wrote:
Show 25 quoted lines
>
> 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.
Show 36 quoted lines
> > 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.