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

[Junio C Hamano <gitster@pobox.com>]

Amisha Chhajed <amishhhaaaa@gmail.com> writes:

>> Perhaps something like this would replace the original "grep | sed"
>> pipeline?
>>
>>         sed -E -e "
>>                 /^[^.]+\.[^.]+$/b out
>>                 /^[^.]+\.[^.]+\.[^.]+$/b out
>>                 d
>>                 : out
>>                 s/\..*//
>>         " human |
>>         sort -u
>>
>>
>
> Thank you for pointing me in the right direction!

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>

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b45bdac688..f0edcf8eb0 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -35,7 +35,7 @@ sub err {
 		chomp;
 	}
 
-	/\bsed\s+-i/ and err 'sed -i is not portable';
+	/\bsed\s+-[^efn]\s+/ and err 'sed option not portable (use only -n, -e, -f)';
 	/\becho\s+-[neE]/ and err 'echo with option is not portable (use printf)';
 	/^\s*declare\s+/ and err 'arrays/declare not portable';
 	/^\s*[^#]\s*which\s/ and err 'which is not portable (use type)';