On 09/23/2010 04:43 AM, Jim Meyering wrote:
From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
From: Jim Meyering<meyering@xxxxxxxxxx>
Date: Thu, 23 Sep 2010 10:11:44 +0200
Subject: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM
(see other email for general response to these changes, comments on
GLib, OOM, etc.)
First off, I ACK (accept) all these changes. Technically they appear
correct, and I am interested in merging them.
But I request a few minor style and workflow adjustments, and a
resubmission. Specific comments:
[style]
1) the functional style of sizeof keyword, with parens, is preferred:
- snprintf(s, 64, "get user '%s'", user);
+ snprintf(s, sizeof s, "get user '%s'", user);
2) it is preferred to omit optional braces for singleton test+stmt style
statements:
+ if (!pass) {
+ goto err_cmp;
+ }
[patch submission administrivia]
3) I process patches similar to how Linus and others in the kernel do
it: "git am /path/to/mbox_of_patches" That tends to impose some
restrictions on the contents of each email.
In your case, while the patch descriptions and diffs themselves are
correct, you seem to be sending one-mbox-per-email, while I'm expecting
one-patch-per-email. If you could tweak your process to make that
change, that would reduce the manual labor on my part.
4) While total number of patches is not really a problem, I would
request sweeping most of the one-and-two-liners in this series into a
single patch, leaving perhaps only the bucket.c and status.c changes as
standalone patches.
It's more an art and style preferences, than science, when deciding how
to separate out changes into patches. Trying to take my cues from the
kernel, it is preferred, for example, that bug fixes be separate from
new features, or whitespace and cosmetic changes separate from
functional changes. But it is also encouraged to group similar changes
together, if, for example, you're making a similar change across a large
number of files.
Mailing list review-ability, useful 'git bisect' boundaries, and a
coherent 'git shortlog' summary tend to be my guides when deciding patch
boundaries.
Thanks!
Jeff
--
To unsubscribe from this list: send the line "unsubscribe hail-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
