add.c: refactor backup file modification checks

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@gmail.com>
Signed-off-by: Allan McRae <allan@archlinux.org>

1 files changed, 57 insertions, 60 deletions

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
index 3ef81e37..9cbf7670 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,81 +330,80 @@ 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 */
+		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) {
+			/* 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 */
+
+			if(oldpkg) {
 				char *newpath;
-				size_t newlen = strlen(filename) + 9;
+				size_t newlen = strlen(filename) + strlen(".pacnew") + 1;
+
+				_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);
+			} else {
+				char *newpath;
+				size_t newlen = strlen(filename) + strlen(".pacorig") + 1;
+
+				_alpm_log(handle, ALPM_LOG_DEBUG,
+						"action: saving existing file with a .pacorig ending"
+						" and installing a new one\n");
+
 				MALLOC(newpath, newlen,
 						errors++; handle->pm_errno = ALPM_ERR_MEMORY; goto needbackup_cleanup);
 				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);
 			}
 		}