Git reflog shouldn't always open the pager

[Avishayy]

I don't know if this is done by design to avoid implementing it correctly for each command, but the only time the pager should open for git reflog is when calling it as is (or with show, which is the default behaviour).

I found this out because I used fugitive to write a vim function that mimicks something like git stash push -k that doesn't push the index to the stash (i.e stashing only whatever is not in the index), it made the reflog messy (I solved it by creating a commit for whatever is in the index and then calling git reset --soft HEAD~1) so I used git reflog delete to cleanup things a bit, but it opened the pager so for now I am just closing the window :)

If fixing this seems right for fugitive, I could try and do it myself.

[tpope]

The goal is to implement it correctly for each command. Ideally we'd let Git decide instead, but Git disables the pager if there isn't a PTY, with no way to override.

[Avishayy]

A few things:

1. For documentation case — if anyone needs to do something similar, you can disable the pager with -P | --no-pager, i.e. I could workaround the issue by calling Git -P reflog delete abcd. Yes, this is a git feature, fugitive implements it as expected but it has no mention of it in the documention (even though -p | --paginate does). Perhaps this should be documented too?
2. @tpope How would you imagine an ideal implementation of such feature? git is open source after all and I don't think they'd mind too much if someone proposed a way to support this. I have some ideas:

• The easiest (and possibly the worst) solution I can think of is a new flag that just returns whether the current git command would use the pager. This is bad for obvious reasons, but it will work.
• A spinoff of the previous solution is to use the flag to create an alternative output method that fugitive will read to determine if a pager is needed without affecting the git command itself. (a file? I'm not sure what else might be appropriate here)
• Somewhat like the previous solutions, but a callback. Similar to git hooks, git will run an executable and pass the pager status to it.
  I'd love to hear your insight on this, I plan on asking on the git mailing list as well, because why not?

3. Thank you for fixing this issue quickly, much appreciated :)

[tpope]

Perhaps this should be documented too?

--paginate gets called out because it has a unique behavior you might not expect. --no-pager, on the other hand, has a very obvious behavior, even if it's not obvious you can use it all. I could see making this a bit clearer, but I don't want to enumerate every supported option - there's too many of them.

How would you imagine an ideal implementation of such feature?

Git consults isatty() to determine whether to enable the pager:

https://github.com/git/git/blob/670b81a890388c60b7032a4f5b879f2ece8c4558/pager.c#L102-L107

The utility function it calls short circuits if it's false, with no way to override:

https://github.com/git/git/blob/670b81a890388c60b7032a4f5b879f2ece8c4558/pager.c#L44-L65

The most straightforward solution would be an environment variable that takes precedence. Something that changes the conditional to perhaps look like:

        if (!git_env_bool("GIT_PAGER_FORCE",` stdout_is_tty))
                return NULL;

With this in place, I could set GIT_PAGER_FORCE=1 and GIT_PAGER to some custom script that signals back to Fugitive that we should capture to a temp buffer. The name GIT_PAGER_FORCE is pretty lackluster - it could be interpreted as applying to all commands as with --paginate - but I'm struggling to top it.

Your ideas sound more convenient than mine, but also higher effort and lower overall value outside of Fugitive. I wouldn't oppose them, but I wouldn't expect Git core to share that opinion.

[Avishayy]

Oh, I haven't thought about using GIT_PAGER, that's cool. Though perhaps GIT_PAGER_FORCE (or whatever we might call it) should just include the path to the custom script, because setting GIT_PAGER_FORCE with a pty GIT_PAGER / PAGER when we don't have a tty doesn't make any sense.

Your ideas sound more convenient than mine

I disagree, your idea sounds much more convenient as it fits exactly to Fugitive's need. I'll create a discussion over in git's mailing list and report back soon, thanks!

[Avishayy]

Hm.. I have a question, why does git not recognize the pty? Is this just to support Windows? Or is this because vim doesn't pass the pty to git?

In fact, I played a bit with autoload/fugitive.vim and changed the PAGER and GIT_PAGER values from cat to rev and saw no change, even though I'm running from a pty (nvim) and getting into that flow by using :Git -P reflog show

[Avishayy]

Okay I played a bit with nvim by calling :!sleep 50 and :call jobstart(["sleep", 56], {'pty': 1}), grepping their pid and looking at the fd lists. The former doesn't have a tty at all and the latter has a new pty, I guess the commands that have no pager use the pty for whatever git might need such as passwords, but I'm not sure why setting the pager to rev didn't yield any results. (The only explanation I have is that git short circuits in git_pager because there's no tty, but there is?)

[tpope]

Windows is the primary place we can't use a PTY; it's also not available on older versions of Vim. You're not seeing results because your -P is passed along to Git and disables the pager. You'll need to change #PagerFor() to prevent Fugitive from sending :Git reflog show directly to a temp file.

[tpope]

Now that I think about it, the lack of a PTY burns us in a few other places. To name 2 examples, :Git push hinges its use of a progress bar on the presence of a PTY, when really we'd rather have it always enabled, and :Git merge defaults to not editing the commit message if no PTY is present, while we want it to invoke the editor. So the behavior I'd like to see is actually a bit simpler: if something like GIT_STDOUT_ISTTY=1 is set, then assume the presence of a dumb terminal even if istty(1) is false.

[Avishayy]

You're not seeing results because your -P is passed along to Git and disables the pager.

Oh, right, I forgot that it is an actual git flag.

:Git push hinges its use of a progress bar on the presence of a PTY, when really we'd rather have it always enabled

I'm not sure I understand how GIT_STDOUT_ISTTY will help here, progress bars need an actual tty to control where the characters are written.

Perhaps you are planning on parsing the escape sequences that will be written to your PAGER and then displaying it as you want?

[tpope]

Progress bars use nothing more than a carriage return to reset the cursor position to the beginning of the line, which Vim's :echon can do just fine. More generally, if TERM=dumb, Git won't use anything fancier than a carriage return.

[Avishayy]

Oh that's neat, TIL about TERM=dumb.

I'm dropping the link to the discussion at the mailing list here https://lore.kernel.org/git/CAJ-0Osy9JhGD0=6eF3jgZuoHJEzymksCWZZZC+A4FtHxzOrdhA@mail.gmail.com/T/#u

[tpope]

Resisting the urge to join that thread (I reserve the right to change my mind) but I want to clarify that while thread pays lip service to "
Forcing git to always act like it has a pty", the implementation still seems laser targeted at pagination. My latest proposal is to make this affect all uses of isatty(). For example, here's one that decides whether git revert will default to using an editor or not which we would want to change:

https://github.com/git/git/blob/670b81a890388c60b7032a4f5b879f2ece8c4558/sequencer.c#L2043

If I were to propose an implementation, it would be to add a utility function:

int git_isatty(int fd)
{
	return git_env_bool("GIT_FORCE_TTY", isatty(fd));
}

And then replace all calls to isatty() with that utility function.

[Avishayy]

Please do join the thread, your understanding of the issue is probably better than most people's view. The new suggestion seems nice, but we should probably check each call site and make sure it makes sense everywhere.