{"thread":{"id":"24850","subject":"Fix 'git log' early pager startup error case","startedAt":"2010-08-24T17:33:59Z","lastAt":"2010-08-26T14:13:22Z","messageCount":9,"participants":["Linus Torvalds","Jonathan Nieder","Johannes Sixt","Eric Blake","Junio C Hamano","Joshua Juran"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"148851","messageId":"alpine.LFD.2.00.1008241029530.1046@i5.linux-foundation.org","threadId":"24850","inReplyTo":null,"subject":"Fix 'git log' early pager startup error case","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-08-24T17:33:59Z","receivedAt":"2010-08-24T17:33:59Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"\nWe start the pager too early for several git commands, which results in \nthe errors sometimes going to the pager rather than show up as errors.\n\nThis is often hidden by the fact that we pass in '-X' to less by default, \nwhich causes 'less' to exit for small output, but if you do\n\n export LESS=-S\n\nyou can then clearly see the problem by doing\n\n git log --prretty\n\nwhich shows the error message (\"fatal: unrecognized argument: --prretty\") \nbeing sent to the pager.\n\nThis happens for pretty much all git commands that use USE_PAGER, and then \ncheck arguments separately. But \"git diff\" does it too early too (even \nthough it does an explicit setup_pager() call)\n\nThis only fixes it for the trivial \"git log\" family case.\n\nSigned-off-by: Linus Torvalds \n---\n\nI dunno. I noticed this as a result of a typo, and some (un)happy timing \n(\"less\" will still start up as a pager if the input is delayed a bit). I \nthink this is the right thing to do, but as mentioned, I only fixed a \nparticular small error case.\n\n builtin/log.c | 7 +------\n git.c | 6 +++---\n 2 files changed, 4 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 08b8722..eaa1ee0 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -125,6 +125,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,\n \t\trev->show_decorations = 1;\n \t\tload_ref_decorations(decoration_style);\n \t}\n+\tsetup_pager();\n }\n \n /*\n@@ -491,12 +492,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix)\n \trev.use_terminator = 1;\n \trev.always_show_header = 1;\n \n-\t/*\n-\t * We get called through \"git reflog\", so unlike the other log\n-\t * routines, we need to set up our pager manually..\n-\t */\n-\tsetup_pager();\n-\n \treturn cmd_log_walk(&rev);\n }\n \ndiff --git a/git.c b/git.c\nindex 6fc07a5..12d2952 100644\n--- a/git.c\n+++ b/git.c\n@@ -337,7 +337,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"index-pack\", cmd_index_pack },\n \t\t{ \"init\", cmd_init_db },\n \t\t{ \"init-db\", cmd_init_db },\n-\t\t{ \"log\", cmd_log, RUN_SETUP | USE_PAGER },\n+\t\t{ \"log\", cmd_log, RUN_SETUP },\n \t\t{ \"ls-files\", cmd_ls_files, RUN_SETUP },\n \t\t{ \"ls-tree\", cmd_ls_tree, RUN_SETUP },\n \t\t{ \"ls-remote\", cmd_ls_remote },\n@@ -381,7 +381,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"send-pack\", cmd_send_pack, RUN_SETUP },\n \t\t{ \"shortlog\", cmd_shortlog, USE_PAGER },\n \t\t{ \"show-branch\", cmd_show_branch, RUN_SETUP },\n-\t\t{ \"show\", cmd_show, RUN_SETUP | USE_PAGER },\n+\t\t{ \"show\", cmd_show, RUN_SETUP },\n \t\t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t\t{ \"stripspace\", cmd_stripspace },\n \t\t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n@@ -396,7 +396,7 @@ static void handle_internal_command(int argc, const char **argv)\n \t\t{ \"var\", cmd_var },\n \t\t{ \"verify-tag\", cmd_verify_tag, RUN_SETUP },\n \t\t{ \"version\", cmd_version },\n-\t\t{ \"whatchanged\", cmd_whatchanged, RUN_SETUP | USE_PAGER },\n+\t\t{ \"whatchanged\", cmd_whatchanged, RUN_SETUP },\n \t\t{ \"write-tree\", cmd_write_tree, RUN_SETUP },\n \t\t{ \"verify-pack\", cmd_verify_pack },\n \t\t{ \"show-ref\", cmd_show_ref, RUN_SETUP },\n"},{"id":"148916","messageId":"20100825013625.GC10423@burratino","threadId":"24850","inReplyTo":"alpine.LFD.2.00.1008241029530.1046@i5.linux-foundation.org","subject":"Re: Fix 'git log' early pager startup error case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-25T01:36:25Z","receivedAt":"2010-08-25T01:36:25Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Linus Torvalds wrote:\n\n> I dunno. I noticed this as a result of a typo, and some (un)happy timing \n> (\"less\" will still start up as a pager if the input is delayed a bit). I \n> think this is the right thing to do, but as mentioned, I only fixed a \n> particular small error case.\n\nI like it.\n\nFWIW the change this undoes is v1.4.2-rc3~25^2~1 (Builtins: control\nthe use of pager from the command table., 2006-07-31). [1]\n\n > AFAICS Matthias' patch has the added benefit of moving setup_pager to\n > before large files (i.e. packs) are mapped. This helps non-COW-fork\n > (i.e. cygwin) tremendously. Actually with Linus' setup refactoring\n > this could probably be easily moved to the wrapper,...\n\nMingw Git uses spawnvpe now, but Cygwin users might still suffer from\nfork() troubles. I think it should be possible to work around that by\nusing posix_spawn() from start_command() on such platforms (or\nsometing similar).\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/24438/focus=24507\n"},{"id":"148952","messageId":"4C74BFA7.1090907@viscovery.net","threadId":"24850","inReplyTo":"20100825013625.GC10423@burratino","subject":"Re: Fix 'git log' early pager startup error case","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-08-25T07:00:55Z","receivedAt":"2010-08-25T07:00:55Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 8/25/2010 3:36, schrieb Jonathan Nieder:\n> Mingw Git uses spawnvpe now, but Cygwin users might still suffer from\n> fork() troubles. I think it should be possible to work around that by\n> using posix_spawn() from start_command() on such platforms (or\n> someting similar).\n\nJust FYI, posix_spawn() is not sufficiently capable for the demands of\nstart_command(): It doesn't allow to set a new CWD for the spawned process.\n\n-- Hannes\n"},{"id":"148970","messageId":"4C752739.3010808@redhat.com","threadId":"24850","inReplyTo":"4C74BFA7.1090907@viscovery.net","subject":"Re: Fix 'git log' early pager startup error case","fromName":"Eric Blake","fromEmail":"eblake@redhat.com","sentAt":"2010-08-25T14:22:49Z","receivedAt":"2010-08-25T14:22:49Z","isPatch":false,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"On 08/25/2010 01:00 AM, Johannes Sixt wrote:\n> Am 8/25/2010 3:36, schrieb Jonathan Nieder:\n>> Mingw Git uses spawnvpe now, but Cygwin users might still suffer from\n>> fork() troubles. I think it should be possible to work around that by\n>> using posix_spawn() from start_command() on such platforms (or\n>> someting similar).\n>\n> Just FYI, posix_spawn() is not sufficiently capable for the demands of\n> start_command(): It doesn't allow to set a new CWD for the spawned process.\n\nAnd even if posix_spawn() were capable, cygwin doesn't yet implement it.\n\n-- \nEric Blake eblake@redhat.com +1-801-349-2682\nLibvirt virtualization library http://libvirt.org\n"},{"id":"148987","messageId":"7vtymiqz9c.fsf@alter.siamese.dyndns.org","threadId":"24850","inReplyTo":"alpine.LFD.2.00.1008241029530.1046@i5.linux-foundation.org","subject":"Re: Fix 'git log' early pager startup error case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-08-25T19:16:15Z","receivedAt":"2010-08-25T19:16:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> We start the pager too early for several git commands, which results in \n> the errors sometimes going to the pager rather than show up as errors.\n\nHmm...\n\n $ LESS=-S git log --prettty; echo $?\n ... less shows the message and then the message is lost from the screen\n 128\n $ git --no-pager log --prettty; echo $?\n fatal: unrecognized argument: --prettty\n 128\n $ LESS=-S git log --prettty 2>err; echo $?; cat err\n ... less shows empty and then screen snaps back\n 128\n fatal: unrecognized argument: --prettty\n $ git --no-pager log --prettty 2>err; echo $?; cat err\n 128\n fatal: unrecognized argument: --prettty\n\nIn all cases when the user wants to see the error message s/he sees it,\nwhen the user wants to capture it to a file, it is captured, and the\ncorrect error status is returned to the calling shell.\n\nThe only difference is that after the user dismisses the pager, the error\nmessage is lost. I am not sure if that is a problem, though.\n\nAh, there actually is another difference. This is broken:\n\n $ PAGER=no-such-pager git log --prettty; echo $?\n ... nothing is shown here ...\n 128\n\nand with yours:\n\n $ PAGER=no-such-pager ./git log --prettty; echo $?\n fatal: unrecognized argument: --prettty\n 128\n\nThanks.\n"},{"id":"148989","messageId":"AANLkTi=49opoB-4kA-cjGEUXmVjO6_d-Qdh6tiRxqPM4@mail.gmail.com","threadId":"24850","inReplyTo":"7vtymiqz9c.fsf@alter.siamese.dyndns.org","subject":"Re: Fix 'git log' early pager startup error case","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2010-08-25T20:05:04Z","receivedAt":"2010-08-25T20:05:04Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Wed, Aug 25, 2010 at 12:16 PM, Junio C Hamano wrote:\n>\n> The only difference is that after the user dismisses the pager, the error\n> message is lost.  I am not sure if that is a problem, though.\n\nNo, there's a much more annoying difference. You mentioned it, but ignored it.\n\nThe \"user dismisses the pager\" part.\n\nThat's ANNOYING. I made a damn typo, my command line was bogus. I\ndon't want that pager. I don't want to have to press 'q' to get out of\nthe pager just to fix the mistake I made. I didn't ask for a pager in\nthe first place, and git isn't really outputting any data, so having\nthe pager there is wrong.\n\nHaving the pager there when git actually outputs pages and pages of\ndata is right. I think the \"use pager by default\" is absolutely the\nright design decision. But that doesn't mean that we should use the\npager when there is no data output, just a command line mistake.\n\n Linus\n"},{"id":"149040","messageId":"20100826061815.GH9708@burratino","threadId":"24850","inReplyTo":"4C752739.3010808@redhat.com","subject":"setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2010-08-26T06:18:15Z","receivedAt":"2010-08-26T06:18:15Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(+cc: austin-group-futures)\n\nEric Blake wrote:\n> On 08/25/2010 01:00 AM, Johannes Sixt wrote:\n\n>> Just FYI, posix_spawn() is not sufficiently capable for the demands of\n>> start_command(): It doesn't allow to set a new CWD for the spawned process.\n>\n> And even if posix_spawn() were capable, cygwin doesn't yet implement it.\n\nHmm, okay. You have access to win32api, though, right? So it should\nbe possible to reuse code from compat/mingw.c::mingw_spawnvpe.\n\nDo you think there would be any interest in a posix_spawn() variant\nthat takes a dir parameter? I am imagining something like this:\n\n int posix_spawn2(pid_t *restrict pid, const char *restrict path,\n\tconst posix_spawn_file_actions_t *file_actions,\n\tconst posix_spawnattr_t *restrict attrp,\n\tchar *const argv[restrict], char *const envp[restrict],\n\tconst char *dir);\n\nor this:\n\n int posix_spawn2(pid_t *restrict pid, const char *restrict path,\n\tconst posix_spawn_file_actions_t *file_actions,\n\tconst posix_spawnattr_t *restrict attrp,\n\tchar *const argv[restrict], char *const envp[restrict],\n\tint dirfd);\n\nor this:\n\n int posix_spawn_file_actions_addchdir(posix_spawn_file_actions_t\n\t*file_actions, int dirfd);\n"},{"id":"149052","messageId":"65AAB642-0AEB-40F3-8C33-81AA2C110D3E@gmail.com","threadId":"24850","inReplyTo":"20100826061815.GH9708@burratino","subject":"Re: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case)","fromName":"Joshua Juran","fromEmail":"jjuran@gmail.com","sentAt":"2010-08-26T07:16:06Z","receivedAt":"2010-08-26T07:16:06Z","isPatch":false,"sender":{"key":"jjuran@gmail.com","avatar":null},"body":"On Aug 25, 2010, at 11:18 PM, Jonathan Nieder wrote:\n\n> Eric Blake wrote:\n>> On 08/25/2010 01:00 AM, Johannes Sixt wrote:\n>\n>>> Just FYI, posix_spawn() is not sufficiently capable for the \n>>> demands of\n>>> start_command(): It doesn't allow to set a new CWD for the spawned \n>>> process.\n>>\n>> And even if posix_spawn() were capable, cygwin doesn't yet \n>> implement it.\n>\n> Hmm, okay. You have access to win32api, though, right? So it should\n> be possible to reuse code from compat/mingw.c::mingw_spawnvpe.\n>\n> Do you think there would be any interest in a posix_spawn() variant\n> that takes a dir parameter?\n\nAnother option would be a variant of vfork() where munging file \ndescriptors, cwd, etc. prior to exec is well defined. In Lamp (Lamp \nain't Mac POSIX), vfork() does this already; in Linux you'd just use \nfork(). The problem is that currently no common interface exists \nwhich guarantees behavior which is both sufficient and cheap -- \nvfork() promises nothing at all, and fork() is expensive (if available \nat all) on some platforms, guaranteeing much more than is often needed.\n\nIt would be useful to have something in between vfork() and fork(), \nthough I'm not sure what it should be called.\n\nJosh\n"},{"id":"149064","messageId":"4C767682.7030700@redhat.com","threadId":"24850","inReplyTo":"20100826061815.GH9708@burratino","subject":"Re: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case)","fromName":"Eric Blake","fromEmail":"eblake@redhat.com","sentAt":"2010-08-26T14:13:22Z","receivedAt":"2010-08-26T14:13:22Z","isPatch":false,"sender":{"key":"eblake@redhat.com","avatar":"https://avatars.githubusercontent.com/u/32933908?v=4"},"body":"On 08/26/2010 12:18 AM, Jonathan Nieder wrote:\n> Do you think there would be any interest in a posix_spawn() variant\n> that takes a dir parameter? I am imagining something like this:\n\nOf your variants, I would most prefer:\n\n> int posix_spawn_file_actions_addchdir(posix_spawn_file_actions_t\n> \t*file_actions, int dirfd);\n\nFor that matter, it may also be worth adding \nposix_spawn_file_actions_addopenat, which mirrors the recent addition of \nopenat() semantics.\n\n-- \nEric Blake eblake@redhat.com +1-801-349-2682\nLibvirt virtualization library http://libvirt.org\n"}]}