[pacman-dev] [PATCH] pacdiff using pacman db v2

[Florian Pritz]

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 at 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 at 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:- at 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
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://mailman.archlinux.org/pipermail/pacman-dev/attachments/20130701/925ed8d5/attachment.asc>