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

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

[Anatoly Borodin]

Hi All!

It can be useful to enable `pager.branch`, `pager.tag`, `pager.config`, etc
for some projects (`git` itself can be a good example, with all its feature
branches and tags).

But it makes commands like `git branch --edit-description`, `git tag -a`,
`git config -e` extremely unhappy. For example, `vim` says

	Vim: Warning: Output is not to a terminal

and then becomes unusable (just as `vim | less` would be).

I think, the pagination should be turned off when the editor is being
called.

-- 
Mit freundlichen Grüßen,
Anatoly Borodin

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

[Anatoly Borodin]

> I think, the pagination should be turned off when the editor is being
> called.

... even if the `[-p|--paginate]` option is used explicitly. Is there a
case when pagination shouldn't be ignored with an editor?

`git -p -c core.editor=gvim config -e` works, but the pagination is not
effective.

`git -p -c core.editor=vim config -e` doesn't work and wants to be killed.

I've tested it on FreeBSD and Linux, have no idea, how it works on Mac or
Windows.

-- 
Mit freundlichen Grüßen,
Anatoly Borodin

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

[Junio C Hamano]

Anatoly Borodin <anatoly.borodin@gmail.com> writes:

>> I think, the pagination should be turned off when the editor is being
>> called.

This is a fun one.  IIRC, we decide to spawn a pager and run our
output via pipe thru it fairly early, even before we figure out
which subcommand is being run (especially if you do "git -p
subcommand"), which by definition is way before we know if that
subcommand wants to let the user edit things with the editor.

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

[Jeff King]

On Mon, Sep 19, 2016 at 09:03:05AM -0700, Junio C Hamano wrote:

> Anatoly Borodin <anatoly.borodin@gmail.com> writes:
> 
> >> I think, the pagination should be turned off when the editor is being
> >> called.
> 
> This is a fun one.  IIRC, we decide to spawn a pager and run our
> output via pipe thru it fairly early, even before we figure out
> which subcommand is being run (especially if you do "git -p
> subcommand"), which by definition is way before we know if that
> subcommand wants to let the user edit things with the editor.

Right. Once the pager is spawned, it is too late to change things (even
if we spawned the editor directed to /dev/tty, it would be racily
competing for input with the pager).

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. So it's
_also_ the wrong thing to do with "git tag foo", which doesn't run an
editor (but you don't tend to notice if you use "less -F" because the
pager just quits immediately).

So you really only want to page tag-listing (and the same for branch,
config, etc). A long time ago I had a patch to add "pager.tag.list", and
the tag command would decide whether and how to page after realizing it
was in listing mode. Looks like I did send it to the list:

  http://public-inbox.org/git/20111007144438.GA30318@sigill.intra.peff.net/

though I think there are some rough edges (like handling "git stash
list").

I also wonder if there are any commands that actually have more than one
sub-command. So another option would be to teach the main git.c a
blacklist of "do not respect pager config" commands (like tag), and then
tag itself could decide to respect pager.tag at the right moment.

I think that makes things worse for a third-party command, though; we
cannot know whether a script "git-foobar" dropped into the $PATH would
like us to respect pager.foobar or if it would prefer to decide itself
later.

-Peff

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

[Junio C Hamano]

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.

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

[Jeff King]

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

> 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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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 level at which configurability happens might be one issue
(i.e. you may want different pager for two operating modes for the
same command, hence your need to use "tag.list" not just "tag"), but
I think another issue is that it conflates if the output need to be
paged (on/off) and what pager should be used when the output is
paged.  When we see that a user sets "pager.tag", we should not have
made it an instruction to Git that _all_ output from "git tag" must
be paged.

If there were no need for supporting separate pagers per operating
mode of a Git command, say "git tag", you would not want to page the
output unless you are producing "git tag [-l]" listing.  You do not
want your interaction with the usual "git tag <name> [<an object>]"
to be paged, even if you want to use a pager different from GIT_PAGER
when you are viewing the tags.

It is good that each codepath can give default in this example

> The individual commands should be in
> charge of it, with something like:
>
>   setup_auto_pager("log", 1);
> ...
>   if (mode_list)
> 	  setup_auto_pager("tag.list", 0);

as the second parameter to setup_auto_pager(), but I think the first
parameter being "tag.list" vs "tag" is a separate issue.  Until
there comes another codepath in "git tag" that wants to call
setup_auto_pager(), it does not make any difference from the end-user's
point of view.  Starting with "tag.list" may futureproof it
(e.g. perhaps somebody wants to use a separate pager for "git tag --help"
and "tag.help" can be added without disrupting existing use of "tag.list")

So I think we are fundamentally on the same page; it is just you are
aiming higher than I was, but we both recognize the need for separate 
codepaths in a single command to decide if the output should be paged.

> 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.

;-)

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

[Jeff King]

On Thu, Sep 22, 2016 at 10:19:32AM -0700, Junio C Hamano wrote:

> The level at which configurability happens might be one issue
> (i.e. you may want different pager for two operating modes for the
> same command, hence your need to use "tag.list" not just "tag"), but
> I think another issue is that it conflates if the output need to be
> paged (on/off) and what pager should be used when the output is
> paged.  When we see that a user sets "pager.tag", we should not have
> made it an instruction to Git that _all_ output from "git tag" must
> be paged.

Yes, we could have done it the other way, but I think this was a natural
consequence of implementing it git.c. It _only_ knows about "all output
from git-tag" and nothing else.

At any rate, I do not see much point in moving away from it even if we
change the underlying implementation to be more flexible, if only
because it would be a gratuitous incompatibility.

> So I think we are fundamentally on the same page; it is just you are
> aiming higher than I was, but we both recognize the need for separate 
> codepaths in a single command to decide if the output should be paged.

Yeah. In my examples there are really two proposed improvements:

  1. The decision over whether and when to start a pager is pushed down
     from git.c into individual commands.

  2. As a side effect of (1), commands must declare "this is who I am"
     to look up the correct config.  But "who I am" no longer needs to
     be a whole command, so we are free to slice up the namespace more
     finely. But we do not have to.

     This might also be an opportunity to add more conditions. Like "run
     the pager if I am doing log output with -p, but not otherwise" or
     something. I dunno. That does not sound useful to me, but maybe
     somebody else would find it so.

And I think you are getting at a (3), which is something like:

  3. The config namespace can be made richer, so that "whether" and
     "how" are split. E.g., "pager.log.command" and "pager.log.enabled"
     or something.

     I do not mind that, but we would probably want to keep "pager.log"
     for compatibility, at which point I wonder if the new system is
     worth the bother.

-Peff