git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
	Yuri <yuri@rawbw.com>
Subject: Re: [PATCH 2/3] setup_pager: set MORE=R
Date: Tue, 21 Jan 2014 00:49:27 -0500	[thread overview]
Message-ID: <20140121054927.GD5878@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqqvbxiwh8y.fsf@gitster.dls.corp.google.com>

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

  parent reply	other threads:[~2014-01-21  5:49 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-01-17  0:34 'git log' escape symbols shown as ESC[33 and ESC[m Yuri
2014-01-17  1:47 ` Jeff King
2014-01-17  2:02   ` Yuri
2014-01-17  2:13     ` Jeff King
2014-01-17  2:28       ` Yuri
2014-01-17  2:32         ` Jeff King
2014-01-17  2:46           ` Yuri
2014-01-17  2:29       ` Jonathan Nieder
2014-01-17  2:35         ` Jeff King
2014-01-17  3:21           ` Jeff King
2014-01-17  4:14           ` [PATCH 0/3] improved out-of-the-box color settings Jeff King
2014-01-17  4:21             ` [PATCH 1/3] setup_pager: refactor LESS/LV environment setting Jeff King
2014-01-17  4:21             ` [PATCH 2/3] setup_pager: set MORE=R Jeff King
2014-01-17  7:26               ` Kyle J. McKay
2014-01-17 19:11                 ` Junio C Hamano
2014-01-21  5:30                 ` Jeff King
2014-01-21  8:42                   ` Kyle J. McKay
2014-01-23  2:14                     ` Jeff King
2014-01-23 17:22                       ` Junio C Hamano
2014-01-17 19:17               ` Junio C Hamano
2014-01-17 19:57                 ` Junio C Hamano
2014-01-17 23:42                   ` Junio C Hamano
2014-01-21  6:13                     ` Jeff King
2014-01-21 19:28                       ` Junio C Hamano
2014-01-21  5:49                 ` Jeff King [this message]
2014-01-21 19:23                   ` Junio C Hamano
2014-02-04 22:12                     ` Jeff King
2014-02-04 22:17                       ` Junio C Hamano
2014-02-04 22:25                         ` Jeff King
2014-02-04 22:45                           ` Yuri
2014-02-04 22:48                             ` Jeff King
2014-02-04 22:54                               ` Junio C Hamano
2014-02-04 23:00                               ` Yuri
2014-02-05  2:11                       ` Kyle J. McKay
2014-01-17  4:24             ` [PATCH 3/3] pager: disable colors for some known-bad configurations Jeff King
2014-01-17  9:13             ` [PATCH 0/3] improved out-of-the-box color settings Yuri
2014-01-17 20:15   ` 'git log' escape symbols shown as ESC[33 and ESC[m Yuri
2014-02-05  1:24   ` Yuri
2014-02-05  1:33     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140121054927.GD5878@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jrnieder@gmail.com \
    --cc=yuri@rawbw.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).