Re: [Libguestfs] [libnbd PATCH 00/18] wrap hand-written source code at 80 characters

On 4/19/23 15:40, Eric Blake wrote: > On Tue, Apr 18, 2023 at 07:26:13PM +0200, Laszlo Ersek wrote: >> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2172516 > > [I started this email yesterday, then postponed it while looking at > individual patches...] > >> >> This series wraps the non-generated C-language source code (*.c and *.h >> files) at 80 characters. >> >> "ocaml/helpers.c" remains overlong, but I couldn't find a way to wrap >> it: its single overlong line contains the comment >> >> /* For how we're getting the exception name, see: >> * https://github.com/libguestfs/libguestfs/blob/5d94be2583d557cfc7f8a8cfee7... >> */ >> >> and even if I truncate the blob hash to 12 nibbles, the line remains too >> long. > > Truncating is fine to reduce the worst of the width, but I also > understand your reluctance to trim too short (git defaults to 7 > nibbles in a fresh repository, but larger repositories like linux.git > output at least 10 nibbles and sometimes more because there are just > that many more hash prefix collisions as history grows - it's never > fun when a link valid today stops working tomorrow when a prefix > collision is introduced into the repo). At any rate, I have no > problems with long URLs in source files that are otherwise > length-constrained. Right, I seem to remember Linux now recommends capturing 12 nibbles. > > >> >> The following files are also too wide: >> >> include/libnbd.h >> lib/api.c >> lib/states-run.c >> lib/states.c >> lib/states.h >> lib/unlocked.h >> ocaml/nbd-c.c >> python/libnbdmod.c >> python/methods.h >> >> but they are all generated; we'll have to discuss them separately. > > Wrapping a generated file for legibility is definitely harder work; > legible generated code still has its benefits, but longer generated > lines for faster coding of the generator is a tolerable tradeoff in my > book. So Rich pointed out in <https://bugzilla.redhat.com/show_bug.cgi?id=2172516#c3> that the generator already had wrapping goodies. The problem that made me put generated code aside immediately was not a lack of wrapping; instead, the code was nicely wrapped (such that I couldn't improve it even by manually tweaking the result!), but it was *still* too wide! Consider, from "include/libnbd.h": -------- extern int nbd_aio_opt_list_meta_context_queries (struct nbd_handle *h, char **queries, nbd_context_callback context_callback, nbd_completion_callback completion_callback) LIBNBD_ATTRIBUTE_NONNULL (1, 2); -------- That's 94 chars wide; even if we drop the "extern ", it remains 87 characters wide. :/ So I wanted to keep that discussion separate. > > Overall, the series looked okay to me at a first read through; I did > spot some things on individual patches where I made comments, but they > are of the nature where I'm also okay with you adding: > > Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; > > whether or not you touch things up. > I'll go through the patches and see how I feel about just pushing them with the suggested updates, or posting v2.