Help/Advice needed on diff bug in xutils.c

Thell Fowler· Aug 4, 2009, 23:33 UTC · lore

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

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

Johannes Schindelin· Aug 5, 2009, 20:45 UTC · re: Thell Fowler · lore

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

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

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

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

Thell Fowler· Aug 10, 2009, 18:54 UTC · re: Johannes Schindelin · lore

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

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.

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

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

> 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

Thell Fowler· Aug 12, 2009, 00:47 UTC · re: Johannes Schindelin · lore

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

  - 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

Thell Fowler· Aug 19, 2009, 23:05 UTC · re: Thell Fowler · lore

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

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

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

Thell Fowler· Aug 21, 2009, 17:39 UTC · re: Thell Fowler · lore

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

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

Perhaps someone can explain what I did wrong when testing?

-- 
Thell

Alex Riesen· Aug 21, 2009, 22:16 UTC · re: Thell Fowler · lore

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

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

Thell Fowler· Aug 22, 2009, 04:23 UTC · re: Alex Riesen · lore

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

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

Thell Fowler· Aug 23, 2009, 03:47 UTC · re: Thell Fowler · lore

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

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

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

Thell Fowler· Aug 23, 2009, 03:49 UTC · re: Thell Fowler · lore

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

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

Thell Fowler· Aug 23, 2009, 03:49 UTC · re: Thell Fowler · lore

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

  - 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

Junio C Hamano· Aug 23, 2009, 07:51 UTC · re: Thell Fowler · lore

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

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.

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

I would write it like this patch instead.

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

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;

Thell Fowler· Aug 23, 2009, 17:02 UTC · re: Junio C Hamano · lore

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

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

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

-- 
Thell

Thell Fowler· Aug 23, 2009, 03:49 UTC · re: Thell Fowler · lore

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

  - 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

Junio C Hamano· Aug 23, 2009, 07:57 UTC · re: Thell Fowler · lore

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

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

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

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

Nanako Shiraishi· Aug 23, 2009, 08:18 UTC · re: Junio C Hamano · lore

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

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.

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

Junio C Hamano· Aug 23, 2009, 08:56 UTC · re: Nanako Shiraishi · lore

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

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?

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

Having said that, I could use something like this.

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

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

Nanako Shiraishi· Aug 23, 2009, 21:07 UTC · re: Junio C Hamano · lore

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

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/

Junio C Hamano· Aug 23, 2009, 21:14 UTC · re: Nanako Shiraishi · lore

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

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.

Thell Fowler· Aug 23, 2009, 22:13 UTC · re: Nanako Shiraishi · lore

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

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

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

-- 
Thell

Junio C Hamano· Aug 23, 2009, 22:30 UTC · re: Thell Fowler · lore

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

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!

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

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

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

Junio C Hamano· Aug 24, 2009, 04:51 UTC · re: Nicolas Sebrecht · lore

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

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.

Junio C Hamano· Aug 24, 2009, 05:36 UTC · re: Junio C Hamano · lore

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

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

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

Junio C Hamano· Aug 24, 2009, 06:58 UTC · re: Nicolas Sebrecht · lore

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

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.

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

( 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

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

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

Junio C Hamano· Aug 24, 2009, 21:48 UTC · re: Don Zickus · lore

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

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.

Nanako Shiraishi· Aug 24, 2009, 05:16 UTC · re: Nicolas Sebrecht · lore

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

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

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

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

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

( 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

Junio C Hamano· Aug 24, 2009, 22:17 UTC · re: Nicolas Sebrecht · lore

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

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

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

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

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark

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

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,

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

Subject: Teach mailinfo to ignore everything before a scissors line

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

  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

Junio C Hamano· Aug 26, 2009, 01:51 UTC · re: Nicolas Sebrecht · lore

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

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

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

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;

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

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

Junio C Hamano· Aug 26, 2009, 03:03 UTC · re: Junio C Hamano · lore

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

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.

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

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.

  scissors.obey

.

-- 
Nicolas Sebrecht

Jakub Narebski· Aug 26, 2009, 08:57 UTC · re: Nicolas Sebrecht · lore

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

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

Johannes Schindelin· Aug 26, 2009, 09:00 UTC · re: Jakub Narebski · lore

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

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

Junio C Hamano· Aug 27, 2009, 05:46 UTC · re: Johannes Schindelin · lore

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

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

Johannes Schindelin· Aug 27, 2009, 10:49 UTC · re: Junio C Hamano · lore

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

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.

Junio C Hamano· Aug 26, 2009, 09:03 UTC · re: Jakub Narebski · lore

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

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.

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

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

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

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

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

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

-- 
Nicolas Sebrecht

Nanako Shiraishi· Aug 24, 2009, 08:09 UTC · re: Nanako Shiraishi · lore

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

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"

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/

Thell Fowler· Aug 23, 2009, 17:01 UTC · re: Junio C Hamano · lore

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

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

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

> +		/*
> +		 * 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);
> +		 *
> +		 */
> +

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

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

-- 
Thell

Junio C Hamano· Aug 23, 2009, 19:40 UTC · re: Thell Fowler · lore

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

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

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

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

	int i1, i2;

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

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

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

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

Thell Fowler· Aug 23, 2009, 20:33 UTC · re: Junio C Hamano · lore

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

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

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

Junio C Hamano· Aug 23, 2009, 21:11 UTC · re: Thell Fowler · lore

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

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.

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

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

Thell Fowler· Aug 24, 2009, 03:26 UTC · re: Junio C Hamano · lore

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

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

And these results are fairly average on what I saw:

Junio C Hamano· Aug 24, 2009, 06:02 UTC · re: Thell Fowler · lore

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

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

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

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

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

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

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

Thell Fowler· Aug 24, 2009, 14:13 UTC · re: Junio C Hamano · lore

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

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

Thell Fowler· Aug 25, 2009, 05:58 UTC · re: Thell Fowler · lore

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

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

-- Thell

Thell Fowler· Aug 23, 2009, 03:49 UTC · re: Thell Fowler · lore

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

  - 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

Thell Fowler· Aug 23, 2009, 03:49 UTC · re: Thell Fowler · lore

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

  - 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

Thell Fowler· Aug 23, 2009, 03:49 UTC · re: Thell Fowler · lore

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

  - 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

Thell Fowler· Aug 19, 2009, 23:06 UTC · lore

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

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

Thell Fowler· Aug 19, 2009, 23:06 UTC · lore

[PATCH 2/6] Make xdl_hash_record_with_whitespace ignore eof

  - 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

Thell Fowler· Aug 19, 2009, 23:07 UTC · lore

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

  - 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

Thell Fowler· Aug 20, 2009, 23:09 UTC · re: Thell Fowler · lore

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

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)

>  					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

Thell Fowler· Aug 19, 2009, 23:07 UTC · lore

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

  - 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

Thell Fowler· Aug 19, 2009, 23:08 UTC · lore

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

  - 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

Thell Fowler· Aug 19, 2009, 23:09 UTC · lore

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

  - 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

Junio C Hamano· Aug 26, 2009, 02:20 UTC · lore

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

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?