Hi Phil,
On Mon, 15 Jun 2020 11:00:44 +0200
Phil Sutter <phil@xxxxxx> wrote:
> Hi Stefano,
>
> On Sun, Jun 14, 2020 at 11:41:49PM +0200, Stefano Brivio wrote:
> > Commit 7d93e2c2fbc7 ("tests: shell: autogenerate dump verification")
> > introduced the definition of DIFF at the top of run-tests.sh, to make
> > it visually part of the configuration section. Commit 68310ba0f9c2
> > ("tests: shell: Search diff tool once and for all") override this
> > definition.
> >
> > Drop the unexpected redefinition of DIFF.
>
> I would fix it the other way round, dropping the first definition.
Then it's not visibly configurable anymore. It was in 2018, so it
looks like a regression to me.
> It's likely a missing bit from commit 68310ba0f9c20, the second
> definition is in line with FIND and MODPROBE definitions immediately
> preceding it.
I see a few issues with those blocks:
- that should be a single function called (once or multiple times) for
nft, find, modprobe, diff, anything else we'll need in the future.
It would avoid any oversight of this kind and keep the script
cleaner. For example, what makes sort(1) special here?
- quotes are applied inconsistently. If you expect multiple words from
which(1), then variables should also be quoted when used, otherwise
the check might go through, and we fail later
- we should use 'command -v', which is the standard and standardised
way of doing this rather than which(1): 'which' has many different
and inconsistent implementations. Will it check aliases? Should you
suppress stdout or stderr? How do you... 'which which'?
- we should extend the configurability for single commands to all of
them. I need to export NFT, 'diff' I can edit on top of the file, the
rest is not configurable at all. It's easy with a single function.
...so I started rewriting that, then realised I didn't have time at the
moment and just fixed the obvious issue I saw.
If the definition on the top is not actually useful, then I'd rather
keep things this way instead of just proposing a cosmetic change for
things that would actually need a small rework.
--
Stefano
