On Wed, Nov 26, 2014 at 09:10:33PM +0100, Ingo Schwarze wrote: > Hi Baptiste, > > Baptiste Daroussin wrote on Mon, Nov 24, 2014 at 03:36:29PM +0100: > > > Here is a new patch that allows to open gzipped .so files > > as well as looking inside mpath for the files requested. > > This is a terrible layering violation for three (!) reasons: > > 1. The mandoc toolbox consists of code in two major categories: > a) code that is handling files irrespective of context > b) code that is aware of file context / file systems > While b) code is allowed to use a) code, the reverse > is not true. > The file read.c is part of the file handling code a), while > manpath.h is part of the b) interface; so read.c cannot > include manpath.h. > It is true that the implementation of .so in roff.c (which > is also part of a) slightly strains the "irrespective of > context" paradigm by accessing another file. But at least > it retrieves the file in exactly one well-defined place > near the original file, and this strain is unavoidable > if we want to support .so at all. > Widening the gap by making read.c search for files along > the path is neither acceptable nor needed. > > 2. As proposed, the patch is horribly inefficient. > For some purposes, the function mparse_readfd() is called > in inner loops, so it is not acceptable to do such expensive > operations in there. > Explained differently, we must not recalculate the manpath > for each and every file we parse. Think about makewhatis(8), > for example. > Calculating the manpath belongs on the level of the main > program, not into some inner loop in a library. > > 3. The patch utterly breaks the library hierarchy. > The function mparse_readfd() is a libmandoc interface > as documented in mandoc(3), while manpath.h is not. > While manpath.c can (and does) use libmandoc, the reverse > cannot happen. > You see part of the havoc wrought in that you suddenly > need manpath.o to link demandoc(1) even though that > program does not use the manpath.h interface. > > I have to think about a better way to solve this. > Maybe i should make mparse_readfd() use mparse_open() > and extend mparse_open a bit to use file.gz if file > is not found. Or something like that. > It shouldn't take too long to figure out. I was expecting the above :) What about using zlib directly, that could simplify a bunch of things. > > > This allows zshall manpage which contains: .so man1/zshmodules.1 > > to work with mandoc, tested on freebsd this .so find as expected > > /usr/local/man/man1/zshmodules.1.gz > > The .so links are a bad idea in the first place, so it would be > good to get rid of them completely. But admittedly, that's a > separate matter. Right now in the ports tree I'm ripping them at package creation using soelim from groff and I wrote minimalistic version in base to be able to continue using that when groff will be removed. regards, Bapt