Re: [PATCH] editor.c: Libify launch_editor()

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 18, 2008, 12:07 UTC
Message-ID: <alpine.DEB.1.00.0807181405510.3932@eeepc-johanness>
In-Reply-To: <1216380408-21671-1-git-send-email-s-beyer@gmx.net>

Hi,

On Fri, 18 Jul 2008, Stephan Beyer wrote:

> This patch removes exit()/die() calls and builtin-specific messages from 
> launch_editor(), so that it can be used as a general libgit.a function 
> to launch an editor.

Thanks.  Now we have to convince Junio that it is a good idea :-)

Show 12 quoted lines

> diff --git a/builtin-commit.c b/builtin-commit.c
> index ed3fe3f..64f69f3 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -647,7 +647,9 @@ static int prepare_to_commit(const char *index_file, const char *prefix)
>  		char index[PATH_MAX];
>  		const char *env[2] = { index, NULL };
>  		snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> -		launch_editor(git_path(commit_editmsg), NULL, env);
> +		if (launch_editor(git_path(commit_editmsg), NULL, env))
> +			die("running editor failed.\n"
> +			"Please supply the message using either -m or -F option.");

In the error case, run_editor() already said more than "running editor failed.", right? Maybe you just want to skip that line and keep the second?

Show 13 quoted lines

> diff --git a/editor.c b/editor.c
> index 483b62d..5d7f5f9 100644
> --- a/editor.c
> +++ b/editor.c
> @@ -17,9 +17,8 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
>  	terminal = getenv("TERM");
>  	if (!editor && (!terminal || !strcmp(terminal, "dumb"))) {
>  		fprintf(stderr,
> -		"Terminal is dumb but no VISUAL nor EDITOR defined.\n"
> -		"Please supply the message using either -m or -F option.\n");
> -		exit(1);
> +		"Terminal is dumb but no VISUAL nor EDITOR defined.\n");
> +		return 1;

Why not "return error()"?

Rest looks obviously correct to me!

Thanks, Dscho