[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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:


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.



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

[Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [XFree86]

Add to Google Powered by Linux