Re: [PATCH 11/14] replay: use standard revision ranges

From: Elijah Newren <newren@gmail.com>
Date: Apr 18, 2023, 04:58 UTC
Message-ID: <CABPp-BHCdjOutYqdMO1NbYKNA0BgkXRgwUEKK=MX0kXM-5G_DQ@mail.gmail.com>
In-Reply-To: <89c78da5-388a-e52b-b20b-e376ac90de14@github.com>

On Fri, Apr 14, 2023 at 7:23 AM Derrick Stolee <derrickstolee@github.com> wrote:

Show 18 quoted lines

>
> (I was able to get a segfault by rebasing this series with
> --author=stolee because the commit list became empty. Something
> to watch for.)
>
> > Something like:
> >
> >       'git replay [options] <base> <tip>'
> >       This mode means to rebase <tip> onto <base>,
> >       detecting the range of commits to rewrite.
> >
> >       'git replay [options] <new-base> <old-base> <tip>'
> >       This mode means to rebase the range <old-base>..<tip>
> >       onto <new-base>.
>
> For that reason, I think we should be using explicit argument
> parsing in the builtin and only transform arguments we
> understand into the setup_revisions() (by building a strvec).

So, it turns out that this suggested solution wouldn't have helped prevent the segfault you found. If someone merely passed <old-base> == <tip>, they'd see the same segfault. In fact, I think you found a latent bug in merge-ort. In particular, cmd_replay() has

    struct merge_options merge_opt;
    struct merge_result result;

    init_merge_options(&merge_opt, the_repository);
    memset(&result, 0, sizeof(result));

    <do N merges, for some value of N>

    merge_finalize(&merge_opt, &result);

This code segfaults if N is 0, because merge_finalize() doesn't return early when result->priv is NULL.

We never triggered this before, because this is the very first code example we've had where someone has tried to call merge_finalize() without first performing a merge. Anyway, nice find and thanks for reporting!