Robert Milkowski wrote: > Hello Brock, > > > See comments below. > > Tuesday, September 30, 2008, 5:32:02 AM, you wrote: > > BP> Webrev: > BP> http://cr.opensolaris.org/~bpytlik/ips-1929-v1/ > > BP> Bug: > BP> pkg list should give an option to display only current latest package > BP> http://defect.opensolaris.org/bz/show_bug.cgi?id=1929 > > BP> This webrev changes the default behavior of list to display only the > BP> most recent version of a package, and the version that's installed, if > BP> one is. It uses the -a option to decide whether to list all packages, or > BP> only those installed, and the -f flag to decide whether to list all > BP> versions, or only up to two. > > Instead of -f which for most users looks like some kind of "force" > option maybe --all would be better? > > My concern is that I find it strange that -a and --all do different things. How does --all-versions sound? My guess is that this will be a rarely used option, so having it be a long option shouldn't be that painful. Other options could be --versions, -V, --av... > BP> I don't feel strongly about changing the default behavior. I don't care > BP> strongly about which options are used to designate what. I do think we > BP> need two options, one for the axis of installed vs all, and one for the > BP> axis of all versions vs up to 2. > > The new default (only most recent version and all installed) is a good > thing. > > Thanks > > BP> Performance: > BP> From my tests, it seems like the new code has the same speed as the > BP> old, or is possibly slightly faster, even when directed to a file for > BP> output, though it may have a slightly longer delay before the first > BP> result is printed to the screen. With the -f, it's about .016 seconds > BP> (on a base of 2.5 seconds) slower than the old pkg list, when both are > BP> directed to files. > > When I was playing with it I remember that pkg.get_name() already > provides you with packages sorted by version with a most recent one > being returned first. So instead of doing sorting again in > last_inventory() something like below is sufficient and probably more > lightweight. > > list_inventory() > last_name = "" > [...] > > if pf != last_name: > msg(fmt_str % (pf, pkg.get_version(), > state["state"], ufix)) > elif state["state"] == "installed": > msg(fmt_str % (pf, pkg.get_version(), > state["state"], ufix)) > > last_name = pf > > > I don't think image.inventory commits to returning its list in a particular order. Since sorting the list seems to take about .02 seconds, I'm inclined to not depend on that feature of inventory, unless I hear that its ordering is going to remain stable.
I tried something similar to what you suggested, and it actually ran a bit slower than the existing code. Honestly, I'm not sure why because I would've expected your approach to prove to be a bit speedier. Thanks for the review, Brock > > Best regards, > Robert Milkowski mailto:[EMAIL PROTECTED] > http://milek.blogspot.com > > _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
