Fix 'git log' early pager startup error case

Fix 'git log' early pager startup error case

[Linus Torvalds]

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 <torvalds@linux-foundation.org>
---

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 },

Re: Fix 'git log' early pager startup error case

[Jonathan Nieder]

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

Re: Fix 'git log' early pager startup error case

[Johannes Sixt]

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

Re: Fix 'git log' early pager startup error case

[Eric Blake]

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

setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case)

[Jonathan Nieder]

(+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);

Re: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case)

[Joshua Juran]

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

Re: setting working dir in posix_spawn() (Re: Fix 'git log' early pager startup error case)

[Eric Blake]

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

Re: Fix 'git log' early pager startup error case

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> 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.

Re: Fix 'git log' early pager startup error case

[Linus Torvalds]

On Wed, Aug 25, 2010 at 12:16 PM, Junio C Hamano <gitster@pobox.com> 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