diff mbox series

[PATCH/RFC,v2,1/1] test-lint: Only use only sed [-n] [-e command] [-f command_file]

Message ID 20190120075350.5950-1-tboegi@web.de (mailing list archive)
State New, archived
Headers show
Series [PATCH/RFC,v2,1/1] test-lint: Only use only sed [-n] [-e command] [-f command_file] | expand

Commit Message

Torsten Bögershausen Jan. 20, 2019, 7:53 a.m. UTC 
From: Torsten Bögershausen <tboegi@web.de>

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>
---
 t/check-non-portable-shell.pl | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--
2.20.1.2.gb21ebb671

Comments

Junio C Hamano Jan. 22, 2019, 7:47 p.m. UTC | #1 
tboegi@web.de writes:

> From: Torsten Bögershausen <tboegi@web.de>
>
> 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.
>
> -	/\bsed\s+-i/ and err 'sed -i is not portable';
> +	/\bsed\s+-[^efn]\s+/ and err 'Not portable option with sed (use only [-n] [-e command] [-f command_file])';

"sed -n -i -E -e 's/foo/bar/p'" won't be caught with this as an
error, but that's OK ;-).

Is this still an RFC patch?
Torsten Bögershausen Jan. 22, 2019, 8 p.m. UTC | #2 
On 22.01.19 20:47, Junio C Hamano wrote:
> tboegi@web.de writes:
>
>> From: Torsten Bögershausen <tboegi@web.de>
>>
>> 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.
>>
>> -	/\bsed\s+-i/ and err 'sed -i is not portable';
>> +	/\bsed\s+-[^efn]\s+/ and err 'Not portable option with sed (use only [-n] [-e command] [-f command_file])';
>
> "sed -n -i -E -e 's/foo/bar/p'" won't be caught with this as an
> error, but that's OK ;-).
>
> Is this still an RFC patch?
>

It seems as if everybody is happy with it,
so it may be ready for for pu.
Eric Sunshine Jan. 22, 2019, 9:15 p.m. UTC | #3 
On Tue, Jan 22, 2019 at 3:00 PM Torsten Bögershausen <tboegi@web.de> wrote:
> On 22.01.19 20:47, Junio C Hamano wrote:
> > tboegi@web.de writes:
> >> -    /\bsed\s+-i/ and err 'sed -i is not portable';
> >> +    /\bsed\s+-[^efn]\s+/ and err 'Not portable option with sed (use only [-n] [-e command] [-f command_file])';
> >
> > "sed -n -i -E -e 's/foo/bar/p'" won't be caught with this as an
> > error, but that's OK ;-).
> > Is this still an RFC patch?
>
> It seems as if everybody is happy with it,
> so it may be ready for for pu.

I'd still prefer to see a more terse[1] (and not capitalized) message
to be consistent with existing error messages and to keep the reported
errors more compact overall to make them easier to digest[2,3]:

    err 'sed option not portable (use only -n, -e, -f)'

But that's just a very minor nit.

[1]: http://public-inbox.org/git/CAPig+cSeDNYFGYC2WznjW3zYMJCWZbZFY1KM5H5ir2L=Jxwy7w@mail.gmail.com/
[2]: http://public-inbox.org/git/20180713055205.32351-3-sunshine@sunshineco.com/
[3]: http://public-inbox.org/git/20180713055205.32351-4-sunshine@sunshineco.com/
Torsten Bögershausen Jan. 23, 2019, 6:35 a.m. UTC | #4 
On 22.01.19 22:15, Eric Sunshine wrote:
> On Tue, Jan 22, 2019 at 3:00 PM Torsten Bögershausen <tboegi@web.de> wrote:
>> On 22.01.19 20:47, Junio C Hamano wrote:
>>> tboegi@web.de writes:
>>>> -    /\bsed\s+-i/ and err 'sed -i is not portable';
>>>> +    /\bsed\s+-[^efn]\s+/ and err 'Not portable option with sed (use only [-n] [-e command] [-f command_file])';
>>>
>>> "sed -n -i -E -e 's/foo/bar/p'" won't be caught with this as an
>>> error, but that's OK ;-).
>>> Is this still an RFC patch?
>>
>> It seems as if everybody is happy with it,
>> so it may be ready for for pu.
>
> I'd still prefer to see a more terse[1] (and not capitalized) message
> to be consistent with existing error messages and to keep the reported
> errors more compact overall to make them easier to digest[2,3]:
>
>     err 'sed option not portable (use only -n, -e, -f)'
>

That's OK for me - lets see if there are more comments.


> But that's just a very minor nit.
>
> [1]: http://public-inbox.org/git/CAPig+cSeDNYFGYC2WznjW3zYMJCWZbZFY1KM5H5ir2L=Jxwy7w@mail.gmail.com/
> [2]: http://public-inbox.org/git/20180713055205.32351-3-sunshine@sunshineco.com/
> [3]: http://public-inbox.org/git/20180713055205.32351-4-sunshine@sunshineco.com/
>
Junio C Hamano Jan. 23, 2019, 5:54 p.m. UTC | #5 
Torsten Bögershausen <tboegi@web.de> writes:

>> I'd still prefer to see a more terse[1] (and not capitalized) message
>> to be consistent with existing error messages and to keep the reported
>> errors more compact overall to make them easier to digest[2,3]:
>>
>>     err 'sed option not portable (use only -n, -e, -f)'
>
> That's OK for me - lets see if there are more comments.

Thanks, both.  Eric's proposed message looks good to me too.
Torsten Bögershausen Jan. 25, 2019, 7:12 p.m. UTC | #6 
On Wed, Jan 23, 2019 at 09:54:25AM -0800, Junio C Hamano wrote:
> Torsten Bögershausen <tboegi@web.de> writes:
>
> >> I'd still prefer to see a more terse[1] (and not capitalized) message
> >> to be consistent with existing error messages and to keep the reported
> >> errors more compact overall to make them easier to digest[2,3]:
> >>
> >>     err 'sed option not portable (use only -n, -e, -f)'
> >
> > That's OK for me - lets see if there are more comments.
>
> Thanks, both.  Eric's proposed message looks good to me too.

Do you want to ammend the patch locally ?
Or should I send a new version ?
Junio C Hamano Jan. 27, 2019, 10:34 p.m. UTC | #7 
Torsten Bögershausen <tboegi@web.de> writes:

>> Thanks, both.  Eric's proposed message looks good to me too.
>
> Do you want to ammend the patch locally ?

I just amended it; thanks.
diff mbox series

Patch

diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b45bdac688..6c798608a9 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 'Not portable option with sed (use only [-n] [-e command] [-f command_file])';
 	/\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)';