add.c and remove.c cleanup

Unification and cleanup of the add.c and remove.c code. It looks remarkably
similar, so this may be a candidate for functionalization at a later time.

Signed-off-by: Dan McGee <dan@archlinux.org>

3 files changed, 74 insertions, 42 deletions

diff --git a/lib/libalpm/alpm.c b/lib/libalpm/alpm.c
index a53e98b8..b4faf571 100644
--- a/lib/libalpm/alpm.c
+++ b/lib/libalpm/alpm.c
@@ -710,7 +710,7 @@ int SYMEXPORT alpm_trans_addtarget(char *target)
 }
 
 /** Prepare a transaction.
- * @param data the address of a PM_LIST where detailed description
+ * @param data the address of an alpm_list where detailed description
  * of an error can be dumped (ie. list of conflicting files)
  * @return 0 on success, -1 on error (pm_errno is set accordingly)
  */
@@ -729,7 +729,7 @@ int SYMEXPORT alpm_trans_prepare(alpm_list_t **data)
 }
 
 /** Commit a transaction.
- * @param data the address of a PM_LIST where detailed description
+ * @param data the address of an alpm_list where detailed description
  * of an error can be dumped (ie. list of conflicting files)
  * @return 0 on success, -1 on error (pm_errno is set accordingly)
  */
diff --git a/src/pacman/add.c b/src/pacman/add.c
index 65d666e9..7a8f7809 100644
--- a/src/pacman/add.c
+++ b/src/pacman/add.c
@@ -37,6 +37,13 @@
 
 extern config_t *config;
 
+/**
+ * @brief Upgrade a specified list of packages.
+ *
+ * @param targets a list of packages (as strings) to upgrade
+ *
+ * @return 0 on success, 1 on failure
+ */
 int pacman_upgrade(alpm_list_t *targets)
 {
 	/* this is basically just a remove-then-add process. pacman_add() will */
@@ -45,9 +52,17 @@ int pacman_upgrade(alpm_list_t *targets)
 	return(pacman_add(targets));
 }
 
+/**
+ * @brief Add a specified list of packages which cannot already be installed.
+ *
+ * @param targets a list of packages (as strings) to add
+ *
+ * @return 0 on success, 1 on failure
+ */
 int pacman_add(alpm_list_t *targets)
 {
-	alpm_list_t *i = targets, *data = NULL;
+	alpm_list_t *i, *data = NULL;
+	pmtranstype_t transtype = PM_TRANS_TYPE_ADD;
 	int retval = 0;
 
 	if(targets == NULL) {
@@ -56,7 +71,7 @@ int pacman_add(alpm_list_t *targets)
 
 	/* Check for URL targets and process them
 	 */
-	while(i) {
+	for(i = targets; i; i = alpm_list_next(i)) {
 		if(strstr(i->data, "://")) {
 			char *str = alpm_fetch_pkgurl(i->data);
 			if(str == NULL) {
@@ -66,13 +81,17 @@ int pacman_add(alpm_list_t *targets)
 				i->data = str;
 			}
 		}
-		i = i->next;
 	}
 
-	/* Step 1: create a new transaction
-	 */
-	if(alpm_trans_init((config->upgrade == 0) ? PM_TRANS_TYPE_ADD : PM_TRANS_TYPE_UPGRADE,
-	                   config->flags, cb_trans_evt, cb_trans_conv, cb_trans_progress) == -1) {
+	/* Step 1: create a new transaction */
+	if(config->upgrade == 1) {
+		/* if upgrade flag was set, change this to an upgrade transaction */
+		transtype = PM_TRANS_TYPE_UPGRADE;
+	}
+
+	if(alpm_trans_init(transtype, config->flags, cb_trans_evt,
+	   cb_trans_conv, cb_trans_progress) == -1) {
+		/* TODO: error messages should be in the front end, not the back */
 		ERR(NL, "%s\n", alpm_strerror(pm_errno));
 		if(pm_errno == PM_ERR_HANDLE_LOCK) {
 			MSG(NL, _("       if you're sure a package manager is not already running,\n"
@@ -81,20 +100,23 @@ int pacman_add(alpm_list_t *targets)
 		return(1);
 	}
 
-	/* and add targets to it */
+	/* add targets to the created transaction */
 	MSG(NL, _("loading package data... "));
-	for(i = targets; i; i = i->next) {
-		if(alpm_trans_addtarget(i->data) == -1) {
+	for(i = targets; i; i = alpm_list_next(i)) {
+		char *targ = alpm_list_getdata(i);
+		if(alpm_trans_addtarget(targ) == -1) {
+			/* TODO: glad this output is hacky */
 			MSG(NL, "\n");
-			ERR(NL, _("failed to add target '%s' (%s)"), (char *)i->data, alpm_strerror(pm_errno));
+			ERR(NL, _("failed to add target '%s' (%s)"), targ,
+			    alpm_strerror(pm_errno));
 			retval = 1;
 			goto cleanup;
 		}
 	}
 	MSG(CL, _("done.\n"));
 
-	/* Step 2: "compute" the transaction based on targets and flags
-	 */
+	/* Step 2: "compute" the transaction based on targets and flags */
+	/* TODO: No, compute nothing. This is stupid. */
 	if(alpm_trans_prepare(&data) == -1) {
 		long long *pkgsize, *freespace;
 
@@ -124,14 +146,14 @@ int pacman_add(alpm_list_t *targets)
 					}
 					MSG(CL, "\n");
 				}
-			break;
+				break;
 			case PM_ERR_CONFLICTING_DEPS:
 				for(i = data; i; i = alpm_list_next(i)) {
 					pmdepmissing_t *miss = alpm_list_getdata(i);
 					MSG(NL, _(":: %s: conflicts with %s"),
 						alpm_dep_get_target(miss), alpm_dep_get_name(miss));
 				}
-			break;
+				break;
 			case PM_ERR_FILE_CONFLICTS:
 				for(i = data; i; i = alpm_list_next(i)) {
 					pmconflict_t *conflict = alpm_list_getdata(i);
@@ -150,9 +172,9 @@ int pacman_add(alpm_list_t *targets)
 					}
 				}
 				MSG(NL, _("\nerrors occurred, no packages were upgraded.\n"));
-			break;
-			/* TODO This is gross... we should not return these values in the same list we
-			 * would get conflicts and such with... it's just silly
+				break;
+			/* TODO This is gross... we should not return these values in the same
+			 * list we would get conflicts and such with... it's just silly
 			 */
 			case PM_ERR_DISK_FULL:
 				i = data;
@@ -160,17 +182,17 @@ int pacman_add(alpm_list_t *targets)
 				i = alpm_list_next(i);
 				freespace = alpm_list_getdata(i);
 					MSG(NL, _(":: %.1f MB required, have %.1f MB"),
-						(double)(*pkgsize / (1024.0*1024.0)), (double)(*freespace / (1024.0*1024.0)));
-			break;
+					    (double)(*pkgsize / (1024.0*1024.0)),
+					    (double)(*freespace / (1024.0*1024.0)));
+				break;
 			default:
-			break;
+				break;
 		}
 		retval=1;
 		goto cleanup;
 	}
 
-	/* Step 3: actually perform the installation
-	 */
+	/* Step 3: perform the installation */
 	if(alpm_trans_commit(NULL) == -1) {
 		ERR(NL, _("failed to commit transaction (%s)\n"), alpm_strerror(pm_errno));
 		retval=1;
diff --git a/src/pacman/remove.c b/src/pacman/remove.c
index e62c641d..941eab52 100644
--- a/src/pacman/remove.c
+++ b/src/pacman/remove.c
@@ -39,9 +39,16 @@ extern config_t *config;
 
 extern pmdb_t *db_local;
 
+/**
+ * @brief Remove a specified list of packages.
+ *
+ * @param targets a list of packages (as strings) to remove from the system
+ *
+ * @return 0 on success, 1 on failure
+ */
 int pacman_remove(alpm_list_t *targets)
 {
-	alpm_list_t *data = NULL, *i, *j, *finaltargs = NULL;
+	alpm_list_t *i, *j, *data = NULL, *finaltargs = NULL;
 	int retval = 0;
 
 	if(targets == NULL) {
@@ -73,9 +80,9 @@ int pacman_remove(alpm_list_t *targets)
 		}
 	}
 
-	/* Step 1: create a new transaction
-	 */
-	if(alpm_trans_init(PM_TRANS_TYPE_REMOVE, config->flags, cb_trans_evt, cb_trans_conv, cb_trans_progress) == -1) {
+	/* Step 1: create a new transaction */
+	if(alpm_trans_init(PM_TRANS_TYPE_REMOVE, config->flags,
+	   cb_trans_evt, cb_trans_conv, cb_trans_progress) == -1) {
 		ERR(NL, _("failed to init transaction (%s)\n"), alpm_strerror(pm_errno));
 		if(pm_errno == PM_ERR_HANDLE_LOCK) {
 			MSG(NL, _("       if you're sure a package manager is not already running,\n"
@@ -84,18 +91,22 @@ int pacman_remove(alpm_list_t *targets)
 		FREELIST(finaltargs);
 		return(1);
 	}
-	/* and add targets to it */
+
+	/* add targets to the created transaction */
+	MSG(NL, _("loading package data... "));
 	for(i = finaltargs; i; i = alpm_list_next(i)) {
 		char *targ = alpm_list_getdata(i);
 		if(alpm_trans_addtarget(targ) == -1) {
-			ERR(NL, _("failed to add target '%s' (%s)\n"), targ, alpm_strerror(pm_errno));
+			/* TODO: glad this output is hacky */
+			MSG(NL, "\n");
+			ERR(NL, _("failed to add target '%s' (%s)\n"), targ,
+			    alpm_strerror(pm_errno));
 			retval = 1;
 			goto cleanup;
 		}
 	}
 
-	/* Step 2: prepare the transaction based on its type, targets and flags
-	 */
+	/* Step 2: prepare the transaction based on its type, targets and flags */
 	if(alpm_trans_prepare(&data) == -1) {
 		ERR(NL, _("failed to prepare transaction (%s)\n"), alpm_strerror(pm_errno));
 		switch(pm_errno) {
@@ -106,19 +117,20 @@ int pacman_remove(alpm_list_t *targets)
 					    alpm_dep_get_name(miss));
 				}
 				alpm_list_free(data);
-			break;
+				break;
 			default:
-			break;
+				break;
 		}
 		retval = 1;
 		goto cleanup;
 	}
 
-	/* Warn user in case of dangerous operation
-	 */
-	if(config->flags & PM_TRANS_FLAG_RECURSE || config->flags & PM_TRANS_FLAG_CASCADE) {
+	/* Warn user in case of dangerous operation */
+	if(config->flags & PM_TRANS_FLAG_RECURSE ||
+	   config->flags & PM_TRANS_FLAG_CASCADE) {
 		/* list transaction targets */
 		alpm_list_t *lst = NULL;
+		/* create a new list of package names only */
 		for(i = alpm_trans_get_pkgs(); i; i = alpm_list_next(i)) {
 			pmpkg_t *pkg = alpm_list_getdata(i);
 			lst = alpm_list_add(lst, strdup(alpm_pkg_get_name(pkg)));
@@ -134,16 +146,14 @@ int pacman_remove(alpm_list_t *targets)
 		MSG(NL, "\n");
 	}
 
-	/* Step 3: actually perform the removal
-	 */
+	/* Step 3: actually perform the removal */
 	if(alpm_trans_commit(NULL) == -1) {
 		ERR(NL, _("failed to commit transaction (%s)\n"), alpm_strerror(pm_errno));
 		retval = 1;
 		goto cleanup;
 	}
 
-	/* Step 4: release transaction resources
-	 */
+	/* Step 4: release transaction resources */
 cleanup:
 	FREELIST(finaltargs);
 	if(alpm_trans_release() == -1) {