Hi Stefano,
On Mon, Jun 15, 2020 at 12:18:11PM +0200, Stefano Brivio wrote:
> 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.
Hmm? Commit 68310ba0f9c2 is from January this year?!
> > 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?
It is simply grown and therefore a bit inconsistent.
> - 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
There are needless quotes around $(...) but within single brackets we
need them if we expect empty or multi-word strings. Typical output would
be 'diff not found' and using '[ ! -x "$DIFF" ]' we check if which
returned diff's path or said error message. We don't expect diff's path
to contain spaces, hence no quoting afterwards.
Cheers, Phil
