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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html