* Help/Advice needed on diff bug in xutils.c
@ 2009-08-04 23:33 Thell Fowler
2009-08-05 20:45 ` Johannes Schindelin
` (3 more replies)
0 siblings, 4 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-04 23:33 UTC (permalink / raw)
To: git
Hi all! Please give me a sanity check before I go crazy...
There is a bug in git diff (ignoring whitespace) that does not take into
account a trailing space at the end of a line at the end of a file when
no new line follows.
Here is the example of the bug:
mkdir test_ws_eof
cd test_ws_eof
git init
echo -n "Test" > test.txt
git add .
git commit -m'test'
git symbolic-ref HEAD refs/heads/with_space
rm .git/index
git clean -f
echo -n "Test ">test.txt
git add .
git commit -m'test'
# Ignoring all whitespace there shouldn't be a diff.
git diff -w master -- test.txt
# Ignoring space at eol there shouldn't be a diff
git diff --ignore-space-at-eol master -- test.txt
# Ignoring with -b might have a case for a diff showing.
git diff -b master -- test.txt
In the xutils.c xdl_hash_record_with_whitespace function the trailing
space prior to eof was being calculated into the hash, I fixed that
with the change below, but there is still a difference being noted in
xdl_recmatch because of the size difference.
Before I go changing something that shouldn't be changed could someone
provide some input please?
Thanks for reading,
Thell
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..623da92 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -243,17 +243,17 @@ static unsigned long
xdl_hash_record_with_whitespace(char
const **data,
if (isspace(*ptr)) {
const char *ptr2 = ptr;
while (ptr + 1 < top && isspace(ptr[1])
- && ptr[1] != '\n')
+ && ( ptr[1] != '\n' && ptr[1] !=
'\0' ) )
ptr++;
if (flags & XDF_IGNORE_WHITESPACE)
; /* already handled */
else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
- && ptr[1] != '\n') {
+ && ( ptr[1] != '\n' && ptr[1] !=
'\0' ) ) {
ha += (ha << 5);
ha ^= (unsigned long) ' ';
}
else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
- && ptr[1] != '\n') {
+ && ( ptr[1] != '\n' && ptr[1] !=
'\0' ) ) {
while (ptr2 != ptr + 1) {
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: Help/Advice needed on diff bug in xutils.c
2009-08-04 23:33 Help/Advice needed on diff bug in xutils.c Thell Fowler
@ 2009-08-05 20:45 ` Johannes Schindelin
2009-08-10 18:54 ` Thell Fowler
2009-08-12 0:47 ` [PATCH/RFC] Add diff tests for trailing-space and now newline Thell Fowler
2009-08-19 23:05 ` [PATCH 0/6 RFC] Series to correct xutils incomplete line handling Thell Fowler
` (2 subsequent siblings)
3 siblings, 2 replies; 63+ messages in thread
From: Johannes Schindelin @ 2009-08-05 20:45 UTC (permalink / raw)
To: Thell Fowler; +Cc: git
Hi,
On Tue, 4 Aug 2009, Thell Fowler wrote:
> There is a bug in git diff (ignoring whitespace) that does not take into
> account a trailing space at the end of a line at the end of a file when
> no new line follows.
>
> Here is the example of the bug:
> mkdir test_ws_eof
> cd test_ws_eof
> git init
> echo -n "Test" > test.txt
> git add .
> git commit -m'test'
> git symbolic-ref HEAD refs/heads/with_space
> rm .git/index
> git clean -f
> echo -n "Test ">test.txt
> git add .
> git commit -m'test'
> # Ignoring all whitespace there shouldn't be a diff.
> git diff -w master -- test.txt
> # Ignoring space at eol there shouldn't be a diff
> git diff --ignore-space-at-eol master -- test.txt
> # Ignoring with -b might have a case for a diff showing.
> git diff -b master -- test.txt
If you turn that into a patch to, say, t/t4015-diff-whitespace.sh (adding
a test_expect_failure for a known bug), it is much easier to convince
developers to work on the issue.
> In the xutils.c xdl_hash_record_with_whitespace function the trailing
> space prior to eof was being calculated into the hash, I fixed that with
> the change below, but there is still a difference being noted in
> xdl_recmatch because of the size difference.
>
> Before I go changing something that shouldn't be changed could someone
> provide some input please?
>
> Thanks for reading,
> Thell
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 04ad468..623da92 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -243,17 +243,17 @@ static unsigned long
> xdl_hash_record_with_whitespace(char
> const **data,
> if (isspace(*ptr)) {
> const char *ptr2 = ptr;
> while (ptr + 1 < top && isspace(ptr[1])
> - && ptr[1] != '\n')
> + && ( ptr[1] != '\n' && ptr[1] !=
> '\0' ) )
First, your coding style is different from the surrounding code. I think
it goes without saying that this should be fixed.
Second, you do not need the parentheses at all (and therefore they should
go).
Third, libxdiff does not assume to be fed NUL delimited strings.
Fourth, that condition "ptr + 1 < top" is already doing what you tried to
accomplish here.
So I guess that you need to do add "ptr + 1 < top" checks
instead.
Thanks,
Dscho
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: Help/Advice needed on diff bug in xutils.c
2009-08-05 20:45 ` Johannes Schindelin
@ 2009-08-10 18:54 ` Thell Fowler
2009-08-12 0:47 ` [PATCH/RFC] Add diff tests for trailing-space and now newline Thell Fowler
1 sibling, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-10 18:54 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
Johannes Schindelin (Johannes.Schindelin@gmx.de) wrote on Aug 5, 2009:
> On Tue, 4 Aug 2009, Thell Fowler wrote:
>
>> There is a bug in git diff (ignoring whitespace) that does not take into
>> account a trailing space at the end of a line at the end of a file when
>> no new line follows.
First, please forgive my hubris at thinking I had _found_ a bug when my
ignorance of the issue was so obvious. I am definitely humbled after
reading every post in the archive having to do with whitespace and diff.
>>
>> Here is the example of the bug:
>> mkdir test_ws_eof
>> cd test_ws_eof
>> git init
>> echo -n "Test" > test.txt
>> git add .
>> git commit -m'test'
>> git symbolic-ref HEAD refs/heads/with_space
>> rm .git/index
>> git clean -f
>> echo -n "Test ">test.txt
>> git add .
>> git commit -m'test'
>> # Ignoring all whitespace there shouldn't be a diff.
>> git diff -w master -- test.txt
>> # Ignoring space at eol there shouldn't be a diff
>> git diff --ignore-space-at-eol master -- test.txt
>> # Ignoring with -b might have a case for a diff showing.
>> git diff -b master -- test.txt
>
> If you turn that into a patch to, say, t/t4015-diff-whitespace.sh (adding
> a test_expect_failure for a known bug), it is much easier to convince
> developers to work on the issue.
>
Thank you. In progress.
I am curious if t4015 is planned to be to be rewritten to follow what
Junio had outlined and Giuseppe implemented for
t4107-apply-ignore-whitespace.sh to make the spaces more obvious to the reader:
One other question on the making of the test in regards to the
following quote from:
http://article.gmane.org/gmane.comp.version-control.git/124765
where Junio C Hamano wrote:
>>>> sed -e 's/Z/ /g' >patch3.patch <<\EOF
>>>> ...
>>>> +Z print_int(func(i));Z
>>>> EOF
>>>>
>>>> to make invisible SP stand out more for the benefit of people reading
>>>> the test script (I know you did not have leading SP before HT in
>>>> yours, but the above illustrates the visibility issues). For other
>>>> tests with test vector patches, visibility of whitespace is not much
>>>> an issue, but this script is _all about_ whitespace, so anything that
>>>> clarifies what is going on better would help.
The test being implemented was t4107-apply-ignore-whitespace.sh.
Are there any plans to have t4015-diff-whitespace.sh tests rewritten in
the same fashion?
> First, your coding style is different from the surrounding code. I think
> it goes without saying that this should be fixed.
>
> Second, you do not need the parentheses at all (and therefore they should
> go).
>
> Third, libxdiff does not assume to be fed NUL delimited strings.
>
You're absolutely right, I'll be more aware in the future.
> Fourth, that condition "ptr + 1 < top" is already doing what you tried to
> accomplish here.
>
> So I guess that you need to do add "ptr + 1 < top" checks
> instead.
>
I'll give it another go.
Thank you for the advice Dscho.
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH/RFC] Add diff tests for trailing-space and now newline
2009-08-05 20:45 ` Johannes Schindelin
2009-08-10 18:54 ` Thell Fowler
@ 2009-08-12 0:47 ` Thell Fowler
1 sibling, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-12 0:47 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
- 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.
2009-08-04 23:33 Help/Advice needed on diff bug in xutils.c Thell Fowler
2009-08-05 20:45 ` Johannes Schindelin
@ 2009-08-19 23:05 ` Thell Fowler
2009-08-21 17:39 ` Thell Fowler
[not found] ` <cover.1250719760.git.git@tbfowler.name>
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
3 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:05 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
[1/6]
Add supplemental test for trailing-whitespace on incomplete lines.
-- This patch is for illustrative purposes only. It exposes the current
failures of git diff whitespace ignore options when dealing with trailing-
spaces on incomplete lines.
[2/6] through [5/6]
Make xdl_hash_record_with_whitespace ignore eof
Make diff -w handle trailing-spaces on incomplete lines.
Make diff -b handle trailing-spaces on incomplete lines.
Make diff --ignore-space-at-eol handle incomplete lines.
-- These alter the record ptr loops to go to the end of the record and to
treat the terminator as it would '\n'.
[6/6]
Add diff tests for trailing-space on incomplete lines
-- Just the seven test cases that would identify future breakage.
t/t4015-diff-trailing-whitespace.sh | 95 +++++++++++++++++++++++++++++++++++
t/t4015-diff-whitespace.sh | 33 ++++++++++++
xdiff/xutils.c | 20 ++++----
3 files changed, 138 insertions(+), 10 deletions(-)
create mode 100755 t/t4015-diff-trailing-whitespace.sh
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH 1/6] Add supplemental test for trailing-whitespace on incomplete lines.
[not found] ` <cover.1250719760.git.git@tbfowler.name>
@ 2009-08-19 23:06 ` Thell Fowler
2009-08-19 23:06 ` [PATCH 2/6] Make xdl_hash_record_with_whitespace ignore eof Thell Fowler
` (4 subsequent siblings)
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
*** 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 2/6] Make xdl_hash_record_with_whitespace ignore eof
[not found] ` <cover.1250719760.git.git@tbfowler.name>
2009-08-19 23:06 ` [PATCH 1/6] Add supplemental test for trailing-whitespace on incomplete lines Thell Fowler
@ 2009-08-19 23:06 ` Thell Fowler
2009-08-19 23:07 ` [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines Thell Fowler
` (3 subsequent siblings)
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:06 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
- 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines.
[not found] ` <cover.1250719760.git.git@tbfowler.name>
2009-08-19 23:06 ` [PATCH 1/6] Add supplemental test for trailing-whitespace on incomplete lines Thell Fowler
2009-08-19 23:06 ` [PATCH 2/6] Make xdl_hash_record_with_whitespace ignore eof Thell Fowler
@ 2009-08-19 23:07 ` Thell Fowler
2009-08-20 23:09 ` Thell Fowler
2009-08-19 23:07 ` [PATCH 4/6] Make diff -b " Thell Fowler
` (2 subsequent siblings)
5 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
- 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 4/6] Make diff -b handle trailing-spaces on incomplete lines.
[not found] ` <cover.1250719760.git.git@tbfowler.name>
` (2 preceding siblings ...)
2009-08-19 23:07 ` [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines Thell Fowler
@ 2009-08-19 23:07 ` Thell Fowler
2009-08-19 23:08 ` [PATCH 5/6] Make diff --ignore-space-at-eol handle " Thell Fowler
2009-08-19 23:09 ` [PATCH 6/6] Add diff tests for trailing-space on " Thell Fowler
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:07 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
- 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 5/6] Make diff --ignore-space-at-eol handle incomplete lines.
[not found] ` <cover.1250719760.git.git@tbfowler.name>
` (3 preceding siblings ...)
2009-08-19 23:07 ` [PATCH 4/6] Make diff -b " Thell Fowler
@ 2009-08-19 23:08 ` Thell Fowler
2009-08-19 23:09 ` [PATCH 6/6] Add diff tests for trailing-space on " Thell Fowler
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:08 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
- 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH 6/6] Add diff tests for trailing-space on incomplete lines
[not found] ` <cover.1250719760.git.git@tbfowler.name>
` (4 preceding siblings ...)
2009-08-19 23:08 ` [PATCH 5/6] Make diff --ignore-space-at-eol handle " Thell Fowler
@ 2009-08-19 23:09 ` Thell Fowler
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-19 23:09 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
- 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines.
2009-08-19 23:07 ` [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines Thell Fowler
@ 2009-08-20 23:09 ` Thell Fowler
0 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-20 23:09 UTC (permalink / raw)
To: Thell Fowler; +Cc: Johannes Schindelin, git
Thell Fowler (git@tbfowler.name) wrote on Aug 19, 2009:
> - When processing trailing spaces with --ignore-all-space a diff
> would be found whenever one side had 0 spaces and either (or both)
> sides was an incomplete line. xdl_recmatch should process the
> full length of the record instead of assuming both sides have a
> terminator.
>
> @@ -191,14 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
> int i1, i2;
>
> if (flags & XDF_IGNORE_WHITESPACE) {
> - for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
> + for (i1 = i2 = 0; i1 <= s1 && i2 <= s2; ) {
> if (isspace(l1[i1]))
> - while (isspace(l1[i1]) && i1 < s1)
> + while (isspace(l1[i1]) && i1 <= s1)
The change from '<' to <=' obviously did do jack-diddly-squat, which I
should have noticed earlier.
> i1++;
> if (isspace(l2[i2]))
> - while (isspace(l2[i2]) && i2 < s2)
> + while (isspace(l2[i2]) && i2 <= s2)
Here too.
> i2++;
> - if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
> + if (i1 <= s1 && i2 <= s2 && l1[i1++] != l2[i2++])
> return 0;
> }
> return (i1 >= s1 && i2 >= s2);
>
Those will be corrected for v2.
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.
2009-08-19 23:05 ` [PATCH 0/6 RFC] Series to correct xutils incomplete line handling Thell Fowler
@ 2009-08-21 17:39 ` Thell Fowler
2009-08-21 22:16 ` Alex Riesen
0 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-21 17:39 UTC (permalink / raw)
To: Thell Fowler; +Cc: Johannes Schindelin, git
Thell Fowler (git@tbfowler.name) wrote on Aug 19, 2009:
> t/t4015-diff-trailing-whitespace.sh | 95 +++++++++++++++++++++++++++++++++++
> t/t4015-diff-whitespace.sh | 33 ++++++++++++
> xdiff/xutils.c | 20 ++++----
> 3 files changed, 138 insertions(+), 10 deletions(-)
> create mode 100755 t/t4015-diff-trailing-whitespace.sh
Don't bother trying this series. I tried it out on live data today and it
does not work. It actually caused regression in the diffs for the
conversion project I'm working on.
What is _REALLY_ odd is that it didn't make any tests fail in the test
dir using master, next, and pu.
Perhaps someone can explain what I did wrong when testing?
git checkout master
make -s clean && make -s all && make -s install && cd t && make
I really did do over 9 hours of testing using the test dir. First with
just the branches with no modification, then with the modified
t4015-diff-whitespace.sh, then with the xutils.c patch. And this was on
each branch at about 40 minutes per run through.
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.
2009-08-21 17:39 ` Thell Fowler
@ 2009-08-21 22:16 ` Alex Riesen
2009-08-22 4:23 ` Thell Fowler
0 siblings, 1 reply; 63+ messages in thread
From: Alex Riesen @ 2009-08-21 22:16 UTC (permalink / raw)
To: Thell Fowler; +Cc: Johannes Schindelin, git
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH 0/6 RFC] Series to correct xutils incomplete line handling.
2009-08-21 22:16 ` Alex Riesen
@ 2009-08-22 4:23 ` Thell Fowler
0 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-22 4:23 UTC (permalink / raw)
To: Alex Riesen; +Cc: Thell Fowler, Johannes Schindelin, git
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 0/6] improvements for trailing-space processing on incomplete lines
2009-08-04 23:33 Help/Advice needed on diff bug in xutils.c Thell Fowler
` (2 preceding siblings ...)
[not found] ` <cover.1250719760.git.git@tbfowler.name>
@ 2009-08-23 3:47 ` Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 1/6] Add supplemental test for trailing-whitespace " Thell Fowler
` (5 more replies)
3 siblings, 6 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:47 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
These patches are directly aimed at making it possible to validate the
results of git apply --whitespace=fix which is currently not possible
in files with incomplete lines in various situations (demonstrated in
the test in PATCH 1).
The goal is to be able to validate the application of a patch generated
from a dirty whitespace source to a clean target.
D2 C2
| \/ |
| /\ |
D1 C1
where D1..D2, C1..C2, C1..D2, D1..C2 should all yield the same patch-id
when passed a diff ignoring the applicable whitespace changes.
Applying these patches does nothing to fix the case of differences when
apply --whitespace=fix limits new blank lines at eof as can happen when
foo\r\n\r\n\r\n is fixed to foo\n\n.
Thell Fowler (6):
Add supplemental test for trailing-whitespace on incomplete lines
xutils: fix hash with whitespace on incomplete line
xutils: fix ignore-all-space on incomplete line
xutils: fix ignore-space-change on incomplete line
xutils: fix ignore-space-at-eol on incomplete line
t4015: add tests for trailing-space on incomplete line
t/t4015-diff-trailing-whitespace.sh | 95 +++++++++++++++++++++++++++++++++++
t/t4015-diff-whitespace.sh | 33 ++++++++++++
xdiff/xutils.c | 39 ++++++++++-----
3 files changed, 154 insertions(+), 13 deletions(-)
create mode 100755 t/t4015-diff-trailing-whitespace.sh
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 1/6] Add supplemental test for trailing-whitespace on incomplete lines
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
@ 2009-08-23 3:49 ` Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line Thell Fowler
` (4 subsequent siblings)
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:49 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
*** For illustrative purposes only and not meant for upstream ***
- Adds a stand-alone test that loops through A-side B-side with
and without new-lines from 0 to 3 spaces per side.
This is a draft test meant to expose the issue with xutils.c
handling of incomplete lines and trailing-spaces.
Signed-off-by: Thell Fowler <git@tbfowler.name>
---
t/t4015-diff-trailing-whitespace.sh | 95 +++++++++++++++++++++++++++++++++++
1 files changed, 95 insertions(+), 0 deletions(-)
create mode 100755 t/t4015-diff-trailing-whitespace.sh
diff --git a/t/t4015-diff-trailing-whitespace.sh b/t/t4015-diff-trailing-whitespace.sh
new file mode 100755
index 0000000..079fba5
--- /dev/null
+++ b/t/t4015-diff-trailing-whitespace.sh
@@ -0,0 +1,95 @@
+#!/bin/sh
+
+test_description='Test trailing whitespace in diff engine.
+
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
+
+# Trailing-space testing with and without newlines.
+prepare_diff_file () {
+ printf "%s%$2s" foo "" >"$1"
+ if [ $3 = "+nl" ]
+ then
+ printf "\n" >>"$1"
+ fi
+}
+
+diff_trailing () {
+ foo="foo___"
+ prepare_diff_file "left" "$2" "$3"
+ lfoo=$( expr substr $foo 1 $((3+$2)) )
+ lfoo=${lfoo}"$3"
+
+ prepare_diff_file "right" "$4" "$5"
+ rfoo=$( expr substr $foo 1 $((3+$4)) )
+ rfoo=${rfoo}"$5"
+
+ label="-$1 $lfoo $rfoo ($6)"
+
+ if [ "$6" != "should_diff" ]
+ then
+ negate='!'
+ else
+ negate=''
+ fi
+
+ if [ -z "$7" ]
+ then
+ test_expect_success "$label" \
+ "$negate git diff --no-index -$1 -- left right | grep -q foo"
+ else
+ test_expect_failure "$label" \
+ "$negate git diff --no-index -$1 -- left right | grep -q foo"
+ fi
+
+ test_debug "git diff --no-index -$1 -- left right | grep foo"
+}
+
+touch diffout
+for arg in -ignore-all-space -ignore-space-at-eol -ignore-space-change
+do
+ for i1 in 0 1 2 3
+ do
+ for i2 in 0 1 2 3
+ do
+ diff_trailing $arg $i1 +nl $i2 -nl should_not_diff >> diffout
+ diff_trailing $arg $i1 -nl $i2 +nl should_not_diff >> diffout
+
+ if [ $i1 -ne $i2 ]
+ then
+ diff_trailing $arg $i1 +nl $i2 +nl should_not_diff >> diffout
+ diff_trailing $arg $i1 -nl $i2 -nl should_not_diff >> diffout
+ fi
+ done
+ done
+done
+
+test_debug 'grep "FAIL" diffout'
+
+for arg in all eol change
+do
+ grep "FAIL" diffout | \
+ grep "$arg" | \
+ cut -d " " -f 4- | \
+
+ ## Playing with filtering to isolate core issue.
+ #sort -k 2,2 -k 3,3 | \
+ #awk '{ forward = $2 " " $3; reverse = $3 " " $2}
+ # !seen[forward]++ && !seen[reverse]++' | \
+ #sort -k 2,2 | \
+
+ ## Playing with filtering to isolate core issue.
+ ## This seems like the most illustrative output...
+ awk '{ key=$3 ; gsub(/-/, "+", key) ; key=$2 ":" key ; if ( hash[key]++ == 0 ) print ; }'
+
+ ## Playing with filtering to isolate core issue.
+ #awk '{ if ( $3 ~ /.*\-/ )
+ # print $0
+ # else
+ # print $1 " " $3 " " $2 " " $4
+ # ; }' | \
+ #sort -k 2,2 -k 3,3
+done
+
+test_done
--
1.6.4.176.g556a4
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 1/6] Add supplemental test for trailing-whitespace " Thell Fowler
@ 2009-08-23 3:49 ` Thell Fowler
2009-08-23 7:51 ` Junio C Hamano
2009-08-23 3:49 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space " Thell Fowler
` (3 subsequent siblings)
5 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:49 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
- Make xdl_hash_record_with_whitespace stop hashing before the
eof when ignoring space change or space at eol on an incomplete
line.
Resolves issue with a final trailing space being included in the
hash on an incomplete line by treating the eof in the same fashion
as a newline.
Signed-off-by: Thell Fowler <git@tbfowler.name>
---
xdiff/xutils.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..c6512a5 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -248,12 +248,12 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
if (flags & XDF_IGNORE_WHITESPACE)
; /* already handled */
else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
- && ptr[1] != '\n') {
+ && ptr[1] != '\n' && ptr + 1 < top) {
ha += (ha << 5);
ha ^= (unsigned long) ' ';
}
else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
- && ptr[1] != '\n') {
+ && ptr[1] != '\n' && ptr + 1 < top) {
while (ptr2 != ptr + 1) {
ha += (ha << 5);
ha ^= (unsigned long) *ptr2;
--
1.6.4.176.g556a4
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 1/6] Add supplemental test for trailing-whitespace " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line Thell Fowler
@ 2009-08-23 3:49 ` Thell Fowler
2009-08-23 7:57 ` Junio C Hamano
2009-08-23 3:49 ` [PATCH-v2/RFC 4/6] xutils: fix ignore-space-change " Thell Fowler
` (2 subsequent siblings)
5 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:49 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
- Allow xdl_recmatch to recognize and continue processing when
at the end of an incomplete line.
Resolves issue with --ignore-all-space when either side 1 or 2
has 0 trailing spaces and either (or both) are incomplete by
allowing the processing loop to continue when one side has
reached the end and includes a check for being at eof on an
incomplete line.
Signed-off-by: Thell Fowler <git@tbfowler.name>
---
xdiff/xutils.c | 8 +++++---
1 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index c6512a5..e22b4bb 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -191,12 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
int i1, i2;
if (flags & XDF_IGNORE_WHITESPACE) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
+ for (i1 = i2 = 0; i1 < s1 || i2 < s2; ) {
if (isspace(l1[i1]))
- while (isspace(l1[i1]) && i1 < s1)
+ while ((isspace(l1[i1]) && i1 < s1)
+ || (i1 + 1 == s1 && l1[s1] != '\n'))
i1++;
if (isspace(l2[i2]))
- while (isspace(l2[i2]) && i2 < s2)
+ while ((isspace(l2[i2]) && i2 < s2)
+ || (i2 + 1 == s2 && l2[s2] != '\n'))
i2++;
if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
return 0;
--
1.6.4.176.g556a4
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 4/6] xutils: fix ignore-space-change on incomplete line
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
` (2 preceding siblings ...)
2009-08-23 3:49 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space " Thell Fowler
@ 2009-08-23 3:49 ` Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 5/6] xutils: fix ignore-space-at-eol " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 6/6] t4015: add tests for trailing-space " Thell Fowler
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:49 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
- Allow xdl_recmatch to recognize and continue processing when at
the end of an incomplete line.
Resolves issue with --ignore-space-change when an incomplete line
terminated before the eol whitespace handling started by allowing
the processing loop to continue when one side has reached the end
and includes a check for being at the eol on an incomplete line.
Signed-off-by: Thell Fowler <git@tbfowler.name>
---
xdiff/xutils.c | 25 ++++++++++++++++++-------
1 files changed, 18 insertions(+), 7 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index e22b4bb..54bb235 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -205,16 +205,27 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
}
return (i1 >= s1 && i2 >= s2);
} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (isspace(l1[i1])) {
- if (!isspace(l2[i2]))
+ for (i1 = i2 = 0; i1 < s1 || i2 < s2;) {
+ if (isspace(l1[i1]) || i1 == s1) {
+ if (!isspace(l2[i2]) && i2 != s2 && l2[s2] != '\n')
return 0;
- while (isspace(l1[i1]) && i1 < s1)
+ while ((isspace(l1[i1]) && i1 < s1)
+ || (i1 + 1 == s1 && l1[s1] != '\n'))
i1++;
- while (isspace(l2[i2]) && i2 < s2)
+ while ((isspace(l2[i2]) && i2 < s2)
+ || (i2 + 1 == s2 && l2[s2] != '\n'))
i2++;
- } else if (l1[i1++] != l2[i2++])
- return 0;
+ } else {
+ if (l1[i1] != l2[i2] && ((i1 != s1 && l1[s1] != '\n')
+ || (i2 != s2 && l2[s2] != '\n')))
+ return 0;
+ else {
+ if (i1 < s1)
+ i1++;
+ if (i2 < s2)
+ i2++;
+ }
+ }
}
return (i1 >= s1 && i2 >= s2);
} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
--
1.6.4.176.g556a4
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 5/6] xutils: fix ignore-space-at-eol on incomplete line
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
` (3 preceding siblings ...)
2009-08-23 3:49 ` [PATCH-v2/RFC 4/6] xutils: fix ignore-space-change " Thell Fowler
@ 2009-08-23 3:49 ` Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 6/6] t4015: add tests for trailing-space " Thell Fowler
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:49 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
- Allow xdl_recmatch to process to the eof.
Resolves issue with --ignore-space-at-eol processing where an
incomplete line terminated processing early before a final check
could be done on the other side.
Signed-off-by: Thell Fowler <git@tbfowler.name>
---
xdiff/xutils.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 54bb235..3e26488 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -229,7 +229,7 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
}
return (i1 >= s1 && i2 >= s2);
} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
+ for (i1 = i2 = 0; i1 <= s1 && i2 <= s2; ) {
if (l1[i1] != l2[i2]) {
while (i1 < s1 && isspace(l1[i1]))
i1++;
--
1.6.4.176.g556a4
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH-v2/RFC 6/6] t4015: add tests for trailing-space on incomplete line
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
` (4 preceding siblings ...)
2009-08-23 3:49 ` [PATCH-v2/RFC 5/6] xutils: fix ignore-space-at-eol " Thell Fowler
@ 2009-08-23 3:49 ` Thell Fowler
5 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 3:49 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, Thell Fowler
- Add 7 --no-index tests to t4015-diff-whitespace.sh to check
that ignore options work on incomplete lines.
Signed-off-by: Thell Fowler <git@tbfowler.name>
---
t/t4015-diff-whitespace.sh | 33 +++++++++++++++++++++++++++++++++
1 files changed, 33 insertions(+), 0 deletions(-)
diff --git a/t/t4015-diff-whitespace.sh b/t/t4015-diff-whitespace.sh
index 6d13da3..193ddbe 100755
--- a/t/t4015-diff-whitespace.sh
+++ b/t/t4015-diff-whitespace.sh
@@ -395,4 +395,37 @@ test_expect_success 'combined diff with autocrlf conversion' '
'
+# Ignore trailing-space testing on incomplete lines.
+prepare_diff_file () {
+ printf "%s%$2s" foo "" >"$1"
+ if [ $3 = "+nl" ]
+ then
+ printf "\n" >>"$1"
+ fi
+}
+
+diff_trailing () {
+ foo="foo___"
+ prepare_diff_file "left" "$2" "$3"
+ lfoo=$( expr substr $foo 1 $((3+$2)) )
+ lfoo=${lfoo}"$3"
+
+ prepare_diff_file "right" "$4" "$5"
+ rfoo=$( expr substr $foo 1 $((3+$4)) )
+ rfoo=${rfoo}"$5"
+
+ label="-$1 $lfoo $rfoo"
+
+ test_expect_success "$label" \
+ "! git diff --no-index -$1 -- left right | grep -q foo"
+}
+
+diff_trailing w 0 +nl 1 -nl
+diff_trailing w 0 -nl 1 -nl
+diff_trailing b 0 +nl 0 -nl
+diff_trailing b 1 +nl 0 -nl
+diff_trailing b 1 -nl 0 -nl
+diff_trailing -ignore-space-at-eol 0 +nl 0 -nl
+diff_trailing -ignore-space-at-eol 2 +nl 2 -nl
+
test_done
--
1.6.4.176.g556a4
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line
2009-08-23 3:49 ` [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line Thell Fowler
@ 2009-08-23 7:51 ` Junio C Hamano
2009-08-23 17:02 ` Thell Fowler
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 7:51 UTC (permalink / raw)
To: Thell Fowler; +Cc: git, Johannes.Schindelin
Thell Fowler <git@tbfowler.name> writes:
> - Make xdl_hash_record_with_whitespace stop hashing before the
> eof when ignoring space change or space at eol on an incomplete
> line.
>
> Resolves issue with a final trailing space being included in the
> hash on an incomplete line by treating the eof in the same fashion
> as a newline.
Please study the style of existing commit messages and imitate them.
> Signed-off-by: Thell Fowler <git@tbfowler.name>
> ---
> xdiff/xutils.c | 4 ++--
> 1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> index 04ad468..c6512a5 100644
> --- a/xdiff/xutils.c
> +++ b/xdiff/xutils.c
> @@ -248,12 +248,12 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
> if (flags & XDF_IGNORE_WHITESPACE)
> ; /* already handled */
> else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
> - && ptr[1] != '\n') {
> + && ptr[1] != '\n' && ptr + 1 < top) {
> ha += (ha << 5);
> ha ^= (unsigned long) ' ';
> }
> else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
> - && ptr[1] != '\n') {
> + && ptr[1] != '\n' && ptr + 1 < top) {
> while (ptr2 != ptr + 1) {
> ha += (ha << 5);
> ha ^= (unsigned long) *ptr2;
Thanks.
The issue you identified and tried to fix is a worthy one. But before the
pre-context of this hunk, I notice these lines:
if (isspace(*ptr)) {
const char *ptr2 = ptr;
while (ptr + 1 < top && isspace(ptr[1])
&& ptr[1] != '\n')
ptr++;
If you have trailing whitespaces on an incomplete line, ptr initially
points at the first such whitespace, ptr2 points at the same location, and
then the while() loop advances ptr to point at the last byte on the line,
which in turn will be the last byte of the file. And the codepath with
your updates still try to access ptr[1] that is beyond that last byte.
I would write it like this patch instead.
The intent is the same as your patch, but it avoids accessing ptr[1] when
that is beyond the end of the buffer, and the logic is easier to follow as
well.
-- >8 --
Subject: xutils: fix hashing an incomplete line with whitespaces at the end
Upon seeing a whitespace, xdl_hash_record_with_whitespace() first skipped
the run of whitespaces (excluding LF) that begins there, ensuring that the
pointer points the last whitespace character in the run, and assumed that
the next character must be LF at the end of the line. This does not work
when hashing an incomplete line, that lacks the LF at the end.
Introduce "at_eol" variable that is true when either we are at the end of
line (looking at LF) or at the end of an incomplete line, and use that
instead throughout the code.
Noticed by Thell Fowler.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
xdiff/xutils.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 04ad468..9411fa9 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -242,18 +242,20 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
for (; ptr < top && *ptr != '\n'; ptr++) {
if (isspace(*ptr)) {
const char *ptr2 = ptr;
+ int at_eol;
while (ptr + 1 < top && isspace(ptr[1])
&& ptr[1] != '\n')
ptr++;
+ at_eol = (top <= ptr + 1 || ptr[1] == '\n');
if (flags & XDF_IGNORE_WHITESPACE)
; /* already handled */
else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
- && ptr[1] != '\n') {
+ && !at_eol) {
ha += (ha << 5);
ha ^= (unsigned long) ' ';
}
else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
- && ptr[1] != '\n') {
+ && !at_eol) {
while (ptr2 != ptr + 1) {
ha += (ha << 5);
ha ^= (unsigned long) *ptr2;
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 3:49 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space " Thell Fowler
@ 2009-08-23 7:57 ` Junio C Hamano
2009-08-23 8:18 ` Nanako Shiraishi
2009-08-23 17:01 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line Thell Fowler
0 siblings, 2 replies; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 7:57 UTC (permalink / raw)
To: Thell Fowler; +Cc: git, Johannes.Schindelin
Thell Fowler <git@tbfowler.name> writes:
> @@ -191,12 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
> int i1, i2;
>
> if (flags & XDF_IGNORE_WHITESPACE) {
> - for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
> + for (i1 = i2 = 0; i1 < s1 || i2 < s2; ) {
> if (isspace(l1[i1]))
> - while (isspace(l1[i1]) && i1 < s1)
> + while ((isspace(l1[i1]) && i1 < s1)
> + || (i1 + 1 == s1 && l1[s1] != '\n'))
This is wrong. If you ran out l1/s1/i1 but you still have remaining
characters in l2/s2/i2, you do not want to even look at l1[i1].
You can fudge this by sprinkling more "(i1 < s1) &&" in many places (and
reordering how your inner while() loop checks (i1 < s1) and l1[i1]), but I
do not think that is the right direction.
The thing is, the loop control in this function is extremely hard to read
to begin with, and now it is "if we haven't run out both", the complexity
seeps into the inner logic.
How about doing it like this patch instead? This counterproposal replaces
your 3 patches starting from [3/6].
-- >8 --
Subject: xutils: Fix xdl_recmatch() on incomplete lines
Thell Fowler noticed that various "ignore whitespace" options to
git diff does not work well with whitespace glitches on an incomplete
line.
The loop control of this function incorrectly handled incomplete lines,
and it was extremely difficult to follow. This restructures the loops for
three variants of "ignore whitespace" logic.
The basic idea of the re-written logic is this.
- An initial loop runs while the characters from both strings we are
looking at match. We declare unmatch immediately when we find
something that does not match and return false from the loop. And we
break out of the loop if we ran out of either side of the string.
The way we skip spaces inside this loop varies depending on the style
of ignoring whitespaces.
- After the loop, the lines can match only if the remainder consists of
nothing but whitespaces. This part of the logic is shared across all
three styles.
The new code is more obvious and should be much easier to follow.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
xdiff/xutils.c | 111 +++++++++++++++++++++++++++++++++++++++-----------------
1 files changed, 77 insertions(+), 34 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9411fa9..dd8b7e7 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -186,50 +186,93 @@ long xdl_guess_lines(mmfile_t *mf) {
return nl + 1;
}
+static int remainder_all_ws(const char *l1, const char *l2,
+ int i1, int i2, long s1, long s2)
+{
+ if (i1 < s1) {
+ while (i1 < s1 && isspace(l1[i1]))
+ i1++;
+ return (s1 == i1);
+ }
+ if (i2 < s2) {
+ while (i2 < s2 && isspace(l2[i2]))
+ i2++;
+ return (s2 == i2);
+ }
+ return 1;
+}
+
int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
{
- int i1, i2;
+ int i1 = 0, i2 = 0;
if (flags & XDF_IGNORE_WHITESPACE) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (isspace(l1[i1]))
- while (isspace(l1[i1]) && i1 < s1)
- i1++;
- if (isspace(l2[i2]))
- while (isspace(l2[i2]) && i2 < s2)
- i2++;
- if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
- return 0;
+ while (1) {
+ while (i1 < s1 && isspace(l1[i1]))
+ i1++;
+ while (i2 < s2 && isspace(l2[i2]))
+ i2++;
+ if (i1 < s1 && i2 < s2) {
+ if (l1[i1++] != l2[i2++])
+ return 0;
+ continue;
+ }
+ break;
}
- return (i1 >= s1 && i2 >= s2);
+
+ /*
+ * we ran out one side; the remaining side must be all
+ * whitespace to match.
+ */
+ return remainder_all_ws(l1, l2, i1, i2, s1, s2);
} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (isspace(l1[i1])) {
- if (!isspace(l2[i2]))
+ while (1) {
+ if (i1 < s1 && i2 < s2) {
+ /* Skip matching spaces */
+ if (isspace(l1[i1]) && isspace(l2[i2])) {
+ while (i1 < s1 && isspace(l1[i1]))
+ i1++;
+ while (i2 < s2 && isspace(l2[i2]))
+ i2++;
+ }
+ }
+ if (i1 < s1 && i2 < s2) {
+ /*
+ * We still have both sides; do they match?
+ */
+ if (l1[i1++] != l2[i2++])
return 0;
- while (isspace(l1[i1]) && i1 < s1)
- i1++;
- while (isspace(l2[i2]) && i2 < s2)
- i2++;
- } else if (l1[i1++] != l2[i2++])
- return 0;
+ continue;
+ }
+ break;
}
- return (i1 >= s1 && i2 >= s2);
+
+ /*
+ * If we do not want -b to imply --ignore-space-at-eol
+ * then you would need to add this:
+ *
+ * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
+ * return (s1 <= i1 && s2 <= i2);
+ *
+ */
+
+ /*
+ * we ran out one side; the remaining side must be all
+ * whitespace to match.
+ */
+ return remainder_all_ws(l1, l2, i1, i2, s1, s2);
+
} else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (l1[i1] != l2[i2]) {
- while (i1 < s1 && isspace(l1[i1]))
- i1++;
- while (i2 < s2 && isspace(l2[i2]))
- i2++;
- if (i1 < s1 || i2 < s2)
- return 0;
- return 1;
- }
- i1++;
- i2++;
+ while (1) {
+ if (i1 < s1 && i2 < s2 && l1[i1++] == l2[i2++])
+ continue;
+ break;
}
- return i1 >= s1 && i2 >= s2;
+ /*
+ * we ran out one side; the remaining side must be all
+ * whitespace to match.
+ */
+ return remainder_all_ws(l1, l2, i1, i2, s1, s2);
} else
return s1 == s2 && !memcmp(l1, l2, s1);
}
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 7:57 ` Junio C Hamano
@ 2009-08-23 8:18 ` Nanako Shiraishi
2009-08-23 8:56 ` Junio C Hamano
2009-08-23 17:01 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line Thell Fowler
1 sibling, 1 reply; 63+ messages in thread
From: Nanako Shiraishi @ 2009-08-23 8:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
Quoting Junio C Hamano <gitster@pobox.com>
> How about doing it like this patch instead? This counterproposal replaces
> your 3 patches starting from [3/6].
>
> -- >8 --
> Subject: xutils: Fix xdl_recmatch() on incomplete lines
>
> Thell Fowler noticed that various "ignore whitespace" options to
> git diff does not work well with whitespace glitches on an incomplete
> line.
I think this should be "options to git diff don't work".
I have two unrelated questions.
(1) Why do you post patches to the list, instead of committing them yourself?
(2) How do I apply a patch like this one to try to my tree? Am I expected to edit the mail message to remove everything before the shears mark before running the git-am command?
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 8:18 ` Nanako Shiraishi
@ 2009-08-23 8:56 ` Junio C Hamano
2009-08-23 21:07 ` Nanako Shiraishi
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 8:56 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Thell Fowler, git, Johannes.Schindelin
Nanako Shiraishi <nanako3@lavabit.com> writes:
> Quoting Junio C Hamano <gitster@pobox.com>
>
>> How about doing it like this patch instead? This counterproposal replaces
>> your 3 patches starting from [3/6].
>>
>> -- >8 --
>> Subject: xutils: Fix xdl_recmatch() on incomplete lines
>>
>> Thell Fowler noticed that various "ignore whitespace" options to
>> git diff does not work well with whitespace glitches on an incomplete
>> line.
>
> I think this should be "options to git diff don't work".
Soory, I kant speel; thanks.
> (1) Why do you post patches to the list, instead of committing them
> yourself?
So that others can catch silly mistakes of mine, like the one you just
caught.
I play three separate roles here, two of which I should send patches out
while playing them.
* Just like everybody else, I find itches to scratch from time to time,
and I build my own topic branches locally for the changes to scratch
them, just like other contributors.
They are indeed committed and often immediately merged to 'pu', but I
send out format-patch output for them, because I firmly believe that
the development _process_, not just the end result, should be in the
open. Everybody's patch should go through the list, get reviewed and
improved by help from others. So should mine.
* I read others' patches, review, comment, and suggest improvements and
make counterproposals, just like others on the list.
The "how about" patches when I am playing this role are often not meant
as the final shape of the patch but to show the direction to improve
upon. They are output from "git diff", not format-patch nor even "git
diff --cached"---I do not commit, nor even add them to the index---and
after I send out e-mails, I typically reset them away to work on
something else, because they are usually not my itch.
* I accept patches that were reviewed favorably on the list by running
"git am" on them.
> (2) How do I apply a patch like this one to try to my tree? Am I
> expected to edit the mail message to remove everything before the shears
> mark before running the git-am command?
That is how I have been doing it. My workflow is:
(1) First read patches in my primary mailbox, while copying promising
ones to a separate mailbox;
(2) And then go through the separate mailbox as a separate pass, while
fixing obvious typos and minor coding style violations still inside
mailbox; and finally
(3) Run "git am" on the (possibly edited) patch to apply.
Because I'll be editing the messages (both log and code) _anyway_,
removing everything before the scissors mark is not much of a trouble.
Having said that, I could use something like this.
-- >8 -- cut here -- >8 --
Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
This teaches mailinfo the scissors -- >8 -- mark; the command ignores
everything before it in the message body.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-mailinfo.c | 37 ++++++++++++++++++++++++++++++++++++-
1 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b0b5d8f..461c47e 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -712,6 +712,34 @@ static inline int patchbreak(const struct strbuf *line)
return 0;
}
+static int scissors(const struct strbuf *line)
+{
+ size_t i, len = line->len;
+ int scissors_dashes_seen = 0;
+ const char *buf = line->buf;
+
+ for (i = 0; i < len; i++) {
+ if (isspace(buf[i]))
+ continue;
+ if (buf[i] == '-') {
+ scissors_dashes_seen |= 02;
+ continue;
+ }
+ if (i + 1 < len && !memcmp(buf + i, ">8", 2)) {
+ scissors_dashes_seen |= 01;
+ i++;
+ continue;
+ }
+ if (i + 7 < len && !memcmp(buf + i, "cut here", 8)) {
+ i += 7;
+ continue;
+ }
+ /* everything else --- not scissors */
+ break;
+ }
+ return scissors_dashes_seen == 03;
+}
+
static int handle_commit_msg(struct strbuf *line)
{
static int still_looking = 1;
@@ -723,10 +751,17 @@ static int handle_commit_msg(struct strbuf *line)
strbuf_ltrim(line);
if (!line->len)
return 0;
- if ((still_looking = check_header(line, s_hdr_data, 0)) != 0)
+ still_looking = check_header(line, s_hdr_data, 0);
+ if (still_looking)
return 0;
}
+ if (scissors(line)) {
+ fseek(cmitmsg, 0L, SEEK_SET);
+ still_looking = 1;
+ return 0;
+ }
+
/* normalize the log message to UTF-8. */
if (metainfo_charset)
convert_to_utf8(line, charset.buf);
--
1.6.4.1
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 7:57 ` Junio C Hamano
2009-08-23 8:18 ` Nanako Shiraishi
@ 2009-08-23 17:01 ` Thell Fowler
2009-08-23 19:40 ` Junio C Hamano
1 sibling, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 17:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:
> Thell Fowler <git@tbfowler.name> writes:
>
> > @@ -191,12 +191,14 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
> > int i1, i2;
> >
> > if (flags & XDF_IGNORE_WHITESPACE) {
> > - for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
> > + for (i1 = i2 = 0; i1 < s1 || i2 < s2; ) {
> > if (isspace(l1[i1]))
> > - while (isspace(l1[i1]) && i1 < s1)
> > + while ((isspace(l1[i1]) && i1 < s1)
> > + || (i1 + 1 == s1 && l1[s1] != '\n'))
>
> This is wrong. If you ran out l1/s1/i1 but you still have remaining
> characters in l2/s2/i2, you do not want to even look at l1[i1].
>
> You can fudge this by sprinkling more "(i1 < s1) &&" in many places (and
> reordering how your inner while() loop checks (i1 < s1) and l1[i1]), but I
> do not think that is the right direction.
>
> The thing is, the loop control in this function is extremely hard to read
> to begin with, and now it is "if we haven't run out both", the complexity
> seeps into the inner logic.
>
I see what you're saying here and your absolutely right. Good thing you
didn't write a critique of the XDF_IGNORE_WHITESPACE_CHANGE case. ;)
> How about doing it like this patch instead? This counterproposal replaces
> your 3 patches starting from [3/6].
[...snip...]
> The basic idea of the re-written logic is this.
>
> - An initial loop runs while the characters from both strings we are
> looking at match. We declare unmatch immediately when we find
> something that does not match and return false from the loop. And we
> break out of the loop if we ran out of either side of the string.
>
> The way we skip spaces inside this loop varies depending on the style
> of ignoring whitespaces.
>
> - After the loop, the lines can match only if the remainder consists of
> nothing but whitespaces. This part of the logic is shared across all
> three styles.
>
> The new code is more obvious and should be much easier to follow.
Because the flow is much more direct it also makes the test additions to
t4015 obsolete as they essentially tested for line end conditions instead
of whitespace (like they should have).
[...clip...]
> + /*
> + * If we do not want -b to imply --ignore-space-at-eol
> + * then you would need to add this:
> + *
> + * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
> + * return (s1 <= i1 && s2 <= i2);
> + *
> + */
> +
While it would be nice to have -b and --ignore-space-at-eol be two
different options that could be merged together the documentation states
that -b ignores spaces at eol, and there are scripts that depend on this
behavior.
IMHO it is wrong to accept that new spaces where none existed before is
akin to having one or more existing spaces coalesced. I seem to recall
reading something about 1.7 having some changes in it that wouldn't be
backward compatible; perhaps -b and --ignore-space-at-eol could be
distinct options for that release.
On another item:
Right now the xdl_recmatch() checks three distinct flags before having the
opportunity to do the default behavior of a straight diff. In
xdl_hash_record there is an initial check for whitespace flags.
...
if (flags & XDF_WHITESPACE_FLAGS)
return xdl_hash_record_with_whitespace(data, top, flags);
...
Perhaps a similar setup for xdl_rematch() and a
xdl_recmatch_with_whitespace() ?
Lastly:
Since your to counter-proposals give the same results, provide safer and
faster processing, eliminate the additional test, as well as being easier
to read and comprehend I propose a v3 with just those two patches. I'll
be glad to post it, with or without a xdl_recmatch_with_whitespace, if
need be. And should I, or do I need to, add something to the commit (ie:
ack, tested, ...) ?
Thank you again for taking the time to look at this change!
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line
2009-08-23 7:51 ` Junio C Hamano
@ 2009-08-23 17:02 ` Thell Fowler
0 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 17:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:
> Thell Fowler <git@tbfowler.name> writes:
>
> > - Make xdl_hash_record_with_whitespace stop hashing before the
> > eof when ignoring space change or space at eol on an incomplete
> > line.
> >
> > Resolves issue with a final trailing space being included in the
> > hash on an incomplete line by treating the eof in the same fashion
> > as a newline.
>
> Please study the style of existing commit messages and imitate them.
>
I'll keep trying.
> > Signed-off-by: Thell Fowler <git@tbfowler.name>
> > ---
> > xdiff/xutils.c | 4 ++--
> > 1 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/xdiff/xutils.c b/xdiff/xutils.c
> > index 04ad468..c6512a5 100644
> > --- a/xdiff/xutils.c
> > +++ b/xdiff/xutils.c
> > @@ -248,12 +248,12 @@ static unsigned long xdl_hash_record_with_whitespace(char const **data,
> > if (flags & XDF_IGNORE_WHITESPACE)
> > ; /* already handled */
> > else if (flags & XDF_IGNORE_WHITESPACE_CHANGE
> > - && ptr[1] != '\n') {
> > + && ptr[1] != '\n' && ptr + 1 < top) {
> > ha += (ha << 5);
> > ha ^= (unsigned long) ' ';
> > }
> > else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL
> > - && ptr[1] != '\n') {
> > + && ptr[1] != '\n' && ptr + 1 < top) {
> > while (ptr2 != ptr + 1) {
> > ha += (ha << 5);
> > ha ^= (unsigned long) *ptr2;
>
> Thanks.
>
> The issue you identified and tried to fix is a worthy one. But before the
> pre-context of this hunk, I notice these lines:
>
> if (isspace(*ptr)) {
> const char *ptr2 = ptr;
> while (ptr + 1 < top && isspace(ptr[1])
> && ptr[1] != '\n')
> ptr++;
>
> If you have trailing whitespaces on an incomplete line, ptr initially
> points at the first such whitespace, ptr2 points at the same location, and
> then the while() loop advances ptr to point at the last byte on the line,
> which in turn will be the last byte of the file. And the codepath with
> your updates still try to access ptr[1] that is beyond that last byte.
>
> I would write it like this patch instead.
>
> The intent is the same as your patch, but it avoids accessing ptr[1] when
> that is beyond the end of the buffer, and the logic is easier to follow as
> well.
>
I appreciate your taking the time to look at the issue and explaining the
reasons for your change.
> -- >8 --
> Subject: xutils: fix hashing an incomplete line with whitespaces at the end
>
> Upon seeing a whitespace, xdl_hash_record_with_whitespace() first skipped
> the run of whitespaces (excluding LF) that begins there, ensuring that the
> pointer points the last whitespace character in the run, and assumed that
> the next character must be LF at the end of the line. This does not work
> when hashing an incomplete line, that lacks the LF at the end.
>
> Introduce "at_eol" variable that is true when either we are at the end of
> line (looking at LF) or at the end of an incomplete line, and use that
> instead throughout the code.
>
> Noticed by Thell Fowler.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Yeah... comparing this commit message to the original shows a pretty stark
difference. I'll get it 'the git way' eventually.
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 17:01 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line Thell Fowler
@ 2009-08-23 19:40 ` Junio C Hamano
2009-08-23 20:33 ` Thell Fowler
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 19:40 UTC (permalink / raw)
To: Thell Fowler; +Cc: git, Johannes.Schindelin
Thell Fowler <git@tbfowler.name> writes:
> Because the flow is much more direct it also makes the test additions to
> t4015 obsolete as they essentially tested for line end conditions instead
> of whitespace (like they should have).
Your patch 6/6 that added the tests were useful to find a bug I originally
had, which is the one below that is commented out.
>> + /*
>> + * If we do not want -b to imply --ignore-space-at-eol
>> + * then you would need to add this:
>> + *
>> + * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
>> + * return (s1 <= i1 && s2 <= i2);
>> + *
>> + */
>> +
>
> While it would be nice to have -b and --ignore-space-at-eol be two
> different options that could be merged together the documentation states
> that -b ignores spaces at eol, and there are scripts that depend on this
> behavior.
Also that is how "diff -b" behaves, and that is why I said your tests
found a _bug_ in my original. I'll drop the above large comment and
replace it with just a "/* -b implies --ignore-space-at-eol */".
> Right now the xdl_recmatch() checks three distinct flags before having the
> opportunity to do the default behavior of a straight diff. In
> xdl_hash_record there is an initial check for whitespace flags.
>
> ...
> if (flags & XDF_WHITESPACE_FLAGS)
> return xdl_hash_record_with_whitespace(data, top, flags);
> ...
>
> Perhaps a similar setup for xdl_rematch() and a
> xdl_recmatch_with_whitespace() ?
Or we can just move the final else clause up and start the function like
this:
int i1, i2;
if (!(flags & XDF_WHITESPACE_FLAGS))
return s1 == s2 && !memcmp(l1, l2, s1);
i1 = i2 = 0;
if (flags & XDF_IGNORE_WHITESPACE) {
...
that would get rid of two unnecessary clearing of variables (i1 and i2,
even though I suspect that the compiler _could_ optimize them out without
such an change), and three flags-bit check in the most common case of not
ignoring any whitespaces.
> Since your to counter-proposals give the same results, provide safer and
> faster processing, eliminate the additional test, as well as being easier
> to read and comprehend I propose a v3 with just those two patches. I'll
> be glad to post it, with or without a xdl_recmatch_with_whitespace, if
> need be. And should I, or do I need to, add something to the commit (ie:
> ack, tested, ...) ?
I can amend the counterproposal patches with tests from your 6/6 and add
your "Tested-by:" and commit them myself.
> Thank you again for taking the time to look at this change!
Thank _you_ for bringing this issue up in the first place.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 19:40 ` Junio C Hamano
@ 2009-08-23 20:33 ` Thell Fowler
2009-08-23 21:11 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 20:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:
> Thell Fowler <git@tbfowler.name> writes:
>
> > Because the flow is much more direct it also makes the test additions to
> > t4015 obsolete as they essentially tested for line end conditions instead
> > of whitespace (like they should have).
>
> Your patch 6/6 that added the tests were useful to find a bug I originally
> had, which is the one below that is commented out.
>
That's good to hear!
> >> + /*
> >> + * If we do not want -b to imply --ignore-space-at-eol
> >> + * then you would need to add this:
> >> + *
> >> + * if (!(flags & XDF_IGNORE_WHITESPACE_AT_EOL))
> >> + * return (s1 <= i1 && s2 <= i2);
> >> + *
> >> + */
> >> +
> >
> > While it would be nice to have -b and --ignore-space-at-eol be two
> > different options that could be merged together the documentation states
> > that -b ignores spaces at eol, and there are scripts that depend on this
> > behavior.
>
> Also that is how "diff -b" behaves, and that is why I said your tests
> found a _bug_ in my original. I'll drop the above large comment and
> replace it with just a "/* -b implies --ignore-space-at-eol */".
>
In that case the only other outstanding issue to being able to use
patch-id to validate a whitespace fixed patch is diff's -B option to catch
the situations where the original has multiple blank newlines at the end
of file.
> > Right now the xdl_recmatch() checks three distinct flags before having the
> > opportunity to do the default behavior of a straight diff. In
> > xdl_hash_record there is an initial check for whitespace flags.
> >
> > ...
> > if (flags & XDF_WHITESPACE_FLAGS)
> > return xdl_hash_record_with_whitespace(data, top, flags);
> > ...
> >
> > Perhaps a similar setup for xdl_rematch() and a
> > xdl_recmatch_with_whitespace() ?
>
> Or we can just move the final else clause up and start the function like
> this:
>
> int i1, i2;
>
> if (!(flags & XDF_WHITESPACE_FLAGS))
> return s1 == s2 && !memcmp(l1, l2, s1);
>
> i1 = i2 = 0;
> if (flags & XDF_IGNORE_WHITESPACE) {
> ...
>
> that would get rid of two unnecessary clearing of variables (i1 and i2,
> even though I suspect that the compiler _could_ optimize them out without
> such an change), and three flags-bit check in the most common case of not
> ignoring any whitespaces.
>
HA! That's a nifty way to do that with the variables.
> > Since your to counter-proposals give the same results, provide safer and
> > faster processing, eliminate the additional test, as well as being easier
> > to read and comprehend I propose a v3 with just those two patches. I'll
> > be glad to post it, with or without a xdl_recmatch_with_whitespace, if
> > need be. And should I, or do I need to, add something to the commit (ie:
> > ack, tested, ...) ?
>
> I can amend the counterproposal patches with tests from your 6/6 and add
> your "Tested-by:" and commit them myself.
>
Excellent.
> > Thank you again for taking the time to look at this change!
>
> Thank _you_ for bringing this issue up in the first place.
My pleasure! It has been quite the learning experience!
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 8:56 ` Junio C Hamano
@ 2009-08-23 21:07 ` Nanako Shiraishi
2009-08-23 21:14 ` Junio C Hamano
2009-08-23 22:13 ` Thell Fowler
0 siblings, 2 replies; 63+ messages in thread
From: Nanako Shiraishi @ 2009-08-23 21:07 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
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/
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 20:33 ` Thell Fowler
@ 2009-08-23 21:11 ` Junio C Hamano
2009-08-24 3:26 ` Thell Fowler
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 21:11 UTC (permalink / raw)
To: Thell Fowler; +Cc: git, Johannes.Schindelin
Thell Fowler <git@tbfowler.name> writes:
>> Or we can just move the final else clause up and start the function like
>> this:
>>
>> int i1, i2;
>>
>> if (!(flags & XDF_WHITESPACE_FLAGS))
>> return s1 == s2 && !memcmp(l1, l2, s1);
>>
>> i1 = i2 = 0;
>> if (flags & XDF_IGNORE_WHITESPACE) {
>> ...
>>
>> that would get rid of two unnecessary clearing of variables (i1 and i2,
>> even though I suspect that the compiler _could_ optimize them out without
>> such an change), and three flags-bit check in the most common case of not
>> ignoring any whitespaces.
>>
>
> HA! That's a nifty way to do that with the variables.
My tentative draft to replace the "how about this" patch further reworks
the loop structure and currently looks like this.
It adds net 15 lines but among that 12 lines are comments, which is not so
bad.
-- >8 --
Subject: [PATCH] xutils: Fix xdl_recmatch() on incomplete lines
Thell Fowler noticed that various "ignore whitespace" options to git diff
do not work well on an incomplete line.
The loop control of the function responsible for these bugs was extremely
difficult to follow. This patch restructures the loops for three variants
of "ignore whitespace" logic.
The basic idea of the re-written logic is:
- A loop runs while the characters from both strings we are looking at
match. We declare unmatch immediately when we find something that does
not match and return false from the function. We break out of the loop
if we ran out of either side of the string.
The way we skip spaces inside this loop varies depending on the style
of ignoring whitespaces.
- After the above loop breaks, we know that the parts of the strings we
inspected so far match, ignoring the whitespaces. The lines can match
only if the remainder consists of nothing but whitespaces. This part
of the logic is shared across all three styles.
The new code is more obvious and should be much easier to follow.
Tested-by: Thell Fowler <git@tbfowler.name>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
xdiff/xutils.c | 77 +++++++++++++++++++++++++++++++++----------------------
1 files changed, 46 insertions(+), 31 deletions(-)
diff --git a/xdiff/xutils.c b/xdiff/xutils.c
index 9411fa9..eb7b597 100644
--- a/xdiff/xutils.c
+++ b/xdiff/xutils.c
@@ -190,48 +190,63 @@ int xdl_recmatch(const char *l1, long s1, const char *l2, long s2, long flags)
{
int i1, i2;
+ if (!(flags & XDF_WHITESPACE_FLAGS))
+ return s1 == s2 && !memcmp(l1, l2, s1);
+
+ i1 = 0;
+ i2 = 0;
+
+ /*
+ * -w matches everything that matches with -b, and -b in turn
+ * matches everything that matches with --ignore-space-at-eol.
+ *
+ * Each flavor of ignoring needs different logic to skip whitespaces
+ * while we have both sides to compare.
+ */
if (flags & XDF_IGNORE_WHITESPACE) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (isspace(l1[i1]))
- while (isspace(l1[i1]) && i1 < s1)
- i1++;
- if (isspace(l2[i2]))
- while (isspace(l2[i2]) && i2 < s2)
- i2++;
- if (i1 < s1 && i2 < s2 && l1[i1++] != l2[i2++])
+ goto skip_ws;
+ while (i1 < s1 && i2 < s2) {
+ if (l1[i1++] != l2[i2++])
return 0;
+ skip_ws:
+ while (i1 < s1 && isspace(l1[i1]))
+ i1++;
+ while (i2 < s2 && isspace(l2[i2]))
+ i2++;
}
- return (i1 >= s1 && i2 >= s2);
} else if (flags & XDF_IGNORE_WHITESPACE_CHANGE) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (isspace(l1[i1])) {
- if (!isspace(l2[i2]))
- return 0;
- while (isspace(l1[i1]) && i1 < s1)
- i1++;
- while (isspace(l2[i2]) && i2 < s2)
- i2++;
- } else if (l1[i1++] != l2[i2++])
- return 0;
- }
- return (i1 >= s1 && i2 >= s2);
- } else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
- for (i1 = i2 = 0; i1 < s1 && i2 < s2; ) {
- if (l1[i1] != l2[i2]) {
+ while (i1 < s1 && i2 < s2) {
+ if (isspace(l1[i1]) && isspace(l2[i2])) {
+ /* Skip matching spaces and try again */
while (i1 < s1 && isspace(l1[i1]))
i1++;
while (i2 < s2 && isspace(l2[i2]))
i2++;
- if (i1 < s1 || i2 < s2)
- return 0;
- return 1;
+ continue;
}
+ if (l1[i1++] != l2[i2++])
+ return 0;
+ }
+ } else if (flags & XDF_IGNORE_WHITESPACE_AT_EOL) {
+ while (i1 < s1 && i2 < s2 && l1[i1++] == l2[i2++])
+ ; /* keep going */
+ }
+
+ /*
+ * After running out of one side, the remaining side must have
+ * nothing but whitespace for the lines to match.
+ */
+ if (i1 < s1) {
+ while (i1 < s1 && isspace(l1[i1]))
i1++;
+ return (s1 == i1);
+ }
+ if (i2 < s2) {
+ while (i2 < s2 && isspace(l2[i2]))
i2++;
- }
- return i1 >= s1 && i2 >= s2;
- } else
- return s1 == s2 && !memcmp(l1, l2, s1);
+ return (s2 == i2);
+ }
+ return 1;
}
static unsigned long xdl_hash_record_with_whitespace(char const **data,
--
1.6.4.1.255.g5556a
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 21:07 ` Nanako Shiraishi
@ 2009-08-23 21:14 ` Junio C Hamano
2009-08-23 22:13 ` Thell Fowler
1 sibling, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 21:14 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Thell Fowler, git, Johannes.Schindelin
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 21:07 ` Nanako Shiraishi
2009-08-23 21:14 ` Junio C Hamano
@ 2009-08-23 22:13 ` Thell Fowler
2009-08-23 22:30 ` Junio C Hamano
1 sibling, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-23 22:13 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Junio C Hamano, Thell Fowler, git, Johannes.Schindelin
Nanako Shiraishi (nanako3@lavabit.com) wrote on Aug 23, 2009:
> Quoting Junio C Hamano <gitster@pobox.com>
>
> > Having said that, I could use something like this.
> >
> > -- >8 -- cut here -- >8 --
> > Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
> >
> > This teaches mailinfo the scissors -- >8 -- mark; the command ignores
> > everything before it in the message body.
> >
> > Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> There are left handed people whose scissors run in the wrong direction.
>
Woohoo! Glad the left handed people aren't being discriminated against.
;)
BTW - I'm happily using this and think it should be in git!
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 22:13 ` Thell Fowler
@ 2009-08-23 22:30 ` Junio C Hamano
2009-08-24 4:16 ` [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark Nicolas Sebrecht
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-23 22:30 UTC (permalink / raw)
To: Thell Fowler; +Cc: Nanako Shiraishi, git, Johannes.Schindelin
Thell Fowler <git@tbfowler.name> writes:
>> > Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
> ...
> BTW - I'm happily using this and think it should be in git!
The one I sent out had two bugs. Please discard and replace it with a
newer one I'll be pushing out on 'pu' later today.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-23 21:11 ` Junio C Hamano
@ 2009-08-24 3:26 ` Thell Fowler
2009-08-24 6:02 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-24 3:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
Junio C Hamano (gitster@pobox.com) wrote on Aug 23, 2009:
> Thell Fowler <git@tbfowler.name> writes:
>
> >> Or we can just move the final else clause up and start the function like
> >> this:
> >>
> >> int i1, i2;
> >>
> >> if (!(flags & XDF_WHITESPACE_FLAGS))
> >> return s1 == s2 && !memcmp(l1, l2, s1);
> >>
> >> i1 = i2 = 0;
> >> if (flags & XDF_IGNORE_WHITESPACE) {
> >> ...
> >>
> >> that would get rid of two unnecessary clearing of variables (i1 and i2,
> >> even though I suspect that the compiler _could_ optimize them out without
> >> such an change), and three flags-bit check in the most common case of not
> >> ignoring any whitespaces.
> >>
> >
> > HA! That's a nifty way to do that with the variables.
>
> My tentative draft to replace the "how about this" patch further reworks
> the loop structure and currently looks like this.
>
> It adds net 15 lines but among that 12 lines are comments, which is not so
> bad.
>
It passed every test I threw at it, although it seemed to be a tad bit
slower than the previous revision on my sample data so I ran the following
command several times for both the previous and current version:
time for i in {1..10}; do ./t4015-diff-whitespace.sh>/dev/null &&
./t4015-diff-trailing-whitespace.sh >/dev/null; done
And these results are fairly average on what I saw:
Previous version:
real 2m32.669s
user 0m44.051s
sys 1m34.702s
Current version:
real 2m56.818s
user 0m47.671s
sys 1m46.723s
--
Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-23 22:30 ` Junio C Hamano
@ 2009-08-24 4:16 ` Nicolas Sebrecht
2009-08-24 4:51 ` Junio C Hamano
2009-08-24 5:16 ` [PATCH] " Nanako Shiraishi
0 siblings, 2 replies; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-24 4:16 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, Nanako Shiraishi, git, Johannes.Schindelin
Subject: [PATCH] Wong title
From: is this one really the author? <email@somebody.dom>
The 23/08/09, Junio C Hamano wrote:
>
> >> > Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
>
> The one I sent out had two bugs. Please discard and replace it with a
> newer one I'll be pushing out on 'pu' later today.
( Tested against current 925bd84 in pu. )
If we have a mail with this form
<header>
Subject: [PATCH] BLAH ONE
</header>
Subject: [PATCH] BLAH TWO
<...>
-- >8 --
Subject: [PATCH] BLAH THREE
the commit message looks like
BLAH TWO
Subject: [PATCH] BLAH THREE
I'd expect that we take the "Subject: " line after the mark and fallback
to the header if missing.
Same applies to the "From: " lines.
This mail should be usable to your own tests.
-- >8 -- Please squash this to 925bd84 -- >8 --
Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..0c9a791 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -138,6 +138,9 @@ The commit message is formed by the title taken from
the
where the patch begins. Excess whitespace at the end of each
line is automatically stripped.
+If a line starts with a "-- >8 --" mark in the body of the message,
+everything before (and the line itself) will be ignored.
+
The patch is expected to be inline, directly following the
message. Any line that is of the form:
--
Nicolas Sebrecht
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 4:16 ` [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark Nicolas Sebrecht
@ 2009-08-24 4:51 ` Junio C Hamano
2009-08-24 5:36 ` Junio C Hamano
2009-08-24 5:16 ` [PATCH] " Nanako Shiraishi
1 sibling, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-24 4:51 UTC (permalink / raw)
To: Nicolas Sebrecht
Cc: Junio C Hamano, Thell Fowler, Nanako Shiraishi, git, Johannes.Schindelin
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 4:16 ` [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark Nicolas Sebrecht
2009-08-24 4:51 ` Junio C Hamano
@ 2009-08-24 5:16 ` Nanako Shiraishi
2009-08-24 7:17 ` [PATCH] " Nicolas Sebrecht
2009-08-24 8:09 ` [PATCH] " Nanako Shiraishi
1 sibling, 2 replies; 63+ messages in thread
From: Nanako Shiraishi @ 2009-08-24 5:16 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Junio C Hamano, Thell Fowler, git, Johannes.Schindelin
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/
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 4:51 ` Junio C Hamano
@ 2009-08-24 5:36 ` Junio C Hamano
2009-08-24 6:21 ` [PATCH] " Nicolas Sebrecht
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-24 5:36 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Thell Fowler, Nanako Shiraishi, git, Johannes.Schindelin
Try this patch, perhaps? I forgot to reset the mysteriously named s_hdr
buffer.
Does anybody remember what these s_hdr (vs p_hdr) buffers stand for, by
the way?
-- >8 -- cut here -- >8 --
Subject: [PATCH] squashme to 925bd84 (Teach mailinfo to ignore everything before -- >8 -- mark, 2009-08-23)
builtin-mailinfo.c | 6 ++++++
t/t5100/sample.mbox | 4 ++++
2 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 26548f0..8a3a184 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -758,9 +758,15 @@ static int handle_commit_msg(struct strbuf *line)
}
if (scissors(line)) {
+ int i;
rewind(cmitmsg);
ftruncate(fileno(cmitmsg), 0);
still_looking = 1;
+ for (i = 0; header[i]; i++) {
+ if (s_hdr_data[i])
+ strbuf_release(s_hdr_data[i]);
+ s_hdr_data[i] = NULL;
+ }
return 0;
}
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index 95b6842..2c3da52 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -566,10 +566,14 @@ From: Junio Hamano <junkio@cox.net>
Date: Thu, 20 Aug 2009 17:18:22 -0700
Subject: Why doesn't git-am does not like >8 scissors mark?
+Subject: [PATCH] BLAH ONE
+
In real life, we will see a discussion that inspired this patch
discussing related and unrelated things around >8 scissors mark
in this part of the message.
+Subject: [PATCH] BLAH TWO
+
And the we will see the scissors.
-- >8 -- cut here -- 8< --
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-24 3:26 ` Thell Fowler
@ 2009-08-24 6:02 ` Junio C Hamano
2009-08-24 14:13 ` Thell Fowler
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-24 6:02 UTC (permalink / raw)
To: Thell Fowler; +Cc: git, Johannes.Schindelin
Thell Fowler <git@tbfowler.name> writes:
> It passed every test I threw at it, although it seemed to be a tad bit
> slower than the previous revision on my sample data so I ran the following
> command several times for both the previous and current version:
>
> time for i in {1..10}; do ./t4015-diff-whitespace.sh>/dev/null &&
> ./t4015-diff-trailing-whitespace.sh >/dev/null; done
>
> And these results are fairly average on what I saw:
>
> Previous version:
> real 2m32.669s
> user 0m44.051s
> sys 1m34.702s
>
>
> Current version:
> real 2m56.818s
> user 0m47.671s
> sys 1m46.723s
Do you mean by "previous version" the one that was broken, or the one I
sent as a "how about" patch?
Here are the numbers I am getting:
$ /usr/bin/time sh -c 'for i in 1 2 3 4 5 6 7 8 9 0; do ./t4015-diff-whitespace.sh; done' >/dev/null
----------------
1.99user 3.65system 0:05.10elapsed 110%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+12560outputs (0major+1288675minor)pagefaults 0swaps
1.86user 3.66system 0:05.04elapsed 109%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+12560outputs (0major+1288618minor)pagefaults 0swaps
1.76user 3.87system 0:05.02elapsed 112%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+12560outputs (0major+1288973minor)pagefaults 0swaps
----------------
1.81user 3.86system 0:05.08elapsed 111%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+12560outputs (0major+1288836minor)pagefaults 0swaps
1.76user 3.87system 0:04.95elapsed 113%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+12560outputs (0major+1288880minor)pagefaults 0swaps
1.81user 3.88system 0:05.04elapsed 112%CPU (0avgtext+0avgdata 0maxresident)k
0inputs+12560outputs (0major+1288530minor)pagefaults 0swaps
----------------
One set is with patch and one set is the patch reverted. I cannot quite
remember which one is which ;-) but the difference is within the noise for me.
I have to revisit this sometime after getting a long rest.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 5:36 ` Junio C Hamano
@ 2009-08-24 6:21 ` Nicolas Sebrecht
2009-08-24 6:58 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-24 6:21 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Sebrecht, Thell Fowler, Nanako Shiraishi, git,
Johannes.Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 6:21 ` [PATCH] " Nicolas Sebrecht
@ 2009-08-24 6:58 ` Junio C Hamano
2009-08-24 7:31 ` Nicolas Sebrecht
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-24 6:58 UTC (permalink / raw)
To: Nicolas Sebrecht
Cc: Junio C Hamano, Thell Fowler, Nanako Shiraishi, git, Johannes.Schindelin
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 5:16 ` [PATCH] " Nanako Shiraishi
@ 2009-08-24 7:17 ` Nicolas Sebrecht
2009-08-24 7:24 ` Nicolas Sebrecht
2009-08-24 22:17 ` Junio C Hamano
2009-08-24 8:09 ` [PATCH] " Nanako Shiraishi
1 sibling, 2 replies; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-24 7:17 UTC (permalink / raw)
To: Nanako Shiraishi
Cc: Nicolas Sebrecht, Junio C Hamano, Thell Fowler, git, Johannes.Schindelin
[ Please, please, please, wrap your lines. ]
The 24/08/09, Nanako Shiraishi wrote:
> Looking at the way other people use the mark in their messages, I think this explanation isn't correct.
I'd say that should not document what people do but what the program
does.
> A scissors mark doesn't have to be at the beginning. The line has to contain the mark, and it has to consist of only the mark, '-' minus, the phrase "cut here", and whitespaces.
...and (">8" or "<8"), you're right. But isn't the following mark a bit
too much permissive?
->8
Subject: [PATCH] squashable to 925bd84 (Teach mailinfo to ignore everything before -- >8 -- mark, 2009-08-23)
Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
This patch supersedes my previous round.
Documentation/git-am.txt | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..1d10371 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -138,6 +138,10 @@ The commit message is formed by the title taken
from the
where the patch begins. Excess whitespace at the end of each
line is automatically stripped.
+If a line starts with a "-- >8 --" mark in the body of the message,
+everything before (and the line itself) will be ignored.
+Whitespaces and strings "cut here" are tolerated.
+
The patch is expected to be inline, directly following the
message. Any line that is of the form:
--
Nicolas Sebrecht
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 7:17 ` [PATCH] " Nicolas Sebrecht
@ 2009-08-24 7:24 ` Nicolas Sebrecht
2009-08-24 22:17 ` Junio C Hamano
1 sibling, 0 replies; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-24 7:24 UTC (permalink / raw)
To: Nicolas Sebrecht
Cc: Nanako Shiraishi, Junio C Hamano, Thell Fowler, git, Johannes.Schindelin
( 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
^ permalink raw reply related [flat|nested] 63+ messages in thread
* [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 6:58 ` Junio C Hamano
@ 2009-08-24 7:31 ` Nicolas Sebrecht
2009-08-24 14:02 ` Don Zickus
0 siblings, 1 reply; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-24 7:31 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Sebrecht, Thell Fowler, Nanako Shiraishi, git,
Johannes.Schindelin, Don Zickus
( 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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 5:16 ` [PATCH] " Nanako Shiraishi
2009-08-24 7:17 ` [PATCH] " Nicolas Sebrecht
@ 2009-08-24 8:09 ` Nanako Shiraishi
1 sibling, 0 replies; 63+ messages in thread
From: Nanako Shiraishi @ 2009-08-24 8:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nicolas Sebrecht, Thell Fowler, git, Johannes.Schindelin
Quoting myself...
> A scissors mark doesn't have to be at the beginning. The line has to
> contain the mark, and it has to consist of only the mark, '-' minus, the
> phrase "cut here", and whitespaces.
Junio, perhaps you want to squash some documentation, too.
-- 8< -- cut here -- 8< -- cut here -- 8< --
Subject: [PATCH] Documentation: describe the scissors mark support of "git am"
Describe what a scissors mark looks like, and explain in what situation
it is often used.
Signed-off-by: Nanako Shiraishi <nanako3@lavabit.com>
---
Documentation/git-am.txt | 16 ++++++++++++----
1 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..fecd5ac 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -128,10 +128,18 @@ the commit, after stripping common prefix "[PATCH <anything>]".
The "Subject: " line is supposed to concisely describe what the
commit is about in one line of text.
-"From: " and "Subject: " lines starting the body (the rest of the
-message after the blank line terminating the RFC2822 headers)
-override the respective commit author name and title values taken
-from the headers.
+A line that contains a scissors mark (either ">8" or "8<") and does not
+have anything other than scissors, dash (-), whitespaces or a phrase "cut
+here" is called a scissors line. If such a line appears in the body of the
+message before the patch, everything before it (including the scissors
+line itself) is ignored. This is useful if you want to begin your message
+in a discussion thread with comments and suggestions on the message you
+are responding to, and to conclude it with a patch submission, separating
+the discussion and the beginning of the proposed commit log message with a
+scissors line.
+
+"From: " and "Subject: " lines starting the body override the respective
+commit author name and title values taken from the headers.
The commit message is formed by the title taken from the
"Subject: ", a blank line and the body of the message up to
--
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 7:31 ` Nicolas Sebrecht
@ 2009-08-24 14:02 ` Don Zickus
2009-08-24 21:48 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Don Zickus @ 2009-08-24 14:02 UTC (permalink / raw)
To: Nicolas Sebrecht
Cc: Junio C Hamano, Thell Fowler, Nanako Shiraishi, git, Johannes.Schindelin
On Mon, Aug 24, 2009 at 09:31:47AM +0200, Nicolas Sebrecht wrote:
> ( cc'ing Don Zickus )
>
> The 23/08/09, Junio C Hamano wrote:
> > Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> >
> > >> Does anybody remember what these s_hdr (vs p_hdr) buffers stand for, by
> > >> the way?
>From what I remember, I used p_hdr to designate primary headers, ie the
original mail headers. s_hdr was supposed to represent the secondary
headers, ie the embedded mail headers in the body of the email that could
override the original primary mail headers.
I hope that clears things up. Let me know if you have more questions and
I will try my best to remember what I did. :-)
Cheers,
Don
> > >
> > > Has been added by 87ab799234639c .
> >
> > That much I know ;-), thanks anyway.
> >
> > The commit does not _explain_ what they are for, what they mean, and what
> > these mysteriously named variables do.
>
> --
> Nicolas Sebrecht
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-24 6:02 ` Junio C Hamano
@ 2009-08-24 14:13 ` Thell Fowler
2009-08-25 5:58 ` Thell Fowler
0 siblings, 1 reply; 63+ messages in thread
From: Thell Fowler @ 2009-08-24 14:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Thell Fowler, git, Johannes.Schindelin
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 14:02 ` Don Zickus
@ 2009-08-24 21:48 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2009-08-24 21:48 UTC (permalink / raw)
To: Don Zickus
Cc: Nicolas Sebrecht, Junio C Hamano, Thell Fowler, Nanako Shiraishi,
git, Johannes.Schindelin
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 7:17 ` [PATCH] " Nicolas Sebrecht
2009-08-24 7:24 ` Nicolas Sebrecht
@ 2009-08-24 22:17 ` Junio C Hamano
2009-08-25 16:18 ` Nicolas Sebrecht
1 sibling, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-24 22:17 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Nanako Shiraishi, Thell Fowler, git, Johannes.Schindelin
Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> ... But isn't the following mark a bit
> too much permissive?
>
> ->8
Yeah, I agree that we should require a bit longer perforation, and perhaps
we should tighten the rules a bit, while at the same time not limiting the
request to cut to the exact phrase "cut here". As you pointed out, we do
not want to be too lenient to allow misidentification, but at the same
time it is nicer to be accomodating and treat something like this as a
scissors line:
- - - >8 - - - remove everything above this line - - - >8 - - -
I think we have bikeshedded long enough, so I won't be touching this code
any further only to change the definition of what a scissors mark looks
like, but here is what I did during lunch break, with another comment
added later to hint what s_hdr_data[] stands for after reading response
from Don Zickus.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
This teaches mailinfo the scissors -- >8 -- mark; the command ignores
everything before it in the message body.
For lefties among us, we also support -- 8< -- ;-)
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin-mailinfo.c | 71 ++++++++++++++++++++++++++++++++++++++++-
t/t5100-mailinfo.sh | 2 +-
t/t5100/info0014 | 5 +++
t/t5100/msg0014 | 4 ++
t/t5100/patch0014 | 64 ++++++++++++++++++++++++++++++++++++
t/t5100/sample.mbox | 89 +++++++++++++++++++++++++++++++++++++++++++++++++++
6 files changed, 233 insertions(+), 2 deletions(-)
create mode 100644 t/t5100/info0014
create mode 100644 t/t5100/msg0014
create mode 100644 t/t5100/patch0014
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index b0b5d8f..7e09b51 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -712,6 +712,56 @@ static inline int patchbreak(const struct strbuf *line)
return 0;
}
+static int is_scissors_line(const struct strbuf *line)
+{
+ size_t i, len = line->len;
+ int scissors = 0, gap = 0;
+ int first_nonblank = -1;
+ int last_nonblank = 0, visible, perforation, in_perforation = 0;
+ const char *buf = line->buf;
+
+ for (i = 0; i < len; i++) {
+ if (isspace(buf[i])) {
+ if (in_perforation) {
+ perforation++;
+ gap++;
+ }
+ continue;
+ }
+ last_nonblank = i;
+ if (first_nonblank < 0)
+ first_nonblank = i;
+ if (buf[i] == '-') {
+ in_perforation = 1;
+ perforation++;
+ continue;
+ }
+ if (i + 1 < len &&
+ (!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
+ in_perforation = 1;
+ perforation += 2;
+ scissors += 2;
+ i++;
+ continue;
+ }
+ in_perforation = 0;
+ }
+
+ /*
+ * The mark must be at least 8 bytes long (e.g. "-- >8 --").
+ * Even though there can be arbitrary cruft on the same line
+ * (e.g. "cut here"), in order to avoid misidentification, the
+ * perforation must occupy more than a third of the visible
+ * width of the line, and dashes and scissors must occupy more
+ * than half of the perforation.
+ */
+
+ visible = last_nonblank - first_nonblank + 1;
+ return (scissors && 8 <= visible &&
+ visible < perforation * 3 &&
+ gap * 2 < perforation);
+}
+
static int handle_commit_msg(struct strbuf *line)
{
static int still_looking = 1;
@@ -723,7 +773,8 @@ static int handle_commit_msg(struct strbuf *line)
strbuf_ltrim(line);
if (!line->len)
return 0;
- if ((still_looking = check_header(line, s_hdr_data, 0)) != 0)
+ still_looking = check_header(line, s_hdr_data, 0);
+ if (still_looking)
return 0;
}
@@ -731,6 +782,24 @@ static int handle_commit_msg(struct strbuf *line)
if (metainfo_charset)
convert_to_utf8(line, charset.buf);
+ if (is_scissors_line(line)) {
+ int i;
+ rewind(cmitmsg);
+ ftruncate(fileno(cmitmsg), 0);
+ still_looking = 1;
+
+ /*
+ * We may have already read "secondary headers"; purge
+ * them to give ourselves a clean restart.
+ */
+ for (i = 0; header[i]; i++) {
+ if (s_hdr_data[i])
+ strbuf_release(s_hdr_data[i]);
+ s_hdr_data[i] = NULL;
+ }
+ return 0;
+ }
+
if (patchbreak(line)) {
fclose(cmitmsg);
cmitmsg = NULL;
diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index e70ea94..e848556 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -11,7 +11,7 @@ test_expect_success 'split sample box' \
'git mailsplit -o. "$TEST_DIRECTORY"/t5100/sample.mbox >last &&
last=`cat last` &&
echo total is $last &&
- test `cat last` = 13'
+ test `cat last` = 14'
for mail in `echo 00*`
do
diff --git a/t/t5100/info0014 b/t/t5100/info0014
new file mode 100644
index 0000000..ab9c8d0
--- /dev/null
+++ b/t/t5100/info0014
@@ -0,0 +1,5 @@
+Author: Junio C Hamano
+Email: gitster@pobox.com
+Subject: Teach mailinfo to ignore everything before -- >8 -- mark
+Date: Thu, 20 Aug 2009 17:18:22 -0700
+
diff --git a/t/t5100/msg0014 b/t/t5100/msg0014
new file mode 100644
index 0000000..259c6a4
--- /dev/null
+++ b/t/t5100/msg0014
@@ -0,0 +1,4 @@
+This teaches mailinfo the scissors -- >8 -- mark; the command ignores
+everything before it in the message body.
+
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/t/t5100/patch0014 b/t/t5100/patch0014
new file mode 100644
index 0000000..124efd2
--- /dev/null
+++ b/t/t5100/patch0014
@@ -0,0 +1,64 @@
+---
+ builtin-mailinfo.c | 37 ++++++++++++++++++++++++++++++++++++-
+ 1 files changed, 36 insertions(+), 1 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index b0b5d8f..461c47e 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -712,6 +712,34 @@ static inline int patchbreak(const struct strbuf *line)
+ return 0;
+ }
+
++static int scissors(const struct strbuf *line)
++{
++ size_t i, len = line->len;
++ int scissors_dashes_seen = 0;
++ const char *buf = line->buf;
++
++ for (i = 0; i < len; i++) {
++ if (isspace(buf[i]))
++ continue;
++ if (buf[i] == '-') {
++ scissors_dashes_seen |= 02;
++ continue;
++ }
++ if (i + 1 < len && !memcmp(buf + i, ">8", 2)) {
++ scissors_dashes_seen |= 01;
++ i++;
++ continue;
++ }
++ if (i + 7 < len && !memcmp(buf + i, "cut here", 8)) {
++ i += 7;
++ continue;
++ }
++ /* everything else --- not scissors */
++ break;
++ }
++ return scissors_dashes_seen == 03;
++}
++
+ static int handle_commit_msg(struct strbuf *line)
+ {
+ static int still_looking = 1;
+@@ -723,10 +751,17 @@ static int handle_commit_msg(struct strbuf *line)
+ strbuf_ltrim(line);
+ if (!line->len)
+ return 0;
+- if ((still_looking = check_header(line, s_hdr_data, 0)) != 0)
++ still_looking = check_header(line, s_hdr_data, 0);
++ if (still_looking)
+ return 0;
+ }
+
++ if (scissors(line)) {
++ fseek(cmitmsg, 0L, SEEK_SET);
++ still_looking = 1;
++ return 0;
++ }
++
+ /* normalize the log message to UTF-8. */
+ if (metainfo_charset)
+ convert_to_utf8(line, charset.buf);
+--
+1.6.4.1
diff --git a/t/t5100/sample.mbox b/t/t5100/sample.mbox
index c3074ac..13fa4ae 100644
--- a/t/t5100/sample.mbox
+++ b/t/t5100/sample.mbox
@@ -561,3 +561,92 @@ From: <a.u.thor@example.com> (A U Thor)
Date: Fri, 9 Jun 2006 00:44:16 -0700
Subject: [PATCH] a patch
+From nobody Mon Sep 17 00:00:00 2001
+From: Junio Hamano <junkio@cox.net>
+Date: Thu, 20 Aug 2009 17:18:22 -0700
+Subject: Why doesn't git-am does not like >8 scissors mark?
+
+Subject: [PATCH] BLAH ONE
+
+In real life, we will see a discussion that inspired this patch
+discussing related and unrelated things around >8 scissors mark
+in this part of the message.
+
+Subject: [PATCH] BLAH TWO
+
+And then we will see the scissors.
+
+ This line is not a scissors mark -- >8 -- but talks about it.
+ - - >8 - - please remove everything above this line - - >8 - -
+
+Subject: [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark
+From: Junio C Hamano <gitster@pobox.com>
+
+This teaches mailinfo the scissors -- >8 -- mark; the command ignores
+everything before it in the message body.
+
+Signed-off-by: Junio C Hamano <gitster@pobox.com>
+---
+ builtin-mailinfo.c | 37 ++++++++++++++++++++++++++++++++++++-
+ 1 files changed, 36 insertions(+), 1 deletions(-)
+
+diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
+index b0b5d8f..461c47e 100644
+--- a/builtin-mailinfo.c
++++ b/builtin-mailinfo.c
+@@ -712,6 +712,34 @@ static inline int patchbreak(const struct strbuf *line)
+ return 0;
+ }
+
++static int scissors(const struct strbuf *line)
++{
++ size_t i, len = line->len;
++ int scissors_dashes_seen = 0;
++ const char *buf = line->buf;
++
++ for (i = 0; i < len; i++) {
++ if (isspace(buf[i]))
++ continue;
++ if (buf[i] == '-') {
++ scissors_dashes_seen |= 02;
++ continue;
++ }
++ if (i + 1 < len && !memcmp(buf + i, ">8", 2)) {
++ scissors_dashes_seen |= 01;
++ i++;
++ continue;
++ }
++ if (i + 7 < len && !memcmp(buf + i, "cut here", 8)) {
++ i += 7;
++ continue;
++ }
++ /* everything else --- not scissors */
++ break;
++ }
++ return scissors_dashes_seen == 03;
++}
++
+ static int handle_commit_msg(struct strbuf *line)
+ {
+ static int still_looking = 1;
+@@ -723,10 +751,17 @@ static int handle_commit_msg(struct strbuf *line)
+ strbuf_ltrim(line);
+ if (!line->len)
+ return 0;
+- if ((still_looking = check_header(line, s_hdr_data, 0)) != 0)
++ still_looking = check_header(line, s_hdr_data, 0);
++ if (still_looking)
+ return 0;
+ }
+
++ if (scissors(line)) {
++ fseek(cmitmsg, 0L, SEEK_SET);
++ still_looking = 1;
++ return 0;
++ }
++
+ /* normalize the log message to UTF-8. */
+ if (metainfo_charset)
+ convert_to_utf8(line, charset.buf);
+--
+1.6.4.1
--
1.6.4.1
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line
2009-08-24 14:13 ` Thell Fowler
@ 2009-08-25 5:58 ` Thell Fowler
0 siblings, 0 replies; 63+ messages in thread
From: Thell Fowler @ 2009-08-25 5:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Johannes Schindelin
Thell Fowler (git@tbfowler.name) wrote on Aug 24, 2009:
> Junio C Hamano (gitster@pobox.com) wrote on Aug 24, 2009:
>
> > Thell Fowler <git@tbfowler.name> writes:
> >
> > > It passed every test I threw at it, although it seemed to be a tad bit
> > > slower than the previous revision on my sample data so I ran the following
> > > command several times for both the previous and current version:
> > >
> >
> > Do you mean by "previous version" the one that was broken, or the one I
> > sent as a "how about" patch?
> >
>
> A quick test shows the version merged to pu is the one that had the
> fastest times. I'll be away from a connection most of today, but will
> test the different versions against the tests and some sample data and
> post back.
>
More extensive testing also shows the version currently in pu is the
fastest on my sample data when applied to master. I'm not sure why pu
shows slower times than those same commits applied to master, but they
are close enough together that I'm guessing no-one would really be
concerned.
I was sitting in a waiting room and decided to have a little fun figuring
out how to average the sys times...
for arg in "" -w -b --ignore-space-at-eol;do sum=0 && for i in {1..50}; \
do n="$(/usr/bin/time -f "%S" -o /dev/stdout sh -c 'git diff $arg dirty_first>/dev/null;')"; \
sum=$sum+$n; done; echo "scale=2; ($sum)/$i"|echo "$(bc) avg for diff $arg"; done;
pu
.28 avg for diff
.29 avg for diff -w
.33 avg for diff -b
.29 avg for diff --ignore-space-at-eol
pu commits applied to master <=== FASTEST
9c0d402 xutils: Fix xdl_recmatch() on incomplete lines
21245fd xutils: Fix hashing an incomplete line with whitespaces at the end
.26 avg for diff
.25 avg for diff -w
.29 avg for diff -b
.31 avg for diff --ignore-space-at-eol
'how about' patch applied to master
.26 avg for diff
.32 avg for diff -w
.29 avg for diff -b
.32 avg for diff --ignore-space-at-eol
current master (in order to see the difference in the basic git diff
-ignoring the fact that incomplete lines where broke since it only affects
2 files in the test data)
.30 avg for diff
.30 avg for diff -w
.29 avg for diff -b
.29 avg for diff --ignore-space-at-eol
-- Thell
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-24 22:17 ` Junio C Hamano
@ 2009-08-25 16:18 ` Nicolas Sebrecht
2009-08-26 1:51 ` Junio C Hamano
0 siblings, 1 reply; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-25 16:18 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Sebrecht, Nanako Shiraishi, Thell Fowler, git,
Johannes.Schindelin
The 24/08/09, Junio C Hamano wrote:
> perhaps
> we should tighten the rules a bit,
<...>
> I think we have bikeshedded long enough, so I won't be touching this code
> any further only to change the definition of what a scissors mark looks
> like,
I'm not sure I understand. Are you still open to a patch touching this code
/too/?
Anyway, here's what I wrote based on your last round in pu. I've change the
rules to something because I think we'd rather simple â and "easy" to
explain to the end-user â rules over "obfuscated" ones.
-- >8 -- squashable to 8683eeb (ogirin/pu) -- >8 --
Subject: Teach mailinfo to ignore everything before a scissors line
This teaches mailinfo the scissors mark (e.g. "-- >8 --");
the command ignores everything before it in the message body.
For lefties among us, we also support -- 8< -- ;-)
We can skip this check using the "--ignore-scissors" option on both
the git-mailinfo and the git-am command line. This is necessary
because the stripped message may be either
interesting from the eyes of the maintainer, regardless what the
author think;
or
the scissors line check is a false positive.
Basically, the rules are:
(1) a scissors mark:
- must be 8 characters long;
- must have a dash;
- must have either ">8" or "<8";
- may contain spaces.
(2) a scissors line:
- must have only one scissors mark;
or
- must have any comment between two identical scissors marks;
- always ignore spaces outside the scissors marks.
Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
Documentation/git-am.txt | 14 +++++-
Documentation/git-mailinfo.txt | 7 ++-
builtin-mailinfo.c | 103 +++++++++++++++++++++++----------------
git-am.sh | 14 ++++-
4 files changed, 90 insertions(+), 48 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index fcacc94..2773a3e 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--3way] [--interactive] [--committer-date-is-author-date]
[--ignore-date] [--ignore-space-change | --ignore-whitespace]
[--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
- [--reject] [-q | --quiet]
+ [--reject] [-q | --quiet] [--ignore-scissors]
[<mbox> | <Maildir>...]
'git am' (--skip | --resolved | --abort)
@@ -118,6 +118,14 @@ default. You can use `--no-utf8` to override this.
--abort::
Restore the original branch and abort the patching operation.
+--ignore-scissors::
+ Do not check for scissors line in the commit message. A scissors
+ line consists of a scissors mark which must be at least 8
+ characters long and which must contain dashes '-' and a scissors
+ (either ">8" or "<8"). Spaces are also permited inside the mark.
+ To add a comment on this line, it must be embedded between two
+ identical marks (e.g. "-- >8 -- squashme to <commit> -- >8 --").
+
DISCUSSION
----------
@@ -131,7 +139,9 @@ commit is about in one line of text.
"From: " and "Subject: " lines starting the body (the rest of the
message after the blank line terminating the RFC2822 headers)
override the respective commit author name and title values taken
-from the headers.
+from the headers. These lines immediatly following a scissors line
+override the respective fields regardless what could stand at the
+beginning of the body.
The commit message is formed by the title taken from the
"Subject: ", a blank line and the body of the message up to
diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index 8d95aaa..e16a577 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -8,7 +8,8 @@ git-mailinfo - Extracts patch and authorship from a single e-mail message
SYNOPSIS
--------
-'git mailinfo' [-k] [-u | --encoding=<encoding> | -n] <msg> <patch>
+'git mailinfo' [-k] [-u | --encoding=<encoding> | -n] [--ignore-scissors]
+<msg> <patch>
DESCRIPTION
@@ -49,6 +50,10 @@ conversion, even with this flag.
-n::
Disable all charset re-coding of the metadata.
+--ignore-scissors::
+ Do not check for a scissors line (see linkgit:git-am[1]
+ for more information on scissors lines).
+
<msg>::
The commit log message extracted from e-mail, usually
except the title line which comes from e-mail Subject.
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 7e09b51..92319f6 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -6,6 +6,7 @@
#include "builtin.h"
#include "utf8.h"
#include "strbuf.h"
+#include "git-compat-util.h"
static FILE *cmitmsg, *patchfile, *fin, *fout;
@@ -25,6 +26,7 @@ static enum {
static struct strbuf charset = STRBUF_INIT;
static int patch_lines;
static struct strbuf **p_hdr_data, **s_hdr_data;
+static int ignore_scissors = 0;
#define MAX_HDR_PARSED 10
#define MAX_BOUNDARIES 5
@@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
static int is_scissors_line(const struct strbuf *line)
{
size_t i, len = line->len;
- int scissors = 0, gap = 0;
- int first_nonblank = -1;
- int last_nonblank = 0, visible, perforation, in_perforation = 0;
const char *buf = line->buf;
+ size_t mark_start = 0, mark_end = 0, mark_len;
+ int scissors_dashes_seen = 0;
for (i = 0; i < len; i++) {
if (isspace(buf[i])) {
- if (in_perforation) {
- perforation++;
- gap++;
- }
+ if (scissors_dashes_seen)
+ mark_end = i;
continue;
}
- last_nonblank = i;
- if (first_nonblank < 0)
- first_nonblank = i;
+ if (!scissors_dashes_seen)
+ mark_start = i;
if (buf[i] == '-') {
- in_perforation = 1;
- perforation++;
+ mark_end = i;
+ scissors_dashes_seen |= 01;
continue;
}
if (i + 1 < len &&
(!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
- in_perforation = 1;
- perforation += 2;
- scissors += 2;
i++;
+ mark_end = i;
+ scissors_dashes_seen |= 02;
continue;
}
- in_perforation = 0;
+ break;
}
- /*
- * The mark must be at least 8 bytes long (e.g. "-- >8 --").
- * Even though there can be arbitrary cruft on the same line
- * (e.g. "cut here"), in order to avoid misidentification, the
- * perforation must occupy more than a third of the visible
- * width of the line, and dashes and scissors must occupy more
- * than half of the perforation.
- */
+ if (scissors_dashes_seen == 03) {
+ /* strip trailing spaces at the end of the mark */
+ for (i = mark_end; i >= mark_start && i <= mark_end; i--) {
+ if (isspace(buf[i]))
+ mark_end--;
+ else
+ break;
+ }
- visible = last_nonblank - first_nonblank + 1;
- return (scissors && 8 <= visible &&
- visible < perforation * 3 &&
- gap * 2 < perforation);
+ mark_len = mark_end - mark_start + 1;
+ if (mark_len >= 8) {
+ /* ignore trailing spaces at the end of the line */
+ len--;
+ for (i = len - 1; i >= 0; i--) {
+ if (isspace(buf[i]))
+ len--;
+ else
+ break;
+ }
+ /*
+ * The mark is 8 charaters long and contains at least one dash and
+ * either a ">8" or "<8". Check if the last mark in the line
+ * matches the first mark found without worrying about what could
+ * be between them. Only one mark in the whole line is permitted.
+ */
+ return (!memcmp(buf + mark_start, buf + len - mark_len, mark_len));
+ }
+ }
+
+ return 0;
}
static int handle_commit_msg(struct strbuf *line)
@@ -782,22 +796,25 @@ static int handle_commit_msg(struct strbuf *line)
if (metainfo_charset)
convert_to_utf8(line, charset.buf);
- if (is_scissors_line(line)) {
- int i;
- rewind(cmitmsg);
- ftruncate(fileno(cmitmsg), 0);
- still_looking = 1;
+ if (!ignore_scissors) {
+ if (is_scissors_line(line)) {
+ warning("scissors line found, will skip text above");
+ int i;
+ rewind(cmitmsg);
+ ftruncate(fileno(cmitmsg), 0);
+ still_looking = 1;
- /*
- * We may have already read "secondary headers"; purge
- * them to give ourselves a clean restart.
- */
- for (i = 0; header[i]; i++) {
- if (s_hdr_data[i])
- strbuf_release(s_hdr_data[i]);
- s_hdr_data[i] = NULL;
+ /*
+ * We may have already read "secondary headers"; purge
+ * them to give ourselves a clean restart.
+ */
+ for (i = 0; header[i]; i++) {
+ if (s_hdr_data[i])
+ strbuf_release(s_hdr_data[i]);
+ s_hdr_data[i] = NULL;
+ }
+ return 0;
}
- return 0;
}
if (patchbreak(line)) {
@@ -1011,6 +1028,8 @@ int cmd_mailinfo(int argc, const char **argv, const char *prefix)
while (1 < argc && argv[1][0] == '-') {
if (!strcmp(argv[1], "-k"))
keep_subject = 1;
+ else if (!strcmp(argv[1], "--ignore-scissors"))
+ ignore_scissors = 1;
else if (!strcmp(argv[1], "-u"))
metainfo_charset = def_charset;
else if (!strcmp(argv[1], "-n"))
diff --git a/git-am.sh b/git-am.sh
index 3c03f3e..17c883f 100755
--- a/git-am.sh
+++ b/git-am.sh
@@ -15,6 +15,7 @@ q,quiet be quiet
s,signoff add a Signed-off-by line to the commit message
u,utf8 recode into utf8 (default)
k,keep pass -k flag to git-mailinfo
+ignore-scissors pass --ignore-scissors flag to git-mailinfo
whitespace= pass it through git-apply
ignore-space-change pass it through git-apply
ignore-whitespace pass it through git-apply
@@ -288,7 +289,7 @@ split_patches () {
prec=4
dotest="$GIT_DIR/rebase-apply"
sign= utf8=t keep= skip= interactive= resolved= rebasing= abort=
-resolvemsg= resume=
+resolvemsg= resume= ignore_scissors=
git_apply_opt=
committer_date_is_author_date=
ignore_date=
@@ -310,6 +311,8 @@ do
utf8= ;;
-k|--keep)
keep=t ;;
+ --ignore-scissors)
+ ignore_scissors=t ;;
-r|--resolved)
resolved=t ;;
--skip)
@@ -435,7 +438,7 @@ else
split_patches "$@"
- # -s, -u, -k, --whitespace, -3, -C, -q and -p flags are kept
+ # Following flags are kept
# for the resuming session after a patch failure.
# -i can and must be given when resuming.
echo " $git_apply_opt" >"$dotest/apply-opt"
@@ -443,6 +446,7 @@ else
echo "$sign" >"$dotest/sign"
echo "$utf8" >"$dotest/utf8"
echo "$keep" >"$dotest/keep"
+ echo "$ignore_scissors" >"$dotest/ignore-scissors"
echo "$GIT_QUIET" >"$dotest/quiet"
echo 1 >"$dotest/next"
if test -n "$rebasing"
@@ -484,6 +488,10 @@ if test "$(cat "$dotest/keep")" = t
then
keep=-k
fi
+if test "$(cat "$dotest/ignore-scissors")" = t
+then
+ ignore_scissors='--ignore-scissors'
+fi
if test "$(cat "$dotest/quiet")" = t
then
GIT_QUIET=t
@@ -538,7 +546,7 @@ do
# by the user, or the user can tell us to do so by --resolved flag.
case "$resume" in
'')
- git mailinfo $keep $utf8 "$dotest/msg" "$dotest/patch" \
+ git mailinfo $keep $ignore_scissors $utf8 "$dotest/msg" "$dotest/patch" \
<"$dotest/$msgnum" >"$dotest/info" ||
stop_here $this
--
1.6.4.1.334.gf42e22
^ permalink raw reply related [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-25 16:18 ` Nicolas Sebrecht
@ 2009-08-26 1:51 ` Junio C Hamano
[not found] ` <20090826110332.6117@nanako3.lavabit.com>
` (2 more replies)
0 siblings, 3 replies; 63+ messages in thread
From: Junio C Hamano @ 2009-08-26 1:51 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Nanako Shiraishi, Thell Fowler, git, Johannes.Schindelin
Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
>> I think we have bikeshedded long enough, so I won't be touching this code
>> any further only to change the definition of what a scissors mark looks
>> like,
>
> I'm not sure I understand. Are you still open to a patch touching this code
> /too/?
What I meant was that I would not want to spend any more of _my_ time on
the definition of the scissors for now. That means spending or wasting
time on improving the 'pu' patch myself, or looking at others patch to
find flaws in them.
Of course, as the maintainer, I would need to look at proposals to improve
or fix bugs in the code before the series hits the master, but I would
give zero priority to the patches that change the definition at least for
now to give myself time to work on more useful things.
I think --ignore-scissors is a good thing to add, regardless of what the
definition of scissors should be. So your patch should definitely be
separated into two parts.
> diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
> index 7e09b51..92319f6 100644
> --- a/builtin-mailinfo.c
> +++ b/builtin-mailinfo.c
> @@ -6,6 +6,7 @@
> #include "builtin.h"
> #include "utf8.h"
> #include "strbuf.h"
> +#include "git-compat-util.h"
Inclusion of builtin.h is designed to be enough. What do you need this
for?
> static FILE *cmitmsg, *patchfile, *fin, *fout;
>
> @@ -25,6 +26,7 @@ static enum {
> static struct strbuf charset = STRBUF_INIT;
> static int patch_lines;
> static struct strbuf **p_hdr_data, **s_hdr_data;
> +static int ignore_scissors = 0;
Don't initialize a static to 0.
> @@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
> if (isspace(buf[i])) {
> + if (scissors_dashes_seen)
> + mark_end = i;
I think you do not want this part, and then you won't have to trim
trailing whitespaces from mark_end later.
> + /*
> + * The mark is 8 charaters long and contains at least one dash and
> + * either a ">8" or "<8". Check if the last mark in the line
> + * matches the first mark found without worrying about what could
> + * be between them. Only one mark in the whole line is permitted.
> + */
This definition makes "- 8<" a scissors.
Even though
"-- 8< -- please cut here -- -- 8< --"
is allowed, so is
"-- 8< -- -- please cut here -- 8< -- --"
it does not allow
"-- 8< -- please cut here -- 8< -- --"
nor
"-- 8< -- -- please cut here -- -- 8< --"
nor
"-- 8< -- -- please cut here -- -- >8 --"
Oh, did I say I won't waste my time on the definition? I should have just
discarded this hunk ;-)
> @@ -782,22 +796,25 @@ static int handle_commit_msg(struct strbuf *line)
> if (metainfo_charset)
> convert_to_utf8(line, charset.buf);
>
> - if (is_scissors_line(line)) {
> - int i;
> - rewind(cmitmsg);
> - ftruncate(fileno(cmitmsg), 0);
> - still_looking = 1;
> + if (!ignore_scissors) {
> + if (is_scissors_line(line)) {
> + warning("scissors line found, will skip text above");
> ...
> + return 0;
Don't re-indent like this. Just do:
if (!ignore_scissors && is_scissors_line(line)) {
...
}
> - # -s, -u, -k, --whitespace, -3, -C, -q and -p flags are kept
> + # Following flags are kept
We seem to have lost the description of what the "Following" are.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
[not found] ` <20090826110332.6117@nanako3.lavabit.com>
@ 2009-08-26 2:20 ` Junio C Hamano
0 siblings, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2009-08-26 2:20 UTC (permalink / raw)
To: Nanako Shiraishi; +Cc: Nicolas Sebrecht, Thell Fowler, git, Johannes.Schindelin
Nanako Shiraishi <nanako3@lavabit.com> writes:
> Quoting Junio C Hamano <gitster@pobox.com>
>
>> What I meant was that I would not want to spend any more of _my_ time on
>> the definition of the scissors for now. That means spending or wasting
>> time on improving the 'pu' patch myself, or looking at others patch to
>> find flaws in them.
>>
>> Of course, as the maintainer, I would need to look at proposals to improve
>> or fix bugs in the code before the series hits the master, but I would
>> give zero priority to the patches that change the definition at least for
>> now to give myself time to work on more useful things.
>
> I am hoping that you didn't mean to say that other people on the list
> mustn't look at such patches and help improve them either?
>
> Perhaps you can rephrase your message in a more positive way, just like
> you request other people to do in their proposed commit log messages?
Ok, I agree that the way I worded the message was suboptimal, so let's try
again.
I would appreciate if the members of the list come up with an alternative
definition with a good implementation that they can agree on, and present
the result as a list consensus, with a solid justification to replace the
crap I have queued in 'pu', in the form of an applicable patch. Because I
consider that the exact definition of what a scissors line looks like is
an insignificant detail, I would prefer to see that process happen without
involving me.
By the way, I already queued your documentation patch, as adding the part
that describes what a "scissors" line is good for is a very good idea.
The "community" patch to replace the definition of scissors line may have
to update the part that describes what a "scissors" line looks like in
your patch.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 1:51 ` Junio C Hamano
[not found] ` <20090826110332.6117@nanako3.lavabit.com>
@ 2009-08-26 3:03 ` Junio C Hamano
2009-08-26 5:02 ` Nicolas Sebrecht
2009-08-26 3:54 ` Nicolas Sebrecht
2 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-26 3:03 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Nanako Shiraishi, Thell Fowler, git, Johannes.Schindelin
Junio C Hamano <gitster@pobox.com> writes:
> I think --ignore-scissors is a good thing to add, regardless of what the
> definition of scissors should be. So your patch should definitely be
> separated into two parts.
Having thought about this a bit more, I do not think --ignore-scissors
makes much sense, for several reasons.
Traditionally, a few lines of "background material" that accompanies a
patch, without being a part of discussion thread that quotes large chunks
of original message with ">" (like you see above), are given below the
three-dash line, not above "scissors". This is a good practice not only
because we did not have "scissors" support in mailinfo, but because it
forced the author to be concise and to the point, and also immediately
below the three-dash lines is where the diffstat comes, and it is a place
designed to be used for memory refreshers (e.g. "I changed this and that
from the previous round based on comments by ...").
The scissors feature shouldn't be used as the replacement for this, not
from technical but from human efficiency reasons, as you have to first
read above scissors and then jump your eyes down to diffstat, before
deciding if it is worth your time to read the commit log message and the
patch.
When there is a long discussion, a message in the thread, after following
the usual discussion style, may give a (counter)proposal as a "how about
this" patch. Such a patch is still _primarily_ for discussion, but it
sometimes turn out to be a good solution to the problem discussed in the
thread. The maintainer (or participant) then deliberately picks that
message and feeds it to "am", and it would be nice if things above
scissors are removed automatically. This is the _only_ intended use case
of the "scissors" line.
I therefore conclude that using the "remove above scissors" should be a
conscious decision, and should not be enabled by default. --obey-scissors
would be a good option for this reason.
Besides, if you _lost_ information because the scissors that is on by
default gave a false positive, you have to reset HEAD^ and re-apply. If
on the other hand we mistakenly kept cruft above a scissors, we can edit
it away using "rebase -i". So the failure recovery is much nicer if the
feature is not on by default.
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 1:51 ` Junio C Hamano
[not found] ` <20090826110332.6117@nanako3.lavabit.com>
2009-08-26 3:03 ` Junio C Hamano
@ 2009-08-26 3:54 ` Nicolas Sebrecht
2 siblings, 0 replies; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-26 3:54 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Sebrecht, Nanako Shiraishi, Thell Fowler, git,
Johannes.Schindelin
The 25/08/09, Junio C Hamano wrote:
> What I meant was that I would not want to spend any more of _my_ time on
> the definition of the scissors for now. That means spending or wasting
> time on improving the 'pu' patch myself, or looking at others patch to
> find flaws in them.
>
> Of course, as the maintainer, I would need to look at proposals to improve
> or fix bugs in the code before the series hits the master, but I would
> give zero priority to the patches that change the definition at least for
> now to give myself time to work on more useful things.
Ok, thank you.
> I think --ignore-scissors is a good thing to add, regardless of what the
> definition of scissors should be. So your patch should definitely be
> separated into two parts.
Could find it at the end of the mails.
> > #include "builtin.h"
> > #include "utf8.h"
> > #include "strbuf.h"
> > +#include "git-compat-util.h"
>
> Inclusion of builtin.h is designed to be enough. What do you need this
> for?
It is for the warning() call
warning("scissors line found, will skip text above");
I've added. That said, moving this declaration to builtin.h could be a
good idea. Hint?
> > @@ -715,51 +717,63 @@ static inline int patchbreak(const struct strbuf *line)
> > if (isspace(buf[i])) {
> > + if (scissors_dashes_seen)
> > + mark_end = i;
>
> I think you do not want this part, and then you won't have to trim
> trailing whitespaces from mark_end later.
Good eyes.
> > + /*
> > + * The mark is 8 charaters long and contains at least one dash and
> > + * either a ">8" or "<8". Check if the last mark in the line
> > + * matches the first mark found without worrying about what could
> > + * be between them. Only one mark in the whole line is permitted.
> > + */
>
> This definition makes "- 8<" a scissors.
Yes. Instead of looking for dashes alone, I will give a try to something
like
if (!scissors_dashes_seen)
mark_start = i;
if (i + 1 < len) {
if (!memcmp(buf + i, ">8", 2) || !memcmp(buf + i, "8<", 2))) {
scissors_dashes_seen |= 02;
i++;
mark_end = i;
continue;
else if (!memcmp(buf + i "--", 2) {
scissors_dashes_seen |= 04;
i++;
mark_end = i;
continue;
}
}
if (i + 2 < len)
if (!memcmp(buf + i + 1, "- -", 3) {
scissors_dashes_seen |= 04;
i += 2;
mark_end = i;
continue;
}
if (buf[i] == '-') {
mark_end = i;
scissors_dashes_seen |= 01;
continue;
}
break;
}
if (scissors_dashes_seen == 07) {
...
> it does not allow
>
> "-- 8< -- please cut here -- 8< -- --"
Actually, I believe this one should really not be a scissors line. If we
accept some random dashes around markers it will break the definition of
the mark itself.
As I said, I'd rather rules easy to define over others because if the
end-user scissors line doesn't work, he can refer to the documentation...
> nor
>
> "-- 8< -- -- please cut here -- -- 8< --"
>
> nor
>
> "-- 8< -- -- please cut here -- -- >8 --"
...and symmetrical markers make sense to the user. Will add this.
> > + if (!ignore_scissors) {
> > + if (is_scissors_line(line)) {
> > + warning("scissors line found, will skip text above");
> > ...
> > + return 0;
>
> Don't re-indent like this. Just do:
>
> if (!ignore_scissors && is_scissors_line(line)) {
> ...
> }
Does the compilers (or a standard) assure that the members are evaluated
in the left-right order?
Otherwise, we may call is_scissors_line() where not needed.
--
Nicolas Sebrecht
^ permalink raw reply [flat|nested] 63+ messages in thread
* [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 3:03 ` Junio C Hamano
@ 2009-08-26 5:02 ` Nicolas Sebrecht
2009-08-26 8:57 ` Jakub Narebski
0 siblings, 1 reply; 63+ messages in thread
From: Nicolas Sebrecht @ 2009-08-26 5:02 UTC (permalink / raw)
To: Junio C Hamano
Cc: Nicolas Sebrecht, Nanako Shiraishi, Thell Fowler, git,
Johannes.Schindelin
The 25/08/09, Junio C Hamano wrote:
> I therefore conclude that using the "remove above scissors" should be a
> conscious decision, and should not be enabled by default. --obey-scissors
> would be a good option for this reason.
I'm not sure what between --obey or --ignore will help most to write
good commit message. I (as a maintainer myself or as a contributor)
usually prefer to --amend a commit rather than play with copy/paste or
starting from scratch. So, I tend to agree even if the reasons are not
exactly the same. :-)
That said, I don't bother what is the default that much. The main
purpose is to have the choice.
For people who _really_ want to obey to scissors by default I'll add an
option to git-config. Whithout more comments, I'll add
scissors.obey
.
--
Nicolas Sebrecht
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 5:02 ` Nicolas Sebrecht
@ 2009-08-26 8:57 ` Jakub Narebski
2009-08-26 9:00 ` Johannes Schindelin
2009-08-26 9:03 ` Junio C Hamano
0 siblings, 2 replies; 63+ messages in thread
From: Jakub Narebski @ 2009-08-26 8:57 UTC (permalink / raw)
To: git
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
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 8:57 ` Jakub Narebski
@ 2009-08-26 9:00 ` Johannes Schindelin
2009-08-27 5:46 ` Junio C Hamano
2009-08-26 9:03 ` Junio C Hamano
1 sibling, 1 reply; 63+ messages in thread
From: Johannes Schindelin @ 2009-08-26 9:00 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 479 bytes --]
Hi,
On Wed, 26 Aug 2009, Jakub Narebski wrote:
> Nicolas Sebrecht wrote:
>
> > For people who _really_ want to obey to scissors by default I'll add
> > an option to git-config. Whithout more comments, I'll add
> >
> > scissors.obey
>
> mailsplit.scissors
Sorry, did not have time to read this thread properly, but has anybody put
thought into the interaction between this patch and "git rebase" (which
uses "git am", and therefore mailsplit, internally)?
Ciao,
Dscho
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 8:57 ` Jakub Narebski
2009-08-26 9:00 ` Johannes Schindelin
@ 2009-08-26 9:03 ` Junio C Hamano
1 sibling, 0 replies; 63+ messages in thread
From: Junio C Hamano @ 2009-08-26 9:03 UTC (permalink / raw)
To: Jakub Narebski
Cc: Nicolas Sebrecht, Nanako Shiraishi, Thell Fowler, git,
Johannes.Schindelin
Jakub Narebski <jnareb@gmail.com> writes:
> Nicolas Sebrecht wrote:
>
>> For people who _really_ want to obey to scissors by default I'll add an
>> option to git-config. Whithout more comments, I'll add
>>
>> scissors.obey
>
> mailsplit.scissors
That may be a better name.
It must take lower precedence than the command line --no-scissors option,
and that option must be given to am when rebase internally runs am, so that
we won't pay attention to scissors when rebasing existing commits.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-26 9:00 ` Johannes Schindelin
@ 2009-08-27 5:46 ` Junio C Hamano
2009-08-27 10:49 ` Johannes Schindelin
0 siblings, 1 reply; 63+ messages in thread
From: Junio C Hamano @ 2009-08-27 5:46 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Jakub Narebski, git
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.
^ permalink raw reply [flat|nested] 63+ messages in thread
* Re: [PATCH] Re: Teach mailinfo to ignore everything before -- >8 -- mark
2009-08-27 5:46 ` Junio C Hamano
@ 2009-08-27 10:49 ` Johannes Schindelin
0 siblings, 0 replies; 63+ messages in thread
From: Johannes Schindelin @ 2009-08-27 10:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jakub Narebski, git
Hi,
On Wed, 26 Aug 2009, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> mailsplit.scissors
> >
> > Sorry, did not have time to read this thread properly, but has anybody put
> > thought into the interaction between this patch and "git rebase" (which
> > uses "git am", and therefore mailsplit, internally)?
>
> I was looking around this area tonight (I promised I won't touch the
> definition of scissors, but I never said I won't work on making it
> usable), as I originally shared the same worry with you.
>
> It turns out that "rebase" invokes "am" with the "--rebasing" option.
> Under this option, "am" uses an equivalent of "commit -C $commit"
> internally to port the message forward. So our worries are unfounded.
Thank you very much! This relieves me, indeed.
Ciao,
Dscho
^ permalink raw reply [flat|nested] 63+ messages in thread
end of thread, other threads:[~2009-08-27 10:49 UTC | newest]
Thread overview: 63+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-08-04 23:33 Help/Advice needed on diff bug in xutils.c Thell Fowler
2009-08-05 20:45 ` Johannes Schindelin
2009-08-10 18:54 ` Thell Fowler
2009-08-12 0:47 ` [PATCH/RFC] Add diff tests for trailing-space and now newline Thell Fowler
2009-08-19 23:05 ` [PATCH 0/6 RFC] Series to correct xutils incomplete line handling Thell Fowler
2009-08-21 17:39 ` Thell Fowler
2009-08-21 22:16 ` Alex Riesen
2009-08-22 4:23 ` Thell Fowler
[not found] ` <cover.1250719760.git.git@tbfowler.name>
2009-08-19 23:06 ` [PATCH 1/6] Add supplemental test for trailing-whitespace on incomplete lines Thell Fowler
2009-08-19 23:06 ` [PATCH 2/6] Make xdl_hash_record_with_whitespace ignore eof Thell Fowler
2009-08-19 23:07 ` [PATCH 3/6] Make diff -w handle trailing-spaces on incomplete lines Thell Fowler
2009-08-20 23:09 ` Thell Fowler
2009-08-19 23:07 ` [PATCH 4/6] Make diff -b " Thell Fowler
2009-08-19 23:08 ` [PATCH 5/6] Make diff --ignore-space-at-eol handle " Thell Fowler
2009-08-19 23:09 ` [PATCH 6/6] Add diff tests for trailing-space on " Thell Fowler
2009-08-23 3:47 ` [PATCH-v2/RFC 0/6] improvements for trailing-space processing " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 1/6] Add supplemental test for trailing-whitespace " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 2/6] xutils: fix hash with whitespace on incomplete line Thell Fowler
2009-08-23 7:51 ` Junio C Hamano
2009-08-23 17:02 ` Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space " Thell Fowler
2009-08-23 7:57 ` Junio C Hamano
2009-08-23 8:18 ` Nanako Shiraishi
2009-08-23 8:56 ` Junio C Hamano
2009-08-23 21:07 ` Nanako Shiraishi
2009-08-23 21:14 ` Junio C Hamano
2009-08-23 22:13 ` Thell Fowler
2009-08-23 22:30 ` Junio C Hamano
2009-08-24 4:16 ` [PATCH] Teach mailinfo to ignore everything before -- >8 -- mark Nicolas Sebrecht
2009-08-24 4:51 ` Junio C Hamano
2009-08-24 5:36 ` Junio C Hamano
2009-08-24 6:21 ` [PATCH] " Nicolas Sebrecht
2009-08-24 6:58 ` Junio C Hamano
2009-08-24 7:31 ` Nicolas Sebrecht
2009-08-24 14:02 ` Don Zickus
2009-08-24 21:48 ` Junio C Hamano
2009-08-24 5:16 ` [PATCH] " Nanako Shiraishi
2009-08-24 7:17 ` [PATCH] " Nicolas Sebrecht
2009-08-24 7:24 ` Nicolas Sebrecht
2009-08-24 22:17 ` Junio C Hamano
2009-08-25 16:18 ` Nicolas Sebrecht
2009-08-26 1:51 ` Junio C Hamano
[not found] ` <20090826110332.6117@nanako3.lavabit.com>
2009-08-26 2:20 ` Junio C Hamano
2009-08-26 3:03 ` Junio C Hamano
2009-08-26 5:02 ` Nicolas Sebrecht
2009-08-26 8:57 ` Jakub Narebski
2009-08-26 9:00 ` Johannes Schindelin
2009-08-27 5:46 ` Junio C Hamano
2009-08-27 10:49 ` Johannes Schindelin
2009-08-26 9:03 ` Junio C Hamano
2009-08-26 3:54 ` Nicolas Sebrecht
2009-08-24 8:09 ` [PATCH] " Nanako Shiraishi
2009-08-23 17:01 ` [PATCH-v2/RFC 3/6] xutils: fix ignore-all-space on incomplete line Thell Fowler
2009-08-23 19:40 ` Junio C Hamano
2009-08-23 20:33 ` Thell Fowler
2009-08-23 21:11 ` Junio C Hamano
2009-08-24 3:26 ` Thell Fowler
2009-08-24 6:02 ` Junio C Hamano
2009-08-24 14:13 ` Thell Fowler
2009-08-25 5:58 ` Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 4/6] xutils: fix ignore-space-change " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 5/6] xutils: fix ignore-space-at-eol " Thell Fowler
2009-08-23 3:49 ` [PATCH-v2/RFC 6/6] t4015: add tests for trailing-space " Thell Fowler
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.