Fix 'git log' early pager startup error case

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

Linus Torvalds wrote:

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

Am 8/25/2010 3:36, schrieb Jonathan Nieder:

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

On 08/25/2010 01:00 AM, Johannes Sixt wrote:

And even if posix_spawn() were capable, cygwin doesn't yet implement it.

(+cc: austin-group-futures)

Eric Blake wrote:

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

On Aug 25, 2010, at 11:18 PM, Jonathan Nieder wrote:

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

On 08/26/2010 12:18 AM, Jonathan Nieder wrote:

Of your variants, I would most prefer:

For that matter, it may also be worth adding posix_spawn_file_actions_addopenat, which mirrors the recent addition of openat() semantics.

Linus Torvalds <torvalds@linux-foundation.org> writes:

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.

On Wed, Aug 25, 2010 at 12:16 PM, Junio C Hamano <gitster@pobox.com> wrote:

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