[PATCH v2] scripts/config: use sed's POSIX interface

[PATCH v2] scripts/config: use sed's POSIX interface

[Clement Chauplannaz]

Script `config' relies on extensions of `GNU sed', and is thus not
working on all Unixes:
- in-place edition of files (-i), which can be replaced with
a temporary file;
- extended-regexps (-r), which can be split into basic regexps;
- single-line calls to `a' command, while some implementations
require a leading newline before the parameter.

Rewrite calls to `sed' to comply with POSIX interface, and move them
to helper functions.

Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
---
Changes in v2:
- ANSI C style quoting to produce newlines ($'\n') is replaced with
  printf's, not to introduce further dependency on bash.
- helper functions are introduced to wrap calls to `sed'.

 scripts/config | 41 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)

diff --git a/scripts/config b/scripts/config
index a65ecbb..a01bc11 100755
--- a/scripts/config
+++ b/scripts/config
@@ -60,15 +60,48 @@ checkarg() {
 	fi
 }
 
+txt_append() {
+	local anchor="$1"
+	local insert="$2"
+	local infile="$3"
+	local outfile="$4"
+
+	# sed append cmd: 'a\' + newline + text + newline
+	cmd="$(printf "a\\%b$insert%b" "\n" "\n")"
+
+	sed -e "/$anchor/$cmd" "$infile" >"$outfile"
+}
+
+txt_subst() {
+	local before="$1"
+	local after="$2"
+	local infile="$3"
+	local outfile="$4"
+
+	sed -e "s/$before/$after/" "$infile" >"$outfile"
+}
+
+txt_delete() {
+	local text="$1"
+	local infile="$2"
+	local outfile="$3"
+
+	sed -e "/$text/d" "$infile" >"$outfile"
+}
+
 set_var() {
 	local name=$1 new=$2 before=$3
 
 	name_re="^($name=|# $name is not set)"
 	before_re="^($before=|# $before is not set)"
 	if test -n "$before" && grep -Eq "$before_re" "$FN"; then
-		sed -ri "/$before_re/a $new" "$FN"
+		txt_append "^$before=" "$new" "$FN" "$FN.swp"
+		txt_append "^# $before is not set" "$new" "$FN.swp" "$FN"
+		rm "$FN.swp"
 	elif grep -Eq "$name_re" "$FN"; then
-		sed -ri "s:$name_re.*:$new:" "$FN"
+		txt_subst "^$name=.*" "$new" "$FN" "$FN.swp"
+		txt_subst "^# $name is not set" "$new" "$FN.swp" "$FN"
+		rm "$FN.swp"
 	else
 		echo "$new" >>"$FN"
 	fi
@@ -77,7 +110,9 @@ set_var() {
 undef_var() {
 	local name=$1
 
-	sed -ri "/^($name=|# $name is not set)/d" "$FN"
+	txt_delete "^$name=" "$FN" "$FN.swp"
+	txt_delete "^# $name is not set" "$FN.swp" "$FN"
+	rm "$FN.swp"
 }
 
 if [ "$1" = "--file" ]; then
-- 
1.8.3.rc1.44.gb387c77.dirty

Re: [PATCH v2] scripts/config: use sed's POSIX interface

[Yann E. MORIN]

Clément, All,

On 2013-07-13 00:26 +0200, Clement Chauplannaz spake thusly:
> Script `config' relies on extensions of `GNU sed', and is thus not
> working on all Unixes:
> - in-place edition of files (-i), which can be replaced with
> a temporary file;
> - extended-regexps (-r), which can be split into basic regexps;
> - single-line calls to `a' command, while some implementations
> require a leading newline before the parameter.

Just nit-picking on the commit log: can you indent the bullet-list, and
the continuation lines, so it is easier to read, like:
  - in-place edition of files (-i), which can be replaced with
    a temporary file;

> Rewrite calls to `sed' to comply with POSIX interface, and move them
> to helper functions.
> 
> Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
[--SNIP--]
> +txt_append() {
> +	local anchor="$1"
> +	local insert="$2"
> +	local infile="$3"
> +	local outfile="$4"
> +
> +	# sed append cmd: 'a\' + newline + text + newline
> +	cmd="$(printf "a\\%b$insert%b" "\n" "\n")"

It seems I've managed the same with a simpler construct:
    cmd="$(printf "a$insert\n")"

> +	sed -e "/$anchor/$cmd" "$infile" >"$outfile"

Why don't you handle the temp file in the helpers?
(OK, I got it later, see below.)

[--SNIP--]
>  set_var() {
>  	local name=$1 new=$2 before=$3
>  
>  	name_re="^($name=|# $name is not set)"
>  	before_re="^($before=|# $before is not set)"
>  	if test -n "$before" && grep -Eq "$before_re" "$FN"; then
> -		sed -ri "/$before_re/a $new" "$FN"
> +		txt_append "^$before=" "$new" "$FN" "$FN.swp"
> +		txt_append "^# $before is not set" "$new" "$FN.swp" "$FN"

What about doing it in a single run:
    txt_append "^\($before=\|# $before is not set\)" [...]

> +		rm "$FN.swp"

OK, this is sneaky. Since you use two calls, you use arg-swapping in the
second call so that the temp file is the source and the actual file is
the destination. That made me wonder why on Earth you would just 'rm'
that tempfile without doing anything with it. Sneaky, I almost missed
that. At least, put in a comment when using such tricks.

Besides, that won't work if you do it all in one pass, as shown above.

So:
  - do the munging in one pass
  - move handling the temp file in the helpers

>  	elif grep -Eq "$name_re" "$FN"; then
> -		sed -ri "s:$name_re.*:$new:" "$FN"
> +		txt_subst "^$name=.*" "$new" "$FN" "$FN.swp"
> +		txt_subst "^# $name is not set" "$new" "$FN.swp" "$FN"
> +		rm "$FN.swp"

Ditto (one pass & temp file).

>  	else
>  		echo "$new" >>"$FN"
>  	fi
> @@ -77,7 +110,9 @@ set_var() {
>  undef_var() {
>  	local name=$1
>  
> -	sed -ri "/^($name=|# $name is not set)/d" "$FN"
> +	txt_delete "^$name=" "$FN" "$FN.swp"
> +	txt_delete "^# $name is not set" "$FN.swp" "$FN"
> +	rm "$FN.swp"

Ditto (one pass &  temp file).

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

Re: [PATCH v2] scripts/config: use sed's POSIX interface

[Clément Chauplannaz]

Yann,

Thanks (once more!) for the careful review. See my answers inline.

2013/7/13 Yann E. MORIN <yann.morin.1998@free.fr>:
> Clément, All,
>
> On 2013-07-13 00:26 +0200, Clement Chauplannaz spake thusly:
>> Script `config' relies on extensions of `GNU sed', and is thus not
>> working on all Unixes:
>> - in-place edition of files (-i), which can be replaced with
>> a temporary file;
>> - extended-regexps (-r), which can be split into basic regexps;
>> - single-line calls to `a' command, while some implementations
>> require a leading newline before the parameter.
>
> Just nit-picking on the commit log: can you indent the bullet-list, and
> the continuation lines, so it is easier to read, like:
>   - in-place edition of files (-i), which can be replaced with
>     a temporary file;
Ack. Applied.

>> Rewrite calls to `sed' to comply with POSIX interface, and move them
>> to helper functions.
>>
>> Signed-off-by: Clement Chauplannaz <chauplac@gmail.com>
> [--SNIP--]
>> +txt_append() {
>> +     local anchor="$1"
>> +     local insert="$2"
>> +     local infile="$3"
>> +     local outfile="$4"
>> +
>> +     # sed append cmd: 'a\' + newline + text + newline
>> +     cmd="$(printf "a\\%b$insert%b" "\n" "\n")"
>
> It seems I've managed the same with a simpler construct:
>     cmd="$(printf "a$insert\n")"
This does work with GNU-sed, but not with BSD-sed which does require
separating the command character 'a' from its argument with a
backslash (as per sed's POSIX definition).

>> +     sed -e "/$anchor/$cmd" "$infile" >"$outfile"
>
> Why don't you handle the temp file in the helpers?
> (OK, I got it later, see below.)
This makes it clearer indeed. Applied.

>
> [--SNIP--]
>>  set_var() {
>>       local name=$1 new=$2 before=$3
>>
>>       name_re="^($name=|# $name is not set)"
>>       before_re="^($before=|# $before is not set)"
>>       if test -n "$before" && grep -Eq "$before_re" "$FN"; then
>> -             sed -ri "/$before_re/a $new" "$FN"
>> +             txt_append "^$before=" "$new" "$FN" "$FN.swp"
>> +             txt_append "^# $before is not set" "$new" "$FN.swp" "$FN"
>
> What about doing it in a single run:
>     txt_append "^\($before=\|# $before is not set\)" [...]
This also works with GNU-sed, but not with BSD-sed. Alternation
(matching either one of the strings separated by the vertical line) is
not supported in Basic Regular Expressions; this is a GNU-sed
extension.

>> +             rm "$FN.swp"
>
> OK, this is sneaky. Since you use two calls, you use arg-swapping in the
> second call so that the temp file is the source and the actual file is
> the destination. That made me wonder why on Earth you would just 'rm'
> that tempfile without doing anything with it. Sneaky, I almost missed
> that. At least, put in a comment when using such tricks.
>
> Besides, that won't work if you do it all in one pass, as shown above.
>
> So:
>   - do the munging in one pass
Cannot apply, for the reasons exposed above.
>   - move handling the temp file in the helpers
Ack. Done.

>>       elif grep -Eq "$name_re" "$FN"; then
>> -             sed -ri "s:$name_re.*:$new:" "$FN"
>> +             txt_subst "^$name=.*" "$new" "$FN" "$FN.swp"
>> +             txt_subst "^# $name is not set" "$new" "$FN.swp" "$FN"
>> +             rm "$FN.swp"
>
> Ditto (one pass & temp file).
Ack.
>
>>       else
>>               echo "$new" >>"$FN"
>>       fi
>> @@ -77,7 +110,9 @@ set_var() {
>>  undef_var() {
>>       local name=$1
>>
>> -     sed -ri "/^($name=|# $name is not set)/d" "$FN"
>> +     txt_delete "^$name=" "$FN" "$FN.swp"
>> +     txt_delete "^# $name is not set" "$FN.swp" "$FN"
>> +     rm "$FN.swp"
>
> Ditto (one pass &  temp file).
Ack.
>
> Regards,
> Yann E. MORIN.

I will send a v3 with the changes listed above.

Thanks & regards,
Clement