[PATCH] Move stripspace() and launch_editor() to strbuf.c

[PATCH] Move stripspace() and launch_editor() to strbuf.c

[Johannes Schindelin]

These functions are already declared in strbuf.h, so it is only
logical to move their implementations to the corresponding file.
Particularly, since strbuf.h is in LIB_H, but both functions
were missing from libgit.a.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 builtin-stripspace.c |   67 --------------------------------
 builtin-tag.c        |   35 -----------------
 strbuf.c             |  103 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 103 insertions(+), 102 deletions(-)

diff --git a/builtin-stripspace.c b/builtin-stripspace.c
index c0b2130..5de5a3f 100644
--- a/builtin-stripspace.c
+++ b/builtin-stripspace.c
@@ -1,73 +1,6 @@
 #include "builtin.h"
 #include "cache.h"
 
-/*
- * Returns the length of a line, without trailing spaces.
- *
- * If the line ends with newline, it will be removed too.
- */
-static size_t cleanup(char *line, size_t len)
-{
-	while (len) {
-		unsigned char c = line[len - 1];
-		if (!isspace(c))
-			break;
-		len--;
-	}
-
-	return len;
-}
-
-/*
- * Remove empty lines from the beginning and end
- * and also trailing spaces from every line.
- *
- * Note that the buffer will not be NUL-terminated.
- *
- * Turn multiple consecutive empty lines between paragraphs
- * into just one empty line.
- *
- * If the input has only empty lines and spaces,
- * no output will be produced.
- *
- * If last line does not have a newline at the end, one is added.
- *
- * Enable skip_comments to skip every line starting with "#".
- */
-void stripspace(struct strbuf *sb, int skip_comments)
-{
-	int empties = 0;
-	size_t i, j, len, newlen;
-	char *eol;
-
-	/* We may have to add a newline. */
-	strbuf_grow(sb, 1);
-
-	for (i = j = 0; i < sb->len; i += len, j += newlen) {
-		eol = memchr(sb->buf + i, '\n', sb->len - i);
-		len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
-
-		if (skip_comments && len && sb->buf[i] == '#') {
-			newlen = 0;
-			continue;
-		}
-		newlen = cleanup(sb->buf + i, len);
-
-		/* Not just an empty line? */
-		if (newlen) {
-			if (empties > 0 && j > 0)
-				sb->buf[j++] = '\n';
-			empties = 0;
-			memmove(sb->buf + j, sb->buf + i, newlen);
-			sb->buf[newlen + j++] = '\n';
-		} else {
-			empties++;
-		}
-	}
-
-	strbuf_setlen(sb, j);
-}
-
 int cmd_stripspace(int argc, const char **argv, const char *prefix)
 {
 	struct strbuf buf;
diff --git a/builtin-tag.c b/builtin-tag.c
index 566b9d1..c70564b 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -17,41 +17,6 @@ static const char builtin_tag_usage[] =
 
 static char signingkey[1000];
 
-void launch_editor(const char *path, struct strbuf *buffer)
-{
-	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, ":")) {
-		const char *args[] = { editor, path, NULL };
-
-		if (run_command_v_opt(args, 0))
-			die("There was a problem with the editor %s.", editor);
-	}
-
-	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/strbuf.c b/strbuf.c
index 536b432..6e99358 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "run-command.h"
 
 /*
  * Used as the default ->buf value, so that people can always assume
@@ -224,3 +225,105 @@ int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)
 
 	return len;
 }
+
+/*
+ * Returns the length of a line, without trailing spaces.
+ *
+ * If the line ends with newline, it will be removed too.
+ */
+static size_t cleanup(char *line, size_t len)
+{
+	while (len) {
+		unsigned char c = line[len - 1];
+		if (!isspace(c))
+			break;
+		len--;
+	}
+
+	return len;
+}
+
+/*
+ * Remove empty lines from the beginning and end
+ * and also trailing spaces from every line.
+ *
+ * Note that the buffer will not be NUL-terminated.
+ *
+ * Turn multiple consecutive empty lines between paragraphs
+ * into just one empty line.
+ *
+ * If the input has only empty lines and spaces,
+ * no output will be produced.
+ *
+ * If last line does not have a newline at the end, one is added.
+ *
+ * Enable skip_comments to skip every line starting with "#".
+ */
+void stripspace(struct strbuf *sb, int skip_comments)
+{
+	int empties = 0;
+	size_t i, j, len, newlen;
+	char *eol;
+
+	/* We may have to add a newline. */
+	strbuf_grow(sb, 1);
+
+	for (i = j = 0; i < sb->len; i += len, j += newlen) {
+		eol = memchr(sb->buf + i, '\n', sb->len - i);
+		len = eol ? eol - (sb->buf + i) + 1 : sb->len - i;
+
+		if (skip_comments && len && sb->buf[i] == '#') {
+			newlen = 0;
+			continue;
+		}
+		newlen = cleanup(sb->buf + i, len);
+
+		/* Not just an empty line? */
+		if (newlen) {
+			if (empties > 0 && j > 0)
+				sb->buf[j++] = '\n';
+			empties = 0;
+			memmove(sb->buf + j, sb->buf + i, newlen);
+			sb->buf[newlen + j++] = '\n';
+		} else {
+			empties++;
+		}
+	}
+
+	strbuf_setlen(sb, j);
+}
+
+void launch_editor(const char *path, struct strbuf *buffer)
+{
+	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, ":")) {
+		const char *args[] = { editor, path, NULL };
+
+		if (run_command_v_opt(args, 0))
+			die("There was a problem with the editor %s.", editor);
+	}
+
+	if (strbuf_read_file(buffer, path, 0) < 0)
+		die("could not read message file '%s': %s",
+		    path, strerror(errno));
+}
-- 
1.5.3.5.1693.g26ed

Re: [PATCH] Move stripspace() and launch_editor() to strbuf.c

[Junio C Hamano]

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

> These functions are already declared in strbuf.h, so it is only
> logical to move their implementations to the corresponding file.
> Particularly, since strbuf.h is in LIB_H, but both functions
> were missing from libgit.a.

I think this makes sense for stripspace(), but I have trouble
with launch_editor().

I do not object to have a function in strbuf API that takes a
buffer, allows the end user to interactively edit its content
and returns the updated content.  The function was perfectly
fine as a special purpose helper for git-commit and git-tag, but
if you look at the current launch_editor(), it is not suitable
as a generic strbuf library function:

 * "Launch" feels as if we are initiating an async operation and
   returning from the function without waiting for its
   completion, but this is not "launch" but "launch, wait and
   return the resulting string".  Probably this should be called
   edit_buffer() or something like that.

 * Instead of dying, it should return exit code and have the
   caller choose to die or take any alternative action.  The
   library function definitely should not say "if you are in an
   environment where we cannot let you interactively edit, use
   -F or -m option".

Re: [PATCH] Move stripspace() and launch_editor() to strbuf.c

[Johannes Schindelin]

Hi,

On Sun, 11 Nov 2007, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > These functions are already declared in strbuf.h, so it is only
> > logical to move their implementations to the corresponding file.
> > Particularly, since strbuf.h is in LIB_H, but both functions
> > were missing from libgit.a.
> 
> I think this makes sense for stripspace(), but I have trouble
> with launch_editor().
> 
> I do not object to have a function in strbuf API that takes a
> buffer, allows the end user to interactively edit its content
> and returns the updated content.  The function was perfectly
> fine as a special purpose helper for git-commit and git-tag, but
> if you look at the current launch_editor(), it is not suitable
> as a generic strbuf library function:
> 
>  * "Launch" feels as if we are initiating an async operation and
>    returning from the function without waiting for its
>    completion, but this is not "launch" but "launch, wait and
>    return the resulting string".  Probably this should be called
>    edit_buffer() or something like that.
> 
>  * Instead of dying, it should return exit code and have the
>    caller choose to die or take any alternative action.  The
>    library function definitely should not say "if you are in an
>    environment where we cannot let you interactively edit, use
>    -F or -m option".

All valid points.  Will add to my TODO list and repost when done.

Maybe call_editor() instead of edit_buffer()?  Since we technically read 
the file specified by "path" into the buffer "buffer", after having called 
the editor.

Ciao,
Dscho

Re: [PATCH] Move stripspace() and launch_editor() to strbuf.c

[Junio C Hamano]

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

> Maybe call_editor() instead of edit_buffer()?  Since we technically read 
> the file specified by "path" into the buffer "buffer", after having called 
> the editor.

Calling $EDITOR to edit the buffer is an implementation detail
of letting the user edit the buffer.  I think the function name
should express what it does more than how it does it.

My suggested name does not convey the "letting the user" part,
though.