11 Jul
2006
11 Jul
'06
7:05 p.m.
On Tue, Jul 11, 2006 at 07:43:09PM +0200, Johannes Winkelmann wrote: > - a port changing one of its binaries to be suid root (or not suid root > anymore), or any other mode change > - a port (e.g. a daemon) changing to be run as nobody from root, thus > requiring its data file to be owned by nobody:nobody > - same problem: a port starting to use a particular group (think: video, > audio etc.) which previously used 'root' User warned, if it's ok to user, then he/she pass -f option. But if mode changes happened silently, then something may break (own scripts, cron jobs...) > - human errors where a directory wasn't writable or a file not > executable, and fixed by the maintainer by changing a mode/ownership > of some files or directories User warned, maintainer noticed, maintainer got heads-up quickly after erroneous commit. Further more, if maintainer tried to pkgadd own port, he will noticed himself first. > > > > This patch fixes bugs mentioned in subject. > > > > > > > > The patch is about the same size as summary sizes of all *.cc files. > > > > But! Please, don't be scared, there are really small amount of logic > > > > changes. > > > Then it should be a small patch :-) > > > > Mmm.. no. :-) That is a point of refactoring: [...] > Well, I meant the bugfix which could be a small(er) patch when separated > from the refactorings. The bugfix is impossible with old meanings of "file". Refactoring is about to teach pkgutils to see not only filename, but pair of filename and its info. If you think that w/o splitting and debugging changes the patch would be far smaller, you are very wrong. > > > > > Most line-consuming changes are: new types, old types typedef'ed, > > > > long functions splitting, new debugging code, pkgadd.cc changes regarding > > > > indentation and long lines. > > > I'd appreciate if you could split it up. > > > > It's hard to do, because things is somewhat twisted. > I'd tend to disagree that it's hard, but maybe I'm looking at it from a > different point of view: From a quick glance, I see the following quite > distinct refactorings: > > - replace NDEBUG/cerr with dbg macros > - introduce typedefs for existing template types > - extract method in config parsing > - extract resolve_conflicts() > - make make_keep_list a template > - extract pkginfo_t out of pkgutil class > - change indentation in void pkgadd::run() which looks like it could be > done before any other changes (didn't investigate that closer, > though). I can't produce split like that. Things twisted on each other. Each such split will not compilable if others not applied. See below. > In addition, there's the bugfix itself. That would be eight patches > which all introduce one change (be it logical or cosmetical), and none > of them would be 200+ lines. > > > > If it's really > > important you to read changes splitted, I will try. > It seems quite important, for two reasons: > - isolated changes are easier to understand, and therefore potential > problems/errors are easier to spot ack > - we can reject one particular change without rejecting the whole > feature Doubtfully. There are two changes you can drop: debugging and read_config splitting. Both drops will harm readability. All others changes are necessary. I can split patch for only three pieces: debug, bugfixing by refactoring, splitting read_config. But I don't see much sense of such split. > > Yes, I use and like typedefs. So do Per as far as I can see, because he > > introduced packages_t in pkgutil.h, but for unknown reason he do not use > > it in many places of code. I suppose he just don't find enough time to > > switch all set<> to proper types. This what I've done. > Or one could say that he decided to use the proper types throughout the > code, and forgot to switch the package_t typedef. Only he can tell. > > > > For the most of time stl algorithms do not care which type you use, be it > > vector, deque or set. It's all containers, which is nameless, and it's up > > to programmer to name it. > I can only agree to some extend here. While the algorithm might work > either way, it can still have a performance impact. So if efficiency is > a criteria, the type matters. Also, depending on the problem it matters > a lot whether a container allows duplicate values etc. > > In those cases, it's important to know which type we're dealing with, > and my argument is that it's easier to keep track of type if they're not > hidden by typedefs. > > > The code will rather ugly and obscure without typedefs. That is why typedef > > invented, btw. ;-) > Well, if I see > std::pair<string, string> file; You got error here: it's pair<string,fileinfo_t>. With typedef you are safe from errors like that. > I can immediately tell the type of 'file'. If it's > file_t file > I have to look it up. You always have to look up or remember things. You have to lookup map<> functions, vector<> functions its types, differences and whatnot. Also you have to look up pkgutil functions, its arguments, the order of arguments, return types. Now you have to remember or look up few other things. I don't see the point of using explicit types. > So if you ask me, it's the typedefs which are obscure, since they hide > information. Don't get me wrong, typedefs are handy, but in cases like > - void db_rm_pkg(const string& name, const set<string>& keep_list); > + void db_rm_pkg(const string& name, const filenames_t& keep_list); > > I strongly prefer the explicit form. Also, for the record, I don't > consider the template notation ugly, although I'm aware that not > everyone shares this opinion :-). > > Regards, Johannes > -- -- Anton (irc: bd2)