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

[Stefano Brivio <sbrivio@redhat.com>]

Hi Phil,

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.

> 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