In D58345#1338876, @joneum wrote:

Hi Matthias,

thanks for the cleanup — the input hardening in particular looks sensible.

I'm a bit curious why ports-secteam was added as a reviewer here. These are commit-side hook scripts, which I'd have read as portmgr territory. If secteam was pulled in specifically for the adversarial-input parts (check_aaa_filenames, the ERE escaping, printf %s, IFS=$'\n'), that's understandable — but I don't think that needs a formal secteam sign-off to proceed. Happy to resign ports-secteam from the review if that was the only reason; just let me know if you actually wanted a security look at something specific.

joneum — ports-secteam

I've ignored formatting matters since those would be going forth and back, and I'd look to portmgr@ for setting up a style guide before we should start looking over style. I don't mind much if I add the ; at the end of the command or with a blank before the ; - either is arguable.

• after tcberner's accident with a status=0 invalidating the if [ $? -ne 0 ] let's just inline everyting into the if.
• add check_no_binaries to reject files (using od and grep) that have certain control characters or NUL bytes in the first 16 kB
• add check_unmerged scripts that rejects conflict markers (we only check <<<<<<< ||||||| >>>>>>>, not ======= which might be in MD or RST or plain text files individuall)
• make check_category_makefile also check on $CATEGORY/Makefile edits that we don't break the SUBDIR += ... enumeration versus what's actually in Git.
• set -o pipefile in a few places,
• ensure we run git's core.quotePath=true (via environment)
• quote all shell variables unless we safely set them ourselves
• fix shellcheck warnings (pkg install hs-ShellCheck)
• fix excess ":" prefixes on some multi-line error messages by switching from $* to $@ in common.sh
• tweak logging

So, let me upload a new version as followup, which adds two more tools
addresses the ${var} brace syntax,
and also fix shellcheck complaints about the scripts,
except those telling us what we can't have in POSIX sh, as in shellcheck -s sh -e1090,3003,3037,3040,3043.

Added "PR: line" to the commit message and re-diffed with git format-patch HEAD^ -U9999 so as to provide full context if reviewers want to look at context.

See inline comment with links to a fix we need to add on top https://github.com/freebsd/freebsd-ports/pull/567 and the bug report https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=296889

What gives you the right to remove reviewers that the submitter placed?

The wait3() path reported failing previously via https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=295965 seems to be fixed for me now. Thank you.

I think I have no more ports affected by this change, stepping down from reviewing this change proposal.

Great, thanks, @tijl and @jrm!

Approving for devel/gettext-po-mode.

OK for archivers/py-borgbackup

Thanks for this contribution, tested, approved & committed.