Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Feb 22, 2023, 10:16 UTC
Message-ID: <36090abb-72a0-f9b7-5a41-ddd8ccde92bd@dunelm.org.uk>
In-Reply-To: <CAMMLpeSMO38acvnxK2QX18CdoaEiUojwLsV+AQZYOTe5h4Nq=Q@mail.gmail.com>

Hi Alex

On 22/02/2023 01:37, Alex Henrie wrote:

Show 15 quoted lines

> On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 21/02/2023 05:58, Alex Henrie wrote:
>>> +test_expect_success 'do not rebase merges unless asked to' '
>>> +     git checkout -b rebase-merges-default E &&
>>> +     before="$(git rev-parse --verify HEAD)" &&
>>> +     test_tick &&
>>> +     git rebase --rebase-merges C &&
>>
>> I don't quite follow what this part of the test is for
> 
> The test is modeled after the existing test "do not rebase cousins
> unless asked for". First, it verifies that --rebase-merges rebases the
> merges, which in this case results in no changes to the branch. Then,
> it verifies that `git rebase` without arguments flattens the history.

I think "do not rebase cousins unless asked for" is a bit different because it is checking the default for --rebase-merges which seems reasonable. I cannot see the point of checking that --rebase-merges works in this test as we have a whole file of tests that already do that.

Best Wishes

Phillip

Show 26 quoted lines

>>> +test_expect_success '--no-rebase-merges countermands --rebase-merges' '
>>> +     git checkout -b no-rebase-merges E &&
>>> +     git rebase --rebase-merges --no-rebase-merges C &&
>>> +     test_cmp_graph C.. <<-\EOF
>>> +     * B
>>> +     * D
>>> +     o C
>>> +     EOF
>>> +'
>>
>> This test looks good. I think we could probably have just this second
>> test
> 
> I like having a test for the default behavior. Nevertheless, I am
> happy to remove that test as requested. Does anyone else have an
> opinion?
> 
>> and squash this into the previous patch - improving the
>> documentation and test coverage for --no-rebase-merges would make a nice
>> self-contained commit.
> 
> Sure, squashing the first two patches is no problem.
> 
> Thanks for the feedback,
> 
> -Alex