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

[Dan McGee]

2008/2/11 Nagy Gabor <ngaba at bibl.u-szeged.hu>:
> Idézés Dan McGee <dpmcgee at gmail.com>:
> > 2008/2/11 Nagy Gabor <ngaba at bibl.u-szeged.hu>:
> > > 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. :)

Revised patch:

diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c
index c093705..3442902 100644
--- a/lib/libalpm/conflict.c
+++ b/lib/libalpm/conflict.c
@@ -251,10 +251,7 @@ static alpm_list_t
*chk_filedifference(alpm_list_t *filesA, alpm_list_t *filesB)
        alpm_list_t *ret = NULL;
        alpm_list_t *pA = filesA, *pB = filesB;

-       if(pB == NULL) {
-               return(alpm_list_strdup(pA));
-       }
-
+       /* if both filesA and filesB have entries, do this loop */
        while(pA && pB) {
                const char *strA = pA->data;
                const char *strB = pB->data;
@@ -279,6 +276,15 @@ 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;
+               /* skip directories */
+               if(strA[strlen(strA)-1] != '/') {
+                       ret = alpm_list_add(ret, strdup(strA));
+               }
+               pA = pA->next;
+       }

        return(ret);
 }

> A possible hotfix:
> We may call _alpm_db_find_fileconflicts() with --force too (which is in fact a
> fileconflict PLUS filerelocation check), and simply ignore its reported
> fileconflict errors.
>
> One more small but must-mention issue:
> See upgrade041.py and conflict.c:428 and consider the following case: user
> deleted /usr/share/file by hand [this is not forbidden, however not suggested],
> then this filerelocation simply won't be detected and the issue appear again.
>
> Long run:
> Fixing these issues is not easy at all (I mean cleaning up this part: moving
> filerelocation stuff to add.c and fix these issues), I'm afraid of a possible
> appearing slowdown (see the last issue for example).
> On the other hand, transaction rollback will eliminate this whole filerelocation
> computation, since then we will do the "final step" (the REAL removal and and
> install) at transaction level, not in "package level", so we will just see:
> file.OLD-PID, file.NEW-PID at the end or something like this. And this fact is
> quite unmotivating :-P
> Transaction rollback can also easily fix trans001.py and fileconflict00?.py so
> this is just an other reason for implementing it (however, that will be
> difficult too ;-P and personally I'm not a big fan of that, but I must admit
> that it is needed.)

My take:
* When a user uses --force, they should know what they are doing. So
I'm less concerned about fixing that situation than the normal,
not-forced case.
* The above patch fixes the original issue, correct? Where
chk_fileconflicts was not always producing the correct list.

-Dan