Re: [Qemu-devel] Large USB-Patch Documentation and todays CVS patch

[andrzej zaborowski]

Hi,

On 27/04/06, address@hidden <address@hidden> wrote:
> Hello Johannes,
>
> Thanks for your comments.
>
> Johannes Schindelin wrote:
> > I am quite sure you put a lot of work into this patch, but you sure make it
> > hard to appreciate, too.
> >
> > First note that applying such a huge patch is bad. Let me help you (a little
> > more than last time)
> Sorry I dont know why, but I have no other message from you in my Inbox,
> so I can't tell what you have written last time.
Please check the www archive.
> > to understand that: You are almost guaranteed to
> > introduce bugs, and what's worse, regressions. Because it is so huge, you
> > are further guaranteed to have a real hard time tracking that regression.
> >
> I agree with you on that. But I have a reason for doing it and I have no
> one heard complaining, this or that has been broken, which worked
> before. On the other side, I can say for sure, that with that patch a
> lot more things do work. So even if something may be broken and cvs
> changes will with great probability break things (see softfloat-native.h
> today), it does not mean the patch is bad. The only question which has
> to be asked is, could it be done with smaller patches. And on that point
> I disagree with you. I claim that it could not be done.
You could see Fabrice Bellard doing it now -- dividing the huge patch
into small consistent commits, so it definitely is possible. And this
is not the maintainer's task, it is normally done by the person who
submits the changes before these changes can be considered for
integrating (at least in theory).
> > Also, you make it really, really awkward to pick the changes which are
> > unquestionable, and skip the questionable ones.
> >
> > For example, your patch "fixes" a few white spaces. This does not hurt, but
> > has no meaning either, and slows down patch reading.
> > Also, you rename some things when it has no apparent use to rename them,
> > such as usb_device_add, or product_name.
> >
> Sorry but on that point you say things which are in no case provable by
> any study. The contrary is true, good readable code, many comments, good
> structered function names, which follow a pattern, help to write good
> and error free code. When I got my hands on the usb code, there were as
> many as almost none comments. I needed more than one hour to even get a
> glue what is going on in usb.h (which is apparently not the reason for
> having header files). The usb code as I obtained it, was in a state
> which you can describe in the words "quick and dirty". If you find such
> conditions you have to change without mercy. If you don't do it and the
> person, which has applied the first changes to cvs, has done a quick and
> dirty job, you will always have a patch work software, without any
> structure and without any consistent interface.  So maybe that the
> person, who wants to commit the patch to cvs, has more work to do. But
> this persons work is absolutly nonrelevant in compare to the many people
> who will read the (then better) code, and understand it faster and
> deeper. I'm absolutly sure, that you will find examples where I have not
> choosen the best solution, but if I would look into any code which you
> have written the same would be valid and that is not a problem as long
> as the changes which are good outweigh the bad ones.
> > Parts of the patch are plainly unreadable, because you move code around. I
> > suppose you not only moved them around, but made subtle changes --
> > hard-to-notice-because-moved changes.
> >
> > As of now, no files in CVS that I am aware of contain "chagelog" lines.
> Until the 19th century it was not common that a doctor has washed his
> hands when he had evaluated a patient. This doesn't mean it is a bad
> idea to do so. My experiences with other projects have shown, that it is
> in most cases a good idea to have these changelog lines even if you use
> cvs. But if the majority of the people here is against them, I will take
> them out - that is not a problem. It is sometimes not so easy to find
> the person who had developed a portion of source code after lets say 5
> years. In the cvs you will normaly find only the person who has applied
> the patch and not the person who has developed the code. If you then
> want to contact the original developer it can be a hard job to find him.
Yes, that's one of the disadvantages of CVS which are very well dealt
with in GIT (don't know about other systems).
> So thats the reason for making these short changelog lines. Some
> opensource licenses (not the one for qemu) even require such changelog
> lines and I personaly believe, it is good to have them.
> > However, these reveal that you made changes as early as 20th April of 2005?
> > Plenty of time feeding small patches since then ;-)
> >
> > Continuing with comments: you change the comment "QEMU USB HUB emulation" in
> > usb-hub.c to "QEMU PC System Emulator" and backdate Fabrice's copyright from
> > 2005 to 2003-2004? Why?
> >
> Yes sorry about that, it was not my intention to do so. Let me explain
> how that was happening. As you may have noticed, Fabrice has added
> usb-hub.c earlier this week, after I had already submitted my patch. As
> I had updated my local cvs version this changes were rejected. And for
> one or the other reason I had not noticed, the difference in the
> comment. I haved changed it now.
> > In the patch, you made plenty of white-space changes which make it very
> > difficult to spot real changes.
> >
> > Again, I think you did some very valuable work. However, I'd be so much
> > happier if you split the patch up into meaningful patches, not a rollup
> > which is hard to understand *and* huge.
> > I now spent about one hour trying to understand your fixes, and I know
> > almost as much as when I started about your fixes.
> >
> >
> One hour is nothing I had to spend one day to understand even the data
> path between the different usb functions. :)
> > Hope this helps,
> > Dscho
> >
> I too hope this helps,
> Tino H. Seifert
>
>
> _______________________________________________
> Qemu-devel mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/qemu-devel
>


--
balrog 2oo6

Dear Outlook users: Please remove me from your address books
http://www.newsforge.com/article.pl?sid=03/08/21/143258