Re: bzr commit into mysql-6.0-backup branch (cbell:2648) Bug#35230

[Rafal Somla]

Hi Chuck,

Here is my reply for your general comments regarding style of work etc...

cbell@stripped wrote:
> Rafal,
> 
> I am disappointed that you regard the kernel with such high emotional 
> protectiveness. I understand most of your points you outlined but I 
> think to work effectively we must devoid ourselves from certain 
> prejudices regarding some types of changes to the code.
> 

I do treat backup kernel emotionally because I wrote this code - I think this is 
understandable. But I don't think I have any prejudices. As I explain below I 
think I had right to ask for the changes you introduced and that would be the 
case regardless of what part of the code you were changing. The only thing is 
that I know backup kernel better than other parts of the code and it is easier 
for me to spot problems there.

> I am ok with you suggesting changes to how parameters are passed and 
> such -- that's not what I am complaining about. What bothers me most is 
> that it seems any time I make a change to any part of the kernel it's 
> never good enough to pass your reviews and it takes a minimum of four 
> reviews to satisfy you. We all must simply learn to relax a little! :)
> 
> Since you feel so strongly about changing the parameter names, types, 
> etc., please feel free to make those changes -- just have Svoj or 
> someone review them first.

I don't feel strong against changing parameter names or types. I feel strong 
against changing them without a reason - this is different. I might not see or 
understand the reasons behind some changes and this is why I ask questions.

(cut)
>> Apart from that there are few changes which I don't think are needed
>> and thus are a bit annoying for an author of the code. These are things
>> like changing types of function parameters, renaming the parameters and
>> such. I'd like to see the reasons behind introducing such changes...
> 
> IMHO (and this is not an accusation, merely commentary): we as 
> developers in a team should never regard any portion of the code higher 
> than any other or have moving standards. Ownwership and authorship have 
> no place in a modern agile development (like) environment. Arguing over 
> parameter names and "why did you add a parameter to my function" are 
> controversial and adversarial at best. Let us refrain from this mentality.
> 

I strongly disagree here. I am used to open environments where it is always ok 
to asks "why?" questions. For example in the scientific world, if you publish 
something, you should be always prepared that someone will read your paper 
carefully and either criticize it pointing at the errors or (more often) ask you 
for explanations. Such behaviour is never considered bad or rude, but it is even 
appreciated, because it is one of the main ways of ensuring quality of the 
scientific work. I, as an author of scientific papers, am always very happy if 
someone asks me for explaining some points and even more happy if he points at 
errors in my work because it is the best way for improving it.

I also disagree with "no responsibility for the code" principle. I think not 
having a clear responsibilities is one of the things that stops our company from 
delivering better results. I'd like to have it very clearly defined and always 
know to whom I can turn if I see problems in some parts of the code.

So, I take full responsibility for the code I create and I expect the same from 
other developers. And part of this responsibility is to be ready to answer 
questions "why have you done it that way". I think it is an obligation of each 
developer to be able to answer such questions, especially if asked by reviewer. 
For one thing, it can help us to eliminate code which is there for no reason...

Now I even recall some "how to review" document saying that you should not 
accept code which you don't understand. And who should explain the code if not 
its author?

You should probably also agree that if someone implements feature X, then his 
patch should include only changes needed to implement feature X and nothing 
more. This is a principle which sometimes can have exceptions (e.g. we fix some 
problem or do refactoring in the same patch) but this is an extraordinary 
situation and thus it needs a good justification. Changing "const String&" 
parameter to "LEX_STRING*" just because I like LEX_STRING more than String is 
not a good enough reason...

Ok, that is perhaps enough to explain my position on this. I'm sorry if we 
disagree again, but I find it unlikely that I'll change my attitude here.

Rafal