[pacman-dev] [PATCH] New feature: files verification
changaco
changaco at laposte.net
Mon Mar 30 17:51:46 EDT 2009
Dan McGee wrote :
> On Sun, Mar 29, 2009 at 3:50 PM, changaco <changaco at laposte.net> wrote:
> > +*-k \--check*::
> > + Check that the files owned by the package(s) are present on the system,
> > + that they haven't been deleted. Given no argument it will check all the
> > + packages installed.
> Redundant language, but your native language is not English, so no worries.
>
> How about:
> Check that all files owned by the given packages(s) are present on the
> system. If packages are not specified, check all installed packages.
>
Works for me, changed in the new patch.
> > @@ -510,7 +515,9 @@ static int parseargs(int argc, char *argv[])
> > config->flags |= PM_TRANS_FLAG_DOWNLOADONLY;
> > config->flags |= PM_TRANS_FLAG_NOCONFLICTS;
> > break;
> > - case 'y': (config->op_s_sync)++; break;
> > + case 'y':
> > + (config->op_s_sync)++;
> > + break;
> Unnecessary change now, let's just revert these to the original line.
>
That's right, changed too.
> > + if(strncmp(f, "/tmp", 4)==0){
> > + continue; // ignore files in /tmp
> > + }
> Did you explain why this is necessary? I think all special case
> handling should be dropped, and this check is not correct anyway as it
> assumes a root of '/'.
>
I fixed that with the same method you used for the root in your patch.
Should I remove the test or not ?
> > + FREELIST(damaged);
> You get your double free() because you definitely don't want to be
> using this macro. What this macro does is free both the list objects
> AND their contents. However, when you build the list, you are not
> duplicating the list contents, so freeing the list contents here is
> something we don't want to do- we only want to free the list itself.
> This line should just be a:
>
> alpm_list_free(damaged);
>
> > if(filter(pkg)) {
> > - display(pkg);
> > + if(config->op_q_check){
> > + pkgs = alpm_list_add(pkgs, pkg);
> > + }
> > + else{
> Add a space before the { and put the "} else {" on one line.
>
> > + if(config->op_q_check){
> > + check(pkgs);
> > + }
> > +
> > + FREELIST(pkgs);
> s/FREELIST/alpm_list_free/
> And this free can really be moved inside the if() statement right
> above it, as any adds are protected by the same case.
>
All fixed according to your patch.
> > + display(pkg);
> > + }
> > }
> So with this logic, we end up calling filter() twice for every
> package, as you also called it in the check() function above. I
> believe that is not necessary, and the filter() call in check should
> be dropped. However, that breaks the no explicit packages logic.
>
> I'm not sure how you want to fix or clean it up, but we shouldn't filter twice.
>
> A few random tests:
>
> dmcgee at galway ~/projects/pacman (master)
> $ ./src/pacman/pacman -Qtdk
> Check complete: 1296 files (693 packages)
>
> The packages count is definitely wrong there, and looks like it is
> wrong in several other filter tests (e.g. pacman -Qtek).
>
> I'm still not a fan of the flickering progress bar. After you make a
> few of these other adjustments I may see what I can come up with.
>
Problem fixed. See the new patch for details.
More information about the pacman-dev
mailing list