Re: [Bitcoin-development] Code review

[Andy Parkins <andyparkins@gmail.com>]

On Friday 04 October 2013 13:32:47 you wrote:
> > There is more to a git branch than just the overall difference.  Every
> > single
> > log message and diff is individually valuable.
> 
> When the log messages don't accurately describe the contents of the diff,
> it's just misinformation and noise. Everyone starts out by wanting a neat
> collection of easy to understand and review commits, but in practice it's
> extremely hard to always get it.

Then your request should be for better commits, not for just squashing the lot 
into some incoherent blob.

The alternatives under discussion are:

 - Coder produces long chain of commits on feature branch.  Compresses them, 
throwing away any individual and accurate messages into one large diff.  It's 
unlikely you'll get a log message that is as descriptive in the large one if 
you made them throw away the little ones.  Large diff is offered for review.  
Review is of one large diff.

 - Coder produces long chain on commits on feature branch.  Offers them for 
review.  Reviewer only likes to review large diffs, so uses the tools 
available to produce it.

Exactly the same diff is being reviewed, but in one case you're throwing away 
information.  There is no getting that information back ever.

You're also discarding the advantages of individual commits.

 - Merges are considerably harder than rebases.  You have to resolve all the 
conflicts at once with a merge, with a rebase you can resolve them with the 
log message and original isolated diff to help you.

 - Bisect doesn't give as fine-grained an answer.

> I know how to make squashed commits, thanks. I've done LOTS of code review

Excellent.  Don't take it personally -- I only offered it in case you didn't 
know.  Not everyone is familiar with git plumbing.

> in my life. I'm making a point here as one of the few people who goes
> through large pull requests and reviews them line by line. It's hard,

That doesn't make you the only person who does code reviews.  I do plenty of 
reviews here; they're just not bitcoin reviews.  Obviously we're talking about 
bitcoin, so you get to decide in the end.

> partly because github sucks, and partly because reviewing lots of small
> commits sucks.

I'm not suggesting you review lots of small commits anyway.  I can't comment 
on whether github sucks or not -- that's obviously personal preference.  
However, nothing stops you doing reviews on your own local checkout.

> There's nothing that makes a single large commit harder to review. It's the
> same amount of code or strictly less, given the tendency for later commits

That's not true.  There are often lots of small changes that are manifestly 
correct -- let's use string changes as an example -- in the large commit, they 
are just noise.  You want to be able to focus on the hard commits.  However -- 
I am not trying to persuade you to review small commits, I'm trying to 
persuade you not to throw away the small commits, gone forever, merely because 
your preference is to review large commits.

> to change earlier ones. You can easily search the entire change whilst
> reviewing. There are lots of things that make it easier.

Since the large commit is always available, no facilities have been lost.

Personally I work hard in my repositories to make coherent, small, well 
described commits.  If I had gone to that effort for a bitcoin branch only to 
be told to collapse them all and throw away that effort, I'd think I'd been 
wasting my time.



Andy

-- 
Dr Andy Parkins
andyparkins@gmail.com