the pager

the pager

[Dale R. Worley]

I've noticed that Git by default puts long output through "less" as a
pager.  I don't like that, but this is not the time to change
established behavior.  But while tracking that down, I noticed that
the paging behavior is controlled by at least 5 things:

the -p/--paginate/--no-pager options
the GIT_PAGER environment variable
the PAGER environment variable
the core.pager Git configuration variable
the build-in default (which seems to usually be "less")

There is documentation in git.1 and git-config.1, and the two are not
coordinated to make it clear what happens in all cases.  And the
built-in default is not mentioned at all.

What is the (intended) order of precedence of specifiers of paging
behavior?  My guess is that it should be the order I've given above.

Dale

Re: the pager

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

> I've noticed that Git by default puts long output through "less" as a
> pager.  I don't like that, but this is not the time to change
> established behavior.  But while tracking that down, I noticed that
> the paging behavior is controlled by at least 5 things:
>
> the -p/--paginate/--no-pager options
> the GIT_PAGER environment variable
> the PAGER environment variable
> the core.pager Git configuration variable
> the build-in default (which seems to usually be "less")
> ...
> What is the (intended) order of precedence of specifiers of paging
> behavior?  My guess is that it should be the order I've given above.

I think that sounds about right (I didn't check the code, though).
The most specific to the command line invocation (i.e. option)
trumps the environment, which trumps the configured default, and the
hard wired stuff is used as the fallback default.

I am not sure about PAGER environment and core.pager, though.
People want Git specific pager that applies only to Git process
specified to core.pager, and still want to use their own generic
PAGER to other programs, so my gut feeling is that it would make
sense to consider core.pager a way to specify GIT_PAGER without
contaminating the environment, and use both to override the generic
PAGER (in other words, core.pager should take precedence over PAGER
as far as Git is concerned).

Re: the pager

[Dale R. Worley]

> From: Junio C Hamano <gitster@pobox.com>
> 
> > I've noticed that Git by default puts long output through "less" as a
> > pager.  I don't like that, but this is not the time to change
> > established behavior.  But while tracking that down, I noticed that
> > the paging behavior is controlled by at least 5 things:
> >
> > the -p/--paginate/--no-pager options
> > the GIT_PAGER environment variable
> > the PAGER environment variable
> > the core.pager Git configuration variable
> > the build-in default (which seems to usually be "less")
> > ...
> > What is the (intended) order of precedence of specifiers of paging
> > behavior?  My guess is that it should be the order I've given above.
> 
> I think that sounds about right (I didn't check the code, though).
> The most specific to the command line invocation (i.e. option)
> trumps the environment, which trumps the configured default, and the
> hard wired stuff is used as the fallback default.
> 
> I am not sure about PAGER environment and core.pager, though.
> People want Git specific pager that applies only to Git process
> specified to core.pager, and still want to use their own generic
> PAGER to other programs, so my gut feeling is that it would make
> sense to consider core.pager a way to specify GIT_PAGER without
> contaminating the environment, and use both to override the generic
> PAGER (in other words, core.pager should take precedence over PAGER
> as far as Git is concerned).

I've just discovered this bit of documentation.  Within the git-var
manual page is this entry:

       GIT_PAGER
           Text viewer for use by git commands (e.g., less). The value is
           meant to be interpreted by the shell. The order of preference is
           the $GIT_PAGER environment variable, then core.pager configuration,
           then $PAGER, and then finally less.

This suggests that the ordering is GIT_PAGER > core.pager > PAGER >
default.

Dale

Re: the pager

[Junio C Hamano]

worley@alum.mit.edu (Dale R. Worley) writes:

>> From: Junio C Hamano <gitster@pobox.com>
>> 
>> > I've noticed that Git by default puts long output through "less" as a
>> > pager.  I don't like that, but this is not the time to change
>> > established behavior.  But while tracking that down, I noticed that
>> > the paging behavior is controlled by at least 5 things:
>> >
>> > the -p/--paginate/--no-pager options
>> > the GIT_PAGER environment variable
>> > the PAGER environment variable
>> > the core.pager Git configuration variable
>> > the build-in default (which seems to usually be "less")
>> > ...
>> > What is the (intended) order of precedence of specifiers of paging
>> > behavior?  My guess is that it should be the order I've given above.
>> 
>> I think that sounds about right (I didn't check the code, though).
>> The most specific to the command line invocation (i.e. option)
>> trumps the environment, which trumps the configured default, and the
>> hard wired stuff is used as the fallback default.
>> 
>> I am not sure about PAGER environment and core.pager, though.
>> People want Git specific pager that applies only to Git process
>> specified to core.pager, and still want to use their own generic
>> PAGER to other programs, so my gut feeling is that it would make
>> sense to consider core.pager a way to specify GIT_PAGER without
>> contaminating the environment, and use both to override the generic
>> PAGER (in other words, core.pager should take precedence over PAGER
>> as far as Git is concerned).
>
> I've just discovered this bit of documentation.  Within the git-var
> manual page is this entry:
>
>        GIT_PAGER
>            Text viewer for use by git commands (e.g., less). The value is
>            meant to be interpreted by the shell. The order of preference is
>            the $GIT_PAGER environment variable, then core.pager configuration,
>            then $PAGER, and then finally less.
>
> This suggests that the ordering is GIT_PAGER > core.pager > PAGER >
> default.

OK, that means that my gut feeling was right, we do the right thing,
and we do document it.

But your original "documentation in git.1 and git-config.1, and the
two are not coordinated to make it clear what happens in all cases."
still stands. How can we improve the documentation to make the above
paragraph easier to discover?  Perhaps use the above wording to
update git-config.1 that already mentions GIT_PAGER in the section
for core.pager?

The description over there is so incoherent that I needed to read it
three times to see what points are mentioned.

How about doing this?

-- >8 --
config: rewrite core.pager documentation

The text mentions core.pager and GIT_PAGER without giving the
overall picture of precedences.  Borrow a better description from
the git-var(1) documentation.

The use of the mechanism to allow system-wide, global and
per-repository configuration files is not limited to this particular
variable.  Remove it to clarify the paragraph.

Rewrite the part that explains how the environment variable LESS is
set to Git's default value, and how to selectively customize it.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/config.txt | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ec57a15..7f9bc38 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -553,22 +553,18 @@ sequence.editor::
 	When not configured the default commit message editor is used instead.
 
 core.pager::
-	The command that Git will use to paginate output.  Can
-	be overridden with the `GIT_PAGER` environment
-	variable.  Note that Git sets the `LESS` environment
-	variable to `FRSX` if it is unset when it runs the
-	pager.  One can change these settings by setting the
-	`LESS` variable to some other value.  Alternately,
-	these settings can be overridden on a project or
-	global basis by setting the `core.pager` option.
-	Setting `core.pager` has no effect on the `LESS`
-	environment variable behaviour above, so if you want
-	to override Git's default settings this way, you need
-	to be explicit.  For example, to disable the S option
-	in a backward compatible manner, set `core.pager`
-	to `less -+S`.  This will be passed to the shell by
-	Git, which will translate the final command to
-	`LESS=FRSX less -+S`.
+	Text viewer for use by Git commands (e.g., 'less').  The value
+	is meant to be interpreted by the shell.  The order of preference
+	is the `$GIT_PAGER` environment variable, then `core.pager`
+	configuration, then `$PAGER`, and then the default chosen at
+	compile time (usually 'less').
++
+When the `LESS` environment variable is unset, Git sets it to `FRSX`
+(if `LESS` environment variable is set, Git does not change it at
+all).  If you want to override Git's default setting for `LESS`, you
+can set `core.pager` to `less -+S`.  This will be passed to the
+shell by Git, which will translate the final command to `LESS=FRSX
+less -+S`.
 
 core.whitespace::
 	A comma separated list of common whitespace problems to

Re: the pager

[Dale R. Worley]

So I set out to verify in the code that the order of priority of pager
specification is

    GIT_PAGER > core.pager > PAGER > default

I discovered that there is also a pager.<command> configuration
variable.

I was expecting the code to be simple, uniform (with regard to the 5
sources), and reasonably well documented.  The relevant parts of the
code that I have located so far include:

in environment.c:

    const char *pager_program;

in config.c:

    int git_config_with_options(config_fn_t fn, void *data,
                                const char *filename,
                                const char *blob_ref,
                                int respect_includes)
    {
            char *repo_config = NULL;
            int ret;
            struct config_include_data inc = CONFIG_INCLUDE_INIT;

            if (respect_includes) {
                    inc.fn = fn;
                    inc.data = data;
                    fn = git_config_include;
                    data = &inc;
            }

            /*
             * If we have a specific filename, use it. Otherwise, follow the
             * regular lookup sequence.
             */
            if (filename)
                    return git_config_from_file(fn, filename, data);
            else if (blob_ref)
                    return git_config_from_blob_ref(fn, blob_ref, data);

            repo_config = git_pathdup("config");
            ret = git_config_early(fn, data, repo_config);
            if (repo_config)
                    free(repo_config);
            return ret;
    }

in pager.c:

    /* 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.want = -1;
            c.value = NULL;
            git_config(pager_command_config, &c);
            if (c.value)
                    pager_program = c.value;
            return c.want;
    }

    const char *git_pager(int stdout_is_tty)
    {
            const char *pager;

            if (!stdout_is_tty)
                    return NULL;

            pager = getenv("GIT_PAGER");
            if (!pager) {
                    if (!pager_program)
                            git_config(git_default_config, NULL);
                    pager = pager_program;
            }
            if (!pager)
                    pager = getenv("PAGER");
            if (!pager)
                    pager = DEFAULT_PAGER;
            else if (!*pager || !strcmp(pager, "cat"))
                    pager = NULL;

            return pager;
    }

What's with the code?  It's not simple, it's not uniform (e.g.,
setting env. var. PAGER to "cat" will cause git_pager() to return
NULL, but setting preprocessor var. DEFAULT_PAGER to "cat" will cause
it to return "cat"), and it's barely got any comments at all (a global
variable has *no description whatsoever*).

I'd like to clean up the manual pages at least, but it would take me
hours to figure out what the code *does*.

I know I'm griping here, but I thought that part of the reward for
contributing to an open-source project was as a showcase of one's
work.  Commenting your code is what you learn first in programming.

Dale

Re: the pager

[Matthieu Moy]

worley@alum.mit.edu (Dale R. Worley) writes:

>     const char *git_pager(int stdout_is_tty)
>     {
>             const char *pager;
>
>             if (!stdout_is_tty)
>                     return NULL;
>
>             pager = getenv("GIT_PAGER");
>             if (!pager) {
>                     if (!pager_program)
>                             git_config(git_default_config, NULL);
>                     pager = pager_program;
>             }
>             if (!pager)
>                     pager = getenv("PAGER");
>             if (!pager)
>                     pager = DEFAULT_PAGER;
>             else if (!*pager || !strcmp(pager, "cat"))
>                     pager = NULL;

I guess the "else" could and should be dropped. If you do so (and
possibly insert a blank line between the DEFAULT_PAGER case and the
"pager = NULL" case), you get a nice pattern

if (!pager)
	try_something();
if (!pager)
	try_next_option();
...

> Commenting your code is what you learn first in programming.

Not commenting too much is the second thing you learn ;-).

I agree that a comment like this would help, though:

--- a/cache.h
+++ b/cache.h
@@ -1266,7 +1266,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
 
 /* pager.c */
 extern void setup_pager(void);
-extern const char *pager_program;
+extern const char *pager_program; /* value read from git_config() */
 extern int pager_in_use(void);
 extern int pager_use_color;
 extern int term_columns(void);


-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: the pager

[Dale R. Worley]

> From: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>

> >     const char *git_pager(int stdout_is_tty)
> >     {
> >             const char *pager;
> >
> >             if (!stdout_is_tty)
> >                     return NULL;
> >
> >             pager = getenv("GIT_PAGER");
> >             if (!pager) {
> >                     if (!pager_program)
> >                             git_config(git_default_config, NULL);
> >                     pager = pager_program;
> >             }
> >             if (!pager)
> >                     pager = getenv("PAGER");
> >             if (!pager)
> >                     pager = DEFAULT_PAGER;
> >             else if (!*pager || !strcmp(pager, "cat"))
> >                     pager = NULL;
> 
> I guess the "else" could and should be dropped. If you do so (and
> possibly insert a blank line between the DEFAULT_PAGER case and the
> "pager = NULL" case), you get a nice pattern
> 
> if (!pager)
> 	try_something();
> if (!pager)
> 	try_next_option();

That's true, but it would change the effect of using "cat" as a value:
"cat" as a value of DEFAULT_PAGER would cause git_pager() to return
NULL, whereas now it causes git_pager() to return "cat".  (All other
places where "cat" can be a value are translated to NULL already.)

This is why I want to know what the *intended* behavior is, because we
might be changing Git's behavior, and I want to know that if we do
that, we're changing it to what it should be.  And I haven't seen
anyone venture an opinion on what the intended behavior is.

> I agree that a comment like this would help, though:
> 
> --- a/cache.h
> +++ b/cache.h
> @@ -1266,7 +1266,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
>  
>  /* pager.c */
>  extern void setup_pager(void);
> -extern const char *pager_program;
> +extern const char *pager_program; /* value read from git_config() */
>  extern int pager_in_use(void);
>  extern int pager_use_color;
>  extern int term_columns(void);

First off, the wording is wrong, it should be "value set by
git_config()".

But that doesn't tell the reader what the significance of the value
is.  I suspect that a number of global variables need to be marked:

> /* The pager program name, or "cat" if there is no pager.
>  * Can be overridden by the pager.<cmd> configuration value for a
>  * single command, or suppressed by the --no-pager option.
>  * Set by calling git_config().
>  * NULL if hasn't been set yet by calling git_config(). */
> extern const char *pager_program;

Dale

Re: the pager

[Dale R. Worley]

> I've noticed that Git by default puts long output through "less" as a
> pager.  I don't like that, but this is not the time to change
> established behavior.  But while tracking that down, I noticed that
> the paging behavior is controlled by at least 5 things:
> 
> the -p/--paginate/--no-pager options
> the GIT_PAGER environment variable
> the PAGER environment variable
> the core.pager Git configuration variable
> the build-in default (which seems to usually be "less")

One complication is the meaning of -p/--no-pager:

With the remaining sources, we assume that there is a priority
sequence, and that is used to determine what the pager is.

There is a somewhat independent question of when the pager is
activated.  What I know so far is that some commands use the pager by
default and some by default do not.  My expectation is that
--no-pager can be used to suppress the pager for *any* command.  Is it
also true that -p can force the pager for *any* command, or are there
commands which will not page even with -p?

I assume that if -p is specified but the "which pager" selection is
"cat" (or some other specification of no pager), then there is no
paging operation.

Dale

Re: the pager

[Jonathan Nieder]

Hi,

Dale R. Worley wrote:

> That's true, but it would change the effect of using "cat" as a value:
> "cat" as a value of DEFAULT_PAGER would cause git_pager() to return
> NULL, whereas now it causes git_pager() to return "cat".  (All other
> places where "cat" can be a value are translated to NULL already.)
>
> This is why I want to know what the *intended* behavior is, because we
> might be changing Git's behavior, and I want to know that if we do
> that, we're changing it to what it should be.  And I haven't seen
> anyone venture an opinion on what the intended behavior is.

I don't really follow.

For all practical purposes, "cat" is equivalent to no pager at all,
no?  And the git-var(1) manpage describes the intended order of
precedence, as far as I can tell, except that it was written before
v1.7.4-rc0~76^2 (allow command-specific pagers in pager.<cmd>,
2010-11-17) which forgot to update some documentation.

Suggested wording for improving the documentation or its organization
would of course be welcome.  And I agree with Matthieu that the name
of the pager_program global variable is needlessly confusing ---
perhaps it should be called config_pager_program or similar.

Thanks,
Jonathan

[PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Jeff King]

On Mon, Sep 02, 2013 at 10:27:48PM -0400, Dale R. Worley wrote:

> > I guess the "else" could and should be dropped. If you do so (and
> > possibly insert a blank line between the DEFAULT_PAGER case and the
> > "pager = NULL" case), you get a nice pattern
> > 
> > if (!pager)
> > 	try_something();
> > if (!pager)
> > 	try_next_option();
> 
> That's true, but it would change the effect of using "cat" as a value:
> "cat" as a value of DEFAULT_PAGER would cause git_pager() to return
> NULL, whereas now it causes git_pager() to return "cat".  (All other
> places where "cat" can be a value are translated to NULL already.)
> 
> This is why I want to know what the *intended* behavior is, because we
> might be changing Git's behavior, and I want to know that if we do
> that, we're changing it to what it should be.  And I haven't seen
> anyone venture an opinion on what the intended behavior is.

I'll venture my opinion. We should do this:

-- >8 --
Subject: pager: turn on "cat" optimization for DEFAULT_PAGER

If the user specifies a pager of "cat" (or the empty
string), whether it is in the environment or from config, we
automagically optimize it out to mean "no pager" and avoid
forking at all. We treat an empty pager variable similary.

However, we did not apply this optimization when
DEFAULT_PAGER was set to "cat" (or the empty string). There
is no reason to treat DEFAULT_PAGER any differently. The
optimization should not be user-visible (unless the user has
a bizarre "cat" in their PATH). And even if it is, we are
better off behaving consistently between the compile-time
default and the environment and config settings.

The stray "else" we are removing from this code was
introduced by 402461a (pager: do not fork a pager if PAGER
is set to empty., 2006-04-16). At that time, the line
directly above used:

   if (!pager)
	   pager = "less";

as a fallback, meaning that it could not possibly trigger
the optimization. Later, a3d023d (Provide a build time
default-pager setting, 2009-10-30) turned that constant into
a build-time setting which could be anything, but didn't
loosen the "else" to let DEFAULT_PAGER use the optimization.

Noticed-by: Dale R. Worley <worley@alum.mit.edu>
Suggested-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
Signed-off-by: Jeff King <peff@peff.net>
---
 pager.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/pager.c b/pager.c
index c1ecf65..fa19765 100644
--- a/pager.c
+++ b/pager.c
@@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
 		pager = getenv("PAGER");
 	if (!pager)
 		pager = DEFAULT_PAGER;
-	else if (!*pager || !strcmp(pager, "cat"))
+	if (!*pager || !strcmp(pager, "cat"))
 		pager = NULL;
 
 	return pager;
-- 
1.8.4.2.g87d4a77

Re: the pager

[Jeff King]

On Mon, Sep 02, 2013 at 10:37:45PM -0400, Dale R. Worley wrote:

> > I've noticed that Git by default puts long output through "less" as a
> > pager.  I don't like that, but this is not the time to change
> > established behavior.  But while tracking that down, I noticed that
> > the paging behavior is controlled by at least 5 things:
> > 
> > the -p/--paginate/--no-pager options
> > the GIT_PAGER environment variable
> > the PAGER environment variable
> > the core.pager Git configuration variable
> > the build-in default (which seems to usually be "less")

This list has some orthogonal concepts. The "-p" and "--no-pager"
variables decide _whether_ to run the pager. The GIT_PAGER and PAGER
environment variables, along with core.pager and the compile-time
default, decide _which_ pager to run.

The fact that "cat" or the empty string becomes "no pager" is purely an
optimization (we could fork and run "sh -c ''" or "cat", but it would be
a no-op). So even though you might have instructed git to run the pager,
it may be a noop if your pager is "cat", and we optimize it out.

The confusing one (and missing from your list) is pager.$program, which
originally was a "whether", but later learned to optionally be a
"which". And you also omit the built-in defaults for "whether" on each
command (e.g., "log" runs a pager, "push" does not).

> There is a somewhat independent question of when the pager is
> activated.  What I know so far is that some commands use the pager by
> default and some by default do not.  My expectation is that
> --no-pager can be used to suppress the pager for *any* command.  Is it
> also true that -p can force the pager for *any* command, or are there
> commands which will not page even with -p?

Yes, --no-pager and -p suppress or force, respectively, for any command.
They take precedence over config (pager.$command), which in turn takes
precedence over builtin defaults (per-command defaults, in this case).

Environment variables should generally be less than command-line
options, but greater than config. But there is no "definitely use a
pager" environment variable, so it doesn't apply here.

And I say generally because we should put git-specific environment
variables over git-specific config, but git-specific config over general
environment variables (so similarly we should respect user.email in the
config over $EMAIL in the environment, but under $GIT_COMMITTER_EMAIL).

> I assume that if -p is specified but the "which pager" selection is
> "cat" (or some other specification of no pager), then there is no
> paging operation.

There is a pager in that case, but it doesn't do anything. And then we
optimize it out because it doesn't do anything. :) That is somewhat
tongue-in-cheek, but I hope it shows the mental model that goes into the
decision.

-Peff

Re: the pager

[Jeff King]

On Thu, Aug 29, 2013 at 11:41:56AM -0400, Dale R. Worley wrote:

> I know I'm griping here, but I thought that part of the reward for
> contributing to an open-source project was as a showcase of one's
> work.  Commenting your code is what you learn first in programming.

You will find that the best comments in the git source code are those
written in the commit messages. Learn to use "git blame" (or I recommend
"tig blame" for interactive use), "git log -S", and the new "git log -L"
for finding the commits that touched an area.

It is also sometimes useful to look at the review and discussion that
accompanied the original patches on the list, if you are looking for
rationale or alternatives that did not make it into the commit message.
You can simply search on gmane, but Thomas Rast also maintains a mapping
of commits back to their original discussions. You can fetch his notes
by doing:

  git config remote.mailnotes.url git://github.com/trast/git.git
  git config remote.mailnotes.fetch refs/heads/notes/*:refs/notes/*
  git fetch mailnotes

You can then use "git notes --ref=gmane show" to show notes for specific
commits, or just "git log --notes=gmane" to view them along with the
regular logs.

-Peff

Re: [PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'll venture my opinion. We should do this:
>
> -- >8 --
> Subject: pager: turn on "cat" optimization for DEFAULT_PAGER
>
> If the user specifies a pager of "cat" (or the empty
> string), whether it is in the environment or from config, we
> automagically optimize it out to mean "no pager" and avoid
> forking at all. We treat an empty pager variable similary.
>
> However, we did not apply this optimization when
> DEFAULT_PAGER was set to "cat" (or the empty string). There
> is no reason to treat DEFAULT_PAGER any differently. The
> optimization should not be user-visible (unless the user has
> a bizarre "cat" in their PATH). And even if it is, we are
> better off behaving consistently between the compile-time
> default and the environment and config settings.
>
> The stray "else" we are removing from this code was
> introduced by 402461a (pager: do not fork a pager if PAGER
> is set to empty., 2006-04-16). At that time, the line
> directly above used:
>
>    if (!pager)
> 	   pager = "less";
>
> as a fallback, meaning that it could not possibly trigger
> the optimization. Later, a3d023d (Provide a build time
> default-pager setting, 2009-10-30) turned that constant into
> a build-time setting which could be anything, but didn't
> loosen the "else" to let DEFAULT_PAGER use the optimization.
>
> Noticed-by: Dale R. Worley <worley@alum.mit.edu>
> Suggested-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

Makes sense.  Thanks.

>  pager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pager.c b/pager.c
> index c1ecf65..fa19765 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
>  		pager = getenv("PAGER");
>  	if (!pager)
>  		pager = DEFAULT_PAGER;
> -	else if (!*pager || !strcmp(pager, "cat"))
> +	if (!*pager || !strcmp(pager, "cat"))
>  		pager = NULL;
>  
>  	return pager;

Re: [PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Erik Faye-Lund]

On Tue, Sep 3, 2013 at 9:41 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Sep 02, 2013 at 10:27:48PM -0400, Dale R. Worley wrote:
>
>> > I guess the "else" could and should be dropped. If you do so (and
>> > possibly insert a blank line between the DEFAULT_PAGER case and the
>> > "pager = NULL" case), you get a nice pattern
>> >
>> > if (!pager)
>> >     try_something();
>> > if (!pager)
>> >     try_next_option();
>>
>> That's true, but it would change the effect of using "cat" as a value:
>> "cat" as a value of DEFAULT_PAGER would cause git_pager() to return
>> NULL, whereas now it causes git_pager() to return "cat".  (All other
>> places where "cat" can be a value are translated to NULL already.)
>>
>> This is why I want to know what the *intended* behavior is, because we
>> might be changing Git's behavior, and I want to know that if we do
>> that, we're changing it to what it should be.  And I haven't seen
>> anyone venture an opinion on what the intended behavior is.
>
> I'll venture my opinion. We should do this:
>
> -- >8 --
> Subject: pager: turn on "cat" optimization for DEFAULT_PAGER
>
> If the user specifies a pager of "cat" (or the empty
> string), whether it is in the environment or from config, we
> automagically optimize it out to mean "no pager" and avoid
> forking at all. We treat an empty pager variable similary.
>
> However, we did not apply this optimization when
> DEFAULT_PAGER was set to "cat" (or the empty string). There
> is no reason to treat DEFAULT_PAGER any differently. The
> optimization should not be user-visible (unless the user has
> a bizarre "cat" in their PATH). And even if it is, we are
> better off behaving consistently between the compile-time
> default and the environment and config settings.
>
> The stray "else" we are removing from this code was
> introduced by 402461a (pager: do not fork a pager if PAGER
> is set to empty., 2006-04-16). At that time, the line
> directly above used:
>
>    if (!pager)
>            pager = "less";
>
> as a fallback, meaning that it could not possibly trigger
> the optimization. Later, a3d023d (Provide a build time
> default-pager setting, 2009-10-30) turned that constant into
> a build-time setting which could be anything, but didn't
> loosen the "else" to let DEFAULT_PAGER use the optimization.
>
> Noticed-by: Dale R. Worley <worley@alum.mit.edu>
> Suggested-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  pager.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/pager.c b/pager.c
> index c1ecf65..fa19765 100644
> --- a/pager.c
> +++ b/pager.c
> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
>                 pager = getenv("PAGER");
>         if (!pager)
>                 pager = DEFAULT_PAGER;
> -       else if (!*pager || !strcmp(pager, "cat"))
> +       if (!*pager || !strcmp(pager, "cat"))

Hmmpf. It's sometimes useful to actually pipe through cat rather than
disabling the pager, as this changes the return-code from isatty. I
sometimes use this for debugging-purposes. Does this patch break that?

Re: [PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Jeff King]

On Wed, Nov 20, 2013 at 06:24:45PM +0100, Erik Faye-Lund wrote:

> > diff --git a/pager.c b/pager.c
> > index c1ecf65..fa19765 100644
> > --- a/pager.c
> > +++ b/pager.c
> > @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
> >                 pager = getenv("PAGER");
> >         if (!pager)
> >                 pager = DEFAULT_PAGER;
> > -       else if (!*pager || !strcmp(pager, "cat"))
> > +       if (!*pager || !strcmp(pager, "cat"))
> 
> Hmmpf. It's sometimes useful to actually pipe through cat rather than
> disabling the pager, as this changes the return-code from isatty. I
> sometimes use this for debugging-purposes. Does this patch break that?

My patch should not change the behavior of PAGER=cat, GIT_PAGER=cat,
core.pager, etc. It should _only_ impact the case where DEFAULT_PAGER is
set to "cat" (or NULL), and bring it in line with the other cases.

I am not clear on how you are using "cat", so I can't say whether it is
broken. But if you are doing:

  PAGER=cat git log

that already is a no-op, and that is not changed by my patch. If you
want to make stdout not a tty, I'd think:

  git log | cat

is the right way to do it (and anyway, when the pager is in effect git
will pretend that stdout is a tty, since you would still want things
like auto-color to go to the pager).

-Peff

Re: [PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Erik Faye-Lund]

On Wed, Nov 20, 2013 at 6:30 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Nov 20, 2013 at 06:24:45PM +0100, Erik Faye-Lund wrote:
>
>> > diff --git a/pager.c b/pager.c
>> > index c1ecf65..fa19765 100644
>> > --- a/pager.c
>> > +++ b/pager.c
>> > @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
>> >                 pager = getenv("PAGER");
>> >         if (!pager)
>> >                 pager = DEFAULT_PAGER;
>> > -       else if (!*pager || !strcmp(pager, "cat"))
>> > +       if (!*pager || !strcmp(pager, "cat"))
>>
>> Hmmpf. It's sometimes useful to actually pipe through cat rather than
>> disabling the pager, as this changes the return-code from isatty. I
>> sometimes use this for debugging-purposes. Does this patch break that?
>
> My patch should not change the behavior of PAGER=cat, GIT_PAGER=cat,
> core.pager, etc. It should _only_ impact the case where DEFAULT_PAGER is
> set to "cat" (or NULL), and bring it in line with the other cases.
>
> I am not clear on how you are using "cat", so I can't say whether it is
> broken. But if you are doing:
>
>   PAGER=cat git log
>
> that already is a no-op, and that is not changed by my patch. If you
> want to make stdout not a tty, I'd think:
>
>   git log | cat
>
> is the right way to do it (and anyway, when the pager is in effect git
> will pretend that stdout is a tty, since you would still want things
> like auto-color to go to the pager).

You are of course right. Explicitly piping through cat is plenty fine
for my purposes, sorry for disturbing you.

Re: [PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

>> ...
>> is set to empty., 2006-04-16). At that time, the line
>> directly above used:
>>
>>    if (!pager)
>>            pager = "less";
>>
>> as a fallback, meaning that it could not possibly trigger
>> the optimization. Later, a3d023d (Provide a build time
>> default-pager setting, 2009-10-30) turned that constant into
>> a build-time setting which could be anything, but didn't
>> loosen the "else" to let DEFAULT_PAGER use the optimization.
>>
>> Noticed-by: Dale R. Worley <worley@alum.mit.edu>
>> Suggested-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>> Signed-off-by: Jeff King <peff@peff.net>
>> ---
>>  pager.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/pager.c b/pager.c
>> index c1ecf65..fa19765 100644
>> --- a/pager.c
>> +++ b/pager.c
>> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
>>                 pager = getenv("PAGER");
>>         if (!pager)
>>                 pager = DEFAULT_PAGER;
>> -       else if (!*pager || !strcmp(pager, "cat"))
>> +       if (!*pager || !strcmp(pager, "cat"))
>
> Hmmpf. It's sometimes useful to actually pipe through cat rather than
> disabling the pager, as this changes the return-code from isatty. I
> sometimes use this for debugging-purposes. Does this patch break that?

If you have been running "GIT_PAGER=cat git whatever" and the like,
we did not pipe the output through "cat" and this has been the case
for a long time.  The only thing the patch in question changed is
for those who build with

	make DEFAULT_PAGER=cat

and I doubt that you have been debugging git by rebuilding it with
such a setting, so....

Re: [PATCH] pager: turn on "cat" optimization for DEFAULT_PAGER

[Erik Faye-Lund]

On Wed, Nov 20, 2013 at 6:33 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> ...
>>> is set to empty., 2006-04-16). At that time, the line
>>> directly above used:
>>>
>>>    if (!pager)
>>>            pager = "less";
>>>
>>> as a fallback, meaning that it could not possibly trigger
>>> the optimization. Later, a3d023d (Provide a build time
>>> default-pager setting, 2009-10-30) turned that constant into
>>> a build-time setting which could be anything, but didn't
>>> loosen the "else" to let DEFAULT_PAGER use the optimization.
>>>
>>> Noticed-by: Dale R. Worley <worley@alum.mit.edu>
>>> Suggested-by: Matthieu Moy <Matthieu.Moy@grenoble-inp.fr>
>>> Signed-off-by: Jeff King <peff@peff.net>
>>> ---
>>>  pager.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/pager.c b/pager.c
>>> index c1ecf65..fa19765 100644
>>> --- a/pager.c
>>> +++ b/pager.c
>>> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)
>>>                 pager = getenv("PAGER");
>>>         if (!pager)
>>>                 pager = DEFAULT_PAGER;
>>> -       else if (!*pager || !strcmp(pager, "cat"))
>>> +       if (!*pager || !strcmp(pager, "cat"))
>>
>> Hmmpf. It's sometimes useful to actually pipe through cat rather than
>> disabling the pager, as this changes the return-code from isatty. I
>> sometimes use this for debugging-purposes. Does this patch break that?
>
> If you have been running "GIT_PAGER=cat git whatever" and the like,
> we did not pipe the output through "cat" and this has been the case
> for a long time.  The only thing the patch in question changed is
> for those who build with
>
>         make DEFAULT_PAGER=cat
>
> and I doubt that you have been debugging git by rebuilding it with
> such a setting, so....
>

Yep. This was me simply not thinking things through :)