2009/9/9 Nagy Gabor <ngaba@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) ?