[pacman-dev] [PATCH 2/2] Add new --print operation for all operations

[Xavier]

On Mon, Nov 16, 2009 at 4:02 AM, Dan McGee <dpmcgee at gmail.com> wrote:
>> +*\--print*::
>> +       Only print the targets instead of performing the actual operation (sync,
>> +       remove or upgrade).  Use '\--print-format' to specify how targets are
>> +       displayed.
>> +
>> +*\--print-format* <'format'>::
>> +       Specify a printf-like format to control the output of the '\--print'
>> +       operation.  The possible are attributes are : %n for pkgname, %v for pkgver, %l
>> +       for location, %r for repo and %s for size. The default format string is "%l",
>> +       which displays url with '-S', filename with '-U' and pkgname-pkgver with '-R'.
>> +
>
> * Isn't --print actually -p/--print?
> * Should the default format stuff actually be in the --print
> documentation (either as well, or completely move it there?). I feel
> like I would look there first.
> * You didn't kill the print-uri documentation.
>

done done done

>> +       /* set up the print operations */
>> +       if(config->print) {
>> +               config->noconfirm = 1;
>> +               config->flags |= PM_TRANS_FLAG_NOCONFLICTS;
>> +               config->flags |= PM_TRANS_FLAG_NOLOCK;
>> +               /* Display only errors */
>> +               config->logmask &= ~PM_LOG_WARNING;
> Man, I hate having to do this. We really should think about a
> stderr/stdout jihad.
>

So like we would display all warnings/errors to stderr and the useful
stuff to stdout ?

And it  indeed sounds like it would be a lot of work.. It seems like
pacman was not written having in mind it could be used in
scripts/wrapper.

>> +       }
>> +
>> +
>>  #if defined(HAVE_GETEUID) && !defined(CYGWIN)
>>        /* check if we have sufficient permission for the requested operation */
>>        if(myuid > 0 && needs_root()) {
>> diff --git a/src/pacman/remove.c b/src/pacman/remove.c
>> index b5119fa..06503d3 100644
>> --- a/src/pacman/remove.c
>> +++ b/src/pacman/remove.c
>> @@ -159,6 +159,12 @@ int pacman_remove(alpm_list_t *targets)
>>        }
>>
>>        /* Step 3: actually perform the removal */
>> +       if(config->print) { /* --print */
>> +               print_packages(alpm_trans_get_pkgs());
>> +               /* we are done */
> None of the comments here seem particularly necessary. (/*--print*/,
> /*we are done*/). Can you kill them?
>

done

>> -       /* Display only errors with -Sp and -Sw operations */
>> -       if((config->flags & PM_TRANS_FLAG_DOWNLOADONLY) || config->op_s_printuris) {
>> +       /* Display only errors with -Sw operations */
>> +       if(config->flags & PM_TRANS_FLAG_DOWNLOADONLY) {
> Why do we do this for -Sw?
>

That question was not directly related to my patch but yeah, I have no
idea why we do this for -Sw so I killed it.

>>
>>        /* Step 3: perform the installation */
>> +       if(config->print) { /* --print */
>> +               print_packages(alpm_trans_get_pkgs());
>> +               /* we are done */
>> +               trans_release();
>> +               return(0);
> Kill comments

done

>> +               /* %r : repo */
>> +               if(strstr(temp,"%r")) {
>> +                       const char *repo = "local";
>> +                       pmdb_t *db = alpm_pkg_get_db(pkg);
>> +                       if(db) {
>> +                               repo = alpm_db_get_name(db);
>> +                       }
>> +                       string = strreplace(temp, "%r", repo);
>> +                       free(temp);
>> +                       temp = string;
>> +               }
>> +               /* %s : size */
>> +               if(strstr(temp,"%s")) {
>> +                       char *size;
>> +                       double mbsize = 0.0;
>> +                       mbsize = pkg_get_size(pkg) / (1024.0 * 1024.0);
>> +                       asprintf(&size, "%.2f", mbsize);
>> +                       string = strreplace(temp, "%s", size);
>> +                       free(size);
>> +                       free(temp);
>> +                       temp = string;
>> +               }
> This strreplace() stuff seems a bit naive, considering we could do a
> lot of replaces on the string. I'm not sure we need to worry about it
> but could be a place for some improvement.
>

Do you mean that we are walking through the same string many times ?

I thought at some point we could have a multireplace function which
could take a list of struct replace { char *old ; char *new; } but ...
firstly it looked a bit overkill
and secondly, I was worried that all the pkg/db accessors could be
more costly than these strings operations.
Though that could also be solved by doing a first pass on the string
to see what format options we have, and then build the replace list
with only the stuff we need.

I agree the current code is quite naive but it does not affect
performance so much that it makes the operation unusable.
pacman -S kde gnome --print --printformat '%n %v %l %r %s' takes 0.7 second.

>
> OK, and now for some usability testing.
>
> $ ./src/pacman/pacman -Q
> <big list of packages>
> ...
> zope-interface 3.5.1-1
> zvbi 0.2.33-1
>
> $ ./src/pacman/pacman -Q --print
> error: no targets specified (use -h for help)
>
> So two things here- I expected --print to work and it did not, and we
> already have the standard "pkgname <space> pkgver" output that our
> default print format uses. Do we want to invent a new one with the
> default --print for remove?
>
> Oh wait, does --print even work for -Q? Now I'm all confused. :)
> $ ./src/pacman/pacman -Q --print pacman-git
> error: package "pacman-git" not found
>
> $ ./src/pacman/pacman -Q pacman-git
> pacman-git 20091018-1
>
> More unexpected behavior...
>
> $ ./src/pacman/pacman -Ss --print pacman
> core/pacman 3.3.3-1 (base)
>    A library-based package manager with dependency support
> core/pacman-mirrorlist 20090616-1 (base) [installed]
>    Arch Linux mirror list for use by pacman
> extra/namcap 2.4-1 [installed]
>    A Pacman package analyzer
>
> I wanted a list of URLs for all packages that matched my pacman
> search, I did not get that...
>
> -Dan
>
>

About that last part, I already replied one month ago :)
Since the changes I just did were quite trivial, I will just push an
updated version to my print branch.