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