threads / patch / 14514

patchMove launch_editor() from builtin-tag.c to editor.c

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

## tl;dr

17 messages between Jul 18, 2008 and Jul 26, 2008. Diffs are folded; open one to read it.

replies: 16people: 3as markdown or json

Stephan Beyer· Jul 18, 2008, 00:36 UTC · lore

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
Show changes to 3 files +57 −53

Makefile, builtin-tag.c, 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
Johannes Schindelin· Jul 18, 2008, 01:14 UTC · re: Stephan Beyer · lore

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

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

Stephan Beyer· Jul 18, 2008, 01:26 UTC · re: Johannes Schindelin · lore

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

Hi,
Johannes Schindelin wrote:
Show 11 quoted lines
> 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
Stephan Beyer· Jul 18, 2008, 11:26 UTC · re: Johannes Schindelin · lore

[PATCH] editor.c: Libify launch_editor()

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(-)
Show changes to 4 files +19 −12

builtin-commit.c, builtin-tag.c, editor.c, strbuf.h

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
Johannes Schindelin· Jul 18, 2008, 12:07 UTC · re: Stephan Beyer · lore

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

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

Stephan Beyer· Jul 18, 2008, 12:35 UTC · re: Johannes Schindelin · lore

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

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:
Show 24 quoted lines
> 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.
Show 15 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()"?
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
Show changes to 5 files +24 −15

builtin-commit.c, builtin-tag.c, editor.c, foo, strbuf.h

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
Stephan Beyer· Jul 23, 2008, 23:09 UTC · re: Johannes Schindelin · lore

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

Hi,
Johannes Schindelin wrote:
Show 5 quoted lines
> > 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
Stephan Beyer· Jul 24, 2008, 01:48 UTC · re: Stephan Beyer · lore

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

Stephan Beyer wrote:
Show 13 quoted lines
> 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:
Show 6 quoted lines
> 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
Show 6 quoted lines
> 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
Show 5 quoted lines
> 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
Show 10 quoted lines
> 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
Junio C Hamano· Jul 25, 2008, 08:36 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 9 quoted lines
> 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.
Show 9 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
> ...
> Why not "return error()"?
>
> Rest looks obviously correct to me!
This is a patch to an existing file editor.c???
Johannes Schindelin· Jul 25, 2008, 10:28 UTC · re: Junio C Hamano · lore

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

Hi,
On Fri, 25 Jul 2008, Junio C Hamano wrote:
Show 15 quoted lines
> 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

Stephan Beyer· Jul 25, 2008, 14:15 UTC · re: Johannes Schindelin · lore

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

Hi,
Johannes Schindelin wrote:
Show 18 quoted lines
> 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
Johannes Schindelin· Jul 25, 2008, 14:36 UTC · re: Stephan Beyer · lore

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

Hi,
On Fri, 25 Jul 2008, Stephan Beyer wrote:
Show 6 quoted lines
> 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

Stephan Beyer· Jul 25, 2008, 16:28 UTC · re: Johannes Schindelin · lore

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

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
Show changes to 3 files +57 −53

Makefile, builtin-tag.c, 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
Stephan Beyer· Jul 25, 2008, 16:28 UTC · re: Stephan Beyer · lore

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

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
Show changes to 5 files +24 −15

builtin-commit.c, builtin-tag.c, editor.c, foo, strbuf.h

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
Stephan Beyer· Jul 25, 2008, 17:16 UTC · re: Stephan Beyer · lore

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

Hi,
Stephan Beyer wrote:
Show 10 quoted lines
> 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
Johannes Schindelin· Jul 26, 2008, 03:00 UTC · re: Stephan Beyer · lore

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

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"

Stephan Beyer· Jul 26, 2008, 03:14 UTC · re: Johannes Schindelin · lore

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

Hi,
Johannes Schindelin wrote:
Show 8 quoted lines
> 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

← back to recent threads