# Fix 'git log' early pager startup error case 9 messages from 2010-08-24 to 2010-08-26. Participants: Linus Torvalds, Jonathan Nieder, Johannes Sixt, Eric Blake, Junio C Hamano, Joshua Juran. Thread: https://gitlist.dev/t/24850 ## Linus Torvalds, 2010-08-24 17:33 Subject: Fix 'git log' early pager startup error case Message-ID: URL: https://gitlist.dev/e/alpine.LFD.2.00.1008241029530.1046%40i5.linux-foundation.org ``` We start the pager too early for several git commands, which results in the errors sometimes going to the pager rather than show up as errors. This is often hidden by the fact that we pass in '-X' to less by default, which causes 'less' to exit for small output, but if you do export LESS=-S you can then clearly see the problem by doing git log --prretty which shows the error message ("fatal: unrecognized argument: --prretty") being sent to the pager. This happens for pretty much all git commands that use USE_PAGER, and then check arguments separately. But "git diff" does it too early too (even though it does an explicit setup_pager() call) This only fixes it for the trivial "git log" family case. Signed-off-by: Linus Torvalds --- I dunno. I noticed this as a result of a typo, and some (un)happy timing ("less" will still start up as a pager if the input is delayed a bit). I think this is the right thing to do, but as mentioned, I only fixed a particular small error case. builtin/log.c | 7 +------ git.c | 6 +++--- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 08b8722..eaa1ee0 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -125,6 +125,7 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix, rev->show_decorations = 1; load_ref_decorations(decoration_style); } + setup_pager(); } /* @@ -491,12 +492,6 @@ int cmd_log_reflog(int argc, const char **argv, const char *prefix) rev.use_terminator = 1; rev.always_show_header = 1; - /* - * We get called through "git reflog", so unlike the other log - * routines, we need to set up our pager manually.. - */ - setup_pager(); - return cmd_log_walk(&rev); } diff --git a/git.c b/git.c index 6fc07a5..12d2952 100644 --- a/git.c +++ b/git.c @@ -337,7 +337,7 @@ static void handle_internal_command(int argc, const char **argv) { "index-pack", cmd_index_pack }, { "init", cmd_init_db }, { "init-db", cmd_init_db }, - { "log", cmd_log, RUN_SETUP | USE_PAGER }, + { "log", cmd_log, RUN_SETUP }, { "ls-files", cmd_ls_files, RUN_SETUP }, { "ls-tree", cmd_ls_tree, RUN_SETUP }, { "ls-remote", cmd_ls_remote }, @@ -381,7 +381,7 @@ static void handle_internal_command(int argc, const char **argv) { "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 }, + { "show", cmd_show, RUN_SETUP }, { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE }, { "stripspace", cmd_stripspace }, { "symbolic-ref", cmd_symbolic_ref, RUN_SETUP }, @@ -396,7 +396,7 @@ static void handle_internal_command(int argc, const char **argv) { "var", cmd_var }, { "verify-tag", cmd_verify_tag, RUN_SETUP }, { "version", cmd_version }, - { "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER }, + { "whatchanged", cmd_whatchanged, RUN_SETUP }, { "write-tree", cmd_write_tree, RUN_SETUP }, { "verify-pack", cmd_verify_pack }, { "show-ref", cmd_show_ref, RUN_SETUP }, ``` ## Jonathan Nieder, 2010-08-25 01:36 Subject: Re: Fix 'git log' early pager startup error case Message-ID: <20100825013625.GC10423@burratino> URL: https://gitlist.dev/e/20100825013625.GC10423%40burratino In-Reply-To: ``` Linus Torvalds wrote: > I dunno. I noticed this as a result of a typo, and some (un)happy timing > ("less" will still start up as a pager if the input is delayed a bit). I > think this is the right thing to do, but as mentioned, I only fixed a > particular small error case. I like it. FWIW the change this undoes is v1.4.2-rc3~25^2~1 (Builtins: control the use of pager from the command table., 2006-07-31). [1] > AFAICS Matthias' patch has the added benefit of moving setup_pager to > before large files (i.e. packs) are mapped. This helps non-COW-fork > (i.e. cygwin) tremendously. Actually with Linus' setup refactoring > this could probably be easily moved to the wrapper,... Mingw Git uses spawnvpe now, but Cygwin users might still suffer from fork() troubles. I think it should be possible to work around that by using posix_spawn() from start_command() on such platforms (or someting similar). [1] http://thread.gmane.org/gmane.comp.version-control.git/24438/focus=24507 ``` ## Johannes Sixt, 2010-08-25 07:00 Subject: Re: Fix 'git log' early pager startup error case Message-ID: <4C74BFA7.1090907@viscovery.net> URL: https://gitlist.dev/e/4C74BFA7.1090907%40viscovery.net In-Reply-To: <20100825013625.GC10423@burratino> ``` Am 8/25/2010 3:36, schrieb Jonathan Nieder: > Mingw Git uses spawnvpe now, but Cygwin users might still suffer from > fork() troubles. I think it should be possible to work around that by > using posix_spawn() from start_command() on such platforms (or > someting similar). Just FYI, posix_spawn() is not sufficiently capable for the demands of start_command(): It doesn't allow to set a new CWD for the spawned process. -- Hannes ``` ## Eric Blake, 2010-08-25 14:22 Subject: Re: Fix 'git log' early pager startup error case Message-ID: <4C752739.3010808@redhat.com> URL: https://gitlist.dev/e/4C752739.3010808%40redhat.com In-Reply-To: <4C74BFA7.1090907@viscovery.net> ``` On 08/25/2010 01:00 AM, Johannes Sixt wrote: > Am 8/25/2010 3:36, schrieb Jonathan Nieder: >> Mingw Git uses spawnvpe now, but Cygwin users might still suffer from >> fork() troubles. I think it should be possible to work around that by >> using posix_spawn() from start_command() on such platforms (or >> someting similar). > > Just FYI, posix_spawn() is not sufficiently capable for the demands of > start_command(): It doesn't allow to set a new CWD for the spawned process. And even if posix_spawn() were capable, cygwin doesn't yet implement it. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org ``` ## Junio C Hamano, 2010-08-25 19:16 Subject: Re: Fix 'git log' early pager startup error case Message-ID: <7vtymiqz9c.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vtymiqz9c.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Linus Torvalds writes: > We start the pager too early for several git commands, which results in > the errors sometimes going to the pager rather than show up as errors. Hmm... $ LESS=-S git log --prettty; echo $? ... less shows the message and then the message is lost from the screen 128 $ git --no-pager log --prettty; echo $? fatal: unrecognized argument: --prettty 128 $ LESS=-S git log --prettty 2>err; echo $?; cat err ... less shows empty and then screen snaps back 128 fatal: unrecognized argument: --prettty $ git --no-pager log --prettty 2>err; echo $?; cat err 128 fatal: unrecognized argument: --prettty In all cases when the user wants to see the error message s/he sees it, when the user wants to capture it to a file, it is captured, and the correct error status is returned to the calling shell. The only difference is that after the user dismisses the pager, the error message is lost. I am not sure if that is a problem, though. Ah, there actually is another difference. This is broken: $ PAGER=no-such-pager git log --prettty; echo $? ... nothing is shown here ... 128 and with yours: $ PAGER=no-such-pager ./git log --prettty; echo $? fatal: unrecognized argument: --prettty 128 Thanks. ``` ## Linus Torvalds, 2010-08-25 20:05 Subject: Re: Fix 'git log' early pager startup error case Message-ID: URL: https://gitlist.dev/e/AANLkTi%3D49opoB-4kA-cjGEUXmVjO6_d-Qdh6tiRxqPM4%40mail.gmail.com In-Reply-To: <7vtymiqz9c.fsf@alter.siamese.dyndns.org> ``` On Wed, Aug 25, 2010 at 12:16 PM, Junio C Hamano wrote: > > The only difference is that after the user dismisses the pager, the error > message is lost.  I am not sure if that is a problem, though. No, there's a much more annoying difference. You mentioned it, but ignored it. The "user dismisses the pager" part. That's ANNOYING. I made a damn typo, my command line was bogus. I don't want that pager. I don't want to have to press 'q' to get out of the pager just to fix the mistake I made. I didn't ask for a pager in the first place, and git isn't really outputting any data, so having the pager there is wrong. Having the pager there when git actually outputs pages and pages of data is right. I think the "use pager by default" is absolutely the right design decision. But that doesn't mean that we should use the pager when there is no data output, just a command line mistake. Linus ``` ## Jonathan Nieder, 2010-08-26 06:18 Subject: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case) Message-ID: <20100826061815.GH9708@burratino> URL: https://gitlist.dev/e/20100826061815.GH9708%40burratino In-Reply-To: <4C752739.3010808@redhat.com> ``` (+cc: austin-group-futures) Eric Blake wrote: > On 08/25/2010 01:00 AM, Johannes Sixt wrote: >> Just FYI, posix_spawn() is not sufficiently capable for the demands of >> start_command(): It doesn't allow to set a new CWD for the spawned process. > > And even if posix_spawn() were capable, cygwin doesn't yet implement it. Hmm, okay. You have access to win32api, though, right? So it should be possible to reuse code from compat/mingw.c::mingw_spawnvpe. Do you think there would be any interest in a posix_spawn() variant that takes a dir parameter? I am imagining something like this: int posix_spawn2(pid_t *restrict pid, const char *restrict path, const posix_spawn_file_actions_t *file_actions, const posix_spawnattr_t *restrict attrp, char *const argv[restrict], char *const envp[restrict], const char *dir); or this: int posix_spawn2(pid_t *restrict pid, const char *restrict path, const posix_spawn_file_actions_t *file_actions, const posix_spawnattr_t *restrict attrp, char *const argv[restrict], char *const envp[restrict], int dirfd); or this: int posix_spawn_file_actions_addchdir(posix_spawn_file_actions_t *file_actions, int dirfd); ``` ## Joshua Juran, 2010-08-26 07:16 Subject: Re: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case) Message-ID: <65AAB642-0AEB-40F3-8C33-81AA2C110D3E@gmail.com> URL: https://gitlist.dev/e/65AAB642-0AEB-40F3-8C33-81AA2C110D3E%40gmail.com In-Reply-To: <20100826061815.GH9708@burratino> ``` On Aug 25, 2010, at 11:18 PM, Jonathan Nieder wrote: > Eric Blake wrote: >> On 08/25/2010 01:00 AM, Johannes Sixt wrote: > >>> Just FYI, posix_spawn() is not sufficiently capable for the >>> demands of >>> start_command(): It doesn't allow to set a new CWD for the spawned >>> process. >> >> And even if posix_spawn() were capable, cygwin doesn't yet >> implement it. > > Hmm, okay. You have access to win32api, though, right? So it should > be possible to reuse code from compat/mingw.c::mingw_spawnvpe. > > Do you think there would be any interest in a posix_spawn() variant > that takes a dir parameter? Another option would be a variant of vfork() where munging file descriptors, cwd, etc. prior to exec is well defined. In Lamp (Lamp ain't Mac POSIX), vfork() does this already; in Linux you'd just use fork(). The problem is that currently no common interface exists which guarantees behavior which is both sufficient and cheap -- vfork() promises nothing at all, and fork() is expensive (if available at all) on some platforms, guaranteeing much more than is often needed. It would be useful to have something in between vfork() and fork(), though I'm not sure what it should be called. Josh ``` ## Eric Blake, 2010-08-26 14:13 Subject: Re: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case) Message-ID: <4C767682.7030700@redhat.com> URL: https://gitlist.dev/e/4C767682.7030700%40redhat.com In-Reply-To: <20100826061815.GH9708@burratino> ``` On 08/26/2010 12:18 AM, Jonathan Nieder wrote: > Do you think there would be any interest in a posix_spawn() variant > that takes a dir parameter? I am imagining something like this: Of your variants, I would most prefer: > int posix_spawn_file_actions_addchdir(posix_spawn_file_actions_t > *file_actions, int dirfd); For that matter, it may also be worth adding posix_spawn_file_actions_addopenat, which mirrors the recent addition of openat() semantics. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org ```