Hello, On Sun 21 Jul 2019 at 06:05PM +01, Ian Jackson wrote:
> Sean Whitton writes ("Bug#932477: git-debpush checking that patches
> (un)apply"):
>> I've implemented this in branch 'series/git-debpush-apply-patches-v1' of
>> repo <https://git.spwhitton.name/dgit>.
>
> Hi. I've reviewed your substantive code. Looking reasonably good but
> I do have some nontrivial comments.
Thank you for a useful review.
Revised series is in branch 'series/git-debpush-apply-patches-v2' of
repo <https://git.spwhitton.name/dgit>.
> This seems like a very long way of writing a very short sed rune ?
Now that there's only one while loop, I don't believe this comment
applies.
In the revised series, I find the explicit -z check (l. 146) more
readable than avoiding that check by means of sed. So I'd like to keep
the use of extglob.
> This separation of the <../series from while makes it a bit harder to
> follow. Maybe write one of
> + <../series while read patch; do
> + while read patch <../series; do
> ?
The first one is a syntax error and the second one feeds the first line
of ../series to the loop over and over. So unless we are willing to
spawn an additional process by doing `cat ../series | while` (I'd rather
not), I think we have to live with this.
--
Sean Whitton
signature.asc
Description: PGP signature
