[pacman-dev] backup handling ugly mess

[Xavier]

On Tue, Dec 04, 2007 at 09:18:08PM +0100, Xavier wrote:
> The first part of this commit is supposed to temporarily add all old backup
> entries (besides the new) to the NoUpgrade array. Because of a bug, it
> didn't have any effect.
> 
> The second part was an idea of me to handle the pacsave during an
> removeupgrade just like we already did for a remove, but I finally think
> this is a really stupid / misleading / confusing idea. This code shouldn't
> even be reached during a removeupgrade if the first part was fixed anyway.
> 

Here is a patch which fix the first part, and reverts the second.


>From e3aa6ba2d7315bda9330b0f00014416540681881 Mon Sep 17 00:00:00 2001
From: Chantry Xavier <shiningxc at gmail.com>
Date: Wed, 5 Dec 2007 00:47:53 +0100
Subject: [PATCH] libalpm/add.c : bugfix in backup handling.

commit 843d368ef60a74719dfc74a27de3fe3ef441951f was supposed to use the
backup array of the old package (besides the new one), but the backup line
wasn't correctly parsed, so this change didn't have any effect.

Now that this is fixed, the second part of the commit doesn't make much
sense. In case of a REMOVEUPGRADE transaction, the backup files are in the
NoUpgrade array, and thus the unlink_file function will exit early.
The .pacsave handling in unlink_file only concerns REMOVE transaction.

Note: this breaks upgrade024, upgrade025 and the newly added upgrade026
pactests, because .pacsave file are never created on an upgrade operation.

Ref: http://www.archlinux.org/pipermail/pacman-dev/2007-December/010416.html

Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
---
 lib/libalpm/add.c           |    4 ++--
 lib/libalpm/remove.c        |   20 ++++++++++++--------
 pactest/tests/upgrade026.py |   17 +++++++++++++++++
 3 files changed, 31 insertions(+), 10 deletions(-)
 create mode 100644 pactest/tests/upgrade026.py

diff --git a/lib/libalpm/add.c b/lib/libalpm/add.c
index 24f2b51..8c01985 100644
--- a/lib/libalpm/add.c
+++ b/lib/libalpm/add.c
@@ -244,7 +244,7 @@ static int upgrade_remove(pmpkg_t *oldpkg, pmpkg_t *newpkg, pmtrans_t *trans, pm
 		const char *backup = b->data;
 		_alpm_log(PM_LOG_DEBUG, "adding %s to the NoUpgrade array temporarily\n",
 				backup);
-		handle->noupgrade = alpm_list_add(handle->noupgrade, strdup(backup));
+		handle->noupgrade = alpm_list_add(handle->noupgrade, _alpm_backup_file(backup));
 	}
 	/* new package backup list */
 	for(b = alpm_pkg_get_backup(newpkg); b; b = b->next) {
@@ -253,7 +253,7 @@ static int upgrade_remove(pmpkg_t *oldpkg, pmpkg_t *newpkg, pmtrans_t *trans, pm
 		if(!alpm_list_find_ptr(handle->noupgrade, backup)) {
 			_alpm_log(PM_LOG_DEBUG, "adding %s to the NoUpgrade array temporarily\n",
 					backup);
-			handle->noupgrade = alpm_list_add(handle->noupgrade, strdup(backup));
+			handle->noupgrade = alpm_list_add(handle->noupgrade, _alpm_backup_file(backup));
 		}
 	}
 
diff --git a/lib/libalpm/remove.c b/lib/libalpm/remove.c
index 349ff10..5ab8001 100644
--- a/lib/libalpm/remove.c
+++ b/lib/libalpm/remove.c
@@ -227,14 +227,18 @@ static void unlink_file(pmpkg_t *info, alpm_list_t *lp, pmtrans_t *trans)
 			return;
 		} else if(needbackup) {
 			/* if the file is flagged, back it up to .pacsave */
-			if(!(trans->flags & PM_TRANS_FLAG_NOSAVE)) {
-				char newpath[PATH_MAX];
-				snprintf(newpath, PATH_MAX, "%s.pacsave", file);
-				rename(file, newpath);
-				_alpm_log(PM_LOG_WARNING, _("%s saved as %s\n"), file, newpath);
-				return;
-			} else {
-				_alpm_log(PM_LOG_DEBUG, "transaction is set to NOSAVE, not backing up '%s'\n", file);
+			if(!(trans->type == PM_TRANS_TYPE_REMOVEUPGRADE)) {
+				/* if it was an upgrade, the file would be left alone because
+				 * pacman_add() would handle it */
+				if(!(trans->flags & PM_TRANS_FLAG_NOSAVE)) {
+					char newpath[PATH_MAX];
+					snprintf(newpath, PATH_MAX, "%s.pacsave", file);
+					rename(file, newpath);
+					_alpm_log(PM_LOG_WARNING, _("%s saved as %s\n"), file, newpath);
+					return;
+				} else {
+					_alpm_log(PM_LOG_DEBUG, "transaction is set to NOSAVE, not backing up '%s'\n", file);
+				}
 			}
 		}
 		_alpm_log(PM_LOG_DEBUG, "unlinking %s\n", file);
diff --git a/pactest/tests/upgrade026.py b/pactest/tests/upgrade026.py
new file mode 100644
index 0000000..8ad87e3
--- /dev/null
+++ b/pactest/tests/upgrade026.py
@@ -0,0 +1,17 @@
+self.description = "Upgrade a package, with a file leaving the pkg but staying in 'backup'"
+
+lp = pmpkg("dummy")
+lp.files = ["etc/dummy.conf*"]
+lp.backup = ["etc/dummy.conf"]
+self.addpkg2db("local", lp)
+
+p = pmpkg("dummy", "1.0-2")
+p.backup = ["etc/dummy.conf"]
+self.addpkg(p)
+
+self.args = "-U %s" % p.filename()
+
+self.addrule("PKG_VERSION=dummy|1.0-2")
+self.addrule("FILE_PACSAVE=etc/dummy.conf")
+self.addrule("!FILE_PACNEW=etc/dummy.conf")
+self.addrule("FILE_EXIST=etc/dummy.conf")
-- 
1.5.3.7