Re: [pacman-dev] [PATCH] Detect inter-package conflicts between files and directories

On Tue, Jul 17, 2012 at 11:50:26AM +1000, Allan McRae wrote:

Detect a conflict between a file/symlink in one package and a directory in another when both are being installed at once.

A side effect is the creation on conflicts between a direcotry symlink and a real directory (e.g lib -> usr/lib in pkg1 and /lib in pkg2). Given we can not guarantee pkg1 is installed before pkg2, this is a genuine conflict.

Signed-off-by: Allan McRae --- lib/libalpm/conflict.c | 48 +++++++++++++++++++++++++----------- test/pacman/tests/fileconflict002.py | 2 -- test/pacman/tests/fileconflict015.py | 2 -- 3 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/lib/libalpm/conflict.c b/lib/libalpm/conflict.c index f3b269f..041ba6f 100644 --- a/lib/libalpm/conflict.c +++ b/lib/libalpm/conflict.c @@ -274,28 +274,48 @@ static alpm_list_t *filelist_intersection(alpm_filelist_t *filesA, size_t ctrA = 0, ctrB = 0;

while(ctrA < filesA->count && ctrB < filesB->count) { + int cmp, isdirA, isdirB; + alpm_file_t *fileA = filesA->files + ctrA; alpm_file_t *fileB = filesB->files + ctrB; - const char *strA = fileA->name; - const char *strB = fileB->name; - /* skip directories, we don't care about them */ + char *strA = strdup(fileA->name); + char *strB = strdup(fileB->name); + + isdirA = 0; if(strA[strlen(strA)-1] == '/') { + isdirA = 1; + strA[strlen(strA)-1] = '\0'; + } + + isdirB = 0; + if(strB[strlen(strB)-1] == '/') { + isdirB = 1; + strB[strlen(strB)-1] = '\0'; + } + + cmp = strcmp(strA, strB);

Allocating extra memory on the heap just to chop off one character before comparing doesn't look like the right thing to do... The problem is that we probably cannot leave the strings as-is without writing our custom strcmp() implementation or doing some strncmp() magic (I didn't look into strncmp() in detail), though. Even if you want to keep it simple and use strdup(), you could initialize "strA" and "strB" with pointers to the original strings and move the strdup() into the two if-branches. Note that in this case, you will also add one additional check to the free() statements but this will remove the need for heap and copy operations for the majority of files, won't it? Also, why do we use mixedCase for variables here (same applies to the patch you submitted before this one)? :)

+ if(cmp < 0) { ctrA++; - } else if(strB[strlen(strB)-1] == '/') { + } else if(cmp > 0) { ctrB++; } else { - int cmp = strcmp(strA, strB); - if(cmp < 0) { - ctrA++; - } else if(cmp > 0) { - ctrB++; - } else { - /* item in both, qualifies as an intersect */ + /* TODO: this creates conflicts between a symlink to a directory in + * one package and a real directory in the other. For example, + * lib -> /usr/lib in pkg1 and /lib in pkg2. This would be allowed + * when installing one package at a time _provided_ pkg1 is installed + * first. This will need adjusted if the order of package install can + * be guaranteed to install the symlink first */ + + /* when not directories, item in both qualifies as an intersect */ + if(! (isdirA && isdirB)) { ret = alpm_list_add(ret, fileA); - ctrA++; - ctrB++; } - } + ctrA++; + ctrB++; + } + + free(strA); + free(strB); }

return ret; diff --git a/test/pacman/tests/fileconflict002.py b/test/pacman/tests/fileconflict002.py index e107cd2..1e6113c 100644 --- a/test/pacman/tests/fileconflict002.py +++ b/test/pacman/tests/fileconflict002.py @@ -19,5 +19,3 @@ self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") self.addrule("!FILE_EXIST=dir/realdir/file") - -self.expectfailure = True diff --git a/test/pacman/tests/fileconflict015.py b/test/pacman/tests/fileconflict015.py index 78634d7..3c80bbc 100644 --- a/test/pacman/tests/fileconflict015.py +++ b/test/pacman/tests/fileconflict015.py @@ -13,5 +13,3 @@ self.addrule("PACMAN_RETCODE=1") self.addrule("!PKG_EXIST=pkg1") self.addrule("!PKG_EXIST=pkg2") - -self.expectfailure = True -- 1.7.11.2