Re: bzr commit into mysql-6.0-backup branch (ritheesh.vedire:2888) Bug#45587

[Charles Bell]

STATUS
------
Approved pending changes.

Note: When you see this it means anything in the 'REQUIRED' section is 
assumed to be acted on without varying from the edict given before the 
patch is pushed. If you have objections or cannot meet any such 
requirements, you should contact the reviewer to discuss a compromise 
(but in this case it's an easy fix that I do not need to see again).

REQUIRED
--------
1. You must supply a problem statement and a summary of what the patch 
is doing in the patch comments.

2. You must provide meaningful per-file comments as well.

COMMENTARY
----------
Thank you for putting in the comment concerning the change. That eases 
my worries completely.

Many people in the development process use the patch comments to 
determine what is going on with the changes. We must provide commentary 
in the patches to let folks know what is changing and why. We do not 
have to write a thesis mind you, just the facts.

It is worth taking a moment and executing this statement on one of your 
source trees:

bzr log --limit=20 --longallmsg

Take a look at the output and notice how (99%) of the patches (that 
aren't merges or auto merges) contain not only a short description of 
the change being made but also how each file is being changed.

Out of curiosity, are you using a QT-based commit dialog or something 
like 'bzr commit -m'? The dialogs I use on all three platforms make it 
very easy to add comments and you can see at a glance not only what you 
have written for each file but also how the changes look. Of course, it 
would be better if it had a line position so you'd know how long your 
lines are but I guess that's asking for too much. :)

DETAILS
-------
>  2888 Ritheesh Vedire	2009-11-06
>       BUG#45587: Backup of tables using a non-existent storage engine does not give
> warning/error

[1] You must state the problem and how this patch fixes the problem in 
this section. Do not leave patch comments blank.

>      @ mysql-test/suite/backup/r/backup_no_engine.result
>         T

[2] What? Please fill out all file comments correctly. In this case, we 
normally say something like, "Corrected result file."

>      @ mysql-test/suite/backup/t/backup_no_engine.test
>         Test case suppresses errors in the Server Log file.

[2] Do you mean you fixed the test case or what? This statement doesn't 
say you did anything. I would have written, "Changed test to suppress 
errors." But I think you did more than that because you changed the code 
to throw an error.

[2] No comments for si_object.cc? There should be. You should state what 
you have changed in the code (briefly). For example, I would have 
written, "Changed the SELECT statement <in method> to use an alternative 
form to fix <problem>." Hint: fill in the blanks for <>. ;)

Another hint: It would be really good to see a statement about the bug 
referenced in the code in either the file or patch comments.

Chuck