FS#16764 - pacdiff from pacman-contrib 3.3.1-1 doesn't find every *.pac{new;save;orig} file

Attached to Project: Pacman
Opened by Heiko Baums (cyberpatrol) - Wednesday, 21 October 2009, 11:11 GMT
Last edited by Xavier (shining) - Thursday, 05 November 2009, 17:17 GMT
Task Type Bug Report
Category Scripts & Tools
Status Closed
Assigned To Xavier (shining)
Architecture All
Severity Medium
Priority Normal
Reported Version 3.3.1
Due in Version 3.4.0
Due Date Undecided
Percent Complete 100%
Votes 0
Private No

Details

Description:
pacdiff from pacman-contrib 3.3.1-1 doesn't find every *.pac{new;save;orig} file except if it's run with the parameter -l, but then it doesn't find the new ones.

I think it's because of this line in the function cmd:
find /etc/ \( -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave \) -print0

It would be better if it would search the whole directory tree:
find / \( -name \*.pacnew -o -name \*.pacorig -o -name \*.pacsave \) -print0

It's because of the NoUpgrade option in /etc/pacman.conf, which can contain files in any directories. I e.g. set some .desktop files in /usr/share/applications to NoUpgrade to be able to reorganize my DE menu.

To find the new files with the -l parameter it would probably a good idea to add an optional updatedb or sudo updatedb before locate is run. It could be implemented by another parameter which could be given in addition to the -l parameter (e.g. pacdiff -l -u).
This task depends upon

Closed by  Xavier (shining)
Thursday, 05 November 2009, 17:17 GMT
Reason for closing:  Fixed
Additional comments about closing:  fixed by 6ed7d001f6
Comment by Allan McRae (Allan) - Wednesday, 21 October 2009, 11:37 GMT
  • Field changed: Category (Packages: Extra → Scripts & Tools)
  • Field changed: Reported Version ( → 3.3.1)
  • Field changed: Architecture (All → All)
  • Task reassigned to Allan McRae (Allan), Xavier (shining)
@Xavier: Tagged you as it looks like you were the last to edit that section.

I guess this is done as "find /" will be very, very slow. So it is a trade-off between a very long find operation and a locate one that requires updatedb to be run before-hand. I'm not sure there is a better option here...
Comment by Xavier (shining) - Wednesday, 21 October 2009, 11:50 GMT
1) find / is too slow
2) running updatedb is not the job of pacdiff

closing as "not a bug" ?
Comment by Allan McRae (Allan) - Wednesday, 21 October 2009, 11:58 GMT
I agree "not a bug" as it is the intended behaviour.
Comment by Heiko Baums (cyberpatrol) - Wednesday, 21 October 2009, 13:10 GMT
  • Field changed: Percent Complete (100% → 0%)
I don't think, that just closing a bug as "Not a bug" is a solution.

This is what pacdiff says: "pacdiff : a simple pacnew/pacorig/pacsave updater". And pacdiff could be pretty useful.

I know, that find / is pretty slow. But find /etc is not sufficient. Otherwise pacdiff is pretty useless.

Probably /etc/pacman.conf could be parsed for the NoUpgrade options and find could search these directories in addition to /etc. This would be faster than find /.
Comment by Xavier (shining) - Wednesday, 21 October 2009, 13:18 GMT
I find pacdiff VERY useful with find /etc. It is fast and catch 99% of the config changes, which are in /etc/.

If you start adding more directories, you end up adding them all, because config files can be put ANYWHERE. And this has nothing to do with NoUpgrade !
For example grub has a config in /boot, some other apps might have them in /usr/, some in /opt, it never ends..

The only compromise I see is adding a configuration option, something like this in pseudo-bash :
searchdir=SEARCHDIR:-/etc
find $searchdir ...

then you could do SEARCHDIR="/" pacdiff or SEARCHDIR="/etc/ /boot/ /usr" pacdiff or whatever makes you happy.

And now I just thought of a completely different approach, which would be to make use of $(pacman -Qii) output. I would have to experiment with this idea.
Comment by Heiko Baums (cyberpatrol) - Wednesday, 21 October 2009, 15:07 GMT
I like the idea with the $searchdir variable.
The problem with $(pacman -Qii) output is that it doesn't catch the NoUpgrade files.
Both together could be the ultimate solution.

$searchdir could probably be set in pacman.conf, commented by default.
Comment by Xavier (shining) - Wednesday, 21 October 2009, 15:20 GMT
No, it does not belong in pacman.conf at all. We only put pacman options there, never options for external tools.

It should be inside pacdiff, like diffprog :
20 diffprog=${DIFFPROG:-vimdiff}
and the default value can be overridden using the environment variable DIFFPROG

Do you want to try to implement searchdir in a similar way than diffprog ?
Otherwise I can probably do it.
Comment by Heiko Baums (cyberpatrol) - Wednesday, 21 October 2009, 15:59 GMT
Here it is.

I added a variable $diffsearchpath. The default value is /etc which can be changed by setting the environment variable DIFFSEARCHPATH="path1 path2 path3 ...". Separator is " " because of the syntax of find.
Comment by Xavier (shining) - Wednesday, 21 October 2009, 21:50 GMT
Looks fine to me, thanks, pushed to my working branch.
Comment by Heiko Baums (cyberpatrol) - Wednesday, 21 October 2009, 22:08 GMT
Maybe it would be good if both variables $DIFFPROG and $DIFFSEARCHPATH would be documented in the function usage, so that the average user knows these features.
Comment by Laszlo Papp (djszapi) - Saturday, 24 October 2009, 04:02 GMT
The best practice for patch sending to implement the documentation of it as well as the feature implementation.
Comment by Xavier (shining) - Saturday, 24 October 2009, 12:15 GMT
To Heiko : help for documentation is always very welcome.

Anyway, what about this ?

pacdiff : a simple pacnew/pacorig/pacsave updater
Usage : pacdiff [-l]
-l/--locate makes pacdiff use locate rather than find
DIFFPROG variable allows to override the default vimdiff
DIFFSEARCHPATH allows to override the default /etc path
Comment by Heiko Baums (cyberpatrol) - Saturday, 24 October 2009, 13:15 GMT
Sounds good. Maybe with an additional syntax explanation.

DIFFSEARCHPATH allows to override the default /etc path (Use " " as separator.)
or
DIFFSEARCHPATH allows to override the default /etc path ("path1 path2 path3 ...")
or something similar if you know how to explain it better.
Comment by Xavier (shining) - Saturday, 24 October 2009, 14:16 GMT
Good point. What about adding an example :
Usage : pacdiff [-l]
-l/--locate makes pacdiff use locate rather than find
DIFFPROG variable allows to override the default vimdiff
DIFFSEARCHPATH allows to override the default /etc path
Example : DIFFPROG=meld DIFFSEARCHPATH="/boot /etc /usr" pacdiff
Comment by Heiko Baums (cyberpatrol) - Saturday, 24 October 2009, 14:24 GMT
I think this looks perfect.

Loading...