Re: [PATCH tabled 1/2] server/config.c: don't dereference NULL on OOM

[Jeff Garzik <jeff@garzik.org>]

On 09/23/2010 04:43 AM, Jim Meyering wrote:
>> From fb7865d158b0d32907dde703c4d37c70a26e738c Mon Sep 17 00:00:00 2001
> From: Jim Meyering<meyering@redhat.com>
> 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