Re: [PATCH nft] tests: shell: Drop redefinition of DIFF variable

[Phil Sutter <phil@nwl.cc>]

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@nwl.cc> 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