Pascal Hambourg <pascal@plouf.fr.eu.org> (2023-03-16): > IIUC the script arguments are expected to be network interface names > except the first one which may be "-n". In this case, I believe it > should be shifted away before setting IFACES="$@". OK, this might be fixing an actual issue. > > > - check-missing-firmware.sh: define local variables in functions > > These variables are intended to be used only inside their respective > functions, so it is cleaner and safer if they are declared as local > (like in other hw-detect scripts) to avoid potential side effects with > other functions or the main body if they use variables with the same > names. OK, this doesn't seem to be fixing anything at the moment. > > The commit messages say what you do, not why. > > Sorry, I thought it was obvious. > > > > - check-missing-firmware.sh: replace spaces with tabs in indentation > > > > NACK. We have mixed tabs and spaces all over the place, in hw-detect and > > in other components. We don't need noise. Especially not at this stage. > > Spaces for indentation are present only in one function, so I thought > it might be a text editor glitch or human error and it was desirable > to make it consistent with the rest of the script. AFAICS the only > consistent use of spaces for indentation in other hw-detect scripts is > before "case" patterns, which makes sense. Most other occurrences look > like mistakes. But I take your point about avoiding cosmetic fixes > now. They can't be mistakes since I'm not aware of a defined let alone enforced code style. Fixing inconsistencies locally when actually working on the code doesn't seem like a bad idea. Touching code just to make it “pretty” doesn't look so appealing to me. There's nothing more annoying than ending up with merge conflicts after working on something for a while, just because someone toyed with whitespaces… Cheers, -- Cyril Brulebois (kibi@debian.org) <https://debamax.com/> D-I release manager -- Release team member -- Freelance Consultant
Attachment:
signature.asc
Description: PGP signature