[pacman-dev] (no subject)
Jonathan Frazier
eyeswide at gmail.com
Mon Jul 1 23:48:32 EDT 2013
Subject: re: [pacman-dev] [PATCH] pacdiff using pacman db v2
Reply-To: In-Reply-To: 51D1468A.3030209 at xinu.at
On 07/01/13 at 11:06am, Florian Pritz wrote:
> 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.
Ok will do.
>
> 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.
>
First let me say that I am not stuck on this being the default option,
but it is the one I will use. I could say locate is inferior because it
finds pacsaves not in use. giving more noise and less signal. My
rational is more that it finds the active configs more throughly. Speed
is not the only factor. I will try and add more descriptive commit msgs.
> > 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.
>
Adding support for these extra pacsave files is a new feature and worth
noting in the changelog. this feature was requested in another thread. I
can make it a separate patch if the rest of this is going to take longer.
> 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).
>
I will try to be more descriptive in my changlog.
> >
> > 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.
>
DIFFSEARCHPATH is misleading and I find it difficult to read. the old name
implies it works for all search types, when it only applied to find.
Since nobody is defending find as the default, it should be changed
along with the default. Adding underscores was just cause I am fixing
it. Is DIFFFINDPATH more acceptable?
> > 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.
>
Does it make sense to have an option for the default? I don't think so.
Whatever the default ends up being it doesn't need a option.
> > + 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.
>
I was concerned about the scope of the variables it contains. as none
are not defined at that time. I don't remember if it was necessary or
just my correcting "bad" style from writing in other languages.
> > 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?
>
I didn't think to test if find or locate could not be found. I can add
that. The idea was to only define a variable for the active search type
and use that to define the mode later. using the path is convenient.
> > -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").
>
I will add a var for just the pacman db search. which will replace this.
> > + 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)
>
This code is what is used in bacman and pacscripts as others requested I
use as an example. What is the reason to make it a variable?
The only reason I could come up with for making it variable was to
support running in a alternate root. Adding such support seemed overly
complex given the current state of pacdiff.
I was thinking of adding this feature after converting the option
handling to allow options with associated strings. I don't see it as a
reason to hold up the rest of this code.
> > + 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).
>
again this was asked for in my previous submission. I will try and
document it better. the original subject contained all three notes, so
it was excessively long.
> > + 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).
>
I agree that q is not necessary, but I find it annoying that it is not
there. I have been thanked for adding it when sharing a previous version
of this code. At least it shows that that is a reasonable time to quit.
/shrug
> > while read c; do
> > case $c in
> > + q|Q) exit 1;;
>
> The user asked us to quit, not an error.
>
will fix, thanks
> > 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
> >
>
>
thanks for the comments. Do you think adding a prompt asking if pacdiff
should run updatedb when called with -l is a good idea?
Given the confusion brought by my misuse of git format-patch and
combining commits into one. If it is preferred I can go back to pushing
code to github and ask for further reviews when ready.
Jonathan
More information about the pacman-dev
mailing list