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

[Lukas Fleischer]

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 <allan at archlinux.org>
> ---
>  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