tests: use correct HEAD to list files

[matttbe]

When checking the files before the patch, after a checkout to HEAD~, it is required to use the previous HEAD, not the new one to look at the same files and not others.

While at it, always use the $HEAD variable, and also fix a related comment + added an extra one to explain the diff.

[matttbe]

I forgot to add: I didn't check, and I only noticed this while quickly looking at the new scripts. So hopefully I'm fixing the right thing correctly :)

[matttbe]

Not related to the modifications here, but about the new tests. For Shellcheck, when I started to enforce it in MPTCP, I ended up disabling SC2086 (Double quote to prevent globbing and word splitting) for most files (see mptcp_connect.sh for example). This rule makes sense to avoid some issues, but when this is not done from the beginning, likely too many lines need to be adapted to support it.

I guess we don't want massive clean-ups simply adding double quotes where it was safe without them. Maybe we could keep this rule for new files (and if it was compliant before), but it adds some complexity in the scripts here. Or adding #shellcheck disable=SC2086 in existing files if it was not compliant (or check if the file was present in the future v6.16-rc0).

Maybe shellcheck could be used with --severity=warning to focus on the more important ones?

[kuba-moo]

Thanks for the fix!!

[kuba-moo]

Not related to the modifications here, but about the new tests. For Shellcheck, when I started to enforce it in MPTCP, I ended up disabling SC2086 (Double quote to prevent globbing and word splitting) for most files (see mptcp_connect.sh for example). This rule makes sense to avoid some issues, but when this is not done from the beginning, likely too many lines need to be adapted to support it.

I guess we don't want massive clean-ups simply adding double quotes where it was safe without them. Maybe we could keep this rule for new files (and if it was compliant before), but it adds some complexity in the scripts here. Or adding #shellcheck disable=SC2086 in existing files if it was not compliant (or check if the file was present in the future v6.16-rc0).

I was wondering about that one. It's really annoying but it has found bugs in my code in the past :S
I guess bash is the one that's annoying rather than the check..

In any case, I trust your judgement on this, I'm not a bash expert. If you think it's going to be too noisy to be practical we can disable it in the shellcheck script. Maybe @pabeni has a preference ?

Maybe shellcheck could be used with --severity=warning to focus on the more important ones?

Or we can adjust the regexp for counting? Again, I lack the bash experience - but for pylint our regexps are matching on errors, warnings and checks. There is another level with the letter R which I guess stands for Ridiculous, IDK.. but we ignore those. Perhaps it'd indeed be more intelligent to add the command line option not to print these at all, not sure.
But in terms of shellcheck - I was going to see how much noise the checks produce and whether they are worth looking at.

Oh, and as you probably noticed we count errors and warnings+checks separately. The checks and warnings are pretty much "for maintainer review", not a hard failure. We exit with 250 if we found only warnings and checks and NIPA reports that to patchwork as warning rather than fail.

[matttbe]

I was wondering about that one. It's really annoying but it has found bugs in my code in the past :S

Yes, me too, that's why I keep it usually for new scripts.

Or we can adjust the regexp for counting? Again, I lack the bash experience - but for pylint our regexps are matching on errors, warnings and checks. There is another level with the letter R which I guess stands for Ridiculous, IDK.. but we ignore those. Perhaps it'd indeed be more intelligent to add the command line option not to print these at all, not sure.

Good idea, so we can still see the ignored ones in the logs.

I can look at that during my lunch break.

But in terms of shellcheck - I was going to see how much noise the checks produce and whether they are worth looking at.

I quickly checked:

tools/testing/selftests/net $ shellcheck -x *.sh | grep -o " SC[0-9]* ([a-z]*)" | sort | uniq -c | sort -g
      1  SC1087 (error)
      1  SC2016 (info)
      1  SC2091 (warning)
      1  SC2119 (info)
      1  SC2145 (error)
      1  SC2214 (warning)
      1  SC2231 (info)
      1  SC2320 (warning)
      1  SC3028 (warning)
      2  SC1007 (warning)
      2  SC1105 (error)
      2  SC2003 (style)
      2  SC2005 (style)
      2  SC2076 (warning)
      2  SC3037 (warning)
      3  SC2116 (style)
      3  SC2143 (style)
      3  SC2242 (error)
      3  SC3045 (warning)
      4  SC2002 (style)
      4  SC2027 (warning)
      4  SC2062 (warning)
      4  SC2183 (warning)
      4  SC2220 (warning)
      4  SC2223 (info)
      4  SC2319 (warning)
      5  SC2048 (warning)
      5  SC2053 (warning)
      8  SC2268 (style)
     11  SC2153 (info)
     11  SC2316 (error)
     12  SC2236 (style)
     17  SC2068 (error)
     17  SC2254 (warning)
     18  SC2015 (info)
     19  SC2069 (warning)
     20  SC2126 (style)
     25  SC2124 (warning)
     26  SC2166 (warning)
     28  SC2059 (info)
     39  SC2034 (warning)
     43  SC2006 (style)
     52  SC2162 (info)
     71  SC2046 (warning)
     80  SC3043 (warning)
    102  SC2154 (warning)
    102  SC2155 (warning)
    105  SC2004 (style)
    144  SC1083 (warning)
    165  SC2181 (style)
   5919  SC2086 (info)
   7187  SC2317 (info)

The two big ones are "info" type ones:

• Command appears to be unreachable. Check usage (or ignore if invoked indirectly).: likely harmless because some functions are not called directly.
• Double quote to prevent globbing and word splitting.: the one we discussed.

Oh, and as you probably noticed we count errors and warnings+checks separately. The checks and warnings are pretty much "for maintainer review", not a hard failure. We exit with 250 if we found only warnings and checks and NIPA reports that to patchwork as warning rather than fail.

It makes sense. Hopefully it will be easy to spot the warning/error ones :)