{"thread":{"id":"34771","subject":"the pager","startedAt":"2013-08-26T19:57:41Z","lastAt":"2013-11-20T17:34:45Z","messageCount":18,"participants":["Dale R. Worley","Junio C Hamano","Matthieu Moy","Jonathan Nieder","Jeff King","Erik Faye-Lund"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"225925","messageId":"201308261957.r7QJvfjF028935@freeze.ariadne.com","threadId":"34771","inReplyTo":null,"subject":"the pager","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-26T19:57:41Z","receivedAt":"2013-08-26T19:57:41Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"I've noticed that Git by default puts long output through \"less\" as a\npager. I don't like that, but this is not the time to change\nestablished behavior. But while tracking that down, I noticed that\nthe paging behavior is controlled by at least 5 things:\n\nthe -p/--paginate/--no-pager options\nthe GIT_PAGER environment variable\nthe PAGER environment variable\nthe core.pager Git configuration variable\nthe build-in default (which seems to usually be \"less\")\n\nThere is documentation in git.1 and git-config.1, and the two are not\ncoordinated to make it clear what happens in all cases. And the\nbuilt-in default is not mentioned at all.\n\nWhat is the (intended) order of precedence of specifiers of paging\nbehavior? My guess is that it should be the order I've given above.\n\nDale\n"},{"id":"225978","messageId":"xmqqd2ozhhob.fsf@gitster.dls.corp.google.com","threadId":"34771","inReplyTo":"201308261957.r7QJvfjF028935@freeze.ariadne.com","subject":"Re: the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-27T04:38:28Z","receivedAt":"2013-08-27T04:38:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n> I've noticed that Git by default puts long output through \"less\" as a\n> pager. I don't like that, but this is not the time to change\n> established behavior. But while tracking that down, I noticed that\n> the paging behavior is controlled by at least 5 things:\n>\n> the -p/--paginate/--no-pager options\n> the GIT_PAGER environment variable\n> the PAGER environment variable\n> the core.pager Git configuration variable\n> the build-in default (which seems to usually be \"less\")\n> ...\n> What is the (intended) order of precedence of specifiers of paging\n> behavior? My guess is that it should be the order I've given above.\n\nI think that sounds about right (I didn't check the code, though).\nThe most specific to the command line invocation (i.e. option)\ntrumps the environment, which trumps the configured default, and the\nhard wired stuff is used as the fallback default.\n\nI am not sure about PAGER environment and core.pager, though.\nPeople want Git specific pager that applies only to Git process\nspecified to core.pager, and still want to use their own generic\nPAGER to other programs, so my gut feeling is that it would make\nsense to consider core.pager a way to specify GIT_PAGER without\ncontaminating the environment, and use both to override the generic\nPAGER (in other words, core.pager should take precedence over PAGER\nas far as Git is concerned).\n"},{"id":"226093","messageId":"201308281819.r7SIJmnh025977@freeze.ariadne.com","threadId":"34771","inReplyTo":"xmqqd2ozhhob.fsf@gitster.dls.corp.google.com","subject":"Re: the pager","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-28T18:19:48Z","receivedAt":"2013-08-28T18:19:48Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Junio C Hamano \n> \n> > I've noticed that Git by default puts long output through \"less\" as a\n> > pager. I don't like that, but this is not the time to change\n> > established behavior. But while tracking that down, I noticed that\n> > the paging behavior is controlled by at least 5 things:\n> >\n> > the -p/--paginate/--no-pager options\n> > the GIT_PAGER environment variable\n> > the PAGER environment variable\n> > the core.pager Git configuration variable\n> > the build-in default (which seems to usually be \"less\")\n> > ...\n> > What is the (intended) order of precedence of specifiers of paging\n> > behavior? My guess is that it should be the order I've given above.\n> \n> I think that sounds about right (I didn't check the code, though).\n> The most specific to the command line invocation (i.e. option)\n> trumps the environment, which trumps the configured default, and the\n> hard wired stuff is used as the fallback default.\n> \n> I am not sure about PAGER environment and core.pager, though.\n> People want Git specific pager that applies only to Git process\n> specified to core.pager, and still want to use their own generic\n> PAGER to other programs, so my gut feeling is that it would make\n> sense to consider core.pager a way to specify GIT_PAGER without\n> contaminating the environment, and use both to override the generic\n> PAGER (in other words, core.pager should take precedence over PAGER\n> as far as Git is concerned).\n\nI've just discovered this bit of documentation. Within the git-var\nmanual page is this entry:\n\n GIT_PAGER\n Text viewer for use by git commands (e.g., less). The value is\n meant to be interpreted by the shell. The order of preference is\n the $GIT_PAGER environment variable, then core.pager configuration,\n then $PAGER, and then finally less.\n\nThis suggests that the ordering is GIT_PAGER > core.pager > PAGER >\ndefault.\n\nDale\n"},{"id":"226110","messageId":"xmqqr4dd8suz.fsf@gitster.dls.corp.google.com","threadId":"34771","inReplyTo":"201308281819.r7SIJmnh025977@freeze.ariadne.com","subject":"Re: the pager","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-28T20:26:12Z","receivedAt":"2013-08-28T20:26:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n>> From: Junio C Hamano \n>> \n>> > I've noticed that Git by default puts long output through \"less\" as a\n>> > pager. I don't like that, but this is not the time to change\n>> > established behavior. But while tracking that down, I noticed that\n>> > the paging behavior is controlled by at least 5 things:\n>> >\n>> > the -p/--paginate/--no-pager options\n>> > the GIT_PAGER environment variable\n>> > the PAGER environment variable\n>> > the core.pager Git configuration variable\n>> > the build-in default (which seems to usually be \"less\")\n>> > ...\n>> > What is the (intended) order of precedence of specifiers of paging\n>> > behavior? My guess is that it should be the order I've given above.\n>> \n>> I think that sounds about right (I didn't check the code, though).\n>> The most specific to the command line invocation (i.e. option)\n>> trumps the environment, which trumps the configured default, and the\n>> hard wired stuff is used as the fallback default.\n>> \n>> I am not sure about PAGER environment and core.pager, though.\n>> People want Git specific pager that applies only to Git process\n>> specified to core.pager, and still want to use their own generic\n>> PAGER to other programs, so my gut feeling is that it would make\n>> sense to consider core.pager a way to specify GIT_PAGER without\n>> contaminating the environment, and use both to override the generic\n>> PAGER (in other words, core.pager should take precedence over PAGER\n>> as far as Git is concerned).\n>\n> I've just discovered this bit of documentation. Within the git-var\n> manual page is this entry:\n>\n> GIT_PAGER\n> Text viewer for use by git commands (e.g., less). The value is\n> meant to be interpreted by the shell. The order of preference is\n> the $GIT_PAGER environment variable, then core.pager configuration,\n> then $PAGER, and then finally less.\n>\n> This suggests that the ordering is GIT_PAGER > core.pager > PAGER >\n> default.\n\nOK, that means that my gut feeling was right, we do the right thing,\nand we do document it.\n\nBut your original \"documentation in git.1 and git-config.1, and the\ntwo are not coordinated to make it clear what happens in all cases.\"\nstill stands. How can we improve the documentation to make the above\nparagraph easier to discover? Perhaps use the above wording to\nupdate git-config.1 that already mentions GIT_PAGER in the section\nfor core.pager?\n\nThe description over there is so incoherent that I needed to read it\nthree times to see what points are mentioned.\n\nHow about doing this?\n\n-- >8 --\nconfig: rewrite core.pager documentation\n\nThe text mentions core.pager and GIT_PAGER without giving the\noverall picture of precedences. Borrow a better description from\nthe git-var(1) documentation.\n\nThe use of the mechanism to allow system-wide, global and\nper-repository configuration files is not limited to this particular\nvariable. Remove it to clarify the paragraph.\n\nRewrite the part that explains how the environment variable LESS is\nset to Git's default value, and how to selectively customize it.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/config.txt | 28 ++++++++++++----------------\n 1 file changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex ec57a15..7f9bc38 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -553,22 +553,18 @@ sequence.editor::\n \tWhen not configured the default commit message editor is used instead.\n \n core.pager::\n-\tThe command that Git will use to paginate output. Can\n-\tbe overridden with the `GIT_PAGER` environment\n-\tvariable. Note that Git sets the `LESS` environment\n-\tvariable to `FRSX` if it is unset when it runs the\n-\tpager. One can change these settings by setting the\n-\t`LESS` variable to some other value. Alternately,\n-\tthese settings can be overridden on a project or\n-\tglobal basis by setting the `core.pager` option.\n-\tSetting `core.pager` has no effect on the `LESS`\n-\tenvironment variable behaviour above, so if you want\n-\tto override Git's default settings this way, you need\n-\tto be explicit. For example, to disable the S option\n-\tin a backward compatible manner, set `core.pager`\n-\tto `less -+S`. This will be passed to the shell by\n-\tGit, which will translate the final command to\n-\t`LESS=FRSX less -+S`.\n+\tText viewer for use by Git commands (e.g., 'less'). The value\n+\tis meant to be interpreted by the shell. The order of preference\n+\tis the `$GIT_PAGER` environment variable, then `core.pager`\n+\tconfiguration, then `$PAGER`, and then the default chosen at\n+\tcompile time (usually 'less').\n++\n+When the `LESS` environment variable is unset, Git sets it to `FRSX`\n+(if `LESS` environment variable is set, Git does not change it at\n+all). If you want to override Git's default setting for `LESS`, you\n+can set `core.pager` to `less -+S`. This will be passed to the\n+shell by Git, which will translate the final command to `LESS=FRSX\n+less -+S`.\n \n core.whitespace::\n \tA comma separated list of common whitespace problems to\n"},{"id":"226179","messageId":"201308291541.r7TFfuJr023110@freeze.ariadne.com","threadId":"34771","inReplyTo":"xmqqr4dd8suz.fsf@gitster.dls.corp.google.com","subject":"Re: the pager","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-08-29T15:41:56Z","receivedAt":"2013-08-29T15:41:56Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"So I set out to verify in the code that the order of priority of pager\nspecification is\n\n GIT_PAGER > core.pager > PAGER > default\n\nI discovered that there is also a pager. configuration\nvariable.\n\nI was expecting the code to be simple, uniform (with regard to the 5\nsources), and reasonably well documented. The relevant parts of the\ncode that I have located so far include:\n\nin environment.c:\n\n const char *pager_program;\n\nin config.c:\n\n int git_config_with_options(config_fn_t fn, void *data,\n const char *filename,\n const char *blob_ref,\n int respect_includes)\n {\n char *repo_config = NULL;\n int ret;\n struct config_include_data inc = CONFIG_INCLUDE_INIT;\n\n if (respect_includes) {\n inc.fn = fn;\n inc.data = data;\n fn = git_config_include;\n data = &inc;\n }\n\n /*\n * If we have a specific filename, use it. Otherwise, follow the\n * regular lookup sequence.\n */\n if (filename)\n return git_config_from_file(fn, filename, data);\n else if (blob_ref)\n return git_config_from_blob_ref(fn, blob_ref, data);\n\n repo_config = git_pathdup(\"config\");\n ret = git_config_early(fn, data, repo_config);\n if (repo_config)\n free(repo_config);\n return ret;\n }\n\nin pager.c:\n\n /* returns 0 for \"no pager\", 1 for \"use pager\", and -1 for \"not specified\" */\n int check_pager_config(const char *cmd)\n {\n struct pager_config c;\n c.cmd = cmd;\n c.want = -1;\n c.value = NULL;\n git_config(pager_command_config, &c);\n if (c.value)\n pager_program = c.value;\n return c.want;\n }\n\n const char *git_pager(int stdout_is_tty)\n {\n const char *pager;\n\n if (!stdout_is_tty)\n return NULL;\n\n pager = getenv(\"GIT_PAGER\");\n if (!pager) {\n if (!pager_program)\n git_config(git_default_config, NULL);\n pager = pager_program;\n }\n if (!pager)\n pager = getenv(\"PAGER\");\n if (!pager)\n pager = DEFAULT_PAGER;\n else if (!*pager || !strcmp(pager, \"cat\"))\n pager = NULL;\n\n return pager;\n }\n\nWhat's with the code? It's not simple, it's not uniform (e.g.,\nsetting env. var. PAGER to \"cat\" will cause git_pager() to return\nNULL, but setting preprocessor var. DEFAULT_PAGER to \"cat\" will cause\nit to return \"cat\"), and it's barely got any comments at all (a global\nvariable has *no description whatsoever*).\n\nI'd like to clean up the manual pages at least, but it would take me\nhours to figure out what the code *does*.\n\nI know I'm griping here, but I thought that part of the reward for\ncontributing to an open-source project was as a showcase of one's\nwork. Commenting your code is what you learn first in programming.\n\nDale\n"},{"id":"226182","messageId":"vpqsixsv6dq.fsf@anie.imag.fr","threadId":"34771","inReplyTo":"201308291541.r7TFfuJr023110@freeze.ariadne.com","subject":"Re: the pager","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2013-08-29T15:55:29Z","receivedAt":"2013-08-29T15:55:29Z","isPatch":false,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"worley@alum.mit.edu (Dale R. Worley) writes:\n\n> const char *git_pager(int stdout_is_tty)\n> {\n> const char *pager;\n>\n> if (!stdout_is_tty)\n> return NULL;\n>\n> pager = getenv(\"GIT_PAGER\");\n> if (!pager) {\n> if (!pager_program)\n> git_config(git_default_config, NULL);\n> pager = pager_program;\n> }\n> if (!pager)\n> pager = getenv(\"PAGER\");\n> if (!pager)\n> pager = DEFAULT_PAGER;\n> else if (!*pager || !strcmp(pager, \"cat\"))\n> pager = NULL;\n\nI guess the \"else\" could and should be dropped. If you do so (and\npossibly insert a blank line between the DEFAULT_PAGER case and the\n\"pager = NULL\" case), you get a nice pattern\n\nif (!pager)\n\ttry_something();\nif (!pager)\n\ttry_next_option();\n...\n\n> Commenting your code is what you learn first in programming.\n\nNot commenting too much is the second thing you learn ;-).\n\nI agree that a comment like this would help, though:\n\n--- a/cache.h\n+++ b/cache.h\n@@ -1266,7 +1266,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n \n /* pager.c */\n extern void setup_pager(void);\n-extern const char *pager_program;\n+extern const char *pager_program; /* value read from git_config() */\n extern int pager_in_use(void);\n extern int pager_use_color;\n extern int term_columns(void);\n\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"226571","messageId":"201309030227.r832RmBd013888@freeze.ariadne.com","threadId":"34771","inReplyTo":"vpqsixsv6dq.fsf@anie.imag.fr","subject":"Re: the pager","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-09-03T02:27:48Z","receivedAt":"2013-09-03T02:27:48Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> From: Matthieu Moy \n\n> > const char *git_pager(int stdout_is_tty)\n> > {\n> > const char *pager;\n> >\n> > if (!stdout_is_tty)\n> > return NULL;\n> >\n> > pager = getenv(\"GIT_PAGER\");\n> > if (!pager) {\n> > if (!pager_program)\n> > git_config(git_default_config, NULL);\n> > pager = pager_program;\n> > }\n> > if (!pager)\n> > pager = getenv(\"PAGER\");\n> > if (!pager)\n> > pager = DEFAULT_PAGER;\n> > else if (!*pager || !strcmp(pager, \"cat\"))\n> > pager = NULL;\n> \n> I guess the \"else\" could and should be dropped. If you do so (and\n> possibly insert a blank line between the DEFAULT_PAGER case and the\n> \"pager = NULL\" case), you get a nice pattern\n> \n> if (!pager)\n> \ttry_something();\n> if (!pager)\n> \ttry_next_option();\n\nThat's true, but it would change the effect of using \"cat\" as a value:\n\"cat\" as a value of DEFAULT_PAGER would cause git_pager() to return\nNULL, whereas now it causes git_pager() to return \"cat\". (All other\nplaces where \"cat\" can be a value are translated to NULL already.)\n\nThis is why I want to know what the *intended* behavior is, because we\nmight be changing Git's behavior, and I want to know that if we do\nthat, we're changing it to what it should be. And I haven't seen\nanyone venture an opinion on what the intended behavior is.\n\n> I agree that a comment like this would help, though:\n> \n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1266,7 +1266,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n> \n> /* pager.c */\n> extern void setup_pager(void);\n> -extern const char *pager_program;\n> +extern const char *pager_program; /* value read from git_config() */\n> extern int pager_in_use(void);\n> extern int pager_use_color;\n> extern int term_columns(void);\n\nFirst off, the wording is wrong, it should be \"value set by\ngit_config()\".\n\nBut that doesn't tell the reader what the significance of the value\nis. I suspect that a number of global variables need to be marked:\n\n> /* The pager program name, or \"cat\" if there is no pager.\n> * Can be overridden by the pager. configuration value for a\n> * single command, or suppressed by the --no-pager option.\n> * Set by calling git_config().\n> * NULL if hasn't been set yet by calling git_config(). */\n> extern const char *pager_program;\n\nDale\n"},{"id":"226572","messageId":"201309030237.r832bjZp014060@freeze.ariadne.com","threadId":"34771","inReplyTo":"201308261957.r7QJvfjF028935@freeze.ariadne.com","subject":"Re: the pager","fromName":"Dale R. Worley","fromEmail":"worley@alum.mit.edu","sentAt":"2013-09-03T02:37:45Z","receivedAt":"2013-09-03T02:37:45Z","isPatch":false,"sender":{"key":"worley@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/19911107?v=4"},"body":"> I've noticed that Git by default puts long output through \"less\" as a\n> pager. I don't like that, but this is not the time to change\n> established behavior. But while tracking that down, I noticed that\n> the paging behavior is controlled by at least 5 things:\n> \n> the -p/--paginate/--no-pager options\n> the GIT_PAGER environment variable\n> the PAGER environment variable\n> the core.pager Git configuration variable\n> the build-in default (which seems to usually be \"less\")\n\nOne complication is the meaning of -p/--no-pager:\n\nWith the remaining sources, we assume that there is a priority\nsequence, and that is used to determine what the pager is.\n\nThere is a somewhat independent question of when the pager is\nactivated. What I know so far is that some commands use the pager by\ndefault and some by default do not. My expectation is that\n--no-pager can be used to suppress the pager for *any* command. Is it\nalso true that -p can force the pager for *any* command, or are there\ncommands which will not page even with -p?\n\nI assume that if -p is specified but the \"which pager\" selection is\n\"cat\" (or some other specification of no pager), then there is no\npaging operation.\n\nDale\n"},{"id":"226573","messageId":"20130903025711.GA25617@elie.Belkin","threadId":"34771","inReplyTo":"201309030227.r832RmBd013888@freeze.ariadne.com","subject":"Re: the pager","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-09-03T02:57:11Z","receivedAt":"2013-09-03T02:57:11Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDale R. Worley wrote:\n\n> That's true, but it would change the effect of using \"cat\" as a value:\n> \"cat\" as a value of DEFAULT_PAGER would cause git_pager() to return\n> NULL, whereas now it causes git_pager() to return \"cat\". (All other\n> places where \"cat\" can be a value are translated to NULL already.)\n>\n> This is why I want to know what the *intended* behavior is, because we\n> might be changing Git's behavior, and I want to know that if we do\n> that, we're changing it to what it should be. And I haven't seen\n> anyone venture an opinion on what the intended behavior is.\n\nI don't really follow.\n\nFor all practical purposes, \"cat\" is equivalent to no pager at all,\nno? And the git-var(1) manpage describes the intended order of\nprecedence, as far as I can tell, except that it was written before\nv1.7.4-rc0~76^2 (allow command-specific pagers in pager.,\n2010-11-17) which forgot to update some documentation.\n\nSuggested wording for improving the documentation or its organization\nwould of course be welcome. And I agree with Matthieu that the name\nof the pager_program global variable is needlessly confusing ---\nperhaps it should be called config_pager_program or similar.\n\nThanks,\nJonathan\n"},{"id":"226599","messageId":"20130903074150.GE3608@sigill.intra.peff.net","threadId":"34771","inReplyTo":"201309030227.r832RmBd013888@freeze.ariadne.com","subject":"[PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T07:41:50Z","receivedAt":"2013-09-03T07:41:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2013 at 10:27:48PM -0400, Dale R. Worley wrote:\n\n> > I guess the \"else\" could and should be dropped. If you do so (and\n> > possibly insert a blank line between the DEFAULT_PAGER case and the\n> > \"pager = NULL\" case), you get a nice pattern\n> > \n> > if (!pager)\n> > \ttry_something();\n> > if (!pager)\n> > \ttry_next_option();\n> \n> That's true, but it would change the effect of using \"cat\" as a value:\n> \"cat\" as a value of DEFAULT_PAGER would cause git_pager() to return\n> NULL, whereas now it causes git_pager() to return \"cat\". (All other\n> places where \"cat\" can be a value are translated to NULL already.)\n> \n> This is why I want to know what the *intended* behavior is, because we\n> might be changing Git's behavior, and I want to know that if we do\n> that, we're changing it to what it should be. And I haven't seen\n> anyone venture an opinion on what the intended behavior is.\n\nI'll venture my opinion. We should do this:\n\n-- >8 --\nSubject: pager: turn on \"cat\" optimization for DEFAULT_PAGER\n\nIf the user specifies a pager of \"cat\" (or the empty\nstring), whether it is in the environment or from config, we\nautomagically optimize it out to mean \"no pager\" and avoid\nforking at all. We treat an empty pager variable similary.\n\nHowever, we did not apply this optimization when\nDEFAULT_PAGER was set to \"cat\" (or the empty string). There\nis no reason to treat DEFAULT_PAGER any differently. The\noptimization should not be user-visible (unless the user has\na bizarre \"cat\" in their PATH). And even if it is, we are\nbetter off behaving consistently between the compile-time\ndefault and the environment and config settings.\n\nThe stray \"else\" we are removing from this code was\nintroduced by 402461a (pager: do not fork a pager if PAGER\nis set to empty., 2006-04-16). At that time, the line\ndirectly above used:\n\n if (!pager)\n\t pager = \"less\";\n\nas a fallback, meaning that it could not possibly trigger\nthe optimization. Later, a3d023d (Provide a build time\ndefault-pager setting, 2009-10-30) turned that constant into\na build-time setting which could be anything, but didn't\nloosen the \"else\" to let DEFAULT_PAGER use the optimization.\n\nNoticed-by: Dale R. Worley \nSuggested-by: Matthieu Moy \nSigned-off-by: Jeff King \n---\n pager.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/pager.c b/pager.c\nindex c1ecf65..fa19765 100644\n--- a/pager.c\n+++ b/pager.c\n@@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n \t\tpager = getenv(\"PAGER\");\n \tif (!pager)\n \t\tpager = DEFAULT_PAGER;\n-\telse if (!*pager || !strcmp(pager, \"cat\"))\n+\tif (!*pager || !strcmp(pager, \"cat\"))\n \t\tpager = NULL;\n \n \treturn pager;\n-- \n1.8.4.2.g87d4a77\n"},{"id":"226602","messageId":"20130903080119.GF3608@sigill.intra.peff.net","threadId":"34771","inReplyTo":"201309030237.r832bjZp014060@freeze.ariadne.com","subject":"Re: the pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T08:01:19Z","receivedAt":"2013-09-03T08:01:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2013 at 10:37:45PM -0400, Dale R. Worley wrote:\n\n> > I've noticed that Git by default puts long output through \"less\" as a\n> > pager. I don't like that, but this is not the time to change\n> > established behavior. But while tracking that down, I noticed that\n> > the paging behavior is controlled by at least 5 things:\n> > \n> > the -p/--paginate/--no-pager options\n> > the GIT_PAGER environment variable\n> > the PAGER environment variable\n> > the core.pager Git configuration variable\n> > the build-in default (which seems to usually be \"less\")\n\nThis list has some orthogonal concepts. The \"-p\" and \"--no-pager\"\nvariables decide _whether_ to run the pager. The GIT_PAGER and PAGER\nenvironment variables, along with core.pager and the compile-time\ndefault, decide _which_ pager to run.\n\nThe fact that \"cat\" or the empty string becomes \"no pager\" is purely an\noptimization (we could fork and run \"sh -c ''\" or \"cat\", but it would be\na no-op). So even though you might have instructed git to run the pager,\nit may be a noop if your pager is \"cat\", and we optimize it out.\n\nThe confusing one (and missing from your list) is pager.$program, which\noriginally was a \"whether\", but later learned to optionally be a\n\"which\". And you also omit the built-in defaults for \"whether\" on each\ncommand (e.g., \"log\" runs a pager, \"push\" does not).\n\n> There is a somewhat independent question of when the pager is\n> activated. What I know so far is that some commands use the pager by\n> default and some by default do not. My expectation is that\n> --no-pager can be used to suppress the pager for *any* command. Is it\n> also true that -p can force the pager for *any* command, or are there\n> commands which will not page even with -p?\n\nYes, --no-pager and -p suppress or force, respectively, for any command.\nThey take precedence over config (pager.$command), which in turn takes\nprecedence over builtin defaults (per-command defaults, in this case).\n\nEnvironment variables should generally be less than command-line\noptions, but greater than config. But there is no \"definitely use a\npager\" environment variable, so it doesn't apply here.\n\nAnd I say generally because we should put git-specific environment\nvariables over git-specific config, but git-specific config over general\nenvironment variables (so similarly we should respect user.email in the\nconfig over $EMAIL in the environment, but under $GIT_COMMITTER_EMAIL).\n\n> I assume that if -p is specified but the \"which pager\" selection is\n> \"cat\" (or some other specification of no pager), then there is no\n> paging operation.\n\nThere is a pager in that case, but it doesn't do anything. And then we\noptimize it out because it doesn't do anything. :) That is somewhat\ntongue-in-cheek, but I hope it shows the mental model that goes into the\ndecision.\n\n-Peff\n"},{"id":"226605","messageId":"20130903081652.GG3608@sigill.intra.peff.net","threadId":"34771","inReplyTo":"201308291541.r7TFfuJr023110@freeze.ariadne.com","subject":"Re: the pager","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T08:16:52Z","receivedAt":"2013-09-03T08:16:52Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2013 at 11:41:56AM -0400, Dale R. Worley wrote:\n\n> I know I'm griping here, but I thought that part of the reward for\n> contributing to an open-source project was as a showcase of one's\n> work. Commenting your code is what you learn first in programming.\n\nYou will find that the best comments in the git source code are those\nwritten in the commit messages. Learn to use \"git blame\" (or I recommend\n\"tig blame\" for interactive use), \"git log -S\", and the new \"git log -L\"\nfor finding the commits that touched an area.\n\nIt is also sometimes useful to look at the review and discussion that\naccompanied the original patches on the list, if you are looking for\nrationale or alternatives that did not make it into the commit message.\nYou can simply search on gmane, but Thomas Rast also maintains a mapping\nof commits back to their original discussions. You can fetch his notes\nby doing:\n\n git config remote.mailnotes.url git://github.com/trast/git.git\n git config remote.mailnotes.fetch refs/heads/notes/*:refs/notes/*\n git fetch mailnotes\n\nYou can then use \"git notes --ref=gmane show\" to show notes for specific\ncommits, or just \"git log --notes=gmane\" to view them along with the\nregular logs.\n\n-Peff\n"},{"id":"226645","messageId":"xmqqzjrtst9h.fsf@gitster.dls.corp.google.com","threadId":"34771","inReplyTo":"20130903074150.GE3608@sigill.intra.peff.net","subject":"Re: [PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-09-03T17:35:22Z","receivedAt":"2013-09-03T17:35:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'll venture my opinion. We should do this:\n>\n> -- >8 --\n> Subject: pager: turn on \"cat\" optimization for DEFAULT_PAGER\n>\n> If the user specifies a pager of \"cat\" (or the empty\n> string), whether it is in the environment or from config, we\n> automagically optimize it out to mean \"no pager\" and avoid\n> forking at all. We treat an empty pager variable similary.\n>\n> However, we did not apply this optimization when\n> DEFAULT_PAGER was set to \"cat\" (or the empty string). There\n> is no reason to treat DEFAULT_PAGER any differently. The\n> optimization should not be user-visible (unless the user has\n> a bizarre \"cat\" in their PATH). And even if it is, we are\n> better off behaving consistently between the compile-time\n> default and the environment and config settings.\n>\n> The stray \"else\" we are removing from this code was\n> introduced by 402461a (pager: do not fork a pager if PAGER\n> is set to empty., 2006-04-16). At that time, the line\n> directly above used:\n>\n> if (!pager)\n> \t pager = \"less\";\n>\n> as a fallback, meaning that it could not possibly trigger\n> the optimization. Later, a3d023d (Provide a build time\n> default-pager setting, 2009-10-30) turned that constant into\n> a build-time setting which could be anything, but didn't\n> loosen the \"else\" to let DEFAULT_PAGER use the optimization.\n>\n> Noticed-by: Dale R. Worley \n> Suggested-by: Matthieu Moy \n> Signed-off-by: Jeff King \n> ---\n\nMakes sense. Thanks.\n\n> pager.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pager.c b/pager.c\n> index c1ecf65..fa19765 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n> \t\tpager = getenv(\"PAGER\");\n> \tif (!pager)\n> \t\tpager = DEFAULT_PAGER;\n> -\telse if (!*pager || !strcmp(pager, \"cat\"))\n> +\tif (!*pager || !strcmp(pager, \"cat\"))\n> \t\tpager = NULL;\n> \n> \treturn pager;\n"},{"id":"230848","messageId":"CABPQNSb6PD+oSw_LT6KaUYd8BTeN-WHJFodcuiLe=u76rFYFJw@mail.gmail.com","threadId":"34771","inReplyTo":"20130903074150.GE3608@sigill.intra.peff.net","subject":"Re: [PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2013-11-20T17:24:45Z","receivedAt":"2013-11-20T17:24:45Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, Sep 3, 2013 at 9:41 AM, Jeff King wrote:\n> On Mon, Sep 02, 2013 at 10:27:48PM -0400, Dale R. Worley wrote:\n>\n>> > I guess the \"else\" could and should be dropped. If you do so (and\n>> > possibly insert a blank line between the DEFAULT_PAGER case and the\n>> > \"pager = NULL\" case), you get a nice pattern\n>> >\n>> > if (!pager)\n>> > try_something();\n>> > if (!pager)\n>> > try_next_option();\n>>\n>> That's true, but it would change the effect of using \"cat\" as a value:\n>> \"cat\" as a value of DEFAULT_PAGER would cause git_pager() to return\n>> NULL, whereas now it causes git_pager() to return \"cat\". (All other\n>> places where \"cat\" can be a value are translated to NULL already.)\n>>\n>> This is why I want to know what the *intended* behavior is, because we\n>> might be changing Git's behavior, and I want to know that if we do\n>> that, we're changing it to what it should be. And I haven't seen\n>> anyone venture an opinion on what the intended behavior is.\n>\n> I'll venture my opinion. We should do this:\n>\n> -- >8 --\n> Subject: pager: turn on \"cat\" optimization for DEFAULT_PAGER\n>\n> If the user specifies a pager of \"cat\" (or the empty\n> string), whether it is in the environment or from config, we\n> automagically optimize it out to mean \"no pager\" and avoid\n> forking at all. We treat an empty pager variable similary.\n>\n> However, we did not apply this optimization when\n> DEFAULT_PAGER was set to \"cat\" (or the empty string). There\n> is no reason to treat DEFAULT_PAGER any differently. The\n> optimization should not be user-visible (unless the user has\n> a bizarre \"cat\" in their PATH). And even if it is, we are\n> better off behaving consistently between the compile-time\n> default and the environment and config settings.\n>\n> The stray \"else\" we are removing from this code was\n> introduced by 402461a (pager: do not fork a pager if PAGER\n> is set to empty., 2006-04-16). At that time, the line\n> directly above used:\n>\n> if (!pager)\n> pager = \"less\";\n>\n> as a fallback, meaning that it could not possibly trigger\n> the optimization. Later, a3d023d (Provide a build time\n> default-pager setting, 2009-10-30) turned that constant into\n> a build-time setting which could be anything, but didn't\n> loosen the \"else\" to let DEFAULT_PAGER use the optimization.\n>\n> Noticed-by: Dale R. Worley \n> Suggested-by: Matthieu Moy \n> Signed-off-by: Jeff King \n> ---\n> pager.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/pager.c b/pager.c\n> index c1ecf65..fa19765 100644\n> --- a/pager.c\n> +++ b/pager.c\n> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n> pager = getenv(\"PAGER\");\n> if (!pager)\n> pager = DEFAULT_PAGER;\n> - else if (!*pager || !strcmp(pager, \"cat\"))\n> + if (!*pager || !strcmp(pager, \"cat\"))\n\nHmmpf. It's sometimes useful to actually pipe through cat rather than\ndisabling the pager, as this changes the return-code from isatty. I\nsometimes use this for debugging-purposes. Does this patch break that?\n"},{"id":"230849","messageId":"20131120173054.GA15339@sigill.intra.peff.net","threadId":"34771","inReplyTo":"CABPQNSb6PD+oSw_LT6KaUYd8BTeN-WHJFodcuiLe=u76rFYFJw@mail.gmail.com","subject":"Re: [PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-20T17:30:54Z","receivedAt":"2013-11-20T17:30:54Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 20, 2013 at 06:24:45PM +0100, Erik Faye-Lund wrote:\n\n> > diff --git a/pager.c b/pager.c\n> > index c1ecf65..fa19765 100644\n> > --- a/pager.c\n> > +++ b/pager.c\n> > @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n> > pager = getenv(\"PAGER\");\n> > if (!pager)\n> > pager = DEFAULT_PAGER;\n> > - else if (!*pager || !strcmp(pager, \"cat\"))\n> > + if (!*pager || !strcmp(pager, \"cat\"))\n> \n> Hmmpf. It's sometimes useful to actually pipe through cat rather than\n> disabling the pager, as this changes the return-code from isatty. I\n> sometimes use this for debugging-purposes. Does this patch break that?\n\nMy patch should not change the behavior of PAGER=cat, GIT_PAGER=cat,\ncore.pager, etc. It should _only_ impact the case where DEFAULT_PAGER is\nset to \"cat\" (or NULL), and bring it in line with the other cases.\n\nI am not clear on how you are using \"cat\", so I can't say whether it is\nbroken. But if you are doing:\n\n PAGER=cat git log\n\nthat already is a no-op, and that is not changed by my patch. If you\nwant to make stdout not a tty, I'd think:\n\n git log | cat\n\nis the right way to do it (and anyway, when the pager is in effect git\nwill pretend that stdout is a tty, since you would still want things\nlike auto-color to go to the pager).\n\n-Peff\n"},{"id":"230851","messageId":"CABPQNSYWntx_36kTB4GG4i+m8tXn4k+YSDEWn9KZU-9t6xCAXQ@mail.gmail.com","threadId":"34771","inReplyTo":"20131120173054.GA15339@sigill.intra.peff.net","subject":"Re: [PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2013-11-20T17:33:05Z","receivedAt":"2013-11-20T17:33:05Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Nov 20, 2013 at 6:30 PM, Jeff King wrote:\n> On Wed, Nov 20, 2013 at 06:24:45PM +0100, Erik Faye-Lund wrote:\n>\n>> > diff --git a/pager.c b/pager.c\n>> > index c1ecf65..fa19765 100644\n>> > --- a/pager.c\n>> > +++ b/pager.c\n>> > @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n>> > pager = getenv(\"PAGER\");\n>> > if (!pager)\n>> > pager = DEFAULT_PAGER;\n>> > - else if (!*pager || !strcmp(pager, \"cat\"))\n>> > + if (!*pager || !strcmp(pager, \"cat\"))\n>>\n>> Hmmpf. It's sometimes useful to actually pipe through cat rather than\n>> disabling the pager, as this changes the return-code from isatty. I\n>> sometimes use this for debugging-purposes. Does this patch break that?\n>\n> My patch should not change the behavior of PAGER=cat, GIT_PAGER=cat,\n> core.pager, etc. It should _only_ impact the case where DEFAULT_PAGER is\n> set to \"cat\" (or NULL), and bring it in line with the other cases.\n>\n> I am not clear on how you are using \"cat\", so I can't say whether it is\n> broken. But if you are doing:\n>\n> PAGER=cat git log\n>\n> that already is a no-op, and that is not changed by my patch. If you\n> want to make stdout not a tty, I'd think:\n>\n> git log | cat\n>\n> is the right way to do it (and anyway, when the pager is in effect git\n> will pretend that stdout is a tty, since you would still want things\n> like auto-color to go to the pager).\n\nYou are of course right. Explicitly piping through cat is plenty fine\nfor my purposes, sorry for disturbing you.\n"},{"id":"230850","messageId":"xmqqob5f6krn.fsf@gitster.dls.corp.google.com","threadId":"34771","inReplyTo":"CABPQNSb6PD+oSw_LT6KaUYd8BTeN-WHJFodcuiLe=u76rFYFJw@mail.gmail.com","subject":"Re: [PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-11-20T17:33:16Z","receivedAt":"2013-11-20T17:33:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n>> ...\n>> is set to empty., 2006-04-16). At that time, the line\n>> directly above used:\n>>\n>> if (!pager)\n>> pager = \"less\";\n>>\n>> as a fallback, meaning that it could not possibly trigger\n>> the optimization. Later, a3d023d (Provide a build time\n>> default-pager setting, 2009-10-30) turned that constant into\n>> a build-time setting which could be anything, but didn't\n>> loosen the \"else\" to let DEFAULT_PAGER use the optimization.\n>>\n>> Noticed-by: Dale R. Worley \n>> Suggested-by: Matthieu Moy \n>> Signed-off-by: Jeff King \n>> ---\n>> pager.c | 2 +-\n>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>\n>> diff --git a/pager.c b/pager.c\n>> index c1ecf65..fa19765 100644\n>> --- a/pager.c\n>> +++ b/pager.c\n>> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n>> pager = getenv(\"PAGER\");\n>> if (!pager)\n>> pager = DEFAULT_PAGER;\n>> - else if (!*pager || !strcmp(pager, \"cat\"))\n>> + if (!*pager || !strcmp(pager, \"cat\"))\n>\n> Hmmpf. It's sometimes useful to actually pipe through cat rather than\n> disabling the pager, as this changes the return-code from isatty. I\n> sometimes use this for debugging-purposes. Does this patch break that?\n\nIf you have been running \"GIT_PAGER=cat git whatever\" and the like,\nwe did not pipe the output through \"cat\" and this has been the case\nfor a long time. The only thing the patch in question changed is\nfor those who build with\n\n\tmake DEFAULT_PAGER=cat\n\nand I doubt that you have been debugging git by rebuilding it with\nsuch a setting, so....\n"},{"id":"230852","messageId":"CABPQNSb9ndc+QxTR6mcdnvt7oGQ7DbY6KXGt0LZOXshrg5Scow@mail.gmail.com","threadId":"34771","inReplyTo":"xmqqob5f6krn.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] pager: turn on \"cat\" optimization for DEFAULT_PAGER","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2013-11-20T17:34:45Z","receivedAt":"2013-11-20T17:34:45Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Nov 20, 2013 at 6:33 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>>> ...\n>>> is set to empty., 2006-04-16). At that time, the line\n>>> directly above used:\n>>>\n>>> if (!pager)\n>>> pager = \"less\";\n>>>\n>>> as a fallback, meaning that it could not possibly trigger\n>>> the optimization. Later, a3d023d (Provide a build time\n>>> default-pager setting, 2009-10-30) turned that constant into\n>>> a build-time setting which could be anything, but didn't\n>>> loosen the \"else\" to let DEFAULT_PAGER use the optimization.\n>>>\n>>> Noticed-by: Dale R. Worley \n>>> Suggested-by: Matthieu Moy \n>>> Signed-off-by: Jeff King \n>>> ---\n>>> pager.c | 2 +-\n>>> 1 file changed, 1 insertion(+), 1 deletion(-)\n>>>\n>>> diff --git a/pager.c b/pager.c\n>>> index c1ecf65..fa19765 100644\n>>> --- a/pager.c\n>>> +++ b/pager.c\n>>> @@ -54,7 +54,7 @@ const char *git_pager(int stdout_is_tty)\n>>> pager = getenv(\"PAGER\");\n>>> if (!pager)\n>>> pager = DEFAULT_PAGER;\n>>> - else if (!*pager || !strcmp(pager, \"cat\"))\n>>> + if (!*pager || !strcmp(pager, \"cat\"))\n>>\n>> Hmmpf. It's sometimes useful to actually pipe through cat rather than\n>> disabling the pager, as this changes the return-code from isatty. I\n>> sometimes use this for debugging-purposes. Does this patch break that?\n>\n> If you have been running \"GIT_PAGER=cat git whatever\" and the like,\n> we did not pipe the output through \"cat\" and this has been the case\n> for a long time. The only thing the patch in question changed is\n> for those who build with\n>\n> make DEFAULT_PAGER=cat\n>\n> and I doubt that you have been debugging git by rebuilding it with\n> such a setting, so....\n>\n\nYep. This was me simply not thinking things through :)\n"}]}