From: Andy Parkins <andyparkins@gmail•com>
To: bitcoin-development@lists•sourceforge.net
Subject: Re: [Bitcoin-development] Code review
Date: Fri, 04 Oct 2013 13:34:19 +0100 [thread overview]
Message-ID: <1625158.f2FY7VNCrz@momentum> (raw)
In-Reply-To: <CANEZrP1-4vP10w6_Yg3tXAKcMh406rw2bsAnvML2WoaU5SUjYw@mail.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
next prev parent reply other threads:[~2013-10-04 12:34 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-04 10:30 Mike Hearn
2013-10-04 10:42 ` Andy Parkins
2013-10-04 11:32 ` Mike Hearn
2013-10-04 12:34 ` Andy Parkins [this message]
2013-10-04 11:35 ` Peter Todd
2013-10-04 11:58 ` Arto Bendiken
2013-10-04 12:14 ` Peter Todd
2013-10-04 12:34 ` Andy Parkins
2013-10-04 11:53 ` Peter Todd
2013-10-04 12:14 ` Mike Hearn
2013-10-04 12:22 ` Eugen Leitl
2013-10-05 2:31 ` Gavin Andresen
2013-10-05 4:02 ` Warren Togami Jr.
2013-10-05 11:36 ` Mike Hearn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1625158.f2FY7VNCrz@momentum \
--to=andyparkins@gmail$(echo .)com \
--cc=bitcoin-development@lists$(echo .)sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox