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

From: Jens Lehmann <jens.lehmann@web.de>
Date: Nov 16, 2013, 22:52 UTC
Message-ID: <5287F735.3030306@web.de>
In-Reply-To: <xmqq7gccdq67.fsf@gitster.dls.corp.google.com>

When using the '-v' option of "git commit" the diff added to the commit message temporarily for editing is stripped off after the user exited the editor by searching for "\ndiff --git " and truncating the commmit message there if it is found. But this approach has two problems: when the commit message itself contains a line starting with "diff --git" it will be truncated there prematurely. And when the "diff.submodule" setting is set to "log", the diff may start with "Submodule <hash1>..<hash2>", which will be left in the commit message while it shouldn't.

Fix that by introducing a special scissor separator line starting with the comment character '#' followed by a line describing what it is for. The scissor line is used to reliably detect the start of the diff so it can be chopped off from the commit message, no matter what the user enters there. Turn a known test failure fixed by this change into a successful test and add another one for a diff starting with a submodule log.

Reported-by: Ari Pollak <ari@debian.org>
Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
---

Am 13.11.2013 21:04, schrieb Junio C Hamano:

Show 18 quoted lines

> Jens Lehmann <Jens.Lehmann@web.de> writes:
> 
>>> If we were introducing a divider line for machine consumption, I do
>>> not think it is wise to let that line even translated...
>>
>> Ok, but then it won't mean much to readers who don't understand
>> English. I assume prefixing all diff lines with "# " is out of
>> the question because of backwards compatibility, so what about
>> using a descriptive text together with a scissor line? The former
>> can be be translated (and won't make it into the commit message
>> because it starts with a "#") while the latter serves as a robust
>> divider line:
>>
>> # Everything below the following line is a diff that will be removed.
>> # --------------------------------8<--------------------------------
> 
> Yeah, or swap them around if you are trying to protect the part
> above the divider from getting contaminated by the noise.

Ok, did that. I couldn't find another user of the verbose setting, so emitting the scissor line together with the description inside wt_status_print_verbose() should be ok (or did I miss something here?). But currently these lines use the hardcoded '#' instead of the status.displaycommentprefix configuration and ignores coloring, looks like I need to fix this in the next iteration.

 builtin/commit.c          |  6 +++---
 t/t7507-commit-verbose.sh | 15 ++++++++++++++-
 wt-status.c               |  4 ++++
 wt-status.h               |  2 ++
 4 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..091a6e7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1602,9 +1602,9 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

 	/* Truncate the message just before the diff, if any. */
 	if (verbose) {
-		p = strstr(sb.buf, "\ndiff --git ");
-		if (p != NULL)
-			strbuf_setlen(&sb, p - sb.buf + 1);
+		p = strstr(sb.buf, wt_status_diff_divider);
+		if ((p != NULL) && (p > sb.buf) && (p[-1] == '\n'))
+			strbuf_setlen(&sb, p - sb.buf);
 	}

 	if (cleanup_mode != CLEANUP_NONE)
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index da5bd3b..09c1150 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -65,9 +65,22 @@ test_expect_success 'diff in message is retained without -v' '
 	check_message diff
 '

-test_expect_failure 'diff in message is retained with -v' '
+test_expect_success 'diff in message is retained with -v' '
 	git commit --amend -F diff -v &&
 	check_message diff
 '

+test_expect_success 'submodule log is stripped out too with -v' '
+	git config diff.submodule log &&
+	git submodule add ./. sub &&
+	git commit -m "sub added" &&
+	(
+		cd sub &&
+		echo "more" >>file &&
+		git commit -a -m "submodule commit"
+	) &&
+	GIT_EDITOR=cat test_must_fail git commit -a -v 2>err &&
+	test_i18ngrep "Aborting commit due to empty commit message." err
+'
+
 test_done
diff --git a/wt-status.c b/wt-status.c
index b4e44ba..a3f7115 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,8 @@
 #include "column.h"
 #include "strbuf.h"

+const char wt_status_diff_divider[] = "# -----------------------------------8<-----------------------------------\n";
+
 static char default_wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
 	GIT_COLOR_GREEN,  /* WT_STATUS_UPDATED */
@@ -791,6 +793,8 @@ static void wt_status_print_verbose(struct wt_status *s)
 	 */
 	if (s->fp != stdout)
 		rev.diffopt.use_color = 0;
+	fprintf(s->fp, wt_status_diff_divider);
+	fprintf(s->fp, _("# The diff below will be removed when keeping the previous line.\n"));
 	run_diff_index(&rev, 1);
 }

diff --git a/wt-status.h b/wt-status.h
index 6c29e6f..cd2709f 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -91,6 +91,8 @@ struct wt_status_state {
 	unsigned char cherry_pick_head_sha1[20];
 };

+const char wt_status_diff_divider[];
+
 void wt_status_prepare(struct wt_status *s);
 void wt_status_print(struct wt_status *s);
 void wt_status_collect(struct wt_status *s);

-- 
1.8.5.rc2.1.g537c6ea