Re: [PATCH v3] pager: move pager-specific setup into the build

[Jeff King <peff@peff.net>]

On Thu, Aug 04, 2016 at 11:34:10AM +0000, Eric Wong wrote:

> > > --- a/config.mak.uname
> > > +++ b/config.mak.uname
> > > @@ -209,6 +209,7 @@ ifeq ($(uname_S),FreeBSD)
> > >  	HAVE_PATHS_H = YesPlease
> > >  	GMTIME_UNRELIABLE_ERRORS = UnfortunatelyYes
> > >  	HAVE_BSD_SYSCTL = YesPlease
> > > +	PAGER_ENV = LESS=FRX LV=-c MORE=FRX
> > >  endif
> > 
> > Is it worth setting up PAGER_ENV's default values before including
> > config.mak.*, and then using "+=" here? That avoids this line getting
> > out of sync with the usual defaults.
> 
> Good point, but it makes ordering problematic for folks
> who want to override it config.mak or command-line.

I'm not sure it changes much for them. Their "=" in config.mak, etc,
would override our default, and anything on the command line overrides
all of the in-Makefile stuff anyway. The only difference would be if
they use "+=" in config.mak, but there I think it would be an
improvement.

I'm OK to leave it as-is until somebody actually cares, though.

> > I know you said you don't like string parsing in C. Here is a patch (on
> > top of yours) that converts the parsing to shell, and generates a
> > pre-built array-of-struct (this is similar to the big series I posted
> > long ago, but just touching this one spot, not invading the whole
> > Makefile). Feel free to ignore it as over-engineered, but I thought I'd
> > throw it out there in case it appeals.
> 
> Yeah, but I'd rather not introduce more complexity into the
> build process, either (unless it's a performance-sensitive part,
> which this is not).  Also, while my original 2/2 to make it
> configurable at runtime was discarded, I wouldn't rule out
> somebody making a compelling case for it and it would be
> an easier change from the parse-at-runtime code.

Yeah, I had similar thoughts while writing it.

Your v4 patch looks fine to me.

-Peff