[pacman-dev] [PATCH] contrib/pacdiff : big rework and cleanup

newer
[pacman-dev] [PATCH 2/4] repo-add...

older
[pacman-dev] [GIT] The official...

Xavier Chantry

18 Feb 2009 18 Feb '09

6:44 a.m.

I initially only wanted to add a -l/--locate option to use locate instead of find, which should have been easy. Then I thought I would try to support filename with whitespaces while I was at it, and this was a bit more complex. Then I asked for some help on #bash, and I received a lot of criticism on the script :) I tried to address them all, so hopefully that script is in a better state now. Signed-off-by: Xavier Chantry --- contrib/pacdiff | 64 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 39 insertions(+), 25 deletions(-) diff --git a/contrib/pacdiff b/contrib/pacdiff index 6493649..f9c29fb 100755 --- a/contrib/pacdiff +++ b/contrib/pacdiff @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # pacdiff : a simple pacnew/pacorig/pacsave updater for /etc/ # # Copyright (c) 2007 Aaron Griffin @@ -17,29 +17,43 @@ # along with this program. If not, see http://www.gnu.org/licenses/. # -# Original http://phraktured.net/config/bin/pacdiff - diffprog=${DIFFPROG:-vimdiff} -for x in $(find /etc/ -name "*.pacnew" -o -name "*.pacorig" -o -name "*.pacsave") -do - echo "File: ${x%.pac*}" - chk="$(cmp $x ${x%.pac*})" - if [ -z "${chk}" ]; then - echo " Files are identical, removing..." - rm $x - else - echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " - read c - c="$(echo $c| tr A-Z a-z)" #tolower - if [ "$c" = "r" ]; then - rm $x - elif [ "$c" = "s" ]; then - continue - else - [ "$c" = "n" -o "$c" = "N" ] || $diffprog $x ${x%.pac*} - echo -n " Remove file? [Y/n] " - read c - [ "$c" = "n" -o "$c" = "N" ] || rm $x - fi - fi + +case $1 in + -l|--locate) + cmd() { locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave; } + ;; + *) + cmd() { find /etc/ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -print0; } + ;; +esac + +exec 3< <(cmd) +while IFS= read -r -d '' pacfile <&3; do + file="${pacfile%.pac*}" + echo "File: $file" + if [ ! -f "$file" ]; then + echo " $file does not exist" + rm -i "$pacfile" + continue + fi + check="$(cmp "$pacfile" "$file")" + if [ -z "${check}" ]; then + echo " Files are identical, removing..." + rm "$pacfile" + else + echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " + while read c; do + case $c in + r|R) rm "$pacfile"; break ;; + v|V) + $diffprog "$pacfile" "$file" + rm -i "$pacfile"; break ;; + s|S) break ;; + *) echo -n " Invalid answer. Try again: [v/s/r] "; continue ;; + esac + done + fi done +exec 3>&- +exit 0 -- 1.6.1.3

Show replies by date

Aaron Griffin

18 Feb 18 Feb

7:22 a.m.

On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry wrote:

...

+exec 3< <(cmd) +while IFS= read -r -d '' pacfile <&3; do ... +exec 3>&-

Wow this looks ugly. Is the anything gained over using a construct like this: cmd | while read pacfile; do ... done

Xavier

3:41 p.m.

On Wed, Feb 18, 2009 at 12:22 AM, Aaron Griffin wrote:

...

On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry wrote:

...
+exec 3< <(cmd) +while IFS= read -r -d '' pacfile <&3; do ... +exec 3>&-

Wow this looks ugly. Is the anything gained over using a construct like this:

cmd | while read pacfile; do ... done

In practice, I guess not much. In theory, this is the only way to deal with any kind of filenames. -print0 True; print the full file name on the standard output, followed by a null character (instead of the newline character that -print uses). This allows file names that contain newlines or other types of white space to be correctly interpreted by pro‐ grams that process the find output. This option corresponds to the -0 option of xargs. And I believe that your construct cannot deal with this null character separation. Unfortunately that construct I used also required a redirection, because we still need to read the terminal input inside the loop (for read and vimdiff). The best is to use find -exec, but unfortunately locate does not support that.. Anyway, I am fine if you want to aim more for readability rather than correctness, it's very unlikely we are going to encounter weird filenames with this script. I can easily switch "cmd | while read pacfile; do", it has been an useful exercise for me anyway.

Aaron Griffin

19 Feb 19 Feb

12:35 a.m.

On Wed, Feb 18, 2009 at 1:41 AM, Xavier wrote:

...

On Wed, Feb 18, 2009 at 12:22 AM, Aaron Griffin wrote:

...
On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry wrote:

...
+exec 3< <(cmd) +while IFS= read -r -d '' pacfile <&3; do ... +exec 3>&-

Wow this looks ugly. Is the anything gained over using a construct like this:

cmd | while read pacfile; do ... done

In practice, I guess not much. In theory, this is the only way to deal with any kind of filenames.

-print0 True; print the full file name on the standard output, followed by a null character (instead of the newline character that -print uses). This allows file names that contain newlines or other types of white space to be correctly interpreted by pro‐ grams that process the find output. This option corresponds to the -0 option of xargs.

And I believe that your construct cannot deal with this null character separation. Unfortunately that construct I used also required a redirection, because we still need to read the terminal input inside the loop (for read and vimdiff). The best is to use find -exec, but unfortunately locate does not support that.. Anyway, I am fine if you want to aim more for readability rather than correctness, it's very unlikely we are going to encounter weird filenames with this script. I can easily switch "cmd | while read pacfile; do", it has been an useful exercise for me anyway.

That's why I don't use print0 for this sort of stuff. "while read line" will use newlines as separators. If you remove the -print0 and -0 args to find and locate (respectively), then my construct should work fine

Dan McGee

18 Feb 18 Feb

8:03 a.m.

On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry wrote:

...

I initially only wanted to add a -l/--locate option to use locate instead of find, which should have been easy.

Then I thought I would try to support filename with whitespaces while I was at it, and this was a bit more complex.

Then I asked for some help on #bash, and I received a lot of criticism on the script :) I tried to address them all, so hopefully that script is in a better state now.

I think some of the developers on our side are going to think differently than them. :)

...

Signed-off-by: Xavier Chantry --- contrib/pacdiff | 64 +++++++++++++++++++++++++++++++++--------------------- 1 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/contrib/pacdiff b/contrib/pacdiff index 6493649..f9c29fb 100755 --- a/contrib/pacdiff +++ b/contrib/pacdiff @@ -1,4 +1,4 @@ -#!/bin/sh +#!/bin/bash # pacdiff : a simple pacnew/pacorig/pacsave updater for /etc/ # # Copyright (c) 2007 Aaron Griffin @@ -17,29 +17,43 @@ # along with this program. If not, see http://www.gnu.org/licenses/. #

-# Original http://phraktured.net/config/bin/pacdiff - diffprog=${DIFFPROG:-vimdiff} -for x in $(find /etc/ -name "*.pacnew" -o -name "*.pacorig" -o -name "*.pacsave") -do - echo "File: ${x%.pac*}" - chk="$(cmp $x ${x%.pac*})" - if [ -z "${chk}" ]; then - echo " Files are identical, removing..." - rm $x - else - echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " - read c - c="$(echo $c| tr A-Z a-z)" #tolower - if [ "$c" = "r" ]; then - rm $x - elif [ "$c" = "s" ]; then - continue - else - [ "$c" = "n" -o "$c" = "N" ] || $diffprog $x ${x%.pac*} - echo -n " Remove file? [Y/n] " - read c - [ "$c" = "n" -o "$c" = "N" ] || rm $x - fi - fi + +case $1 in + -l|--locate) + cmd() { locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave; } + ;; + *) + cmd() { find /etc/ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -print0; } + ;; +esac Isn't this over-complex? It just seems like overkill to me. I'd rather have the argument searching be in one place, and then cmd() does an if/else based on the result of that (similar to makepkg, although they really seemed to hate on that in #bash).

...

+ +exec 3< <(cmd) This is a bit...sick. I agree with Aaron here.

...

+while IFS= read -r -d '' pacfile <&3; do + file="${pacfile%.pac*}" + echo "File: $file" + if [ ! -f "$file" ]; then + echo " $file does not exist" + rm -i "$pacfile" + continue + fi + check="$(cmp "$pacfile" "$file")" + if [ -z "${check}" ]; then + echo " Files are identical, removing..." + rm "$pacfile" + else + echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " + while read c; do + case $c in + r|R) rm "$pacfile"; break ;; + v|V) + $diffprog "$pacfile" "$file" + rm -i "$pacfile"; break ;; + s|S) break ;; + *) echo -n " Invalid answer. Try again: [v/s/r] "; continue ;; + esac + done + fi done +exec 3>&- +exit 0 --

The rest seems fine. -Dan

Xavier

3:46 p.m.

On Wed, Feb 18, 2009 at 1:03 AM, Dan McGee wrote:

...

...
+ +case $1 in + -l|--locate) + cmd() { locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave; } + ;; + *) + cmd() { find /etc/ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -print0; } + ;; +esac Isn't this over-complex? It just seems like overkill to me. I'd rather have the argument searching be in one place, and then cmd() does an if/else based on the result of that (similar to makepkg, although they really seemed to hate on that in #bash).

Oh no, in this case it's just me being very stupid. In the beginning I just had a simple variable : cmd="find ..." that I could override. I realized it didn't work, so I switched cmd to be a function. It didn't occur to me I could simply have an if/else inside cmd() rather than keep overriding it :)

...

...
+ +exec 3< <(cmd) This is a bit...sick. I agree with Aaron here.

I agree it's a bit sick, it would be interesting to know if there is a cleaner way that is still as correct. Anyway, see my answer to Aaron.

Xavier Chantry

9:20 p.m.

New subject: [pacman-dev] [PATCH] contrib/pacdiff : rework and cleanup

I initially only wanted to add a -l/--locate option to use locate instead of find, which should have been easy. Then I thought I would try to support filename with whitespaces while I was at it, and this was a bit more complex. The safest ways seem to be the following ones : http://mywiki.wooledge.org/BashFAQ/020 Then I received a lot of suggestions on #bash about how to improve the script, which I tried to address. Signed-off-by: Xavier Chantry --- contrib/pacdiff | 83 +++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 56 insertions(+), 27 deletions(-) diff --git a/contrib/pacdiff b/contrib/pacdiff index 6493649..4b2aa9e 100755 --- a/contrib/pacdiff +++ b/contrib/pacdiff @@ -1,5 +1,5 @@ -#!/bin/sh -# pacdiff : a simple pacnew/pacorig/pacsave updater for /etc/ +#!/bin/bash +# pacdiff : a simple pacnew/pacorig/pacsave updater # # Copyright (c) 2007 Aaron Griffin # @@ -17,29 +17,58 @@ # along with this program. If not, see http://www.gnu.org/licenses/. # -# Original http://phraktured.net/config/bin/pacdiff - diffprog=${DIFFPROG:-vimdiff} -for x in $(find /etc/ -name "*.pacnew" -o -name "*.pacorig" -o -name "*.pacsave") -do - echo "File: ${x%.pac*}" - chk="$(cmp $x ${x%.pac*})" - if [ -z "${chk}" ]; then - echo " Files are identical, removing..." - rm $x - else - echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " - read c - c="$(echo $c| tr A-Z a-z)" #tolower - if [ "$c" = "r" ]; then - rm $x - elif [ "$c" = "s" ]; then - continue - else - [ "$c" = "n" -o "$c" = "N" ] || $diffprog $x ${x%.pac*} - echo -n " Remove file? [Y/n] " - read c - [ "$c" = "n" -o "$c" = "N" ] || rm $x - fi - fi -done +locate=0 + +usage() { + echo "pacdiff : a simple pacnew/pacorig/pacsave updater" + echo "Usage : pacdiff [-l]" + echo "The -l/--locate flag makes pacdiff use locate rather than find" +} + +cmd() { + if [ $locate -eq 1 ]; then + locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave + else + find /etc/ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -print0 + fi +} + +if [ $# -gt 0 ]; then + case $1 in + -l|--locate) + locate=1;; + *) + usage; exit 0;; + esac +fi + +# see http://mywiki.wooledge.org/BashFAQ/020 +while IFS= read -u 3 -r -d '' pacfile; do + file="${pacfile%.pac*}" + echo "File: $file" + if [ ! -f "$file" ]; then + echo " $file does not exist" + rm -i "$pacfile" + continue + fi + check="$(cmp "$pacfile" "$file")" + if [ -z "${check}" ]; then + echo " Files are identical, removing..." + rm "$pacfile" + else + echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " + while read c; do + case $c in + r|R) rm "$pacfile"; break ;; + v|V) + $diffprog "$pacfile" "$file" + rm -i "$pacfile"; break ;; + s|S) break ;; + *) echo -n " Invalid answer. Try again: [v/s/r] "; continue ;; + esac + done + fi +done 3< <(cmd) + +exit 0 -- 1.6.1.3

Dan McGee

19 Feb 19 Feb

9:23 a.m.

New subject: [pacman-dev] [PATCH] contrib/pacdiff : rework and cleanup

On Wed, Feb 18, 2009 at 7:20 AM, Xavier Chantry wrote:

...

I initially only wanted to add a -l/--locate option to use locate instead of find, which should have been easy.

Then I thought I would try to support filename with whitespaces while I was at it, and this was a bit more complex. The safest ways seem to be the following ones : http://mywiki.wooledge.org/BashFAQ/020

Then I received a lot of suggestions on #bash about how to improve the script, which I tried to address.

Signed-off-by: Xavier Chantry Looks pretty good, one note inline. I've made the fix locally, so don't worry about resubmitting.

...

--- contrib/pacdiff | 83 +++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 56 insertions(+), 27 deletions(-)

diff --git a/contrib/pacdiff b/contrib/pacdiff index 6493649..4b2aa9e 100755 --- a/contrib/pacdiff +++ b/contrib/pacdiff @@ -1,5 +1,5 @@ -#!/bin/sh -# pacdiff : a simple pacnew/pacorig/pacsave updater for /etc/ +#!/bin/bash +# pacdiff : a simple pacnew/pacorig/pacsave updater # # Copyright (c) 2007 Aaron Griffin # @@ -17,29 +17,58 @@ # along with this program. If not, see http://www.gnu.org/licenses/. #

-# Original http://phraktured.net/config/bin/pacdiff - diffprog=${DIFFPROG:-vimdiff} -for x in $(find /etc/ -name "*.pacnew" -o -name "*.pacorig" -o -name "*.pacsave") -do - echo "File: ${x%.pac*}" - chk="$(cmp $x ${x%.pac*})" - if [ -z "${chk}" ]; then - echo " Files are identical, removing..." - rm $x - else - echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " - read c - c="$(echo $c| tr A-Z a-z)" #tolower - if [ "$c" = "r" ]; then - rm $x - elif [ "$c" = "s" ]; then - continue - else - [ "$c" = "n" -o "$c" = "N" ] || $diffprog $x ${x%.pac*} - echo -n " Remove file? [Y/n] " - read c - [ "$c" = "n" -o "$c" = "N" ] || rm $x - fi - fi -done +locate=0 + +usage() { + echo "pacdiff : a simple pacnew/pacorig/pacsave updater" + echo "Usage : pacdiff [-l]" + echo "The -l/--locate flag makes pacdiff use locate rather than find" +} + +cmd() { + if [ $locate -eq 1 ]; then + locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave + else + find /etc/ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -print0 This didn't work out of the box. I had to use grouping to get it to work correctly: find /etc/ $ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave $ -print0

...

+ fi +} + +if [ $# -gt 0 ]; then + case $1 in + -l|--locate) + locate=1;; + *) + usage; exit 0;; + esac +fi + +# see http://mywiki.wooledge.org/BashFAQ/020 +while IFS= read -u 3 -r -d '' pacfile; do + file="${pacfile%.pac*}" + echo "File: $file" + if [ ! -f "$file" ]; then + echo " $file does not exist" + rm -i "$pacfile" + continue + fi + check="$(cmp "$pacfile" "$file")" + if [ -z "${check}" ]; then + echo " Files are identical, removing..." + rm "$pacfile" + else + echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " + while read c; do + case $c in + r|R) rm "$pacfile"; break ;; + v|V) + $diffprog "$pacfile" "$file" + rm -i "$pacfile"; break ;; + s|S) break ;; + *) echo -n " Invalid answer. Try again: [v/s/r] "; continue ;; + esac + done + fi +done 3< <(cmd) + +exit 0 -- 1.6.1.3

_______________________________________________ pacman-dev mailing list pacman-dev@archlinux.org http://www.archlinux.org/mailman/listinfo/pacman-dev

Xavier

3:58 p.m.

New subject: [pacman-dev] [PATCH] contrib/pacdiff : rework and cleanup

On Thu, Feb 19, 2009 at 2:23 AM, Dan McGee wrote:

...

On Wed, Feb 18, 2009 at 7:20 AM, Xavier Chantry wrote:

...
I initially only wanted to add a -l/--locate option to use locate instead of find, which should have been easy.

Then I thought I would try to support filename with whitespaces while I was at it, and this was a bit more complex. The safest ways seem to be the following ones : http://mywiki.wooledge.org/BashFAQ/020

Then I received a lot of suggestions on #bash about how to improve the script, which I tried to address.

Signed-off-by: Xavier Chantry Looks pretty good, one note inline. I've made the fix locally, so don't worry about resubmitting.

...
--- contrib/pacdiff | 83 +++++++++++++++++++++++++++++++++++++------------------ 1 files changed, 56 insertions(+), 27 deletions(-)

diff --git a/contrib/pacdiff b/contrib/pacdiff index 6493649..4b2aa9e 100755 --- a/contrib/pacdiff +++ b/contrib/pacdiff @@ -1,5 +1,5 @@ -#!/bin/sh -# pacdiff : a simple pacnew/pacorig/pacsave updater for /etc/ +#!/bin/bash +# pacdiff : a simple pacnew/pacorig/pacsave updater # # Copyright (c) 2007 Aaron Griffin # @@ -17,29 +17,58 @@ # along with this program. If not, see http://www.gnu.org/licenses/. #

-# Original http://phraktured.net/config/bin/pacdiff - diffprog=${DIFFPROG:-vimdiff} -for x in $(find /etc/ -name "*.pacnew" -o -name "*.pacorig" -o -name "*.pacsave") -do - echo "File: ${x%.pac*}" - chk="$(cmp $x ${x%.pac*})" - if [ -z "${chk}" ]; then - echo " Files are identical, removing..." - rm $x - else - echo -n " File differences found. (V)iew, (S)kip, (R)emove: [v/s/r] " - read c - c="$(echo $c| tr A-Z a-z)" #tolower - if [ "$c" = "r" ]; then - rm $x - elif [ "$c" = "s" ]; then - continue - else - [ "$c" = "n" -o "$c" = "N" ] || $diffprog $x ${x%.pac*} - echo -n " Remove file? [Y/n] " - read c - [ "$c" = "n" -o "$c" = "N" ] || rm $x - fi - fi -done +locate=0 + +usage() { + echo "pacdiff : a simple pacnew/pacorig/pacsave updater" + echo "Usage : pacdiff [-l]" + echo "The -l/--locate flag makes pacdiff use locate rather than find" +} + +cmd() { + if [ $locate -eq 1 ]; then + locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave + else + find /etc/ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -print0 This didn't work out of the box. I had to use grouping to get it to work correctly: find /etc/ $ -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave $ -print0

Arf sorry, probably my zsh shell would played me tricks again, I should always test with bash.

5552

Age (days ago)

5554

Last active (days ago)

List overview

Download

8 comments

4 participants

participants (4)

Aaron Griffin
Dan McGee
Xavier
Xavier Chantry