[pacman-dev] (no subject)

[Florian Pritz]

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

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

-------------- 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/20130702/de5d1bf8/attachment.asc>