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