Re: [PATCH 2/3] setup_pager: set MORE=R

From: Jeff King <peff@peff.net>
Date: Jan 21, 2014, 05:49 UTC
Message-ID: <20140121054927.GD5878@sigill.intra.peff.net>
In-Reply-To: <xmqqvbxiwh8y.fsf@gitster.dls.corp.google.com>

On Fri, Jan 17, 2014 at 11:17:01AM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> > +#ifdef PAGER_MORE_UNDERSTANDS_R
> > +	if (!getenv("MORE"))
> > +		argv_array_push(&env, "MORE=R");
> > +#endif
> >  	pager_process.env = argv_array_detach(&env, NULL);
> >  
> >  	if (start_command(&pager_process))
> 
> Let me repeat from $gmane/240110:
> 
>  - Can we generalize this a bit so that a builder can pass a list
>    of var=val pairs and demote the existing LESS=FRSX to just a
>    canned setting of such a mechanism?
> 
> As we need to maintain this "set these environments when unset" here
> and also in git-sh-setup.sh, I think it is about time to do that
> clean-up.  Duplicating two settings was borderline OK, but seeing
> the third added fairly soon after the second was added tells me that
> the clean-up must come before adding the third.

Wow, I somehow _completely_ missed that thread. When I looked at the code with LV, I assumed it was just something that had happened long ago and I had forgotten about. I didn't even bother reading "git log".

Yeah, I agree that git-sh-setup should be consistent with what the C porcelains do. However, adding build-time variables like:

  PAGER_ENV = LESS=-FRSX LV=-c

does complicate the point of my series, which was to add more intimate logic about how we handle LESS. With the current code, I can know that an unset "LESS" variable means we will set "R" ourselves and turn on color. We can get around that with something like:

  int pager_can_handle_color(void)
  {
        const char *pager = get_pager();

        if (!strcmp(pager, "less")) {
                const char *x = getenv("LESS");
                if (!x) {
                        const char *e;
                        for (e = pager_env; *e; e++)
                                if ((x = skip_prefix(e, "LESS=")))
                                        break;
                }
                return !x || strchr(x, 'R');
    }

    [...]
  }

but we are still hard-coding a lot of intelligence about "less" here. I wonder if the abstraction provided by the Makefile variable is really worthwhile. Anybody adding a new line to it would also want to tweak pager_can_handle_color to add similar logic.

Taking a step back for a moment, we are getting two things out of such a Makefile variable:

  1. An easy way for packager to add intelligence about common pagers on
     their system.

  2. DRY between git-sh-setup and the C code.

I do like (1), but I do not know if we want to try to encode the "can handle color" logic into a Makefile variable. What would it look like?

For (2), an alternate method would be to simply provide "git pager" as C code, which spawns the appropriate pager as the C code would. Then git-sh-setup can easily build around that.

-Peff