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

[Jim Meyering <jim@meyering.net>]

Jeff Garzik wrote:
> 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);

Sure.  Adjusted.

> 2) it is preferred to omit optional braces for singleton test+stmt
> style statements:
>
> +	if (!pass) {
> +		goto err_cmp;
> +	}

Gladly.  That's what I would have done in code I own, but there is a
braced single-line "else" block just above, so I presumed that the
style was "always use braces".  (I think we have the same preferences,
since I too would use braces around the single-line "else" in that case,
though not if the "then" block had also been a one-liner.

> [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.

No problem.

> 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.

Will do.
You can tell that I'm too accustomed to posting FYI-patches
that I will shortly push -- or that I'll push upon review.

> 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.

Preaching to the choir ;-)

Thanks for spelling out your guidelines.