[pacman-dev] [PATCH] Introduce -D, --database

[Xavier]

2009/9/9 Nagy Gabor <ngaba at bibl.u-szeged.hu>:
>>
>> Not so fond of this way of organizing (or disorganizing?) the manpage.
>> We don't do this anywhere else.
>
> --asdeps and --asexplicit are listed in "general options" section,
> that's why I've chosen this solution. I am fine with moving them to -S,
> -U and -D sections. Atm there is no -Q and -D options sections, so maybe
> we should just remove the 2 added lines from --asdeps/--asexplicit, and
> make the -D description more concrete, I dunno.

I would be fine with that.
see http://code.toofishes.net/cgit/xavier/pacman.git/commit/?h=universal&id=a58c95a80668814546dbe0ce78dd1eaf9611f293

>>
>> This seems silly to me. Why can't the signature be the standard
>>     int alpm_db_set_pkgreason(pmpkg_t *pkg, pmpkgreason_t reason);
>> ? Just ensure there is a check that it isn't a package file, and it
>> saves all the lookup junk since that is a public API anyway.
>
> This was done for safety reason, I even didn't want to add the db
> param, and force the use of local db (but see [*]). Well, probably the
> front-end cannot do so much dirty things (store pmpkg_t, remove that
> package, call alpm_db_set_pkgreason), so maybe I was too paranoid. But
> I still think this is better. (The main problem is that after a pmpkg_t
> is duplicated, it seems like the new pmpkg_t is the part of pkgcache,
> but it is not, it is "detached".) The speed is not an issue here,
> the only difference between your and my proposal is that front-end or
> alpm scans pkgcache for name.
>
> [*] One more weak reasoning: ~all alpm_db functions have db param.
>
> But if I didn't convince you, of course I will change this.
>

In my opinion, this operation is quite new and special, and there is
no standard to follow.
If we introduce new operations of this kind, then these should be
consistent. Othewise, no big deal imo.

I agree with removing db parameter and forcing local db usage, I had
the same thought when reading the patch.

>> > +
>> > +       _alpm_log(PM_LOG_DEBUG, "setting install reason %u for %s/%s\n", reason, db->treename, name);
>> > +       /* read DESC */
>> > +       if(_alpm_db_read(db, pkg, INFRQ_DESC)) {
>> > +               return(-1);
>> > +       }
>> > +       if(pkg->reason == reason) {
>> > +               /* we are done */
>> > +               return(0);
>> > +       }
>> > +       /* set reason (in pkgcache) */
>> > +       pkg->reason = reason;
>> This is scary- manual DESC level reading, direct ->reason accessors-
>> don't we abstract this all away in package.c?
>
> I don't agree here. pkg_get_reason would be equivalent here, but I
> still think this is better. We want to read the _whole_ DESC file here,
> because we want to write it back to hdd later (after changing reason).
> Of course the same thing happens with pkg_get_reason, but imho this is
> more suggestive. _alpm_db_read is intelligent, if DESC is already
> loaded, it won't be loaded again.
>
> However, there is a valid concern here: What if we move reason field to
> another file, and we will forget about this... Then we should also
> change the _alpm_db_write line too, so this is not a problem of
> _alpm_db_read usage. (Btw, database*.py pactests would catch this
> immediately.)
>

I  think you had a consistent usage of db_read and db_write.

If we really want to use pkg_get_reason instead, why don't you
introduce a new pkg_set_reason next to it (this would do db_write
desc) ?