I'm not the world's greatest expert on code review. To start with, I call it change review. Anyway, I think the point of the review is:

* is this a change we want?
* is it done in a way we can accept, esp. re maintainability and style
* has test suite been changed to cover this?
* what can everyone in the project learn from this?
* in general, help people new to project become familiar with the code base

I don't think the purpose of the review is to find bugs.

@liw
> I don't think the purpose of the review is to find bugs.

100% agreed. Although frequently at least 2 and 3 will do just that.

@unixsmurf It's what a lot of people think the purpose of the review is, certainly.

@liw
For me, I might want to add:
* Is the purpose of the change, as well as what it actually does, clearly described?

This arguably falls under 2, but I like calling it out explicitly.

@liw @zhenech There‘s also the point if shared responsibility: If there is a major bug in it and neither you nor me found it, then it was not obvious. If the customer system breaks, you are not alone responsible now, we both nodded and accepted that change. No need to feel personal guilt. To me as a newcomer back when, this was *the* major point that enabled me to do changes to production code.

@liw Also, as a maintainer of several OSS libs/frameworks (hell even work stuff):

* is this change aligned with the style/philosophy/design of the library? (Consistent naming and arguments style of api)
* is this a change _I_ want to maintain, or _who_ will maintain it?
* does it introduce dependencies that break the above
* is the change solving a problem in a library centric/general way, or is it solving Joe Randoms' unique quirk

@liw @unixsmurf Not bugs persae, but design assumptions that may bite you elsewhere in a form of a "bug" from a caller you weren't familiar with. (Had that today, design choice was questioned in review for 'feeling wrong', we went with it tho - the not 15 minutes later - yep - that choice whilst 99.999% was good, a certain usecase wants access to data thats now blocked - need to rethink the api of the later service to accommodate...

@unixsmurf @liw this is why I think one of the most important things in reviewing someone's prior work and/or an interview is seeing if they can write a proper commit message that includes:

1) one line summary you can grep for later
2) accurate problem statement
3) description of the change
4) explanation of how 3 relates to 2

If the candidate's work is sus but they can get all of that right, it's probably worth the risk. If they can't get that right, I can't fix what they get wrong.

@unixsmurf @liw so many people make a fairly complex change and their commit message just references the bug and… like, fine, yeah, that was a well written bug report, but damn, I've been around to long to trust in the continuity of any one system

@vathpela @liw
And cover letter. Being able to clearly describe the scope of the full changeset.

This is one of my major bugbears with ... all the forges (I think) ... They throw that out with the bath water effectively.

Like, when github is gone, all reasoning over why a set of changes happened once upon a time will disappear.

Meanwhile, I can easily access that information for (most of) the linux kernel even for changes that predate git.

Also ties in with the comment from @meena

@wouter @liw that's assuming a properly tested piece of code, which unfortunately is not as pervasive in work environments as we wanted.

For instance, yesterday I caught a bug were a Python function was returning the wrong type. Yes, you could argue that the code should be typed and tested in CI, and we try, but for reasons we can't always do.

So my change reviews (good term!) include bugs and all of the above (and some of the below :)

@wouter insert "you guys are paid?" meme with "you guys are given time to add tests?" :(

@liw When you are taking a safety oriented approach (which admittedly is novel to rather too many software people) the question isn't "does it contain bugs" it's "what does it do to the probability of bugs and their severity"

It's actually often far more important that an API is hard to use wrongly, than a specific bug is present. The specific bug will get squashed, the API flaw will haunt you forever and keep generating new problems.

@wouter @liw yeah, well, you're talking about 80-90% coverage, I'm more like 20yo codebase with 10-20% if I'm lucky :)

So yeah, in green pastures, bugs should be caught automatically by tests+CI. In the desert, check under big rocks at least :)

@mdione @wouter If reviews are the only realistic way for you to find bugs, then you review to find bugs.

But it's not a good situation to be in, in my opinion, and I think Wouter agrees. It'd be good to work towards getting into a situation with a test suite that gives you confidence the software works. That's going to take a lot of effort, and won't be fun.

@liw we do it as much as we can, but for reasons I can't talk about we can't right now @wouter

@unixsmurf @liw @meena I think I come down on the other side of that for exactly the same reasons - a cover letter is nice but if everything isn't in the commit message as well, it's going to get lost.

@vathpela
What I'm suggesting is that the cover letter _should_ be a commit in the repo, but that mailing lists have fewer pathological disappearing act issues than forges.