[pacman-dev] (no subject)
Subject: re: [pacman-dev] [PATCH] pacdiff using pacman db v2 Reply-To: In-Reply-To: 51D1468A.3030209@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@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@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:-@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
On 02.07.2013 05:48, Jonathan Frazier wrote:
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.
I like making defaults have options since it allows to just append " (default)" to the option list and it allows to set an alias that for example uses locate and sets DIFF_PROG, but you are still able to easily switch to pacsave without having to change the alias or set DIFF_PROG again.
}
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.
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.
I believe that if you call a function in bash it doesn't matter where it was defined, it will always have access to all currently existing global variables. I don't know how local variables behave, Dave probably does.
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.
Not sure what you were after here that "find=1" and "locate=1" can't provide
+ 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.
Alright, not a big deal, I just like variables better because then you can look at installed script and see it's not a magic value.
+ 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
I don't feel strongly about the feature, but I do about it being out of scope of this patch. More about that below.
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?
Sounds reasonable since people probably run pacdiff after installing new packages and updatedb won't have been run in the meantime.
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.
"combining commits into one"? Does that mean you created multiple commits and then squashed them into one before sending the patch to the list? Anyway, please don't feel bad about sending 5 smaller patches to the list, it's actually easier to review small changes and it's not considered spamming (we had a thread with something like 20 or 30 patches at once and nobody complained). git send-email creates a proper thread and some of the patches might just be applied without much discussion. Also it will result in a way better history if you create one patch for each logical change especially if each patch has a proper subject and if applicable a summary of the reasons behind the change in the commit message. In your case I'd break this apart into (in roughly that order): "detect numbered pacsave files" (just for locate and find), "add quit hotkey", "prompt for updatedb when using locate", "rename DIFFSEARCHPATH" (I'm not really a fan of that, but for the sake of completeness), "improve performance/foo by using the pacman db" Each of these should get a commit message explaining why the change is done and a proper subject (mine are just quick examples).
On 03/07/13 00:31, Florian Pritz wrote:
In your case I'd break this apart into (in roughly that order): "detect numbered pacsave files" (just for locate and find), "add quit hotkey", "prompt for updatedb when using locate", "rename DIFFSEARCHPATH" (I'm not really a fan of that, but for the sake of completeness), "improve performance/foo by using the pacman db"
That is exactly what I was about to reply and ask for. Thanks, Allan
participants (3)
-
Allan McRae
-
Florian Pritz
-
Jonathan Frazier