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).