Help/Advice needed on diff bug in xutils.c

Hi all! Please give me a sanity check before I go crazy...

There is a bug in git diff (ignoring whitespace) that does not take into account a trailing space at the end of a line at the end of a file when no new line follows.

Here is the example of the bug:
mkdir test_ws_eof
cd test_ws_eof
git init
echo -n "Test" > test.txt
git add .
git commit -m'test'
git symbolic-ref HEAD refs/heads/with_space
rm .git/index
git clean -f
echo -n "Test ">test.txt
git add .
git commit -m'test'
# Ignoring all whitespace there shouldn't be a diff. git diff -w master -- test.txt
# Ignoring space at eol there shouldn't be a diff git diff --ignore-space-at-eol master -- test.txt # Ignoring with -b might have a case for a diff showing. git diff -b master -- test.txt

In the xutils.c xdl_hash_record_with_whitespace function the trailing space prior to eof was being calculated into the hash, I fixed that with the change below, but there is still a difference being noted in xdl_recmatch because of the size difference.

Before I go changing something that shouldn't be changed could someone provide some input please?

Thanks for reading,
Thell

index 04ad468..623da92 100644
@@ -243,17 +243,17 @@ static unsigned long

xdl_hash_record_with_whitespace(char
const **data,

                if (isspace(*ptr)) {
                        const char *ptr2 = ptr;
                        while (ptr + 1 < top && isspace(ptr[1])

- && ptr[1] != '\n')
+ && ( ptr[1] != '\n' && ptr[1] !=
'\0' ) )
ptr++;
if (flags & XDF_IGNORE_WHITESPACE)
; /* already handled */
else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
- && ptr[1] != '\n') {
+ && ( ptr[1] != '\n' && ptr[1] !=

'\0' ) ) {
                                ha += (ha << 5);
                                ha ^= (unsigned long) ' ';
                        }
                        else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
-                                       && ptr[1] != '\n') {
+                                       && ( ptr[1] != '\n' && ptr[1] !=
'\0' ) ) {
                                while (ptr2 != ptr + 1) {

Hi,

If you turn that into a patch to, say, t/t4015-diff-whitespace.sh (adding a test_expect_failure for a known bug), it is much easier to convince developers to work on the issue.

First, your coding style is different from the surrounding code. I think it goes without saying that this should be fixed.

Second, you do not need the parentheses at all (and therefore they should go).

Third, libxdiff does not assume to be fed NUL delimited strings.

Fourth, that condition "ptr + 1 < top" is already doing what you tried to accomplish here.

So I guess that you need to do add "ptr + 1 < top" checks instead.

Thanks,
Dscho

Johannes Schindelin ([email protected]) wrote on Aug 5, 2009:

First, please forgive my hubris at thinking I had _found_ a bug when my ignorance of the issue was so obvious. I am definitely humbled after reading every post in the archive having to do with whitespace and diff.

Thank you. In progress.

I am curious if t4015 is planned to be to be rewritten to follow what Junio had outlined and Giuseppe implemented for t4107-apply-ignore-whitespace.sh to make the spaces more obvious to the reader:

One other question on the making of the test in regards to the following quote from:
http://article.gmane.org/gmane.comp.version-control.git/124765

The test being implemented was t4107-apply-ignore-whitespace.sh.

Are there any plans to have t4015-diff-whitespace.sh tests rewritten in the same fashion?

You're absolutely right, I'll be more aware in the future.

I'll give it another go.

Thank you for the advice Dscho.

Thell

- Test each diff whitespace ignore option on trailing-space at eof

---

Johannes Schindelin ([email protected]) wrote on Aug 5, 2009:

Is this more along the right line?

Thell

 t/t4015-diff-whitespace.sh |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

index 6d13da3..fddbf20 100755
@@ -395,4 +395,27 @@ test_expect_success 'combined diff with autocrlf conversion' '
 
 '
 
+test_expect_failure 'diff -w on trailing-space with no newline' '
+
+	git reset --hard &&
+	printf "foo " >x &&
+	git commit -m "trailing-space @ eof test" x &&
+	printf "foo" >x &&
+	git commit -m "trailing-space @ eof test" x &&
+	test_must_pass git diff -w HEAD^ -- x | grep "foo "
+
+'
+
+test_expect_failure 'diff -b on trailing-space with no newline' '
+
+	test_must_pass git diff -b HEAD^ -- x | grep "foo "
+
+'
+
+test_expect_failure 'diff --ignore-space-at-eol on trailing-space with no newline' '
+
+	test_must_pass git diff -ignore-space-at-eol HEAD^ -- x | grep "foo "
+
+'
+
 test_done

1.6.4.240.g4cd31

[1/6]
Add supplemental test for trailing-whitespace on incomplete lines.

-- This patch is for illustrative purposes only. It exposes the current failures of git diff whitespace ignore options when dealing with trailing- spaces on incomplete lines.

[2/6] through [5/6]
Make xdl_hash_record_with_whitespace ignore eof Make diff -w handle trailing-spaces on incomplete lines. Make diff -b handle trailing-spaces on incomplete lines. Make diff --ignore-space-at-eol handle incomplete lines.

-- These alter the record ptr loops to go to the end of the record and to treat the terminator as it would '\n'.

[6/6]
Add diff tests for trailing-space on incomplete lines

-- Just the seven test cases that would identify future breakage.

 t/t4015-diff-trailing-whitespace.sh |   95 +++++++++++++++++++++++++++++++++++
 t/t4015-diff-whitespace.sh          |   33 ++++++++++++
 xdiff/xutils.c                      |   20 ++++----
 3 files changed, 138 insertions(+), 10 deletions(-)
 create mode 100755 t/t4015-diff-trailing-whitespace.sh

Thell Fowler ([email protected]) wrote on Aug 19, 2009:

Don't bother trying this series. I tried it out on live data today and it does not work. It actually caused regression in the diffs for the conversion project I'm working on.

What is _REALLY_ odd is that it didn't make any tests fail in the test dir using master, next, and pu.

Perhaps someone can explain what I did wrong when testing?

git checkout master
make -s clean && make -s all && make -s install && cd t && make

I really did do over 9 hours of testing using the test dir. First with just the branches with no modification, then with the modified t4015-diff-whitespace.sh, then with the xutils.c patch. And this was on each branch at about 40 minutes per run through.

Thell

The test suite has a very good coverage, but it surely is not complete.

This should have worked. Although I prefer just:

  $ make -j4 && make test -j16

Alex Riesen ([email protected]) wrote on Aug 21, 2009:

Well, two lessons learned...
1) Don't do isolated tests and count on the bundled tests to catch the corner cases.
2) Write more tests.

Good to know I was on the right track. Thanks.

Thell

*** For illustrative purposes only and not meant for upstream ***

- Adds a stand-alone test that loops through A-side B-side with and without new-lines from 0 to 3 spaces per side. This is a draft test meant to expose the issue with xutils.c handling of incomplete lines and trailing-spaces.

---
 t/t4015-diff-trailing-whitespace.sh |   95 +++++++++++++++++++++++++++++++++++
 1 files changed, 95 insertions(+), 0 deletions(-)
 create mode 100755 t/t4015-diff-trailing-whitespace.sh

new file mode 100755
index 0000000000000000000000000000000000000000..c4937c1b457c24b35565b09e7b262443a05f9795
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='Test trailing whitespace in diff engine.
+
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
+
+# Trailing-space testing with and without newlines.
+prepare_diff_file () {
+	printf "%s%$2s" foo "" >"$1"
+	if [ $3 = "+nl" ]
+	then
+		printf "\n" >>"$1"
+	fi
+}
+
+diff_trailing () {
+	foo="foo___"
+	prepare_diff_file "left" "$2" "$3"
+	lfoo=$( expr substr $foo 1 $((3+$2)) )
+	lfoo=${lfoo}"$3"
+
+	prepare_diff_file "right" "$4" "$5"
+	rfoo=$( expr substr $foo 1 $((3+$4)) )
+	rfoo=${rfoo}"$5"
+
+	label="-$1 $lfoo $rfoo ($6)"
+
+	if [ "$6" != "should_diff" ]
+	then
+		negate='!'
+	else
+		negate=''
+	fi
+
+	if [ -z "$7" ]
+	then
+		test_expect_success "$label" \
+		"$negate git diff --no-index -$1 -- left right | grep -q foo"
+	else
+		test_expect_failure "$label" \
+		"$negate git diff --no-index -$1 -- left right | grep -q foo"
+	fi
+
+	test_debug "git diff --no-index -$1 -- left right | grep foo"
+}
+
+touch diffout
+for arg in -ignore-all-space -ignore-space-at-eol -ignore-space-change
+do
+	for i1 in 0 1 2 3
+	do
+		for i2 in 0 1 2 3
+		do
+			diff_trailing $arg $i1 +nl $i2 -nl should_not_diff >> diffout
+			diff_trailing $arg $i1 -nl $i2 +nl should_not_diff >> diffout
+
+			if [ $i1 -ne $i2 ]
+			then
+				diff_trailing $arg $i1 +nl $i2 +nl should_not_diff >> diffout
+				diff_trailing $arg $i1 -nl $i2 -nl should_not_diff >> diffout
+			fi
+		done
+	done
+done
+
+test_debug 'grep "FAIL" diffout'
+
+for arg in all eol change
+do
+	grep "FAIL" diffout | \
+	grep "$arg" | \
+	cut -d " " -f 4- | \
+
+	##  Playing with filtering to isolate core issue.
+	#sort -k 2,2 -k 3,3 | \
+	#awk '{ forward = $2 " " $3; reverse = $3 " " $2}
+	#	!seen[forward]++ && !seen[reverse]++' | \
+	#sort -k 2,2 | \
+
+	##  Playing with filtering to isolate core issue.
+	##  This seems like the most illustrative output...
+	awk '{ key=$3 ; gsub(/-/, "+", key) ; key=$2 ":" key ; if ( hash[key]++ == 0 ) print ; }'
+	
+	##  Playing with filtering to isolate core issue.
+	#awk '{ if ( $3 ~ /.*\-/ )
+	#		print $0
+	#	else
+	#		print $1 " " $3 " " $2 " " $4
+	#	; }' | \
+	#sort -k 2,2 -k 3,3
+done
+
+test_done

1.6.4.172.g5c0d0.dirty

- When xdl_hash_record_with_whitespace encountered an incomplete line the hash would be different than the identical line with either --ignore-space-change or --ignore-space-at-eol on an incomplete line because they only terminated with a check for a new-line.

---
 xdiff/xutils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 04ad468702209b77427e635370d41001986042ce..c6512a53b08a8c9039614738310aa2786f4fbb1c 100644
@@ -248,12 +248,12 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
 			if (flags & XDF_IGNORE_WHITESPACE)
 				; /* already handled */
 			else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
-					&& ptr[1] != '\n') {
+					&& ptr[1] != '\n' && ptr + 1 < top) {
 				ha += (ha << 5);
 				ha ^= (unsigned long) ' ';
 			}
 			else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
-					&& ptr[1] != '\n') {
+					&& ptr[1] != '\n' && ptr + 1 < top) {
 				while (ptr2 != ptr + 1) {
 					ha += (ha << 5);
 					ha ^= (unsigned long) *ptr2;

1.6.4.172.g5c0d0.dirty

- When processing trailing spaces with --ignore-all-space a diff would be found whenever one side had 0 spaces and either (or both) sides was an incomplete line. xdl_recmatch should process the full length of the record instead of assuming both sides have a terminator.

---
 xdiff/xutils.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

index c6512a53b08a8c9039614738310aa2786f4fbb1c..1f28f4fb4e0a8fdc6c9aa1904cf0362dd1e7b977 100644
@@ -191,14 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 	int i1, i2;
 
 	if (flags & XDF_IGNORE_WHITESPACE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
+		for (i1 = i2 = 0; i1 <= s1 && i2 <= s2; ) {
 			if (isspace(l1[i1]))
-				while (isspace(l1[i1]) && i1 < s1)
+				while (isspace(l1[i1]) && i1 <= s1)
 					i1++;
 			if (isspace(l2[i2]))
-				while (isspace(l2[i2]) && i2 < s2)
+				while (isspace(l2[i2]) && i2 <= s2)
 					i2++;
-			if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
+			if (i1 <= s1 && i2 <= s2 && l1[i1++] != l2[i2++])
 				return 0;
 		}
 		return (i1 >= s1 && i2 >= s2);

1.6.4.172.g5c0d0.dirty

Thell Fowler ([email protected]) wrote on Aug 19, 2009:

Thell

- When processing trailing spaces with --ignore-space-change a diff would be found whenever an incomplete line terminated before the whitespace handling started regardless of actual trailing-spaces. xdl_recmatch should process the full length of the record instead of assuming both sides have a terminator, and should treat the terminator as a whitespace like it does with '\n'.

---
 xdiff/xutils.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

index 1f28f4fb4e0a8fdc6c9aa1904cf0362dd1e7b977..e126de450c99fb1e557c2cfc0ffe54e8e3e80394 100644
@@ -203,9 +203,9 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 		}
 		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1])) {
-				if (!isspace(l2[i2]))
+		for (i1 = i2 = 0; i1 <= s1 && i2 <= s2; ) {
+			if (isspace(l1[i1]) || (i1 == s1 && i2 < s2)) {
+				if (!isspace(l2[i2]) && i2 != s2)
 					return 0;
 				while (isspace(l1[i1]) && i1 < s1)
 					i1++;

1.6.4.172.g5c0d0.dirty

- When processing with --ignore-space-change a diff would be found whenever an incomplete line was encountered. xdl_recmatch should process the full length of the record instead of assuming both sides have a terminator.

---
 xdiff/xutils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index e126de450c99fb1e557c2cfc0ffe54e8e3e80394..a8ed102d528bdb5d8f0839eb392b35dc1c534fba 100644
@@ -216,7 +216,7 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 		}
 		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
+		for (i1 = i2 = 0; i1 <= s1 && i2 <= s2; ) {
 			if (l1[i1] != l2[i2]) {
 				while (i1 < s1 && isspace(l1[i1]))
 					i1++;

1.6.4.172.g5c0d0.dirty

- Adds 7 --no-index tests to t4015-diff-whitespace.sh specifically to ensure that xutils.c xdl_hash_record_with_whitespace and xdl_recmatch process to the end of the record and handle an incomplete line terminator the same as a new-line.

---
 t/t4015-diff-whitespace.sh |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

index 6d13da30dad5a78fb17a01e86ef33072ea9e6250..193ddbe0659ede17154ffda3b25ebc5e6c686d6e 100755
@@ -395,4 +395,37 @@ test_expect_success 'combined diff with autocrlf conversion' '
 
 '
 
+# Ignore trailing-space testing on incomplete lines.
+prepare_diff_file () {
+	printf "%s%$2s" foo "" >"$1"
+	if [ $3 = "+nl" ]
+	then
+		printf "\n" >>"$1"
+	fi
+}
+
+diff_trailing () {
+	foo="foo___"
+	prepare_diff_file "left" "$2" "$3"
+	lfoo=$( expr substr $foo 1 $((3+$2)) )
+	lfoo=${lfoo}"$3"
+
+	prepare_diff_file "right" "$4" "$5"
+	rfoo=$( expr substr $foo 1 $((3+$4)) )
+	rfoo=${rfoo}"$5"
+
+	label="-$1 $lfoo $rfoo"
+
+	test_expect_success "$label" \
+	"! git diff --no-index -$1 -- left right | grep -q foo"
+}
+
+diff_trailing w 0 +nl 1 -nl
+diff_trailing w 0 -nl 1 -nl
+diff_trailing b 0 +nl 0 -nl
+diff_trailing b 1 +nl 0 -nl
+diff_trailing b 1 -nl 0 -nl
+diff_trailing -ignore-space-at-eol 0 +nl 0 -nl
+diff_trailing -ignore-space-at-eol 2 +nl 2 -nl
+
 test_done

1.6.4.172.g5c0d0.dirty

These patches are directly aimed at making it possible to validate the results of git apply --whitespace=fix which is currently not possible in files with incomplete lines in various situations (demonstrated in the test in PATCH 1).

The goal is to be able to validate the application of a patch generated from a dirty whitespace source to a clean target.

        D2  C2
        | \/ |
        | /\ |
        D1  C1

where D1..D2, C1..C2, C1..D2, D1..C2 should all yield the same patch-id when passed a diff ignoring the applicable whitespace changes.

Applying these patches does nothing to fix the case of differences when apply --whitespace=fix limits new blank lines at eof as can happen when foo\r\n\r\n\r\n is fixed to foo\n\n.

Thell Fowler (6):
Add supplemental test for trailing-whitespace on incomplete lines
xutils: fix hash with whitespace on incomplete line
xutils: fix ignore-all-space on incomplete line
xutils: fix ignore-space-change on incomplete line
xutils: fix ignore-space-at-eol on incomplete line
t4015: add tests for trailing-space on incomplete line

 t/t4015-diff-trailing-whitespace.sh |   95 +++++++++++++++++++++++++++++++++++
 t/t4015-diff-whitespace.sh          |   33 ++++++++++++
 xdiff/xutils.c                      |   39 ++++++++++-----
 3 files changed, 154 insertions(+), 13 deletions(-)
 create mode 100755 t/t4015-diff-trailing-whitespace.sh

*** For illustrative purposes only and not meant for upstream ***

- Adds a stand-alone test that loops through A-side B-side with and without new-lines from 0 to 3 spaces per side. This is a draft test meant to expose the issue with xutils.c handling of incomplete lines and trailing-spaces.

---
 t/t4015-diff-trailing-whitespace.sh |   95 +++++++++++++++++++++++++++++++++++
 1 files changed, 95 insertions(+), 0 deletions(-)
 create mode 100755 t/t4015-diff-trailing-whitespace.sh

new file mode 100755
index 0000000..079fba5
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='Test trailing whitespace in diff engine.
+
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
+
+# Trailing-space testing with and without newlines.
+prepare_diff_file () {
+	printf "%s%$2s" foo "" >"$1"
+	if [ $3 = "+nl" ]
+	then
+		printf "\n" >>"$1"
+	fi
+}
+
+diff_trailing () {
+	foo="foo___"
+	prepare_diff_file "left" "$2" "$3"
+	lfoo=$( expr substr $foo 1 $((3+$2)) )
+	lfoo=${lfoo}"$3"
+
+	prepare_diff_file "right" "$4" "$5"
+	rfoo=$( expr substr $foo 1 $((3+$4)) )
+	rfoo=${rfoo}"$5"
+
+	label="-$1 $lfoo $rfoo ($6)"
+
+	if [ "$6" != "should_diff" ]
+	then
+		negate='!'
+	else
+		negate=''
+	fi
+
+	if [ -z "$7" ]
+	then
+		test_expect_success "$label" \
+		"$negate git diff --no-index -$1 -- left right | grep -q foo"
+	else
+		test_expect_failure "$label" \
+		"$negate git diff --no-index -$1 -- left right | grep -q foo"
+	fi
+
+	test_debug "git diff --no-index -$1 -- left right | grep foo"
+}
+
+touch diffout
+for arg in -ignore-all-space -ignore-space-at-eol -ignore-space-change
+do
+	for i1 in 0 1 2 3
+	do
+		for i2 in 0 1 2 3
+		do
+			diff_trailing $arg $i1 +nl $i2 -nl should_not_diff >> diffout
+			diff_trailing $arg $i1 -nl $i2 +nl should_not_diff >> diffout
+
+			if [ $i1 -ne $i2 ]
+			then
+				diff_trailing $arg $i1 +nl $i2 +nl should_not_diff >> diffout
+				diff_trailing $arg $i1 -nl $i2 -nl should_not_diff >> diffout
+			fi
+		done
+	done
+done
+
+test_debug 'grep "FAIL" diffout'
+
+for arg in all eol change
+do
+	grep "FAIL" diffout | \
+	grep "$arg" | \
+	cut -d " " -f 4- | \
+
+	##  Playing with filtering to isolate core issue.
+	#sort -k 2,2 -k 3,3 | \
+	#awk '{ forward = $2 " " $3; reverse = $3 " " $2}
+	#	!seen[forward]++ && !seen[reverse]++' | \
+	#sort -k 2,2 | \
+
+	##  Playing with filtering to isolate core issue.
+	##  This seems like the most illustrative output...
+	awk '{ key=$3 ; gsub(/-/, "+", key) ; key=$2 ":" key ; if ( hash[key]++ == 0 ) print ; }'
+
+	##  Playing with filtering to isolate core issue.
+	#awk '{ if ( $3 ~ /.*\-/ )
+	#		print $0
+	#	else
+	#		print $1 " " $3 " " $2 " " $4
+	#	; }' | \
+	#sort -k 2,2 -k 3,3
+done
+
+test_done

1.6.4.176.g556a4

- Make xdl_hash_record_with_whitespace stop hashing before the eof when ignoring space change or space at eol on an incomplete line.

Resolves issue with a final trailing space being included in the hash on an incomplete line by treating the eof in the same fashion as a newline.

---
 xdiff/xutils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 04ad468..c6512a5 100644
@@ -248,12 +248,12 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
 			if (flags & XDF_IGNORE_WHITESPACE)
 				; /* already handled */
 			else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
-					&& ptr[1] != '\n') {
+					&& ptr[1] != '\n' && ptr + 1 < top) {
 				ha += (ha << 5);
 				ha ^= (unsigned long) ' ';
 			}
 			else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
-					&& ptr[1] != '\n') {
+					&& ptr[1] != '\n' && ptr + 1 < top) {
 				while (ptr2 != ptr + 1) {
 					ha += (ha << 5);
 					ha ^= (unsigned long) *ptr2;

1.6.4.176.g556a4

Please study the style of existing commit messages and imitate them.

		if (isspace(*ptr)) {
			const char *ptr2 = ptr;
			while (ptr + 1 < top && isspace(ptr[1])
					&& ptr[1] != '\n')
				ptr++;

---
 xdiff/xutils.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

index 04ad468..9411fa9 100644
@@ -242,18 +242,20 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
 	for (; ptr < top && *ptr != '\n'; ptr++) {
 		if (isspace(*ptr)) {
 			const char *ptr2 = ptr;
+			int at_eol;
 			while (ptr + 1 < top && isspace(ptr[1])
 					&& ptr[1] != '\n')
 				ptr++;
+			at_eol = (top <= ptr + 1 || ptr[1] == '\n');
 			if (flags & XDF_IGNORE_WHITESPACE)
 				; /* already handled */
 			else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
-					&& ptr[1] != '\n') {
+				 && !at_eol) {
 				ha += (ha << 5);
 				ha ^= (unsigned long) ' ';
 			}
 			else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
-					&& ptr[1] != '\n') {
+				 && !at_eol) {
 				while (ptr2 != ptr + 1) {
 					ha += (ha << 5);
 					ha ^= (unsigned long) *ptr2;

Junio C Hamano ([email protected]) wrote on Aug 23, 2009:

I'll keep trying.

I appreciate your taking the time to look at the issue and explaining the reasons for your change.

Yeah... comparing this commit message to the original shows a pretty stark difference. I'll get it 'the git way' eventually.

Thell

- Allow xdl_recmatch to recognize and continue processing when at the end of an incomplete line.

Resolves issue with --ignore-all-space when either side 1 or 2 has 0 trailing spaces and either (or both) are incomplete by allowing the processing loop to continue when one side has reached the end and includes a check for being at eof on an incomplete line.

---
 xdiff/xutils.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

index c6512a5..e22b4bb 100644
@@ -191,12 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 	int i1, i2;
 
 	if (flags & XDF_IGNORE_WHITESPACE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
+		for (i1 = i2 = 0; i1 < s1 || i2 < s2; ) {
 			if (isspace(l1[i1]))
-				while (isspace(l1[i1]) && i1 < s1)
+				while ((isspace(l1[i1]) && i1 < s1)
+						|| (i1 + 1 == s1 && l1[s1] != '\n'))
 					i1++;
 			if (isspace(l2[i2]))
-				while (isspace(l2[i2]) && i2 < s2)
+				while ((isspace(l2[i2]) && i2 < s2)
+						|| (i2 + 1 == s2 && l2[s2] != '\n'))
 					i2++;
 			if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
 				return 0;

1.6.4.176.g556a4

Quoting Junio C Hamano <[email protected]>

I think this should be "options to git diff don't work".

I have two unrelated questions.

(1) Why do you post patches to the list, instead of committing them yourself?
(2) How do I apply a patch like this one to try to my tree? Am I expected to edit the mail message to remove everything before the shears mark before running the git-am command?

Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Soory, I kant speel; thanks.

So that others can catch silly mistakes of mine, like the one you just caught.

I play three separate roles here, two of which I should send patches out while playing them.

* Just like everybody else, I find itches to scratch from time to time, and I build my own topic branches locally for the changes to scratch them, just like other contributors.

They are indeed committed and often immediately merged to 'pu', but I send out format-patch output for them, because I firmly believe that the development _process_, not just the end result, should be in the open. Everybody's patch should go through the list, get reviewed and improved by help from others. So should mine.

* I read others' patches, review, comment, and suggest improvements and make counterproposals, just like others on the list.

The "how about" patches when I am playing this role are often not meant as the final shape of the patch but to show the direction to improve upon. They are output from "git diff", not format-patch nor even "git diff --cached"---I do not commit, nor even add them to the index---and after I send out e-mails, I typically reset them away to work on something else, because they are usually not my itch.

* I accept patches that were reviewed favorably on the list by running "git am" on them.

That is how I have been doing it. My workflow is:

(1) First read patches in my primary mailbox, while copying promising

     ones to a separate mailbox;

(2) And then go through the separate mailbox as a separate pass, while fixing obvious typos and minor coding style violations still inside mailbox; and finally

(3) Run "git am" on the (possibly edited) patch to apply.

Because I'll be editing the messages (both log and code) _anyway_, removing everything before the scissors mark is not much of a trouble.

Having said that, I could use something like this.

-- >8 -- cut here -- >8 --
Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

This teaches mailinfo the scissors -- >8 -- mark; the command ignores everything before it in the message body.

---
 builtin-mailinfo.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

index b0b5d8f..461c47e 100644
@@ -712,6 +712,34 @@ static inline int patchbreak(const struct strbuf *line)
 	return 0;
 }
 
+static int scissors(const struct strbuf *line)
+{
+	size_t i, len = line->len;
+	int scissors_dashes_seen = 0;
+	const char *buf = line->buf;
+
+	for (i = 0; i < len; i++) {
+		if (isspace(buf[i]))
+			continue;
+		if (buf[i] == '-') {
+			scissors_dashes_seen |= 02;
+			continue;
+		}
+		if (i + 1 < len && !memcmp(buf + i, ">8", 2)) {
+			scissors_dashes_seen |= 01;
+			i++;
+			continue;
+		}
+		if (i + 7 < len && !memcmp(buf + i, "cut here", 8)) {
+			i += 7;
+			continue;
+		}
+		/* everything else --- not scissors */
+		break;
+	}
+	return scissors_dashes_seen == 03;
+}
+
 static int handle_commit_msg(struct strbuf *line)
 {
 	static int still_looking = 1;

@@ -723,10 +751,17 @@ static int handle_commit_msg(struct strbuf *line)
 		strbuf_ltrim(line);
 		if (!line->len)
 			return 0;
-		if ((still_looking = check_header(line, s_hdr_data, 0)) != 0)
+		still_looking = check_header(line, s_hdr_data, 0);
+		if (still_looking)
 			return 0;
 	}
 
+	if (scissors(line)) {
+		fseek(cmitmsg, 0L, SEEK_SET);
+		still_looking = 1;
+		return 0;
+	}
+
 	/* normalize the log message to UTF-8. */
 	if (metainfo_charset)
 		convert_to_utf8(line, charset.buf);

1.6.4.1

Quoting Junio C Hamano <[email protected]>

There are left handed people whose scissors run in the wrong direction.

index b0906ef..38c01e4 100644
@@ -725,7 +725,8 @@ static int scissors(const struct strbuf *line)
 			scissors_dashes_seen |= 02;
 			continue;
 		}
-		if (i + 1 < len && !memcmp(buf + i, ">8", 2)) {
+		if (i + 1 < len &&
+		    !memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2)) {
 			scissors_dashes_seen |= 01;
 			i++;
 			continue;

Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Heh.

Nanako Shiraishi ([email protected]) wrote on Aug 23, 2009:

Woohoo! Glad the left handed people aren't being discriminated against. ;)

BTW - I'm happily using this and think it should be in git!

Thell

The one I sent out had two bugs. Please discard and replace it with a newer one I'll be pushing out on 'pu' later today.

Subject: [PATCH] Wong title
From: is this one really the author? <[email protected]>

( Tested against current 925bd84 in pu. )

If we have a mail with this form

<header>
Subject: [PATCH] BLAH ONE
</header>

Subject: [PATCH] BLAH TWO
<...>
-- >8 --
Subject: [PATCH] BLAH THREE

the commit message looks like

BLAH TWO

Subject: [PATCH] BLAH THREE

I'd expect that we take the "Subject: " line after the mark and fallback to the header if missing.

Same applies to the "From: " lines.

This mail should be usable to your own tests.

-- >8 -- Please squash this to 925bd84 -- >8 --

---

index fcacc94..0c9a791 100644
@@ -138,6 +138,9 @@ The commit message is formed by the title taken from

the
where the patch begins. Excess whitespace at the end of each line is automatically stripped.

+If a line starts with a "-- >8 --" mark in the body of the message, +everything before (and the line itself) will be ignored. +
The patch is expected to be inline, directly following the message. Any line that is of the form:

Nicolas Sebrecht

[ Please, please, please, wrap your lines. ]

I'd say that should not document what people do but what the program does.

...and (">8" or "<8"), you're right. But isn't the following mark a bit too much permissive?

->8
Subject: [PATCH] squashable to 925bd84 (Teach mailinfo to ignore everything before -- >8 -- mark, 2009-08-23)

---

This patch supersedes my previous round.

 Documentation/git-am.txt |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

index fcacc94..1d10371 100644
@@ -138,6 +138,10 @@ The commit message is formed by the title taken

from the
where the patch begins. Excess whitespace at the end of each line is automatically stripped.

+If a line starts with a "-- >8 --" mark in the body of the message, +everything before (and the line itself) will be ignored. +Whitespaces and strings "cut here" are tolerated.
+
The patch is expected to be inline, directly following the message. Any line that is of the form:

Nicolas Sebrecht

( Paste error, sorry. )

->8
Subject: [PATCH] squashme to 925bd84 (Teach mailinfo to ignore everything before -- >8 -- mark, 2009-08-23)

---
 Documentation/git-am.txt |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

index fcacc94..5294d47 100644
@@ -138,6 +138,12 @@ The commit message is formed by the title taken

from the
where the patch begins. Excess whitespace at the end of each line is automatically stripped.

+If a line contains a mark in the body of the message, everything +before (and the line itself) will be ignored. A mark has typically +the form "-- >8 -- cut here -- >8 --". Strictly speacking, it must +have one dash at least and a ">8" (or "<8"). Spaces and strings +"cut here" are permited.
+
The patch is expected to be inline, directly following the message. Any line that is of the form:

Nicolas Sebrecht

Yeah, I agree that we should require a bit longer perforation, and perhaps we should tighten the rules a bit, while at the same time not limiting the request to cut to the exact phrase "cut here". As you pointed out, we do not want to be too lenient to allow misidentification, but at the same time it is nicer to be accomodating and treat something like this as a scissors line:

- - - >8 - - - remove everything above this line - - - >8 - - -

I think we have bikeshedded long enough, so I won't be touching this code any further only to change the definition of what a scissors mark looks like, but here is what I did during lunch break, with another comment added later to hint what s_hdr_data[] stands for after reading response from Don Zickus.

-- >8 --
From: Junio C Hamano <[email protected]>
Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

This teaches mailinfo the scissors -- >8 -- mark; the command ignores everything before it in the message body.

For lefties among us, we also support -- 8< -- ;-)

---
 builtin-mailinfo.c  |   71 ++++++++++++++++++++++++++++++++++++++++-
 t/t5100-mailinfo.sh |    2 +-
 t/t5100/info0014    |    5 +++
 t/t5100/msg0014     |    4 ++
 t/t5100/patch0014   |   64 ++++++++++++++++++++++++++++++++++++
 t/t5100/sample.mbox |   89 +++++++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 233 insertions(+), 2 deletions(-)
 create mode 100644 t/t5100/info0014
 create mode 100644 t/t5100/msg0014
 create mode 100644 t/t5100/patch0014

index b0b5d8f..7e09b51 100644
@@ -712,6 +712,56 @@ static inline int patchbreak(const struct strbuf *line)
 	return 0;
 }
 
+static int is_scissors_line(const struct strbuf *line)
+{
+	size_t i, len = line->len;
+	int scissors = 0, gap = 0;
+	int first_nonblank = -1;
+	int last_nonblank = 0, visible, perforation, in_perforation = 0;
+	const char *buf = line->buf;
+
+	for (i = 0; i < len; i++) {
+		if (isspace(buf[i])) {
+			if (in_perforation) {
+				perforation++;
+				gap++;
+			}
+			continue;
+		}
+		last_nonblank = i;
+		if (first_nonblank < 0)
+			first_nonblank = i;
+		if (buf[i] == '-') {
+			in_perforation = 1;
+			perforation++;
+			continue;
+		}
+		if (i + 1 < len &&
+		    (!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
+			in_perforation = 1;
+			perforation += 2;
+			scissors += 2;
+			i++;
+			continue;
+		}
+		in_perforation = 0;
+	}
+
+	/*
+	 * The mark must be at least 8 bytes long (e.g. "-- >8 --").
+	 * Even though there can be arbitrary cruft on the same line
+	 * (e.g. "cut here"), in order to avoid misidentification, the
+	 * perforation must occupy more than a third of the visible
+	 * width of the line, and dashes and scissors must occupy more
+	 * than half of the perforation.
+	 */
+
+	visible = last_nonblank - first_nonblank + 1;
+	return (scissors && 8 <= visible &&
+		visible < perforation * 3 &&
+		gap * 2 < perforation);
+}
+
 static int handle_commit_msg(struct strbuf *line)
 {
 	static int still_looking = 1;

@@ -723,7 +773,8 @@ static int handle_commit_msg(struct strbuf *line)
 		strbuf_ltrim(line);
 		if (!line->len)
 			return 0;
-		if ((still_looking = check_header(line, s_hdr_data, 0)) != 0)
+		still_looking = check_header(line, s_hdr_data, 0);
+		if (still_looking)
 			return 0;
 	}
 

@@ -731,6 +782,24 @@ static int handle_commit_msg(struct strbuf *line)
 	if (metainfo_charset)
 		convert_to_utf8(line, charset.buf);
 
+	if (is_scissors_line(line)) {
+		int i;
+		rewind(cmitmsg);
+		ftruncate(fileno(cmitmsg), 0);
+		still_looking = 1;
+
+		/*
+		 * We may have already read "secondary headers"; purge
+		 * them to give ourselves a clean restart.
+		 */
+		for (i = 0; header[i]; i++) {
+			if (s_hdr_data[i])
+				strbuf_release(s_hdr_data[i]);
+			s_hdr_data[i] = NULL;
+		}
+		return 0;
+	}
+
 	if (patchbreak(line)) {
 		fclose(cmitmsg);
 		cmitmsg = NULL;

index e70ea94..e848556 100755
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
 	'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
 	last=`cat last` &&
 	echo total is $last &&
-	test `cat last` = 13'
+	test `cat last` = 14'
 
 for mail in `echo 00*`
 do

new file mode 100644
index 0000000..ab9c8d0
@@ -0,0 +1,5 @@
+Author: Junio C Hamano
+Email: [email protected]
+Subject: Teach mailinfo to ignore everything before -- >8 -- mark
+Date: Thu, 20 Aug 2009 17:18:22 -0700
+

new file mode 100644
index 0000000..259c6a4
@@ -0,0 +1,4 @@
+This teaches mailinfo the scissors -- >8 -- mark; the command ignores
+everything before it in the message body.
+
+Signed-off-by: Junio C Hamano <[email protected]>

1.6.4.1

<...>

I'm not sure I understand. Are you still open to a patch touching this code /too/?

Anyway, here's what I wrote based on your last round in pu. I've change the rules to something because I think we'd rather simple ― and "easy" to explain to the end-user ― rules over "obfuscated" ones.

-- >8 -- squashable to 8683eeb (ogirin/pu) -- >8 --

Subject: Teach mailinfo to ignore everything before a scissors line

This teaches mailinfo the scissors mark (e.g. "-- >8 --"); the command ignores everything before it in the message body.

For lefties among us, we also support -- 8< -- ;-)

We can skip this check using the "--ignore-scissors" option on both the git-mailinfo and the git-am command line. This is necessary because the stripped message may be either

interesting from the eyes of the maintainer, regardless what the author think;

or

the scissors line check is a false positive.

Basically, the rules are:

(1) a scissors mark:

- must be 8 characters long;
- must have a dash;
- must have either ">8" or "<8";
- may contain spaces.

(2) a scissors line:

- must have only one scissors mark;
or
- must have any comment between two identical scissors marks;
- always ignore spaces outside the scissors marks.

---
 Documentation/git-am.txt       |   14 +++++-
 Documentation/git-mailinfo.txt |    7 ++-
 builtin-mailinfo.c             |  103 +++++++++++++++++++++++----------------
 git-am.sh                      |   14 ++++-
 4 files changed, 90 insertions(+), 48 deletions(-)

index fcacc94..2773a3e 100644
@@ -13,7 +13,7 @@ SYNOPSIS
 	 [--3way] [--interactive] [--committer-date-is-author-date]
 	 [--ignore-date] [--ignore-space-change | --ignore-whitespace]
 	 [--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
-	 [--reject] [-q | --quiet]
+	 [--reject] [-q | --quiet] [--ignore-scissors]
 	 [<mbox> | <Maildir>...]
 'git am' (--skip | --resolved | --abort)
 

@@ -118,6 +118,14 @@ default.   You can use `--no-utf8` to override this.
 --abort::
 	Restore the original branch and abort the patching operation.
 
+--ignore-scissors::
+	Do not check for scissors line in the commit message.  A scissors
+	line consists of a scissors mark which must be at least 8
+	characters long and which must contain dashes '-' and a scissors
+	(either ">8" or "<8").  Spaces are also permited inside the mark.
+	To add a comment on this line, it must be embedded between two
+	identical marks (e.g. "-- >8 -- squashme to <commit> -- >8 --").
+
 DISCUSSION
 ----------
 

@@ -131,7 +139,9 @@ commit is about in one line of text.
 "From: " and "Subject: " lines starting the body (the rest of the
 message after the blank line terminating the RFC2822 headers)
 override the respective commit author name and title values taken
-from the headers.
+from the headers. These lines immediatly following a scissors line
+override the respective fields regardless what could stand at the
+beginning of the body.
 
 The commit message is formed by the title taken from the
 "Subject: ", a blank line and the body of the message up to

index 8d95aaa..e16a577 100644
@@ -8,7 +8,8 @@ git-mailinfo - Extracts patch and authorship from a single e-mail message
 
 SYNOPSIS
 --------
-'git mailinfo' [-k] [-u | --encoding=<encoding> | -n] <msg> <patch>
+'git mailinfo' [-k] [-u | --encoding=<encoding> | -n] [--ignore-scissors]
+<msg> <patch>
 
 
 DESCRIPTION

@@ -49,6 +50,10 @@ conversion, even with this flag.
 -n::
 	Disable all charset re-coding of the metadata.
 
+--ignore-scissors::
+	Do not check for a scissors line (see linkgit:git-am[1]
+	for more information on scissors lines).
+
 <msg>::
 	The commit log message extracted from e-mail, usually
 	except the title line which comes from e-mail Subject.

index 3c03f3e..17c883f 100755
@@ -15,6 +15,7 @@ q,quiet         be quiet
 s,signoff       add a Signed-off-by line to the commit message
 u,utf8          recode into utf8 (default)
 k,keep          pass -k flag to git-mailinfo
+ignore-scissors pass --ignore-scissors flag to git-mailinfo
 whitespace=     pass it through git-apply
 ignore-space-change pass it through git-apply
 ignore-whitespace pass it through git-apply

@@ -288,7 +289,7 @@ split_patches () {
 prec=4
 dotest="$GIT_DIR/rebase-apply"
 sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=
-resolvemsg= resume=
+resolvemsg= resume= ignore_scissors=
 git_apply_opt=
 committer_date_is_author_date=
 ignore_date=

@@ -310,6 +311,8 @@ do
 		utf8= ;;
 	-k|--keep)
 		keep=t ;;
+	--ignore-scissors)
+		ignore_scissors=t ;;
 	-r|--resolved)
 		resolved=t ;;
 	--skip)

@@ -435,7 +438,7 @@ else
 
 	split_patches "$@"
 
-	# -s, -u, -k, --whitespace, -3, -C, -q and -p flags are kept
+	# Following flags are kept
 	# for the resuming session after a patch failure.
 	# -i can and must be given when resuming.
 	echo " $git_apply_opt" >"$dotest/apply-opt"

@@ -443,6 +446,7 @@ else
 	echo "$sign" >"$dotest/sign"
 	echo "$utf8" >"$dotest/utf8"
 	echo "$keep" >"$dotest/keep"
+	echo "$ignore_scissors" >"$dotest/ignore-scissors"
 	echo "$GIT_QUIET" >"$dotest/quiet"
 	echo 1 >"$dotest/next"
 	if test -n "$rebasing"

@@ -484,6 +488,10 @@ if test "$(cat "$dotest/keep")" = t
 then
 	keep=-k
 fi
+if test "$(cat "$dotest/ignore-scissors")" = t
+then
+	ignore_scissors='--ignore-scissors'
+fi
 if test "$(cat "$dotest/quiet")" = t
 then
 	GIT_QUIET=t

@@ -538,7 +546,7 @@ do
 	# by the user, or the user can tell us to do so by --resolved flag.
 	case "$resume" in
 	'')
-		git mailinfo $keep $utf8 "$dotest/msg" "$dotest/patch" \
+		git mailinfo $keep $ignore_scissors $utf8 "$dotest/msg" "$dotest/patch" \
 			<"$dotest/$msgnum" >"$dotest/info" ||
 			stop_here $this
 

1.6.4.1.334.gf42e22

What I meant was that I would not want to spend any more of _my_ time on the definition of the scissors for now. That means spending or wasting time on improving the 'pu' patch myself, or looking at others patch to find flaws in them.

Of course, as the maintainer, I would need to look at proposals to improve or fix bugs in the code before the series hits the master, but I would give zero priority to the patches that change the definition at least for now to give myself time to work on more useful things.

I think --ignore-scissors is a good thing to add, regardless of what the definition of scissors should be. So your patch should definitely be separated into two parts.

    "-- 8< -- please cut here -- -- 8< --" 

    "-- 8< -- -- please cut here -- 8< -- --"

    "-- 8< -- please cut here -- 8< -- --"

    "-- 8< -- -- please cut here -- -- 8< --"

    "-- 8< -- -- please cut here -- -- >8 --"

	if (!ignore_scissors && is_scissors_line(line)) {
        	...
	}

Ok, thank you.

Could find it at the end of the mails.

It is for the warning() call

warning("scissors line found, will skip text above");

I've added. That said, moving this declaration to builtin.h could be a good idea. Hint?

Good eyes.

Yes. Instead of looking for dashes alone, I will give a try to something like

	  if (!scissors_dashes_seen)
	    mark_start = i;
	  if (i + 1 < len) {
	    if (!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
	      scissors_dashes_seen |= 02;
	      i++;
	      mark_end = i;
	      continue;
	    else if (!memcmp(buf + i "--", 2) {
	      scissors_dashes_seen |= 04;
	      i++;
	      mark_end = i;
	      continue;
	    }
	  }
	  if (i + 2 < len)
	    if (!memcmp(buf + i + 1, "- -", 3) {
	      scissors_dashes_seen |= 04;
	      i += 2;
	      mark_end = i;
	      continue;
	    }
	  if (buf[i] == '-') {
	    mark_end = i;
	    scissors_dashes_seen |= 01;
	    continue;
	  }
	  break;
	}
	
	if (scissors_dashes_seen == 07) {
	  ...

Actually, I believe this one should really not be a scissors line. If we accept some random dashes around markers it will break the definition of the mark itself.

As I said, I'd rather rules easy to define over others because if the end-user scissors line doesn't work, he can refer to the documentation...

...and symmetrical markers make sense to the user. Will add this.

Does the compilers (or a standard) assure that the members are evaluated in the left-right order?

Otherwise, we may call is_scissors_line() where not needed.

Nicolas Sebrecht

Ok, I agree that the way I worded the message was suboptimal, so let's try again.

I would appreciate if the members of the list come up with an alternative definition with a good implementation that they can agree on, and present the result as a list consensus, with a solid justification to replace the crap I have queued in 'pu', in the form of an applicable patch. Because I consider that the exact definition of what a scissors line looks like is an insignificant detail, I would prefer to see that process happen without involving me.

By the way, I already queued your documentation patch, as adding the part that describes what a "scissors" line is good for is a very good idea. The "community" patch to replace the definition of scissors line may have to update the part that describes what a "scissors" line looks like in your patch.

Having thought about this a bit more, I do not think --ignore-scissors makes much sense, for several reasons.

Traditionally, a few lines of "background material" that accompanies a patch, without being a part of discussion thread that quotes large chunks of original message with ">" (like you see above), are given below the three-dash line, not above "scissors". This is a good practice not only because we did not have "scissors" support in mailinfo, but because it forced the author to be concise and to the point, and also immediately below the three-dash lines is where the diffstat comes, and it is a place designed to be used for memory refreshers (e.g. "I changed this and that from the previous round based on comments by ...").

The scissors feature shouldn't be used as the replacement for this, not from technical but from human efficiency reasons, as you have to first read above scissors and then jump your eyes down to diffstat, before deciding if it is worth your time to read the commit log message and the patch.

When there is a long discussion, a message in the thread, after following the usual discussion style, may give a (counter)proposal as a "how about this" patch. Such a patch is still _primarily_ for discussion, but it sometimes turn out to be a good solution to the problem discussed in the thread. The maintainer (or participant) then deliberately picks that message and feeds it to "am", and it would be nice if things above scissors are removed automatically. This is the _only_ intended use case of the "scissors" line.

I therefore conclude that using the "remove above scissors" should be a conscious decision, and should not be enabled by default. --obey-scissors would be a good option for this reason.

Besides, if you _lost_ information because the scissors that is on by default gave a false positive, you have to reset HEAD^ and re-apply. If on the other hand we mistakenly kept cruft above a scissors, we can edit it away using "rebase -i". So the failure recovery is much nicer if the feature is not on by default.

I'm not sure what between --obey or --ignore will help most to write good commit message. I (as a maintainer myself or as a contributor) usually prefer to --amend a commit rather than play with copy/paste or starting from scratch. So, I tend to agree even if the reasons are not exactly the same. :-)

That said, I don't bother what is the default that much. The main purpose is to have the choice.

For people who _really_ want to obey to scissors by default I'll add an option to git-config. Whithout more comments, I'll add

scissors.obey

.

Nicolas Sebrecht

mailsplit.scissors

Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

That may be a better name.

It must take lower precedence than the command line --no-scissors option, and that option must be given to am when rebase internally runs am, so that we won't pay attention to scissors when rebasing existing commits.

Hi,

Sorry, did not have time to read this thread properly, but has anybody put thought into the interaction between this patch and "git rebase" (which uses "git am", and therefore mailsplit, internally)?

Ciao,
Dscho

I was looking around this area tonight (I promised I won't touch the definition of scissors, but I never said I won't work on making it usable), as I originally shared the same worry with you.

It turns out that "rebase" invokes "am" with the "--rebasing" option. Under this option, "am" uses an equivalent of "commit -C $commit" internally to port the message forward. So our worries are unfounded.

Hi,

Thank you very much! This relieves me, indeed.

Ciao,
Dscho

Quoting myself...

Junio, perhaps you want to squash some documentation, too.

-- 8< -- cut here -- 8< -- cut here -- 8< --
Subject: [PATCH] Documentation: describe the scissors mark support of "git am"

Describe what a scissors mark looks like, and explain in what situation it is often used.

---
 Documentation/git-am.txt |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

index fcacc94..fecd5ac 100644
@@ -128,10 +128,18 @@ the commit, after stripping common prefix "[PATCH <anything>]".
 The "Subject: " line is supposed to concisely describe what the
 commit is about in one line of text.
 
-"From: " and "Subject: " lines starting the body (the rest of the
-message after the blank line terminating the RFC2822 headers)
-override the respective commit author name and title values taken
-from the headers.
+A line that contains a scissors mark (either ">8" or "8<") and does not
+have anything other than scissors, dash (-), whitespaces or a phrase "cut
+here" is called a scissors line. If such a line appears in the body of the
+message before the patch, everything before it (including the scissors
+line itself) is ignored. This is useful if you want to begin your message
+in a discussion thread with comments and suggestions on the message you
+are responding to, and to conclude it with a patch submission, separating
+the discussion and the beginning of the proposed commit log message with a
+scissors line.
+
+"From: " and "Subject: " lines starting the body override the respective
+commit author name and title values taken from the headers.
 
 The commit message is formed by the title taken from the
 "Subject: ", a blank line and the body of the message up to

Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Patches welcome.

Try this patch, perhaps? I forgot to reset the mysteriously named s_hdr buffer.

Does anybody remember what these s_hdr (vs p_hdr) buffers stand for, by the way?

-- >8 -- cut here -- >8 --
Subject: [PATCH] squashme to 925bd84 (Teach mailinfo to ignore everything before -- >8 -- mark, 2009-08-23)

 builtin-mailinfo.c  |    6 ++++++
 t/t5100/sample.mbox |    4 ++++
 2 files changed, 10 insertions(+), 0 deletions(-)

index 26548f0..8a3a184 100644
@@ -758,9 +758,15 @@ static int handle_commit_msg(struct strbuf *line)
 	}
 
 	if (scissors(line)) {
+		int i;
 		rewind(cmitmsg);
 		ftruncate(fileno(cmitmsg), 0);
 		still_looking = 1;
+		for (i = 0; header[i]; i++) {
+			if (s_hdr_data[i])
+				strbuf_release(s_hdr_data[i]);
+			s_hdr_data[i] = NULL;
+		}
 		return 0;
 	}
 

index 95b6842..2c3da52 100644
@@ -566,10 +566,14 @@ From: Junio Hamano <[email protected]>
 Date: Thu, 20 Aug 2009 17:18:22 -0700
 Subject: Why doesn't git-am does not like >8 scissors mark?
 
+Subject: [PATCH] BLAH ONE
+
 In real life, we will see a discussion that inspired this patch
 discussing related and unrelated things around >8 scissors mark
 in this part of the message.
 
+Subject: [PATCH] BLAH TWO
+
 And the we will see the scissors.
 
 -- >8 -- cut here -- 8< --

Nice. Please add

Has been added by 87ab799234639c .

Nicolas Sebrecht

That much I know ;-), thanks anyway.

The commit does not _explain_ what they are for, what they mean, and what these mysteriously named variables do.

( cc'ing Don Zickus )

Nicolas Sebrecht

original mail headers. s_hdr was supposed to represent the secondary headers, ie the embedded mail headers in the body of the email that could override the original primary mail headers.

I hope that clears things up. Let me know if you have more questions and I will try my best to remember what I did. :-)

Cheers,
Don

Ah, p for primary and s for secondary. Now it makes sense.

Thanks.

Junio C Hamano ([email protected]) wrote on Aug 23, 2009:

I see what you're saying here and your absolutely right. Good thing you didn't write a critique of the XDF_IGNORE_WHITESPACE_CHANGE case. ;)

[...snip...]

Because the flow is much more direct it also makes the test additions to t4015 obsolete as they essentially tested for line end conditions instead of whitespace (like they should have).
[...clip...]

	if (flags & XDF_WHITESPACE_FLAGS)
		return xdl_hash_record_with_whitespace(data, top, flags);

Thell

Your patch 6/6 that added the tests were useful to find a bug I originally had, which is the one below that is commented out.

Also that is how "diff -b" behaves, and that is why I said your tests found a _bug_ in my original. I'll drop the above large comment and replace it with just a "/* -b implies --ignore-space-at-eol */".

Or we can just move the final else clause up and start the function like this:

	int i1, i2;

	if (!(flags & XDF_WHITESPACE_FLAGS))

return s1 == s2 && !memcmp(l1, l2, s1);

	i1 = i2 = 0;
 	if (flags & XDF_IGNORE_WHITESPACE) {
		...

that would get rid of two unnecessary clearing of variables (i1 and i2, even though I suspect that the compiler _could_ optimize them out without such an change), and three flags-bit check in the most common case of not ignoring any whitespaces.

I can amend the counterproposal patches with tests from your 6/6 and add your "Tested-by:" and commit them myself.

Thank _you_ for bringing this issue up in the first place.

Junio C Hamano ([email protected]) wrote on Aug 23, 2009:

That's good to hear!

In that case the only other outstanding issue to being able to use patch-id to validate a whitespace fixed patch is diff's -B option to catch the situations where the original has multiple blank newlines at the end of file.

HA! That's a nifty way to do that with the variables.

Excellent.

My pleasure! It has been quite the learning experience!

Thell

My tentative draft to replace the "how about this" patch further reworks the loop structure and currently looks like this.

It adds net 15 lines but among that 12 lines are comments, which is not so bad.

-- >8 --
Subject: [PATCH] xutils: Fix xdl_recmatch() on incomplete lines

Thell Fowler noticed that various "ignore whitespace" options to git diff do not work well on an incomplete line.

The loop control of the function responsible for these bugs was extremely difficult to follow. This patch restructures the loops for three variants of "ignore whitespace" logic.

The basic idea of the re-written logic is:

- A loop runs while the characters from both strings we are looking at match. We declare unmatch immediately when we find something that does not match and return false from the function. We break out of the loop if we ran out of either side of the string.

The way we skip spaces inside this loop varies depending on the style of ignoring whitespaces.

- After the above loop breaks, we know that the parts of the strings we inspected so far match, ignoring the whitespaces. The lines can match only if the remainder consists of nothing but whitespaces. This part of the logic is shared across all three styles.

The new code is more obvious and should be much easier to follow.

---
 xdiff/xutils.c |   77 +++++++++++++++++++++++++++++++++----------------------
 1 files changed, 46 insertions(+), 31 deletions(-)

index 9411fa9..eb7b597 100644
@@ -190,48 +190,63 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 {
 	int i1, i2;
 
+	if (!(flags & XDF_WHITESPACE_FLAGS))
+		return s1 == s2 && !memcmp(l1, l2, s1);
+
+	i1 = 0;
+	i2 = 0;
+
+	/*
+	 * -w matches everything that matches with -b, and -b in turn
+	 * matches everything that matches with --ignore-space-at-eol.
+	 *
+	 * Each flavor of ignoring needs different logic to skip whitespaces
+	 * while we have both sides to compare.
+	 */
 	if (flags & XDF_IGNORE_WHITESPACE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1]))
-				while (isspace(l1[i1]) && i1 < s1)
-					i1++;
-			if (isspace(l2[i2]))
-				while (isspace(l2[i2]) && i2 < s2)
-					i2++;
-			if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
+		goto skip_ws;
+		while (i1 < s1 && i2 < s2) {
+			if (l1[i1++] != l2[i2++])
 				return 0;
+		skip_ws:
+			while (i1 < s1 && isspace(l1[i1]))
+				i1++;
+			while (i2 < s2 && isspace(l2[i2]))
+				i2++;
 		}
-		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1])) {
-				if (!isspace(l2[i2]))
-					return 0;
-				while (isspace(l1[i1]) && i1 < s1)
-					i1++;
-				while (isspace(l2[i2]) && i2 < s2)
-					i2++;
-			} else if (l1[i1++] != l2[i2++])
-				return 0;
-		}
-		return (i1 >= s1 && i2 >= s2);
-	} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (l1[i1] != l2[i2]) {
+		while (i1 < s1 && i2 < s2) {
+			if (isspace(l1[i1]) && isspace(l2[i2])) {
+				/* Skip matching spaces and try again */
 				while (i1 < s1 && isspace(l1[i1]))
 					i1++;
 				while (i2 < s2 && isspace(l2[i2]))
 					i2++;
-				if (i1 < s1 || i2 < s2)
-					return 0;
-				return 1;
+				continue;
 			}
+			if (l1[i1++] != l2[i2++])
+				return 0;
+		}
+	} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
+		while (i1 < s1 && i2 < s2 && l1[i1++] == l2[i2++])
+			; /* keep going */
+	}
+
+	/*
+	 * After running out of one side, the remaining side must have
+	 * nothing but whitespace for the lines to match.
+	 */
+	if (i1 < s1) {
+		while (i1 < s1 && isspace(l1[i1]))
 			i1++;
+		return (s1 == i1);
+	}
+	if (i2 < s2) {
+		while (i2 < s2 && isspace(l2[i2]))
 			i2++;
-		}
-		return i1 >= s1 && i2 >= s2;
-	} else
-		return s1 == s2 && !memcmp(l1, l2, s1);
+		return (s2 == i2);
+	}
+	return 1;
 }
 
 static unsigned long xdl_hash_record_with_whitespace(char const **data,

1.6.4.1.255.g5556a

Junio C Hamano ([email protected]) wrote on Aug 23, 2009:

It passed every test I threw at it, although it seemed to be a tad bit slower than the previous revision on my sample data so I ran the following command several times for both the previous and current version:

time for i in {1..10}; do ./t4015-diff-whitespace.sh>/dev/null && ./t4015-diff-trailing-whitespace.sh >/dev/null; done

And these results are fairly average on what I saw:

Previous version:
real 2m32.669s
user 0m44.051s
sys 1m34.702s

Current version:
real 2m56.818s
user 0m47.671s
sys 1m46.723s

--
Thell

Do you mean by "previous version" the one that was broken, or the one I sent as a "how about" patch?

Here are the numbers I am getting:

$ /usr/bin/time sh -c 'for i in 1 2 3 4 5 6 7 8 9 0; do  ./t4015-diff-whitespace.sh; done' >/dev/null

----------------

1.99user 3.65system 0:05.10elapsed 110%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+12560outputs (0major+1288675minor)pagefaults 0swaps

1.86user 3.66system 0:05.04elapsed 109%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+12560outputs (0major+1288618minor)pagefaults 0swaps

1.76user 3.87system 0:05.02elapsed 112%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+12560outputs (0major+1288973minor)pagefaults 0swaps

----------------

1.81user 3.86system 0:05.08elapsed 111%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+12560outputs (0major+1288836minor)pagefaults 0swaps

1.76user 3.87system 0:04.95elapsed 113%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+12560outputs (0major+1288880minor)pagefaults 0swaps

1.81user 3.88system 0:05.04elapsed 112%CPU (0avgtext+0avgdata 0maxresident)k 0inputs+12560outputs (0major+1288530minor)pagefaults 0swaps

----------------

One set is with patch and one set is the patch reverted. I cannot quite remember which one is which ;-) but the difference is within the noise for me.

I have to revisit this sometime after getting a long rest.

Junio C Hamano ([email protected]) wrote on Aug 24, 2009:

A quick test shows the version merged to pu is the one that had the fastest times. I'll be away from a connection most of today, but will test the different versions against the tests and some sample data and post back.

--
Thell

Thell Fowler ([email protected]) wrote on Aug 24, 2009:

More extensive testing also shows the version currently in pu is the fastest on my sample data when applied to master. I'm not sure why pu shows slower times than those same commits applied to master, but they are close enough together that I'm guessing no-one would really be concerned.

I was sitting in a waiting room and decided to have a little fun figuring out how to average the sys times...

for arg in "" -w -b --ignore-space-at-eol;do sum=0 && for i in {1..50}; \
do n="$(/usr/bin/time -f "%S" -o /dev/stdout sh -c 'git diff $arg dirty_first>/dev/null;')"; \
sum=$sum+$n; done; echo "scale=2; ($sum)/$i"|echo "$(bc) avg for diff $arg"; done;

pu
.28 avg for diff
.29 avg for diff -w
.33 avg for diff -b
.29 avg for diff --ignore-space-at-eol

pu commits applied to master <=== FASTEST
9c0d402 xutils: Fix xdl_recmatch() on incomplete lines
21245fd xutils: Fix hashing an incomplete line with whitespaces at the end .26 avg for diff
.25 avg for diff -w
.29 avg for diff -b
.31 avg for diff --ignore-space-at-eol

'how about' patch applied to master
.26 avg for diff
.32 avg for diff -w
.29 avg for diff -b
.32 avg for diff --ignore-space-at-eol

current master (in order to see the difference in the basic git diff -ignoring the fact that incomplete lines where broke since it only affects 2 files in the test data)
.30 avg for diff
.30 avg for diff -w
.29 avg for diff -b
.29 avg for diff --ignore-space-at-eol

-- Thell

- Allow xdl_recmatch to recognize and continue processing when at the end of an incomplete line.

Resolves issue with --ignore-space-change when an incomplete line terminated before the eol whitespace handling started by allowing the processing loop to continue when one side has reached the end and includes a check for being at the eol on an incomplete line.

---
 xdiff/xutils.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

index e22b4bb..54bb235 100644
@@ -205,16 +205,27 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 		}
 		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1])) {
-				if (!isspace(l2[i2]))
+		for (i1 = i2 = 0; i1 < s1 || i2 < s2;) {
+			if (isspace(l1[i1]) || i1 == s1) {
+				if (!isspace(l2[i2]) && i2 != s2 && l2[s2] != '\n')
 					return 0;
-				while (isspace(l1[i1]) && i1 < s1)
+				while ((isspace(l1[i1]) && i1 < s1)
+						|| (i1 + 1 == s1 && l1[s1] != '\n'))
 					i1++;
-				while (isspace(l2[i2]) && i2 < s2)
+				while ((isspace(l2[i2]) && i2 < s2)
+						|| (i2 + 1 == s2 && l2[s2] != '\n'))
 					i2++;
-			} else if (l1[i1++] != l2[i2++])
-				return 0;
+			} else {
+				if (l1[i1] != l2[i2] && ((i1 != s1 && l1[s1] != '\n')
+						|| (i2 != s2 && l2[s2] != '\n')))
+					return 0;
+				else {
+					if (i1 < s1)
+						i1++;
+					if (i2 < s2)
+						i2++;
+				}
+			}
 		}
 		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {

1.6.4.176.g556a4

- Allow xdl_recmatch to process to the eof.

Resolves issue with --ignore-space-at-eol processing where an incomplete line terminated processing early before a final check could be done on the other side.

---
 xdiff/xutils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

index 54bb235..3e26488 100644
@@ -229,7 +229,7 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 		}
 		return (i1 >= s1 && i2 >= s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
+		for (i1 = i2 = 0; i1 <= s1 && i2 <= s2; ) {
 			if (l1[i1] != l2[i2]) {
 				while (i1 < s1 && isspace(l1[i1]))
 					i1++;

1.6.4.176.g556a4

- Add 7 --no-index tests to t4015-diff-whitespace.sh to check that ignore options work on incomplete lines.

---
 t/t4015-diff-whitespace.sh |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

index 6d13da3..193ddbe 100755
@@ -395,4 +395,37 @@ test_expect_success 'combined diff with autocrlf conversion' '
 
 '
 
+# Ignore trailing-space testing on incomplete lines.
+prepare_diff_file () {
+	printf "%s%$2s" foo "" >"$1"
+	if [ $3 = "+nl" ]
+	then
+		printf "\n" >>"$1"
+	fi
+}
+
+diff_trailing () {
+	foo="foo___"
+	prepare_diff_file "left" "$2" "$3"
+	lfoo=$( expr substr $foo 1 $((3+$2)) )
+	lfoo=${lfoo}"$3"
+
+	prepare_diff_file "right" "$4" "$5"
+	rfoo=$( expr substr $foo 1 $((3+$4)) )
+	rfoo=${rfoo}"$5"
+
+	label="-$1 $lfoo $rfoo"
+
+	test_expect_success "$label" \
+	"! git diff --no-index -$1 -- left right | grep -q foo"
+}
+
+diff_trailing w 0 +nl 1 -nl
+diff_trailing w 0 -nl 1 -nl
+diff_trailing b 0 +nl 0 -nl
+diff_trailing b 1 +nl 0 -nl
+diff_trailing b 1 -nl 0 -nl
+diff_trailing -ignore-space-at-eol 0 +nl 0 -nl
+diff_trailing -ignore-space-at-eol 2 +nl 2 -nl
+
 test_done

1.6.4.176.g556a4