Subject: [pacman-dev] [PATCH] pacdiff using pacman db v2 Please move that v2 into the PATCH tag like so: [PATCH v2]. Otherwise it sounds like you use "version 2 of the pacman db" and it might end up in the final commit. I'd also suggest to change the subject to something like "pacdiff: use pacman db for lookups to increase performance" so it's more obvious what this patch does and why. Still missing the "why is using that faster/better than locate", but that can be in the commit message. On 01.07.2013 02:00, Jonathan Frazier wrote:

Implement the default search using the local pacman db.

I'm not sure the default should switch to an inferior method (inferior because it doesn't find everything). updatedb might be too slow on certain hardware or whatever, but I think that's a minority so we should stick with locate. Also that way long time users won't have left over .pacsaves.

Use @sysconfdir@/pacman.conf to find the local db Search for pacsave.0 files.

You also search for .pacsave, .pacsave.1, ..., .pacnew, .pacorig. No need to mention this explicitly here. Also the comment about pacman.conf doesn't belong here unless you explain why you do it this way and you have a good reason not to do anything else. Actually none of the 3 lines explain why any of this is done. I know these are the changes you made compared to the previous patch, but such changes belong to the comment section below (between the "---" marker and the list of changed files) not into the commit message. All I remember (now) is that you do this because updatedb is slow on some low end hardware, but that should really be mentioned here because I might forget (or am I already wrong? I honestly don't know) and others won't even have known in the first place and someone might care when they look through the commit log in a few years to find why a certain feature appeared (been there, done that).

Signed-off-by: Jonathan Frazier <eyeswide@gmail.com> --- contrib/pacdiff.sh.in | 86 ++++++++++++++++++++++++++++++++++++++------------- 1 file changed, 64 insertions(+), 22 deletions(-)

diff --git a/contrib/pacdiff.sh.in b/contrib/pacdiff.sh.in index 47779d6..ee80c9c 100644 --- a/contrib/pacdiff.sh.in +++ b/contrib/pacdiff.sh.in @@ -21,9 +21,8 @@ declare -r myname='pacdiff' declare -r myver='@PACKAGE_VERSION@'

-diffprog=${DIFFPROG:-vimdiff} -diffsearchpath=${DIFFSEARCHPATH:-/etc} -locate=0 +diffprog=${DIFF_PROG:-vimdiff} +difffindpath="${DIFF_FIND_PATH:-/etc}"

Out of scope of this patch.

USE_COLOR='y'

m4_include(../scripts/library/output_format.sh) @@ -32,11 +31,19 @@ m4_include(../scripts/library/term_colors.sh)

usage() { echo "$myname : a simple pacnew/pacorig/pacsave updater" - echo "Usage : $myname [-l]" - echo " -l/--locate makes $myname use locate rather than find" - echo " DIFFPROG variable allows to override the default vimdiff" - echo " DIFFSEARCHPATH allows to override the default /etc path" - echo "Example : DIFFPROG=meld DIFFSEARCHPATH=\"/boot /etc /usr\" $myname" + echo "" + echo "Usage : $myname [-lf]" + echo " -l/--locate makes $myname search using locate" + echo " -f/--find makes $myname search using find"

Add an option for your pacsave method. Maybe -p.

+ echo "" + echo "Note: the default search looks for backup files in the local pacman db" + echo " this generally will not find *.pacsave files" + echo "" + echo " DIFF_PROG variable will override the default editor: vimdiff" + echo " DIFF_FIND_PATH will override the default path when using --find" + echo "" + echo "Example : DIFF_PROG=meld $myname" + echo "Example : DIFF_FIND_PATH=\"/boot /etc /usr\" $myname -f"

Out of scope.

}

version() { @@ -45,27 +52,61 @@ version() { echo 'Copyright (C) 2013 Pacman Development Team <pacman-dev@archlinux.org>' }

-cmd() { - if [ $locate -eq 1 ]; then - locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave - else - find $diffsearchpath \( -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave \) -print0 - fi -} -

Any reason you moved cmd()? Makes reviewing the changes harder.

if [ $# -gt 0 ]; then case $1 in -l|--locate) - locate=1;; + locate=$(type -P locate);; + -f|--find) + find=$(type -P find);;

Why did you change that to do a path lookup and not exit if you can't find it?

-V|--version) - version; exit 0;; + version; exit 0;; -h|--help) - usage; exit 0;; + usage; exit 0;; *) - usage; exit 1;; + usage; exit 1;; esac fi

+if [[ ! -n $locate && ! -n $find ]]; then

Don't do that. Someone might add a 4th option in the future and then they have to adjust everything again. Just use a $mode variable and set it to a default or to the last specified option. I'd also use strings here for easier understanding when you use them in conditions ("find", "locate", "pacsave").

+ if [[ ! -r @sysconfdir@/pacman.conf ]]; then + error "unable to read @sysconfdir@/pacman.conf" + usage; exit 1 + fi + + eval $(awk '/DBPath/ {print $1$2$3}' @sysconfdir@/pacman.conf)

Make sysconfdir a variable like myver, don't hardcode it directly. (same for localstatedir below)

+ pac_db="${DBPath:-@localstatedir@/lib/pacman/}local" + if [[ ! -d "${pac_db}" ]]; then + error "unable to read pacman db %s". "${pac_db}" + usage; exit 1 + fi +fi + +check_backup() { + [[ -f "$1" ]] && printf "$1"'\0' +} + +cmd() { + if [[ -n $locate ]]; then + locate -0 -e -b \*.pacnew \*.pacorig \*.pacsave '*.pacsave.[0-9]' + elif [[ -n $find ]]; then + find "${difffindpath}" \( -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave -o -name '*.pacsave.[0-9]' \) -print0 2> /dev/null

.pacsave pattern change is out of scope. This isn't a logical change given the patch subject. Better to split that into it's own patch (before this patch so it can be merge independently).

+ else + # parse local pacman db for backup files and look for pac* based on them + awk '/^%BACKUP%$/ { + while (getline) { + if (/^$/) { nextfile } + print $1 + } + }' "${pac_db}"/*/files | while read -r bkup; do + check_backup "/$bkup.pacnew" + check_backup "/$bkup.pacorig" + for f in "/$bkup.pacsave"{,.{0..9}}; do + check_backup "$f" + done + done + fi +} + # see http://mywiki.wooledge.org/BashFAQ/020 while IFS= read -u 3 -r -d '' pacfile; do file="${pacfile%.pac*}" @@ -82,16 +123,17 @@ while IFS= read -u 3 -r -d '' pacfile; do msg2 "Files are identical, removing..." rm -v "$pacfile" else - ask "(V)iew, (S)kip, (R)emove %s, (O)verwrite with %s: [v/s/r/o] " "$file_type" "$file_type" + ask "(V)iew, (S)kip, (R)emove %s, (O)verwrite with %s, (Q)uit: [v/s/r/o/q] " "$file_type" "$file_type"

Out of the scope of this patch and IMHO unnecessary (^C is enough).

while read c; do case $c in + q|Q) exit 1;;

The user asked us to quit, not an error.

r|R) rm -v "$pacfile"; break ;; o|O) mv -v "$pacfile" "$file"; break ;; v|V) $diffprog "$pacfile" "$file" rm -iv "$pacfile"; break ;; s|S) break ;; - *) ask "Invalid answer. Try again: [v/s/r/o] "; continue ;; + *) ask "Invalid answer. Try again: [v/s/r/o/q] "; continue ;; esac done fi