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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.,
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: the pager

[Dale R. Worley]

> From: Matthieu Moy 

> > 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. 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: the pager

[Matthieu Moy]

wor...@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/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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. 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: the pager

[Junio C Hamano]

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

>> From: Junio C Hamano 
>> 
>> > 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 
---
 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
+c

Re: the pager

[Dale R. Worley]

> From: Junio C Hamano 
> 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: the pager

[Junio C Hamano]

wor...@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).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html