[pacman-dev] [PATCH] Move -Sy operation into its own transaction

[Dan McGee]

On Feb 17, 2008 9:12 AM, Chantry Xavier <shiningxc at gmail.com> wrote:
> This allows us to remove the sync_only flag, and also do the following
> steps in the future :
> 1) refresh the database (if asked)
> 2) do other stuff (eg checking if a newer pacman version is available)
> 3) start the actual transaction
>
> Currently when we detect a newer pacman version, we have to release the
> current transaction and start a new one.

Looks good to me, I like this way of thinking.

> Signed-off-by: Chantry Xavier <shiningxc at gmail.com>
> ---
>  src/pacman/sync.c |   56 ++++++++++++++++++++++++++++++++--------------------
>  1 files changed, 34 insertions(+), 22 deletions(-)
>
> diff --git a/src/pacman/sync.c b/src/pacman/sync.c
> index 27218d6..bb2a8bb 100644
> --- a/src/pacman/sync.c
> +++ b/src/pacman/sync.c
> @@ -202,6 +202,17 @@ static int sync_synctree(int level, alpm_list_t *syncs)
>         alpm_list_t *i;
>         int success = 0, ret;
>
> +       if(alpm_trans_init(PM_TRANS_TYPE_SYNC, 0, cb_trans_evt,
> +                cb_trans_conv, cb_trans_progress) == -1) {
> +               fprintf(stderr, _("error: failed to init transaction (%s)\n"),
> +                       alpm_strerrorlast());
> +               if(pm_errno == PM_ERR_HANDLE_LOCK) {
> +                       printf(_("  if you're sure a package manager is not already\n"
> +                                "  running, you can remove %s.\n"), alpm_option_get_lockfile());
> +               }
> +               return(0);
> +       }
> +

Don't we have this same code in a few different places now? Can we
refactor this somehow?

>         for(i = syncs; i; i = alpm_list_next(i)) {
>                 pmdb_t *db = alpm_list_getdata(i);
>
> @@ -229,10 +240,18 @@ static int sync_synctree(int level, alpm_list_t *syncs)
>                 }
>         }
>
> +       if(alpm_trans_release() == -1) {
> +               fprintf(stderr, _("error: failed to release transaction (%s)\n"),
> +                       alpm_strerrorlast());
> +               return(0);
> +       }
>         /* We should always succeed if at least one DB was upgraded - we may possibly
>          * fail later with unresolved deps, but that should be rare, and would be
>          * expected
>          */
What is this comment even referring to? I'm confused here. And you had
code changes on either side of it.

> +       if(!success) {
> +               fprintf(stderr, _("error: failed to synchronize any databases\n"));
> +       }
>         return(success > 0);
>  }
>
> @@ -469,7 +488,7 @@ static int sync_list(alpm_list_t *syncs, alpm_list_t *targets)
>         return(0);
>  }
>
> -static int sync_trans(alpm_list_t *targets, int sync_only)
> +static int sync_trans(alpm_list_t *targets)
>  {
>         int retval = 0;
>         alpm_list_t *data = NULL;
> @@ -487,23 +506,8 @@ static int sync_trans(alpm_list_t *targets, int sync_only)
>                 return(1);
>         }
>
> -       if(config->op_s_sync) {
> -               /* grab a fresh package list */
> -               printf(_(":: Synchronizing package databases...\n"));
> -               alpm_logaction("synchronizing package lists\n");
> -               if(!sync_synctree(config->op_s_sync, sync_dbs)) {
> -                       fprintf(stderr, _("error: failed to synchronize any databases\n"));
> -                       retval = 1;
> -                       goto cleanup;
> -               }
> -               if(sync_only) {
> -                       goto cleanup;
> -               }
> -       }
> -
>         if(config->op_s_upgrade) {
>                 alpm_list_t *pkgs, *i;
> -
>                 printf(_(":: Starting full system upgrade...\n"));
>                 alpm_logaction("starting full system upgrade\n");
>                 if(alpm_trans_sysupgrade() == -1) {
> @@ -756,7 +760,6 @@ cleanup:
>  int pacman_sync(alpm_list_t *targets)
>  {
>         alpm_list_t *sync_dbs = NULL;
> -       int sync_only = 0;
>
>         /* clean the cache */
>         if(config->op_s_clean) {
> @@ -772,18 +775,27 @@ int pacman_sync(alpm_list_t *targets)
>                 return(1);
>         }
>
> -       if(config->op_s_search || config->group
> -                       || config->op_s_info || config->op_q_list) {
> -               sync_only = 1;
> -       } else if(targets == NULL && !(config->op_s_sync || config->op_s_upgrade)) {
> +       if(targets == NULL && !(config->op_s_sync || config->op_s_upgrade
> +                               || config->op_s_search || config->group
> +                               || config->op_s_info || config->op_q_list)) {
>                 /* don't proceed here unless we have an operation that doesn't require
>                  * a target list */
>                 pm_printf(PM_LOG_ERROR, _("no targets specified (use -h for help)\n"));
>                 return(1);
>         }
>
> +       if(config->op_s_sync) {
> +               /* grab a fresh package list */
> +               printf(_(":: Synchronizing package databases...\n"));
> +               alpm_logaction("synchronizing package lists\n");
> +               if(!sync_synctree(config->op_s_sync, sync_dbs)) {
> +                       return(1);
> +               }
> +               config->op_s_sync = 0;
> +       }
> +
>         if(needs_transaction()) {
> -               if(sync_trans(targets, sync_only) == 1) {
> +               if(sync_trans(targets) == 1) {
>                         return(1);
>                 }
>         }
> --
> 1.5.4