Re: Bug: pager.<cmd> doesn't work well with editors

From: Jeff King <peff@peff.net>
Date: Sep 22, 2016, 06:47 UTC
Message-ID: <20160922064730.277nzkqlxbcx2kjg@sigill.intra.peff.net>
In-Reply-To: <xmqqponxb56a.fsf@gitster.mtv.corp.google.com>

On Wed, Sep 21, 2016 at 09:15:09AM -0700, Junio C Hamano wrote:

Show 19 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > And this isn't really limited to the editor. It's more _annoying_ with
> > the editor, but really "pager.tag" does not make any sense to set right
> > now, because it is handled outside of the "tag" command entirely, and
> > doesn't know what mode the tag command will be running in.
> 
> Stepping back even further, perhaps the whole pager.<cmd> was a bad
> interim move.  For those who set "less" without "-F", being able to
> set pager.<cmd> to false may still be necessary, but I am wondering
> about setting it to true or a command string here.
> 
> It did mean well and may have helped when "git <cmd>" that produces
> reams of output had not yet learned to auto-paginate as a stop-gap
> measure by allowing users to set pager.<cmd>, but I wonder if the
> ideal course of action was to identify (or "wait until people show
> their desire") individual operating modes of various commands and
> teach them to auto-paginate.  For example, "tag -l" may be one of
> them that we would want to teach to.

I don't think it is a bad move overall. I use "pager.log" to pipe through a specific command (that is different than I would use for other commands).

So I like the idea of configurability; the problem is just that it is happening at the wrong level. The individual commands should be in charge of it, with something like:

  /*
   * See if pager.log is configured, falling back to "true" (due to the
   * second argument). If so, and stdout is a tty, run the pager.
   *
   * This would be run at the top of cmd_log().
   */
  setup_auto_pager("log", 1);

...

  /*
   * As above, but note that we run this in cmd_tag() only at the right
   * moment. We'd probably actually flip the "0" here to a "1", but
   * this represents the current default.
   */
  if (mode_list)
	  setup_auto_pager("tag.list", 0);

That's a lot more boilerplate (each command needs to decide if and when it should support the pager), but a one-liner in each spot is not so bad. Both builtins and external could use the C interface, but it's trickier for other languages to redirect their own stdout (we want the pager to run as a separate process, but we also need to wait for it at the end).

Though I suspect for most cases, external scripts are really paging the output of some other command anyway. So it would be enough to provide something like:

  if git pager --query bisect.log
  then
	bisect_log | git pager bisect.log
  else
	bisect_log
  fi

(you can lose the "--query" form if you don't mind _always_ piping through "cat" when there's no pager in use, but that seems like a pointless inefficiency).

So I think it's all workable, and for the most part would even remain backwards compatible, with the exception that "pager.foo" would not work for a third-party "git-foo" until the author updates it to call "git pager".

I don't have a particular plan to work on it anytime soon, but maybe somebody could pick it up as relatively low-hanging fruit.

-Peff