about c8af1de9 (git status uses pager)

about c8af1de9 (git status uses pager)

[Jan Engelhardt]

Since git 1.5.6, `git status` always invokes a pager, which is really 
annoying when the output is less than the number of terminal rows 
available. Can I turn that off somehow or do I need to send a reverting 
patch?


Jan

Re: about c8af1de9 (git status uses pager)

[Vegard Nossum]

On Sat, Jun 21, 2008 at 11:21 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>
> Since git 1.5.6, `git status` always invokes a pager, which is really
> annoying when the output is less than the number of terminal rows
> available. Can I turn that off somehow or do I need to send a reverting
> patch?

I think it would work to set PAGER="less -F" (a.k.a. --quit-if-one-screen)?

There's also GIT_PAGER variable, core.pager git setting, etc.


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: about c8af1de9 (git status uses pager)

[Junio C Hamano]

"Vegard Nossum" <vegard.nossum@gmail.com> writes:

> On Sat, Jun 21, 2008 at 11:21 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>>
>> Since git 1.5.6, `git status` always invokes a pager, which is really
>> annoying when the output is less than the number of terminal rows
>> available. Can I turn that off somehow or do I need to send a reverting
>> patch?
>
> I think it would work to set PAGER="less -F" (a.k.a. --quit-if-one-screen)?

Probably better with LESS=FRSX, which is what git uses as a sane default
if nothing is set.

Re: about c8af1de9 (git status uses pager)

[Jan Engelhardt]

On Saturday 2008-06-21 23:42, Junio C Hamano wrote:

>"Vegard Nossum" <vegard.nossum@gmail.com> writes:
>
>> On Sat, Jun 21, 2008 at 11:21 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>>>
>>> Since git 1.5.6, `git status` always invokes a pager, which is really
>>> annoying when the output is less than the number of terminal rows
>>> available. Can I turn that off somehow or do I need to send a reverting
>>> patch?
>>
>> I think it would work to set PAGER="less -F" (a.k.a. --quit-if-one-screen)?
>
>Probably better with LESS=FRSX, which is what git uses as a sane default
>if nothing is set.
>
I went with Vegard's suggestion to change the pager command in
~/.gitconfig, since I have the $LESS environment variable already
defined as "-MSi", and I do not want to change that; because if I am
going to run less (often at the end of a pipe), I certainly do not
want it to just quit on me. So -F in $LESS is a no-no.

Since I need "-MRSi" for git anyhow, tweaking ~/.gitconfig was easy.

Re: about c8af1de9 (git status uses pager)

[Jan Engelhardt]

On Saturday 2008-06-21 23:45, Jan Engelhardt wrote:
>On Saturday 2008-06-21 23:42, Junio C Hamano wrote:
>>"Vegard Nossum" <vegard.nossum@gmail.com> writes:
>>
>>> On Sat, Jun 21, 2008 at 11:21 PM, Jan Engelhardt <jengelh@medozas.de> wrote:
>>>>
>>>> Since git 1.5.6, `git status` always invokes a pager, which is really
>>>> annoying when the output is less than the number of terminal rows
>>>> available. Can I turn that off somehow or do I need to send a reverting
>>>> patch?
>>>
>>> I think it would work to set PAGER="less -F" (a.k.a. --quit-if-one-screen)?
>>
>>Probably better with LESS=FRSX, which is what git uses as a sane default
>>if nothing is set.
>>
>I went with Vegard's suggestion to change the pager command in
>~/.gitconfig, since I have the $LESS environment variable already
>defined as "-MSi", and I do not want to change that; because if I am
>going to run less (often at the end of a pipe), I certainly do not
>want it to just quit on me. So -F in $LESS is a no-no.
>
>Since I need "-MRSi" for git anyhow, tweaking ~/.gitconfig was easy.
>
I've played with `less -F` for a while now, and there is an inherent
problem with that. Or should I say, xterm.
When less quits, the xterm "screen" blits back to the shell output
that was previously visible, so
	git status
with less -F as a pager displays practically *nothing*.
I am aware that this is a less / xterm issue, but I wanted to let
you know.

Re: about c8af1de9 (git status uses pager)

[Junio C Hamano]

Jan Engelhardt <jengelh@medozas.de> writes:

> I am aware that this is a less / xterm issue, but I wanted to let
> you know.

Do you mean "me" by "let you know"?  Well, I have already been aware of
it, thanks.

Re: about c8af1de9 (git status uses pager)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jan Engelhardt <jengelh@medozas.de> writes:
>
>> I am aware that this is a less / xterm issue, but I wanted to let
>> you know.
>
> Do you mean "me" by "let you know"?  Well, I have already been aware of
> it, thanks.

Having said all that, I have to say I am regretting to have accepted that
patch to enable pager on status, not because it bothers me personally (it
doesn't primarily because I practically never run git-status because I
consider the command useless and living almost always in Emacs helps), but
because in principle changing anything that existing users are used to is
bad.

Jeff had a patch to allow boolean configuration variable "pager.<command>"
to override the built-in pager settings during 1.5.6 cycle, and I think it
was a reasonable approach to take.  People who want to page output from
git-status can then set "pager.status = true" in their configuration (and
then we can revert c8af1de (make git-status use a pager, 2008-04-23)).
Alternatively we could keep the current status-quo for the default, and
people can say "pager.status = false" in their configuration.

Re: about c8af1de9 (git status uses pager)

[Jeff King]

On Sun, Jun 22, 2008 at 03:01:07AM -0700, Junio C Hamano wrote:

> Jeff had a patch to allow boolean configuration variable "pager.<command>"
> to override the built-in pager settings during 1.5.6 cycle, and I think it
> was a reasonable approach to take.  People who want to page output from
> git-status can then set "pager.status = true" in their configuration (and
> then we can revert c8af1de (make git-status use a pager, 2008-04-23)).
> Alternatively we could keep the current status-quo for the default, and
> people can say "pager.status = false" in their configuration.

I have been running with the patch for a month or two, and it works fine
for controlling the pager. Unfortunately, there is a nasty interaction
in the git wrapper with reading the config file early, and we end up not
calculating the GIT_DIR and worktree in the same way. I think this is
part of a larger problem which needs solving, but everytime I look at
it, my eyes start bleeding (and I have to admit, since the patch does
work, I have forgotten how annoyed I was at the paging behavior in the
first place, and I don't have as much motivation to work on it).

I think this is deeply related to the "git config alias.st status && cd
.git && git st" problem, which is also on my long-term todo. So I'll see
if I can do something about it during this release cycle.

-Peff

Re: about c8af1de9 (git status uses pager)

[Tim Stoakes]

Junio C Hamano(gitster@pobox.com)@220608-03:01:
> Having said all that, I have to say I am regretting to have accepted that
> patch to enable pager on status, not because it bothers me personally (it
> doesn't primarily because I practically never run git-status because I
> consider the command useless and living almost always in Emacs helps), but
> because in principle changing anything that existing users are used to is
> bad.
> 
> Jeff had a patch to allow boolean configuration variable "pager.<command>"
> to override the built-in pager settings during 1.5.6 cycle, and I think it
> was a reasonable approach to take.  People who want to page output from
> git-status can then set "pager.status = true" in their configuration (and
> then we can revert c8af1de (make git-status use a pager, 2008-04-23)).
> Alternatively we could keep the current status-quo for the default, and
> people can say "pager.status = false" in their configuration.

I'd really like to see this. Setting core.pager to `less -FSRX` or
similar is not useful for me - I *want* to have -X for eg. `git diff`,
but I don't want paging at all for status.

This was quite a nasty change to sneak on people I think.

Tim

-- 
Tim Stoakes

Re: about c8af1de9 (git status uses pager)

[Jeff King]

On Thu, Jul 03, 2008 at 11:45:41AM +0930, Tim Stoakes wrote:

> > Jeff had a patch to allow boolean configuration variable "pager.<command>"
> > to override the built-in pager settings during 1.5.6 cycle, and I think it
> > was a reasonable approach to take.  People who want to page output from
> > git-status can then set "pager.status = true" in their configuration (and
> > then we can revert c8af1de (make git-status use a pager, 2008-04-23)).
> > Alternatively we could keep the current status-quo for the default, and
> > people can say "pager.status = false" in their configuration.
> 
> I'd really like to see this. Setting core.pager to `less -FSRX` or
> similar is not useful for me - I *want* to have -X for eg. `git diff`,
> but I don't want paging at all for status.

OK, here is a revised patch that actually passes the tests. I'm not
incredibly happy with it (see the caveats below), but I think this is
the best we can do without major surgery on the git_dir setup (there
seems to be some nasty interaction between setup_git_env and
setup_git_directory, but several attempts at obvious fixes have left me
pulling my hair out).

-- >8 --
allow per-command pager config

There is great debate over whether some commands should set
up a pager automatically. This patch allows individuals to
set their own pager preferences for each command, overriding
the default. For example, to disable the pager for git
status:

  git config pager.status false

If "--pager" or "--no-pager" is specified on the command
line, it takes precedence over the config option.

There are two caveats:

  - you can turn on the pager for plumbing commands.
    Combined with "core.pager = always", this will probably
    break a lot of things. Don't do it.

  - This only works for builtin commands. The reason is
    somewhat complex:

    Calling git_config before we do setup_git_directory
    has bad side effects, because it wants to know where
    the git_dir is to find ".git/config". Unfortunately,
    we cannot call setup_git_directory indiscriminately,
    because some builtins (like "init") break if we do.

    For builtins, this is OK, since we can just wait until
    after we call setup_git_directory. But for aliases, we
    don't know until we expand (recursively) which command
    we're doing. This should not be a huge problem for
    aliases, which can simply use "--pager" or "--no-pager"
    in the alias as appropriate.

    For external commands, however, we don't know we even
    have an external command until we exec it, and by then
    it is too late to check the config.

    An alternative approach would be to have a config mode
    where we don't bother looking at .git/config, but only
    at the user and system config files. This would make the
    behavior consistent across builtins, aliases, and
    external commands, at the cost of not allowing per-repo
    pager config for at all.
---
 git.c |   51 +++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 22ac522..426a17f 100644
--- a/git.c
+++ b/git.c
@@ -9,6 +9,43 @@ const char git_usage_string[] =
 const char git_more_info_string[] =
 	"See 'git help COMMAND' for more information on a specific command.";
 
+static int use_pager = -1;
+struct pager_config {
+	const char *cmd;
+	int val;
+};
+
+static int pager_command_config(const char *var, const char *value, void *data)
+{
+	struct pager_config *c = data;
+	if (!prefixcmp(var, "pager.") && !strcmp(var + 6, c->cmd))
+		c->val = git_config_bool(var, value);
+	return 0;
+}
+
+/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
+int check_pager_config(const char *cmd)
+{
+	struct pager_config c;
+	c.cmd = cmd;
+	c.val = -1;
+	git_config(pager_command_config, &c);
+	return c.val;
+}
+
+static void commit_pager_choice(void) {
+	switch (use_pager) {
+	case 0:
+		setenv("GIT_PAGER", "cat", 1);
+		break;
+	case 1:
+		setup_pager();
+		break;
+	default:
+		break;
+	}
+}
+
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -38,9 +75,9 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			setup_pager();
+			use_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
-			setenv("GIT_PAGER", "cat", 1);
+			use_pager = 0;
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--git-dir")) {
@@ -242,8 +279,13 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	if (p->option & RUN_SETUP)
 		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
-		setup_pager();
+
+	if (use_pager == -1 && p->option & RUN_SETUP)
+		use_pager = check_pager_config(p->cmd);
+	if (use_pager == -1 && p->option & USE_PAGER)
+		use_pager = 1;
+	commit_pager_choice();
+
 	if (p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
@@ -453,6 +495,7 @@ int main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
+	commit_pager_choice();
 	if (argc > 0) {
 		if (!prefixcmp(argv[0], "--"))
 			argv[0] += 2;
-- 
1.5.6.1.158.g8cb5.dirty

Re: about c8af1de9 (git status uses pager)

[Johannes Schindelin]

Hi,

On Thu, 3 Jul 2008, Tim Stoakes wrote:

> This [changing git status to use the pager automatically] was quite a 
> nasty change to sneak on people I think.

Well, I think that a command producing pages and pages of output without 
stopping is useless.  Therefore, _I_ maintain that it makes tons of sense.

Of course, if you find enough support for a patch reverting it, I might 
have to accept the fact that "git status" becomes less useful for me.

Ciao,
Dscho

Re: about c8af1de9 (git status uses pager)

[Wincent Colaiuta]

El 3/7/2008, a las 14:11, Johannes Schindelin escribió:

> Hi,
>
> On Thu, 3 Jul 2008, Tim Stoakes wrote:
>
>> This [changing git status to use the pager automatically] was quite a
>> nasty change to sneak on people I think.
>
> Well, I think that a command producing pages and pages of output  
> without
> stopping is useless.  Therefore, _I_ maintain that it makes tons of  
> sense.

Ditto. For me, the change was very welcome. Sure, usually the status  
output is short, but on those occasions where it isn't (big file  
reorganizations etc) I was quite sick of doing the "git status, oops  
that output was too long let's try again, git -p status" dance. It was  
also incongruous with my very first experiences with Git, where I had  
been pleasantly surprised that "git log" automatically invoked the  
pager whereas "svn log" just spewed output into my console until ^C.

Cheers,
Wincent

Re: about c8af1de9 (git status uses pager)

[Jeff King]

On Thu, Jul 03, 2008 at 03:37:30PM +0200, Wincent Colaiuta wrote:

>>> This [changing git status to use the pager automatically] was quite a
>>> nasty change to sneak on people I think.
>>
>> Well, I think that a command producing pages and pages of output
>> without stopping is useless.  Therefore, _I_ maintain that it makes
>> tons of  sense.
>
> Ditto. For me, the change was very welcome. Sure, usually the status
> output is short, but on those occasions where it isn't (big file
> reorganizations etc) I was quite sick of doing the "git status, oops
> that output was too long let's try again, git -p status" dance. It was
> also incongruous with my very first experiences with Git, where I had
> been pleasantly surprised that "git log" automatically invoked the
> pager whereas "svn log" just spewed output into my console until ^C.

I think there is not much point in discussing whether "git status"
should use a pager. It is obvious at this point that some people love
it, and some people despise it, and it seems to be a matter of personal
preference. The only sane way forward seems to be configurable paging.
So the next steps are:

  - getting a configurable paging patch that is good enough; I just
    posted another trial, but there are still a few caveats. I would
    love to hear commentary on whether people find it acceptable.

  - once there is a patch, there is still the matter of "on by default,
    make people configure off" versus "off by default, make people
    configure on". And then you can make arguments about how the default
    changes the new user experience, but hopefully there will be a
    little less vehemence on both sides, since it is "here's what new
    users should see" and not "here's what I am forced to see every
    day".

-Peff

Re: about c8af1de9 (git status uses pager)

[Wincent Colaiuta]

El 3/7/2008, a las 21:08, Jeff King escribió:

> On Thu, Jul 03, 2008 at 03:37:30PM +0200, Wincent Colaiuta wrote:
>
>>>> This [changing git status to use the pager automatically] was  
>>>> quite a
>>>> nasty change to sneak on people I think.
>>>
>>> Well, I think that a command producing pages and pages of output
>>> without stopping is useless.  Therefore, _I_ maintain that it makes
>>> tons of  sense.
>>
>> Ditto. For me, the change was very welcome. Sure, usually the status
>> output is short, but on those occasions where it isn't (big file
>> reorganizations etc) I was quite sick of doing the "git status, oops
>> that output was too long let's try again, git -p status" dance. It  
>> was
>> also incongruous with my very first experiences with Git, where I had
>> been pleasantly surprised that "git log" automatically invoked the
>> pager whereas "svn log" just spewed output into my console until ^C.
>
> I think there is not much point in discussing whether "git status"
> should use a pager. It is obvious at this point that some people love
> it, and some people despise it, and it seems to be a matter of  
> personal
> preference. The only sane way forward seems to be configurable paging.

I completely agree. I just don't like it when people use words like  
"nasty" and "sneak" to describe a change that was discussed in one  
short and one long thread prior to being incorporated. But you're  
right; there's no sense in discussing the "whether" any more.

Cheers,
Wincent

Re: about c8af1de9 (git status uses pager)

[Vegard Nossum]

On Sun, Jun 22, 2008 at 11:09 AM, Jan Engelhardt <jengelh@medozas.de> wrote:
> I've played with `less -F` for a while now, and there is an inherent
> problem with that. Or should I say, xterm.
> When less quits, the xterm "screen" blits back to the shell output
> that was previously visible, so
>        git status
> with less -F as a pager displays practically *nothing*.
> I am aware that this is a less / xterm issue, but I wanted to let
> you know.
>

Option -X should do the trick :-)


Vegard

-- 
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
	-- E. W. Dijkstra, EWD1036

Re: about c8af1de9 (git status uses pager)

[Johannes Gilger]

On 21/06/08 23:21, Jan Engelhardt wrote:
> 
> Since git 1.5.6, `git status` always invokes a pager, which is really 
> annoying when the output is less than the number of terminal rows 
> available. Can I turn that off somehow or do I need to send a reverting 
> patch?
> 

Wow, I just noticed it myself. Why was that changed? I don't know about 
your status lines, but I for one find it really annoying. Anything 
that's in a pager isn't visible in my console afterwards. What's next? 
git branch in a pager too?

Regards,
Jojo

-- 
Johannes Gilger <heipei@hackvalue.de>
http://hackvalue.de/heipei/
GPG-Key: 0x42F6DE81
GPG-Fingerprint: BB49 F967 775E BB52 3A81  882C 58EE B178 42F6 DE81

Re: about c8af1de9 (git status uses pager)

[Johannes Gilger]

On 21/06/08 23:42, Johannes Gilger wrote:
> Wow, I just noticed it myself. Why was that changed? I don't know about 
> your status lines, but I for one find it really annoying. Anything 
> that's in a pager isn't visible in my console afterwards. What's next? 
> git branch in a pager too?

Seems like I got ahead of myself there. After setting the core.pager to 
less -FRSX everything works fine ;)

Regards,
Jojo
-- 
Johannes Gilger <heipei@hackvalue.de>
http://hackvalue.de/heipei/
GPG-Key: 0x42F6DE81
GPG-Fingerprint: BB49 F967 775E BB52 3A81  882C 58EE B178 42F6 DE81

Re: about c8af1de9 (git status uses pager)

[Dan McGee]

On 06/22/2008 05:01 AM, Junio C Hamano wrote:
>> Having said all that, I have to say I am regretting to have accepted that
>> patch to enable pager on status, not because it bothers me personally (it
>> doesn't primarily because I practically never run git-status because I
>> consider the command useless and living almost always in Emacs helps), but
>> because in principle changing anything that existing users are used to is
>> bad.

I'm a bit late on the uptake here, but I wanted to throw my two cents in- this was one of the biggest usability flubs I've seen with git in a long time. I suspect (actually, I *know*) there were many users like I that used git-status all the time in the console to get a handle on what their work tree was looking like. That became impossible with 1.5.6 out of the box, and I have to spend an hour reading mailing list posts trying to restore the former non-broken behavior.

Why did this patch get pulled in with so little discussion? Didn't someone think that there must be a reason git-status didn't use a pager before?

On 06/22/2008 02:24 AM, Johannes Gilger wrote:
> On 21/06/08 23:42, Johannes Gilger wrote:
>> Wow, I just noticed it myself. Why was that changed? I don't know about 
>> your status lines, but I for one find it really annoying. Anything 
>> that's in a pager isn't visible in my console afterwards. What's next? 
>> git branch in a pager too?
> 
> Seems like I got ahead of myself there. After setting the core.pager to 
> less -FRSX everything works fine ;)

This is a terrible thing to force users to have to do. Not only that, but the interaction between GIT_PAGER, core.pager, and everything else just makes this change even more hard to swallow.

I probably came across as flame/troll here, but I felt I needed to speak up for those of us that never knew this change was coming. I would go so far as to say this should be reverted for a maint release.

-Dan

Re: about c8af1de9 (git status uses pager)

[Matthieu Moy]

Dan McGee <dpmcgee@gmail.com> writes:

> That became impossible with 1.5.6 out of the box,

Actually, by default it does. It does not for you because you've set
$LESS. That said, I argee that a config option to enable/disable the
pager per-command would have been better that the change we're talking
about.

-- 
Matthieu

Re: about c8af1de9 (git status uses pager)

[Wincent Colaiuta]

El 26/6/2008, a las 5:53, Dan McGee escribió:

> Why did this patch get pulled in with so little discussion? Didn't  
> someone think that there must be a reason git-status didn't use a  
> pager before?

I believe it was discussed in at least a couple of threads back then:

Original patch in late April, 3 messages:

   http://article.gmane.org/gmane.comp.version-control.git/80279/

Follow-up discussion in early May, 31 messages:

   http://article.gmane.org/gmane.comp.version-control.git/80957/

Wincent

Re: about c8af1de9 (git status uses pager)

[Jeff King]

On Thu, Jun 26, 2008 at 12:17:53PM +0200, Wincent Colaiuta wrote:

> Follow-up discussion in early May, 31 messages:
>
>   http://article.gmane.org/gmane.comp.version-control.git/80957/

And I posted several patches in that thread to make paging configuration
more fine-grained, but they got caught up in some setup_git_* bermuda
triangle. Dan (and others who dislike the new change), I would be very
happy if you wanted to take a look at them and see if you can address
some of the issues I raised.

-Peff