[pacman-dev] [PATCH 4/5] Versioned provisions

[Dan McGee]

On Nov 17, 2007 7:51 AM, Nagy Gabor <shiningxc at gmail.com> wrote:
> This patch introduces versioned provisions in "provision 1.0-1" format.
> _alpm_db_whatprovides was modified accordingly (added sync500.py)
> alpm_depcmp was modified accordingly (add043.py passes now; added add044.py and add045.py)
> Note: alpm_db_search now uses the whole versioned %PROVIDES% string in its search
> Note: debug logging was simplified in alpm_depcmp

>  int SYMEXPORT alpm_depcmp(pmpkg_t *pkg, pmdepend_t *dep)
>  {
> -       int equal = 0;
> +       alpm_list_t *i;
>
>         ALPM_LOG_FUNC;
>
>         const char *pkgname = alpm_pkg_get_name(pkg);
> -       const char *pkgversion = alpm_pkg_get_version(pkg);
> +       const char *pkgver = alpm_pkg_get_version(pkg);
> +       int satisfy = 0;
>
> -       if(strcmp(pkgname, dep->name) == 0
> -                       || alpm_list_find_str(alpm_pkg_get_provides(pkg), dep->name)) {
> +       /* check (pkg->name, pkg->version) */
> +       satisfy = (!strcmp(pkgname, dep->name)

== 0 is much clearer, so I'm going to change this back if I pull the
patch. Let's not take shortcuts in C just because they work- lets make
the code readable for everyone. It will all get compiled the same way
anyway. As a general rule, I always want to see an == or != used with
strcmp- it is just easier to digest.

> +                       && dep_vercmp(pkgver, dep->mod, dep->version));
>
> -               equal = dep_vercmp(pkgversion, dep->mod, dep->version);
> +       /* check provisions, format : "name version" */
> +       for(i = alpm_pkg_get_provides(pkg); i && !satisfy; i = i->next) {
> +               char *provname = strdup(i->data);
> +               char *provver = strchr(provname, ' ');
>
> -               char *mod = "~=";
> -               switch(dep->mod) {
> -                       case PM_DEP_MOD_EQ: mod = "=="; break;
> -                       case PM_DEP_MOD_GE: mod = ">="; break;
> -                       case PM_DEP_MOD_LE: mod = "<="; break;
> -                       default: break;
> -               }
> -
> -               if(strlen(dep->version) > 0) {
> -                       _alpm_log(PM_LOG_DEBUG, "depcmp: %s-%s %s %s-%s => %s\n",
> -                                                               pkgname, pkgversion,
> -                                                               mod, dep->name, dep->version, (equal ? "match" : "no match"));
> +               if(provver == NULL) { /* no provision version */
> +                       satisfy = (dep->mod == PM_DEP_MOD_ANY
> +                                       && !strcmp(provname, dep->name));
>                 } else {
> -                       _alpm_log(PM_LOG_DEBUG, "depcmp: %s-%s %s %s => %s\n",
> -                                                               pkgname, pkgversion,
> -                                                               mod, dep->name, (equal ? "match" : "no match"));
> +                       *provver = '\0';
> +                       provver += 1;
This is ugly, and I won't take it again without at least a one line
comment describing what is being done.

> +                       satisfy = (!strcmp(provname, dep->name)
> +                                       && dep_vercmp(provver, dep->mod, dep->version));
>                 }
> +               free(provname);
>         }
>
> -       return(equal);
> +       char *depstring = alpm_dep_get_string(dep);
> +       _alpm_log(PM_LOG_DEBUG, "alpm_depcmp %s-%s %s : %smatch\n",
> +                       pkgname, pkgver, depstring, satisfy ? "" : "no ");
No need for tricks here to save 5 characters with the match/no match
stuff. Code should be easily decipherable!

> +       free(depstring);
> +       return(satisfy);
>  }

Looks good besides that. I'm pulling it into my tree.

-Dan