Re: [PATCH/RFC] notice error exit from pager

From: Johannes Sixt <j6t@kdbg.org>
Date: Jul 27, 2011, 19:36 UTC
Message-ID: <4E3068D4.4050006@kdbg.org>
In-Reply-To: <20110726210401.GA25207@toss.lan>

Am 26.07.2011 23:04, schrieb Clemens Buchacher:

Show 11 quoted lines

> If the pager fails to run, git produces no output, e.g.:
> 
>  $ GIT_PAGER=not-a-command git log
> 
> The error reporting fails for two reasons:
> 
>  (1) start_command: There is a mechanism that detects errors during
>      execvp introduced in 2b541bf8 (start_command: detect execvp
>      failures early). ... This mechanism is
>      incompatible with the workaround introduced in 35ce8622
>      (pager: Work around window resizing bug in 'less')

You analysis is correct. I think the bug in less was fixed shortly after this workaround was introduced. I would like to remove this workaround for a different reason, but it seems there are people who are quite fond of it ;)

Show 23 quoted lines

> diff --git a/run-command.c b/run-command.c
> index 70e8a24..944a882 100644
> --- a/run-command.c
> +++ b/run-command.c
> @@ -127,9 +127,6 @@ static int wait_or_whine(pid_t pid, const char *argv0, int silent_exec_failure)
>  		if (code == 127) {
>  			code = -1;
>  			failed_errno = ENOENT;
> -			if (!silent_exec_failure)
> -				error("cannot run %s: %s", argv0,
> -					strerror(ENOENT));
>  		}
>  	} else {
>  		error("waitpid is confused (%s)", argv0);
> @@ -287,10 +284,14 @@ fail_pipe:
>  		 * Do not check for cmd->silent_exec_failure; the parent
>  		 * process will check it when it sees this exit code.
>  		 */
> -		if (errno == ENOENT)
> +		if (errno == ENOENT) {
> +			if (!cmd->silent_exec_failure)
> +				error("cannot run %s: %s", cmd->argv[0],
> +					strerror(ENOENT));

This change is not good enough: There is no guarantee that this message goes to the right channel (stderr in the child can be redirected). Look carefully: We set a special die routine in the forked child that ensures that the die() messages are written to the right channel; we do not have a similar facility for error() messages (or do we?).

Show 8 quoted lines

>  			exit(127);
> -		else
> +		} else {
>  			die_errno("cannot exec '%s'", cmd->argv[0]);
> +		}
>  	}
>  	if (cmd->pid < 0)
>  		error("cannot fork() for %s: %s", cmd->argv[0],

-- Hannes