[PATCH] Move launch_editor() from builtin-tag.c to editor.c

[PATCH] Move launch_editor() from builtin-tag.c to editor.c

[Stephan Beyer]

launch_editor() is declared in strbuf.h but defined in builtin-tag.c.
This patch moves launch_editor() into a new source file editor.c,
but keeps the declaration in strbuf.h.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Hi,

why editor.c and not strbuf.c?
See http://thread.gmane.org/gmane.comp.version-control.git/86636/focus=88939

And I wonder why launch_editor is part of the strbuf API at all. ;)

Regards,
  Stephan

 Makefile      |    1 +
 builtin-tag.c |   53 -----------------------------------------------------
 editor.c      |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 53 deletions(-)
 create mode 100644 editor.c

diff --git a/Makefile b/Makefile
index 75c4ead..4bec4b3 100644
--- a/Makefile
+++ b/Makefile
@@ -409,6 +409,7 @@ LIB_OBJS += diff-no-index.o
 LIB_OBJS += diff-lib.o
 LIB_OBJS += diff.o
 LIB_OBJS += dir.o
+LIB_OBJS += editor.o
 LIB_OBJS += entry.o
 LIB_OBJS += environment.o
 LIB_OBJS += exec_cmd.o
diff --git a/builtin-tag.c b/builtin-tag.c
index c2cca6c..219f51d 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -23,59 +23,6 @@ static const char * const git_tag_usage[] = {
 
 static char signingkey[1000];
 
-void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
-{
-	const char *editor, *terminal;
-
-	editor = getenv("GIT_EDITOR");
-	if (!editor && editor_program)
-		editor = editor_program;
-	if (!editor)
-		editor = getenv("VISUAL");
-	if (!editor)
-		editor = getenv("EDITOR");
-
-	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);
-	}
-
-	if (!editor)
-		editor = "vi";
-
-	if (strcmp(editor, ":")) {
-		size_t len = strlen(editor);
-		int i = 0;
-		const char *args[6];
-		struct strbuf arg0;
-
-		strbuf_init(&arg0, 0);
-		if (strcspn(editor, "$ \t'") != len) {
-			/* there are specials */
-			strbuf_addf(&arg0, "%s \"$@\"", editor);
-			args[i++] = "sh";
-			args[i++] = "-c";
-			args[i++] = arg0.buf;
-		}
-		args[i++] = editor;
-		args[i++] = path;
-		args[i] = NULL;
-
-		if (run_command_v_opt_cd_env(args, 0, NULL, env))
-			die("There was a problem with the editor %s.", editor);
-		strbuf_release(&arg0);
-	}
-
-	if (!buffer)
-		return;
-	if (strbuf_read_file(buffer, path, 0) < 0)
-		die("could not read message file '%s': %s",
-		    path, strerror(errno));
-}
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
diff --git a/editor.c b/editor.c
new file mode 100644
index 0000000..483b62d
--- /dev/null
+++ b/editor.c
@@ -0,0 +1,56 @@
+#include "cache.h"
+#include "strbuf.h"
+#include "run-command.h"
+
+void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor, *terminal;
+
+	editor = getenv("GIT_EDITOR");
+	if (!editor && editor_program)
+		editor = editor_program;
+	if (!editor)
+		editor = getenv("VISUAL");
+	if (!editor)
+		editor = getenv("EDITOR");
+
+	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);
+	}
+
+	if (!editor)
+		editor = "vi";
+
+	if (strcmp(editor, ":")) {
+		size_t len = strlen(editor);
+		int i = 0;
+		const char *args[6];
+		struct strbuf arg0;
+
+		strbuf_init(&arg0, 0);
+		if (strcspn(editor, "$ \t'") != len) {
+			/* there are specials */
+			strbuf_addf(&arg0, "%s \"$@\"", editor);
+			args[i++] = "sh";
+			args[i++] = "-c";
+			args[i++] = arg0.buf;
+		}
+		args[i++] = editor;
+		args[i++] = path;
+		args[i] = NULL;
+
+		if (run_command_v_opt_cd_env(args, 0, NULL, env))
+			die("There was a problem with the editor %s.", editor);
+		strbuf_release(&arg0);
+	}
+
+	if (!buffer)
+		return;
+	if (strbuf_read_file(buffer, path, 0) < 0)
+		die("could not read message file '%s': %s",
+		    path, strerror(errno));
+}
-- 
1.5.6.3.390.g7b30

Re: [PATCH] Move launch_editor() from builtin-tag.c to editor.c

[Johannes Schindelin]

Hi,

On Fri, 18 Jul 2008, Stephan Beyer wrote:

> launch_editor() is declared in strbuf.h but defined in builtin-tag.c. 
> This patch moves launch_editor() into a new source file editor.c, but 
> keeps the declaration in strbuf.h.

Sorry, but that has been tried before.  Junio pointed out that 
launch_editor() iss too die()-happy, and that the messages are too bound 
to the current callers.

You need to at least provide an add-on patch to fix both issues.

Ciao,
Dscho

Re: [PATCH] Move launch_editor() from builtin-tag.c to editor.c

[Stephan Beyer]

Hi,

Johannes Schindelin wrote:
> Hi,
> 
> On Fri, 18 Jul 2008, Stephan Beyer wrote:
> 
> > launch_editor() is declared in strbuf.h but defined in builtin-tag.c. 
> > This patch moves launch_editor() into a new source file editor.c, but 
> > keeps the declaration in strbuf.h.
> 
> Sorry, but that has been tried before.  Junio pointed out that 
> launch_editor() iss too die()-happy, and that the messages are too bound 
> to the current callers.

Ah, a short note: this is not a real "libify" patch. It is more an
"Oh, launch_editor is used for git-commit and git-tag, but it is only
 defined in builtin_tag.c, but declared (as libgit.a member) in strbuf.h,
 so let's move it" patch without further changes.

But of course you and Junio are right.

> You need to at least provide an add-on patch to fix both issues.

Tomorrow, I need sleep first :)

Regards.

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

[PATCH] editor.c: Libify launch_editor()

[Stephan Beyer]

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.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
Is that ok? :)

 builtin-commit.c |    4 +++-
 builtin-tag.c    |    4 +++-
 editor.c         |   21 ++++++++++++---------
 strbuf.h         |    2 +-
 4 files changed, 19 insertions(+), 12 deletions(-)

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.");
 	}
 
 	if (!no_verify &&
diff --git a/builtin-tag.c b/builtin-tag.c
index 219f51d..3b7a25e 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -295,7 +295,9 @@ static void create_tag(const unsigned char *object, const char *tag,
 			write_or_die(fd, tag_template, strlen(tag_template));
 		close(fd);
 
-		launch_editor(path, buf, NULL);
+		if (launch_editor(path, buf, NULL))
+			die("running editor failed.\n"
+			"Please supply the message using either -m or -F option.");
 
 		unlink(path);
 		free(path);
diff --git a/editor.c b/editor.c
index 483b62d..5d7f5f9 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
 	const char *editor, *terminal;
 
@@ -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;
 	}
 
 	if (!editor)
@@ -28,6 +27,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
+		int failed;
 		const char *args[6];
 		struct strbuf arg0;
 
@@ -43,14 +43,17 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		args[i++] = path;
 		args[i] = NULL;
 
-		if (run_command_v_opt_cd_env(args, 0, NULL, env))
-			die("There was a problem with the editor %s.", editor);
+		failed = run_command_v_opt_cd_env(args, 0, NULL, env);
 		strbuf_release(&arg0);
+		if (failed)
+			return error("There was a problem with the editor %s.",
+					editor);
 	}
 
 	if (!buffer)
-		return;
+		return 0;
 	if (strbuf_read_file(buffer, path, 0) < 0)
-		die("could not read message file '%s': %s",
-		    path, strerror(errno));
+		return error("could not read file '%s': %s",
+				path, strerror(errno));
+	return 0;
 }
diff --git a/strbuf.h b/strbuf.h
index 0c6ffad..eba7ba4 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -123,6 +123,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);
-extern void launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
 
 #endif /* STRBUF_H */
-- 
1.5.6.3.390.g7b30

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

[Johannes Schindelin]

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

> 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?

> 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

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

[Stephan Beyer]

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.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

Hi,

Johannes Schindelin wrote:
> 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 :-)
> 
> > 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?

Well, I wanted to do it like that, but die_builtin() usually adds "fatal: ",
so the message becomes like:

	error: There was a problem with the editor xxx.
	fatal: Please supply the message using either -m or -F option.
        ^^^^^^^^^^^^^
        That looks odd.

Btw, using the former patch, it is:

	error: There was a problem with the editor xxx.
	fatal: running editor failed.
	Please supply the message using either -m or -F option.


So I wonder if I should not die() at all, but do a
	fprintf(stderr, "Please supply the message using either -m or -F option.\n");
	exit(1);
which is done in *this* version of the patch. So that the message becomes:

	error: There was a problem with the editor 'xxx'.
	Please supply the message using either -m or -F option.

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

I was unsure here, too, but you're right.

Regards.

 builtin-commit.c |    6 +++++-
 builtin-tag.c    |    6 +++++-
 editor.c         |   24 ++++++++++++------------
 foo              |    1 +
 strbuf.h         |    2 +-
 5 files changed, 24 insertions(+), 15 deletions(-)
 create mode 100644 foo

diff --git a/builtin-commit.c b/builtin-commit.c
index ed3fe3f..23ec629 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -647,7 +647,11 @@ 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)) {
+			fprintf(stderr,
+			"Please supply the message using either -m or -F option.\n");
+			exit(1);
+		}
 	}
 
 	if (!no_verify &&
diff --git a/builtin-tag.c b/builtin-tag.c
index 219f51d..325b1b2 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -295,7 +295,11 @@ static void create_tag(const unsigned char *object, const char *tag,
 			write_or_die(fd, tag_template, strlen(tag_template));
 		close(fd);
 
-		launch_editor(path, buf, NULL);
+		if (launch_editor(path, buf, NULL)) {
+			fprintf(stderr,
+			"Please supply the message using either -m or -F option.\n");
+			exit(1);
+		}
 
 		unlink(path);
 		free(path);
diff --git a/editor.c b/editor.c
index 483b62d..eebc3e9 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
 	const char *editor, *terminal;
 
@@ -15,12 +15,8 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		editor = getenv("EDITOR");
 
 	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);
-	}
+	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
+		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
 
 	if (!editor)
 		editor = "vi";
@@ -28,6 +24,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
+		int failed;
 		const char *args[6];
 		struct strbuf arg0;
 
@@ -43,14 +40,17 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		args[i++] = path;
 		args[i] = NULL;
 
-		if (run_command_v_opt_cd_env(args, 0, NULL, env))
-			die("There was a problem with the editor %s.", editor);
+		failed = run_command_v_opt_cd_env(args, 0, NULL, env);
 		strbuf_release(&arg0);
+		if (failed)
+			return error("There was a problem with the editor '%s'.",
+					editor);
 	}
 
 	if (!buffer)
-		return;
+		return 0;
 	if (strbuf_read_file(buffer, path, 0) < 0)
-		die("could not read message file '%s': %s",
-		    path, strerror(errno));
+		return error("could not read file '%s': %s",
+				path, strerror(errno));
+	return 0;
 }
diff --git a/foo b/foo
new file mode 100644
index 0000000..8b13789
--- /dev/null
+++ b/foo
@@ -0,0 +1 @@
+
diff --git a/strbuf.h b/strbuf.h
index 0c6ffad..eba7ba4 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -123,6 +123,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);
-extern void launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
 
 #endif /* STRBUF_H */
-- 
1.5.6.3.390.g7b30

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

[Stephan Beyer]

Hi,

Johannes Schindelin 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 :-)

Well, I've seen that *a lot* of lib code (15 functions, see below) is in 
the builtins.

Cleaning that up seems to be good to have a real separation between
libgit and builtins, but I guess such a change would not find its way
into 1.6.0, would it?

Regards,
  Stephan

PS: I've spontaneously decided to make a list:

defined-in	func-name	 - used in builtin-\1.c

builtin-add.c:
	add_files_to_cache()	 - add, checkout, commit
	interactive_add()	 - add, commit

builtin-archive.c:
	parse_pathspec_arg()	 - archive, uploard-archive

builtin-init-db.c:
	init_db()		 - init-db, clone

builtin-ls-files.c:
	overlay_tree_on_cache()	 - ls-files, commit
	report_path_error()	 - ls-files, checkout, commit

builtin-mailsplit.c:
	read_line_with_nul()	 - mailsplit, mailinfo

builtin-merge-recursive.c:
	write_tree_from_memory() - merge-recursive, checkout

builtin-prune-packed.c:
	prune_packed_objects()	 - prune-packed, prune

builtin-shortlog.c:
	shortlog_add_commit()	 - shortlog, log
	shortlog_init()		 - shortlog, log
	shortlog_output()	 - shortlog, log
	shortlog_init()		 - shortlog, log

builtin-stripspace.c:
	stripspace()		 - stripspace, commit, tag

And launch_editor() that is handled by the patch in this thread.

Then there are functions that are non-static, but not declined in
any .h file. A patch that makes them static follows in a separate mail.

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

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

[Stephan Beyer]

Stephan Beyer wrote:
> Well, I've seen that *a lot* of lib code (15 functions, see below) is in 
> the builtins.
> 
> Cleaning that up seems to be good to have a real separation between
> libgit and builtins, but I guess such a change would not find its way
> into 1.6.0, would it?
> 
> Regards,
>   Stephan
> 
> PS: I've spontaneously decided to make a list:
> 
> defined-in	func-name	 - used in builtin-\1.c

Of course my list was not complete.
(My regex was too simple.)

I add some:
> builtin-add.c:
> 	add_files_to_cache()	 - add, checkout, commit
> 	interactive_add()	 - add, commit
> 
> builtin-archive.c:
> 	parse_pathspec_arg()	 - archive, upload-archive

	parse_treeish_arg()	 - archive, upload-archive
	parse_archive_args()	 - archive, upload-archive

builtin-commit-tree.c:
	commit_tree()		 - commit-tree, merge

builtin-fetch-pack.c:
	fetch_pack()		 - also used in transport.c

> builtin-init-db.c:
> 	init_db()		 - init-db, clone
> 
> builtin-ls-files.c:
> 	overlay_tree_on_cache()	 - ls-files, commit
> 	report_path_error()	 - ls-files, checkout, commit

	pathspec_match()	 - ls-files, checkout, commit

> builtin-mailsplit.c:
> 	read_line_with_nul()	 - mailsplit, mailinfo
> 
> builtin-merge-recursive.c:
> 	write_tree_from_memory() - merge-recursive, checkout

	merge_trees()		 - merge-recursive, checkout
	merge_recursive()	 - merge-recursive (used nowhere else)

> builtin-prune-packed.c:
> 	prune_packed_objects()	 - prune-packed, prune

builtin-send-pack.c:
	send_pack()		 - also used in transport.c

> builtin-shortlog.c:
> 	shortlog_add_commit()	 - shortlog, log
> 	shortlog_init()		 - shortlog, log
> 	shortlog_output()	 - shortlog, log
> 	shortlog_init()		 - shortlog, log
> 
> builtin-stripspace.c:
> 	stripspace()		 - stripspace, commit, tag
> 
> And launch_editor() that is handled by the patch in this thread.


So keep everything or is it worth splitting that somehow up?

Regards.

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Eh, excuse me.

>> 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
> ...
> Why not "return error()"?
>
> Rest looks obviously correct to me!

This is a patch to an existing file editor.c???

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

[Johannes Schindelin]

Hi,

On Fri, 25 Jul 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Fri, 18 Jul 2008, Stephan Beyer wrote:
> >
> >> 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
> > ...
> > Why not "return error()"?
> >
> > Rest looks obviously correct to me!
> 
> This is a patch to an existing file editor.c???

Yes, Stephan sent the two patches unrelatedly, even if they should have 
been marked "1/2" and "2/2".  I hope he does so now.

Ciao,
Dscho

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

[Stephan Beyer]

Hi,

Johannes Schindelin wrote:
> On Fri, 25 Jul 2008, Junio C Hamano wrote:
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > > On Fri, 18 Jul 2008, Stephan Beyer wrote:
> > >
> > >> 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
> > > ...
> > > Why not "return error()"?
> > >
> > > Rest looks obviously correct to me!
> > 
> > This is a patch to an existing file editor.c???
> 
> Yes, Stephan sent the two patches unrelatedly,

Yes, because when I've sent 1/2, I didn't knew that 2/2 will exist.
So I've just sent the second patch "into this thread" and hoped it would
be clear how it is meant. :)

So, to summarize:
[PATCH 1/2] http://thread.gmane.org/gmane.comp.version-control.git/88940/focus=88940
[PATCH 2/2] http://thread.gmane.org/gmane.comp.version-control.git/88940/focus=89035

They still apply cleanly to current master.

> even if they should have been marked "1/2" and "2/2".
> I hope he does so now.

This means I should send them again?

Regards.

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

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

[Johannes Schindelin]

Hi,

On Fri, 25 Jul 2008, Stephan Beyer wrote:

> Johannes Schindelin wrote:
>
> > even if they should have been marked "1/2" and "2/2". I hope he does 
> > so now.
> 
> This means I should send them again?

If you want to be nice to the maintainer, yes.  If not, no.

Ciao,
Dscho

[PATCH 1/2] Move launch_editor() from builtin-tag.c to editor.c

[Stephan Beyer]

launch_editor() is declared in strbuf.h but defined in builtin-tag.c.
This patch moves launch_editor() into a new source file editor.c,
but keeps the declaration in strbuf.h.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---

Johannes Schindelin wrote:
> > This means I should send them again?
>
> If you want to be nice to the maintainer, yes.  If not, no.

I understand ;-)

No need to play the bad guy, so here's the patchset again.

To be kind to the maintainer, I've also run the test suite again,
all tests passed except t4116*.sh, but this is not my fault.
It's the fault of 9a885fac.

Regards,
  Stephan

 Makefile      |    1 +
 builtin-tag.c |   53 -----------------------------------------------------
 editor.c      |   56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 53 deletions(-)
 create mode 100644 editor.c

diff --git a/Makefile b/Makefile
index b01cf1c..df5b0d0 100644
--- a/Makefile
+++ b/Makefile
@@ -410,6 +410,7 @@ LIB_OBJS += diff-no-index.o
 LIB_OBJS += diff-lib.o
 LIB_OBJS += diff.o
 LIB_OBJS += dir.o
+LIB_OBJS += editor.o
 LIB_OBJS += entry.o
 LIB_OBJS += environment.o
 LIB_OBJS += exec_cmd.o
diff --git a/builtin-tag.c b/builtin-tag.c
index c2cca6c..219f51d 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -23,59 +23,6 @@ static const char * const git_tag_usage[] = {
 
 static char signingkey[1000];
 
-void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
-{
-	const char *editor, *terminal;
-
-	editor = getenv("GIT_EDITOR");
-	if (!editor && editor_program)
-		editor = editor_program;
-	if (!editor)
-		editor = getenv("VISUAL");
-	if (!editor)
-		editor = getenv("EDITOR");
-
-	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);
-	}
-
-	if (!editor)
-		editor = "vi";
-
-	if (strcmp(editor, ":")) {
-		size_t len = strlen(editor);
-		int i = 0;
-		const char *args[6];
-		struct strbuf arg0;
-
-		strbuf_init(&arg0, 0);
-		if (strcspn(editor, "$ \t'") != len) {
-			/* there are specials */
-			strbuf_addf(&arg0, "%s \"$@\"", editor);
-			args[i++] = "sh";
-			args[i++] = "-c";
-			args[i++] = arg0.buf;
-		}
-		args[i++] = editor;
-		args[i++] = path;
-		args[i] = NULL;
-
-		if (run_command_v_opt_cd_env(args, 0, NULL, env))
-			die("There was a problem with the editor %s.", editor);
-		strbuf_release(&arg0);
-	}
-
-	if (!buffer)
-		return;
-	if (strbuf_read_file(buffer, path, 0) < 0)
-		die("could not read message file '%s': %s",
-		    path, strerror(errno));
-}
-
 struct tag_filter {
 	const char *pattern;
 	int lines;
diff --git a/editor.c b/editor.c
new file mode 100644
index 0000000..483b62d
--- /dev/null
+++ b/editor.c
@@ -0,0 +1,56 @@
+#include "cache.h"
+#include "strbuf.h"
+#include "run-command.h"
+
+void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+{
+	const char *editor, *terminal;
+
+	editor = getenv("GIT_EDITOR");
+	if (!editor && editor_program)
+		editor = editor_program;
+	if (!editor)
+		editor = getenv("VISUAL");
+	if (!editor)
+		editor = getenv("EDITOR");
+
+	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);
+	}
+
+	if (!editor)
+		editor = "vi";
+
+	if (strcmp(editor, ":")) {
+		size_t len = strlen(editor);
+		int i = 0;
+		const char *args[6];
+		struct strbuf arg0;
+
+		strbuf_init(&arg0, 0);
+		if (strcspn(editor, "$ \t'") != len) {
+			/* there are specials */
+			strbuf_addf(&arg0, "%s \"$@\"", editor);
+			args[i++] = "sh";
+			args[i++] = "-c";
+			args[i++] = arg0.buf;
+		}
+		args[i++] = editor;
+		args[i++] = path;
+		args[i] = NULL;
+
+		if (run_command_v_opt_cd_env(args, 0, NULL, env))
+			die("There was a problem with the editor %s.", editor);
+		strbuf_release(&arg0);
+	}
+
+	if (!buffer)
+		return;
+	if (strbuf_read_file(buffer, path, 0) < 0)
+		die("could not read message file '%s': %s",
+		    path, strerror(errno));
+}
-- 
1.6.0.rc0.102.ga1791

[PATCH 2/2] editor.c: Libify launch_editor()

[Stephan Beyer]

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.

Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
---
 builtin-commit.c |    6 +++++-
 builtin-tag.c    |    6 +++++-
 editor.c         |   24 ++++++++++++------------
 foo              |    1 +
 strbuf.h         |    2 +-
 5 files changed, 24 insertions(+), 15 deletions(-)
 create mode 100644 foo

diff --git a/builtin-commit.c b/builtin-commit.c
index 6a9dc0e..9a11ca0 100644
--- a/builtin-commit.c
+++ b/builtin-commit.c
@@ -646,7 +646,11 @@ 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)) {
+			fprintf(stderr,
+			"Please supply the message using either -m or -F option.\n");
+			exit(1);
+		}
 	}
 
 	if (!no_verify &&
diff --git a/builtin-tag.c b/builtin-tag.c
index 219f51d..325b1b2 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -295,7 +295,11 @@ static void create_tag(const unsigned char *object, const char *tag,
 			write_or_die(fd, tag_template, strlen(tag_template));
 		close(fd);
 
-		launch_editor(path, buf, NULL);
+		if (launch_editor(path, buf, NULL)) {
+			fprintf(stderr,
+			"Please supply the message using either -m or -F option.\n");
+			exit(1);
+		}
 
 		unlink(path);
 		free(path);
diff --git a/editor.c b/editor.c
index 483b62d..eebc3e9 100644
--- a/editor.c
+++ b/editor.c
@@ -2,7 +2,7 @@
 #include "strbuf.h"
 #include "run-command.h"
 
-void launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
+int launch_editor(const char *path, struct strbuf *buffer, const char *const *env)
 {
 	const char *editor, *terminal;
 
@@ -15,12 +15,8 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		editor = getenv("EDITOR");
 
 	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);
-	}
+	if (!editor && (!terminal || !strcmp(terminal, "dumb")))
+		return error("Terminal is dumb but no VISUAL nor EDITOR defined.");
 
 	if (!editor)
 		editor = "vi";
@@ -28,6 +24,7 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 	if (strcmp(editor, ":")) {
 		size_t len = strlen(editor);
 		int i = 0;
+		int failed;
 		const char *args[6];
 		struct strbuf arg0;
 
@@ -43,14 +40,17 @@ void launch_editor(const char *path, struct strbuf *buffer, const char *const *e
 		args[i++] = path;
 		args[i] = NULL;
 
-		if (run_command_v_opt_cd_env(args, 0, NULL, env))
-			die("There was a problem with the editor %s.", editor);
+		failed = run_command_v_opt_cd_env(args, 0, NULL, env);
 		strbuf_release(&arg0);
+		if (failed)
+			return error("There was a problem with the editor '%s'.",
+					editor);
 	}
 
 	if (!buffer)
-		return;
+		return 0;
 	if (strbuf_read_file(buffer, path, 0) < 0)
-		die("could not read message file '%s': %s",
-		    path, strerror(errno));
+		return error("could not read file '%s': %s",
+				path, strerror(errno));
+	return 0;
 }
diff --git a/foo b/foo
new file mode 100644
index 0000000..8b13789
--- /dev/null
+++ b/foo
@@ -0,0 +1 @@
+
diff --git a/strbuf.h b/strbuf.h
index 0c6ffad..eba7ba4 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -123,6 +123,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);
-extern void launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
+extern int launch_editor(const char *path, struct strbuf *buffer, const char *const *env);
 
 #endif /* STRBUF_H */
-- 
1.6.0.rc0.102.ga1791

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

[Stephan Beyer]

Hi,

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.
> 
> Signed-off-by: Stephan Beyer <s-beyer@gmx.net>
> ---
>  builtin-commit.c |    6 +++++-
>  builtin-tag.c    |    6 +++++-
>  editor.c         |   24 ++++++++++++------------
>  foo              |    1 +

Ouch! Please exclude "foo"...
Sorry, for this maintainer-unfriendly patch again. *embarrassed*

Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F

Re: [PATCH 1/2] Move launch_editor() from builtin-tag.c to editor.c

[Johannes Schindelin]

Hi,

On Fri, 25 Jul 2008, Stephan Beyer wrote:

> To be kind to the maintainer, I've also run the test suite again, all 
> tests passed except t4116*.sh, but this is not my fault. It's the fault 
> of 9a885fac.

You do understand that you cost everybody, who actually cared to take a 
look for herself, a few minutes?

Just to see that the change you referenced (but did not describe at all) 
is "tar -> $TAR".

And now, everybody who cared will be just puzzled.  In the best case, he 
will reply to you that your hint left to be wished for.  I do not have to 
describe the worst case, do I?

Ciao,
Dscho "who thinks that so many mails would be better if the posters would 
read the mails themselves and try to imagine how readers would perceive 
them"

Re: [PATCH 1/2] Move launch_editor() from builtin-tag.c to editor.c

[Stephan Beyer]

Hi,

Johannes Schindelin wrote:
> On Fri, 25 Jul 2008, Stephan Beyer wrote:
> 
> > To be kind to the maintainer, I've also run the test suite again, all 
> > tests passed except t4116*.sh, but this is not my fault. It's the fault 
> > of 9a885fac.
> 
> You do understand that you cost everybody, who actually cared to take a 
> look for herself, a few minutes?

Yes, now I see that I forgot to mention the subject.

I'm sorry.

> Dscho "who thinks that so many mails would be better if the posters would 
> read the mails themselves and try to imagine how readers would perceive 
> them"

You're right.

In my case I perhaps should've *first* looked if I could fix the TAR issue
and then first send the fix for the TAR stuff and then remove the text from
here. But I noticed the failure, wrote about it here, sent this mail, looked
for the reason of the error, ... the wrong order, as it seems.

I try to improve.

Regards,
  Stephan

-- 
Stephan Beyer <s-beyer@gmx.net>, PGP 0x6EDDD207FCC5040F