[pacman-dev] [PATCHv2] add.c: refactor backup file modification checks

[Dave Reisner]

On Mon, May 13, 2013 at 04:36:15PM -0400, Dave Reisner wrote:
> On Mon, May 13, 2013 at 04:03:54PM -0400, Andrew Gregory wrote:
> > The previous implementation was overly complex with unnecessary checks
> > and nested conditionals.  By reordering the tests and changing them to
> > all be checks for positive hash matches rather than non-matches, we can
> > collapse several cases and make the process much more linear.  This
> > removes the need to set hash_orig = "" just to reach some of the checks
> > and corrects a faulty assumption that files are equivalent when the
> > hashing process fails.
> > 
> > Signed-off-by: Andrew Gregory <andrew.gregory.8 at gmail.com>
> > ---
> > 
> > Turns out we can do even better.  Reordered the tests to collapse two more
> > cases and updated a few of the comments to better reflect the more linear
> > layout.
> > 
> >  lib/libalpm/add.c | 114 ++++++++++++++++++++++++------------------------------
> >  1 file changed, 50 insertions(+), 64 deletions(-)
> > 
> > diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
> > index 3ef81e3..ba808a5 100644
> > --- a/lib/libalpm/add.c
> > +++ b/lib/libalpm/add.c
> > @@ -273,8 +273,6 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
> >  				/* check newpkg first, so that adding backup files is retroactive */
> >  				backup = _alpm_needbackup(entryname, newpkg);
> >  				if(backup) {
> > -					/* if we force hash_orig to be non-NULL retroactive backup works */
> > -					hash_orig = "";
> >  					needbackup = 1;
> >  				}
> >  
> > @@ -332,83 +330,71 @@ static int extract_single_file(alpm_handle_t *handle, struct archive *archive,
> >  		_alpm_log(handle, ALPM_LOG_DEBUG, "new:      %s\n", hash_pkg);
> >  		_alpm_log(handle, ALPM_LOG_DEBUG, "original: %s\n", hash_orig);
> >  
> > -		if(!oldpkg) {
> > -			if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) != 0) {
> > -				/* looks like we have a local file that has a different hash as the
> > -				 * file in the package, move it to a .pacorig */
> > -				char *newpath;
> > -				size_t newlen = strlen(filename) + 9;
> > -				MALLOC(newpath, newlen,
> > -						errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
> > +		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);
> > +		} else if(hash_orig && hash_pkg && strcmp(hash_orig, hash_pkg) == 0) {
> 
> Mini-rant: Why can't we have hash methods which return int
> representations of the hashes instead of strings? It would save us from
> needing to use strcmp to compare them.
> 

Uggh. Suppose I should have looked at the APIs we have for this before
asking....

> > +			/* original and new files are the same, leave the local version alone,
> > +			 * including any user changes */
> > +			_alpm_log(handle, ALPM_LOG_DEBUG,
> > +					"action: leaving existing file in place\n");
> > +			unlink(checkfile);
> > +		} else if(hash_orig && hash_local && strcmp(hash_orig, hash_local) == 0) {
> > +			/* installed file has NOT been changed by user,
> > +			 * update to the new version */
> > +			_alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",
> > +					entryname_orig);
> > +			if(try_rename(handle, checkfile, filename)) {
> > +				errors++;
> > +			}
> > +		} else {
> > +			/* none of the three files matched another, unpack the new file alongside
> > +			 * the local file */
> > +			char *newpath;
> > +			size_t newlen = strlen(filename) + 9;
> > +			MALLOC(newpath, newlen,
> > +					errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
> > +
> > +			if(oldpkg) {
> > +				_alpm_log(handle, ALPM_LOG_DEBUG,
> > +						"action: keeping current file and installing"
> > +						" new one with .pacnew ending\n");
> > +				snprintf(newpath, newlen, "%s.pacnew", filename);
> > +				if(try_rename(handle, checkfile, newpath)) {
> > +					errors++;
> > +				} else {
> > +					_alpm_log(handle, ALPM_LOG_WARNING, _("%s installed as %s\n"),
> > +							filename, newpath);
> > +					alpm_logaction(handle, ALPM_CALLER_PREFIX,
> > +							"warning: %s installed as %s\n", filename, newpath);
> > +				}
> > +			} else {
> > +				_alpm_log(handle, ALPM_LOG_DEBUG,
> > +						"action: saving existing file with a .pacorig ending"
> > +						" and installing a new one\n");
> >  				snprintf(newpath, newlen, "%s.pacorig", filename);
> >  
> >  				/* move the existing file to the "pacorig" */
> >  				if(try_rename(handle, filename, newpath)) {
> > -						errors++;
> > -					errors++;
> > +					errors++;   /* failed rename filename  -> filename.pacorig */
> > +					errors++;   /* failed rename checkfile -> filename */
> >  				} else {
> >  					/* rename the file we extracted to the real name */
> >  					if(try_rename(handle, checkfile, filename)) {
> >  						errors++;
> >  					} else {
> > -						_alpm_log(handle, ALPM_LOG_WARNING, _("%s saved as %s\n"), filename, newpath);
> > +						_alpm_log(handle, ALPM_LOG_WARNING,
> > +								_("%s saved as %s\n"), filename, newpath);
> >  						alpm_logaction(handle, ALPM_CALLER_PREFIX,
> >  								"warning: %s saved as %s\n", filename, newpath);
> >  					}
> >  				}
> > -				free(newpath);
> > -			} else {
> > -				/* local file is identical to pkg one, so just remove pkg one */
> > -				unlink(checkfile);
> >  			}
> > -		} else if(hash_orig) {
> > -			/* the fun part */
> > -
> > -			if(hash_local && strcmp(hash_orig, hash_local) == 0) {
> > -				/* installed file has NOT been changed by user */
> > -				if(hash_pkg && strcmp(hash_orig, hash_pkg) != 0) {
> > -					_alpm_log(handle, ALPM_LOG_DEBUG, "action: installing new file: %s\n",
> > -							entryname_orig);
> >  
> > -					if(try_rename(handle, checkfile, filename)) {
> > -						errors++;
> > -					}
> > -				} else {
> > -					/* no sense in installing the same file twice, install
> > -					 * ONLY if the original and package hashes differ */
> > -					_alpm_log(handle, ALPM_LOG_DEBUG, "action: leaving existing file in place\n");
> > -					unlink(checkfile);
> > -				}
> > -			} else if(hash_pkg && strcmp(hash_orig, hash_pkg) == 0) {
> > -				/* originally installed file and new file are the same - this
> > -				 * implies the case above failed - i.e. the file was changed by a
> > -				 * user */
> > -				_alpm_log(handle, ALPM_LOG_DEBUG, "action: leaving existing file in place\n");
> > -				unlink(checkfile);
> > -			} else if(hash_local && hash_pkg && strcmp(hash_local, hash_pkg) == 0) {
> > -				/* this would be magical.  The above two cases failed, but the
> > -				 * user changes just so happened to make the new file exactly the
> > -				 * same as the one in the package... skip it */
> > -				_alpm_log(handle, ALPM_LOG_DEBUG, "action: leaving existing file in place\n");
> > -				unlink(checkfile);
> > -			} else {
> > -				char *newpath;
> > -				size_t newlen = strlen(filename) + 8;
> > -				_alpm_log(handle, ALPM_LOG_DEBUG, "action: keeping current file and installing"
> > -						" new one with .pacnew ending\n");
> > -				MALLOC(newpath, newlen,
> > -						errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
> > -				snprintf(newpath, newlen, "%s.pacnew", filename);
> > -				if(try_rename(handle, checkfile, newpath)) {
> > -					errors++;
> > -				} else {
> > -					_alpm_log(handle, ALPM_LOG_WARNING, _("%s installed as %s\n"),
> > -							filename, newpath);
> > -					alpm_logaction(handle, ALPM_CALLER_PREFIX,
> > -							"warning: %s installed as %s\n", filename, newpath);
> > -				}
> > -				free(newpath);
> > -			}
> > +			free(newpath);
> >  		}
> >  
> >  needbackup_cleanup:
> > -- 
> > 1.8.2.3
> > 
> >