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

> > +#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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to