[pacman-dev] Fwd: [BUG] chk_filedifference is buggy!!

[Dan McGee]

2008/2/11 Nagy Gabor <ngaba at bibl.u-szeged.hu>:
> > Nagy, we've discussed this before in the past. *Please* do not just
> > point out problems with the expectation that we will fix it. I know
> > you are a very competent coder, and I'd appreciate you contributing
> > solutions rather than inferring our code is shit. Saying things like
> > "totally needless" does not start things off on the right foot with
> > Aaron and I, who have really tried hard to clean up some questionable
> > code. We do make mistakes.
>
> Oh, I didn't want to say, that your code is shit (however: serious). Instead,
> that function is quite tricky, and not easy to understand at all (that's why I
> didn't provide a patch yet...: 1. I'm also quite tired/ill nowadays => I don't
> trust on myself now; 2. I want to do a bigger rewrite of that part)
>
> The word "totally needless" referred to the following: there is (should be) no
> difference between empty and n-element list, so that pB==NULL check is not
> needed <- the incorrect handling of pB==NULL was a special case of this BUG.
>
> > diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
> > index c093705..9ef5c9f 100644
> > --- a/lib/libalpm/conflict.c
> > +++ b/lib/libalpm/conflict.c
> > @@ -279,6 +279,12 @@ static alpm_list_t
> > *chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
> >                         }
> >           }
> >         }
> > +       /* ensure we have completely emptied pA */
> > +       while(pA) {
> > +               const char *strA = pA->data;
> > +               ret = alpm_list_add(ret, strdup(strA));
> > +               pA = pA->next;
> > +       }
> >
> >         return(ret);
> >  }
> >
>
> This seems OK, but the previous code filters out directories, so this is needed
> here too (this was missed in the old pB==NULL handler part, too).

Ahh, you are correct. If you would have said this originally, then I
would understand why this problem was not easy at first sight. :)

> > This patch fixes the revised upgrade040 and 041 pactests, as well as
> > upgrade011 as attached. It doesn't fix upgrade046.py. Any thoughts for
> > that issue? I'm not familiar enough with the whole program flow there
> > yet.
> >
>
> Yes, for fixing upgrade046.py bigger efforts are needed (however, that is
> probably rarer issue). The problem is, when you use --force, the
> _alpm_db_find_fileconflicts is not called at all. And this file relocation
> "stuff" (skip_remove) is computed in that function; so this computation is not
> done at all with --force.

Yeah, interesting. We may have to rework this somehow...hmm.

-Dan