To page or not to page

To page or not to page

[Kevin Ballard]

c8af1de9cfa0a5678ae766777e0f905e60b69fda makes git status use a pager.  
I think this is a horrible choice. Now when I type `git status` I have  
to type "q" to get back to my shell, and I've lost the output of the  
pager. It makes sense for commands with long output like `git log`,  
but `git status` rarely has output longer than my terminal's height,  
and so making it use the pager simply detracts from its functionality.

Does anybody have any comments or opinions about this either way?

-Kevin Ballard

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: To page or not to page

[Jeff King]

On Fri, May 02, 2008 at 01:41:05AM -0400, Kevin Ballard wrote:

> c8af1de9cfa0a5678ae766777e0f905e60b69fda makes git status use a pager. I 
> think this is a horrible choice. Now when I type `git status` I have to 
> type "q" to get back to my shell, and I've lost the output of the pager. 
> It makes sense for commands with long output like `git log`, but `git 
> status` rarely has output longer than my terminal's height, and so making 
> it use the pager simply detracts from its functionality.
>
> Does anybody have any comments or opinions about this either way?

I agree with you; I don't like it at all. Probably whether or not to use
a pager for a given command should be controlled by a "pager.<cmd>"
config variable.

In the meantime, try adding FX to your LESS environment variable.

-Peff

Re: To page or not to page

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 02, 2008 at 01:41:05AM -0400, Kevin Ballard wrote:
>
>> Does anybody have any comments or opinions about this either way?
>
> I agree with you; I don't like it at all.

I do not care either way.  As I do not have LESS defined in my environment
(which makes git use its built-in that contains FX), I actually never
noticed the difference before or after that patch.

Hmmm, I thought I heard you cheered on that patch?  Perhaps it was
somebody else.

Re: To page or not to page

[Kevin Ballard]

On May 2, 2008, at 1:56 AM, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
>
>> On Fri, May 02, 2008 at 01:41:05AM -0400, Kevin Ballard wrote:
>>
>>> Does anybody have any comments or opinions about this either way?
>>
>> I agree with you; I don't like it at all.
>
> I do not care either way.  As I do not have LESS defined in my  
> environment
> (which makes git use its built-in that contains FX), I actually never
> noticed the difference before or after that patch.

Even if I put FX into LESS (I have it set normally so it contains R),  
it still doesn't behave correctly. I compulsively clear my terminal  
screen whenever I don't need the current contents, but with FX in  
LESS, `git status` throws the output at the bottom of my terminal,  
leaving a lot of blank space at the top. This is extremely irritating  
to me.

Is there any good reason to make git-status use the pager? The output  
is very rarely long enough to warrant it, and if I need a pager I can  
always just pipe it to less myself.

-Kevin Ballard

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: To page or not to page

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

> Is there any good reason to make git-status use the pager? The output
> is very rarely long enough to warrant it, and if I need a pager I can
> always just pipe it to less myself.

I have no motivation to defend that change.  I'll leave that to the
original submitter and list archive ;-)

Re: To page or not to page

[Bart Trojanowski]

* Junio C Hamano <gitster@pobox.com> [080502 02:03]:
> Kevin Ballard <kevin@sb.org> writes:
> 
> > Is there any good reason to make git-status use the pager? The output
> > is very rarely long enough to warrant it, and if I need a pager I can
> > always just pipe it to less myself.
> 
> I have no motivation to defend that change.  I'll leave that to the
> original submitter and list archive ;-)

My goal was to improve consistency with git log and git diff.  

-Bart

-- 
				WebSig: http://www.jukie.net/~bart/sig/

Re: To page or not to page

[Pedro Melo]

On May 2, 2008, at 7:04 AM, Kevin Ballard wrote:

> On May 2, 2008, at 1:56 AM, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> On Fri, May 02, 2008 at 01:41:05AM -0400, Kevin Ballard wrote:
>>>
>>>> Does anybody have any comments or opinions about this either way?
>>>
>>> I agree with you; I don't like it at all.
>>
>> I do not care either way.  As I do not have LESS defined in my  
>> environment
>> (which makes git use its built-in that contains FX), I actually never
>> noticed the difference before or after that patch.
>
> Even if I put FX into LESS (I have it set normally so it contains  
> R), it still doesn't behave correctly. I compulsively clear my  
> terminal screen whenever I don't need the current contents, but  
> with FX in LESS, `git status` throws the output at the bottom of my  
> terminal, leaving a lot of blank space at the top. This is  
> extremely irritating to me.

hmms... I remember you being a Mac user. I'm on 10.4.11 and with

LESS=iFMRSXW
LESSCHARSET=utf-8

it works as if no pager has been set.

Personally, I like the pager because the best part of git-status is  
at the top, so if I hit git-status thats the one I want to see.

Best regards,
-- 
Pedro Melo
Blog: http://www.simplicidade.org/notes/
XMPP ID: melo@simplicidade.org
Use XMPP!

Re: To page or not to page

[Kevin Ballard]

On May 2, 2008, at 5:41 AM, Pedro Melo wrote:

>> Even if I put FX into LESS (I have it set normally so it contains  
>> R), it still doesn't behave correctly. I compulsively clear my  
>> terminal screen whenever I don't need the current contents, but  
>> with FX in LESS, `git status` throws the output at the bottom of my  
>> terminal, leaving a lot of blank space at the top. This is  
>> extremely irritating to me.
>
> hmms... I remember you being a Mac user. I'm on 10.4.11 and with
>
> LESS=iFMRSXW
> LESSCHARSET=utf-8
>
> it works as if no pager has been set.

Except that's not true. As I've mentioned before, if I've cleared the  
terminal, and I run less with FX set, and the output is less than the  
height of my terminal, all the output ends up at the bottom of my  
terminal and the rest of the terminal is blank space. It's rather  
annoying, because it's the exact opposite of how I usually work (I  
compulsively clear my terminal so I can work at the top of the window  
instead of the bottom).

-Kevin

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: To page or not to page

[Wincent Colaiuta]

El 2/5/2008, a las 8:04, Kevin Ballard escribió:

> Is there any good reason to make git-status use the pager? The  
> output is very rarely long enough to warrant it, and if I need a  
> pager I can always just pipe it to less myself.

I for one welcome this change and often have status output that is  
long enough to warrant using a pager. I am constantly seeing more than  
a full screen of results and then either re-running the command as  
"git -p status" or otherwise scrolling upwards to see the part of the  
status output that is actually interesting to me. It has often annoyed  
me that commands like "git log" have always given me a highly usable  
default behaviour "out of the box" (one of the first things I noticed  
about Git when I started using it that made me think, "ah, that's a  
nice touch") while "git status" didn't.

But evidently given that there has been so much backlash against the  
patch the only way to keep everyone happy will be to make this  
configurable.

Cheers,
Wincent

Re: To page or not to page

[Jeff King]

On Fri, May 02, 2008 at 12:34:10PM +0200, Wincent Colaiuta wrote:

> But evidently given that there has been so much backlash against the  
> patch the only way to keep everyone happy will be to make this  
> configurable.

Agreed.

But I wonder why there seems to be such a split between people who
clearly have short git-status output, and those who have long git-status
output.

I keep my "untracked files" list tidy. IOW, I always get:

  $ git status
  # On branch master
  nothing to commit (working directory clean)

and if I don't, then I should be taking some action to commit things,
clean them up, or add them to my .git/info/exclude file. Do other people
generally carry around a lot of cruft that "git status" reports?

-Peff

Re: To page or not to page

[Pedro Melo]

Hi,

On May 2, 2008, at 1:36 PM, Jeff King wrote:

> But I wonder why there seems to be such a split between people who
> clearly have short git-status output, and those who have long git- 
> status
> output.
>
> I keep my "untracked files" list tidy. IOW, I always get:
>
>   $ git status
>   # On branch master
>   nothing to commit (working directory clean)
>
> and if I don't, then I should be taking some action to commit things,
> clean them up, or add them to my .git/info/exclude file. Do other  
> people
> generally carry around a lot of cruft that "git status" reports?

I tend to accumulate "screen-and-a-half" git status outputs.

I usually hack on something until it works, and then I use git-gui  
and the hunk-based selection to order my work into a sane set of  
commits.

Commit'ing more often would make me wander of my task.

I guess its a matter of personal preference.

Best regards,
-- 
Pedro Melo
Blog: http://www.simplicidade.org/notes/
XMPP ID: melo@simplicidade.org
Use XMPP!

Re: To page or not to page

[Aidan Van Dyk]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

* Jeff King <peff@peff.net> [080501 00:00]:
> On Fri, May 02, 2008 at 12:34:10PM +0200, Wincent Colaiuta wrote:
> 
> > But evidently given that there has been so much backlash against the  
> > patch the only way to keep everyone happy will be to make this  
> > configurable.
> 
> Agreed.
> 
> But I wonder why there seems to be such a split between people who
> clearly have short git-status output, and those who have long git-status
> output.
> 
> I keep my "untracked files" list tidy. IOW, I always get:
> 
>   $ git status
>   # On branch master
>   nothing to commit (working directory clean)
> 
> and if I don't, then I should be taking some action to commit things,
> clean them up, or add them to my .git/info/exclude file. Do other people
> generally carry around a lot of cruft that "git status" reports?

I like the change.  I don't usually have long git status output; I
manage my .gitignore and $GITDIR/info/exclude religously.  But in
those cases, the pager has no affect (LESS=FRSX), so it doesn't bother
me.

But sometime I have done something which affects a lot of files.  Either
on purpose, or by accident, and suddenly git status has lots of output.
And I'm usually not thinking git status will have reams of output and
at that time, I really like *not* having to re-run the command with -p
or | $PAGER.

a.


-- 
Aidan Van Dyk                                             Create like a god,
aidan@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: To page or not to page

[Wincent Colaiuta]

El 2/5/2008, a las 14:36, Jeff King escribió:
> On Fri, May 02, 2008 at 12:34:10PM +0200, Wincent Colaiuta wrote:
>
>> But evidently given that there has been so much backlash against the
>> patch the only way to keep everyone happy will be to make this
>> configurable.
>
> Agreed.
>
> But I wonder why there seems to be such a split between people who
> clearly have short git-status output, and those who have long git- 
> status
> output.

I generally try to work on one thing at a time and keep the status  
output short -- and I imagine that that's the way most people work --  
but there are certain things where long output is going to be  
unavoidable, like renaming a directory with lots of files in it. And  
when that happens, seeing only the bottom of the "git status" output  
and not the top is not what I want 100% of the time.

Cheers,
Wincent

Re: To page or not to page

[Kevin Ballard]

On May 2, 2008, at 12:13 PM, Wincent Colaiuta wrote:

> El 2/5/2008, a las 14:36, Jeff King escribió:
>> On Fri, May 02, 2008 at 12:34:10PM +0200, Wincent Colaiuta wrote:
>>
>>> But evidently given that there has been so much backlash against the
>>> patch the only way to keep everyone happy will be to make this
>>> configurable.
>>
>> Agreed.
>>
>> But I wonder why there seems to be such a split between people who
>> clearly have short git-status output, and those who have long git- 
>> status
>> output.
>
> I generally try to work on one thing at a time and keep the status  
> output short -- and I imagine that that's the way most people work  
> -- but there are certain things where long output is going to be  
> unavoidable, like renaming a directory with lots of files in it. And  
> when that happens, seeing only the bottom of the "git status" output  
> and not the top is not what I want 100% of the time.

Doesn't your terminal have the ability to scroll? If my git-status  
output is longer than my terminal's height, I just hit Page Up to see  
the top.

-Kevin

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: To page or not to page

[Wincent Colaiuta]

El 2/5/2008, a las 18:56, Kevin Ballard escribió:
> On May 2, 2008, at 12:13 PM, Wincent Colaiuta wrote:
>
>> El 2/5/2008, a las 14:36, Jeff King escribió:
>>> On Fri, May 02, 2008 at 12:34:10PM +0200, Wincent Colaiuta wrote:
>>>
>>>> But evidently given that there has been so much backlash against  
>>>> the
>>>> patch the only way to keep everyone happy will be to make this
>>>> configurable.
>>>
>>> Agreed.
>>>
>>> But I wonder why there seems to be such a split between people who
>>> clearly have short git-status output, and those who have long git- 
>>> status
>>> output.
>>
>> I generally try to work on one thing at a time and keep the status  
>> output short -- and I imagine that that's the way most people work  
>> -- but there are certain things where long output is going to be  
>> unavoidable, like renaming a directory with lots of files in it.  
>> And when that happens, seeing only the bottom of the "git status"  
>> output and not the top is not what I want 100% of the time.
>
> Doesn't your terminal have the ability to scroll? If my git-status  
> output is longer than my terminal's height, I just hit Page Up to  
> see the top.

Of course it does, but page up won't necessarily take me to the top of  
the "git status" output; usually it takes me to somewhere before it or  
somewhere after it.

Wincent

Re: To page or not to page

[Jeff King]

On Thu, May 01, 2008 at 10:56:40PM -0700, Junio C Hamano wrote:

> I do not care either way.  As I do not have LESS defined in my environment
> (which makes git use its built-in that contains FX), I actually never
> noticed the difference before or after that patch.

I actually like the screen-clearing after-effect, but obviously "F" is
useless without "X".

> Hmmm, I thought I heard you cheered on that patch?  Perhaps it was
> somebody else.

Definitely not me. I think it was Dscho.

-Peff

Re: To page or not to page

[Johannes Schindelin]

Hi,

On Fri, 2 May 2008, Jeff King wrote:

> On Thu, May 01, 2008 at 10:56:40PM -0700, Junio C Hamano wrote:
> 
> > Hmmm, I thought I heard you cheered on that patch?  Perhaps it was 
> > somebody else.
> 
> Definitely not me. I think it was Dscho.

Yep.

I find it amazingly useful to see the first part, you know, the part with 
the staged changes, instead of them whizzing by and me only seeing the 
last part of the untracked files.  And of course, I do not even see the 
label "Untracked files:", because there are so many.

I agree, though, that the paging only makes sense with FSRX (not only 
FX!), and that "less" should have that as default, too.

Ciao,
Dscho

Re: To page or not to page

[Jeff King]

On Fri, May 02, 2008 at 01:45:08AM -0400, Jeff King wrote:

> I agree with you; I don't like it at all. Probably whether or not to use
> a pager for a given command should be controlled by a "pager.<cmd>"
> config variable.

Here is a quick and dirty patch to do that. It should probably be split
into two (there is a big code movement of the commands array), and it
needs documentation and tests. But I'm going to sleep for now.

diff --git a/git.c b/git.c
index 89b431f..ffb2650 100644
--- a/git.c
+++ b/git.c
@@ -6,6 +6,8 @@
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
+int git_wrapper_config(const char *, const char *);
+
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
@@ -239,6 +241,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	if (p->option & RUN_SETUP)
 		prefix = setup_git_directory();
+	git_config(git_wrapper_config);
 	if (p->option & USE_PAGER)
 		setup_pager();
 	if (p->option & NEED_WORK_TREE)
@@ -267,103 +270,103 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	return 0;
 }
 
+static struct cmd_struct commands[] = {
+	{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
+	{ "annotate", cmd_annotate, RUN_SETUP },
+	{ "apply", cmd_apply },
+	{ "archive", cmd_archive },
+	{ "blame", cmd_blame, RUN_SETUP },
+	{ "branch", cmd_branch, RUN_SETUP },
+	{ "bundle", cmd_bundle },
+	{ "cat-file", cmd_cat_file, RUN_SETUP },
+	{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
+	{ "checkout-index", cmd_checkout_index, RUN_SETUP | NEED_WORK_TREE },
+	{ "check-ref-format", cmd_check_ref_format },
+	{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
+	{ "cherry", cmd_cherry, RUN_SETUP },
+	{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+	{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
+	{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
+	{ "commit-tree", cmd_commit_tree, RUN_SETUP },
+	{ "config", cmd_config },
+	{ "count-objects", cmd_count_objects, RUN_SETUP },
+	{ "describe", cmd_describe, RUN_SETUP },
+	{ "diff", cmd_diff },
+	{ "diff-files", cmd_diff_files },
+	{ "diff-index", cmd_diff_index, RUN_SETUP },
+	{ "diff-tree", cmd_diff_tree, RUN_SETUP },
+	{ "fast-export", cmd_fast_export, RUN_SETUP },
+	{ "fetch", cmd_fetch, RUN_SETUP },
+	{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
+	{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
+	{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
+	{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
+	{ "format-patch", cmd_format_patch, RUN_SETUP },
+	{ "fsck", cmd_fsck, RUN_SETUP },
+	{ "fsck-objects", cmd_fsck, RUN_SETUP },
+	{ "gc", cmd_gc, RUN_SETUP },
+	{ "get-tar-commit-id", cmd_get_tar_commit_id },
+	{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
+	{ "help", cmd_help },
+#ifndef NO_CURL
+	{ "http-fetch", cmd_http_fetch, RUN_SETUP },
+#endif
+	{ "init", cmd_init_db },
+	{ "init-db", cmd_init_db },
+	{ "log", cmd_log, RUN_SETUP | USE_PAGER },
+	{ "ls-files", cmd_ls_files, RUN_SETUP },
+	{ "ls-tree", cmd_ls_tree, RUN_SETUP },
+	{ "ls-remote", cmd_ls_remote },
+	{ "mailinfo", cmd_mailinfo },
+	{ "mailsplit", cmd_mailsplit },
+	{ "merge-base", cmd_merge_base, RUN_SETUP },
+	{ "merge-file", cmd_merge_file },
+	{ "merge-ours", cmd_merge_ours, RUN_SETUP },
+	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
+	{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
+	{ "name-rev", cmd_name_rev, RUN_SETUP },
+	{ "pack-objects", cmd_pack_objects, RUN_SETUP },
+	{ "peek-remote", cmd_ls_remote },
+	{ "pickaxe", cmd_blame, RUN_SETUP },
+	{ "prune", cmd_prune, RUN_SETUP },
+	{ "prune-packed", cmd_prune_packed, RUN_SETUP },
+	{ "push", cmd_push, RUN_SETUP },
+	{ "read-tree", cmd_read_tree, RUN_SETUP },
+	{ "reflog", cmd_reflog, RUN_SETUP },
+	{ "remote", cmd_remote, RUN_SETUP },
+	{ "repo-config", cmd_config },
+	{ "rerere", cmd_rerere, RUN_SETUP },
+	{ "reset", cmd_reset, RUN_SETUP },
+	{ "rev-list", cmd_rev_list, RUN_SETUP },
+	{ "rev-parse", cmd_rev_parse },
+	{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
+	{ "rm", cmd_rm, RUN_SETUP },
+	{ "send-pack", cmd_send_pack, RUN_SETUP },
+	{ "shortlog", cmd_shortlog, USE_PAGER },
+	{ "show-branch", cmd_show_branch, RUN_SETUP },
+	{ "show", cmd_show, RUN_SETUP | USE_PAGER },
+	{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE | USE_PAGER },
+	{ "stripspace", cmd_stripspace },
+	{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
+	{ "tag", cmd_tag, RUN_SETUP },
+	{ "tar-tree", cmd_tar_tree },
+	{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
+	{ "update-index", cmd_update_index, RUN_SETUP },
+	{ "update-ref", cmd_update_ref, RUN_SETUP },
+	{ "upload-archive", cmd_upload_archive },
+	{ "verify-tag", cmd_verify_tag, RUN_SETUP },
+	{ "version", cmd_version },
+	{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
+	{ "write-tree", cmd_write_tree, RUN_SETUP },
+	{ "verify-pack", cmd_verify_pack },
+	{ "show-ref", cmd_show_ref, RUN_SETUP },
+	{ "pack-refs", cmd_pack_refs, RUN_SETUP },
+};
+
 static void handle_internal_command(int argc, const char **argv)
 {
 	const char *cmd = argv[0];
-	static struct cmd_struct commands[] = {
-		{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
-		{ "annotate", cmd_annotate, RUN_SETUP },
-		{ "apply", cmd_apply },
-		{ "archive", cmd_archive },
-		{ "blame", cmd_blame, RUN_SETUP },
-		{ "branch", cmd_branch, RUN_SETUP },
-		{ "bundle", cmd_bundle },
-		{ "cat-file", cmd_cat_file, RUN_SETUP },
-		{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
-		{ "checkout-index", cmd_checkout_index,
-			RUN_SETUP | NEED_WORK_TREE},
-		{ "check-ref-format", cmd_check_ref_format },
-		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
-		{ "cherry", cmd_cherry, RUN_SETUP },
-		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
-		{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
-		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
-		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
-		{ "config", cmd_config },
-		{ "count-objects", cmd_count_objects, RUN_SETUP },
-		{ "describe", cmd_describe, RUN_SETUP },
-		{ "diff", cmd_diff },
-		{ "diff-files", cmd_diff_files },
-		{ "diff-index", cmd_diff_index, RUN_SETUP },
-		{ "diff-tree", cmd_diff_tree, RUN_SETUP },
-		{ "fast-export", cmd_fast_export, RUN_SETUP },
-		{ "fetch", cmd_fetch, RUN_SETUP },
-		{ "fetch-pack", cmd_fetch_pack, RUN_SETUP },
-		{ "fetch--tool", cmd_fetch__tool, RUN_SETUP },
-		{ "fmt-merge-msg", cmd_fmt_merge_msg, RUN_SETUP },
-		{ "for-each-ref", cmd_for_each_ref, RUN_SETUP },
-		{ "format-patch", cmd_format_patch, RUN_SETUP },
-		{ "fsck", cmd_fsck, RUN_SETUP },
-		{ "fsck-objects", cmd_fsck, RUN_SETUP },
-		{ "gc", cmd_gc, RUN_SETUP },
-		{ "get-tar-commit-id", cmd_get_tar_commit_id },
-		{ "grep", cmd_grep, RUN_SETUP | USE_PAGER },
-		{ "help", cmd_help },
-#ifndef NO_CURL
-		{ "http-fetch", cmd_http_fetch, RUN_SETUP },
-#endif
-		{ "init", cmd_init_db },
-		{ "init-db", cmd_init_db },
-		{ "log", cmd_log, RUN_SETUP | USE_PAGER },
-		{ "ls-files", cmd_ls_files, RUN_SETUP },
-		{ "ls-tree", cmd_ls_tree, RUN_SETUP },
-		{ "ls-remote", cmd_ls_remote },
-		{ "mailinfo", cmd_mailinfo },
-		{ "mailsplit", cmd_mailsplit },
-		{ "merge-base", cmd_merge_base, RUN_SETUP },
-		{ "merge-file", cmd_merge_file },
-		{ "merge-ours", cmd_merge_ours, RUN_SETUP },
-		{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
-		{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE },
-		{ "mv", cmd_mv, RUN_SETUP | NEED_WORK_TREE },
-		{ "name-rev", cmd_name_rev, RUN_SETUP },
-		{ "pack-objects", cmd_pack_objects, RUN_SETUP },
-		{ "peek-remote", cmd_ls_remote },
-		{ "pickaxe", cmd_blame, RUN_SETUP },
-		{ "prune", cmd_prune, RUN_SETUP },
-		{ "prune-packed", cmd_prune_packed, RUN_SETUP },
-		{ "push", cmd_push, RUN_SETUP },
-		{ "read-tree", cmd_read_tree, RUN_SETUP },
-		{ "reflog", cmd_reflog, RUN_SETUP },
-		{ "remote", cmd_remote, RUN_SETUP },
-		{ "repo-config", cmd_config },
-		{ "rerere", cmd_rerere, RUN_SETUP },
-		{ "reset", cmd_reset, RUN_SETUP },
-		{ "rev-list", cmd_rev_list, RUN_SETUP },
-		{ "rev-parse", cmd_rev_parse },
-		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
-		{ "rm", cmd_rm, RUN_SETUP },
-		{ "send-pack", cmd_send_pack, RUN_SETUP },
-		{ "shortlog", cmd_shortlog, USE_PAGER },
-		{ "show-branch", cmd_show_branch, RUN_SETUP },
-		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
-		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE | USE_PAGER },
-		{ "stripspace", cmd_stripspace },
-		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
-		{ "tag", cmd_tag, RUN_SETUP },
-		{ "tar-tree", cmd_tar_tree },
-		{ "unpack-objects", cmd_unpack_objects, RUN_SETUP },
-		{ "update-index", cmd_update_index, RUN_SETUP },
-		{ "update-ref", cmd_update_ref, RUN_SETUP },
-		{ "upload-archive", cmd_upload_archive },
-		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
-		{ "version", cmd_version },
-		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
-		{ "write-tree", cmd_write_tree, RUN_SETUP },
-		{ "verify-pack", cmd_verify_pack },
-		{ "show-ref", cmd_show_ref, RUN_SETUP },
-		{ "pack-refs", cmd_pack_refs, RUN_SETUP },
-	};
 	int i;
 
 	/* Turn "git cmd --help" into "git help cmd" */
@@ -380,6 +383,31 @@ static void handle_internal_command(int argc, const char **argv)
 	}
 }
 
+void set_command_pager(const char *cmd, int use_pager)
+{
+	int i;
+	for (i = 0; i < ARRAY_SIZE(commands); i++) {
+		struct cmd_struct *p = commands+i;
+		if (!strcmp(p->cmd, cmd)) {
+			if (use_pager)
+				p->option |= USE_PAGER;
+			else
+				p->option &= ~USE_PAGER;
+			return;
+		}
+	}
+}
+
+int git_wrapper_config(const char *var, const char *value)
+{
+	if (!prefixcmp(var, "pager.")) {
+		set_command_pager(var + 6, git_config_bool(var, value));
+		return 0;
+	}
+
+	return 0;
+}
+
 int main(int argc, const char **argv)
 {
 	const char *cmd = argv[0] ? argv[0] : "git-help";

Re: To page or not to page

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> @@ -267,103 +270,103 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
>  	return 0;
>  }
>  
> +static struct cmd_struct commands[] = {
> +	{ "add", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> +	{ "annotate", cmd_annotate, RUN_SETUP },
> +	{ "apply", cmd_apply },
> +	{ "archive", cmd_archive },
> +	{ "blame", cmd_blame, RUN_SETUP },
> +	{ "branch", cmd_branch, RUN_SETUP },
> +	{ "bundle", cmd_bundle },
> +	{ "cat-file", cmd_cat_file, RUN_SETUP },
...
> +};
> ...
> @@ -380,6 +383,31 @@ static void handle_internal_command(int argc, const char **argv)
>  	}
>  }
>  
> +void set_command_pager(const char *cmd, int use_pager)
> +{
> +	int i;
> +	for (i = 0; i < ARRAY_SIZE(commands); i++) {
> +		struct cmd_struct *p = commands+i;
> +		if (!strcmp(p->cmd, cmd)) {
> +			if (use_pager)
> +				p->option |= USE_PAGER;
> +			else
> +				p->option &= ~USE_PAGER;
> +			return;
> +		}
> +	}
> +}

Heh, I like it.  I briefly thought that pager.cat-file may wreak havoc on
scripts, but our pager machanism should be clever enough not to, and
cat-file is a valid variable name in the configuration file format ;-).

Re: To page or not to page

[Jeff King]

On Thu, May 01, 2008 at 11:19:49PM -0700, Junio C Hamano wrote:

> Heh, I like it.  I briefly thought that pager.cat-file may wreak havoc on
> scripts, but our pager machanism should be clever enough not to, and
> cat-file is a valid variable name in the configuration file format ;-).

Yes, I sort of assumed that the pager "auto" setting would take care of
most things. I guess somebody could be crazy enough to set pager to
"always" and pager.mailinfo to "true", but I'm not sure that's worth
avoiding.

My bigger worry is that this affects only builtins. Which makes it
sufficient for turning off the pager for anything that does USE_PAGER.
But you can't turn _on_ the pager for arbitrary commands (e.g.,
pager.pull would be ignored). And some commands use pagers from
sub-commands; e.g., git-stash calls git-diff to show a stash; so turning
off the pager entails setting pager.diff, with no way to differentiate
between stash and regular diff.

So it would be inconsistent and expose implementation details. But maybe
that is OK for now, and we just say "well, everything will become a
builtin eventually." ;)

-Peff

Re: To page or not to page

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> My bigger worry is that this affects only builtins. Which makes it
> sufficient for turning off the pager for anything that does USE_PAGER.

Hmm. How about doing things this way?

 - at the beginning of handle_options() remember argv[0]

 - restructure handle_options() so that it does not run setup_pager() and
   setenv("GIT_PAGER", "cat", 1) inside the loop, but instead remember
   what we had on the command line;

 - after the handle_options() loop, if we saw an explicit --pager,
   --no-pager, that's the decision;

 - otherwise:

   - look at argv[0] to see what the command is;

   - do the config thing to see if there is user preference; if there is
     one, that setting decides;

   - otherwise:

     - see the built-in defaults;

 - and finally use or not use pager depending on what we found above.

I suspect we would want a similar restructure of handle_options() loop
about --git-dir and --git-work-tree so that the loop is only used to
decide what to do and action is carried out after the loop exits, but that
is a separate topic.

Re: To page or not to page

[Jeff King]

On Fri, May 02, 2008 at 11:18:02AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > My bigger worry is that this affects only builtins. Which makes it
> > sufficient for turning off the pager for anything that does USE_PAGER.
> 
> Hmm. How about doing things this way?
> 
>  - at the beginning of handle_options() remember argv[0]
> 
>  - restructure handle_options() so that it does not run setup_pager() and
>    setenv("GIT_PAGER", "cat", 1) inside the loop, but instead remember
>    what we had on the command line;
> 
>  - after the handle_options() loop, if we saw an explicit --pager,
>    --no-pager, that's the decision;
> 
>  - otherwise:
> 
>    - look at argv[0] to see what the command is;
> 
>    - do the config thing to see if there is user preference; if there is
>      one, that setting decides;
> 
>    - otherwise:
> 
>      - see the built-in defaults;
> 
>  - and finally use or not use pager depending on what we found above.

OK, that makes some sense. I think some of what you describe is just
refactoring (e.g., it doesn't matter if we actually do things when we
see --no-pager or afterwards, since it always takes precedence). The key
things are:

  - work not just on running builtins, but before we even figure out
    whether we have a builtin or a script

  - in my patch the config just says "ignore the default USE_PAGER", but
    it really should be "turn off the pager via GIT_PAGER=cat". That way
    you can say pager.stash = false, and it will impact the git-diff
    invocation run by stash.

But that isn't to say the refactoring isn't worth doing to keep things
clean. I will take a stab at restructuring it the way you specified.

There is one remaining annoyance, though: this code is only run via the
git wrapper. That means that you will get different behavior for
"git-stash" versus "git stash". To make that work, we would have to put
equivalent support into each script (although we could hit several at
once with git-sh-setup.sh) and each non-builtin.

-Peff

Re: To page or not to page

[Jeff King]

On Fri, May 02, 2008 at 11:18:02AM -0700, Junio C Hamano wrote:

> Hmm. How about doing things this way?
> 
>  - at the beginning of handle_options() remember argv[0]
> 
>  - restructure handle_options() so that it does not run setup_pager() and
>    setenv("GIT_PAGER", "cat", 1) inside the loop, but instead remember
>    what we had on the command line;
> 
>  - after the handle_options() loop, if we saw an explicit --pager,
>    --no-pager, that's the decision;

This is actually a bit tricky, since there are two code paths; builtins
called as git-foo never even go to handle_options, but they can still
look up the config value.

So how about this:

 - keep a global use_pager = { 0 (explicit no), 1 (explicit yes), -1
   (unknown) }

 - if git-foo, lookup config for pager.foo

 - otherwise we have "git [options] foo"; look for -p / --no-pager; if
   none found, then lookup config for "foo"

 - before proceeding further, "commit" the pager choice by running it
   (if 1), munging GIT_PAGER=cat (if 0), or doing nothing (if -1)

 - before handling an internal command, if use_pager is -1 and the
   command defaults to a pager, we run it then

The patch below implements this.

It would be nice to actually defer running the pager until we are about
to run a git command. I.e., never "commit" to the pager until we are
actually running an internal command or exec'ing an external command.
That way it would be safe to make an alias that called "--no-pager"
(which is currently disallowed).

That works for internal commands, since we know we are going to run one,
and if it fails, we die.  However, for external commands, we just exec
and hope it works. So if we run the pager beforehand, we are now
committed to it, and a further alias cannot set --no-pager. We could
alternatively run it after deciding that running the command is what
we'll do, but then we have to do the PATH lookup ourselves.

So anyway, here is the less invasive version.

---
 git.c |   48 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/git.c b/git.c
index 89b431f..dadca41 100644
--- a/git.c
+++ b/git.c
@@ -6,12 +6,46 @@
 const char git_usage_string[] =
 	"git [--version] [--exec-path[=GIT_EXEC_PATH]] [-p|--paginate|--no-pager] [--bare] [--git-dir=GIT_DIR] [--work-tree=GIT_WORK_TREE] [--help] COMMAND [ARGS]";
 
+static int use_pager = -1;
+static const char *pager_command_key;
+static int pager_command_value;
+
+int pager_command_config(const char *var, const char *value)
+{
+	if (!prefixcmp(var, "pager.") && !strcmp(var + 6, pager_command_key))
+		pager_command_value = git_config_bool(var, value);
+	return 0;
+}
+
+/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
+int check_pager_config(const char *cmd)
+{
+	pager_command_key = cmd;
+	pager_command_value = -1;
+	git_config(pager_command_config);
+	return pager_command_value;
+}
+
+static void commit_pager_choice(void) {
+	switch (use_pager) {
+	case 0:
+		setenv("GIT_PAGER", "cat", 1);
+		break;
+	case 1:
+		setup_pager();
+		break;
+	default:
+		break;
+	}
+}
+
 static int handle_options(const char*** argv, int* argc, int* envchanged)
 {
 	int handled = 0;
+	const char *cmd = NULL;
 
 	while (*argc > 0) {
-		const char *cmd = (*argv)[0];
+		cmd = (*argv)[0];
 		if (cmd[0] != '-')
 			break;
 
@@ -35,9 +69,9 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 				exit(0);
 			}
 		} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
-			setup_pager();
+			use_pager = 1;
 		} else if (!strcmp(cmd, "--no-pager")) {
-			setenv("GIT_PAGER", "cat", 1);
+			use_pager = 0;
 			if (envchanged)
 				*envchanged = 1;
 		} else if (!strcmp(cmd, "--git-dir")) {
@@ -84,6 +118,9 @@ static int handle_options(const char*** argv, int* argc, int* envchanged)
 		(*argc)--;
 		handled++;
 	}
+
+	if (use_pager == -1 && cmd)
+		use_pager = check_pager_config(cmd);
 	return handled;
 }
 
@@ -239,7 +276,7 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	if (p->option & RUN_SETUP)
 		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
+	if (use_pager == -1 && p->option & USE_PAGER)
 		setup_pager();
 	if (p->option & NEED_WORK_TREE)
 		setup_work_tree();
@@ -411,6 +448,8 @@ int main(int argc, const char **argv)
 	if (!prefixcmp(cmd, "git-")) {
 		cmd += 4;
 		argv[0] = cmd;
+		use_pager = check_pager_config(cmd);
+		commit_pager_choice();
 		handle_internal_command(argc, argv);
 		die("cannot handle %s internally", cmd);
 	}
@@ -419,6 +458,7 @@ int main(int argc, const char **argv)
 	argv++;
 	argc--;
 	handle_options(&argv, &argc, NULL);
+	commit_pager_choice();
 	if (argc > 0) {
 		if (!prefixcmp(argv[0], "--"))
 			argv[0] += 2;
-- 
1.5.5.1.244.g148a.dirty

Re: To page or not to page

[Jeff King]

On Tue, May 06, 2008 at 01:51:28AM -0400, Jeff King wrote:

> So anyway, here is the less invasive version.
> 
> ---
>  git.c |   48 ++++++++++++++++++++++++++++++++++++++++++++----
>  1 files changed, 44 insertions(+), 4 deletions(-)

NB: this of course works only for git-* that is a builtin, and does
nothing for an external "git-foo" (of course it does work for "git foo"
in that case).

We need to decide if that is a showstopper, or if it is OK for that
inconsistency to live (I don't think manually fixing all such git-foo is
possible, since they could be user scripts).

-Peff

Re: To page or not to page

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> So how about this:
>
>  - keep a global use_pager = { 0 (explicit no), 1 (explicit yes), -1
>    (unknown) }
>
>  - if git-foo, lookup config for pager.foo
>
>  - otherwise we have "git [options] foo"; look for -p / --no-pager; if
>    none found, then lookup config for "foo"
>
>  - before proceeding further, "commit" the pager choice by running it
>    (if 1), munging GIT_PAGER=cat (if 0), or doing nothing (if -1)
>
>  - before handling an internal command, if use_pager is -1 and the
>    command defaults to a pager, we run it then
>
> The patch below implements this.
>
> It would be nice to actually defer running the pager until we are about
> to run a git command. I.e., never "commit" to the pager until we are
> actually running an internal command or exec'ing an external command.
> That way it would be safe to make an alias that called "--no-pager"
> (which is currently disallowed).

Ok, I agree that this is a less nice (as you mention ablve) but a workable
compromise with the reality.

Re: To page or not to page

[Jeff King]

On Tue, May 06, 2008 at 01:51:28AM -0400, Jeff King wrote:

> So anyway, here is the less invasive version.
> [of configurable paging]

Gah. Lacking any more input, I was going to clean this up and re-submit,
but it seems to fail some unrelated tests.

It appears that with this patch, you can no longer do:

  cd .git && git show HEAD

because of some awful ordering constraints in the wrapper. Specifically,
looking in the config for pager.* entails calling git_path, which ends
up calling setup_git_env, which says "I guess our git_dir is '.git'".
Which is of course totally wrong, and calling setup_git_directory would
find the right thing.

So the logic in setup_git_env seems bogus, but should basically never be
invoked because we do generally call setup_git_directory_gently before
then. Either it should probably call setup_git_directory_gently (though
I am afraid of what awful side effects that could have), or it should
just barf, and people should do setup_git_directory beforehand (and I'm
sure that will break something too).

Blargh.

-Peff

Re: To page or not to page

[Jeff King]

On Fri, May 16, 2008 at 12:42:38AM -0400, Jeff King wrote:

> So the logic in setup_git_env seems bogus, but should basically never be
> invoked because we do generally call setup_git_directory_gently before
> then. Either it should probably call setup_git_directory_gently (though
> I am afraid of what awful side effects that could have), or it should
> just barf, and people should do setup_git_directory beforehand (and I'm
> sure that will break something too).

Hrm. So there are lots of programs that actually _do_ end up needing
this lazy load of the git_dir, but it's just that we have
setup_git_directory'd ourselves into the top of the work tree by then.

So it would be nice if we could move that earlier so that the wrapper
could do useful things like look at the proper config. But I think that
opens a whole can of worms with running setup_git_directory twice, IIRC.

-Peff

Re: To page or not to page

[Johannes Schindelin]

Hi,

On Fri, 16 May 2008, Jeff King wrote:

> On Fri, May 16, 2008 at 12:42:38AM -0400, Jeff King wrote:
> 
> > So the logic in setup_git_env seems bogus, but should basically never 
> > be invoked because we do generally call setup_git_directory_gently 
> > before then. Either it should probably call setup_git_directory_gently 
> > (though I am afraid of what awful side effects that could have), or it 
> > should just barf, and people should do setup_git_directory beforehand 
> > (and I'm sure that will break something too).
> 
> Hrm. So there are lots of programs that actually _do_ end up needing 
> this lazy load of the git_dir, but it's just that we have 
> setup_git_directory'd ourselves into the top of the work tree by then.
> 
> So it would be nice if we could move that earlier so that the wrapper 
> could do useful things like look at the proper config. But I think that 
> opens a whole can of worms with running setup_git_directory twice, IIRC.

Yes.  There be dragons.

Ciao,
Dscho

Re: To page or not to page

[Jakub Narebski]

Jeff King <peff@peff.net> writes:

> On Fri, May 02, 2008 at 01:45:08AM -0400, Jeff King wrote:
> 
> > I agree with you; I don't like it at all. Probably whether or not to use
> > a pager for a given command should be controlled by a "pager.<cmd>"
> > config variable.
> 
> Here is a quick and dirty patch to do that. It should probably be split
> into two (there is a big code movement of the commands array), and it
> needs documentation and tests. But I'm going to sleep for now.

You should then accept pager.core (or pager.ui) as alias to existing
core.pager configuration variable... well, perhaps you did that...

-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: To page or not to page

[Jeff King]

On Thu, May 01, 2008 at 11:56:15PM -0700, Jakub Narebski wrote:

> > Here is a quick and dirty patch to do that. It should probably be split
> > into two (there is a big code movement of the commands array), and it
> > needs documentation and tests. But I'm going to sleep for now.
> 
> You should then accept pager.core (or pager.ui) as alias to existing
> core.pager configuration variable... well, perhaps you did that...

Why? I am claiming the pager.* namespace for turning the pager off and
on for specific commands, which neither of those do (the exception is
the historic pager.color, which would still work).

If you want to argue that all pager options should be consolidated under
pager.*, I think that is a totally separate issue (and one which my
patch argues against, since it could lead to name collisions).

-Peff

Re: To page or not to page

[Jeff King]

On Fri, May 02, 2008 at 02:09:30AM -0400, Jeff King wrote:

> > I agree with you; I don't like it at all. Probably whether or not to use
> > a pager for a given command should be controlled by a "pager.<cmd>"
> > config variable.
> 
> Here is a quick and dirty patch to do that. It should probably be split
> into two (there is a big code movement of the commands array), and it
> needs documentation and tests. But I'm going to sleep for now.

Here is a cleaner patch. Rather than looking at all of pager.*, it waits
until we see which command to execute, and just looks up pager.cmd (we
end up having to parse the config the same number of times). And we
don't have to munge the static global commands array, which just feels a
little cleaner.

Still no documentation, and still not a "real" patch; I am curious to
see the list reaction on the issues I raised elsewhere in the thread
(like the user-facing inconsistencies).

---
 git.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/git.c b/git.c
index 89b431f..68d8b37 100644
--- a/git.c
+++ b/git.c
@@ -230,6 +230,25 @@ struct cmd_struct {
 	int option;
 };
 
+static const char *pager_command_key;
+static int pager_command_value;
+
+int pager_command_config(const char *var, const char *value)
+{
+	if (!prefixcmp(var, "pager.") && !strcmp(var + 6, pager_command_key))
+		pager_command_value = git_config_bool(var, value);
+	return 0;
+}
+
+/* returns 0 for "no pager", 1 for "use pager", and -1 for "not specified" */
+int check_pager_config(const char *cmd)
+{
+	pager_command_key = cmd;
+	pager_command_value = -1;
+	git_config(pager_command_config);
+	return pager_command_value;
+}
+
 static int run_command(struct cmd_struct *p, int argc, const char **argv)
 {
 	int status;
@@ -239,8 +258,17 @@ static int run_command(struct cmd_struct *p, int argc, const char **argv)
 	prefix = NULL;
 	if (p->option & RUN_SETUP)
 		prefix = setup_git_directory();
-	if (p->option & USE_PAGER)
+	switch (check_pager_config(p->cmd)) {
+	case 0:
+		break;
+	case 1:
 		setup_pager();
+		break;
+	default:
+		if (p->option & USE_PAGER)
+			setup_pager();
+		break;
+	}
 	if (p->option & NEED_WORK_TREE)
 		setup_work_tree();
 
-- 
1.5.5.1.221.ga481.dirty