Re: the pager
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
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
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
> 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
> 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
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
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
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
> 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
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