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

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

>  	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));
>  	}

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

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

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

> > 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.
> 
> t/perf/perf-lib.sh and t/t5551-http-fetch.sh seem to use "seq";
> perhaps we should replace them, then.

> Jeff King <peff <at> peff.net> writes:
> 
> > -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
> > +		for i in $("$PERL_PATH" -le "print for 1..$GIT_PERF_REPEAT_COUNT"); do
> 
> Maybe you could introduce "test_seq" instead.

> On Fri, Aug 03, 2012 at 07:49:47AM +0000, Michał Kiedrowicz wrote:
>
>> Jeff King <peff <at> peff.net> writes:
>> 
>> > -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
>> > +		for i in $("$PERL_PATH" -le "print for 1..$GIT_PERF_REPEAT_COUNT"); do
>> 
>> Maybe you could introduce "test_seq" instead.
>
> I don't have a strong preference, as there are only two callsites. Do
> you want to make a patch?

> >> Maybe you could introduce "test_seq" instead.
> >
> > I don't have a strong preference, as there are only two callsites. Do
> > you want to make a patch?
> 
> If you run "for . in . . ." in t/, we see quite a many hits, so
> "only two callsites" might be undercounting the candidates.

> Jeff King wrote:
> 
> 	The seq command is GNU-ism, and is missing at least in older BSD
> 	releases and their derivatives, not to mention antique
> 	commercial Unixes.
> 
> 	We already purged it in b3431bc (Don't use seq in tests, not
> 	everyone has it, 2007-05-02), but a few new instances have crept
> 	in. They went unnoticed because they are in scripts that are not
> 	run by default.
> 
> This commit replaces them with test_seq that is implemented with a Perl
> snippet (proposed by Jeff).  This is better than inlining this snippet
> everywhere it's needed because it's easier to read and it's easier to
> change the implementation (e.g. to C) if we ever decide to remove Perl
> from the test suite.
> 
> Note that test_seq is not a complete replacement for seq(1).  It just
> has what we need now.
> 
> There are also many places that do `for i in 1 2 3 ...` but I'm not sure
> if it's worth converting them to test_seq.  That would introduce running
> more processes of Perl during the tests and might increase the total
> time tests take.
> 
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>

> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 5580c22..a1361e5 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -163,7 +163,7 @@ test_perf () {
>  		else
>  			echo "perf $test_count - $1:"
>  		fi
> -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
> +		for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do

> +# test_seq is a portable replacement for seq(1).
> +# It may be used like:
> +#
> +#	for i in `test_seq 100`; do
> +#		echo $i
> +#	done
> +
> +test_seq () {
> +	test $# = 1 ||
> +	error "bug in the test script: not 1 parameter to test_seq"
> +	last=$1
> +	"$PERL_PATH" -le "print for 1..$last"
> +}

> On Fri, Aug 03, 2012 at 09:57:15PM +0200, Michał Kiedrowicz wrote:
>
>> Jeff King wrote:
>> 
>> 	The seq command is GNU-ism, and is missing at least in older BSD
>> 	releases and their derivatives, not to mention antique
>> 	commercial Unixes.
>> 
>> 	We already purged it in b3431bc (Don't use seq in tests, not
>> 	everyone has it, 2007-05-02), but a few new instances have crept
>> 	in. They went unnoticed because they are in scripts that are not
>> 	run by default.
>> 
>> This commit replaces them with test_seq that is implemented with a Perl
>> snippet (proposed by Jeff).

> Fine explanation, but...
>
>> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
>> index 5580c22..a1361e5 100644
>> --- a/t/perf/perf-lib.sh
>> +++ b/t/perf/perf-lib.sh
>> @@ -163,7 +163,7 @@ test_perf () {
>>  		else
>>  			echo "perf $test_count - $1:"
>>  		fi
>> -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
>> +		for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
>
> Two args to test_seq, but...
>
>> +# test_seq is a portable replacement for seq(1).
>> +# It may be used like:
>> +#
>> +#	for i in `test_seq 100`; do
>> +#		echo $i
>> +#	done
>> +
>> +test_seq () {
>> +	test $# = 1 ||
>> +	error "bug in the test script: not 1 parameter to test_seq"
>> +	last=$1
>> +	"$PERL_PATH" -le "print for 1..$last"
>> +}
>
> it wants only one.
>
> I think you would want:
>
>   test $# = 1 && set -- 1 "$@"
>   "$PERL_PATH" -le "print for $1..$2"
>
> It might also be worth quoting the parameters like this:
>
>   "$PERL_PATH" -le "print for '$1'..'$2'"
>
> so that "test_seq a f" works, too.

> Wouldn't it be cleaner and readable to write it like this
> 
> 	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$1" "$2"
> 
> by the way?

> Jeff King <peff@peff.net> writes:
> 
> > On Fri, Aug 03, 2012 at 09:57:15PM +0200, Michał Kiedrowicz wrote:
> >
> >> Jeff King wrote:
> >> 
> >> 	The seq command is GNU-ism, and is missing at least in older BSD
> >> 	releases and their derivatives, not to mention antique
> >> 	commercial Unixes.
> >> 
> >> 	We already purged it in b3431bc (Don't use seq in tests, not
> >> 	everyone has it, 2007-05-02), but a few new instances have crept
> >> 	in. They went unnoticed because they are in scripts that are not
> >> 	run by default.
> >> 
> >> This commit replaces them with test_seq that is implemented with a Perl
> >> snippet (proposed by Jeff).
> 
> Just say "Replace them with test_seq...", without "This commit".
> 
> > Fine explanation, but...
> >
> >> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> >> index 5580c22..a1361e5 100644
> >> --- a/t/perf/perf-lib.sh
> >> +++ b/t/perf/perf-lib.sh
> >> @@ -163,7 +163,7 @@ test_perf () {
> >>  		else
> >>  			echo "perf $test_count - $1:"
> >>  		fi
> >> -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
> >> +		for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
> >
> > Two args to test_seq, but...
> >
> >> +# test_seq is a portable replacement for seq(1).
> >> +# It may be used like:
> >> +#
> >> +#	for i in `test_seq 100`; do
> >> +#		echo $i
> >> +#	done
> >> +
> >> +test_seq () {
> >> +	test $# = 1 ||
> >> +	error "bug in the test script: not 1 parameter to test_seq"
> >> +	last=$1
> >> +	"$PERL_PATH" -le "print for 1..$last"
> >> +}
> >
> > it wants only one.
> >
> > I think you would want:
> >
> >   test $# = 1 && set -- 1 "$@"
> >   "$PERL_PATH" -le "print for $1..$2"
> >
> > It might also be worth quoting the parameters like this:
> >
> >   "$PERL_PATH" -le "print for '$1'..'$2'"
> >
> > so that "test_seq a f" works, too.
> 
> Yeah, I like that last one, but then unlike the claim in the comment
> before the function definition, it is not "a portable replacement
> for seq(1)" at all, but something a lot more suited for our purpose.
> So at least the comment needs to be updated.  I do not have strong
> opinion on calling this test_seq when it acts differently from seq;
> it is not confusing enough to make me push something longer that is
> different from "seq", e.g. test_sequence.
> 

> Wouldn't it be cleaner and readable to write it like this
> 
> 	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$1" "$2"
> 
> by the way?

> Junio C Hamano <gitster@pobox.com> wrote:
>> I do not have strong
>> opinion on calling this test_seq when it acts differently from seq;
>> it is not confusing enough to make me push something longer that is
>> different from "seq", e.g. test_sequence.
> 
> I prefer "test_seq" because it reminds seq which helps learning how to
> use it.  If some other seq feature is ever needed (e.g. increment value,
> decrementing), it may be added at any time (but I don't think so, there
> are only few usages after years of test suite existence).

> And the reason for this is that we always told people "don't use seq"
> and they submitted an updated patch. What would we have to do now? We
> have to tell them "don't use seq, use test_seq". Therefore, the patch
> does not accomplish anything useful, IMO.
>
> The function should really just be named 'seq'.
>
> Or how about this strategy:
> ...
> but it is not my favorite.

> Am 04.08.2012 00:09, schrieb Michał Kiedrowicz:
> > Junio C Hamano <gitster@pobox.com> wrote:
> >> I do not have strong
> >> opinion on calling this test_seq when it acts differently from seq;
> >> it is not confusing enough to make me push something longer that is
> >> different from "seq", e.g. test_sequence.
> > 
> > I prefer "test_seq" because it reminds seq which helps learning how to
> > use it.  If some other seq feature is ever needed (e.g. increment value,
> > decrementing), it may be added at any time (but I don't think so, there
> > are only few usages after years of test suite existence).
> 
> And the reason for this is that we always told people "don't use seq"
> and they submitted an updated patch. What would we have to do now? We
> have to tell them "don't use seq, use test_seq". Therefore, the patch
> does not accomplish anything useful, IMO.
> 
> The function should really just be named 'seq'.

> 
> Or how about this strategy:
> 
> seq () {
> 	unset -f seq
> 	if ! seq 1 2 >/dev/null 2>&1
> 	then
> 		# don't have a working seq; provide it as a function
> 		seq () {
> 			insert your definition here
> 		}
> 	fi
> 	seq "$@"
> }
> 
> but it is not my favorite.
> 
> -- Hannes

> And the reason for this is that we always told people "don't use seq"
> and they submitted an updated patch. What would we have to do now? We
> have to tell them "don't use seq, use test_seq". Therefore, the patch
> does not accomplish anything useful, IMO.
> 
> The function should really just be named 'seq'.
> 
> Or how about this strategy:
> 
> seq () {
> 	unset -f seq
> 	if ! seq 1 2 >/dev/null 2>&1
> 	then
> 		# don't have a working seq; provide it as a function
> 		seq () {
> 			insert your definition here
> 		}
> 	fi
> 	seq "$@"
> }
> 
> but it is not my favorite.

> Jeff King wrote:
>
> 	The seq command is GNU-ism, and is missing at least in older BSD
> 	releases and their derivatives, not to mention antique
> 	commercial Unixes.
>
> 	We already purged it in b3431bc (Don't use seq in tests, not
> 	everyone has it, 2007-05-02), but a few new instances have crept
> 	in. They went unnoticed because they are in scripts that are not
> 	run by default.
>
> Replace them with test_seq that is implemented with a Perl snippet
> (proposed by Jeff).  This is better than inlining this snippet
> everywhere it's needed because it's easier to read and it's easier to
> change the implementation (e.g. to C) if we ever decide to remove Perl
> from the test suite.
>
> Note that test_seq is not a complete replacement for seq(1).  It just
> has what we need now.
>
> There are also many places that do `for i in 1 2 3 ...` but I'm not sure
> if it's worth converting them to test_seq.  That would introduce running
> more processes of Perl.
>
> Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> ---

> Changes since previous version:
>
> 	* Removed "This commit replaces" from commit message
> 	* Reworded test_seq description
> 	* Now $first and $last are passed to Perl as arguments
>
>  t/perf/perf-lib.sh      |  2 +-
>  t/t5551-http-fetch.sh   |  2 +-
>  t/test-lib-functions.sh | 20 ++++++++++++++++++++
>  3 files changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
> index 5580c22..a1361e5 100644
> --- a/t/perf/perf-lib.sh
> +++ b/t/perf/perf-lib.sh
> @@ -163,7 +163,7 @@ test_perf () {
>  		else
>  			echo "perf $test_count - $1:"
>  		fi
> -		for i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do
> +		for i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do
>  			say >&3 "running: $2"
>  			if test_run_perf_ "$2"
>  			then
> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
> index fadf2f2..91eaf53 100755
> --- a/t/t5551-http-fetch.sh
> +++ b/t/t5551-http-fetch.sh
> @@ -114,7 +114,7 @@ test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
>  test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
>  	(
>  	cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
> -	for i in `seq 50000`
> +	for i in `test_seq 50000`
>  	do
>  		echo "commit refs/heads/too-many-refs"
>  		echo "mark :$i"
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index 80daaca..c8b4ae3 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -530,6 +530,26 @@ test_cmp() {
>  	$GIT_TEST_CMP "$@"
>  }
>  
> +# Print a sequence of numbers or letters in increasing order.  This is
> +# similar to GNU seq(1), but the latter might not be available
> +# everywhere.  It may be used like:
> +#
> +#	for i in `test_seq 100`; do
> +#		for j in `test_seq 10 20`; do
> +#			for k in `test_seq a z`; do
> +#				echo $i-$j-$k
> +#			done
> +#		done
> +#	done
> +
> +test_seq () {
> +	test $# = 2 && { first=$1; shift; } || first=1
> +	test $# = 1 ||
> +	error "bug in the test script: not 1 or 2 parameters to test_seq"
> +	last=$1
> +	"$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"

> +}
> +
>  # This function can be used to schedule some commands to be run
>  # unconditionally at the end of the test to restore sanity:
>  #

> Michał Kiedrowicz <michal.kiedrowicz@gmail.com> writes:
> 
> > Jeff King wrote:
> >
> > 	The seq command is GNU-ism, and is missing at least in older BSD
> > 	releases and their derivatives, not to mention antique
> > 	commercial Unixes.
> >
> > 	We already purged it in b3431bc (Don't use seq in tests, not
> > 	everyone has it, 2007-05-02), but a few new instances have crept
> > 	in. They went unnoticed because they are in scripts that are not
> > 	run by default.
> >
> > Replace them with test_seq that is implemented with a Perl snippet
> > (proposed by Jeff).  This is better than inlining this snippet
> > everywhere it's needed because it's easier to read and it's easier to
> > change the implementation (e.g. to C) if we ever decide to remove Perl
> > from the test suite.
> >
> > Note that test_seq is not a complete replacement for seq(1).  It just
> > has what we need now.
> >
> > There are also many places that do `for i in 1 2 3 ...` but I'm not sure
> > if it's worth converting them to test_seq.  That would introduce running
> > more processes of Perl.
> >
> > Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
> > ---
> 
> Thanks; Jeff, ack?

> Tentatively I'll queue this one on top, but I am tempted to squash
> this in before merging the topic down.
> 
> -- >8 --
> Subject: [PATCH] fixup! tests: Introduce test_seq
> 
> Complex chains of && and || are harder to read when used as
> replacement for if/else statements, but it is easy to rewrite it
> with a case/esac in this case.

> 
> Avoid using unnecessary variables $first and $last.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  t/test-lib-functions.sh | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index c8b4ae3..7dc70eb 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -532,7 +532,7 @@ test_cmp() {
>  
>  # Print a sequence of numbers or letters in increasing order.  This is
>  # similar to GNU seq(1), but the latter might not be available
> -# everywhere.  It may be used like:
> +# everywhere (and does not do letters).  It may be used like:
>  #
>  #	for i in `test_seq 100`; do
>  #		for j in `test_seq 10 20`; do
> @@ -543,11 +543,12 @@ test_cmp() {
>  #	done
>  
>  test_seq () {
> -	test $# = 2 && { first=$1; shift; } || first=1
> -	test $# = 1 ||
> -	error "bug in the test script: not 1 or 2 parameters to test_seq"
> -	last=$1
> -	"$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
> +	case $# in
> +	1)	set 1 "$@" ;;
> +	2)	;;
> +	*)	error "bug in the test script: not 1 or 2 parameters to test_seq" ;;
> +	esac
> +	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
>  }
>  
>  # This function can be used to schedule some commands to be run

> Junio C Hamano <gitster <at> pobox.com> wrote:
> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> > index c8b4ae3..7dc70eb 100644
> > --- a/t/test-lib-functions.sh
> > +++ b/t/test-lib-functions.sh
> > @@ -543,11 +543,12 @@ test_cmp() {
> >  #	done
> >  
> >  test_seq () {
> > -	test $# = 2 && { first=$1; shift; } || first=1
> > -	test $# = 1 ||
> > -	error "bug in the test script: not 1 or 2 parameters to test_seq"
> > -	last=$1
> > -	"$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
> > +	case $# in
> > +	1)	set 1 "$@" ;;
> > +	2)	;;
> > +	*)	error "bug in the test script: not 1 or 2 parameters to 

> > +	esac
> > +	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
> >  }
> >  
> >  # This function can be used to schedule some commands to be run

> +# test_seq is a portable replacement for seq(1).
> +# It may be used like:
> +#
> +#	for i in `test_seq 100`; do
> +#		echo $i
> +#	done

> On Fri, Aug 03, 2012 at 10:04:50PM +0200, Michał Kiedrowicz wrote:
> 
> > Previous patch didn't support `test_seq 1 50` (I removed it accidentally).
> 
> Our emails just crossed paths. :)

> 
> > +# test_seq is a portable replacement for seq(1).
> > +# It may be used like:
> > +#
> > +#	for i in `test_seq 100`; do
> > +#		echo $i
> > +#	done
> 
> This should probably note that it is a subset of seq's behavior. You
> talked about it in the commit message, but the in-code comment is a much
> more likely thing for a potential user to read.
> 
> -Peff

> Changes since previous patch:
> 
> 	* Added quotes around arguments, allowing `test_seq a z`
> 	* Improved test_seq comments
> 
>  t/perf/perf-lib.sh      |  2 +-
>  t/t5551-http-fetch.sh   |  2 +-
>  t/test-lib-functions.sh | 19 +++++++++++++++++++
>  3 files changed, 21 insertions(+), 2 deletions(-)

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

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

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

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

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

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

> Adam Butcher <dev.lists@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.
>

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

> and even if it did, the right way to
> write it would be
>
> 	test_might_fail git cmd >res &&
>

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

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

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

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

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