Re: [PATCH v3] commit -v: strip diffs and submodule shortlogs from the commit message

Jens Lehmann [off-list ref] writes:

Changes since v2:

- Honor the core.commentChar setting and add a test for that.

- Fix the submodule test to set the editor in a portable way.

- Only print scissor and description lines when not going to stdout
  (otherwise a "git status -v" prints that on stdout too, which
  doesn't make much sense). Now we definitely do not have to care
  about coloring these lines either.

Hmph.  It makes me suspect that we were drunk when we decided to let
"git status -v" to keep showing diff when we declared that "git
status" is different from "git commit --dry-run", but it is too late
to fix it now, I think.

- Nicer formatting of the commit message.

Certainly a lot easier to read, at least to me.

I wonder if this comment still is valid, but it probably is OK.

-	if (verbose) {
-		p = strstr(sb.buf, "\ndiff --git ");
-		if (p != NULL)
-			strbuf_setlen(&sb, p - sb.buf + 1);
-	}
+	if (verbose)
+		wt_status_truncate_message_at_cut_line(&sb);

Perhaps it may happen that all the current callers have called
wt_status_print_verbose() to cause wt_status_cut_line[0] to hold
comment_line_char, but relying on that calling sequence somehow
makes me feel uneasy.

Perhaps cut_line[] should only have "--- >8 ---" part and both
printing side (below) and finding side (this one) should check these
separately?  That is:

	p = buf->buf;
	while (p && *p) {
		p = strchr(p, comment_line_char);
                if (!p)
			break;
		if (strstr(p + 1, cut_line) == p + 1)
			break;
		p++;
                continue;
	}
        if (p && *p && (p == buf->buf || p[-1] == '\n'))
		strbuf_setlen(buf, p - buf->buf);

or something (the above is deliberately less-efficient-than-ideal,
because I want to keep the code structure in such a way that we can
later turn comment_line_char to a string[] that can hold "//" to
allow a multi-char comment introducer more easily)?

I didn't bother with my "how about this" version, but we may want to
use strbuf_add_commented_lines() to help i18n/l10n folks.  Depending
on the l10n, this message may want to become more or less than 2
lines.

Am 21.11.2013 00:04, schrieb Junio C Hamano:

Jens Lehmann [off-list ref] writes:

quoted

diff --git a/wt-status.c b/wt-status.c
index b4e44ba..734f94b 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -16,6 +16,9 @@
 #include "column.h"
 #include "strbuf.h"

+static char wt_status_cut_line[] = /* 'X' is replaced with comment_line_char */
+"X ------------------------ >8 ------------------------\n";
+
 static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
 	GIT_COLOR_GREEN,  /* WT_STATUS_UPDATED */

@@ -767,6 +770,15 @@ conclude:
 	status_printf_ln(s, GIT_COLOR_NORMAL, "");
 }

+void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
+{
+	const char *p;
+
+	p = strstr(buf->buf, wt_status_cut_line);
+	if (p && (p == buf->buf || p[-1] == '\n'))
+		strbuf_setlen(buf, p - buf->buf);
+}

Perhaps it may happen that all the current callers have called
wt_status_print_verbose() to cause wt_status_cut_line[0] to hold
comment_line_char, but relying on that calling sequence somehow
makes me feel uneasy.

I initialized the place to be occupied by the comment_line_char
in wt_status_cut_line with 'X' on purpose to notice such a
problem. But I'd be also fine with setting wt_status_cut_line[0]
again here just to be sure. But please also see below ...

Perhaps cut_line[] should only have "--- >8 ---" part and both
printing side (below) and finding side (this one) should check these
separately?

... ok ...

That is:

	p = buf->buf;
	while (p && *p) {
		p = strchr(p, comment_line_char);
                if (!p)
			break;
		if (strstr(p + 1, cut_line) == p + 1)
			break;
		p++;
                continue;
	}
        if (p && *p && (p == buf->buf || p[-1] == '\n'))
		strbuf_setlen(buf, p - buf->buf);

or something (the above is deliberately less-efficient-than-ideal,
because I want to keep the code structure in such a way that we can
later turn comment_line_char to a string[] that can hold "//" to
allow a multi-char comment introducer more easily)?

Hmm, I'm a bit reluctant to go that far to optimize this patch for
another one that might materialize later. But what about this:

	struct strbuf cut_line = STRBUF_INIT;
	strbuf_addf(&cut_line, "%c %s", comment_line_char, wt_status_cut_line);
	p = strstr(buf->buf, cut_line.buf);
	if (p && (p == buf->buf || p[-1] == '\n'))
		strbuf_setlen(buf, p - buf->buf);
	strbuf_release(&cut_line);

That is shorter can easily be adapted to a comment line string later.
And even though it's slightly less performant should not be a problem
here as this happens only once after invoking an editor for user input.

quoted

 static void wt_status_print_verbose(struct wt_status *s)
 {
 	struct rev_info rev;

@@ -787,10 +799,17 @@ static void wt_status_print_verbose(struct wt_status *s)
 	 * If we're not going to stdout, then we definitely don't
 	 * want color, since we are going to the commit message
 	 * file (and even the "auto" setting won't work, since it
-	 * will have checked isatty on stdout).
+	 * will have checked isatty on stdout). But we then do want
+	 * to insert the scissor line here to reliably remove the
+	 * diff before committing.
 	 */
-	if (s->fp != stdout)
+	if (s->fp != stdout) {
 		rev.diffopt.use_color = 0;
+		wt_status_cut_line[0] = comment_line_char;
+		fprintf(s->fp, wt_status_cut_line);
+		fprintf(s->fp, _("%c Do not touch the line above.\n"), comment_line_char);
+		fprintf(s->fp, _("%c Everything below will be removed.\n"), comment_line_char);
+	}

I didn't bother with my "how about this" version, but we may want to
use strbuf_add_commented_lines() to help i18n/l10n folks.  Depending
on the l10n, this message may want to become more or less than 2
lines.

Makes sense, will change that (maybe using strbuf_commented_addf()
instead) for v4.