Adam Butcher <dev.lists@xxxxxxxxxxxxxxx> 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 would at least want a sign-off. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html