| Message ID | 1373667977-8034-1-git-send-email-chauplac@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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(-)