Help/Advice needed on diff bug in xutils.c

Help/Advice needed on diff bug in xutils.c

[Thell Fowler]

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

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..623da92 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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) {

Re: Help/Advice needed on diff bug in xutils.c

[Johannes Schindelin]

Hi,

On Tue, 4 Aug 2009, Thell Fowler wrote:

> 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

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.

> 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
> 
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 04ad468..623da92 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -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' ) )

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

Re: Help/Advice needed on diff bug in xutils.c

[Thell Fowler]

Johannes Schindelin (Johannes.Schindelin@gmx.de) wrote on Aug 5, 2009:

> On Tue, 4 Aug 2009, Thell Fowler wrote:
>
>> 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.

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.

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

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
where Junio C Hamano wrote:

>>>>	sed -e 's/Z/ /g' >patch3.patch <<\EOF
>>>>        ...
>>>>        +Z 	print_int(func(i));Z
>>>>        EOF
>>>>
>>>> to make invisible SP stand out more for the benefit of people reading 
>>>> the test script (I know you did not have leading SP before HT in 
>>>> yours, but the above illustrates the visibility issues).  For other 
>>>> tests with test vector patches, visibility of whitespace is not much 
>>>> an issue, but this script is _all about_ whitespace, so anything that 
>>>> clarifies what is going on better would help.

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?

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

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

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

I'll give it another go.

Thank you for the advice Dscho.

-- 
Thell

[PATCH/RFC] Add diff tests for trailing-space and now newline

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---

Johannes Schindelin (Johannes.Schindelin@gmx.de) wrote on Aug 5, 2009:
>On Tue, 4 Aug 2009, Thell Fowler wrote:
>
>> 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
>
>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.

Is this more along the right line?

Thell


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

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6d13da3..fddbf20 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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

[PATCH 0/6 RFC] Series to correct xutils incomplete line handling.

[Thell Fowler]

[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

Re: [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.

[Thell Fowler]

Thell Fowler (git@tbfowler.name) wrote on Aug 19, 2009:

>  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

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

Re: [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.

[Alex Riesen]

On Fri, Aug 21, 2009 at 19:39, Thell Fowler<git@tbfowler.name> wrote:
> What is _REALLY_ odd is that it didn't make any tests fail in the test
> dir using master, next, and pu.

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

> 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

This should have worked. Although I prefer just:

  $ make -j4 && make test -j16

Re: [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.

[Thell Fowler]

Alex Riesen (raa.lkml@gmail.com) wrote on Aug 21, 2009:

> On Fri, Aug 21, 2009 at 19:39, Thell Fowler<git@tbfowler.name> wrote:
> > What is _REALLY_ odd is that it didn't make any tests fail in the test
> > dir using master, next, and pu.
> 
> The test suite has a very good coverage, but it surely is not complete.
> 

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.

> > 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
> 
> This should have worked. Although I prefer just:
> 
>   $ make -j4 && make test -j16
> 

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

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

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 t/t4015-diff-trailing-whitespace.sh |   95 +++++++++++++++++++++++++++++++++++
 1 files changed, 95 insertions(+), 0 deletions(-)
 create mode 100755 t/t4015-diff-trailing-whitespace.sh

diff --git a/t/t4015-diff-trailing-whitespace.sh b/t/t4015-diff-trailing-whitespace.sh
new file mode 100755
index 0000000000000000000000000000000000000000..c4937c1b457c24b35565b09e7b262443a05f9795
--- /dev/null
+++ b/t/t4015-diff-trailing-whitespace.sh
@@ -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

[PATCH 2/6] Make xdl_hash_record_with_whitespace ignore eof

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468702209b77427e635370d41001986042ce..c6512a53b08a8c9039614738310aa2786f4fbb1c 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

[PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines.

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index c6512a53b08a8c9039614738310aa2786f4fbb1c..1f28f4fb4e0a8fdc6c9aa1904cf0362dd1e7b977 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

Re: [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines.

[Thell Fowler]

Thell Fowler (git@tbfowler.name) wrote on Aug 19, 2009:

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

The change from '<' to <=' obviously did do jack-diddly-squat, which I 
should have noticed earlier.

>  					i1++;
>  			if (isspace(l2[i2]))
> -				while (isspace(l2[i2]) && i2 < s2)
> +				while (isspace(l2[i2]) && i2 <= s2)

Here too.

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


Those will be corrected for v2.

-- 
Thell

[PATCH 4/6] Make diff -b handle trailing-spaces on incomplete lines.

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 1f28f4fb4e0a8fdc6c9aa1904cf0362dd1e7b977..e126de450c99fb1e557c2cfc0ffe54e8e3e80394 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

[PATCH 5/6] Make diff --ignore-space-at-eol handle incomplete lines.

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index e126de450c99fb1e557c2cfc0ffe54e8e3e80394..a8ed102d528bdb5d8f0839eb392b35dc1c534fba 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

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

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 t/t4015-diff-whitespace.sh |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6d13da30dad5a78fb17a01e86ef33072ea9e6250..193ddbe0659ede17154ffda3b25ebc5e6c686d6e 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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

[PATCH-v2/RFC 0/6] improvements for trailing-space processing on incomplete lines

[Thell Fowler]

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

[PATCH-v2/RFC 1/6] Add supplemental test for trailing-whitespace on incomplete lines

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 t/t4015-diff-trailing-whitespace.sh |   95 +++++++++++++++++++++++++++++++++++
 1 files changed, 95 insertions(+), 0 deletions(-)
 create mode 100755 t/t4015-diff-trailing-whitespace.sh

diff --git a/t/t4015-diff-trailing-whitespace.sh b/t/t4015-diff-trailing-whitespace.sh
new file mode 100755
index 0000000..079fba5
--- /dev/null
+++ b/t/t4015-diff-trailing-whitespace.sh
@@ -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

[PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..c6512a5 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

Re: [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line

[Junio C Hamano]

Thell Fowler <git@tbfowler.name> writes:

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

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

> Signed-off-by: Thell Fowler <git@tbfowler.name>
> ---
>  xdiff/xutils.c |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 04ad468..c6512a5 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -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;

Thanks.

The issue you identified and tried to fix is a worthy one.  But before the
pre-context of this hunk, I notice these lines:

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

If you have trailing whitespaces on an incomplete line, ptr initially
points at the first such whitespace, ptr2 points at the same location, and
then the while() loop advances ptr to point at the last byte on the line,
which in turn will be the last byte of the file.  And the codepath with
your updates still try to access ptr[1] that is beyond that last byte.

I would write it like this patch instead.

The intent is the same as your patch, but it avoids accessing ptr[1] when
that is beyond the end of the buffer, and the logic is easier to follow as
well.

-- >8 --
Subject: xutils: fix hashing an incomplete line with whitespaces at the end

Upon seeing a whitespace, xdl_hash_record_with_whitespace() first skipped
the run of whitespaces (excluding LF) that begins there, ensuring that the
pointer points the last whitespace character in the run, and assumed that
the next character must be LF at the end of the line.  This does not work
when hashing an incomplete line, that lacks the LF at the end.

Introduce "at_eol" variable that is true when either we are at the end of
line (looking at LF) or at the end of an incomplete line, and use that
instead throughout the code.

Noticed by Thell Fowler.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xutils.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..9411fa9 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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;

Re: [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line

[Thell Fowler]

Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:
> Thell Fowler <git@tbfowler.name> writes:
> 
> >   - 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.
> 
> Please study the style of existing commit messages and imitate them.
> 

I'll keep trying.

> > Signed-off-by: Thell Fowler <git@tbfowler.name>
> > ---
> >  xdiff/xutils.c |    4 ++--
> >  1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> > index 04ad468..c6512a5 100644
> > --- a/xdiff/xutils.c
> > +++ b/xdiff/xutils.c
> > @@ -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;
> 
> Thanks.
> 
> The issue you identified and tried to fix is a worthy one.  But before the
> pre-context of this hunk, I notice these lines:
> 
> 		if (isspace(*ptr)) {
> 			const char *ptr2 = ptr;
> 			while (ptr + 1 < top && isspace(ptr[1])
> 					&& ptr[1] != '\n')
> 				ptr++;
> 
> If you have trailing whitespaces on an incomplete line, ptr initially
> points at the first such whitespace, ptr2 points at the same location, and
> then the while() loop advances ptr to point at the last byte on the line,
> which in turn will be the last byte of the file.  And the codepath with
> your updates still try to access ptr[1] that is beyond that last byte.
> 
> I would write it like this patch instead.
> 
> The intent is the same as your patch, but it avoids accessing ptr[1] when
> that is beyond the end of the buffer, and the logic is easier to follow as
> well.
> 

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

> -- >8 --
> Subject: xutils: fix hashing an incomplete line with whitespaces at the end
> 
> Upon seeing a whitespace, xdl_hash_record_with_whitespace() first skipped
> the run of whitespaces (excluding LF) that begins there, ensuring that the
> pointer points the last whitespace character in the run, and assumed that
> the next character must be LF at the end of the line.  This does not work
> when hashing an incomplete line, that lacks the LF at the end.
> 
> Introduce "at_eol" variable that is true when either we are at the end of
> line (looking at LF) or at the end of an incomplete line, and use that
> instead throughout the code.
> 
> Noticed by Thell Fowler.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

-- 
Thell

[PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index c6512a5..e22b4bb 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Thell Fowler <git@tbfowler.name> writes:

> @@ -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'))

This is wrong.  If you ran out l1/s1/i1 but you still have remaining
characters in l2/s2/i2, you do not want to even look at l1[i1].

You can fudge this by sprinkling more "(i1 < s1) &&" in many places (and
reordering how your inner while() loop checks (i1 < s1) and l1[i1]), but I
do not think that is the right direction.

The thing is, the loop control in this function is extremely hard to read
to begin with, and now it is "if we haven't run out both", the complexity
seeps into the inner logic.

How about doing it like this patch instead?  This counterproposal replaces
your 3 patches starting from [3/6].

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

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

The loop control of this function incorrectly handled incomplete lines,
and it was extremely difficult to follow.  This restructures the loops for
three variants of "ignore whitespace" logic.

The basic idea of the re-written logic is this.

 - An initial 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 loop.  And 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 loop, 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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9411fa9..dd8b7e7 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -186,50 +186,93 @@ long xdl_guess_lines(mmfile_t *mf) {
 	return nl + 1;
 }
 
+static int remainder_all_ws(const char *l1, const char *l2,
+			    int i1, int i2, long s1, long s2)
+{
+	if (i1 < s1) {
+		while (i1 < s1 && isspace(l1[i1]))
+			i1++;
+		return (s1 == i1);
+	}
+	if (i2 < s2) {
+		while (i2 < s2 && isspace(l2[i2]))
+			i2++;
+		return (s2 == i2);
+	}
+	return 1;
+}
+
 int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
 {
-	int i1, i2;
+	int i1 = 0, i2 = 0;
 
 	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++])
-				return 0;
+		while (1) {
+			while (i1 < s1 && isspace(l1[i1]))
+				i1++;
+			while (i2 < s2 && isspace(l2[i2]))
+				i2++;
+			if (i1 < s1 && i2 < s2) {
+				if (l1[i1++] != l2[i2++])
+					return 0;
+				continue;
+			}
+			break;
 		}
-		return (i1 >= s1 && i2 >= s2);
+
+		/*
+		 * we ran out one side; the remaining side must be all
+		 * whitespace to match.
+		 */
+		return remainder_all_ws(l1, l2, i1, i2, s1, s2);
 	} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
-		for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
-			if (isspace(l1[i1])) {
-				if (!isspace(l2[i2]))
+		while (1) {
+			if (i1 < s1 && i2 < s2) {
+				/* Skip matching spaces */
+				if (isspace(l1[i1]) && isspace(l2[i2])) {
+					while (i1 < s1 && isspace(l1[i1]))
+						i1++;
+					while (i2 < s2 && isspace(l2[i2]))
+						i2++;
+				}
+			}
+			if (i1 < s1 && i2 < s2) {
+				/*
+				 * We still have both sides; do they match?
+				 */
+				if (l1[i1++] != 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;
+				continue;
+			}
+			break;
 		}
-		return (i1 >= s1 && i2 >= s2);
+
+		/*
+		 * If we do not want -b to imply --ignore-space-at-eol
+		 * then you would need to add this:
+		 *
+		 * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
+		 *	return (s1 <= i1 && s2 <= i2);
+		 *
+		 */
+
+		/*
+		 * we ran out one side; the remaining side must be all
+		 * whitespace to match.
+		 */
+		return remainder_all_ws(l1, l2, i1, i2, s1, 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 && isspace(l1[i1]))
-					i1++;
-				while (i2 < s2 && isspace(l2[i2]))
-					i2++;
-				if (i1 < s1 || i2 < s2)
-					return 0;
-				return 1;
-			}
-			i1++;
-			i2++;
+		while (1) {
+			if (i1 < s1 && i2 < s2 && l1[i1++] == l2[i2++])
+				continue;
+			break;
 		}
-		return i1 >= s1 && i2 >= s2;
+		/*
+		 * we ran out one side; the remaining side must be all
+		 * whitespace to match.
+		 */
+		return remainder_all_ws(l1, l2, i1, i2, s1, s2);
 	} else
 		return s1 == s2 && !memcmp(l1, l2, s1);
 }

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> How about doing it like this patch instead?  This counterproposal replaces
> your 3 patches starting from [3/6].
>
> -- >8 --
> Subject: xutils: Fix xdl_recmatch() on incomplete lines
>
> Thell Fowler noticed that various "ignore whitespace" options to
> git diff does not work well with whitespace glitches on an incomplete
> line.

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/

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>
>
>> How about doing it like this patch instead?  This counterproposal replaces
>> your 3 patches starting from [3/6].
>>
>> -- >8 --
>> Subject: xutils: Fix xdl_recmatch() on incomplete lines
>>
>> Thell Fowler noticed that various "ignore whitespace" options to
>> git diff does not work well with whitespace glitches on an incomplete
>> line.
>
> I think this should be "options to git diff don't work".

Soory, I kant speel; thanks.

> (1) Why do you post patches to the list, instead of committing them
> yourself?

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.

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

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-mailinfo.c |   37 ++++++++++++++++++++++++++++++++++++-
 1 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b0b5d8f..461c47e 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>

> 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.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

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

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b0906ef..38c01e4 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -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/

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

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

Heh.

> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index b0906ef..38c01e4 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -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;

You need a pair of parentheses around the memcmp || memcmp.

I'll squash that in.

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

Nanako Shiraishi (nanako3@lavabit.com) wrote on Aug 23, 2009:

> Quoting Junio C Hamano <gitster@pobox.com>
> 
> > 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.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
> 
> There are left handed people whose scissors run in the wrong direction.
> 

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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Thell Fowler <git@tbfowler.name> writes:

>> > Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
> ...
> BTW - I'm happily using this and think it should be in git!

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.

[PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

Subject: [PATCH] Wong title
From: is this one really the author? <email@somebody.dom>

The 23/08/09, Junio C Hamano wrote:
> 
> >> > Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
> 
> 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.

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

    Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..0c9a791 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -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

Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

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

Patches welcome.

Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

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

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 26548f0..8a3a184 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -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;
 	}
 
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 95b6842..2c3da52 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -566,10 +566,14 @@ From: Junio Hamano <junkio@cox.net>
 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< --

[PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

The 23/08/09, Junio C Hamano wrote:

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

Nice. Please add

	Tested-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>

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

Has been added by 87ab799234639c .

-- 
Nicolas Sebrecht

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

>> Does anybody remember what these s_hdr (vs p_hdr) buffers stand for, by
>> the way?
>
> Has been added by 87ab799234639c .

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.

[PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

( cc'ing Don Zickus )

The 23/08/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> 
> >> Does anybody remember what these s_hdr (vs p_hdr) buffers stand for, by
> >> the way?
> >
> > Has been added by 87ab799234639c .
> 
> 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.

-- 
Nicolas Sebrecht

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Don Zickus]

On Mon, Aug 24, 2009 at 09:31:47AM +0200, Nicolas Sebrecht wrote:
> ( cc'ing Don Zickus )
> 
> The 23/08/09, Junio C Hamano wrote:
> > Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> > 
> > >> Does anybody remember what these s_hdr (vs p_hdr) buffers stand for, by
> > >> the way?

>From what I remember, I used p_hdr to designate primary headers, ie the
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

> > >
> > > Has been added by 87ab799234639c .
> > 
> > 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.
> 
> -- 
> Nicolas Sebrecht

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Don Zickus <dzickus@redhat.com> writes:

> From what I remember, I used p_hdr to designate primary headers, ie the
> 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.

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

Thanks.

Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

[Nanako Shiraishi]

Quoting Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index fcacc94..0c9a791 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -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.

Looking at the way other people use the mark in their messages, I think this explanation isn't correct.

A scissors mark doesn't have to be at the beginning. The line has to contain the mark, and it has to consist of only the mark, '-' minus, the phrase "cut here", and whitespaces.

I am not familiar enough with the code to comment on the bug you are reporting.

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

[PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

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

The 24/08/09, Nanako Shiraishi wrote:

> Looking at the way other people use the mark in their messages, I think this explanation isn't correct.

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

> A scissors mark doesn't have to be at the beginning. The line has to contain the mark, and it has to consist of only the mark, '-' minus, the phrase "cut here", and whitespaces.

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

Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---

This patch supersedes my previous round.

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

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..1d10371 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -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

[PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

( Paste error, sorry. )

The 24/08/09, Nicolas Sebrecht wrote:

> [ Please, please, please, wrap your lines. ]
> 
> The 24/08/09, Nanako Shiraishi wrote:
> 
> > Looking at the way other people use the mark in their messages, I think this explanation isn't correct.
> 
> I'd say that should not document what people do but what the program
> does.
> 
> > A scissors mark doesn't have to be at the beginning. The line has to contain the mark, and it has to consist of only the mark, '-' minus, the phrase "cut here", and whitespaces.
> 
> ...and (">8" or "<8"), you're right. But isn't the following mark a bit
> too much permissive?

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

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..5294d47 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -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

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

> ... But isn't the following mark a bit
> too much permissive?
>
> ->8

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 <gitster@pobox.com>
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< -- ;-)

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 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

diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b0b5d8f..7e09b51 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -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;
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e70ea94..e848556 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -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
diff --git a/t/t5100/info0014 b/t/t5100/info0014
new file mode 100644
index 0000000..ab9c8d0
--- /dev/null
+++ b/t/t5100/info0014
@@ -0,0 +1,5 @@
+Author: Junio C Hamano
+Email: gitster@pobox.com
+Subject: Teach mailinfo to ignore everything before -- >8 -- mark
+Date: Thu, 20 Aug 2009 17:18:22 -0700
+
diff --git a/t/t5100/msg0014 b/t/t5100/msg0014
new file mode 100644
index 0000000..259c6a4
--- /dev/null
+++ b/t/t5100/msg0014
@@ -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 <gitster@pobox.com>
diff --git a/t/t5100/patch0014 b/t/t5100/patch0014
new file mode 100644
index 0000000..124efd2
--- /dev/null
+++ b/t/t5100/patch0014
@@ -0,0 +1,64 @@
+---
+ builtin-mailinfo.c |   37 ++++++++++++++++++++++++++++++++++++-
+ 1 files changed, 36 insertions(+), 1 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index b0b5d8f..461c47e 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -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
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index c3074ac..13fa4ae 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -561,3 +561,92 @@ From: <a.u.thor@example.com> (A U Thor)
 Date: Fri, 9 Jun 2006 00:44:16 -0700
 Subject: [PATCH] a patch
 
+From nobody Mon Sep 17 00:00:00 2001
+From: Junio Hamano <junkio@cox.net>
+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 then we will see the scissors.
+
+ This line is not a scissors mark -- >8 -- but talks about it.
+ - - >8 - - please remove everything above this line - - >8 - -
+
+Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
+From: Junio C Hamano <gitster@pobox.com>
+
+This teaches mailinfo the scissors -- >8 -- mark; the command ignores
+everything before it in the message body.
+
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
+---
+ builtin-mailinfo.c |   37 ++++++++++++++++++++++++++++++++++++-
+ 1 files changed, 36 insertions(+), 1 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index b0b5d8f..461c47e 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -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
-- 
1.6.4.1

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

The 24/08/09, Junio C Hamano wrote:

>                                                                    perhaps                                                                                                
> we should tighten the rules a bit,

<...>

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

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.


Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
 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(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..2773a3e 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -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
diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index 8d95aaa..e16a577 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -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.
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 7e09b51..92319f6 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -6,6 +6,7 @@
 #include "builtin.h"
 #include "utf8.h"
 #include "strbuf.h"
+#include "git-compat-util.h"
 
 static FILE *cmitmsg, *patchfile, *fin, *fout;
 
@@ -25,6 +26,7 @@ static enum  {
 static struct strbuf charset = STRBUF_INIT;
 static int patch_lines;
 static struct strbuf **p_hdr_data, **s_hdr_data;
+static int ignore_scissors = 0;
 
 #define MAX_HDR_PARSED 10
 #define MAX_BOUNDARIES 5
@@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
 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;
+	size_t mark_start = 0, mark_end = 0, mark_len;
+	int scissors_dashes_seen = 0;
 
 	for (i = 0; i < len; i++) {
 		if (isspace(buf[i])) {
-			if (in_perforation) {
-				perforation++;
-				gap++;
-			}
+			if (scissors_dashes_seen)
+				mark_end = i;
 			continue;
 		}
-		last_nonblank = i;
-		if (first_nonblank < 0)
-			first_nonblank = i;
+		if (!scissors_dashes_seen)
+			mark_start = i;
 		if (buf[i] == '-') {
-			in_perforation = 1;
-			perforation++;
+			mark_end = i;
+			scissors_dashes_seen |= 01;
 			continue;
 		}
 		if (i + 1 < len &&
 		    (!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
-			in_perforation = 1;
-			perforation += 2;
-			scissors += 2;
 			i++;
+			mark_end = i;
+			scissors_dashes_seen |= 02;
 			continue;
 		}
-		in_perforation = 0;
+		break;
 	}
 
-	/*
-	 * 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.
-	 */
+	if (scissors_dashes_seen == 03) {
+		/* strip trailing spaces at the end of the mark */
+		for (i = mark_end; i >= mark_start && i <= mark_end; i--) {
+			if (isspace(buf[i]))
+				mark_end--;
+			else
+				break;
+		}
 
-	visible = last_nonblank - first_nonblank + 1;
-	return (scissors && 8 <= visible &&
-		visible < perforation * 3 &&
-		gap * 2 < perforation);
+		mark_len = mark_end - mark_start + 1;
+		if (mark_len >= 8) {
+			/* ignore trailing spaces at the end of the line */
+			len--;
+			for (i = len - 1; i >= 0; i--) {
+				if (isspace(buf[i]))
+					len--;
+				else
+					break;
+			}
+			/*
+			 * The mark is 8 charaters long and contains at least one dash and
+			 * either a ">8" or "<8". Check if the last mark in the line
+			 * matches the first mark found without worrying about what could
+			 * be between them. Only one mark in the whole line is permitted.
+			 */
+			return (!memcmp(buf + mark_start, buf + len - mark_len, mark_len));
+		}
+	}
+
+	return 0;
 }
 
 static int handle_commit_msg(struct strbuf *line)
@@ -782,22 +796,25 @@ 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;
+	if (!ignore_scissors) {
+		if (is_scissors_line(line)) {
+			warning("scissors line found, will skip text above");
+			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;
+			/*
+			 * 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;
 		}
-		return 0;
 	}
 
 	if (patchbreak(line)) {
@@ -1011,6 +1028,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
 	while (1 < argc && argv[1][0] == '-') {
 		if (!strcmp(argv[1], "-k"))
 			keep_subject = 1;
+		else if (!strcmp(argv[1], "--ignore-scissors"))
+			ignore_scissors = 1;
 		else if (!strcmp(argv[1], "-u"))
 			metainfo_charset = def_charset;
 		else if (!strcmp(argv[1], "-n"))
diff --git a/git-am.sh b/git-am.sh
index 3c03f3e..17c883f 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -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

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:

>> 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,
>
> I'm not sure I understand. Are you still open to a patch touching this code
> /too/?

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.

> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 7e09b51..92319f6 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -6,6 +6,7 @@
>  #include "builtin.h"
>  #include "utf8.h"
>  #include "strbuf.h"
> +#include "git-compat-util.h"

Inclusion of builtin.h is designed to be enough.  What do you need this
for?

>  static FILE *cmitmsg, *patchfile, *fin, *fout;
>  
> @@ -25,6 +26,7 @@ static enum  {
>  static struct strbuf charset = STRBUF_INIT;
>  static int patch_lines;
>  static struct strbuf **p_hdr_data, **s_hdr_data;
> +static int ignore_scissors = 0;

Don't initialize a static to 0.

> @@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
>  		if (isspace(buf[i])) {
> +			if (scissors_dashes_seen)
> +				mark_end = i;

I think you do not want this part, and then you won't have to trim
trailing whitespaces from mark_end later.


> +			/*
> +			 * The mark is 8 charaters long and contains at least one dash and
> +			 * either a ">8" or "<8". Check if the last mark in the line
> +			 * matches the first mark found without worrying about what could
> +			 * be between them. Only one mark in the whole line is permitted.
> +			 */

This definition makes "-            8<" a scissors.  

Even though

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

is allowed, so is

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

it does not allow

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

nor

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

nor

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

Oh, did I say I won't waste my time on the definition?  I should have just
discarded this hunk ;-)

> @@ -782,22 +796,25 @@ 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;
> +	if (!ignore_scissors) {
> +		if (is_scissors_line(line)) {
> +			warning("scissors line found, will skip text above");
> ...
> +			return 0;

Don't re-indent like this.  Just do:

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

> -	# -s, -u, -k, --whitespace, -3, -C, -q and -p flags are kept
> +	# Following flags are kept

We seem to have lost the description of what the "Following" are.

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>
>
>> 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 am hoping that you didn't mean to say that other people on the list
> mustn't look at such patches and help improve them either?
>
> Perhaps you can rephrase your message in a more positive way, just like
> you request other people to do in their proposed commit log messages?

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.

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

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

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.

[PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

The 25/08/09, Junio C Hamano wrote:

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

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

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Jakub Narebski]

Nicolas Sebrecht wrote:

> 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

mailsplit.scissors

-- 
Jakub Narebski
Warsaw, Poland
ShadeHawk on #git

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 479 bytes --]

Hi,

On Wed, 26 Aug 2009, Jakub Narebski wrote:

> Nicolas Sebrecht wrote:
> 
> > 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
> 
> mailsplit.scissors

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

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> mailsplit.scissors
>
> 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)?

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.

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Johannes Schindelin]

Hi,

On Wed, 26 Aug 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> mailsplit.scissors
> >
> > 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)?
> 
> 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.

Thank you very much!  This relieves me, indeed.

Ciao,
Dscho

Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Nicolas Sebrecht wrote:
>
>> 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
>
> mailsplit.scissors

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.

[PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark

[Nicolas Sebrecht]

The 25/08/09, Junio C Hamano wrote:

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

Ok, thank you.

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

Could find it at the end of the mails.

> >  #include "builtin.h"
> >  #include "utf8.h"
> >  #include "strbuf.h"
> > +#include "git-compat-util.h"
> 
> Inclusion of builtin.h is designed to be enough.  What do you need this
> for?

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?

> > @@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
> >  		if (isspace(buf[i])) {
> > +			if (scissors_dashes_seen)
> > +				mark_end = i;
> 
> I think you do not want this part, and then you won't have to trim
> trailing whitespaces from mark_end later.

Good eyes.

> > +			/*
> > +			 * The mark is 8 charaters long and contains at least one dash and
> > +			 * either a ">8" or "<8". Check if the last mark in the line
> > +			 * matches the first mark found without worrying about what could
> > +			 * be between them. Only one mark in the whole line is permitted.
> > +			 */
> 
> This definition makes "-            8<" a scissors.  

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

> it does not allow
> 
>     "-- 8< -- please cut here -- 8< -- --"

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

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

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

> > +	if (!ignore_scissors) {
> > +		if (is_scissors_line(line)) {
> > +			warning("scissors line found, will skip text above");
> > ...
> > +			return 0;
> 
> Don't re-indent like this.  Just do:
> 
> 	if (!ignore_scissors && is_scissors_line(line)) {
>         	...
> 	}

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

Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

[Nanako Shiraishi]

Quoting myself...

> A scissors mark doesn't have to be at the beginning. The line has to
> contain the mark, and it has to consist of only the mark, '-' minus, the
> phrase "cut here", and whitespaces.

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.

Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
 Documentation/git-am.txt |   16 ++++++++++++----
 1 files changed, 12 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..fecd5ac 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -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/

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:

> Thell Fowler <git@tbfowler.name> writes:
> 
> > @@ -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'))
> 
> This is wrong.  If you ran out l1/s1/i1 but you still have remaining
> characters in l2/s2/i2, you do not want to even look at l1[i1].
> 
> You can fudge this by sprinkling more "(i1 < s1) &&" in many places (and
> reordering how your inner while() loop checks (i1 < s1) and l1[i1]), but I
> do not think that is the right direction.
> 
> The thing is, the loop control in this function is extremely hard to read
> to begin with, and now it is "if we haven't run out both", the complexity
> seeps into the inner logic.
> 

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

> How about doing it like this patch instead?  This counterproposal replaces
> your 3 patches starting from [3/6].
[...snip...]
> The basic idea of the re-written logic is this.
> 
>  - An initial 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 loop.  And 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 loop, 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.

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 we do not want -b to imply --ignore-space-at-eol
> +		 * then you would need to add this:
> +		 *
> +		 * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
> +		 *	return (s1 <= i1 && s2 <= i2);
> +		 *
> +		 */
> +

While it would be nice to have -b and --ignore-space-at-eol be two 
different options that could be merged together the documentation states 
that -b ignores spaces at eol, and there are scripts that depend on this 
behavior.

IMHO  it is wrong to accept that new spaces where none existed before is 
akin to having one or more existing spaces coalesced.  I seem to recall 
reading something about 1.7 having some changes in it that wouldn't be 
backward compatible; perhaps -b and --ignore-space-at-eol could be 
distinct options for that release.

On another item:
Right now the xdl_recmatch() checks three distinct flags before having the 
opportunity to do the default behavior of a straight diff.  In 
xdl_hash_record there is an initial check for whitespace flags.

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

Perhaps a similar setup for xdl_rematch() and a 
xdl_recmatch_with_whitespace() ?

Lastly:
Since your to counter-proposals give the same results, provide safer and 
faster processing, eliminate the additional test, as well as being easier 
to read and comprehend I propose a v3 with just those two patches.  I'll 
be glad to post it, with or without a xdl_recmatch_with_whitespace, if 
need be.  And should I, or do I need to, add something to the commit (ie: 
ack, tested, ...) ?

Thank you again for taking the time to look at this change!

-- 
Thell

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Thell Fowler <git@tbfowler.name> writes:

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

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.

>> +		/*
>> +		 * If we do not want -b to imply --ignore-space-at-eol
>> +		 * then you would need to add this:
>> +		 *
>> +		 * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
>> +		 *	return (s1 <= i1 && s2 <= i2);
>> +		 *
>> +		 */
>> +
>
> While it would be nice to have -b and --ignore-space-at-eol be two 
> different options that could be merged together the documentation states 
> that -b ignores spaces at eol, and there are scripts that depend on this 
> behavior.

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 */".

> Right now the xdl_recmatch() checks three distinct flags before having the 
> opportunity to do the default behavior of a straight diff.  In 
> xdl_hash_record there is an initial check for whitespace flags.
>
> ...
> 	if (flags & XDF_WHITESPACE_FLAGS)
> 		return xdl_hash_record_with_whitespace(data, top, flags);
> ...
>
> Perhaps a similar setup for xdl_rematch() and a 
> xdl_recmatch_with_whitespace() ?

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.

> Since your to counter-proposals give the same results, provide safer and 
> faster processing, eliminate the additional test, as well as being easier 
> to read and comprehend I propose a v3 with just those two patches.  I'll 
> be glad to post it, with or without a xdl_recmatch_with_whitespace, if 
> need be.  And should I, or do I need to, add something to the commit (ie: 
> ack, tested, ...) ?

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

> Thank you again for taking the time to look at this change!

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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:

> Thell Fowler <git@tbfowler.name> writes:
> 
> > 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).
> 
> 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.
> 

That's good to hear!

> >> +		/*
> >> +		 * If we do not want -b to imply --ignore-space-at-eol
> >> +		 * then you would need to add this:
> >> +		 *
> >> +		 * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
> >> +		 *	return (s1 <= i1 && s2 <= i2);
> >> +		 *
> >> +		 */
> >> +
> >
> > While it would be nice to have -b and --ignore-space-at-eol be two 
> > different options that could be merged together the documentation states 
> > that -b ignores spaces at eol, and there are scripts that depend on this 
> > behavior.
> 
> 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 */".
> 

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.


> > Right now the xdl_recmatch() checks three distinct flags before having the 
> > opportunity to do the default behavior of a straight diff.  In 
> > xdl_hash_record there is an initial check for whitespace flags.
> >
> > ...
> > 	if (flags & XDF_WHITESPACE_FLAGS)
> > 		return xdl_hash_record_with_whitespace(data, top, flags);
> > ...
> >
> > Perhaps a similar setup for xdl_rematch() and a 
> > xdl_recmatch_with_whitespace() ?
> 
> 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.
> 

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

> > Since your to counter-proposals give the same results, provide safer and 
> > faster processing, eliminate the additional test, as well as being easier 
> > to read and comprehend I propose a v3 with just those two patches.  I'll 
> > be glad to post it, with or without a xdl_recmatch_with_whitespace, if 
> > need be.  And should I, or do I need to, add something to the commit (ie: 
> > ack, tested, ...) ?
> 
> I can amend the counterproposal patches with tests from your 6/6 and add
> your "Tested-by:" and commit them myself.
> 

Excellent.

> > Thank you again for taking the time to look at this change!
> 
> Thank _you_ for bringing this issue up in the first place.

My pleasure!  It has been quite the learning experience!

-- 
Thell

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Thell Fowler <git@tbfowler.name> writes:

>> 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.
>> 
>
> HA!  That's a nifty way to do that with the variables.

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.

Tested-by: Thell Fowler <git@tbfowler.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 xdiff/xutils.c |   77 +++++++++++++++++++++++++++++++++----------------------
 1 files changed, 46 insertions(+), 31 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9411fa9..eb7b597 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:

> Thell Fowler <git@tbfowler.name> writes:
> 
> >> 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.
> >> 
> >
> > HA!  That's a nifty way to do that with the variables.
> 
> 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.
> 

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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Junio C Hamano]

Thell Fowler <git@tbfowler.name> writes:

> 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

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.

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

Junio C Hamano (gitster@pobox.com) wrote on Aug 24, 2009:

> Thell Fowler <git@tbfowler.name> writes:
> 
> > 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:
> >
> 
> Do you mean by "previous version" the one that was broken, or the one I
> sent as a "how about" patch?
> 

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

Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line

[Thell Fowler]

Thell Fowler (git@tbfowler.name) wrote on Aug 24, 2009:

> Junio C Hamano (gitster@pobox.com) wrote on Aug 24, 2009:
> 
> > Thell Fowler <git@tbfowler.name> writes:
> > 
> > > 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:
> > >
> > 
> > Do you mean by "previous version" the one that was broken, or the one I
> > sent as a "how about" patch?
> > 
> 
> 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.
> 

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

[PATCH-v2/RFC 4/6] xutils: fix ignore-space-change on incomplete line

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |   25 ++++++++++++++++++-------
 1 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index e22b4bb..54bb235 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

[PATCH-v2/RFC 5/6] xutils: fix ignore-space-at-eol on incomplete line

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 xdiff/xutils.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 54bb235..3e26488 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -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

[PATCH-v2/RFC 6/6] t4015: add tests for trailing-space on incomplete line

[Thell Fowler]

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

Signed-off-by: Thell Fowler <git@tbfowler.name>
---
 t/t4015-diff-whitespace.sh |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6d13da3..193ddbe 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -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