On 4/19/23 16:01, Laszlo Ersek wrote: > 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/5d94be2583d557cfc7f8a8cfee7988abfa45a3f8/daemon/daemon-c.c#L40 >>> */ >>> >>> 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 <ebl...@redhat.com> >> >> 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.
I've pushed the series as commit range 1acdeec1a248..a458f0644de0, *except* patch #16 (I've updated patches #5 and #11). I'll post a v2 of patch #16. Thanks! Laszlo _______________________________________________ Libguestfs mailing list Libguestfs@redhat.com https://listman.redhat.com/mailman/listinfo/libguestfs
