[PATCH] Fix '\ No newline...' annotation in rewrite diffs
When a file that ends with an incomplete line is expressed as a
complete rewrite with the -B option, git diff incorrectly appends the
incomplete line indicator \ No newline at end of file after such a
line, rather than writing it on a line of its own (the output codepath
for normal output without -B does not have this problem). Add a LF
after the incomplete line before writing the \ No newline ... out
to fix this.
Add a couple of tests to confirm that the indicator comment is
generated on its own line in both plain diff and rewrite mode.
Signed-off-by: Adam Butcher dev.li...@jessamine.co.uk
---
Updates:
- replace commit msg with revised suggestion from Junio
- remove hardcoded 'No newline...' in tests and simplify
diff.c | 1 +
t/t4022-diff-rewrite.sh | 33 +
2 files changed, 34 insertions(+)
diff --git a/diff.c b/diff.c
index 1a594df..f333de8 100644
--- a/diff.c
+++ b/diff.c
@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
if (!endp) {
const char *plain = diff_get_color(ecb-color_diff,
DIFF_PLAIN);
+ putc('\n', ecb-opt-file);
emit_line_0(ecb-opt, plain, reset, '\\',
nneof, strlen(nneof));
}
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index c00a94b..05ac3e9 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -66,5 +66,38 @@ test_expect_success 'suppress deletion diff with -B -D' '
grep -v Linus Torvalds actual
'
+test_expect_success 'generate initial no newline at eof sequence file and
commit' '
+
+ test_seq 1 99 seq
+ printf 100 seq
+ git add seq
+ git commit seq -m seq
+'
+
+test_expect_success 'rewrite the middle 90% of sequence file and terminate
with newline' '
+
+ test_seq 1 5 seq
+ test_seq 9331 9420 seq
+ test_seq 96 100 seq
+'
+
+test_expect_success 'confirm that sequence file is considered a rewrite' '
+
+ git diff -B seq res
+ grep dissimilarity index res
+'
+
+test_expect_success 'no newline at eof is on its own line without -B' '
+
+ git diff seq res
+ grep ^ res ! grep ^..* res
+'
+
+test_expect_success 'no newline at eof is on its own line with -B' '
+
+ git diff -B seq res
+ grep ^ res ! grep ^..* res
+'
+
test_done
--
1.7.11.msysgit.1.1.gf0affa1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
When operating in --break-rewrites (-B) mode on a file with no newline
terminator (and assuming --break-rewrites determines that the diff
_is_ a rewrite), git diff previously concatenated the indicator comment
'\ No newline at end of file' directly to the terminating line rather
than on a line of its own. The resulting diff is broken; claiming
that the last line actually contains the indicator text. Without -B
there is no problem with the same files.
This patch fixes the former case by inserting a newline into the
output prior to emitting the indicator comment.
A couple of tests have been added to the rewrite suite to confirm that
the indicator comment is generated on its own line in both plain diff
and rewrite mode. The latter test fails if the functional part of
this patch (i.e. diff.c) is reverted.
---
Updates: Test only:
- removed redundant para from commit msg
- use test_seq shell function instead of seq
- pull prep statements into individual tests
- test expected success of git commands in prep
- confirm that rewrite is considered a rewrite by diff -B
- remove superfluous comments in favor of test descriptions
- use variable to spell 'no newline' annotation to support simpler
maintenance whilst still allowing to check for unexpected leading
or trailing characters.
diff.c | 1 +
t/t4022-diff-rewrite.sh | 42 ++
2 files changed, 43 insertions(+)
diff --git a/diff.c b/diff.c
index 1a594df..f333de8 100644
--- a/diff.c
+++ b/diff.c
@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,
if (!endp) {
const char *plain = diff_get_color(ecb-color_diff,
DIFF_PLAIN);
+ putc('\n', ecb-opt-file);
emit_line_0(ecb-opt, plain, reset, '\\',
nneof, strlen(nneof));
}
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index c00a94b..1b7ae9f 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -66,5 +66,47 @@ test_expect_success 'suppress deletion diff with -B -D' '
grep -v Linus Torvalds actual
'
+test_expect_success 'generate initial no newline at eof sequence file and
commit' '
+
+ test_seq 1 99 seq
+ printf 100 seq
+ git add seq
+ git commit seq -m seq
+'
+
+test_expect_success 'rewrite the middle 90% of sequence file and terminate
with
newline' '
+
+ test_seq 1 5 seq
+ test_seq 9331 9420 seq
+ test_seq 96 100 seq
+'
+
+test_expect_success 'confirm that sequence file is considered a rewrite' '
+
+ git diff -B seq res
+ grep dissimilarity index res
+'
+
+# Full annotation string used to check for erroneous leading or
+# trailing characters. Backslash is double escaped due to usage
+# within dq argument to grep expansion below.
+no_newline_anno=' No newline at end of file'
+
+test_expect_success 'no newline at eof is on its own line without -B' '
+
+ git diff seq res
+ grep ^'$no_newline_anno'$ res
+ grep -v ^.\\+'$no_newline_anno' res
+ grep -v '$no_newline_anno'.\\+$ res
+'
+
+test_expect_success 'no newline at eof is on its own line with -B' '
+
+ git diff -B seq res
+ grep ^'$no_newline_anno'$ res
+ grep -v ^.\\+'$no_newline_anno' res
+ grep -v '$no_newline_anno'.\\+$ res
+'
+
test_done
--
1.7.11.msysgit.1.1.gf0affa1
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Adam Butcher dev.li...@jessamine.co.uk writes: When operating in --break-rewrites (-B) mode on a file with no newline terminator (and assuming --break-rewrites determines that the diff _is_ a rewrite), git diff previously concatenated the indicator comment '\ No newline at end of file' directly to the terminating line rather than on a line of its own. The resulting diff is broken; claiming that the last line actually contains the indicator text. Without -B there is no problem with the same files. This patch fixes the former case by inserting a newline into the output prior to emitting the indicator comment. A couple of tests have been added to the rewrite suite to confirm that the indicator comment is generated on its own line in both plain diff and rewrite mode. The latter test fails if the functional part of this patch (i.e. diff.c) is reverted. --- Thanks. You need your sign-off immediately before the --- line. When the problem description at the beginning of a log message is about the current status of the code (which is almost always the case), it generally does not need to be clarified with previously. A (POSIXy technical term) for the last line that does not end with the newline is incomplete line, I think. Cf. http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67 I'd describe this perhaps like so if I were doing this patch: Fix '\ No newline...' annotation in rewrite diffs When a file that ends with an incomplete line is expressed as a complete rewrite with the -B option, git diff incorrectly appends the incomplete line indicator \ No newline at end of file after such a line, rather than writing it on a line of its own (the output codepath for normal output without -B does not have this problem). Add a LF after the incomplete line before writing the \ No newline ... out to fix this. Add a couple of tests to confirm that the indicator comment is generated on its own line in both plain diff and rewrite mode. diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh index c00a94b..1b7ae9f 100755 --- a/t/t4022-diff-rewrite.sh +++ b/t/t4022-diff-rewrite.sh @@ -66,5 +66,47 @@ test_expect_success 'suppress deletion diff with -B -D' ' grep -v Linus Torvalds actual ' +test_expect_success 'generate initial no newline at eof sequence file and commit' ' Line-wrapped. +test_expect_success 'confirm that sequence file is considered a rewrite' ' + + git diff -B seq res + grep dissimilarity index res +' Good thinking to make sure the condition to trigger the issue still holds in the future. +# Full annotation string used to check for erroneous leading or +# trailing characters. Backslash is double escaped due to usage +# within dq argument to grep expansion below. +no_newline_anno=' No newline at end of file' + +test_expect_success 'no newline at eof is on its own line without -B' ' + + git diff seq res + grep ^'$no_newline_anno'$ res I think it is sufficient to write this line as: grep ^$no_newline_anno$ res The third parameter to test_expect_success function is inside a sq, so it will have the above string as-is, with $no_newline_anno not expanded, and then when the string is eval'ed, the variable is visible to the eval. So the above should be more like: # Full annotation string used to check for erroneous leading or # trailing characters. no_newline_anno='\\ No newline at end of file' test_expect_success 'no newline at eof is on its own line without -B' ' git diff seq res grep ^$no_newline_anno$ res + grep -v ^.\\+'$no_newline_anno' res + grep -v '$no_newline_anno'.\\+$ res Converting these two the same way, we would get grep -v ^.\\+$no_newline_anno res grep -v $no_newline_anno.\\+$ res but isn't this doubly wrong? (1) \+ to require one-or-more, which is otherwise not supported in BRE, is a GNU extension. It is simple to fix it by writing ^..*$no_newline_anno to say what we try to find appears somewhere not at the beginning of line. (2) The grep -v shows the lines that express all the additions and deletions prefixed with + and - as they do not match the line has the marker misplaced in the middle of the line criteria. Doesn't grep return true in that case, as it found some matching lines, even if you had \ No newline in the middle of some lines? As I already said, I do not think hardcoding the whole No newline at end of line in this test is a good idea anyway, and because you know the text being compared does not have any backslash in it, it suffices to make sure that the only occurrence of a backslash is on a single line and at the beginning, I think. In other words, grep ^\\ res ! grep ^..*\\ res or something. I'll tentatively queue a tweaked version on 'pu', but we
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Jeff King peff at peff.net writes: - for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do + for i in $($PERL_PATH -le print for 1..$GIT_PERF_REPEAT_COUNT); do Maybe you could introduce test_seq instead. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
On Fri, Aug 03, 2012 at 07:49:47AM +, Michał Kiedrowicz wrote: Jeff King peff at peff.net writes: - for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do + for i in $($PERL_PATH -le print for 1..$GIT_PERF_REPEAT_COUNT); do Maybe you could introduce test_seq instead. I don't have a strong preference, as there are only two callsites. Do you want to make a patch? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Jeff King p...@peff.net writes: On Fri, Aug 03, 2012 at 07:49:47AM +, Michał Kiedrowicz wrote: Jeff King peff at peff.net writes: - for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do + for i in $($PERL_PATH -le print for 1..$GIT_PERF_REPEAT_COUNT); do Maybe you could introduce test_seq instead. I don't have a strong preference, as there are only two callsites. Do you want to make a patch? If you run for . in . . . in t/, we see quite a many hits, so only two callsites might be undercounting the candidates. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
On Fri, Aug 03, 2012 at 09:46:22AM -0700, Junio C Hamano wrote: Maybe you could introduce test_seq instead. I don't have a strong preference, as there are only two callsites. Do you want to make a patch? If you run for . in . . . in t/, we see quite a many hits, so only two callsites might be undercounting the candidates. True. Although a good number of them are not numeric sequences (however perl being perl, I think my one-liner would take a and g as end-points just as readily). I have no problem with converting them all. I just didn't want to personally go to the work myself. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:
From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001
From: Adam Butcher dev.li...@jessamine.co.uk
Date: Wed, 1 Aug 2012 22:25:09 +0100
Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
You can drop these lines from the email body; they are redundant with
what's in your actual header.
When operating in --break-rewrites (-B) mode on a file with no newline
terminator (and assuming --break-rewrites determines that the diff
_is_ a rewrite), git diff previously concatenated the indicator comment
'\ No newline at end of file' directly to the terminating line rather
than on a line of its own. The resulting diff is broken; claiming
that the last line actually contains the indicator text. Without -B
there is no problem with the same files.
This patch fixes the former case by inserting a newline into the
output prior to emitting the indicator comment.
Makes sense.
Potential issue: Currently this emits an ASCII 10 newline character
only. I'm not sure whether this will be okay on all platforms; it
seems to work fine on Windows and GNU at least.
This should not be a problem. Git always outputs newlines; it is stdio
who might munge it into CRLF if need be (and your patch uses putc, so we
should be fine).
A couple of tests have been added to the rewrite suite to confirm that
the indicator comment is generated on its own line in both plain diff
and rewrite mode. The latter test fails if the functional part of
this patch (i.e. diff.c) is reverted.
Yay, tests.
---
diff.c | 1 +
t/t4022-diff-rewrite.sh | 27 +++
2 files changed, 28 insertions(+)
diff --git a/diff.c b/diff.c
index 95706a5..77d4e84 100644
--- a/diff.c
+++ b/diff.c
@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct
emit_callback *ecb,
Your patch is line-wrapped and cannot be applied as-is (try turning off
flowed text in your MUA).
if (!endp) {
const char *plain = diff_get_color(ecb-color_diff,
DIFF_PLAIN);
+ putc('\n', ecb-opt-file);
emit_line_0(ecb-opt, plain, reset, '\\',
nneof, strlen(nneof));
}
Looks correct. I was curious how the regular (non-rewrite) code path did
this, and it just sticks the \n as part of the nneof string. However,
we would not want that here, because each line should have its own
color markers.
+# create a file containing numbers with no newline at
+# the end and modify it such that the starting 10 lines
+# are unchanged, the next 101 are rewritten and the last
+# line differs only in that in is terminated by a newline.
+seq 1 10 seq
+seq 100 +1 200 seq
+printf 201 seq
+(git add seq; git commit seq -m seq) /dev/null
+seq 1 10 seq
+seq 300 -1 200 seq
Seq is (unfortunately) not portable. I usually use a perl snippet
instead, like:
perl -le 'print for (1..10)'
Though I think we are adjusting that to use $PERL_PATH these days.
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Jeff King p...@peff.net writes:
On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:
From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001
From: Adam Butcher dev.li...@jessamine.co.uk
Date: Wed, 1 Aug 2012 22:25:09 +0100
Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
You can drop these lines from the email body; they are redundant with
what's in your actual header.
s/can/should/ actually, for readability.
When operating in --break-rewrites (-B) mode on a file with no newline
terminator (and assuming --break-rewrites determines that the diff
_is_ a rewrite), git diff previously concatenated the indicator comment
'\ No newline at end of file' directly to the terminating line rather
than on a line of its own. The resulting diff is broken; claiming
that the last line actually contains the indicator text. Without -B
there is no problem with the same files.
This patch fixes the former case by inserting a newline into the
output prior to emitting the indicator comment.
Makes sense.
Potential issue: Currently this emits an ASCII 10 newline character
only. I'm not sure whether this will be okay on all platforms; it
seems to work fine on Windows and GNU at least.
This should not be a problem. Git always outputs newlines; it is stdio
who might munge it into CRLF if need be (and your patch uses putc, so we
should be fine).
A couple of tests have been added to the rewrite suite to confirm that
the indicator comment is generated on its own line in both plain diff
and rewrite mode. The latter test fails if the functional part of
this patch (i.e. diff.c) is reverted.
Yay, tests.
---
Sign-off needed.
diff.c | 1 +
t/t4022-diff-rewrite.sh | 27 +++
2 files changed, 28 insertions(+)
diff --git a/diff.c b/diff.c
index 95706a5..77d4e84 100644
--- a/diff.c
+++ b/diff.c
@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct
emit_callback *ecb,
Your patch is line-wrapped and cannot be applied as-is (try turning off
flowed text in your MUA).
if (!endp) {
const char *plain = diff_get_color(ecb-color_diff,
DIFF_PLAIN);
+putc('\n', ecb-opt-file);
emit_line_0(ecb-opt, plain, reset, '\\',
nneof, strlen(nneof));
}
Looks correct. I was curious how the regular (non-rewrite) code path did
this, and it just sticks the \n as part of the nneof string. However,
we would not want that here, because each line should have its own
color markers.
+# create a file containing numbers with no newline at
+# the end and modify it such that the starting 10 lines
+# are unchanged, the next 101 are rewritten and the last
+# line differs only in that in is terminated by a newline.
+seq 1 10 seq
+seq 100 +1 200 seq
+printf 201 seq
+(git add seq; git commit seq -m seq) /dev/null
+seq 1 10 seq
+seq 300 -1 200 seq
Seq is (unfortunately) not portable. I usually use a perl snippet
instead, like:
perl -le 'print for (1..10)'
Though I think we are adjusting that to use $PERL_PATH these days.
t/perf/perf-lib.sh and t/t5551-http-fetch.sh seem to use seq;
perhaps we should replace them, then.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
Adam Butcher dev.li...@jessamine.co.uk writes: +# create a file containing numbers with no newline at +# the end and modify it such that the starting 10 lines +# are unchanged, the next 101 are rewritten and the last +# line differs only in that in is terminated by a newline. +seq 1 10 seq +seq 100 +1 200 seq +printf 201 seq +(git add seq; git commit seq -m seq) /dev/null +seq 1 10 seq +seq 300 -1 200 seq We would prefer to have these set-up steps in test_expect_success. That way, we will have more chance to catch potential and unintended breakage to git add and git commit when people attempt to update them. Also, the redirect target sticks to redirect operator in our scripts, i.e. cmd seq not cmd seq. +test_expect_success 'no newline at eof is on its own line without -B' + + (git diff seq; true) res What is this subshell and true about? A git diff does not exit with non zero to signal differences, and even if it did, the right way to write it would be test_might_fail git cmd res to allow us to make sure that the git command that may or may not exit with zero still does not die an uncontrolled death (e.g. segv). + grep ^ No newline at end of file$ res + grep -v ^.\\+ No newline at end of file res + grep -v No newline at end of file.\\+$ res +' It is preferrable not to spell No newline at ... part out, so that we won't have to worry about future rewords and i18n. There are older tests that predate i18n and they do spell these out, but that is not a good reason to make things worse than they already are. git apply only looks at the backslash-space at the beginning of line anyway. +test_expect_success 'no newline at eof is on its own line with -B' ' + + (git diff -B seq; true) res + grep ^ No newline at end of file$ res + grep -v ^.\\+ No newline at end of file res + grep -v No newline at end of file.\\+$ res +' Likewise. test_done Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
On Thu, Aug 02, 2012 at 02:52:56PM -0700, Junio C Hamano wrote:
Seq is (unfortunately) not portable. I usually use a perl snippet
instead, like:
perl -le 'print for (1..10)'
Though I think we are adjusting that to use $PERL_PATH these days.
t/perf/perf-lib.sh and t/t5551-http-fetch.sh seem to use seq;
perhaps we should replace them, then.
Traditionally, BSD did not have seq (they have jot instead). However,
my OS X 10.7 box does have seq, and its manpage claims that it appeared
in FreeBSD 9.0. But we should be able to run the test suite on older
versions of both (9.0 is barely 6 months old).
I suspect people on those platforms did not notice because t5551 does
not run by default (not only due to the apache requirement, but you have
to set GIT_TEST_LONG to trigger the particular test that uses it), and
people don't typically run the perf code regularly to look for
regressions.
-- 8 --
Subject: [PATCH] stop using 'seq' in test scripts
The seq command is GNU-ism, and is missing at least in older
BSD releases and their derivatives, not to mention antique
commercial Unixes.
We already purged it in b3431bc (Don't use seq in tests, not
everyone has it, 2007-05-02), but a few new instances have
crept in. They went unnoticed because they are in scripts
that are not run by default.
Let's replace them with a perl snippet (which we already
assume to be everywhere elsewhere in the test suite).
---
b3431bc used a while loop with increment to replace it, which we could
also do. I think the perl script is a little easier to read. If we
ever want to drop the perl dependency for the test suite, we could write
a 5-liner test-seq.c replacement.
t/perf/perf-lib.sh| 2 +-
t/t5551-http-fetch.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5580c22..8bf8d69 100644
--- a/t/perf/perf-lib.sh
+++ b/t/perf/perf-lib.sh
@@ -163,7 +163,7 @@ test_perf () {
else
echo perf $test_count - $1:
fi
- for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
+ for i in $($PERL_PATH -le print for
1..$GIT_PERF_REPEAT_COUNT); do
say 3 running: $2
if test_run_perf_ $2
then
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index fadf2f2..e858a31 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -114,7 +114,7 @@ test -n $GIT_TEST_LONG test_set_prereq EXPENSIVE
test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
(
cd $HTTPD_DOCUMENT_ROOT_PATH/repo.git
- for i in `seq 5`
+ for i in `$PERL_PATH -le print for (1..5)`
do
echo commit refs/heads/too-many-refs
echo mark :$i
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
On 02.08.2012 22:33, Jeff King wrote:
On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:
From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00
2001
From: Adam Butcher dev.li...@jessamine.co.uk
Date: Wed, 1 Aug 2012 22:25:09 +0100
Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
You can drop these lines from the email body; they are redundant with
what's in your actual header.
I sent via a webmail interface and wasn't sure what format the
resulting mail would have so decided to paste the entire formatted patch
in. Seeing as the webmailer has corrupted my patch with word wrapping
(which I noticed almost immediately when my post hit gmane and have
since found out that it apparently cannot be disabled?!) it was a bad
idea all round. I could attach as a file but this is cumbersome from a
review and apply point of view so I think I'll hook up git to gmail's
tls smtp server so that I can use git send-email direct rather than
messing about with a GUI.
When operating in --break-rewrites (-B) mode on a file with no
newline
terminator (and assuming --break-rewrites determines that the diff
_is_ a rewrite), git diff previously concatenated the indicator
comment
'\ No newline at end of file' directly to the terminating line
rather
than on a line of its own. The resulting diff is broken; claiming
that the last line actually contains the indicator text. Without -B
there is no problem with the same files.
This patch fixes the former case by inserting a newline into the
output prior to emitting the indicator comment.
Makes sense.
Potential issue: Currently this emits an ASCII 10 newline character
only. I'm not sure whether this will be okay on all platforms; it
seems to work fine on Windows and GNU at least.
This should not be a problem. Git always outputs newlines; it is
stdio
who might munge it into CRLF if need be (and your patch uses putc, so
we
should be fine).
Great.
A couple of tests have been added to the rewrite suite to confirm
that
the indicator comment is generated on its own line in both plain
diff
and rewrite mode. The latter test fails if the functional part of
this patch (i.e. diff.c) is reverted.
Yay, tests.
---
diff.c | 1 +
t/t4022-diff-rewrite.sh | 27 +++
2 files changed, 28 insertions(+)
diff --git a/diff.c b/diff.c
index 95706a5..77d4e84 100644
--- a/diff.c
+++ b/diff.c
@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct
emit_callback *ecb,
Your patch is line-wrapped and cannot be applied as-is (try turning
off
flowed text in your MUA).
Indeed. Grr. If only I could. I'll test that whatever solution I
come up with works before posting again with an update addressing yours
and Junio's comments.
if (!endp) {
const char *plain = diff_get_color(ecb-color_diff,
DIFF_PLAIN);
+ putc('\n', ecb-opt-file);
emit_line_0(ecb-opt, plain, reset, '\\',
nneof, strlen(nneof));
}
Looks correct. I was curious how the regular (non-rewrite) code path
did
this, and it just sticks the \n as part of the nneof string.
However,
we would not want that here, because each line should have its own
color markers.
+# create a file containing numbers with no newline at
+# the end and modify it such that the starting 10 lines
+# are unchanged, the next 101 are rewritten and the last
+# line differs only in that in is terminated by a newline.
+seq 1 10 seq
+seq 100 +1 200 seq
+printf 201 seq
+(git add seq; git commit seq -m seq) /dev/null
+seq 1 10 seq
+seq 300 -1 200 seq
Seq is (unfortunately) not portable. I usually use a perl snippet
instead, like:
perl -le 'print for (1..10)'
Though I think we are adjusting that to use $PERL_PATH these days.
No probs. Will change.
Cheers
Adam
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.
On 02.08.2012 23:00, Junio C Hamano wrote: Adam Butcher dev.li...@jessamine.co.uk writes: +# create a file containing numbers with no newline at +# the end and modify it such that the starting 10 lines +# are unchanged, the next 101 are rewritten and the last +# line differs only in that in is terminated by a newline. +seq 1 10 seq +seq 100 +1 200 seq +printf 201 seq +(git add seq; git commit seq -m seq) /dev/null +seq 1 10 seq +seq 300 -1 200 seq We would prefer to have these set-up steps in test_expect_success. That way, we will have more chance to catch potential and unintended breakage to git add and git commit when people attempt to update them. Cool, no probs. I had originally put them at the start of the first test that I added but decided to pull them out as prep. I think that msysGit or something about my Windows shell session may have played a part in my not chaining them with also (see below). I'll clean them up and wrap them in a test_expect_success. Also, the redirect target sticks to redirect operator in our scripts, i.e. cmd seq not cmd seq. Okay, will change. +test_expect_success 'no newline at eof is on its own line without -B' + + (git diff seq; true) res What is this subshell and true about? A git diff does not exit with non zero to signal differences, Hmm, (?confused?) yeah actually I didn't think it did -- I was surprised when git returned 1 for this line. I think it must have been an issue with the version msysGit I was using or something sticking errorlevel in my Windows shell. Git seemed to return 1 ALWAYS! I usually use gnu/linux but on this occasion I wrote the fix and tests blind on a Windows machine testing the logic manually with msysGit. I ran the tests on a linux machine at work and they did what I expected so I left them as was without rechecking this. I'm glad that this can be simplified. It felt wrong -- similar lines elsewhere in the script didn't do it so I wasn't really happy with it. Turns out it looks to be a Windows/environment issue. I cannot reproduce it now. and even if it did, the right way to write it would be test_might_fail git cmd res Fair enough. Good to know that's available. to allow us to make sure that the git command that may or may not exit with zero still does not die an uncontrolled death (e.g. segv). + grep ^ No newline at end of file$ res + grep -v ^.\\+ No newline at end of file res + grep -v No newline at end of file.\\+$ res +' It is preferrable not to spell No newline at ... part out, so that we won't have to worry about future rewords and i18n. Okay no probs. I was originally going to spell it out only once and use parameter expansion. However I understand the point of not spelling it out at all. The only reason I did so was to catch other potential errors where text may have been 'attached' to either side of the annotation string by some future (or other) bug. There are older tests that predate i18n and they do spell these out, but that is not a good reason to make things worse than they already are. Agreed. Should I just test for this prefix case (i.e. the bug at hand) only and not preempt future potential issues? Or should I just stick the current string in a variable and keep the logic as is; at least then there would only be one place requiring a fix in the reword case (but still additional rework in the i18n case -- though I assume the test could force a particular locale to evade this). git apply only looks at the backslash-space at the beginning of line anyway. Okay. +test_expect_success 'no newline at eof is on its own line with -B' ' + + (git diff -B seq; true) res + grep ^ No newline at end of file$ res + grep -v ^.\\+ No newline at end of file res + grep -v No newline at end of file.\\+$ res +' Likewise. test_done Thanks. No probs. I will address both your and Jeff's comments sometime tomorrow and hopefully send a well formatted patch in next time. Cheers, Adam -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
