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

Jeff King [off-list ref] writes:

On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:

quoted

From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001
From: Adam Butcher <redacted>
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.

quoted

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.

quoted

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

quoted

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.

quoted

---

Sign-off needed.

quoted

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

quoted

 	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.

quoted

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

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

quoted

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"

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:

quoted

-		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

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

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

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

Fine explanation, but...

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

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

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

Jeff King [off-list ref] wrote:

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

quoted

Previous patch didn't support `test_seq 1 50` (I removed it accidentally).

Our emails just crossed paths. :)

Yeah :)

quoted

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

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

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