[pacman-dev] [PATCH] New feature: files verification

[changaco]

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.