Re: [PATCH 1/3] rebase -r: do create merge commit after empty resolution

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 28, 2025, 17:23 UTC
Message-ID: <CAPig+cThwsBdumXB3m2ZA-_tmDVTMojkYx7_YxNp49eK6a2HMg@mail.gmail.com>
In-Reply-To: <CAPig+cS92W_gYuNsaTvQxiP3xBK7Wpg0__uVkgAU1x0OFJUZgQ@mail.gmail.com>

On Fri, Mar 28, 2025 at 1:14 PM Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 17 quoted lines

> On Fri, Mar 28, 2025 at 1:03 PM Philippe Blain via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
> > +test_expect_success '--continue creates merge commit after empty resolution' '
> > +       [...]
> > +       git commit --no-edit &&
> > +       FAKE_LINES="1 2 3 5 6 7 8 9 10 11" &&
> > +       export FAKE_LINES &&
> > +       test_must_fail git rebase -ir main &&
>
> I don't think you want to be setting FAKE_LINES like this since doing
> so will pollute the environment for all tests following this one. You
> can find existing precedent in this script which demonstrates the
> correct way to handle this case. Specifically, you'd want:
>
>     test_must_fail env FAKE_LINES="1 2 3 5 6 7 8 9 10 11" \
>         git rebase -ir main &&

To clarify, by "pollute", I mean that it can impact subsequent tests which don't take care to override FAKE_LINES as necessary. There certainly are test scripts which use the:

    FAKE_LINES=... &&
    export FAKE_LINES &&

form successfully, but such scripts are careful to override/set FAKE_LINES in every test. This particular script (t3418), on the other hand, does not otherwise employ the form in which the variable is exported, so introducing it in a test which is inserted into the middle of the script feels dangerous.