From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from localhost (localhost [127.0.0.1]) by olra.theworths.org (Postfix) with ESMTP id 01B8A431FBC for ; Mon, 11 Feb 2013 10:54:13 -0800 (PST) X-Virus-Scanned: Debian amavisd-new at olra.theworths.org X-Spam-Flag: NO X-Spam-Score: 0.01 X-Spam-Level: X-Spam-Status: No, score=0.01 tagged_above=-999 required=5 tests=[T_MIME_NO_TEXT=0.01] autolearn=disabled Received: from olra.theworths.org ([127.0.0.1]) by localhost (olra.theworths.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id B8uIba-sBZ6y for ; Mon, 11 Feb 2013 10:54:12 -0800 (PST) Received: from arlo.cworth.org (arlo.cworth.org [50.126.95.6]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by olra.theworths.org (Postfix) with ESMTPS id 1FC16431FAE for ; Mon, 11 Feb 2013 10:54:12 -0800 (PST) Received: from yoom.home.cworth.org (localhost [127.0.0.1]) by arlo.cworth.org (Postfix) with ESMTP id 2D07F6DE0C03; Mon, 11 Feb 2013 10:54:10 -0800 (PST) Received: by yoom.home.cworth.org (Postfix, from userid 1000) id 9B76E60C12; Tue, 12 Feb 2013 05:56:57 +1100 (EST) From: Carl Worth To: Robert Mast , notmuch@notmuchmail.org Subject: Re: [PATCH] convert bitmap to unsigned char In-Reply-To: <1360501997-4967-1-git-send-email-beheerder@tekenbeetziekten.nl> References: <1359917491-17178-1-git-send-email-beheerder@tekenbeetziekten.nl> <1360501997-4967-1-git-send-email-beheerder@tekenbeetziekten.nl> User-Agent: Notmuch/0.13.1 (http://notmuchmail.org) Emacs/23.4.1 (x86_64-pc-linux-gnu) Date: Mon, 11 Feb 2013 10:56:51 -0800 Message-ID: <87r4kmk7bg.fsf@yoom.home.cworth.org> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha1; protocol="application/pgp-signature" Cc: Robert Mast X-BeenThere: notmuch@notmuchmail.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: "Use and development of the notmuch mail system." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Feb 2013 18:54:13 -0000 --=-=-= Content-Transfer-Encoding: quoted-printable Robert Mast writes: > --- Hi Robert, It looks like the git exercise is proving useful. Keep it up! If you look through the git logs a bit you'll find that the overwhelming convention is for a commit message to have a single-line summary followed by a (potentially longer) expanded comment. The convention I like best is for the single-line summary to efficiently describe everything about "what" the patch does. If it's hard to fit this into a single line there's a good chance your patch should be split up. With your commit message here, the one-line summary is great, (and much better than in your first submission). The expanded comment should describe the "why" of the change. And here, your commit message doesn't have anything. So I'm left wondering why your commit exists. Does this save memory? Does this fix some type error or expected alignment somewhere? etc. Please re-submit your patch with a little more explanation of the motivation behind the patch. =2DCarl PS. I do know that you did have more in the commit message originally, and Austin recommended removing "snide" issues, (doubts and meta-questions about the patch---things that don't belong in the commit history). Your previous commit message was: Reading it in detail I thought it allocated way too much memory and didn't use the full size of the allocated unsigned ints for storing bits. Am I right, and is this the right way to patch code to notmuch? I'm not actually looking at the code in context now, so I can't render a correct commit message, but this attempted rewording should hopefully give you the idea: Using char instead of int allows for simpler definitions of the DOCIDSET macros so the code is easier to understand. --- Am I reading that correctly? Or is there also some space saving happening here as well? Please re-submit a new patch with a commit message that makes things clear one way or the other. Do you see how this "why" part of the commit message is ready to stand alone in our commit history (assuming it's correct and accepted). Meta-questions like "is this even a sane way of doing things?" are great to ask, and very helpful for code reviewers, but best placed after the "---" so that they don't become part of the commit message. Thanks for playing with notmuch! =2DCarl =2D-=20 carl.d.worth@intel.com --=-=-= Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iEYEARECAAYFAlEZPvMACgkQ6JDdNq8qSWhuawCfZTraPY2wUSv62MTQl5qhHO99 lo8An1xH+tz66hfppsPU7KtTq/YiSVsA =fzpL -----END PGP SIGNATURE----- --=-=-=--