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.
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' '
> +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
I'll tentatively queue a tweaked version on 'pu', but we would at
least want a sign-off.
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