GNU bug report logs - #59761
[PATCH 0/2] Add u-boot-ts7970-q-2g-1000mhz-c.

Previous Next

Full log

View this message in rfc822 format

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ricardo Wurmus <rekado <at> elephly.net>
Cc: 59761 <at> debbugs.gnu.org
Subject: [bug#59761] [PATCH 0/2] Add u-boot-ts7970-q-2g-1000mhz-c.
Date: Tue, 03 Jan 2023 23:17:00 -0500

Hi Ricardo,

Ricardo Wurmus <rekado <at> elephly.net> writes:

> Hi Maxim,
>
> there seems to be some overlap between this and
> https://issues.guix.gnu.org/60224.

Yes, I ended up splitting my changes focusing on u-boot in #60224, which
should be reviewed before and blocking this change here, which is based
on it.

> Looking just at v4 I only have one
> comment.
>
> In your substitute* replacements it’s better not to use string-append.

Oh?  Why is this so?  There must be hundreds of string-append occurences
used in such place, so I'm curious.

> You can include real line breaks in a string and escape line breaks with
> \.  This is preferable to gluing strings together.

OK, I guess this is your rationale for the above comment (cleaner).

> For something as
> long as the replacements in this package consider using a patch file
> instead.  This has the added advantage of failing the build when the
> patch cannot be applied cleanly.

I agree that a patch would be most suitable here, especially that if
something breaks, if would likely be silent (unlikely to be caught at
build time).  I'll extract this as a patch.

> The rest looks good to me.

OK.  I'll await your comments on #60224, which is awaiting feedback
post-rework based on your earlier feedback.

PS: I had also missed that email; please keep me in CC in all your
replies :-).

-- 
Thanks,
Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.