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

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

[Jens Lehmann]

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 ('#' or the core.commentChar config if set) followed by
two lines describing what it is for. The scissor line - which will not be
translated - 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;
also add one for a diff starting with a submodule log and another one for
proper handling of the comment char.

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


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.

- Nicer formatting of the commit message.


 builtin/commit.c          |  9 +++------
 t/t7507-commit-verbose.sh | 28 +++++++++++++++++++++++++++-
 wt-status.c               | 23 +++++++++++++++++++++--
 wt-status.h               |  1 +
 4 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..fedb45a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1505,7 +1505,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
-	char *nl, *p;
+	char *nl;
 	unsigned char sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
@@ -1601,11 +1601,8 @@ 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);
-	}
+	if (verbose)
+		wt_status_truncate_message_at_cut_line(&sb);

 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index da5bd3b..2ddf28c 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -65,9 +65,35 @@ 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 &&
+		export GIT_EDITOR &&
+		test_must_fail git commit -a -v 2>err
+	) &&
+	test_i18ngrep "Aborting commit due to empty commit message." err
+'
+
+test_expect_success 'verbose diff is stripped out with set core.commentChar' '
+	(
+		GIT_EDITOR=cat &&
+		export GIT_EDITOR &&
+		test_must_fail git -c core.commentchar=";" 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..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);
+}
+
 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);
+	}
 	run_diff_index(&rev, 1);
 }

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

+void wt_status_truncate_message_at_cut_line(struct strbuf *);
 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.7.g9470f78.dirty

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> 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.

> @@ -1601,11 +1601,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	}
>
>  	/* Truncate the message just before the diff, if any. */

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);

> 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.

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)?

>  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.

>  	run_diff_index(&rev, 1);
>  }
>
> diff --git a/wt-status.h b/wt-status.h
> index 6c29e6f..30a4812 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -91,6 +91,7 @@ struct wt_status_state {
>  	unsigned char cherry_pick_head_sha1[20];
>  };
>
> +void wt_status_truncate_message_at_cut_line(struct strbuf *);
>  void wt_status_prepare(struct wt_status *s);
>  void wt_status_print(struct wt_status *s);
>  void wt_status_collect(struct wt_status *s);

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

[Jens Lehmann]

Am 21.11.2013 00:04, schrieb Junio C Hamano:
> Jens Lehmann <Jens.Lehmann@web.de> writes:
>> 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.

>>  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.

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> 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.

Sure, that would work fine.

Thanks.

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

[Jens Lehmann]

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 ('#' or the core.commentChar config if set) followed by
two lines describing what it is for. The scissor line - which will not be
translated - 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;
also add one for a diff starting with a submodule log and another one for
proper handling of the comment char.

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

Changes to v3:

- separating comment_line_char from the cut_line

- using strbuf_add_commented_lines() for the comment

All issues raised should be addressed with this version.


 builtin/commit.c          |  9 +++------
 t/t7507-commit-verbose.sh | 28 +++++++++++++++++++++++++++-
 wt-status.c               | 29 +++++++++++++++++++++++++++--
 wt-status.h               |  1 +
 4 files changed, 58 insertions(+), 9 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 6ab4605..fedb45a 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1505,7 +1505,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
 	struct strbuf sb = STRBUF_INIT;
 	struct strbuf author_ident = STRBUF_INIT;
 	const char *index_file, *reflog_msg;
-	char *nl, *p;
+	char *nl;
 	unsigned char sha1[20];
 	struct ref_lock *ref_lock;
 	struct commit_list *parents = NULL, **pptr = &parents;
@@ -1601,11 +1601,8 @@ 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);
-	}
+	if (verbose)
+		wt_status_truncate_message_at_cut_line(&sb);

 	if (cleanup_mode != CLEANUP_NONE)
 		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
index da5bd3b..2ddf28c 100755
--- a/t/t7507-commit-verbose.sh
+++ b/t/t7507-commit-verbose.sh
@@ -65,9 +65,35 @@ 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 &&
+		export GIT_EDITOR &&
+		test_must_fail git commit -a -v 2>err
+	) &&
+	test_i18ngrep "Aborting commit due to empty commit message." err
+'
+
+test_expect_success 'verbose diff is stripped out with set core.commentChar' '
+	(
+		GIT_EDITOR=cat &&
+		export GIT_EDITOR &&
+		test_must_fail git -c core.commentchar=";" 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..99c3d1c 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -16,6 +16,9 @@
 #include "column.h"
 #include "strbuf.h"

+static char cut_line[] =
+"------------------------ >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,18 @@ conclude:
 	status_printf_ln(s, GIT_COLOR_NORMAL, "");
 }

+void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
+{
+	const char *p;
+	struct strbuf pattern = STRBUF_INIT;
+
+	strbuf_addf(&pattern, "%c %s", comment_line_char, cut_line);
+	p = strstr(buf->buf, pattern.buf);
+	if (p && (p == buf->buf || p[-1] == '\n'))
+		strbuf_setlen(buf, p - buf->buf);
+	strbuf_release(&pattern);
+}
+
 static void wt_status_print_verbose(struct wt_status *s)
 {
 	struct rev_info rev;
@@ -787,10 +802,20 @@ 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) {
+		const char *explanation = _("Do not touch the line above.\nEverything below will be removed.");
+		struct strbuf buf = STRBUF_INIT;
+
 		rev.diffopt.use_color = 0;
+		fprintf(s->fp, "%c %s", comment_line_char, cut_line);
+		strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
+		fprintf(s->fp, buf.buf);
+		strbuf_release(&buf);
+	}
 	run_diff_index(&rev, 1);
 }

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

+void wt_status_truncate_message_at_cut_line(struct strbuf *);
 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.4.g9a09ec7

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> 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 ('#' or the core.commentChar config if set) followed by
> two lines describing what it is for. The scissor line - which will not be
> translated - 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;
> also add one for a diff starting with a submodule log and another one for
> proper handling of the comment char.
>
> Reported-by: Ari Pollak <ari@debian.org>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
>
> Changes to v3:
>
> - separating comment_line_char from the cut_line
>
> - using strbuf_add_commented_lines() for the comment
>
> All issues raised should be addressed with this version.

Nicely done.

Thanks, will replace and queue.

>  builtin/commit.c          |  9 +++------
>  t/t7507-commit-verbose.sh | 28 +++++++++++++++++++++++++++-
>  wt-status.c               | 29 +++++++++++++++++++++++++++--
>  wt-status.h               |  1 +
>  4 files changed, 58 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 6ab4605..fedb45a 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1505,7 +1505,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	struct strbuf sb = STRBUF_INIT;
>  	struct strbuf author_ident = STRBUF_INIT;
>  	const char *index_file, *reflog_msg;
> -	char *nl, *p;
> +	char *nl;
>  	unsigned char sha1[20];
>  	struct ref_lock *ref_lock;
>  	struct commit_list *parents = NULL, **pptr = &parents;
> @@ -1601,11 +1601,8 @@ 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);
> -	}
> +	if (verbose)
> +		wt_status_truncate_message_at_cut_line(&sb);
>
>  	if (cleanup_mode != CLEANUP_NONE)
>  		stripspace(&sb, cleanup_mode == CLEANUP_ALL);
> diff --git a/t/t7507-commit-verbose.sh b/t/t7507-commit-verbose.sh
> index da5bd3b..2ddf28c 100755
> --- a/t/t7507-commit-verbose.sh
> +++ b/t/t7507-commit-verbose.sh
> @@ -65,9 +65,35 @@ 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 &&
> +		export GIT_EDITOR &&
> +		test_must_fail git commit -a -v 2>err
> +	) &&
> +	test_i18ngrep "Aborting commit due to empty commit message." err
> +'
> +
> +test_expect_success 'verbose diff is stripped out with set core.commentChar' '
> +	(
> +		GIT_EDITOR=cat &&
> +		export GIT_EDITOR &&
> +		test_must_fail git -c core.commentchar=";" 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..99c3d1c 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -16,6 +16,9 @@
>  #include "column.h"
>  #include "strbuf.h"
>
> +static char cut_line[] =
> +"------------------------ >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,18 @@ conclude:
>  	status_printf_ln(s, GIT_COLOR_NORMAL, "");
>  }
>
> +void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
> +{
> +	const char *p;
> +	struct strbuf pattern = STRBUF_INIT;
> +
> +	strbuf_addf(&pattern, "%c %s", comment_line_char, cut_line);
> +	p = strstr(buf->buf, pattern.buf);
> +	if (p && (p == buf->buf || p[-1] == '\n'))
> +		strbuf_setlen(buf, p - buf->buf);
> +	strbuf_release(&pattern);
> +}
> +
>  static void wt_status_print_verbose(struct wt_status *s)
>  {
>  	struct rev_info rev;
> @@ -787,10 +802,20 @@ 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) {
> +		const char *explanation = _("Do not touch the line above.\nEverything below will be removed.");
> +		struct strbuf buf = STRBUF_INIT;
> +
>  		rev.diffopt.use_color = 0;
> +		fprintf(s->fp, "%c %s", comment_line_char, cut_line);
> +		strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
> +		fprintf(s->fp, buf.buf);
> +		strbuf_release(&buf);
> +	}
>  	run_diff_index(&rev, 1);
>  }
>
> diff --git a/wt-status.h b/wt-status.h
> index 6c29e6f..30a4812 100644
> --- a/wt-status.h
> +++ b/wt-status.h
> @@ -91,6 +91,7 @@ struct wt_status_state {
>  	unsigned char cherry_pick_head_sha1[20];
>  };
>
> +void wt_status_truncate_message_at_cut_line(struct strbuf *);
>  void wt_status_prepare(struct wt_status *s);
>  void wt_status_print(struct wt_status *s);
>  void wt_status_collect(struct wt_status *s);

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

[Junio C Hamano]

Jens Lehmann <Jens.Lehmann@web.de> writes:

> +		fprintf(s->fp, "%c %s", comment_line_char, cut_line);
> +		strbuf_add_commented_lines(&buf, explanation, strlen(explanation));
> +		fprintf(s->fp, buf.buf);

This is better done with:

	fputs(buf.buf, s->fp);

Already locally tweaked while applying, so no need to resend only
for this change.