Re: [Qemu-devel] [PATCH v2 07/45] tests: remove alt num-int cases

[Markus Armbruster <armbru@redhat.com>]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> From: Markus Armbruster <armbru@redhat.com>
>
> Marc-André Lureau <marcandre.lureau@redhat.com> writes:
>
>> There are no real users of this case, and it's going to be invalid after
>> merging of QFloat and QInt use the same QNum type in the following patch.
>
> Invalid because our alternate code is insufficiently sophisticated.  In
> other words fixable.  See "[PATCH 4/4] qapi: Reject alternates that
> can't work with keyval_parse()" I just posted.
>
> My patch's commit message describes two related issues, one fixable and
> one unfixable.  The fixable one already exists, but only in QGA.  I
> intend to fix it, but it's not a priority.
>
> Fixing the issue you describe seems even less important.  I considered
> outlawing it in my series (patch appended for your reading pleasure),
> but decided to leave it to yours.  I don't expect you to replace your
> patch.  Perhaps my patch helps you with rebasing yours should that
> become necessary.
>
> Message-Id: <87tw4cn7sh.fsf@dusky.pond.sub.org>

Uh, this got a bit messed up :)

Your original patch only dropped test cases that will become invalid.

In my review, I elaborated a bit on "invalid", and mentioned that I had
considered outlawing such alternates independently of this series, but
refrained.  I appended a patch for reference, and in the hope that it
helps you rebasing your patch.

It looks like you replaced your patch by mine, and the commit message by
my review.  The resulting patch does more than just "tests: remove alt
num-int cases".  It also lacks your S-o-b.

I recommend to drop the change to qapi.py, and restore your original
commit message.  Should effectively be a rebase of your v1.

I further suggest a bit more detail in the commit message, perhaps like
this:

    tests: Remove test cases for alternates of 'number' and 'int'

    Alternates with both a 'number' and an 'int' branch will become
    invalid when the next patch merges of QFloat and QInt into QNum.
    More sophisticated alternate code could keep them valid, but since
    we have no users outside tests, simply drop the tests.

    Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>

You can then add
Reviewed-by: Markus Armbruster <armbru@redhat.com>