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

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

[Adam Butcher]

 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.

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.

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.

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.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));
  	}
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index c00a94b..c85154d 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -66,5 +66,32 @@ test_expect_success 'suppress deletion diff with -B 
-D' '
  	grep -v "Linus Torvalds" actual
  '

+# 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 without -B' 
'
+
+	(git diff 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
+'
+
+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
+'
+
  test_done

-- 
1.7.11.msysgit.0

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

[Jeff King]

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.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

s/can/should/ actually, for readability.

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

Sign-off needed.

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

t/perf/perf-lib.sh and t/t5551-http-fetch.sh seem to use "seq";
perhaps we should replace them, then.

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

[Jeff King]

On Thu, Aug 02, 2012 at 02:52:56PM -0700, Junio C Hamano wrote:

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

Traditionally, BSD did not have seq (they have "jot" instead). However,
my OS X 10.7 box does have seq, and its manpage claims that it appeared
in FreeBSD 9.0. But we should be able to run the test suite on older
versions of both (9.0 is barely 6 months old).

I suspect people on those platforms did not notice because t5551 does
not run by default (not only due to the apache requirement, but you have
to set GIT_TEST_LONG to trigger the particular test that uses it), and
people don't typically run the perf code regularly to look for
regressions.

-- >8 --
Subject: [PATCH] stop using 'seq' in test scripts

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.

Let's replace them with a perl snippet (which we already
assume to be everywhere elsewhere in the test suite).
---
b3431bc used a while loop with increment to replace it, which we could
also do. I think the perl script is a little easier to read. If we
ever want to drop the perl dependency for the test suite, we could write
a 5-liner test-seq.c replacement.

 t/perf/perf-lib.sh    | 2 +-
 t/t5551-http-fetch.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh
index 5580c22..8bf8d69 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 $("$PERL_PATH" -le "print for 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..e858a31 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 `"$PERL_PATH" -le "print for (1..50000)"`
 	do
 		echo "commit refs/heads/too-many-refs"
 		echo "mark :$i"

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

[Michał Kiedrowicz]

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.

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

[Jeff King]

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?

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

If you run "for . in . . ." in t/, we see quite a many hits, so
"only two callsites" might be undercounting the candidates.

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

[Jeff King]

On Fri, Aug 03, 2012 at 09:46:22AM -0700, Junio C Hamano wrote:

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

True. Although a good number of them are not numeric sequences (however
perl being perl, I think my one-liner would take "a" and "g" as
end-points just as readily).

I have no problem with converting them all. I just didn't want to
personally go to the work myself.

-Peff

[PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

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

> I don't have a strong preference, as there are only two callsites. Do
> you want to make a patch?

Sure. Here it is.

 t/perf/perf-lib.sh      |  2 +-
 t/t5551-http-fetch.sh   |  2 +-
 t/test-lib-functions.sh | 14 ++++++++++++++
 3 files changed, 16 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..7d7424d 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -530,6 +530,20 @@ test_cmp() {
 	$GIT_TEST_CMP "$@"
 }
 
+# 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"
+}
+
 # This function can be used to schedule some commands to be run
 # unconditionally at the end of the test to restore sanity:
 #
-- 
1.7.11.rc0.212.g37218b0.dirty

Re: [PATCH] tests: Introduce test_seq

[Jeff King]

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

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.

-Peff

Re: [PATCH] tests: Introduce test_seq

[Junio C Hamano]

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?

Re: [PATCH] tests: Introduce test_seq

[Jeff King]

On Fri, Aug 03, 2012 at 01:53:15PM -0700, Junio C Hamano wrote:

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

Yeah, that would be more robust (it's longer to type, which is why I
avoided it in the inline replacement, but since we're factoring it out,
that's not an issue).

-Peff

Re: [PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

Junio C Hamano <gitster@pobox.com> wrote:

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

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

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

Re: [PATCH] tests: Introduce test_seq

[Johannes Sixt]

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

Re: [PATCH] tests: Introduce test_seq

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

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

Why not?  That implementation looks like a logical and natural
consequence of "should relly just be named 'seq'" suggestion.

Having said that, we already say "don't use cmp, use test_cmp", so
it might not be such a big deal, even though I find the reasoning in
the first paragraph I quoted above from your message quite sane and
convincing to me.

Re: [PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

Johannes Sixt <j6t@kdbg.org> wrote:

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

My reasoning was that there is already test_cmp, so let's make test_seq,
but I agree with you that it doesn't solve the issue completely. So my 2
cents is that it would be best to stay with not allowing seq in the test
suite.

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

Re: [PATCH] tests: Introduce test_seq

[Jeff King]

On Sat, Aug 04, 2012 at 06:38:08PM +0200, Johannes Sixt wrote:

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

No, falling back just makes that problem worse. Our test_seq is not
fully compatible with seq. So anyone who uses an advanced feature of seq
(like "seq 0 100 10" or "seq -f %02g 1 10") will have the test work on
their system (with seq) and then break on some other random platform.
So instead of saying "no, don't use seq, use test_seq", reviewers have
to catch it and say "don't use some features of seq, because the
fallback doesn't have them".

If you eliminate the fallback, then at least the reviewers do not have
to catch it (the tests will never work for the patch writer, since they
will always use our feature-less seq replacement). But I find it
slightly confusion-inducing to call something that is not seq-compatible
"seq".

-Peff

[PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

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:
 #
-- 
1.7.11.rc0.212.g37218b0.dirty

Re: [PATCH] tests: Introduce test_seq

[Junio C Hamano]

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?

I have one minor nit that I am tempted to fix while queuing---see
below.

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

I'd prefer not to have dq around $ARGV[]; is there a reason to have
one around these?

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

Re: [PATCH] tests: Introduce test_seq

[Jeff King]

On Fri, Aug 03, 2012 at 03:48:19PM -0700, Junio C Hamano wrote:

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

Yeah,

Acked-by: Jeff King <peff@peff.net>

> > +	"$PERL_PATH" -le 'print for "$ARGV[0]".."$ARGV[1]"' "$first" "$last"
> 
> I'd prefer not to have dq around $ARGV[]; is there a reason to have
> one around these?

I don't think they accomplish anything, and it is slightly easier to
read without them. I'm fine either way.

-Peff

Re: [PATCH] tests: Introduce test_seq

[Junio C Hamano]

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
-- 
1.7.12.rc1.50.g3df08cf

Re: [PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

Junio C Hamano <gitster@pobox.com> wrote:

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

I just copied it from test_expect_success, but yeah, case/esac is
clearer.

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

Re: [PATCH] tests: Introduce test_seq

[Adam Butcher]

Michał Kiedrowicz <michal.kiedrowicz <at> gmail.com> writes:
> 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 
test_seq" ;;
> > +	esac
> > +	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' "$@"
> >  }
> >  
> >  # This function can be used to schedule some commands to be run

-- >8 --
Subject: [PATCH] Fixup test_seq: ensure arguments passed to script.

If the arguments passed to to test_seq start with '-' (e.g. negative
integers) they are considered perl options and the program errors.  By
prefixing the user argument list with '--' when passing to perl, this
is avoid and sequences involving negative numbers are possible.
---
 t/test-lib-functions.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 5a1a95a..ed44f5e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -539,7 +539,7 @@ test_seq () {
 	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]' "$@"
+	"$PERL_PATH" -le 'print for $ARGV[0]..$ARGV[1]' -- "$@"
 }
 
 # This function can be used to schedule some commands to be run
-- 
1.7.11.msysgit.1.1.gf0affa1

[PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

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.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
Previous patch didn't support `test_seq 1 50` (I removed it accidentally).

 t/perf/perf-lib.sh      |  2 +-
 t/t5551-http-fetch.sh   |  2 +-
 t/test-lib-functions.sh | 15 +++++++++++++++
 3 files changed, 17 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..9456e65 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -530,6 +530,21 @@ test_cmp() {
 	$GIT_TEST_CMP "$@"
 }
 
+# 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 $# = 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 $first..$last"
+}
+
 # This function can be used to schedule some commands to be run
 # unconditionally at the end of the test to restore sanity:
 #
-- 
1.7.11.rc0.212.g37218b0.dirty

Re: [PATCH] tests: Introduce test_seq

[Jeff King]

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

Re: [PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

Jeff King <peff@peff.net> wrote:

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

Yeah :)

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

OK, I'll quote parameters and add a note.

[PATCH] tests: Introduce test_seq

[Michał Kiedrowicz]

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.

Signed-off-by: Michał Kiedrowicz <michal.kiedrowicz@gmail.com>
---
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(-)

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..bed1f57 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -530,6 +530,25 @@ test_cmp() {
 	$GIT_TEST_CMP "$@"
 }
 
+# test_seq is a portable yet not complete replacement for seq(1).
+# 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 '$first'..'$last'"
+}
+
 # This function can be used to schedule some commands to be run
 # unconditionally at the end of the test to restore sanity:
 #
-- 
1.7.11.rc0.212.g37218b0.dirty

Re: [PATCH] tests: Introduce test_seq

[Jeff King]

On Fri, Aug 03, 2012 at 10:38:24PM +0200, Michał Kiedrowicz wrote:

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

I think this version looks OK.

-Peff

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

[Adam Butcher]

On 02.08.2012 22:33, Jeff King wrote:
> 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.
>
I sent via a webmail interface and wasn't sure what format the 
resulting mail would have so decided to paste the entire formatted patch 
in.  Seeing as the webmailer has corrupted my patch with word wrapping 
(which I noticed almost immediately when my post hit gmane and have 
since found out that it apparently cannot be disabled?!) it was a bad 
idea all round.  I could attach as a file but this is cumbersome from a 
review and apply point of view so I think I'll hook up git to gmail's 
tls smtp server so that I can use git send-email direct rather than 
messing about with a GUI.

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

>> 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).
>
Indeed.  Grr.  If only I could.  I'll test that whatever solution I 
come up with works before posting again with an update addressing yours 
and Junio's comments.

>>  	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.
>
No probs.  Will change.

Cheers
Adam

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

[Junio C Hamano]

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.

Also, the redirect target sticks to redirect operator in our
scripts, i.e. "cmd >seq" not "cmd > seq".

> +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.  There are older
tests that predate i18n and they do spell these out, but that is not
a good reason to make things worse than they already are.

"git apply" only looks at the backslash-space at the beginning of
line anyway.

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

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

[Adam Butcher]

On 02.08.2012 23:00, Junio C Hamano wrote:
> 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.
>
Cool, no probs.  I had originally put them at the start of the first 
test that I added but decided to pull them out as prep.  I think that 
msysGit or something about my Windows shell session may have played a 
part in my not chaining them with && also (see below).  I'll clean them 
up and wrap them in a test_expect_success.

> Also, the redirect target sticks to redirect operator in our
> scripts, i.e. "cmd >seq" not "cmd > seq".
>
Okay, will change.

>> +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,
>
Hmm, (?confused?) yeah actually I didn't think it did -- I was 
surprised when git returned 1 for this line.  I think it must have been 
an issue with the version msysGit I was using or something sticking 
errorlevel in my Windows shell.  Git seemed to return 1 ALWAYS!  I 
usually use gnu/linux but on this occasion I wrote the fix and tests 
blind on a Windows machine testing the logic manually with msysGit.  I 
ran the tests on a linux machine at work and they did what I expected so 
I left them as was without rechecking this.

I'm glad that this can be simplified.  It felt wrong -- similar lines 
elsewhere in the script didn't do it so I wasn't really happy with it.  
Turns out it looks to be a Windows/environment issue.  I cannot 
reproduce it now.

> and even if it did, the right way to
> write it would be
>
> 	test_might_fail git cmd >res &&
>
Fair enough.  Good to know that's available.

> 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.
>
Okay no probs.  I was originally going to spell it out only once and 
use parameter expansion.  However I understand the point of not spelling 
it out at all.  The only reason I did so was to catch other potential 
errors where text may have been 'attached' to either side of the 
annotation string by some future (or other) bug.

> There are older
> tests that predate i18n and they do spell these out, but that is not
> a good reason to make things worse than they already are.
>
Agreed.  Should I just test for this prefix case (i.e. the bug at hand) 
only and not preempt future potential issues?  Or should I just stick 
the current string in a variable and keep the logic as is; at least then 
there would only be one place requiring a fix in the reword case (but 
still additional rework in the i18n case -- though I assume the test 
could force a particular locale to evade this).

> "git apply" only looks at the backslash-space at the beginning of
> line anyway.
>
Okay.

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

No probs.  I will address both your and Jeff's comments sometime 
tomorrow and hopefully send a well formatted patch in next time.

Cheers,
Adam

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

[Adam Butcher]

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

Updates: Test only:

  - removed redundant para from commit msg
  - use test_seq shell function instead of seq
  - pull prep statements into individual tests
  - test expected success of git commands in prep
  - confirm that rewrite is considered a rewrite by diff -B
  - remove superfluous comments in favor of test descriptions
  - use variable to spell 'no newline' annotation to support simpler
    maintenance whilst still allowing to check for unexpected leading
    or trailing characters.

 diff.c                  |  1 +
 t/t4022-diff-rewrite.sh | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

diff --git a/diff.c b/diff.c
index 1a594df..f333de8 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));
 	}
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_seq 1 99 >seq &&
+   printf 100 >>seq &&
+   git add seq &&
+   git commit seq -m seq
+'
+
+test_expect_success 'rewrite the middle 90% of sequence file and terminate with 
newline' '
+
+   test_seq 1 5 >seq &&
+   test_seq 9331 9420 >>seq &&
+   test_seq 96 100 >>seq
+'
+
+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 &&
+	grep -v "^.\\+'"$no_newline_anno"'" res &&
+	grep -v "'"$no_newline_anno"'.\\+$" res
+'
+
+test_expect_success 'no newline at eof is on its own line with -B' '
+
+	git diff -B seq >res &&
+	grep "^'"$no_newline_anno"'$" res &&
+	grep -v "^.\\+'"$no_newline_anno"'" res &&
+	grep -v "'"$no_newline_anno"'.\\+$" res
+'
+
 test_done
 
-- 
1.7.11.msysgit.1.1.gf0affa1

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

[Junio C Hamano]

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

 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.

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

[Adam Butcher]

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.

Signed-off-by: Adam Butcher <dev.lists@jessamine.co.uk>
---

Updates:

  - replace commit msg with revised suggestion from Junio
  - remove hardcoded 'No newline...' in tests and simplify

 diff.c                  |  1 +
 t/t4022-diff-rewrite.sh | 33 +++++++++++++++++++++++++++++++++
 2 files changed, 34 insertions(+)

diff --git a/diff.c b/diff.c
index 1a594df..f333de8 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));
 	}
diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh
index c00a94b..05ac3e9 100755
--- a/t/t4022-diff-rewrite.sh
+++ b/t/t4022-diff-rewrite.sh
@@ -66,5 +66,38 @@ 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_seq 1 99 >seq &&
+	printf 100 >>seq &&
+	git add seq &&
+	git commit seq -m seq
+'
+
+test_expect_success 'rewrite the middle 90% of sequence file and terminate with newline' '
+
+	test_seq 1 5 >seq &&
+	test_seq 9331 9420 >>seq &&
+	test_seq 96 100 >>seq
+'
+
+test_expect_success 'confirm that sequence file is considered a rewrite' '
+
+	git diff -B seq >res &&
+	grep "dissimilarity index" res
+'
+
+test_expect_success 'no newline at eof is on its own line without -B' '
+
+	git diff seq >res &&
+	grep "^\\\\ " res && ! grep "^..*\\\\ " res
+'
+
+test_expect_success 'no newline at eof is on its own line with -B' '
+
+	git diff -B seq >res &&
+	grep "^\\\\ " res && ! grep "^..*\\\\ " res
+'
+
 test_done
 
-- 
1.7.11.msysgit.1.1.gf0affa1