Re: [PATCH] commit: cope with scissors lines in commit message

SZEDER Gábor [off-list ref] writes:

The diff and submodule shortlog appended to the commit message template
by 'git commit --verbose' are not stripped when the commit message
contains an indented scissors line.

When cleaning up a commit message with 'git commit --verbose' or
'--cleanup=scissors' the code is careful and triggers only on a pure
scissors line, i.e. a line containing nothing but a comment character, a
space, and the scissors cut.  This is good, because people can embed
scissor lines in the commit message while using 'git commit --verbose',
and the text they write after their indented scissors line doesn't get
deleted.

While doing so, however, the cleanup function only looks at the first
line matching the scissors pattern and if it doesn't start at the
beginning of the line, then the function just returns without performing
any cleanup.  This is bad, because a "real" scissors line added by 'git
commit --verbose' might follow, and in that case the diff and submodule
shortlog get included in the commit message.

Yikes; this is not just "bad" but simply "wrong".  Thanks for
noticing.

 void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
 {
-	const char *p;
+	const char *p = buf->buf;
 	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);
+	while ((p = strstr(p, pattern.buf))) {
+		if (p == buf->buf || p[-1] == '\n') {
+			strbuf_setlen(buf, p - buf->buf);
+			break;
+		}
+		p++;
+	}

I however wonder if we should make strstr() do more work for us.

	strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
	if (starts_with(buf->buf, pattern.buf + 1))
		strbuf_setlen(buf, 0);
	else if ((p = strstr(buf->buf, pattern.buf)) != NULL)
        	strbuf_setlen(buf, p - buf->buf + 1);
	strbuf_release(&pattern);

perhaps?

Quoting Junio C Hamano [off-list ref]:

SZEDER Gábor [off-list ref] writes:

quoted

The diff and submodule shortlog appended to the commit message template
by 'git commit --verbose' are not stripped when the commit message
contains an indented scissors line.

When cleaning up a commit message with 'git commit --verbose' or
'--cleanup=scissors' the code is careful and triggers only on a pure
scissors line, i.e. a line containing nothing but a comment character, a
space, and the scissors cut.  This is good, because people can embed
scissor lines in the commit message while using 'git commit --verbose',
and the text they write after their indented scissors line doesn't get
deleted.

While doing so, however, the cleanup function only looks at the first
line matching the scissors pattern and if it doesn't start at the
beginning of the line, then the function just returns without performing
any cleanup.  This is bad, because a "real" scissors line added by 'git
commit --verbose' might follow, and in that case the diff and submodule
shortlog get included in the commit message.

Yikes; this is not just "bad" but simply "wrong".  Thanks for
noticing.

Great, the other day I scored a "Gaaah" from Peff, now a "Yikes" from  
you...  Doin' good! ;)

quoted

 void wt_status_truncate_message_at_cut_line(struct strbuf *buf)
 {
-	const char *p;
+	const char *p = buf->buf;
 	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);
+	while ((p = strstr(p, pattern.buf))) {
+		if (p == buf->buf || p[-1] == '\n') {
+			strbuf_setlen(buf, p - buf->buf);
+			break;
+		}
+		p++;
+	}

I however wonder if we should make strstr() do more work for us.

	strbuf_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
	if (starts_with(buf->buf, pattern.buf + 1))
		strbuf_setlen(buf, 0);
	else if ((p = strstr(buf->buf, pattern.buf)) != NULL)
        	strbuf_setlen(buf, p - buf->buf + 1);
	strbuf_release(&pattern);

perhaps?

Though this has one more if statement, it doesn't add a loop and  
eliminates the if (... || ...).
Good, will try to reroll in the evening.

Gábor

The diff and submodule shortlog appended to the commit message template
by 'git commit --verbose' are not stripped when the commit message
contains an indented scissors line.

When cleaning up a commit message with 'git commit --verbose' or
'--cleanup=scissors' the code is careful and triggers only on a pure
scissors line, i.e. a line containing nothing but a comment character, a
space, and the scissors cut.  This is good, because people can embed
scissor lines in the commit message while using 'git commit --verbose',
and the text they write after their indented scissors line doesn't get
deleted.

While doing so, however, the cleanup function only looks at the first
line matching the scissors pattern and if it doesn't start at the
beginning of the line, then the function just returns without performing
any cleanup.  This is wrong, because a "real" scissors line added by
'git commit --verbose' might follow, and in that case the diff and
submodule shortlog get included in the commit message.

Fix this by changing the scissors pattern to match only at the beginning
of the line, yet be careful to catch scissors on the first line as well.

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: SZEDER Gábor <redacted>
---
Changes besides incorporating Junio's suggestion and updating the commit
message accordingly:

 * Instead of adding a new test, modify the existing one to check
   handling indented scissors lines.
 * Add a test to check scissors on the first line
 
 t/t7502-commit.sh | 24 +++++++++++++++++++++++-
 wt-status.c       |  9 +++++----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 2e0d557243..b39e313ac2 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh

@@ -229,14 +229,36 @@ test_expect_success 'cleanup commit messages (scissors option,-F,-e)' '
 	cat >text <<EOF &&
 
 # to be kept
+
+  # ------------------------ >8 ------------------------
+# to be kept, too
 # ------------------------ >8 ------------------------
 to be removed
+# ------------------------ >8 ------------------------
+to be removed, too
+EOF
+
+	cat >expect <<EOF &&
+# to be kept
+
+  # ------------------------ >8 ------------------------
+# to be kept, too
 EOF
-	echo "# to be kept" >expect &&
 	git commit --cleanup=scissors -e -F text -a &&
 	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
 	test_cmp expect actual
+'
 
+test_expect_success 'cleanup commit messages (scissors option,-F,-e, scissors on first line)' '
+
+	echo >>negative &&
+	cat >text <<EOF &&
+# ------------------------ >8 ------------------------
+to be removed
+EOF
+	git commit --cleanup=scissors -e -F text -a --allow-empty-message &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'cleanup commit messages (strip option,-F)' '

diff --git a/wt-status.c b/wt-status.c
index c56c78fb6f..eaed4fed32 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -825,10 +825,11 @@ 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_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
+	if (starts_with(buf->buf, pattern.buf + 1))
+		strbuf_setlen(buf, 0);
+	else if ((p = strstr(buf->buf, pattern.buf)))
+		strbuf_setlen(buf, p - buf->buf + 1);
 	strbuf_release(&pattern);
 }
 

-- 
2.4.3.384.g605df7b

The diff and submodule shortlog appended to the commit message template
by 'git commit --verbose' are not stripped when the commit message
contains an indented scissors line.

When cleaning up a commit message with 'git commit --verbose' or
'--cleanup=scissors' the code is careful and triggers only on a pure
scissors line, i.e. a line containing nothing but a comment character, a
space, and the scissors cut.  This is good, because people can embed
scissors lines in the commit message while using 'git commit --verbose',
and the text they write after their indented scissors line doesn't get
deleted.

While doing so, however, the cleanup function only looks at the first
line matching the scissors pattern and if it doesn't start at the
beginning of the line, then the function just returns without performing
any cleanup.  This is wrong, because a "real" scissors line added by
'git commit --verbose' might follow, and in that case the diff and
submodule shortlog get included in the commit message.

Fix this by changing the scissors pattern to match only at the beginning
of the line, yet be careful to catch scissors on the first line as well.

Helped-by: Junio C Hamano [off-list ref]
Signed-off-by: SZEDER Gábor <redacted>
---
Fixed a typo in the commit message.
 
 t/t7502-commit.sh | 24 +++++++++++++++++++++++-
 wt-status.c       |  9 +++++----
 2 files changed, 28 insertions(+), 5 deletions(-)

diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index 2e0d557243..b39e313ac2 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh

@@ -229,14 +229,36 @@ test_expect_success 'cleanup commit messages (scissors option,-F,-e)' '
 	cat >text <<EOF &&
 
 # to be kept
+
+  # ------------------------ >8 ------------------------
+# to be kept, too
 # ------------------------ >8 ------------------------
 to be removed
+# ------------------------ >8 ------------------------
+to be removed, too
+EOF
+
+	cat >expect <<EOF &&
+# to be kept
+
+  # ------------------------ >8 ------------------------
+# to be kept, too
 EOF
-	echo "# to be kept" >expect &&
 	git commit --cleanup=scissors -e -F text -a &&
 	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
 	test_cmp expect actual
+'
 
+test_expect_success 'cleanup commit messages (scissors option,-F,-e, scissors on first line)' '
+
+	echo >>negative &&
+	cat >text <<EOF &&
+# ------------------------ >8 ------------------------
+to be removed
+EOF
+	git commit --cleanup=scissors -e -F text -a --allow-empty-message &&
+	git cat-file -p HEAD |sed -e "1,/^\$/d">actual &&
+	test_must_be_empty actual
 '
 
 test_expect_success 'cleanup commit messages (strip option,-F)' '

diff --git a/wt-status.c b/wt-status.c
index c56c78fb6f..eaed4fed32 100644
--- a/wt-status.c
+++ b/wt-status.c

@@ -825,10 +825,11 @@ 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_addf(&pattern, "\n%c %s", comment_line_char, cut_line);
+	if (starts_with(buf->buf, pattern.buf + 1))
+		strbuf_setlen(buf, 0);
+	else if ((p = strstr(buf->buf, pattern.buf)))
+		strbuf_setlen(buf, p - buf->buf + 1);
 	strbuf_release(&pattern);
 }
 

-- 
2.4.3.384.g605df7b