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@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);
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.
--
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
