[Bash-completion-devel] Patch review needed

[Ville Skyttä]

On Friday 18 September 2009, Guillaume Rousse wrote:
> David Paleino a écrit :
> > http://cvs.pld-linux.org/cgi-bin/cvsweb.cgi/packages/bash-completion/bash
> >-completion-rpm-cache.patch
[...]
> 
> The patches actually changes 3 things:
> 1) it uses an additional /var/cache/hrmib, which seems to be jbj-version
> specific, if available
> 2) it forces refreshing the dumped package list if writable
> 3) it changes the format of completed package names

...and a 4th, it removes the "ver" local variable, which I believe is fine as 
it seems unused.

> 1 and 3 are OK for me.
> 
> I'm more reserved about 2, tough. /var/log/rpmpkgs is not really a bash
> completion specific file, rather some daily dump of the package
> database. I don't think it really hurt to rebuild it, but so far
> completions never changed the system state. Anyway, this cache
> refreshing should only occurs if cache is outdated (in the else part of
> the last test), not everytime you complete on package list.

I think 1) and 4) are ok, although I have no clue about 1).  But before 3) is 
done, I don't think 1) can be applied as is.

I do object to 2).  This is changing the system state, and screws existing 
functionality if people rely on diffing various rotated /var/log/rpmpkgs* 
files to see what packages have been removed/installed/updated between log 
rotations.  /var/log/rpmpkgs is created by /etc/cron.daily/rpm daily at least 
on Fedora and CentOS.  Caching the info somewhere else would be fine with me 
though (e.g. /var/cache/bash-completion/rpmpkgs), but /var/log/rpmpkgs should 
not be mucked with.

I also have some reservations about 3).  _rpm_installed_packages is used at 
least by apt in bash-completion, as well as rpmlint and rpmdevtools (both 
upstream, not in bash-completion) completions, and I'm not at all sure they 
all handle the name-version-release.arch format.  And I'm not sure if that 
format buys us much at all - I do see the value in name.arch but the rest seem 
pretty much just noise to me (and if one wants it to be as complete as 
possible, the format is missing epoch, BTW).

So I would suggest starting with applying 4) and 1), with 1) modified so that 
it works on plain package names.  Then, with more testing perhaps apply 3) 
later (but I'm inclined to use name.arch for it only), and maybe use a 
different function name than _rpm_installed_packages or args for it in order 
to not screw existing completions out there.