Vincent Lefevre wrote on Thu, 04 Jun 2020 10:03 +0200: > On 2020-06-04 03:32:49 +0000, Daniel Shahaf wrote: > > Furthermore, I'd rather not remove code just because it's currently > > unused in zsh.git. The completion system — especially the Type/* > > functions — is an API, not a blackbox. Does the function proposed for > > removal answer a useful question? Might third party tools (or even > > people's zshrc files) be using or in the future use that function? The > > function has already been written (and debugged, etc); how likely is it > > that if we remove it, someone will have to reinvent the wheel? > > An issue is that deinstall can mean 2 things: > 1. A package that has been uninstalled but not purged. > 2. A package that is still installed but is selected for > deinstallation. > > and that is not currently documented in _deb_packages_update_deinstalled. > I fear that a tool that uses it is likely to be buggy.
Even so, I don't see how that's a reason to remove the function. It's quite common for maintainers of completion functions to have to be well acquainted with uncommon functionalities of the tool they work on, and "All packages that have been selected to be deinstalled" sounds (to this not-familiar-with-dpkg(8)-internals reader) like a fair question to ask. It might not be the _right_ question to ask in every case, but that's a separate problem. Note that alongside «_deb_packages deinstalled» there is also «_deb_packages uninstalled». It seems to me that some documentation is in order, to explain the differences between those two functions and the related pitfalls. While we're there, it would be nice to also explain what xinstalled does (people shouldn't have to read the implementation of _deb_packages to figure out the answer to that). > > So, I agree with Axel about the _deb_packages part of the patch. > > > > As to the _dpkg part of the patch, first of all, it's incomplete: > > it removes the "listfiles" case but not the code that sets that value. > > It does not remove "listfiles", but moves it to use "xinstalled" > instead of "installed" (perhaps I forgot to say I fixed that > too). My apologies. I misread the diff. > This listfiles command still makes sense for any package > listed by "dpkg --get-selections", even if it has already been > uninstalled (see above on openntpd). So you're saying that «dpkg -L» should complete xinstalled rather than deinstalled. I don't know dpkg well enough to have an opinion one way or the other. What I will say is that if «dpkg -L foo» works, then «dpkg -L <TAB>» should offer «foo». > > Once the latter is removed too, the function will then display a single > > asterisk to the user instead of an actual description. And with _that_ > > fixed, there'll still be the issue that, where possible, it's better to > > generate completions than to just tell the user what type of thing they're > > supposed to type in. Incorrect or incomplete code should, if possible, > > be corrected rather than axed. > > I don't understand what you mean. > I don't understand what part you didn't understand, but in any case, just ignore that paragraph; it was a consequent misunderstanding of my misreading of the diff. > > I'm not overly familiar with the aptitude/apt/dpkg/dselect hierarchy of > > semantics, but in general, completion should (1) offer everything that > > the command would accept, (2) not offer things the command won't accept. > > This is what my patch corrects for the remove, status, listfiles and > list commands. That's great. > > That's in descending order of priority: it's usually better to offer too > > much than too little. > > Yes, remove, status and listfiles offered too little (only packages > in the install or hold state). On the opposite, list offered too much > (all packages known to apt), but for a package that is not listed by > "dpkg --get-selections", i.e. for which no information is available > for dpkg, I don't see how this can be useful: "dpkg --list foo", > where foo is neither installed, nor uninstalled (but not purged), > just returns an error. Yes, if «dpkg -L foo» returns an error then «foo» shouldn't be completed. > Axel, do you have any concerns about the _dpkg part of the patch? Cheers, Daniel
