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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 5, 2012, 01:26 UTC
Message-ID: <7vobmq6sd9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <loom.20120804T230218-811@post.gmane.org>

Adam Butcher <dev.lists@jessamine.co.uk> writes:

Show 16 quoted lines

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

Show 10 quoted lines

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

Show 5 quoted lines

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

Show 9 quoted lines

> +# 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.