[pacman-dev] [PATCH] Install unchanged backup files to get correct timestamps.

[Allan McRae]

On 04/06/13 21:10, Ross Lagerwall wrote:
> On Tue, Jun 04, 2013 at 11:20:14AM +0200, Patrick Steinhardt wrote:
>>  		if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) {
>> -			/* local and new files are the same, no sense in installing the file
>> -			 * over itself, regardless of what the original file was */
>> -			_alpm_log(handle, ALPM_LOG_DEBUG,
>> -					"action: leaving existing file in place\n");
>> -			unlink(checkfile);
>> +			/* local and new files are the same, updating anyway to get
>> +			 * correct timestamps */
>> +			_alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",
> 
> Perhaps this should be changed to something like:
> "action: updating existing file's timestamps\n"
> to differentiate it from when the file is actually new.
> 

I like "installing new file" because it is exactly what we say we will
do in the pacman(8) man page.  Also, the situations are easily
distinguished by the previous debug output.

>> +					entryname_orig);
>> +			if(try_rename(handle, checkfile, filename)) {
>> +				errors++;
>> +			}
> 
> Can we not just ignore it if it fails since it is non-fatal?
> 

No.  If something fails here, we are in a very bad situation for the
rest of the transaction.  I want that error to propagate so that we
abort the transaction after this package is finished.

(Aside) What I have just noticed, is we do not print any actual errors
during this whole section.  So the transaction will stop with errors
occurred during extraction but give no indication what actually
happened.   Another patch....

>>  		} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) {
>>  			/* original and new files are the same, leave the local version alone,
>>  			 * including any user changes */


So...  with my two comments above, I think the original patch is fine.
I will pull that in the next few days unless there is objections to my
above reasoning.

Allan