Re: [PATCH v4 1/1] help: cleanup the contruction of keys_uniq

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

This unfortunately runs afoul of t/check-non-portable-shell.pl aka "make -C t test-lint". The particular rule was introduced in early 2019 with e62e225f (test-lint: only use only sed [-n] [-e command] [-f command_file], 2019-01-20). See the attached patch at the end.

It does cite the then-current POSIX.1 (Issue 7, 2018 edition) but the latest edition (Issue 8) documents "-E" as an option to use ERE

I wonder if the situation has improved in the past 7 years.

We seem to have started using "sed -E" without anybody complaining in 2022, with 461fec41 (bisect run: keep some of the post-v2.30.0 output, 2022-11-10). It was hidden because the 'E' was squished with another single letter option.

t/t6030-bisect-porcelain.sh:	sed -En 's/.*(bisect.*code) (-?[0-9]+) (from.*)/\1 -1 \3/p' err >actual &&

So I think of no strong reason to reject another new use of "sed -E". I am tempted to revert e62e225f (test-lint: only use only sed [-n] [-e command] [-f command_file], 2019-01-20), whose intention was to reject anything other than "-[efn]", to its previous form which rejected only "sed -i".

Alternatively, I would of course welcome volunteers to revamp the check-non-portable-shell.pl script to make the pattern more robust, and then add 'E' to the set of allowed options, but I somehow do not think that is a good use of our engineering resources.

For example, in addition to the escape we see in t6030 above, the current pattern would not catch use of -E if it is written this way:

	sed "-E" -e "
		...
	" human |
	sort -u

or

	sed \
		-E -e "
		...
	" human |
	sort -u

and million other ways to subvert the simple-minded pattern-match based check.

Opinions?

commit e62e225ffb589e59c4f64d90b0a393aa6a0a5ace
Author: Torsten Bögershausen <tboegi@web.de>
Date:   Sun Jan 20 08:53:50 2019 +0100

    test-lint: only use only sed [-n] [-e command] [-f command_file]
    
    From `man sed` (on a Mac OS X box):
    The -E, -a and -i options are non-standard FreeBSD extensions and may not be available
    on other operating systems.
    
    From `man sed` on a Linux box:
    REGULAR EXPRESSIONS
           POSIX.2 BREs should be supported, but they aren't completely because of
           performance problems.  The \n sequence in a regular expression matches the newline
           character,  and  similarly  for \a, \t, and other sequences.
           The -E option switches to using extended regular expressions instead; the -E option
           has been supported for years by GNU sed, and is now included in POSIX.
    
    Well, there are still a lot of systems out there, which don't support it.
    Beside that, IEEE Std 1003.1TM-2017, see
    http://pubs.opengroup.org/onlinepubs/9699919799/
    does not mention -E either.
    
    To be on the safe side, don't allow -E (or -r, which is GNU).
    Change check-non-portable-shell.pl to only accept the portable options:
    sed [-n] [-e command] [-f command_file]
    
    Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
    Helped-by: Eric Sunshine <sunshine@sunshineco.com>
    Helped-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
    Signed-off-by: Torsten Bögershausen <tboegi@web.de>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>