{"thread":{"id":"31173","subject":"[PATCH] Fix 'No newline...' annotation in rewrite diffs.","startedAt":"2012-08-02T21:11:02Z","lastAt":"2012-08-06T20:16:00Z","messageCount":34,"participants":["Adam Butcher","Jeff King","Junio C Hamano","Michał Kiedrowicz","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"196372","messageId":"85f291cec03411c61ddf8808e53621ae@imap.force9.net","threadId":"31173","inReplyTo":null,"subject":"[PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Adam Butcher","fromEmail":"dev.lists@jessamine.co.uk","sentAt":"2012-08-02T21:11:02Z","receivedAt":"2012-08-02T21:11:02Z","isPatch":true,"sender":{"key":"dev.lists@jessamine.co.uk","avatar":null},"body":" From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001\n From: Adam Butcher \nDate: Wed, 1 Aug 2012 22:25:09 +0100\nSubject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.\n\nWhen operating in --break-rewrites (-B) mode on a file with no newline\nterminator (and assuming --break-rewrites determines that the diff\n_is_ a rewrite), git diff previously concatenated the indicator comment\n'\\ No newline at end of file' directly to the terminating line rather\nthan on a line of its own. The resulting diff is broken; claiming\nthat the last line actually contains the indicator text. Without -B\nthere is no problem with the same files.\n\nThis patch fixes the former case by inserting a newline into the\noutput prior to emitting the indicator comment.\n\nPotential issue: Currently this emits an ASCII 10 newline character\nonly. I'm not sure whether this will be okay on all platforms; it\nseems to work fine on Windows and GNU at least.\n\nA couple of tests have been added to the rewrite suite to confirm that\nthe indicator comment is generated on its own line in both plain diff\nand rewrite mode. The latter test fails if the functional part of\nthis patch (i.e. diff.c) is reverted.\n---\n diff.c | 1 +\n t/t4022-diff-rewrite.sh | 27 +++++++++++++++++++++++++++\n 2 files changed, 28 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 95706a5..77d4e84 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct emit_callback \n*ecb,\n \tif (!endp) {\n \t\tconst char *plain = diff_get_color(ecb->color_diff,\n \t\t\t\t\t\t DIFF_PLAIN);\n+\t\tputc('\\n', ecb->opt->file);\n \t\temit_line_0(ecb->opt, plain, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t}\ndiff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh\nindex c00a94b..c85154d 100755\n--- a/t/t4022-diff-rewrite.sh\n+++ b/t/t4022-diff-rewrite.sh\n@@ -66,5 +66,32 @@ test_expect_success 'suppress deletion diff with -B \n-D' '\n \tgrep -v \"Linus Torvalds\" actual\n '\n\n+# create a file containing numbers with no newline at\n+# the end and modify it such that the starting 10 lines\n+# are unchanged, the next 101 are rewritten and the last\n+# line differs only in that in is terminated by a newline.\n+seq 1 10 > seq\n+seq 100 +1 200 >> seq\n+printf 201 >> seq\n+(git add seq; git commit seq -m seq) >/dev/null\n+seq 1 10 > seq\n+seq 300 -1 200 >> seq\n+\n+test_expect_success 'no newline at eof is on its own line without -B' \n'\n+\n+\t(git diff seq; true) > res &&\n+\tgrep \"^\\\\\\\\ No newline at end of file$\" res &&\n+\tgrep -v \"^.\\\\+\\\\\\\\ No newline at end of file\" res &&\n+\tgrep -v \"\\\\\\\\ No newline at end of file.\\\\+$\" res\n+'\n+\n+test_expect_success 'no newline at eof is on its own line with -B' '\n+\n+\t(git diff -B seq; true) > res &&\n+\tgrep \"^\\\\\\\\ No newline at end of file$\" res &&\n+\tgrep -v \"^.\\\\+\\\\\\\\ No newline at end of file\" res &&\n+\tgrep -v \"\\\\\\\\ No newline at end of file.\\\\+$\" res\n+'\n+\n test_done\n\n-- \n1.7.11.msysgit.0\n"},{"id":"196375","messageId":"20120802213346.GA575@sigill.intra.peff.net","threadId":"31173","inReplyTo":"85f291cec03411c61ddf8808e53621ae@imap.force9.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-02T21:33:46Z","receivedAt":"2012-08-02T21:33:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:\n\n> From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001\n> From: Adam Butcher \n> Date: Wed, 1 Aug 2012 22:25:09 +0100\n> Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.\n\nYou can drop these lines from the email body; they are redundant with\nwhat's in your actual header.\n\n> When operating in --break-rewrites (-B) mode on a file with no newline\n> terminator (and assuming --break-rewrites determines that the diff\n> _is_ a rewrite), git diff previously concatenated the indicator comment\n> '\\ No newline at end of file' directly to the terminating line rather\n> than on a line of its own. The resulting diff is broken; claiming\n> that the last line actually contains the indicator text. Without -B\n> there is no problem with the same files.\n> \n> This patch fixes the former case by inserting a newline into the\n> output prior to emitting the indicator comment.\n\nMakes sense.\n\n> Potential issue: Currently this emits an ASCII 10 newline character\n> only. I'm not sure whether this will be okay on all platforms; it\n> seems to work fine on Windows and GNU at least.\n\nThis should not be a problem. Git always outputs newlines; it is stdio\nwho might munge it into CRLF if need be (and your patch uses putc, so we\nshould be fine).\n\n> A couple of tests have been added to the rewrite suite to confirm that\n> the indicator comment is generated on its own line in both plain diff\n> and rewrite mode. The latter test fails if the functional part of\n> this patch (i.e. diff.c) is reverted.\n\nYay, tests.\n\n> ---\n> diff.c | 1 +\n> t/t4022-diff-rewrite.sh | 27 +++++++++++++++++++++++++++\n> 2 files changed, 28 insertions(+)\n> \n> diff --git a/diff.c b/diff.c\n> index 95706a5..77d4e84 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct\n> emit_callback *ecb,\n\nYour patch is line-wrapped and cannot be applied as-is (try turning off\n\"flowed text\" in your MUA).\n\n> \tif (!endp) {\n> \t\tconst char *plain = diff_get_color(ecb->color_diff,\n> \t\t\t\t\t\t DIFF_PLAIN);\n> +\t\tputc('\\n', ecb->opt->file);\n> \t\temit_line_0(ecb->opt, plain, reset, '\\\\',\n> \t\t\t nneof, strlen(nneof));\n> \t}\n\nLooks correct. I was curious how the regular (non-rewrite) code path did\nthis, and it just sticks the \"\\n\" as part of the nneof string. However,\nwe would not want that here, because each line should have its own\ncolor markers.\n\n> +# create a file containing numbers with no newline at\n> +# the end and modify it such that the starting 10 lines\n> +# are unchanged, the next 101 are rewritten and the last\n> +# line differs only in that in is terminated by a newline.\n> +seq 1 10 > seq\n> +seq 100 +1 200 >> seq\n> +printf 201 >> seq\n> +(git add seq; git commit seq -m seq) >/dev/null\n> +seq 1 10 > seq\n> +seq 300 -1 200 >> seq\n\nSeq is (unfortunately) not portable. I usually use a perl snippet\ninstead, like:\n\n perl -le 'print for (1..10)'\n\nThough I think we are adjusting that to use $PERL_PATH these days.\n\n-Peff\n"},{"id":"196378","messageId":"7vipd1c66f.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"20120802213346.GA575@sigill.intra.peff.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-02T21:52:56Z","receivedAt":"2012-08-02T21:52:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:\n>\n>> From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 2001\n>> From: Adam Butcher \n>> Date: Wed, 1 Aug 2012 22:25:09 +0100\n>> Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.\n>\n> You can drop these lines from the email body; they are redundant with\n> what's in your actual header.\n\ns/can/should/ actually, for readability.\n\n>> When operating in --break-rewrites (-B) mode on a file with no newline\n>> terminator (and assuming --break-rewrites determines that the diff\n>> _is_ a rewrite), git diff previously concatenated the indicator comment\n>> '\\ No newline at end of file' directly to the terminating line rather\n>> than on a line of its own. The resulting diff is broken; claiming\n>> that the last line actually contains the indicator text. Without -B\n>> there is no problem with the same files.\n>> \n>> This patch fixes the former case by inserting a newline into the\n>> output prior to emitting the indicator comment.\n>\n> Makes sense.\n>\n>> Potential issue: Currently this emits an ASCII 10 newline character\n>> only. I'm not sure whether this will be okay on all platforms; it\n>> seems to work fine on Windows and GNU at least.\n>\n> This should not be a problem. Git always outputs newlines; it is stdio\n> who might munge it into CRLF if need be (and your patch uses putc, so we\n> should be fine).\n>\n>> A couple of tests have been added to the rewrite suite to confirm that\n>> the indicator comment is generated on its own line in both plain diff\n>> and rewrite mode. The latter test fails if the functional part of\n>> this patch (i.e. diff.c) is reverted.\n>\n> Yay, tests.\n>\n>> ---\n\nSign-off needed.\n\n>> diff.c | 1 +\n>> t/t4022-diff-rewrite.sh | 27 +++++++++++++++++++++++++++\n>> 2 files changed, 28 insertions(+)\n>> \n>> diff --git a/diff.c b/diff.c\n>> index 95706a5..77d4e84 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct\n>> emit_callback *ecb,\n>\n> Your patch is line-wrapped and cannot be applied as-is (try turning off\n> \"flowed text\" in your MUA).\n>\n>> \tif (!endp) {\n>> \t\tconst char *plain = diff_get_color(ecb->color_diff,\n>> \t\t\t\t\t\t DIFF_PLAIN);\n>> +\t\tputc('\\n', ecb->opt->file);\n>> \t\temit_line_0(ecb->opt, plain, reset, '\\\\',\n>> \t\t\t nneof, strlen(nneof));\n>> \t}\n>\n> Looks correct. I was curious how the regular (non-rewrite) code path did\n> this, and it just sticks the \"\\n\" as part of the nneof string. However,\n> we would not want that here, because each line should have its own\n> color markers.\n>\n>> +# create a file containing numbers with no newline at\n>> +# the end and modify it such that the starting 10 lines\n>> +# are unchanged, the next 101 are rewritten and the last\n>> +# line differs only in that in is terminated by a newline.\n>> +seq 1 10 > seq\n>> +seq 100 +1 200 >> seq\n>> +printf 201 >> seq\n>> +(git add seq; git commit seq -m seq) >/dev/null\n>> +seq 1 10 > seq\n>> +seq 300 -1 200 >> seq\n>\n> Seq is (unfortunately) not portable. I usually use a perl snippet\n> instead, like:\n>\n> perl -le 'print for (1..10)'\n>\n> Though I think we are adjusting that to use $PERL_PATH these days.\n\nt/perf/perf-lib.sh and t/t5551-http-fetch.sh seem to use \"seq\";\nperhaps we should replace them, then.\n"},{"id":"196380","messageId":"7vehnpc5ti.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"85f291cec03411c61ddf8808e53621ae@imap.force9.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-02T22:00:41Z","receivedAt":"2012-08-02T22:00:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Butcher writes:\n\n> +# create a file containing numbers with no newline at\n> +# the end and modify it such that the starting 10 lines\n> +# are unchanged, the next 101 are rewritten and the last\n> +# line differs only in that in is terminated by a newline.\n> +seq 1 10 > seq\n> +seq 100 +1 200 >> seq\n> +printf 201 >> seq\n> +(git add seq; git commit seq -m seq) >/dev/null\n> +seq 1 10 > seq\n> +seq 300 -1 200 >> seq\n\nWe would prefer to have these set-up steps in test_expect_success.\nThat way, we will have more chance to catch potential and unintended\nbreakage to \"git add\" and \"git commit\" when people attempt to update\nthem.\n\nAlso, the redirect target sticks to redirect operator in our\nscripts, i.e. \"cmd >seq\" not \"cmd > seq\".\n\n> +test_expect_success 'no newline at eof is on its own line without -B'\n> +\n> +\t(git diff seq; true) > res &&\n\nWhat is this subshell and true about? A git diff does not exit with\nnon zero to signal differences, and even if it did, the right way to\nwrite it would be\n\n\ttest_might_fail git cmd >res &&\n\nto allow us to make sure that the git command that may or may not\nexit with zero still does not die an uncontrolled death (e.g. segv).\n\n> +\tgrep \"^\\\\\\\\ No newline at end of file$\" res &&\n> +\tgrep -v \"^.\\\\+\\\\\\\\ No newline at end of file\" res &&\n> +\tgrep -v \"\\\\\\\\ No newline at end of file.\\\\+$\" res\n> +'\n\nIt is preferrable not to spell \"No newline at ...\" part out, so that\nwe won't have to worry about future rewords and i18n. There are older\ntests that predate i18n and they do spell these out, but that is not\na good reason to make things worse than they already are.\n\n\"git apply\" only looks at the backslash-space at the beginning of\nline anyway.\n\n> +test_expect_success 'no newline at eof is on its own line with -B' '\n> +\n> +\t(git diff -B seq; true) > res &&\n> +\tgrep \"^\\\\\\\\ No newline at end of file$\" res &&\n> +\tgrep -v \"^.\\\\+\\\\\\\\ No newline at end of file\" res &&\n> +\tgrep -v \"\\\\\\\\ No newline at end of file.\\\\+$\" res\n> +'\n\nLikewise.\n\n> test_done\n\nThanks.\n"},{"id":"196383","messageId":"20120802221404.GA1682@sigill.intra.peff.net","threadId":"31173","inReplyTo":"7vipd1c66f.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-02T22:14:04Z","receivedAt":"2012-08-02T22:14:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 02, 2012 at 02:52:56PM -0700, Junio C Hamano wrote:\n\n> > Seq is (unfortunately) not portable. I usually use a perl snippet\n> > instead, like:\n> >\n> > perl -le 'print for (1..10)'\n> >\n> > Though I think we are adjusting that to use $PERL_PATH these days.\n> \n> t/perf/perf-lib.sh and t/t5551-http-fetch.sh seem to use \"seq\";\n> perhaps we should replace them, then.\n\nTraditionally, BSD did not have seq (they have \"jot\" instead). However,\nmy OS X 10.7 box does have seq, and its manpage claims that it appeared\nin FreeBSD 9.0. But we should be able to run the test suite on older\nversions of both (9.0 is barely 6 months old).\n\nI suspect people on those platforms did not notice because t5551 does\nnot run by default (not only due to the apache requirement, but you have\nto set GIT_TEST_LONG to trigger the particular test that uses it), and\npeople don't typically run the perf code regularly to look for\nregressions.\n\n-- >8 --\nSubject: [PATCH] stop using 'seq' in test scripts\n\nThe seq command is GNU-ism, and is missing at least in older\nBSD releases and their derivatives, not to mention antique\ncommercial Unixes.\n\nWe already purged it in b3431bc (Don't use seq in tests, not\neveryone has it, 2007-05-02), but a few new instances have\ncrept in. They went unnoticed because they are in scripts\nthat are not run by default.\n\nLet's replace them with a perl snippet (which we already\nassume to be everywhere elsewhere in the test suite).\n---\nb3431bc used a while loop with increment to replace it, which we could\nalso do. I think the perl script is a little easier to read. If we\never want to drop the perl dependency for the test suite, we could write\na 5-liner test-seq.c replacement.\n\n t/perf/perf-lib.sh | 2 +-\n t/t5551-http-fetch.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 5580c22..8bf8d69 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -163,7 +163,7 @@ test_perf () {\n \t\telse\n \t\t\techo \"perf $test_count - $1:\"\n \t\tfi\n-\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n+\t\tfor i in $(\"$PERL_PATH\" -le \"print for 1..$GIT_PERF_REPEAT_COUNT\"); do\n \t\t\tsay >&3 \"running: $2\"\n \t\t\tif test_run_perf_ \"$2\"\n \t\t\tthen\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex fadf2f2..e858a31 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -114,7 +114,7 @@ test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in `seq 50000`\n+\tfor i in `\"$PERL_PATH\" -le \"print for (1..50000)\"`\n \tdo\n \t\techo \"commit refs/heads/too-many-refs\"\n \t\techo \"mark :$i\"\n"},{"id":"196384","messageId":"2398996b47dd4f3b12ea73d79297ca98@imap.force9.net","threadId":"31173","inReplyTo":"20120802213346.GA575@sigill.intra.peff.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Adam Butcher","fromEmail":"dev.lists@jessamine.co.uk","sentAt":"2012-08-02T22:22:50Z","receivedAt":"2012-08-02T22:22:50Z","isPatch":true,"sender":{"key":"dev.lists@jessamine.co.uk","avatar":null},"body":"On 02.08.2012 22:33, Jeff King wrote:\n> On Thu, Aug 02, 2012 at 10:11:02PM +0100, Adam Butcher wrote:\n>\n>> From 01730a741cc5fd7d0a5d8bd0d3df80d12c81fe48 Mon Sep 17 00:00:00 \n>> 2001\n>> From: Adam Butcher \n>> Date: Wed, 1 Aug 2012 22:25:09 +0100\n>> Subject: [PATCH] Fix 'No newline...' annotation in rewrite diffs.\n>\n> You can drop these lines from the email body; they are redundant with\n> what's in your actual header.\n>\nI sent via a webmail interface and wasn't sure what format the \nresulting mail would have so decided to paste the entire formatted patch \nin. Seeing as the webmailer has corrupted my patch with word wrapping \n(which I noticed almost immediately when my post hit gmane and have \nsince found out that it apparently cannot be disabled?!) it was a bad \nidea all round. I could attach as a file but this is cumbersome from a \nreview and apply point of view so I think I'll hook up git to gmail's \ntls smtp server so that I can use git send-email direct rather than \nmessing about with a GUI.\n\n>> When operating in --break-rewrites (-B) mode on a file with no \n>> newline\n>> terminator (and assuming --break-rewrites determines that the diff\n>> _is_ a rewrite), git diff previously concatenated the indicator \n>> comment\n>> '\\ No newline at end of file' directly to the terminating line \n>> rather\n>> than on a line of its own. The resulting diff is broken; claiming\n>> that the last line actually contains the indicator text. Without -B\n>> there is no problem with the same files.\n>>\n>> This patch fixes the former case by inserting a newline into the\n>> output prior to emitting the indicator comment.\n>\n> Makes sense.\n>\n>> Potential issue: Currently this emits an ASCII 10 newline character\n>> only. I'm not sure whether this will be okay on all platforms; it\n>> seems to work fine on Windows and GNU at least.\n>\n> This should not be a problem. Git always outputs newlines; it is \n> stdio\n> who might munge it into CRLF if need be (and your patch uses putc, so \n> we\n> should be fine).\n>\nGreat.\n\n>> A couple of tests have been added to the rewrite suite to confirm \n>> that\n>> the indicator comment is generated on its own line in both plain \n>> diff\n>> and rewrite mode. The latter test fails if the functional part of\n>> this patch (i.e. diff.c) is reverted.\n>\n> Yay, tests.\n>\n>> ---\n>> diff.c | 1 +\n>> t/t4022-diff-rewrite.sh | 27 +++++++++++++++++++++++++++\n>> 2 files changed, 28 insertions(+)\n>>\n>> diff --git a/diff.c b/diff.c\n>> index 95706a5..77d4e84 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct\n>> emit_callback *ecb,\n>\n> Your patch is line-wrapped and cannot be applied as-is (try turning \n> off\n> \"flowed text\" in your MUA).\n>\nIndeed. Grr. If only I could. I'll test that whatever solution I \ncome up with works before posting again with an update addressing yours \nand Junio's comments.\n\n>> \tif (!endp) {\n>> \t\tconst char *plain = diff_get_color(ecb->color_diff,\n>> \t\t\t\t\t\t DIFF_PLAIN);\n>> +\t\tputc('\\n', ecb->opt->file);\n>> \t\temit_line_0(ecb->opt, plain, reset, '\\\\',\n>> \t\t\t nneof, strlen(nneof));\n>> \t}\n>\n> Looks correct. I was curious how the regular (non-rewrite) code path \n> did\n> this, and it just sticks the \"\\n\" as part of the nneof string. \n> However,\n> we would not want that here, because each line should have its own\n> color markers.\n>\n>> +# create a file containing numbers with no newline at\n>> +# the end and modify it such that the starting 10 lines\n>> +# are unchanged, the next 101 are rewritten and the last\n>> +# line differs only in that in is terminated by a newline.\n>> +seq 1 10 > seq\n>> +seq 100 +1 200 >> seq\n>> +printf 201 >> seq\n>> +(git add seq; git commit seq -m seq) >/dev/null\n>> +seq 1 10 > seq\n>> +seq 300 -1 200 >> seq\n>\n> Seq is (unfortunately) not portable. I usually use a perl snippet\n> instead, like:\n>\n> perl -le 'print for (1..10)'\n>\n> Though I think we are adjusting that to use $PERL_PATH these days.\n>\nNo probs. Will change.\n\nCheers\nAdam\n"},{"id":"196391","messageId":"551f7f77570c84017ae93988f9202854@imap.force9.net","threadId":"31173","inReplyTo":"7vehnpc5ti.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Adam Butcher","fromEmail":"dev.lists@jessamine.co.uk","sentAt":"2012-08-02T22:58:56Z","receivedAt":"2012-08-02T22:58:56Z","isPatch":true,"sender":{"key":"dev.lists@jessamine.co.uk","avatar":null},"body":"On 02.08.2012 23:00, Junio C Hamano wrote:\n> Adam Butcher writes:\n>\n>> +# create a file containing numbers with no newline at\n>> +# the end and modify it such that the starting 10 lines\n>> +# are unchanged, the next 101 are rewritten and the last\n>> +# line differs only in that in is terminated by a newline.\n>> +seq 1 10 > seq\n>> +seq 100 +1 200 >> seq\n>> +printf 201 >> seq\n>> +(git add seq; git commit seq -m seq) >/dev/null\n>> +seq 1 10 > seq\n>> +seq 300 -1 200 >> seq\n>\n> We would prefer to have these set-up steps in test_expect_success.\n> That way, we will have more chance to catch potential and unintended\n> breakage to \"git add\" and \"git commit\" when people attempt to update\n> them.\n>\nCool, no probs. I had originally put them at the start of the first \ntest that I added but decided to pull them out as prep. I think that \nmsysGit or something about my Windows shell session may have played a \npart in my not chaining them with && also (see below). I'll clean them \nup and wrap them in a test_expect_success.\n\n> Also, the redirect target sticks to redirect operator in our\n> scripts, i.e. \"cmd >seq\" not \"cmd > seq\".\n>\nOkay, will change.\n\n>> +test_expect_success 'no newline at eof is on its own line without \n>> -B'\n>> +\n>> +\t(git diff seq; true) > res &&\n>\n> What is this subshell and true about? A git diff does not exit with\n> non zero to signal differences,\n>\nHmm, (?confused?) yeah actually I didn't think it did -- I was \nsurprised when git returned 1 for this line. I think it must have been \nan issue with the version msysGit I was using or something sticking \nerrorlevel in my Windows shell. Git seemed to return 1 ALWAYS! I \nusually use gnu/linux but on this occasion I wrote the fix and tests \nblind on a Windows machine testing the logic manually with msysGit. I \nran the tests on a linux machine at work and they did what I expected so \nI left them as was without rechecking this.\n\nI'm glad that this can be simplified. It felt wrong -- similar lines \nelsewhere in the script didn't do it so I wasn't really happy with it. \nTurns out it looks to be a Windows/environment issue. I cannot \nreproduce it now.\n\n> and even if it did, the right way to\n> write it would be\n>\n> \ttest_might_fail git cmd >res &&\n>\nFair enough. Good to know that's available.\n\n> to allow us to make sure that the git command that may or may not\n> exit with zero still does not die an uncontrolled death (e.g. segv).\n>\n>> +\tgrep \"^\\\\\\\\ No newline at end of file$\" res &&\n>> +\tgrep -v \"^.\\\\+\\\\\\\\ No newline at end of file\" res &&\n>> +\tgrep -v \"\\\\\\\\ No newline at end of file.\\\\+$\" res\n>> +'\n>\n> It is preferrable not to spell \"No newline at ...\" part out, so that\n> we won't have to worry about future rewords and i18n.\n>\nOkay no probs. I was originally going to spell it out only once and \nuse parameter expansion. However I understand the point of not spelling \nit out at all. The only reason I did so was to catch other potential \nerrors where text may have been 'attached' to either side of the \nannotation string by some future (or other) bug.\n\n> There are older\n> tests that predate i18n and they do spell these out, but that is not\n> a good reason to make things worse than they already are.\n>\nAgreed. Should I just test for this prefix case (i.e. the bug at hand) \nonly and not preempt future potential issues? Or should I just stick \nthe current string in a variable and keep the logic as is; at least then \nthere would only be one place requiring a fix in the reword case (but \nstill additional rework in the i18n case -- though I assume the test \ncould force a particular locale to evade this).\n\n> \"git apply\" only looks at the backslash-space at the beginning of\n> line anyway.\n>\nOkay.\n\n>> +test_expect_success 'no newline at eof is on its own line with -B' \n>> '\n>> +\n>> +\t(git diff -B seq; true) > res &&\n>> +\tgrep \"^\\\\\\\\ No newline at end of file$\" res &&\n>> +\tgrep -v \"^.\\\\+\\\\\\\\ No newline at end of file\" res &&\n>> +\tgrep -v \"\\\\\\\\ No newline at end of file.\\\\+$\" res\n>> +'\n>\n> Likewise.\n>\n>> test_done\n>\n> Thanks.\n\nNo probs. I will address both your and Jeff's comments sometime \ntomorrow and hopefully send a well formatted patch in next time.\n\nCheers,\nAdam\n"},{"id":"196396","messageId":"loom.20120803T094115-721@post.gmane.org","threadId":"31173","inReplyTo":"20120802221404.GA1682@sigill.intra.peff.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T07:49:47Z","receivedAt":"2012-08-03T07:49:47Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King peff.net> writes:\n\n> -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n> +\t\tfor i in $(\"$PERL_PATH\" -le \"print for 1..$GIT_PERF_REPEAT_COUNT\"); do\n\n\nMaybe you could introduce \"test_seq\" instead.\n"},{"id":"196417","messageId":"20120803160229.GA13094@sigill.intra.peff.net","threadId":"31173","inReplyTo":"loom.20120803T094115-721@post.gmane.org","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T16:02:30Z","receivedAt":"2012-08-03T16:02:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 07:49:47AM +0000, Michał Kiedrowicz wrote:\n\n> Jeff King peff.net> writes:\n> \n> > -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n> > +\t\tfor i in $(\"$PERL_PATH\" -le \"print for 1..$GIT_PERF_REPEAT_COUNT\"); do\n> \n> Maybe you could introduce \"test_seq\" instead.\n\nI don't have a strong preference, as there are only two callsites. Do\nyou want to make a patch?\n\n-Peff\n"},{"id":"196422","messageId":"7vobmrc49t.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"20120803160229.GA13094@sigill.intra.peff.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-03T16:46:22Z","receivedAt":"2012-08-03T16:46:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 03, 2012 at 07:49:47AM +0000, Michał Kiedrowicz wrote:\n>\n>> Jeff King peff.net> writes:\n>> \n>> > -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n>> > +\t\tfor i in $(\"$PERL_PATH\" -le \"print for 1..$GIT_PERF_REPEAT_COUNT\"); do\n>> \n>> Maybe you could introduce \"test_seq\" instead.\n>\n> I don't have a strong preference, as there are only two callsites. Do\n> you want to make a patch?\n\nIf you run \"for . in . . .\" in t/, we see quite a many hits, so\n\"only two callsites\" might be undercounting the candidates.\n"},{"id":"196423","messageId":"20120803170005.GA24068@sigill.intra.peff.net","threadId":"31173","inReplyTo":"7vobmrc49t.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T17:00:06Z","receivedAt":"2012-08-03T17:00:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 09:46:22AM -0700, Junio C Hamano wrote:\n\n> >> Maybe you could introduce \"test_seq\" instead.\n> >\n> > I don't have a strong preference, as there are only two callsites. Do\n> > you want to make a patch?\n> \n> If you run \"for . in . . .\" in t/, we see quite a many hits, so\n> \"only two callsites\" might be undercounting the candidates.\n\nTrue. Although a good number of them are not numeric sequences (however\nperl being perl, I think my one-liner would take \"a\" and \"g\" as\nend-points just as readily).\n\nI have no problem with converting them all. I just didn't want to\npersonally go to the work myself.\n\n-Peff\n"},{"id":"196432","messageId":"1344023835-8947-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"31173","inReplyTo":"20120803160229.GA13094@sigill.intra.peff.net","subject":"[PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T19:57:15Z","receivedAt":"2012-08-03T19:57:15Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n\tThe seq command is GNU-ism, and is missing at least in older BSD\n\treleases and their derivatives, not to mention antique\n\tcommercial Unixes.\n\n\tWe already purged it in b3431bc (Don't use seq in tests, not\n\teveryone has it, 2007-05-02), but a few new instances have crept\n\tin. They went unnoticed because they are in scripts that are not\n\trun by default.\n\nThis commit replaces them with test_seq that is implemented with a Perl\nsnippet (proposed by Jeff). This is better than inlining this snippet\neverywhere it's needed because it's easier to read and it's easier to\nchange the implementation (e.g. to C) if we ever decide to remove Perl\nfrom the test suite.\n\nNote that test_seq is not a complete replacement for seq(1). It just\nhas what we need now.\n\nThere are also many places that do `for i in 1 2 3 ...` but I'm not sure\nif it's worth converting them to test_seq. That would introduce running\nmore processes of Perl during the tests and might increase the total\ntime tests take.\n\nSigned-off-by: Michał Kiedrowicz \n---\n\n> I don't have a strong preference, as there are only two callsites. Do\n> you want to make a patch?\n\nSure. Here it is.\n\n t/perf/perf-lib.sh | 2 +-\n t/t5551-http-fetch.sh | 2 +-\n t/test-lib-functions.sh | 14 ++++++++++++++\n 3 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 5580c22..a1361e5 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -163,7 +163,7 @@ test_perf () {\n \t\telse\n \t\t\techo \"perf $test_count - $1:\"\n \t\tfi\n-\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n+\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n \t\t\tsay >&3 \"running: $2\"\n \t\t\tif test_run_perf_ \"$2\"\n \t\t\tthen\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex fadf2f2..91eaf53 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -114,7 +114,7 @@ test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in `seq 50000`\n+\tfor i in `test_seq 50000`\n \tdo\n \t\techo \"commit refs/heads/too-many-refs\"\n \t\techo \"mark :$i\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80daaca..7d7424d 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -530,6 +530,20 @@ test_cmp() {\n \t$GIT_TEST_CMP \"$@\"\n }\n \n+# test_seq is a portable replacement for seq(1).\n+# It may be used like:\n+#\n+#\tfor i in `test_seq 100`; do\n+#\t\techo $i\n+#\tdone\n+\n+test_seq () {\n+\ttest $# = 1 ||\n+\terror \"bug in the test script: not 1 parameter to test_seq\"\n+\tlast=$1\n+\t\"$PERL_PATH\" -le \"print for 1..$last\"\n+}\n+\n # This function can be used to schedule some commands to be run\n # unconditionally at the end of the test to restore sanity:\n #\n-- \n1.7.11.rc0.212.g37218b0.dirty\n"},{"id":"196434","messageId":"20120803200201.GA10344@sigill.intra.peff.net","threadId":"31173","inReplyTo":"1344023835-8947-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T20:02:01Z","receivedAt":"2012-08-03T20:02:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 09:57:15PM +0200, Michał Kiedrowicz wrote:\n\n> Jeff King wrote:\n> \n> \tThe seq command is GNU-ism, and is missing at least in older BSD\n> \treleases and their derivatives, not to mention antique\n> \tcommercial Unixes.\n> \n> \tWe already purged it in b3431bc (Don't use seq in tests, not\n> \teveryone has it, 2007-05-02), but a few new instances have crept\n> \tin. They went unnoticed because they are in scripts that are not\n> \trun by default.\n> \n> This commit replaces them with test_seq that is implemented with a Perl\n> snippet (proposed by Jeff). This is better than inlining this snippet\n> everywhere it's needed because it's easier to read and it's easier to\n> change the implementation (e.g. to C) if we ever decide to remove Perl\n> from the test suite.\n> \n> Note that test_seq is not a complete replacement for seq(1). It just\n> has what we need now.\n> \n> There are also many places that do `for i in 1 2 3 ...` but I'm not sure\n> if it's worth converting them to test_seq. That would introduce running\n> more processes of Perl during the tests and might increase the total\n> time tests take.\n> \n> Signed-off-by: Michał Kiedrowicz \n\nFine explanation, but...\n\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index 5580c22..a1361e5 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -163,7 +163,7 @@ test_perf () {\n> \t\telse\n> \t\t\techo \"perf $test_count - $1:\"\n> \t\tfi\n> -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n> +\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n\nTwo args to test_seq, but...\n\n> +# test_seq is a portable replacement for seq(1).\n> +# It may be used like:\n> +#\n> +#\tfor i in `test_seq 100`; do\n> +#\t\techo $i\n> +#\tdone\n> +\n> +test_seq () {\n> +\ttest $# = 1 ||\n> +\terror \"bug in the test script: not 1 parameter to test_seq\"\n> +\tlast=$1\n> +\t\"$PERL_PATH\" -le \"print for 1..$last\"\n> +}\n\nit wants only one.\n\nI think you would want:\n\n test $# = 1 && set -- 1 \"$@\"\n \"$PERL_PATH\" -le \"print for $1..$2\"\n\nIt might also be worth quoting the parameters like this:\n\n \"$PERL_PATH\" -le \"print for '$1'..'$2'\"\n\nso that \"test_seq a f\" works, too.\n\n-Peff\n"},{"id":"196435","messageId":"1344024290-9197-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"31173","inReplyTo":"20120803160229.GA13094@sigill.intra.peff.net","subject":"[PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T20:04:50Z","receivedAt":"2012-08-03T20:04:50Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n\tThe seq command is GNU-ism, and is missing at least in older BSD\n\treleases and their derivatives, not to mention antique\n\tcommercial Unixes.\n\n\tWe already purged it in b3431bc (Don't use seq in tests, not\n\teveryone has it, 2007-05-02), but a few new instances have crept\n\tin. They went unnoticed because they are in scripts that are not\n\trun by default.\n\nThis commit replaces them with test_seq that is implemented with a Perl\nsnippet (proposed by Jeff). This is better than inlining this snippet\neverywhere it's needed because it's easier to read and it's easier to\nchange the implementation (e.g. to C) if we ever decide to remove Perl\nfrom the test suite.\n\nNote that test_seq is not a complete replacement for seq(1). It just\nhas what we need now.\n\nThere are also many places that do `for i in 1 2 3 ...` but I'm not sure\nif it's worth converting them to test_seq. That would introduce running\nmore processes of Perl.\n\nSigned-off-by: Michał Kiedrowicz \n---\nPrevious patch didn't support `test_seq 1 50` (I removed it accidentally).\n\n t/perf/perf-lib.sh | 2 +-\n t/t5551-http-fetch.sh | 2 +-\n t/test-lib-functions.sh | 15 +++++++++++++++\n 3 files changed, 17 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 5580c22..a1361e5 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -163,7 +163,7 @@ test_perf () {\n \t\telse\n \t\t\techo \"perf $test_count - $1:\"\n \t\tfi\n-\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n+\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n \t\t\tsay >&3 \"running: $2\"\n \t\t\tif test_run_perf_ \"$2\"\n \t\t\tthen\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex fadf2f2..91eaf53 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -114,7 +114,7 @@ test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in `seq 50000`\n+\tfor i in `test_seq 50000`\n \tdo\n \t\techo \"commit refs/heads/too-many-refs\"\n \t\techo \"mark :$i\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80daaca..9456e65 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -530,6 +530,21 @@ test_cmp() {\n \t$GIT_TEST_CMP \"$@\"\n }\n \n+# test_seq is a portable replacement for seq(1).\n+# It may be used like:\n+#\n+#\tfor i in `test_seq 100`; do\n+#\t\techo $i\n+#\tdone\n+\n+test_seq () {\n+\ttest $# = 2 && { first=$1; shift; } || first=1\n+\ttest $# = 1 ||\n+\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n+\tlast=$1\n+\t\"$PERL_PATH\" -le \"print for $first..$last\"\n+}\n+\n # This function can be used to schedule some commands to be run\n # unconditionally at the end of the test to restore sanity:\n #\n-- \n1.7.11.rc0.212.g37218b0.dirty\n"},{"id":"196436","messageId":"20120803200718.GA10648@sigill.intra.peff.net","threadId":"31173","inReplyTo":"1344024290-9197-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T20:07:18Z","receivedAt":"2012-08-03T20:07:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 10:04:50PM +0200, Michał Kiedrowicz wrote:\n\n> Previous patch didn't support `test_seq 1 50` (I removed it accidentally).\n\nOur emails just crossed paths. :)\n\n> +# test_seq is a portable replacement for seq(1).\n> +# It may be used like:\n> +#\n> +#\tfor i in `test_seq 100`; do\n> +#\t\techo $i\n> +#\tdone\n\nThis should probably note that it is a subset of seq's behavior. You\ntalked about it in the commit message, but the in-code comment is a much\nmore likely thing for a potential user to read.\n\n-Peff\n"},{"id":"196437","messageId":"20120803221203.37e854d1@gmail.com","threadId":"31173","inReplyTo":"20120803200718.GA10648@sigill.intra.peff.net","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T20:12:03Z","receivedAt":"2012-08-03T20:12:03Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n> On Fri, Aug 03, 2012 at 10:04:50PM +0200, Michał Kiedrowicz wrote:\n> \n> > Previous patch didn't support `test_seq 1 50` (I removed it accidentally).\n> \n> Our emails just crossed paths. :)\n\nYeah :)\n\n> \n> > +# test_seq is a portable replacement for seq(1).\n> > +# It may be used like:\n> > +#\n> > +#\tfor i in `test_seq 100`; do\n> > +#\t\techo $i\n> > +#\tdone\n> \n> This should probably note that it is a subset of seq's behavior. You\n> talked about it in the commit message, but the in-code comment is a much\n> more likely thing for a potential user to read.\n> \n> -Peff\n\nOK, I'll quote parameters and add a note.\n"},{"id":"196439","messageId":"1344026304-11687-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"31173","inReplyTo":"20120803221203.37e854d1@gmail.com","subject":"[PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T20:38:24Z","receivedAt":"2012-08-03T20:38:24Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n\tThe seq command is GNU-ism, and is missing at least in older BSD\n\treleases and their derivatives, not to mention antique\n\tcommercial Unixes.\n\n\tWe already purged it in b3431bc (Don't use seq in tests, not\n\teveryone has it, 2007-05-02), but a few new instances have crept\n\tin. They went unnoticed because they are in scripts that are not\n\trun by default.\n\nThis commit replaces them with test_seq that is implemented with a Perl\nsnippet (proposed by Jeff). This is better than inlining this snippet\neverywhere it's needed because it's easier to read and it's easier to\nchange the implementation (e.g. to C) if we ever decide to remove Perl\nfrom the test suite.\n\nNote that test_seq is not a complete replacement for seq(1). It just\nhas what we need now.\n\nThere are also many places that do `for i in 1 2 3 ...` but I'm not sure\nif it's worth converting them to test_seq. That would introduce running\nmore processes of Perl.\n\nSigned-off-by: Michał Kiedrowicz \n---\nChanges since previous patch:\n\n\t* Added quotes around arguments, allowing `test_seq a z`\n\t* Improved test_seq comments\n\n t/perf/perf-lib.sh | 2 +-\n t/t5551-http-fetch.sh | 2 +-\n t/test-lib-functions.sh | 19 +++++++++++++++++++\n 3 files changed, 21 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 5580c22..a1361e5 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -163,7 +163,7 @@ test_perf () {\n \t\telse\n \t\t\techo \"perf $test_count - $1:\"\n \t\tfi\n-\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n+\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n \t\t\tsay >&3 \"running: $2\"\n \t\t\tif test_run_perf_ \"$2\"\n \t\t\tthen\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex fadf2f2..91eaf53 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -114,7 +114,7 @@ test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in `seq 50000`\n+\tfor i in `test_seq 50000`\n \tdo\n \t\techo \"commit refs/heads/too-many-refs\"\n \t\techo \"mark :$i\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80daaca..bed1f57 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -530,6 +530,25 @@ test_cmp() {\n \t$GIT_TEST_CMP \"$@\"\n }\n \n+# test_seq is a portable yet not complete replacement for seq(1).\n+# It may be used like:\n+#\n+#\tfor i in `test_seq 100`; do\n+#\t\tfor j in `test_seq 10 20`; do\n+#\t\t\tfor k in `test_seq a z`; do\n+#\t\t\t\techo $i-$j-$k\n+#\t\t\tdone\n+#\t\tdone\n+#\tdone\n+\n+test_seq () {\n+\ttest $# = 2 && { first=$1; shift; } || first=1\n+\ttest $# = 1 ||\n+\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n+\tlast=$1\n+\t\"$PERL_PATH\" -le \"print for '$first'..'$last'\"\n+}\n+\n # This function can be used to schedule some commands to be run\n # unconditionally at the end of the test to restore sanity:\n #\n-- \n1.7.11.rc0.212.g37218b0.dirty\n"},{"id":"196441","messageId":"20120803204102.GA10908@sigill.intra.peff.net","threadId":"31173","inReplyTo":"1344026304-11687-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T20:41:02Z","receivedAt":"2012-08-03T20:41:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 10:38:24PM +0200, Michał Kiedrowicz wrote:\n\n> Changes since previous patch:\n> \n> \t* Added quotes around arguments, allowing `test_seq a z`\n> \t* Improved test_seq comments\n> \n> t/perf/perf-lib.sh | 2 +-\n> t/t5551-http-fetch.sh | 2 +-\n> t/test-lib-functions.sh | 19 +++++++++++++++++++\n> 3 files changed, 21 insertions(+), 2 deletions(-)\n\nI think this version looks OK.\n\n-Peff\n"},{"id":"196442","messageId":"7v3943bsuc.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"20120803200201.GA10344@sigill.intra.peff.net","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-03T20:53:15Z","receivedAt":"2012-08-03T20:53:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Aug 03, 2012 at 09:57:15PM +0200, Michał Kiedrowicz wrote:\n>\n>> Jeff King wrote:\n>> \n>> \tThe seq command is GNU-ism, and is missing at least in older BSD\n>> \treleases and their derivatives, not to mention antique\n>> \tcommercial Unixes.\n>> \n>> \tWe already purged it in b3431bc (Don't use seq in tests, not\n>> \teveryone has it, 2007-05-02), but a few new instances have crept\n>> \tin. They went unnoticed because they are in scripts that are not\n>> \trun by default.\n>> \n>> This commit replaces them with test_seq that is implemented with a Perl\n>> snippet (proposed by Jeff).\n\nJust say \"Replace them with test_seq...\", without \"This commit\".\n\n> Fine explanation, but...\n>\n>> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n>> index 5580c22..a1361e5 100644\n>> --- a/t/perf/perf-lib.sh\n>> +++ b/t/perf/perf-lib.sh\n>> @@ -163,7 +163,7 @@ test_perf () {\n>> \t\telse\n>> \t\t\techo \"perf $test_count - $1:\"\n>> \t\tfi\n>> -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n>> +\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n>\n> Two args to test_seq, but...\n>\n>> +# test_seq is a portable replacement for seq(1).\n>> +# It may be used like:\n>> +#\n>> +#\tfor i in `test_seq 100`; do\n>> +#\t\techo $i\n>> +#\tdone\n>> +\n>> +test_seq () {\n>> +\ttest $# = 1 ||\n>> +\terror \"bug in the test script: not 1 parameter to test_seq\"\n>> +\tlast=$1\n>> +\t\"$PERL_PATH\" -le \"print for 1..$last\"\n>> +}\n>\n> it wants only one.\n>\n> I think you would want:\n>\n> test $# = 1 && set -- 1 \"$@\"\n> \"$PERL_PATH\" -le \"print for $1..$2\"\n>\n> It might also be worth quoting the parameters like this:\n>\n> \"$PERL_PATH\" -le \"print for '$1'..'$2'\"\n>\n> so that \"test_seq a f\" works, too.\n\nYeah, I like that last one, but then unlike the claim in the comment\nbefore the function definition, it is not \"a portable replacement\nfor seq(1)\" at all, but something a lot more suited for our purpose.\nSo at least the comment needs to be updated. I do not have strong\nopinion on calling this test_seq when it acts differently from seq;\nit is not confusing enough to make me push something longer that is\ndifferent from \"seq\", e.g. test_sequence.\n\nWouldn't it be cleaner and readable to write it like this\n\n\t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$1\" \"$2\"\n\nby the way?\n"},{"id":"196444","messageId":"20120803220237.GA14003@sigill.intra.peff.net","threadId":"31173","inReplyTo":"7v3943bsuc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T22:02:37Z","receivedAt":"2012-08-03T22:02:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 01:53:15PM -0700, Junio C Hamano wrote:\n\n> Wouldn't it be cleaner and readable to write it like this\n> \n> \t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$1\" \"$2\"\n> \n> by the way?\n\nYeah, that would be more robust (it's longer to type, which is why I\navoided it in the inline replacement, but since we're factoring it out,\nthat's not an issue).\n\n-Peff\n"},{"id":"196445","messageId":"20120804000904.13c4162b@gmail.com","threadId":"31173","inReplyTo":"7v3943bsuc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T22:09:04Z","receivedAt":"2012-08-03T22:09:04Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > On Fri, Aug 03, 2012 at 09:57:15PM +0200, Michał Kiedrowicz wrote:\n> >\n> >> Jeff King wrote:\n> >> \n> >> \tThe seq command is GNU-ism, and is missing at least in older BSD\n> >> \treleases and their derivatives, not to mention antique\n> >> \tcommercial Unixes.\n> >> \n> >> \tWe already purged it in b3431bc (Don't use seq in tests, not\n> >> \teveryone has it, 2007-05-02), but a few new instances have crept\n> >> \tin. They went unnoticed because they are in scripts that are not\n> >> \trun by default.\n> >> \n> >> This commit replaces them with test_seq that is implemented with a Perl\n> >> snippet (proposed by Jeff).\n> \n> Just say \"Replace them with test_seq...\", without \"This commit\".\n> \n> > Fine explanation, but...\n> >\n> >> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> >> index 5580c22..a1361e5 100644\n> >> --- a/t/perf/perf-lib.sh\n> >> +++ b/t/perf/perf-lib.sh\n> >> @@ -163,7 +163,7 @@ test_perf () {\n> >> \t\telse\n> >> \t\t\techo \"perf $test_count - $1:\"\n> >> \t\tfi\n> >> -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n> >> +\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n> >\n> > Two args to test_seq, but...\n> >\n> >> +# test_seq is a portable replacement for seq(1).\n> >> +# It may be used like:\n> >> +#\n> >> +#\tfor i in `test_seq 100`; do\n> >> +#\t\techo $i\n> >> +#\tdone\n> >> +\n> >> +test_seq () {\n> >> +\ttest $# = 1 ||\n> >> +\terror \"bug in the test script: not 1 parameter to test_seq\"\n> >> +\tlast=$1\n> >> +\t\"$PERL_PATH\" -le \"print for 1..$last\"\n> >> +}\n> >\n> > it wants only one.\n> >\n> > I think you would want:\n> >\n> > test $# = 1 && set -- 1 \"$@\"\n> > \"$PERL_PATH\" -le \"print for $1..$2\"\n> >\n> > It might also be worth quoting the parameters like this:\n> >\n> > \"$PERL_PATH\" -le \"print for '$1'..'$2'\"\n> >\n> > so that \"test_seq a f\" works, too.\n> \n> Yeah, I like that last one, but then unlike the claim in the comment\n> before the function definition, it is not \"a portable replacement\n> for seq(1)\" at all, but something a lot more suited for our purpose.\n> So at least the comment needs to be updated. I do not have strong\n> opinion on calling this test_seq when it acts differently from seq;\n> it is not confusing enough to make me push something longer that is\n> different from \"seq\", e.g. test_sequence.\n> \n\nI prefer \"test_seq\" because it reminds seq which helps learning how to\nuse it. If some other seq feature is ever needed (e.g. increment value,\ndecrementing), it may be added at any time (but I don't think so, there\nare only few usages after years of test suite existence).\n\n> Wouldn't it be cleaner and readable to write it like this\n> \n> \t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$1\" \"$2\"\n> \n> by the way?\n"},{"id":"196446","messageId":"1344032464-14104-1-git-send-email-michal.kiedrowicz@gmail.com","threadId":"31173","inReplyTo":"7v3943bsuc.fsf@alter.siamese.dyndns.org","subject":"[PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-03T22:21:04Z","receivedAt":"2012-08-03T22:21:04Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Jeff King wrote:\n\n\tThe seq command is GNU-ism, and is missing at least in older BSD\n\treleases and their derivatives, not to mention antique\n\tcommercial Unixes.\n\n\tWe already purged it in b3431bc (Don't use seq in tests, not\n\teveryone has it, 2007-05-02), but a few new instances have crept\n\tin. They went unnoticed because they are in scripts that are not\n\trun by default.\n\nReplace them with test_seq that is implemented with a Perl snippet\n(proposed by Jeff). This is better than inlining this snippet\neverywhere it's needed because it's easier to read and it's easier to\nchange the implementation (e.g. to C) if we ever decide to remove Perl\nfrom the test suite.\n\nNote that test_seq is not a complete replacement for seq(1). It just\nhas what we need now.\n\nThere are also many places that do `for i in 1 2 3 ...` but I'm not sure\nif it's worth converting them to test_seq. That would introduce running\nmore processes of Perl.\n\nSigned-off-by: Michał Kiedrowicz \n---\nChanges since previous version:\n\n\t* Removed \"This commit replaces\" from commit message\n\t* Reworded test_seq description\n\t* Now $first and $last are passed to Perl as arguments\n\n t/perf/perf-lib.sh | 2 +-\n t/t5551-http-fetch.sh | 2 +-\n t/test-lib-functions.sh | 20 ++++++++++++++++++++\n 3 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\nindex 5580c22..a1361e5 100644\n--- a/t/perf/perf-lib.sh\n+++ b/t/perf/perf-lib.sh\n@@ -163,7 +163,7 @@ test_perf () {\n \t\telse\n \t\t\techo \"perf $test_count - $1:\"\n \t\tfi\n-\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n+\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n \t\t\tsay >&3 \"running: $2\"\n \t\t\tif test_run_perf_ \"$2\"\n \t\t\tthen\ndiff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\nindex fadf2f2..91eaf53 100755\n--- a/t/t5551-http-fetch.sh\n+++ b/t/t5551-http-fetch.sh\n@@ -114,7 +114,7 @@ test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n \t(\n \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n-\tfor i in `seq 50000`\n+\tfor i in `test_seq 50000`\n \tdo\n \t\techo \"commit refs/heads/too-many-refs\"\n \t\techo \"mark :$i\"\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 80daaca..c8b4ae3 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -530,6 +530,26 @@ test_cmp() {\n \t$GIT_TEST_CMP \"$@\"\n }\n \n+# Print a sequence of numbers or letters in increasing order. This is\n+# similar to GNU seq(1), but the latter might not be available\n+# everywhere. It may be used like:\n+#\n+#\tfor i in `test_seq 100`; do\n+#\t\tfor j in `test_seq 10 20`; do\n+#\t\t\tfor k in `test_seq a z`; do\n+#\t\t\t\techo $i-$j-$k\n+#\t\t\tdone\n+#\t\tdone\n+#\tdone\n+\n+test_seq () {\n+\ttest $# = 2 && { first=$1; shift; } || first=1\n+\ttest $# = 1 ||\n+\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n+\tlast=$1\n+\t\"$PERL_PATH\" -le 'print for \"$ARGV[0]\"..\"$ARGV[1]\"' \"$first\" \"$last\"\n+}\n+\n # This function can be used to schedule some commands to be run\n # unconditionally at the end of the test to restore sanity:\n #\n-- \n1.7.11.rc0.212.g37218b0.dirty\n"},{"id":"196447","messageId":"7vr4rna8y4.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"1344032464-14104-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-03T22:48:19Z","receivedAt":"2012-08-03T22:48:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michał Kiedrowicz writes:\n\n> Jeff King wrote:\n>\n> \tThe seq command is GNU-ism, and is missing at least in older BSD\n> \treleases and their derivatives, not to mention antique\n> \tcommercial Unixes.\n>\n> \tWe already purged it in b3431bc (Don't use seq in tests, not\n> \teveryone has it, 2007-05-02), but a few new instances have crept\n> \tin. They went unnoticed because they are in scripts that are not\n> \trun by default.\n>\n> Replace them with test_seq that is implemented with a Perl snippet\n> (proposed by Jeff). This is better than inlining this snippet\n> everywhere it's needed because it's easier to read and it's easier to\n> change the implementation (e.g. to C) if we ever decide to remove Perl\n> from the test suite.\n>\n> Note that test_seq is not a complete replacement for seq(1). It just\n> has what we need now.\n>\n> There are also many places that do `for i in 1 2 3 ...` but I'm not sure\n> if it's worth converting them to test_seq. That would introduce running\n> more processes of Perl.\n>\n> Signed-off-by: Michał Kiedrowicz \n> ---\n\nThanks; Jeff, ack?\n\nI have one minor nit that I am tempted to fix while queuing---see\nbelow.\n\n> Changes since previous version:\n>\n> \t* Removed \"This commit replaces\" from commit message\n> \t* Reworded test_seq description\n> \t* Now $first and $last are passed to Perl as arguments\n>\n> t/perf/perf-lib.sh | 2 +-\n> t/t5551-http-fetch.sh | 2 +-\n> t/test-lib-functions.sh | 20 ++++++++++++++++++++\n> 3 files changed, 22 insertions(+), 2 deletions(-)\n>\n> diff --git a/t/perf/perf-lib.sh b/t/perf/perf-lib.sh\n> index 5580c22..a1361e5 100644\n> --- a/t/perf/perf-lib.sh\n> +++ b/t/perf/perf-lib.sh\n> @@ -163,7 +163,7 @@ test_perf () {\n> \t\telse\n> \t\t\techo \"perf $test_count - $1:\"\n> \t\tfi\n> -\t\tfor i in $(seq 1 $GIT_PERF_REPEAT_COUNT); do\n> +\t\tfor i in $(test_seq 1 $GIT_PERF_REPEAT_COUNT); do\n> \t\t\tsay >&3 \"running: $2\"\n> \t\t\tif test_run_perf_ \"$2\"\n> \t\t\tthen\n> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh\n> index fadf2f2..91eaf53 100755\n> --- a/t/t5551-http-fetch.sh\n> +++ b/t/t5551-http-fetch.sh\n> @@ -114,7 +114,7 @@ test -n \"$GIT_TEST_LONG\" && test_set_prereq EXPENSIVE\n> test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '\n> \t(\n> \tcd \"$HTTPD_DOCUMENT_ROOT_PATH/repo.git\" &&\n> -\tfor i in `seq 50000`\n> +\tfor i in `test_seq 50000`\n> \tdo\n> \t\techo \"commit refs/heads/too-many-refs\"\n> \t\techo \"mark :$i\"\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 80daaca..c8b4ae3 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -530,6 +530,26 @@ test_cmp() {\n> \t$GIT_TEST_CMP \"$@\"\n> }\n> \n> +# Print a sequence of numbers or letters in increasing order. This is\n> +# similar to GNU seq(1), but the latter might not be available\n> +# everywhere. It may be used like:\n> +#\n> +#\tfor i in `test_seq 100`; do\n> +#\t\tfor j in `test_seq 10 20`; do\n> +#\t\t\tfor k in `test_seq a z`; do\n> +#\t\t\t\techo $i-$j-$k\n> +#\t\t\tdone\n> +#\t\tdone\n> +#\tdone\n> +\n> +test_seq () {\n> +\ttest $# = 2 && { first=$1; shift; } || first=1\n> +\ttest $# = 1 ||\n> +\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n> +\tlast=$1\n> +\t\"$PERL_PATH\" -le 'print for \"$ARGV[0]\"..\"$ARGV[1]\"' \"$first\" \"$last\"\n\nI'd prefer not to have dq around $ARGV[]; is there a reason to have\none around these?\n\n> +}\n> +\n> # This function can be used to schedule some commands to be run\n> # unconditionally at the end of the test to restore sanity:\n> #\n"},{"id":"196448","messageId":"20120803230804.GA14447@sigill.intra.peff.net","threadId":"31173","inReplyTo":"7vr4rna8y4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-03T23:08:04Z","receivedAt":"2012-08-03T23:08:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 03, 2012 at 03:48:19PM -0700, Junio C Hamano wrote:\n\n> Michał Kiedrowicz writes:\n> \n> > Jeff King wrote:\n> >\n> > \tThe seq command is GNU-ism, and is missing at least in older BSD\n> > \treleases and their derivatives, not to mention antique\n> > \tcommercial Unixes.\n> >\n> > \tWe already purged it in b3431bc (Don't use seq in tests, not\n> > \teveryone has it, 2007-05-02), but a few new instances have crept\n> > \tin. They went unnoticed because they are in scripts that are not\n> > \trun by default.\n> >\n> > Replace them with test_seq that is implemented with a Perl snippet\n> > (proposed by Jeff). This is better than inlining this snippet\n> > everywhere it's needed because it's easier to read and it's easier to\n> > change the implementation (e.g. to C) if we ever decide to remove Perl\n> > from the test suite.\n> >\n> > Note that test_seq is not a complete replacement for seq(1). It just\n> > has what we need now.\n> >\n> > There are also many places that do `for i in 1 2 3 ...` but I'm not sure\n> > if it's worth converting them to test_seq. That would introduce running\n> > more processes of Perl.\n> >\n> > Signed-off-by: Michał Kiedrowicz \n> > ---\n> \n> Thanks; Jeff, ack?\n\nYeah,\n\nAcked-by: Jeff King \n\n> > +\t\"$PERL_PATH\" -le 'print for \"$ARGV[0]\"..\"$ARGV[1]\"' \"$first\" \"$last\"\n> \n> I'd prefer not to have dq around $ARGV[]; is there a reason to have\n> one around these?\n\nI don't think they accomplish anything, and it is slightly easier to\nread without them. I'm fine either way.\n\n-Peff\n"},{"id":"196449","messageId":"7vfw83a7t5.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"1344032464-14104-1-git-send-email-michal.kiedrowicz@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-03T23:12:54Z","receivedAt":"2012-08-03T23:12:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tentatively I'll queue this one on top, but I am tempted to squash\nthis in before merging the topic down.\n\n-- >8 --\nSubject: [PATCH] fixup! tests: Introduce test_seq\n\nComplex chains of && and || are harder to read when used as\nreplacement for if/else statements, but it is easy to rewrite it\nwith a case/esac in this case.\n\nAvoid using unnecessary variables $first and $last.\n\nSigned-off-by: Junio C Hamano \n---\n t/test-lib-functions.sh | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex c8b4ae3..7dc70eb 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -532,7 +532,7 @@ test_cmp() {\n \n # Print a sequence of numbers or letters in increasing order. This is\n # similar to GNU seq(1), but the latter might not be available\n-# everywhere. It may be used like:\n+# everywhere (and does not do letters). It may be used like:\n #\n #\tfor i in `test_seq 100`; do\n #\t\tfor j in `test_seq 10 20`; do\n@@ -543,11 +543,12 @@ test_cmp() {\n #\tdone\n \n test_seq () {\n-\ttest $# = 2 && { first=$1; shift; } || first=1\n-\ttest $# = 1 ||\n-\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n-\tlast=$1\n-\t\"$PERL_PATH\" -le 'print for \"$ARGV[0]\"..\"$ARGV[1]\"' \"$first\" \"$last\"\n+\tcase $# in\n+\t1)\tset 1 \"$@\" ;;\n+\t2)\t;;\n+\t*)\terror \"bug in the test script: not 1 or 2 parameters to test_seq\" ;;\n+\tesac\n+\t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$@\"\n }\n \n # This function can be used to schedule some commands to be run\n-- \n1.7.12.rc1.50.g3df08cf\n"},{"id":"196452","messageId":"20120804101403.10ad79b5@gmail.com","threadId":"31173","inReplyTo":"7vfw83a7t5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-04T08:14:03Z","receivedAt":"2012-08-04T08:14:03Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Junio C Hamano wrote:\n\n> Tentatively I'll queue this one on top, but I am tempted to squash\n> this in before merging the topic down.\n> \n> -- >8 --\n> Subject: [PATCH] fixup! tests: Introduce test_seq\n> \n> Complex chains of && and || are harder to read when used as\n> replacement for if/else statements, but it is easy to rewrite it\n> with a case/esac in this case.\n\nI just copied it from test_expect_success, but yeah, case/esac is\nclearer.\n\n> \n> Avoid using unnecessary variables $first and $last.\n> \n> Signed-off-by: Junio C Hamano \n> ---\n> t/test-lib-functions.sh | 13 +++++++------\n> 1 file changed, 7 insertions(+), 6 deletions(-)\n> \n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index c8b4ae3..7dc70eb 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -532,7 +532,7 @@ test_cmp() {\n> \n> # Print a sequence of numbers or letters in increasing order. This is\n> # similar to GNU seq(1), but the latter might not be available\n> -# everywhere. It may be used like:\n> +# everywhere (and does not do letters). It may be used like:\n> #\n> #\tfor i in `test_seq 100`; do\n> #\t\tfor j in `test_seq 10 20`; do\n> @@ -543,11 +543,12 @@ test_cmp() {\n> #\tdone\n> \n> test_seq () {\n> -\ttest $# = 2 && { first=$1; shift; } || first=1\n> -\ttest $# = 1 ||\n> -\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n> -\tlast=$1\n> -\t\"$PERL_PATH\" -le 'print for \"$ARGV[0]\"..\"$ARGV[1]\"' \"$first\" \"$last\"\n> +\tcase $# in\n> +\t1)\tset 1 \"$@\" ;;\n> +\t2)\t;;\n> +\t*)\terror \"bug in the test script: not 1 or 2 parameters to test_seq\" ;;\n> +\tesac\n> +\t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$@\"\n> }\n> \n> # This function can be used to schedule some commands to be run\n"},{"id":"196459","messageId":"501D4FF0.4060109@kdbg.org","threadId":"31173","inReplyTo":"20120804000904.13c4162b@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2012-08-04T16:38:08Z","receivedAt":"2012-08-04T16:38:08Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 04.08.2012 00:09, schrieb Michał Kiedrowicz:\n> Junio C Hamano wrote:\n>> I do not have strong\n>> opinion on calling this test_seq when it acts differently from seq;\n>> it is not confusing enough to make me push something longer that is\n>> different from \"seq\", e.g. test_sequence.\n> \n> I prefer \"test_seq\" because it reminds seq which helps learning how to\n> use it. If some other seq feature is ever needed (e.g. increment value,\n> decrementing), it may be added at any time (but I don't think so, there\n> are only few usages after years of test suite existence).\n\nAnd the reason for this is that we always told people \"don't use seq\"\nand they submitted an updated patch. What would we have to do now? We\nhave to tell them \"don't use seq, use test_seq\". Therefore, the patch\ndoes not accomplish anything useful, IMO.\n\nThe function should really just be named 'seq'.\n\nOr how about this strategy:\n\nseq () {\n\tunset -f seq\n\tif ! seq 1 2 >/dev/null 2>&1\n\tthen\n\t\t# don't have a working seq; provide it as a function\n\t\tseq () {\n\t\t\tinsert your definition here\n\t\t}\n\tfi\n\tseq \"$@\"\n}\n\nbut it is not my favorite.\n\n-- Hannes\n"},{"id":"196470","messageId":"loom.20120804T230218-811@post.gmane.org","threadId":"31173","inReplyTo":"551f7f77570c84017ae93988f9202854@imap.force9.net","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Adam Butcher","fromEmail":"dev.lists@jessamine.co.uk","sentAt":"2012-08-04T21:07:35Z","receivedAt":"2012-08-04T21:07:35Z","isPatch":true,"sender":{"key":"dev.lists@jessamine.co.uk","avatar":null},"body":"When operating in --break-rewrites (-B) mode on a file with no newline\nterminator (and assuming --break-rewrites determines that the diff\n_is_ a rewrite), git diff previously concatenated the indicator comment\n'\\ No newline at end of file' directly to the terminating line rather\nthan on a line of its own. The resulting diff is broken; claiming\nthat the last line actually contains the indicator text. Without -B\nthere is no problem with the same files.\n\nThis patch fixes the former case by inserting a newline into the\noutput prior to emitting the indicator comment.\n\nA couple of tests have been added to the rewrite suite to confirm that\nthe indicator comment is generated on its own line in both plain diff\nand rewrite mode. The latter test fails if the functional part of\nthis patch (i.e. diff.c) is reverted.\n---\n\nUpdates: Test only:\n\n - removed redundant para from commit msg\n - use test_seq shell function instead of seq\n - pull prep statements into individual tests\n - test expected success of git commands in prep\n - confirm that rewrite is considered a rewrite by diff -B\n - remove superfluous comments in favor of test descriptions\n - use variable to spell 'no newline' annotation to support simpler\n maintenance whilst still allowing to check for unexpected leading\n or trailing characters.\n\n diff.c | 1 +\n t/t4022-diff-rewrite.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 43 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 1a594df..f333de8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,\n \tif (!endp) {\n \t\tconst char *plain = diff_get_color(ecb->color_diff,\n \t\t\t\t\t\t DIFF_PLAIN);\n+\t\tputc('\\n', ecb->opt->file);\n \t\temit_line_0(ecb->opt, plain, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t}\ndiff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh\nindex c00a94b..1b7ae9f 100755\n--- a/t/t4022-diff-rewrite.sh\n+++ b/t/t4022-diff-rewrite.sh\n@@ -66,5 +66,47 @@ test_expect_success 'suppress deletion diff with -B -D' '\n \tgrep -v \"Linus Torvalds\" actual\n '\n \n+test_expect_success 'generate initial \"no newline at eof\" sequence file and \ncommit' '\n+\n+ test_seq 1 99 >seq &&\n+ printf 100 >>seq &&\n+ git add seq &&\n+ git commit seq -m seq\n+'\n+\n+test_expect_success 'rewrite the middle 90% of sequence file and terminate with \nnewline' '\n+\n+ test_seq 1 5 >seq &&\n+ test_seq 9331 9420 >>seq &&\n+ test_seq 96 100 >>seq\n+'\n+\n+test_expect_success 'confirm that sequence file is considered a rewrite' '\n+\n+ git diff -B seq >res &&\n+ grep \"dissimilarity index\" res\n+'\n+\n+# Full annotation string used to check for erroneous leading or\n+# trailing characters. Backslash is double escaped due to usage\n+# within dq argument to grep expansion below. \n+no_newline_anno='\\\\\\\\ No newline at end of file'\n+\n+test_expect_success 'no newline at eof is on its own line without -B' '\n+\n+\tgit diff seq >res &&\n+\tgrep \"^'\"$no_newline_anno\"'$\" res &&\n+\tgrep -v \"^.\\\\+'\"$no_newline_anno\"'\" res &&\n+\tgrep -v \"'\"$no_newline_anno\"'.\\\\+$\" res\n+'\n+\n+test_expect_success 'no newline at eof is on its own line with -B' '\n+\n+\tgit diff -B seq >res &&\n+\tgrep \"^'\"$no_newline_anno\"'$\" res &&\n+\tgrep -v \"^.\\\\+'\"$no_newline_anno\"'\" res &&\n+\tgrep -v \"'\"$no_newline_anno\"'.\\\\+$\" res\n+'\n+\n test_done\n \n-- \n1.7.11.msysgit.1.1.gf0affa1\n"},{"id":"196473","messageId":"loom.20120805T000957-218@post.gmane.org","threadId":"31173","inReplyTo":"20120804101403.10ad79b5@gmail.com","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Adam Butcher","fromEmail":"dev.lists@jessamine.co.uk","sentAt":"2012-08-04T22:10:08Z","receivedAt":"2012-08-04T22:10:08Z","isPatch":true,"sender":{"key":"dev.lists@jessamine.co.uk","avatar":null},"body":"Michał Kiedrowicz gmail.com> writes:\n> Junio C Hamano pobox.com> wrote:\n> > diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> > index c8b4ae3..7dc70eb 100644\n> > --- a/t/test-lib-functions.sh\n> > +++ b/t/test-lib-functions.sh\n> > @@ -543,11 +543,12 @@ test_cmp() {\n> > #\tdone\n> > \n> > test_seq () {\n> > -\ttest $# = 2 && { first=$1; shift; } || first=1\n> > -\ttest $# = 1 ||\n> > -\terror \"bug in the test script: not 1 or 2 parameters to test_seq\"\n> > -\tlast=$1\n> > -\t\"$PERL_PATH\" -le 'print for \"$ARGV[0]\"..\"$ARGV[1]\"' \"$first\" \"$last\"\n> > +\tcase $# in\n> > +\t1)\tset 1 \"$@\" ;;\n> > +\t2)\t;;\n> > +\t*)\terror \"bug in the test script: not 1 or 2 parameters to \ntest_seq\" ;;\n> > +\tesac\n> > +\t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$@\"\n> > }\n> > \n> > # This function can be used to schedule some commands to be run\n\n-- >8 --\nSubject: [PATCH] Fixup test_seq: ensure arguments passed to script.\n\nIf the arguments passed to to test_seq start with '-' (e.g. negative\nintegers) they are considered perl options and the program errors. By\nprefixing the user argument list with '--' when passing to perl, this\nis avoid and sequences involving negative numbers are possible.\n---\n t/test-lib-functions.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 5a1a95a..ed44f5e 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -539,7 +539,7 @@ test_seq () {\n \t2)\t;;\n \t*)\terror \"bug in the test script: not 1 or 2 parameters to \ntest_seq\" ;;\n \tesac\n-\t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' \"$@\"\n+\t\"$PERL_PATH\" -le 'print for $ARGV[0]..$ARGV[1]' -- \"$@\"\n }\n \n # This function can be used to schedule some commands to be run\n-- \n1.7.11.msysgit.1.1.gf0affa1\n"},{"id":"196476","messageId":"7vpq768dhw.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"501D4FF0.4060109@kdbg.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-04T23:05:15Z","receivedAt":"2012-08-04T23:05:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> And the reason for this is that we always told people \"don't use seq\"\n> and they submitted an updated patch. What would we have to do now? We\n> have to tell them \"don't use seq, use test_seq\". Therefore, the patch\n> does not accomplish anything useful, IMO.\n>\n> The function should really just be named 'seq'.\n>\n> Or how about this strategy:\n> ...\n> but it is not my favorite.\n\nWhy not? That implementation looks like a logical and natural\nconsequence of \"should relly just be named 'seq'\" suggestion.\n\nHaving said that, we already say \"don't use cmp, use test_cmp\", so\nit might not be such a big deal, even though I find the reasoning in\nthe first paragraph I quoted above from your message quite sane and\nconvincing to me.\n"},{"id":"196479","messageId":"7vobmq6sd9.fsf@alter.siamese.dyndns.org","threadId":"31173","inReplyTo":"loom.20120804T230218-811@post.gmane.org","subject":"Re: [PATCH] Fix 'No newline...' annotation in rewrite diffs.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-08-05T01:26:58Z","receivedAt":"2012-08-05T01:26:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Butcher writes:\n\n> When operating in --break-rewrites (-B) mode on a file with no newline\n> terminator (and assuming --break-rewrites determines that the diff\n> _is_ a rewrite), git diff previously concatenated the indicator comment\n> '\\ No newline at end of file' directly to the terminating line rather\n> than on a line of its own. The resulting diff is broken; claiming\n> that the last line actually contains the indicator text. Without -B\n> there is no problem with the same files.\n>\n> This patch fixes the former case by inserting a newline into the\n> output prior to emitting the indicator comment.\n>\n> A couple of tests have been added to the rewrite suite to confirm that\n> the indicator comment is generated on its own line in both plain diff\n> and rewrite mode. The latter test fails if the functional part of\n> this patch (i.e. diff.c) is reverted.\n> ---\n\nThanks. You need your sign-off immediately before the \"---\" line.\n\nWhen the problem description at the beginning of a log message is\nabout the current status of the code (which is almost always the\ncase), it generally does not need to be clarified with \"previously\".\n\nA (POSIXy technical term) for the last line that does not end with\nthe newline is \"incomplete line\", I think.\n\n Cf. http://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xbd_chap03.html#tag_21_03_00_67\n\nI'd describe this perhaps like so if I were doing this patch:\n\n Fix '\\ No newline...' annotation in rewrite diffs\n\n When a file that ends with an incomplete line is expressed as a\n complete rewrite with the -B option, git diff incorrectly\n appends the incomplete line indicator \"\\ No newline at end of\n file\" after such a line, rather than writing it on a line of its\n own (the output codepath for normal output without -B does not\n have this problem). Add a LF after the incomplete line before\n writing the \"\\ No newline ...\" out to fix this.\n\n Add a couple of tests to confirm that the indicator comment is\n generated on its own line in both plain diff and rewrite mode.\n\n> diff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh\n> index c00a94b..1b7ae9f 100755\n> --- a/t/t4022-diff-rewrite.sh\n> +++ b/t/t4022-diff-rewrite.sh\n> @@ -66,5 +66,47 @@ test_expect_success 'suppress deletion diff with -B -D' '\n> \tgrep -v \"Linus Torvalds\" actual\n> '\n> \n> +test_expect_success 'generate initial \"no newline at eof\" sequence file and \n> commit' '\n\nLine-wrapped.\n\n> +test_expect_success 'confirm that sequence file is considered a rewrite' '\n> +\n> + git diff -B seq >res &&\n> + grep \"dissimilarity index\" res\n> +'\n\nGood thinking to make sure the condition to trigger the issue still\nholds in the future.\n\n> +# Full annotation string used to check for erroneous leading or\n> +# trailing characters. Backslash is double escaped due to usage\n> +# within dq argument to grep expansion below. \n> +no_newline_anno='\\\\\\\\ No newline at end of file'\n> +\n> +test_expect_success 'no newline at eof is on its own line without -B' '\n> +\n> +\tgit diff seq >res &&\n> +\tgrep \"^'\"$no_newline_anno\"'$\" res &&\n\nI think it is sufficient to write this line as:\n\n\tgrep \"^$no_newline_anno$\" res &&\n\nThe third parameter to test_expect_success function is inside a sq,\nso it will have the above string as-is, with $no_newline_anno not\nexpanded, and then when the string is eval'ed, the variable is\nvisible to the eval.\n\nSo the above should be more like:\n\n # Full annotation string used to check for erroneous leading or\n # trailing characters.\n no_newline_anno='\\\\ No newline at end of file'\n\n test_expect_success 'no newline at eof is on its own line without -B' '\n git diff seq >res &&\n grep \"^$no_newline_anno$\" res &&\n\n> +\tgrep -v \"^.\\\\+'\"$no_newline_anno\"'\" res &&\n> +\tgrep -v \"'\"$no_newline_anno\"'.\\\\+$\" res\n\nConverting these two the same way, we would get\n\n\tgrep -v \"^.\\\\+$no_newline_anno\" res &&\n\tgrep -v \"$no_newline_anno.\\\\+$\" res\n\nbut isn't this doubly wrong?\n\n (1) \\+ to require \"one-or-more\", which is otherwise not supported\n in BRE, is a GNU extension. It is simple to fix it by writing\n \"^..*$no_newline_anno\" to say \"what we try to find appears\n somewhere not at the beginning of line\".\n\n (2) The \"grep -v\" shows the lines that express all the additions\n and deletions prefixed with + and - as they do not match \"the\n line has the marker misplaced in the middle of the line\"\n criteria. Doesn't grep return true in that case, as it found\n some matching lines, even if you had \"\\ No newline\" in the\n middle of some lines?\n\nAs I already said, I do not think hardcoding the whole \"No newline\nat end of line\" in this test is a good idea anyway, and because you\nknow the text being compared does not have any backslash in it, it\nsuffices to make sure that the only occurrence of a backslash is on\na single line and at the beginning, I think.\n\nIn other words,\n\n\tgrep \"^\\\\ \" res && ! grep \"^..*\\\\ \" res\n\nor something.\n\nI'll tentatively queue a tweaked version on 'pu', but we would at\nleast want a sign-off.\n\nThanks.\n"},{"id":"196482","messageId":"1344150365-86764-1-git-send-email-dev.lists@jessamine.co.uk","threadId":"31173","inReplyTo":"7vobmq6sd9.fsf@alter.siamese.dyndns.org","subject":"[PATCH] Fix '\\ No newline...' annotation in rewrite diffs","fromName":"Adam Butcher","fromEmail":"dev.lists@jessamine.co.uk","sentAt":"2012-08-05T07:06:05Z","receivedAt":"2012-08-05T07:06:05Z","isPatch":true,"sender":{"key":"dev.lists@jessamine.co.uk","avatar":null},"body":"When a file that ends with an incomplete line is expressed as a\ncomplete rewrite with the -B option, git diff incorrectly appends the\nincomplete line indicator \"\\ No newline at end of file\" after such a\nline, rather than writing it on a line of its own (the output codepath\nfor normal output without -B does not have this problem). Add a LF\nafter the incomplete line before writing the \"\\ No newline ...\" out\nto fix this.\n\nAdd a couple of tests to confirm that the indicator comment is\ngenerated on its own line in both plain diff and rewrite mode.\n\nSigned-off-by: Adam Butcher \n---\n\nUpdates:\n\n - replace commit msg with revised suggestion from Junio\n - remove hardcoded 'No newline...' in tests and simplify\n\n diff.c | 1 +\n t/t4022-diff-rewrite.sh | 33 +++++++++++++++++++++++++++++++++\n 2 files changed, 34 insertions(+)\n\ndiff --git a/diff.c b/diff.c\nindex 1a594df..f333de8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -574,6 +574,7 @@ static void emit_rewrite_lines(struct emit_callback *ecb,\n \tif (!endp) {\n \t\tconst char *plain = diff_get_color(ecb->color_diff,\n \t\t\t\t\t\t DIFF_PLAIN);\n+\t\tputc('\\n', ecb->opt->file);\n \t\temit_line_0(ecb->opt, plain, reset, '\\\\',\n \t\t\t nneof, strlen(nneof));\n \t}\ndiff --git a/t/t4022-diff-rewrite.sh b/t/t4022-diff-rewrite.sh\nindex c00a94b..05ac3e9 100755\n--- a/t/t4022-diff-rewrite.sh\n+++ b/t/t4022-diff-rewrite.sh\n@@ -66,5 +66,38 @@ test_expect_success 'suppress deletion diff with -B -D' '\n \tgrep -v \"Linus Torvalds\" actual\n '\n \n+test_expect_success 'generate initial \"no newline at eof\" sequence file and commit' '\n+\n+\ttest_seq 1 99 >seq &&\n+\tprintf 100 >>seq &&\n+\tgit add seq &&\n+\tgit commit seq -m seq\n+'\n+\n+test_expect_success 'rewrite the middle 90% of sequence file and terminate with newline' '\n+\n+\ttest_seq 1 5 >seq &&\n+\ttest_seq 9331 9420 >>seq &&\n+\ttest_seq 96 100 >>seq\n+'\n+\n+test_expect_success 'confirm that sequence file is considered a rewrite' '\n+\n+\tgit diff -B seq >res &&\n+\tgrep \"dissimilarity index\" res\n+'\n+\n+test_expect_success 'no newline at eof is on its own line without -B' '\n+\n+\tgit diff seq >res &&\n+\tgrep \"^\\\\\\\\ \" res && ! grep \"^..*\\\\\\\\ \" res\n+'\n+\n+test_expect_success 'no newline at eof is on its own line with -B' '\n+\n+\tgit diff -B seq >res &&\n+\tgrep \"^\\\\\\\\ \" res && ! grep \"^..*\\\\\\\\ \" res\n+'\n+\n test_done\n \n-- \n1.7.11.msysgit.1.1.gf0affa1\n"},{"id":"196552","messageId":"20120806195256.43ec44de@gmail.com","threadId":"31173","inReplyTo":"501D4FF0.4060109@kdbg.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Michał Kiedrowicz","fromEmail":"michal.kiedrowicz@gmail.com","sentAt":"2012-08-06T17:52:56Z","receivedAt":"2012-08-06T17:52:56Z","isPatch":true,"sender":{"key":"michal.kiedrowicz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/14072847?v=4"},"body":"Johannes Sixt wrote:\n\n> Am 04.08.2012 00:09, schrieb Michał Kiedrowicz:\n> > Junio C Hamano wrote:\n> >> I do not have strong\n> >> opinion on calling this test_seq when it acts differently from seq;\n> >> it is not confusing enough to make me push something longer that is\n> >> different from \"seq\", e.g. test_sequence.\n> > \n> > I prefer \"test_seq\" because it reminds seq which helps learning how to\n> > use it. If some other seq feature is ever needed (e.g. increment value,\n> > decrementing), it may be added at any time (but I don't think so, there\n> > are only few usages after years of test suite existence).\n> \n> And the reason for this is that we always told people \"don't use seq\"\n> and they submitted an updated patch. What would we have to do now? We\n> have to tell them \"don't use seq, use test_seq\". Therefore, the patch\n> does not accomplish anything useful, IMO.\n> \n> The function should really just be named 'seq'.\n\nMy reasoning was that there is already test_cmp, so let's make test_seq,\nbut I agree with you that it doesn't solve the issue completely. So my 2\ncents is that it would be best to stay with not allowing seq in the test\nsuite.\n\n> \n> Or how about this strategy:\n> \n> seq () {\n> \tunset -f seq\n> \tif ! seq 1 2 >/dev/null 2>&1\n> \tthen\n> \t\t# don't have a working seq; provide it as a function\n> \t\tseq () {\n> \t\t\tinsert your definition here\n> \t\t}\n> \tfi\n> \tseq \"$@\"\n> }\n> \n> but it is not my favorite.\n> \n> -- Hannes\n"},{"id":"196560","messageId":"20120806201600.GA11078@sigill.intra.peff.net","threadId":"31173","inReplyTo":"501D4FF0.4060109@kdbg.org","subject":"Re: [PATCH] tests: Introduce test_seq","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-08-06T20:16:00Z","receivedAt":"2012-08-06T20:16:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 04, 2012 at 06:38:08PM +0200, Johannes Sixt wrote:\n\n> And the reason for this is that we always told people \"don't use seq\"\n> and they submitted an updated patch. What would we have to do now? We\n> have to tell them \"don't use seq, use test_seq\". Therefore, the patch\n> does not accomplish anything useful, IMO.\n> \n> The function should really just be named 'seq'.\n> \n> Or how about this strategy:\n> \n> seq () {\n> \tunset -f seq\n> \tif ! seq 1 2 >/dev/null 2>&1\n> \tthen\n> \t\t# don't have a working seq; provide it as a function\n> \t\tseq () {\n> \t\t\tinsert your definition here\n> \t\t}\n> \tfi\n> \tseq \"$@\"\n> }\n> \n> but it is not my favorite.\n\nNo, falling back just makes that problem worse. Our test_seq is not\nfully compatible with seq. So anyone who uses an advanced feature of seq\n(like \"seq 0 100 10\" or \"seq -f %02g 1 10\") will have the test work on\ntheir system (with seq) and then break on some other random platform.\nSo instead of saying \"no, don't use seq, use test_seq\", reviewers have\nto catch it and say \"don't use some features of seq, because the\nfallback doesn't have them\".\n\nIf you eliminate the fallback, then at least the reviewers do not have\nto catch it (the tests will never work for the patch writer, since they\nwill always use our feature-less seq replacement). But I find it\nslightly confusion-inducing to call something that is not seq-compatible\n\"seq\".\n\n-Peff\n"}]}