Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.

Adam Butcher [off-list ref] 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.

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.