[maemo-community] Proposal: code review process for community SSU

[From: Alberto Mardegan]

On 02/07/2011 01:43 AM, Sebastian Lauwers wrote:
> On 6 February 2011 19:54, Alberto Mardegan<mardy at users.sourceforge.net>  wrote:
>> Hi Mohammad,
>
>> The main problem with the patch is that it's big (and if I may say so,
>> also a bit messy). When Matan contributed some code to my project, I
>> asked him to rewrite the patches till they were of my liking, and he
>> was kind enough to do that. If he hadn't done that, I would have
>> either discarded the patches (if I was not interested in them) or
>> rewritten them myself.
>
> So this is more a personal style discussion than an actual "let's get
> things done" discussion.

No, you completely missed the point. As I wrote already, those two commits are 
just an example of something which I would not let pass if I had been given a 
chance to look at them. They are not awfully bad or anything.
My main point is about having more people looking through the code before it is 
committed.

> In the end, it doesn't really matter how many
> commits there are (more specifically, having 200 commit for 200 lines
> of change doesn't add any readability when it comes to repository
> maintenance, and we'll soon see complaints that merges should also
> include some form of rebasing in order to limit the amount of "noise"
> generated by overly trigger-happy committers.

Of course.

> I fully support the idea of proper, apt and useful commit messages,
> but complaining that there are too few commits in a patch is just
> plain nonsense.

Generally, yes. But in this case it's perfectly legitimate, because that patch 
includes different features which have nothing to do with each other (such as 
enabling the status menu in portrait mode and adding keyboard bindings).

> git-blame allows to track what happened in a very useful manner.

But git-revert works only if the commits are meaningful chunks of code.

> Whether that means being able to pinpoint commit index + 20 or commit
> index + 30 doesn't *really* impact a lot; most people won't take the
> time, or can't make sense of how people commit anyway. It also doesn't
> hamper git-bisect in any way -- well, it only means that you have to
> understand what is happening in code in order to find a bug, but
> frankly, if you don't, you shouldn't be reviewing code anyway.

What?? What's the harm caused by code reviews??

> FYI: I'm using git at work, and often receive +3k line commits. It
> doesn't mean the coders or sloppy, it's just "one chunk of code that
> does one feature".

Wow, happy reviewing! :-) In my case, such commits go to the trashbin.

> I would like to see a coding style proposal, or a "how to use git
> effectively", or something of the like. I think it would make working
> with others a lot easier.

Sure. But we can think of that only if we first enable code reviews, otherwise 
if it cannot be enforced I don't see much point in it.

Ciao,
   Alberto

-- 
http://blog.mardy.it <-- geek in un lingua international!