[virt-tools-list] [libosinfo] Fix osinfo_list_add_union()

[Zeeshan Ali (Khattak)]

On Wed, Jun 6, 2012 at 12:57 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
> On Tue, Jun 05, 2012 at 05:36:38PM +0300, Zeeshan Ali (Khattak) wrote:
>> On Tue, Jun 5, 2012 at 10:54 AM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> > On Mon, Jun 04, 2012 at 08:38:14PM +0300, Zeeshan Ali (Khattak) wrote:
>> >> On Mon, Jun 4, 2012 at 7:26 PM, Christophe Fergeau <cfergeau at redhat.com> wrote:
>> >> > On Mon, Jun 04, 2012 at 06:44:21PM +0300, Zeeshan Ali (Khattak) wrote:
>> >> >> From: "Zeeshan Ali (Khattak)" <zeeshanak at gnome.org>
>> >> >>
>> >> >> This function was adding the second list elements to new list first so
>> >> >> the order of elements in the new list was contrary to what user will
>> >> >> expect of this function (and all wrapper/using functions).
>> >> >
>> >> > This change makes sense to me, but since you looked at this code to fix a
>> >> > Boxes bug, you should explain what behaviour you were seeing without this
>> >> > patch, what this patch change, and why it's better now (or something in
>> >> > that spirit ;)
>> >>
>> >> If the change makes sense anyways, I don't see the need to waste time
>> >> on justifying it.
>> >
>> > The very fact that I needed to first guess and then explain where this
>> > patch is coming from in the paragraph below should be proof enough that
>> > explaining why you had to look into that code was required to achieve a
>> > proper review of said code.
>> > By doing that, you're not only helping the reviewers, but also your future
>> > self, if you find a bug in this code 6 months from now, having a log
>> > explaining why you made this change a long time ago can be a tremendous
>> > help to make sure you are not introducing regressions.
>> > That said, after the discussion below saying that it's not an actual fix
>> > for the bug you were seeing, this explanation is less needed in this patch,
>> > but will be useful in the patch with the true fix.
>>
>> So this patch can go in already?
>
> Well, it would be bad if this patch went in, and became a good enough work
> around for Boxes, and if the root cause never gets fixed...

I don't at all understand the logic of keeping a bug around even
though there is a fix for it just because it exposes another (bigger
bug). Especially if the other bug is hard to solve. The solution for
the human memory problem would to file a bug about it and/or write it
down somewhere.

So for this patch, all I need to know is if there is any problem with
it still to be addressed?

-- 
Regards,

Zeeshan Ali (Khattak)
FSF member#5124