[PATCH/RFC 0/5] status, commit: separate "# " from the translatable part of output

[PATCH/RFC 0/5] status, commit: separate "# " from the translatable part of output

[Jonathan Nieder]

Hi,

This series makes wt-status and builtin-commit code a little
less explicit about the '#' signs they produce in their output.
The main goal is to make messages like

		"\n"
		"It looks like you may be committing a MERGE.\n"
		"If this is not correct, please remove the file\n"
		"	%s\n"
		"and try again.\n"
		""

safe to translate by getting rid of the functionally important initial
# signs; a nice side effect is to make the source code a little easier
# to read and change (for example if we want to use some other kind
of formatting some day for "git status" output).

The first two patches (va_copy, strbuf_vaddf) are stolen from Jeff's
GIT_TRACE_FACET series.

Thanks for your help and patience so far.  I hope the series can be
useful.

Jeff King (2):
  compat: provide a fallback va_copy definition
  strbuf: add strbuf_vaddf

Jonathan Nieder (3):
  wt-status: add helpers for printing wt-status lines
  commit: refer to commit template as s->fp
  commit, status: use status_printf{,_ln,_more} helpers

 builtin/commit.c  |   58 ++++++++++----------
 color.c           |    9 +++
 color.h           |    3 +
 compat/msvc.h     |    1 -
 fsck.c            |   14 +----
 git-compat-util.h |    4 +
 merge-recursive.c |   15 +-----
 strbuf.c          |   27 +++++----
 strbuf.h          |    2 +
 trace.c           |   32 ++---------
 wt-status.c       |  160 ++++++++++++++++++++++++++++++++++++++--------------
 wt-status.h       |    7 ++
 12 files changed, 195 insertions(+), 137 deletions(-)

[PATCH 1/5] compat: provide a fallback va_copy definition

[Jonathan Nieder]

From: Jeff King <peff@peff.net>

va_copy is C99.  We have avoided using va_copy many times in the past,
which has led to a bunch of cut-and-paste.  From everything I found
searching the web, implementations have historically either provided
va_copy or just let your code assume that simple assignment of worked.

So my guess is that this will be sufficient, though we won't really
know for sure until somebody reports a problem.

Signed-off-by: Jeff King <peff@peff.net>
Improved-by: Erik Faye-Lund <kusmabite@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 compat/msvc.h     |    1 -
 git-compat-util.h |    4 ++++
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/compat/msvc.h b/compat/msvc.h
index 023aba0..a33b01c 100644
--- a/compat/msvc.h
+++ b/compat/msvc.h
@@ -9,7 +9,6 @@
 #define inline __inline
 #define __inline__ __inline
 #define __attribute__(x)
-#define va_copy(dst, src)     ((dst) = (src))
 #define strncasecmp  _strnicmp
 #define ftruncate    _chsize
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 9c23622..00d41e4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -535,6 +535,10 @@ void git_qsort(void *base, size_t nmemb, size_t size,
 #define fstat_is_reliable() 1
 #endif
 
+#ifndef va_copy
+#define va_copy(dst,src) (dst) = (src)
+#endif
+
 /*
  * Preserves errno, prints a message, but gives no warning for ENOENT.
  * Always returns the return value of unlink(2).
-- 
1.7.4.1

[PATCH 2/5] strbuf: add strbuf_vaddf

[Jonathan Nieder]

From: Jeff King <peff@peff.net>

In a variable-args function, the code for writing into a strbuf is
non-trivial. We ended up cutting and pasting it in several places
because there was no vprintf-style function for strbufs (which in turn
was held up by a lack of va_copy).

Now that we have a fallback va_copy, we can add strbuf_vaddf, the
strbuf equivalent of vsprintf. And we can clean up the cut and paste
mess.

Signed-off-by: Jeff King <peff@peff.net>
Improved-by: Christian Couder <christian.couder@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fsck.c            |   14 +-------------
 merge-recursive.c |   15 +--------------
 strbuf.c          |   27 ++++++++++++++++-----------
 strbuf.h          |    2 ++
 trace.c           |   32 ++++++--------------------------
 5 files changed, 26 insertions(+), 64 deletions(-)

diff --git a/fsck.c b/fsck.c
index 3d05d4a..6f266c1 100644
--- a/fsck.c
+++ b/fsck.c
@@ -347,26 +347,14 @@ int fsck_object(struct object *obj, int strict, fsck_error error_func)
 int fsck_error_function(struct object *obj, int type, const char *fmt, ...)
 {
 	va_list ap;
-	int len;
 	struct strbuf sb = STRBUF_INIT;
 
 	strbuf_addf(&sb, "object %s:", obj->sha1?sha1_to_hex(obj->sha1):"(null)");
 
 	va_start(ap, fmt);
-	len = vsnprintf(sb.buf + sb.len, strbuf_avail(&sb), fmt, ap);
+	strbuf_vaddf(&sb, fmt, ap);
 	va_end(ap);
 
-	if (len < 0)
-		len = 0;
-	if (len >= strbuf_avail(&sb)) {
-		strbuf_grow(&sb, len + 2);
-		va_start(ap, fmt);
-		len = vsnprintf(sb.buf + sb.len, strbuf_avail(&sb), fmt, ap);
-		va_end(ap);
-		if (len >= strbuf_avail(&sb))
-			die("this should not happen, your snprintf is broken");
-	}
-
 	error("%s", sb.buf);
 	strbuf_release(&sb);
 	return 1;
diff --git a/merge-recursive.c b/merge-recursive.c
index 16c2dbe..2a4f739 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -137,7 +137,6 @@ static void flush_output(struct merge_options *o)
 __attribute__((format (printf, 3, 4)))
 static void output(struct merge_options *o, int v, const char *fmt, ...)
 {
-	int len;
 	va_list ap;
 
 	if (!show(o, v))
@@ -148,21 +147,9 @@ static void output(struct merge_options *o, int v, const char *fmt, ...)
 	strbuf_setlen(&o->obuf, o->obuf.len + o->call_depth * 2);
 
 	va_start(ap, fmt);
-	len = vsnprintf(o->obuf.buf + o->obuf.len, strbuf_avail(&o->obuf), fmt, ap);
+	strbuf_vaddf(&o->obuf, fmt, ap);
 	va_end(ap);
 
-	if (len < 0)
-		len = 0;
-	if (len >= strbuf_avail(&o->obuf)) {
-		strbuf_grow(&o->obuf, len + 2);
-		va_start(ap, fmt);
-		len = vsnprintf(o->obuf.buf + o->obuf.len, strbuf_avail(&o->obuf), fmt, ap);
-		va_end(ap);
-		if (len >= strbuf_avail(&o->obuf)) {
-			die("this should not happen, your snprintf is broken");
-		}
-	}
-	strbuf_setlen(&o->obuf, o->obuf.len + len);
 	strbuf_add(&o->obuf, "\n", 1);
 	if (!o->buffer_output)
 		flush_output(o);
diff --git a/strbuf.c b/strbuf.c
index 07e8883..77444a9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -195,24 +195,29 @@ void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len)
 
 void strbuf_addf(struct strbuf *sb, const char *fmt, ...)
 {
-	int len;
 	va_list ap;
-
-	if (!strbuf_avail(sb))
-		strbuf_grow(sb, 64);
 	va_start(ap, fmt);
-	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
+	strbuf_vaddf(sb, fmt, ap);
 	va_end(ap);
+}
+
+void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
+{
+	int len;
+	va_list cp;
+
+	if (!strbuf_avail(sb))
+		strbuf_grow(sb, 64);
+	va_copy(cp, ap);
+	len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, cp);
+	va_end(cp);
 	if (len < 0)
-		die("your vsnprintf is broken");
+		die("BUG: your vsnprintf is broken (returned %d)", len);
 	if (len > strbuf_avail(sb)) {
 		strbuf_grow(sb, len);
-		va_start(ap, fmt);
 		len = vsnprintf(sb->buf + sb->len, sb->alloc - sb->len, fmt, ap);
-		va_end(ap);
-		if (len > strbuf_avail(sb)) {
-			die("this should not happen, your snprintf is broken");
-		}
+		if (len > strbuf_avail(sb))
+			die("BUG: your vsnprintf is broken (insatiable)");
 	}
 	strbuf_setlen(sb, sb->len + len);
 }
diff --git a/strbuf.h b/strbuf.h
index 675a91f..f722331 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -120,6 +120,8 @@ extern void strbuf_addbuf_percentquote(struct strbuf *dst, const struct strbuf *
 
 __attribute__((format (printf,2,3)))
 extern void strbuf_addf(struct strbuf *sb, const char *fmt, ...);
+__attribute__((format (printf,2,0)))
+extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);
 
 extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);
 /* XXX: if read fails, any partial read is undone */
diff --git a/trace.c b/trace.c
index 35d388d..eda3f6d 100644
--- a/trace.c
+++ b/trace.c
@@ -64,28 +64,18 @@ static const char err_msg[] = "Could not trace into fd given by "
 
 void trace_printf(const char *fmt, ...)
 {
-	struct strbuf buf;
+	struct strbuf buf = STRBUF_INIT;
 	va_list ap;
-	int fd, len, need_close = 0;
+	int fd, need_close = 0;
 
 	fd = get_trace_fd(&need_close);
 	if (!fd)
 		return;
 
 	set_try_to_free_routine(NULL);	/* is never reset */
-	strbuf_init(&buf, 64);
 	va_start(ap, fmt);
-	len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
+	strbuf_vaddf(&buf, fmt, ap);
 	va_end(ap);
-	if (len >= strbuf_avail(&buf)) {
-		strbuf_grow(&buf, len - strbuf_avail(&buf) + 128);
-		va_start(ap, fmt);
-		len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
-		va_end(ap);
-		if (len >= strbuf_avail(&buf))
-			die("broken vsnprintf");
-	}
-	strbuf_setlen(&buf, len);
 
 	write_or_whine_pipe(fd, buf.buf, buf.len, err_msg);
 	strbuf_release(&buf);
@@ -96,28 +86,18 @@ void trace_printf(const char *fmt, ...)
 
 void trace_argv_printf(const char **argv, const char *fmt, ...)
 {
-	struct strbuf buf;
+	struct strbuf buf = STRBUF_INIT;
 	va_list ap;
-	int fd, len, need_close = 0;
+	int fd, need_close = 0;
 
 	fd = get_trace_fd(&need_close);
 	if (!fd)
 		return;
 
 	set_try_to_free_routine(NULL);	/* is never reset */
-	strbuf_init(&buf, 64);
 	va_start(ap, fmt);
-	len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
+	strbuf_vaddf(&buf, fmt, ap);
 	va_end(ap);
-	if (len >= strbuf_avail(&buf)) {
-		strbuf_grow(&buf, len - strbuf_avail(&buf) + 128);
-		va_start(ap, fmt);
-		len = vsnprintf(buf.buf, strbuf_avail(&buf), fmt, ap);
-		va_end(ap);
-		if (len >= strbuf_avail(&buf))
-			die("broken vsnprintf");
-	}
-	strbuf_setlen(&buf, len);
 
 	sq_quote_argv(&buf, argv, 0);
 	strbuf_addch(&buf, '\n');
-- 
1.7.4.1

[PATCH 3/5] wt-status: add helpers for printing wt-status lines

[Jonathan Nieder]

Introduce status_printf{,_ln,_more} wrapper functions around
color_vfprintf() which take care of adding "#" to the beginning of
status lines automatically.  The semantics:

 - status_printf() is just like color_fprintf() but it adds a "# "
   at the beginning of each line of output;

 - status_printf_ln() is a convenience function that additionally
   adds "\n" at the end;

 - status_printf_more() is a variant of status_printf() used to
   continue lines that have already started.  It suppresses the "#" at
   the beginning of the first line.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 color.c     |    9 +++++++
 color.h     |    3 ++
 wt-status.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 wt-status.h |    7 +++++
 4 files changed, 93 insertions(+), 0 deletions(-)

diff --git a/color.c b/color.c
index 6a5a54e..417cf8f 100644
--- a/color.c
+++ b/color.c
@@ -175,6 +175,15 @@ int git_color_default_config(const char *var, const char *value, void *cb)
 	return git_default_config(var, value, cb);
 }
 
+void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb)
+{
+	if (*color)
+		fprintf(fp, "%s", color);
+	fprintf(fp, "%s", sb->buf);
+	if (*color)
+		fprintf(fp, "%s", GIT_COLOR_RESET);
+}
+
 static int color_vfprintf(FILE *fp, const char *color, const char *fmt,
 		va_list args, const char *trail)
 {
diff --git a/color.h b/color.h
index 170ff40..c0528cf 100644
--- a/color.h
+++ b/color.h
@@ -1,6 +1,8 @@
 #ifndef COLOR_H
 #define COLOR_H
 
+struct strbuf;
+
 /*  2 + (2 * num_attrs) + 8 + 1 + 8 + 'm' + NUL */
 /* "\033[1;2;4;5;7;38;5;2xx;48;5;2xxm\0" */
 /*
@@ -64,6 +66,7 @@ __attribute__((format (printf, 3, 4)))
 int color_fprintf(FILE *fp, const char *color, const char *fmt, ...);
 __attribute__((format (printf, 3, 4)))
 int color_fprintf_ln(FILE *fp, const char *color, const char *fmt, ...);
+void color_print_strbuf(FILE *fp, const char *color, const struct strbuf *sb);
 
 int color_is_nil(const char *color);
 
diff --git a/wt-status.c b/wt-status.c
index 123582b..1abd7c3 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -32,6 +32,80 @@ static const char *color(int slot, struct wt_status *s)
 	return c;
 }
 
+static void status_vprintf(struct wt_status *s, int at_bol, const char *color,
+		const char *fmt, va_list ap, const char *trail)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf linebuf = STRBUF_INIT;
+	const char *line, *eol;
+
+	strbuf_vaddf(&sb, fmt, ap);
+	if (!sb.len) {
+		strbuf_addch(&sb, '#');
+		if (!trail)
+			strbuf_addch(&sb, ' ');
+		color_print_strbuf(s->fp, color, &sb);
+		if (trail)
+			fprintf(s->fp, "%s", trail);
+		strbuf_release(&sb);
+		return;
+	}
+	for (line = sb.buf; *line; line = eol + 1) {
+		eol = strchr(line, '\n');
+
+		strbuf_reset(&linebuf);
+		if (at_bol) {
+			strbuf_addch(&linebuf, '#');
+			if (*line != '\n' && *line != '\t')
+				strbuf_addch(&linebuf, ' ');
+		}
+		if (eol)
+			strbuf_add(&linebuf, line, eol - line);
+		else
+			strbuf_addstr(&linebuf, line);
+		color_print_strbuf(s->fp, color, &linebuf);
+		if (eol)
+			fprintf(s->fp, "\n");
+		else
+			break;
+		at_bol = 1;
+	}
+	if (trail)
+		fprintf(s->fp, "%s", trail);
+	strbuf_release(&linebuf);
+	strbuf_release(&sb);
+}
+
+void status_printf_ln(struct wt_status *s, const char *color,
+			const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	status_vprintf(s, 1, color, fmt, ap, "\n");
+	va_end(ap);
+}
+
+void status_printf(struct wt_status *s, const char *color,
+			const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	status_vprintf(s, 1, color, fmt, ap, NULL);
+	va_end(ap);
+}
+
+void status_printf_more(struct wt_status *s, const char *color,
+			const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	status_vprintf(s, 0, color, fmt, ap, NULL);
+	va_end(ap);
+}
+
 void wt_status_prepare(struct wt_status *s)
 {
 	unsigned char sha1[20];
diff --git a/wt-status.h b/wt-status.h
index 20b17cf..204a519 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -68,4 +68,11 @@ void wt_status_collect(struct wt_status *s);
 void wt_shortstatus_print(struct wt_status *s, int null_termination, int show_branch);
 void wt_porcelain_print(struct wt_status *s, int null_termination);
 
+void status_printf_ln(struct wt_status *s, const char *color, const char *fmt, ...)
+	__attribute__((format(printf, 3, 4)));
+void status_printf(struct wt_status *s, const char *color, const char *fmt, ...)
+	__attribute__((format(printf, 3, 4)));
+void status_printf_more(struct wt_status *s, const char *color, const char *fmt, ...)
+	__attribute__((format(printf, 3, 4)));
+
 #endif /* STATUS_H */
-- 
1.7.4.1

[PATCH 4/5] commit: refer to commit template as s->fp

[Jonathan Nieder]

Instead of maintaining a local variable for it, use s->fp to keep
track of where the commit message template should be written.

This prepares us to take advantage of the status_printf functions,
which use a struct wt_status instead of a FILE pointer to determine
where to send their output.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/commit.c |   27 +++++++++++++--------------
 1 files changed, 13 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d7f55e3..ef5f0b2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -568,7 +568,6 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	int commitable, saved_color_setting;
 	struct strbuf sb = STRBUF_INIT;
 	char *buffer;
-	FILE *fp;
 	const char *hook_arg1 = NULL;
 	const char *hook_arg2 = NULL;
 	int ident_shown = 0;
@@ -657,8 +656,8 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		hook_arg2 = "";
 	}
 
-	fp = fopen(git_path(commit_editmsg), "w");
-	if (fp == NULL)
+	s->fp = fopen(git_path(commit_editmsg), "w");
+	if (s->fp == NULL)
 		die_errno("could not open '%s'", git_path(commit_editmsg));
 
 	if (cleanup_mode != CLEANUP_NONE)
@@ -682,7 +681,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		strbuf_release(&sob);
 	}
 
-	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
+	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno("could not write commit template");
 
 	strbuf_release(&sb);
@@ -695,7 +694,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (use_editor && include_status) {
 		char *ai_tmp, *ci_tmp;
 		if (in_merge)
-			fprintf(fp,
+			fprintf(s->fp,
 				"#\n"
 				"# It looks like you may be committing a MERGE.\n"
 				"# If this is not correct, please remove the file\n"
@@ -704,45 +703,45 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 				"#\n",
 				git_path("MERGE_HEAD"));
 
-		fprintf(fp,
+		fprintf(s->fp,
 			"\n"
 			"# Please enter the commit message for your changes.");
 		if (cleanup_mode == CLEANUP_ALL)
-			fprintf(fp,
+			fprintf(s->fp,
 				" Lines starting\n"
 				"# with '#' will be ignored, and an empty"
 				" message aborts the commit.\n");
 		else /* CLEANUP_SPACE, that is. */
-			fprintf(fp,
+			fprintf(s->fp,
 				" Lines starting\n"
 				"# with '#' will be kept; you may remove them"
 				" yourself if you want to.\n"
 				"# An empty message aborts the commit.\n");
 		if (only_include_assumed)
-			fprintf(fp, "# %s\n", only_include_assumed);
+			fprintf(s->fp, "# %s\n", only_include_assumed);
 
 		ai_tmp = cut_ident_timestamp_part(author_ident->buf);
 		ci_tmp = cut_ident_timestamp_part(committer_ident.buf);
 		if (strcmp(author_ident->buf, committer_ident.buf))
-			fprintf(fp,
+			fprintf(s->fp,
 				"%s"
 				"# Author:    %s\n",
 				ident_shown++ ? "" : "#\n",
 				author_ident->buf);
 
 		if (!user_ident_sufficiently_given())
-			fprintf(fp,
+			fprintf(s->fp,
 				"%s"
 				"# Committer: %s\n",
 				ident_shown++ ? "" : "#\n",
 				committer_ident.buf);
 
 		if (ident_shown)
-			fprintf(fp, "#\n");
+			fprintf(s->fp, "#\n");
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
-		commitable = run_status(fp, index_file, prefix, 1, s);
+		commitable = run_status(s->fp, index_file, prefix, 1, s);
 		s->use_color = saved_color_setting;
 
 		*ai_tmp = ' ';
@@ -764,7 +763,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	}
 	strbuf_release(&committer_ident);
 
-	fclose(fp);
+	fclose(s->fp);
 
 	if (!commitable && !in_merge && !allow_empty &&
 	    !(amend && is_a_merge(head_sha1))) {
-- 
1.7.4.1

[PATCH 5/5] commit, status: use status_printf{,_ln,_more} helpers

[Jonathan Nieder]

wt-status code is used to provide a reminder of changes included and
not included for the commit message template opened in the operator's
text editor by "git commit".  Therefore each line of its output begins
with the comment character "#":

	# Please enter the commit message for your changes. Lines starting

Use the new status_printf{,_ln,_more} functions to take care of adding
"#" to the beginning of such status lines automatically.  Using these
will have two advantages over the current code:

 - The obvious one is to force separation of the "#" from the
   translatable part of the message when git learns to translate its
   output.

 - Another advantage is that this makes it easier for us to drop "#"
   prefix in "git status" output in later versions of git if we want
   to.

Explained-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Thanks for reading.

 builtin/commit.c |   47 +++++++++++++++--------------
 wt-status.c      |   86 +++++++++++++++++++++++++++---------------------------
 2 files changed, 67 insertions(+), 66 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index ef5f0b2..ae62a25 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -694,50 +694,51 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 	if (use_editor && include_status) {
 		char *ai_tmp, *ci_tmp;
 		if (in_merge)
-			fprintf(s->fp,
-				"#\n"
-				"# It looks like you may be committing a MERGE.\n"
-				"# If this is not correct, please remove the file\n"
-				"#	%s\n"
-				"# and try again.\n"
-				"#\n",
+			status_printf_ln(s, GIT_COLOR_NORMAL,
+				"\n"
+				"It looks like you may be committing a MERGE.\n"
+				"If this is not correct, please remove the file\n"
+				"	%s\n"
+				"and try again.\n"
+				"",
 				git_path("MERGE_HEAD"));
 
-		fprintf(s->fp,
-			"\n"
-			"# Please enter the commit message for your changes.");
+		fprintf(s->fp, "\n");
+		status_printf(s, GIT_COLOR_NORMAL,
+			"Please enter the commit message for your changes.");
 		if (cleanup_mode == CLEANUP_ALL)
-			fprintf(s->fp,
+			status_printf_more(s, GIT_COLOR_NORMAL,
 				" Lines starting\n"
-				"# with '#' will be ignored, and an empty"
+				"with '#' will be ignored, and an empty"
 				" message aborts the commit.\n");
 		else /* CLEANUP_SPACE, that is. */
-			fprintf(s->fp,
+			status_printf_more(s, GIT_COLOR_NORMAL,
 				" Lines starting\n"
-				"# with '#' will be kept; you may remove them"
+				"with '#' will be kept; you may remove them"
 				" yourself if you want to.\n"
-				"# An empty message aborts the commit.\n");
+				"An empty message aborts the commit.\n");
 		if (only_include_assumed)
-			fprintf(s->fp, "# %s\n", only_include_assumed);
+			status_printf_ln(s, GIT_COLOR_NORMAL,
+					"%s", only_include_assumed);
 
 		ai_tmp = cut_ident_timestamp_part(author_ident->buf);
 		ci_tmp = cut_ident_timestamp_part(committer_ident.buf);
 		if (strcmp(author_ident->buf, committer_ident.buf))
-			fprintf(s->fp,
+			status_printf_ln(s, GIT_COLOR_NORMAL,
 				"%s"
-				"# Author:    %s\n",
-				ident_shown++ ? "" : "#\n",
+				"Author:    %s",
+				ident_shown++ ? "" : "\n",
 				author_ident->buf);
 
 		if (!user_ident_sufficiently_given())
-			fprintf(s->fp,
+			status_printf_ln(s, GIT_COLOR_NORMAL,
 				"%s"
-				"# Committer: %s\n",
-				ident_shown++ ? "" : "#\n",
+				"Committer: %s",
+				ident_shown++ ? "" : "\n",
 				committer_ident.buf);
 
 		if (ident_shown)
-			fprintf(s->fp, "#\n");
+			status_printf_ln(s, GIT_COLOR_NORMAL, "");
 
 		saved_color_setting = s->use_color;
 		s->use_color = 0;
diff --git a/wt-status.c b/wt-status.c
index 1abd7c3..c14cbe4 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -131,33 +131,33 @@ static void wt_status_print_unmerged_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
-	color_fprintf_ln(s->fp, c, "# Unmerged paths:");
+	status_printf_ln(s, c, "Unmerged paths:");
 	if (!advice_status_hints)
 		return;
 	if (s->in_merge)
 		;
 	else if (!s->is_initial)
-		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
+		status_printf_ln(s, c, "  (use \"git reset %s <file>...\" to unstage)", s->reference);
 	else
-		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
-	color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" as appropriate to mark resolution)");
-	color_fprintf_ln(s->fp, c, "#");
+		status_printf_ln(s, c, "  (use \"git rm --cached <file>...\" to unstage)");
+	status_printf_ln(s, c, "  (use \"git add/rm <file>...\" as appropriate to mark resolution)");
+	status_printf_ln(s, c, "");
 }
 
 static void wt_status_print_cached_header(struct wt_status *s)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
-	color_fprintf_ln(s->fp, c, "# Changes to be committed:");
+	status_printf_ln(s, c, "Changes to be committed:");
 	if (!advice_status_hints)
 		return;
 	if (s->in_merge)
 		; /* NEEDSWORK: use "git reset --unresolve"??? */
 	else if (!s->is_initial)
-		color_fprintf_ln(s->fp, c, "#   (use \"git reset %s <file>...\" to unstage)", s->reference);
+		status_printf_ln(s, c, "  (use \"git reset %s <file>...\" to unstage)", s->reference);
 	else
-		color_fprintf_ln(s->fp, c, "#   (use \"git rm --cached <file>...\" to unstage)");
-	color_fprintf_ln(s->fp, c, "#");
+		status_printf_ln(s, c, "  (use \"git rm --cached <file>...\" to unstage)");
+	status_printf_ln(s, c, "");
 }
 
 static void wt_status_print_dirty_header(struct wt_status *s,
@@ -166,17 +166,17 @@ static void wt_status_print_dirty_header(struct wt_status *s,
 {
 	const char *c = color(WT_STATUS_HEADER, s);
 
-	color_fprintf_ln(s->fp, c, "# Changes not staged for commit:");
+	status_printf_ln(s, c, "Changes not staged for commit:");
 	if (!advice_status_hints)
 		return;
 	if (!has_deleted)
-		color_fprintf_ln(s->fp, c, "#   (use \"git add <file>...\" to update what will be committed)");
+		status_printf_ln(s, c, "  (use \"git add <file>...\" to update what will be committed)");
 	else
-		color_fprintf_ln(s->fp, c, "#   (use \"git add/rm <file>...\" to update what will be committed)");
-	color_fprintf_ln(s->fp, c, "#   (use \"git checkout -- <file>...\" to discard changes in working directory)");
+		status_printf_ln(s, c, "  (use \"git add/rm <file>...\" to update what will be committed)");
+	status_printf_ln(s, c, "  (use \"git checkout -- <file>...\" to discard changes in working directory)");
 	if (has_dirty_submodules)
-		color_fprintf_ln(s->fp, c, "#   (commit or discard the untracked or modified content in submodules)");
-	color_fprintf_ln(s->fp, c, "#");
+		status_printf_ln(s, c, "  (commit or discard the untracked or modified content in submodules)");
+	status_printf_ln(s, c, "");
 }
 
 static void wt_status_print_other_header(struct wt_status *s,
@@ -184,16 +184,16 @@ static void wt_status_print_other_header(struct wt_status *s,
 					 const char *how)
 {
 	const char *c = color(WT_STATUS_HEADER, s);
-	color_fprintf_ln(s->fp, c, "# %s files:", what);
+	status_printf_ln(s, c, "%s files:", what);
 	if (!advice_status_hints)
 		return;
-	color_fprintf_ln(s->fp, c, "#   (use \"git %s <file>...\" to include in what will be committed)", how);
-	color_fprintf_ln(s->fp, c, "#");
+	status_printf_ln(s, c, "  (use \"git %s <file>...\" to include in what will be committed)", how);
+	status_printf_ln(s, c, "");
 }
 
 static void wt_status_print_trailer(struct wt_status *s)
 {
-	color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "#");
+	status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
 }
 
 #define quote_path quote_path_relative
@@ -207,7 +207,7 @@ static void wt_status_print_unmerged_data(struct wt_status *s,
 	const char *one, *how = "bug";
 
 	one = quote_path(it->string, -1, &onebuf, s->prefix);
-	color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "#\t");
+	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	switch (d->stagemask) {
 	case 1: how = "both deleted:"; break;
 	case 2: how = "added by us:"; break;
@@ -217,7 +217,7 @@ static void wt_status_print_unmerged_data(struct wt_status *s,
 	case 6: how = "both added:"; break;
 	case 7: how = "both modified:"; break;
 	}
-	color_fprintf(s->fp, c, "%-20s%s\n", how, one);
+	status_printf_more(s, c, "%-20s%s\n", how, one);
 	strbuf_release(&onebuf);
 }
 
@@ -260,40 +260,40 @@ static void wt_status_print_change_data(struct wt_status *s,
 	one = quote_path(one_name, -1, &onebuf, s->prefix);
 	two = quote_path(two_name, -1, &twobuf, s->prefix);
 
-	color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "#\t");
+	status_printf(s, color(WT_STATUS_HEADER, s), "\t");
 	switch (status) {
 	case DIFF_STATUS_ADDED:
-		color_fprintf(s->fp, c, "new file:   %s", one);
+		status_printf_more(s, c, "new file:   %s", one);
 		break;
 	case DIFF_STATUS_COPIED:
-		color_fprintf(s->fp, c, "copied:     %s -> %s", one, two);
+		status_printf_more(s, c, "copied:     %s -> %s", one, two);
 		break;
 	case DIFF_STATUS_DELETED:
-		color_fprintf(s->fp, c, "deleted:    %s", one);
+		status_printf_more(s, c, "deleted:    %s", one);
 		break;
 	case DIFF_STATUS_MODIFIED:
-		color_fprintf(s->fp, c, "modified:   %s", one);
+		status_printf_more(s, c, "modified:   %s", one);
 		break;
 	case DIFF_STATUS_RENAMED:
-		color_fprintf(s->fp, c, "renamed:    %s -> %s", one, two);
+		status_printf_more(s, c, "renamed:    %s -> %s", one, two);
 		break;
 	case DIFF_STATUS_TYPE_CHANGED:
-		color_fprintf(s->fp, c, "typechange: %s", one);
+		status_printf_more(s, c, "typechange: %s", one);
 		break;
 	case DIFF_STATUS_UNKNOWN:
-		color_fprintf(s->fp, c, "unknown:    %s", one);
+		status_printf_more(s, c, "unknown:    %s", one);
 		break;
 	case DIFF_STATUS_UNMERGED:
-		color_fprintf(s->fp, c, "unmerged:   %s", one);
+		status_printf_more(s, c, "unmerged:   %s", one);
 		break;
 	default:
 		die("bug: unhandled diff status %c", status);
 	}
 	if (extra.len) {
-		color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "%s", extra.buf);
+		status_printf_more(s, color(WT_STATUS_HEADER, s), "%s", extra.buf);
 		strbuf_release(&extra);
 	}
-	fprintf(s->fp, "\n");
+	status_printf_more(s, GIT_COLOR_NORMAL, "\n");
 	strbuf_release(&onebuf);
 	strbuf_release(&twobuf);
 }
@@ -647,9 +647,9 @@ static void wt_status_print_other(struct wt_status *s,
 	for (i = 0; i < l->nr; i++) {
 		struct string_list_item *it;
 		it = &(l->items[i]);
-		color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "#\t");
-		color_fprintf_ln(s->fp, color(WT_STATUS_UNTRACKED, s), "%s",
-				 quote_path(it->string, strlen(it->string),
+		status_printf(s, color(WT_STATUS_HEADER, s), "\t");
+		status_printf_more(s, color(WT_STATUS_UNTRACKED, s),
+			"%s\n", quote_path(it->string, strlen(it->string),
 					    &buf, s->prefix));
 	}
 	strbuf_release(&buf);
@@ -716,17 +716,17 @@ void wt_status_print(struct wt_status *s)
 			branch_status_color = color(WT_STATUS_NOBRANCH, s);
 			on_what = "Not currently on any branch.";
 		}
-		color_fprintf(s->fp, color(WT_STATUS_HEADER, s), "# ");
-		color_fprintf(s->fp, branch_status_color, "%s", on_what);
-		color_fprintf_ln(s->fp, branch_color, "%s", branch_name);
+		status_printf(s, color(WT_STATUS_HEADER, s), "");
+		status_printf_more(s, branch_status_color, "%s", on_what);
+		status_printf_more(s, branch_color, "%s\n", branch_name);
 		if (!s->is_initial)
 			wt_status_print_tracking(s);
 	}
 
 	if (s->is_initial) {
-		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "#");
-		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "# Initial commit");
-		color_fprintf_ln(s->fp, color(WT_STATUS_HEADER, s), "#");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "Initial commit");
+		status_printf_ln(s, color(WT_STATUS_HEADER, s), "");
 	}
 
 	wt_status_print_updated(s);
@@ -743,7 +743,7 @@ void wt_status_print(struct wt_status *s)
 		if (s->show_ignored_files)
 			wt_status_print_other(s, &s->ignored, "Ignored", "add -f");
 	} else if (s->commitable)
-		fprintf(s->fp, "# Untracked files not listed%s\n",
+		status_printf_ln(s, GIT_COLOR_NORMAL, "Untracked files not listed%s",
 			advice_status_hints
 			? " (use -u option to show untracked files)" : "");
 
@@ -751,7 +751,7 @@ void wt_status_print(struct wt_status *s)
 		wt_status_print_verbose(s);
 	if (!s->commitable) {
 		if (s->amend)
-			fprintf(s->fp, "# No changes\n");
+			status_printf_ln(s, GIT_COLOR_NORMAL, "No changes");
 		else if (s->nowarn)
 			; /* nothing */
 		else if (s->workdir_dirty)
-- 
1.7.4.1

Re: [PATCH 5/5] commit, status: use status_printf{,_ln,_more} helpers

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index ef5f0b2..ae62a25 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -694,50 +694,51 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  	if (use_editor && include_status) {
>  		char *ai_tmp, *ci_tmp;
>  		if (in_merge)
> -...
> +			status_printf_ln(s, GIT_COLOR_NORMAL,
> +				"\n"
> +				"It looks like you may be committing a MERGE.\n"
> +				"If this is not correct, please remove the file\n"
> +				"	%s\n"
> +				"and try again.\n"
> +				"",

This trick to avoid difference-in-comma-when-updated was semi "Huh" when
reading.  Is it worth it?

> ...
>  		if (ident_shown)
> -			fprintf(s->fp, "#\n");
> +			status_printf_ln(s, GIT_COLOR_NORMAL, "");

The "attribute((format))" safety seems to bite us here...

    $ Meta/Make --pedantic builtin/commit.o
        CC builtin/commit.o
    cc1: warnings being treated as errors
    builtin/commit.c: In function 'prepare_to_commit':
    builtin/commit.c:741: error: zero-length printf format string

In wt-status.c many similar ones exist, unfortunately.

Re: [PATCH 5/5] commit, status: use status_printf{,_ln,_more} helpers

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>> +++ b/builtin/commit.c
>> @@ -694,50 +694,51 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>>  	if (use_editor && include_status) {
>>  		char *ai_tmp, *ci_tmp;
>>  		if (in_merge)
>> -...
>> +			status_printf_ln(s, GIT_COLOR_NORMAL,
>> +				"\n"
>> +				"It looks like you may be committing a MERGE.\n"
>> +				"If this is not correct, please remove the file\n"
>> +				"	%s\n"
>> +				"and try again.\n"
>> +				"",
>
> This trick to avoid difference-in-comma-when-updated was semi "Huh" when
> reading.  Is it worth it?

The hint starts and ends with a blank (commented) line.

			status_printf(s, GIT_COLOR_NORMAL,
				"\n"
				...
				"and try again.\n"
				"\n",

would be clearer.

> The "attribute((format))" safety seems to bite us here...
>
>     $ Meta/Make --pedantic builtin/commit.o
>         CC builtin/commit.o
>     cc1: warnings being treated as errors
>     builtin/commit.c: In function 'prepare_to_commit':
>     builtin/commit.c:741: error: zero-length printf format string
>
> In wt-status.c many similar ones exist, unfortunately.

What a warning. :)  I don't understand how it is supposed to benefit
the world[1].

	status_printf_ln(s, GIT_COLOR_NORMAL, "%s", "");
	status_printf(s, GIT_COLOR_NORMAL, "\n");

I suppose the latter could be a bearable workaround.

[1] http://gcc.gnu.org/ml/gcc-patches/2002-05/msg01462.html