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

[Andrew Gregory]

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) {
+			/* 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