From: Carl Worth <cworth@cworth.org>
To: Robert Mast <beheerder@tekenbeetziekten.nl>, 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 <beheerder@tekenbeetziekten.nl>
Precedence: list

--=-=-=
Content-Transfer-Encoding: quoted-printable

Robert Mast <beheerder@tekenbeetziekten.nl> 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-----
--=-=-=--