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

[Allan McRae]

On 17/07/12 17:19, Lukas Fleischer wrote:
> 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?

Changed so strndup is only used when the file is a directory.

> Also, why do we use mixedCase for variables here (same applies to the
> patch you submitted before this one)? :)

Because that was the way the variables were named beforehand...

>> +		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
> 
> 
>