Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193] helo=mx.sourceforge.net) by sfs-ml-1.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1VS4aV-0006BL-BC for bitcoin-development@lists.sourceforge.net; Fri, 04 Oct 2013 12:34:31 +0000 Received-SPF: pass (sog-mx-3.v43.ch3.sourceforge.com: domain of gmail.com designates 74.125.82.45 as permitted sender) client-ip=74.125.82.45; envelope-from=andyparkins@gmail.com; helo=mail-wg0-f45.google.com; Received: from mail-wg0-f45.google.com ([74.125.82.45]) by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1VS4aS-0001B8-3c for bitcoin-development@lists.sourceforge.net; Fri, 04 Oct 2013 12:34:31 +0000 Received: by mail-wg0-f45.google.com with SMTP id y10so4073201wgg.12 for ; Fri, 04 Oct 2013 05:34:22 -0700 (PDT) X-Received: by 10.180.81.71 with SMTP id y7mr7070759wix.63.1380890062000; Fri, 04 Oct 2013 05:34:22 -0700 (PDT) Received: from momentum.localnet ([91.84.15.31]) by mx.google.com with ESMTPSA id dl10sm15407271wib.1.1969.12.31.16.00.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Oct 2013 05:34:21 -0700 (PDT) From: Andy Parkins To: bitcoin-development@lists.sourceforge.net Date: Fri, 04 Oct 2013 13:34:19 +0100 Message-ID: <1625158.f2FY7VNCrz@momentum> User-Agent: KMail/4.10.5 (Linux/3.10-2-amd64; KDE/4.10.5; x86_64; ; ) In-Reply-To: References: <3552695.aET6a1zFq8@momentum> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Spam-Score: -1.6 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain 0.0 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (andyparkins[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1VS4aS-0001B8-3c Subject: Re: [Bitcoin-development] Code review X-BeenThere: bitcoin-development@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Oct 2013 12:34:31 -0000 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