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

[Johannes Schindelin]

Hi,

nix.wie.weg wrote:

> 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.

It was more polite. That is why you may have missed it.

http://lists.gnu.org/archive/html/qemu-devel/2006-04/msg00456.html

> > 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.

You really try to mock me? "I have not heard anyone complaining." They
haven't had time to test it! If I find obvious errors on first sight of such
a huge patch, it is no question there are more subtle bugs hidden away.

Let me tell you a story from my day-job life. There was a man who deemed
himself a programmer. One day he decided (without any budget or hint of the
project leader to do so) to rip out a whole database query and reimplement
that in pure C++ code. He argued that the database would be too slow to
handle that. When we proved to him (I had to take out a pencil and a sheet
of paper!) that all of a sudden, the application went from 32MB memory usage
to 512MB. That and other points (the obvious being that Oracle's DBMS is
*designed* to handle such a task much more gracefully) were such a big
problem, even the suits got the point. What was his answer? "You have to
switch to Microsoft Terminal Services, then."

What is the point of this story? If you make a mistake, admit it and correct
it. Don't whine, and certainly do not suggest more work and money and
hassles to others. It will only make you look like a fool.

> 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.

Wrong. Now the maintainer has to do it. Which is you fault, not his.

> > 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.

grep your huge patch for usb_device_add and notice all the "-" in front,
then repeat with usbtree_device_add, and notice all the "+".

Also kindly look into line 1417 and 1419 of your patch. You will find the
renaming o f product_name to prod_name.

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.

I read a lot of Fabrice's code. I am not the only one. Fabrice writes
clear structured code which does not need many comments.

I will not quote the rest of the paragraph which is just nasty towards
Fabrice and the other authors. It's preposterous.

> > 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.

Apples and nuclear submarines?

I track QEmu CVS with GIT, and have no such problems as you described.
To the contrary, the commit history cannot contain holes, whereas
everybody can forget a "chagelog" line (it only gets bad if somebody
commits a *huge* patch with inadequate message). And BTW, I quoted the
"chagelog", because it has a typo. It also has a typo in the
year, backdating your changes one year. It also has a semantic typo, in
that the changes were not done on one single day, the 20th of April.

There are so many errors of form that I *expect* the changes to contain
errors, too.

> > 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.

You know what? That is one of the reasons, why small patches are Best
Practice. You could not possibly have missed that if you had a series of
small patches. Or if you read that patch even once.

> > 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. :)

If I did not know it better, I would think you tried to piss me off there.
You want me to spend as much time to understand what is going on as you did?
Then what does your work try to achieve?

Let me drive the point home one more time (in case you missed it). Sending a
huge patch is wrong, for a plethora of reasons. Expecting others to swallow
it is preposterous. Instead of trying to hide it (which might work in a
closed source world, where you can hide behind a suit, but which fails
utterly in Open Source, where everybody can see what you did), admit the
mistake, work a little more, clean up your patch into a nice patch series,
and make everybody happy.

Hth,
Dscho


-- 
"Feel free" - 10 GB Mailbox, 100 FreeSMS/Monat ...
Jetzt GMX TopMail testen: http://www.gmx.net/de/go/topmail