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

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.

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

On 14/05/13 06:03, 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 ---

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.

Looks good. Minor comment below.

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);

This confused me for a while... Why is it adding 9 to the strlen for ".pacnew"? Oh... because it does ".pacorig" too. So how about moving it below like:

+ + if(oldpkg) { + _alpm_log(handle, ALPM_LOG_DEBUG, + "action: keeping current file and installing" + " new one with .pacnew ending\n");

size_t newlen = strlen(filename) + strlen(".pacnew"); MALLOC(....

+ 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");

size_t newlen = strlen(filename) + strlen(".pacorig"); MALLOC(....

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: