From: Omar Polo <op@omarpolo.com>
Subject: Re: regress: Replace sed -i with ed for portability
To: Christian Weisgerber <naddy@mips.inka.de>
Cc: gameoftrees@openbsd.org
Date: Sun, 05 Mar 2023 09:10:47 +0100

On 2023/03/04 21:34:05 +0100, Christian Weisgerber <naddy@mips.inka.de> wrote:
> Thomas and I would like to replace the ubiquitous use of "sed -i"
> in the regression tests with ed(1).
> 
> "sed -i" is fundamentally unportable.  The implementations that
> support the -i option for in-place editing differ in a crucial
> aspect: GNU and OpenBSD sed(1) treat the extension for the backup
> file as an optional argument and use "sed -i" for no backup file.
> FreeBSD sed(1) treats the extension as an obligatory argument and
> uses "sed -i ''" for no backup file.  There is no single syntax
> that works for both.  Handling this in -portable is painful.
> 
> Apart from calling "sed -i" directly, a few regression tests also
> create shell scripts that contain "sed -i" and are executed later.
> 
> I'm attaching a model diff that replaces all instances of "sed -i"
> with ed.  Take a look, see if you like it.  For the script use,
> check commit.sh or histedit.sh.  If the use of "EOF" to terminate
> nested here documents is too confusing, we can use a different
> delimiter for the inner one.  (Any suggestions?)

The fact that you used EOF in the outer heredoc and -EOF for the
inner, plus the fact that are just a couple of lines, makes it
perfectly readable IMHO.

> We can tweak this to taste, because I used a Perl script to generate
> the diff, which I'm also attaching.
> 
> Testing this has turned up an oddity: test_commit_prepared_logmsg()
> in commit.sh produces a script that performs s/foo/bar/ on a file,
> but that file doesn't contain the string "foo", so no replacement
> happens.  Unlike sed(1), ed(1) treats this as an error.  What is
> that part of the test supposed to accomplish?

The "editor" script shouldn't do anything, it's a test to see if 'got
commit' uses the log message prepared in $testroot/logmsg if I'm
reading this correctly.  it doesn't need sed at all, just a no-op
script.  if this is too cryptic we can replace it with an "exec true"
or something along those lines:

diff /home/op/w/got
commit - b43e02dae2f94bde4c5ea9a51f71720f7713019e
path + /home/op/w/got
blob - 53011f6f6351ec5e36a888fac308c8d4f0f0002e
file + regress/cmdline/commit.sh
--- regress/cmdline/commit.sh
+++ regress/cmdline/commit.sh
@@ -1589,10 +1589,8 @@ test_commit_prepared_logmsg() {
 
 	echo 'test commit_prepared_logmsg' > $testroot/logmsg
 
-	cat > $testroot/editor.sh <<EOF
-#!/bin/sh
-sed -i 's/foo/bar/' "\$1"
-EOF
+	# a no-op editor script
+	> $testroot/editor.sh
 	chmod +x $testroot/editor.sh
 
 	(cd $testroot/wt && env VISUAL="$testroot/editor.sh" \


There are a few places in your diff where we run ed multiple times on
the same file, for e.g.

> -	sed -i 's/2/22/' $testroot/repo/numbers
> -	sed -i 's/8/33/' $testroot/repo/numbers
> +	ed -s $testroot/repo/numbers <<-\EOF
> +	,s/2/22/
> +	w
> +	EOF
> +	ed -s $testroot/repo/numbers <<-\EOF
> +	,s/8/33/
> +	w
> +	EOF

these could be folded in a single ed invocation; however this is not
an issue.

ok op@ for the diff as-is, folding multiple ed invocation can be done
as a separate step if you prefer (or I can do a sweep after this
lands.)