[pacman-dev] [PATCH] contrib/pacdiff : big 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. 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 <shiningxc@gmail.com> --- 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 <aaronmgriffin@gmail.com> @@ -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
On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry <shiningxc@gmail.com> 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
On Wed, Feb 18, 2009 at 12:22 AM, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry <shiningxc@gmail.com> 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.
On Wed, Feb 18, 2009 at 1:41 AM, Xavier <shiningxc@gmail.com> wrote:
On Wed, Feb 18, 2009 at 12:22 AM, Aaron Griffin <aaronmgriffin@gmail.com> wrote:
On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry <shiningxc@gmail.com> 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
On Tue, Feb 17, 2009 at 4:44 PM, Xavier Chantry <shiningxc@gmail.com> 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 <shiningxc@gmail.com> --- 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 <aaronmgriffin@gmail.com> @@ -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
On Wed, Feb 18, 2009 at 1:03 AM, Dan McGee <dpmcgee@gmail.com> 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.
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 <shiningxc@gmail.com> --- 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 <aaronmgriffin@gmail.com> # @@ -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
On Wed, Feb 18, 2009 at 7:20 AM, Xavier Chantry <shiningxc@gmail.com> 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 <shiningxc@gmail.com> 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 <aaronmgriffin@gmail.com> # @@ -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
On Thu, Feb 19, 2009 at 2:23 AM, Dan McGee <dpmcgee@gmail.com> wrote:
On Wed, Feb 18, 2009 at 7:20 AM, Xavier Chantry <shiningxc@gmail.com> 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 <shiningxc@gmail.com> 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 <aaronmgriffin@gmail.com> # @@ -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.
participants (4)
-
Aaron Griffin
-
Dan McGee
-
Xavier
-
Xavier Chantry