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 <[email protected]>
> ---
>
> 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
>
>
