{"thread":{"id":"37655","subject":"[PATCH 1/2] sha1-array: add test-sha1-array and basic tests","startedAt":"2014-10-01T09:40:12Z","lastAt":"2014-10-01T14:23:36Z","messageCount":9,"participants":["René Scharfe","Jeff King","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"250059","messageId":"542BCBFC.5000509@web.de","threadId":"37655","inReplyTo":null,"subject":"[PATCH 1/2] sha1-array: add test-sha1-array and basic tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-01T09:40:12Z","receivedAt":"2014-10-01T09:40:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Signed-off-by: Rene Scharfe <l.s.r@web.de>\n---\n .gitignore            |  1 +\n Makefile              |  1 +\n t/t0064-sha1-array.sh | 64 +++++++++++++++++++++++++++++++++++++++++++++++++++\n test-sha1-array.c     | 34 +++++++++++++++++++++++++++\n 4 files changed, 100 insertions(+)\n create mode 100755 t/t0064-sha1-array.sh\n create mode 100644 test-sha1-array.c\n\ndiff --git a/.gitignore b/.gitignore\nindex 5bfb234..9ec40fa 100644\n--- a/.gitignore\n+++ b/.gitignore\n@@ -199,6 +199,7 @@\n /test-revision-walking\n /test-run-command\n /test-sha1\n+/test-sha1-array\n /test-sigchain\n /test-string-list\n /test-subprocess\ndiff --git a/Makefile b/Makefile\nindex f34a2d4..356feb5 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -568,6 +568,7 @@ TEST_PROGRAMS_NEED_X += test-revision-walking\n TEST_PROGRAMS_NEED_X += test-run-command\n TEST_PROGRAMS_NEED_X += test-scrap-cache-tree\n TEST_PROGRAMS_NEED_X += test-sha1\n+TEST_PROGRAMS_NEED_X += test-sha1-array\n TEST_PROGRAMS_NEED_X += test-sigchain\n TEST_PROGRAMS_NEED_X += test-string-list\n TEST_PROGRAMS_NEED_X += test-subprocess\ndiff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\nnew file mode 100755\nindex 0000000..bd68789\n--- /dev/null\n+++ b/t/t0064-sha1-array.sh\n@@ -0,0 +1,64 @@\n+#!/bin/sh\n+\n+test_description='basic tests for the SHA1 array implementation'\n+. ./test-lib.sh\n+\n+echo20() {\n+\tprefix=\"$1\"\n+\tshift\n+\twhile test $# -gt 0\n+\tdo\n+\t\techo \"$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n+\t\tshift\n+\tdone\n+}\n+\n+test_expect_success 'ordered enumeration' '\n+\techo20 \"\" 44 55 88 aa >expect &&\n+\t{\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo for_each_unique\n+\t} | test-sha1-array >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ordered enumeration with duplicate suppression' '\n+\techo20 \"\" 44 55 88 aa >expect &&\n+\t{\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo for_each_unique\n+\t} | test-sha1-array >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'lookup' '\n+\t{\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"lookup \" 55\n+\t} | test-sha1-array >actual &&\n+\tn=$(cat actual) &&\n+\ttest \"$n\" -eq 1\n+'\n+\n+test_expect_success 'lookup non-existing entry' '\n+\t{\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"lookup \" 33\n+\t} | test-sha1-array >actual &&\n+\tn=$(cat actual) &&\n+\ttest \"$n\" -lt 0\n+'\n+\n+test_expect_success 'lookup with duplicates' '\n+\t{\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"lookup \" 55\n+\t} | test-sha1-array >actual &&\n+\tn=$(cat actual) &&\n+\ttest \"$n\" -ge 2 &&\n+\ttest \"$n\" -le 3\n+'\n+\n+test_done\ndiff --git a/test-sha1-array.c b/test-sha1-array.c\nnew file mode 100644\nindex 0000000..ddc491e\n--- /dev/null\n+++ b/test-sha1-array.c\n@@ -0,0 +1,34 @@\n+#include \"cache.h\"\n+#include \"sha1-array.h\"\n+\n+static void print_sha1(const unsigned char sha1[20], void *data)\n+{\n+\tputs(sha1_to_hex(sha1));\n+}\n+\n+int main(int argc, char **argv)\n+{\n+\tstruct sha1_array array = SHA1_ARRAY_INIT;\n+\tstruct strbuf line = STRBUF_INIT;\n+\n+\twhile (strbuf_getline(&line, stdin, '\\n') != EOF) {\n+\t\tconst char *arg;\n+\t\tunsigned char sha1[20];\n+\n+\t\tif (skip_prefix(line.buf, \"append \", &arg)) {\n+\t\t\tif (get_sha1_hex(arg, sha1))\n+\t\t\t\tdie(\"not a hexadecimal SHA1: %s\", arg);\n+\t\t\tsha1_array_append(&array, sha1);\n+\t\t} else if (skip_prefix(line.buf, \"lookup \", &arg)) {\n+\t\t\tif (get_sha1_hex(arg, sha1))\n+\t\t\t\tdie(\"not a hexadecimal SHA1: %s\", arg);\n+\t\t\tprintf(\"%d\\n\", sha1_array_lookup(&array, sha1));\n+\t\t} else if (!strcmp(line.buf, \"clear\"))\n+\t\t\tsha1_array_clear(&array);\n+\t\telse if (!strcmp(line.buf, \"for_each_unique\"))\n+\t\t\tsha1_array_for_each_unique(&array, print_sha1, NULL);\n+\t\telse\n+\t\t\tdie(\"unknown command: %s\", line.buf);\n+\t}\n+\treturn 0;\n+}\n-- \n2.1.2\n"},{"id":"250060","messageId":"542BCCB9.4050908@web.de","threadId":"37655","inReplyTo":"542BCBFC.5000509@web.de","subject":"[PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-01T09:43:21Z","receivedAt":"2014-10-01T09:43:21Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"If the first 18 bytes of the SHA1's of all entries are the same then\nsha1_pos() dies and reports that the lower and upper limits of the\nbinary search were the same that this wasn't supposed to happen.  This\nis wrong because the remaining two bytes could still differ.\n\nFurthermore: It wouldn't be a problem if they actually were the same,\ni.e. if all entries have the same SHA1.  The code already handles\nduplicates just fine otherwise.  Simply remove the erroneous check.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n sha1-lookup.c         |  2 --\n t/t0064-sha1-array.sh | 20 ++++++++++++++++++++\n 2 files changed, 20 insertions(+), 2 deletions(-)\n\ndiff --git a/sha1-lookup.c b/sha1-lookup.c\nindex 2dd8515..5f06921 100644\n--- a/sha1-lookup.c\n+++ b/sha1-lookup.c\n@@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,\n \t\t\t\tdie(\"BUG: assertion failed in binary search\");\n \t\t\t}\n \t\t}\n-\t\tif (18 <= ofs)\n-\t\t\tdie(\"cannot happen -- lo and hi are identical\");\n \t}\n \n \tdo {\ndiff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\nindex bd68789..3fcb8d8 100755\n--- a/t/t0064-sha1-array.sh\n+++ b/t/t0064-sha1-array.sh\n@@ -61,4 +61,24 @@ test_expect_success 'lookup with duplicates' '\n \ttest \"$n\" -le 3\n '\n \n+test_expect_success 'lookup with almost duplicate values' '\n+\t{\n+\t\techo \"append 5555555555555555555555555555555555555555\" &&\n+\t\techo \"append 555555555555555555555555555555555555555f\" &&\n+\t\techo20 \"lookup \" 55\n+\t} | test-sha1-array >actual &&\n+\tn=$(cat actual) &&\n+\ttest \"$n\" -eq 0\n+'\n+\n+test_expect_success 'lookup with single duplicate value' '\n+\t{\n+\t\techo20 \"append \" 55 55 &&\n+\t\techo20 \"lookup \" 55\n+\t} | test-sha1-array >actual &&\n+\tn=$(cat actual) &&\n+\ttest \"$n\" -ge 0 &&\n+\ttest \"$n\" -le 1\n+'\n+\n test_done\n-- \n2.1.2\n"},{"id":"250103","messageId":"20141001105006.GB10332@peff.net","threadId":"37655","inReplyTo":"542BCCB9.4050908@web.de","subject":"Re: [PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-01T10:50:07Z","receivedAt":"2014-10-01T10:50:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 01, 2014 at 11:43:21AM +0200, René Scharfe wrote:\n\n> If the first 18 bytes of the SHA1's of all entries are the same then\n> sha1_pos() dies and reports that the lower and upper limits of the\n> binary search were the same that this wasn't supposed to happen.  This\n> is wrong because the remaining two bytes could still differ.\n> \n> Furthermore: It wouldn't be a problem if they actually were the same,\n> i.e. if all entries have the same SHA1.  The code already handles\n> duplicates just fine otherwise.  Simply remove the erroneous check.\n\nYeah, I agree that assertion is just wrong.\n\nRegarding duplicates: in sha1_entry_pos, we had to handle the \"not\nfound\" case specially, because we may have found the left-hand or\nright-hand side of a run of duplicates, and we want to return the\ncorrect slot where the new item would go (see the comment added by\n171bdac). I think we don't have to deal with that here, because we are\njust dealing with the initial \"mi\" selection. The actual binary search\nis plain-vanilla, which handles that case just fine.\n\nI wonder if it is worth adding a test (you test only that \"not found\"\nproduces a negative index, but not which index). Like:\n\ndiff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\nindex 3fcb8d8..7781129 100755\n--- a/t/t0064-sha1-array.sh\n+++ b/t/t0064-sha1-array.sh\n@@ -42,12 +42,12 @@ test_expect_success 'lookup' '\n '\n \n test_expect_success 'lookup non-existing entry' '\n+\techo -1 >expect &&\n \t{\n \t\techo20 \"append \" 88 44 aa 55 &&\n \t\techo20 \"lookup \" 33\n \t} | test-sha1-array >actual &&\n-\tn=$(cat actual) &&\n-\ttest \"$n\" -lt 0\n+\ttest_cmp expect actual\n '\n \n test_expect_success 'lookup with duplicates' '\n@@ -61,6 +61,17 @@ test_expect_success 'lookup with duplicates' '\n \ttest \"$n\" -le 3\n '\n \n+test_expect_success 'lookup non-existing entry with duplicates' '\n+\techo -5 >expect &&\n+\t{\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"append \" 88 44 aa 55 &&\n+\t\techo20 \"lookup \" 66\n+\t} | test-sha1-array >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+\n test_expect_success 'lookup with almost duplicate values' '\n \t{\n \t\techo \"append 5555555555555555555555555555555555555555\" &&\n"},{"id":"250104","messageId":"542BE114.9070300@web.de","threadId":"37655","inReplyTo":"20141001105006.GB10332@peff.net","subject":"Re: [PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-01T11:10:12Z","receivedAt":"2014-10-01T11:10:12Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.10.2014 um 12:50 schrieb Jeff King:\n> On Wed, Oct 01, 2014 at 11:43:21AM +0200, René Scharfe wrote:\n>\n>> If the first 18 bytes of the SHA1's of all entries are the same then\n>> sha1_pos() dies and reports that the lower and upper limits of the\n>> binary search were the same that this wasn't supposed to happen.  This\n>> is wrong because the remaining two bytes could still differ.\n>>\n>> Furthermore: It wouldn't be a problem if they actually were the same,\n>> i.e. if all entries have the same SHA1.  The code already handles\n>> duplicates just fine otherwise.  Simply remove the erroneous check.\n>\n> Yeah, I agree that assertion is just wrong.\n>\n> Regarding duplicates: in sha1_entry_pos, we had to handle the \"not\n> found\" case specially, because we may have found the left-hand or\n> right-hand side of a run of duplicates, and we want to return the\n> correct slot where the new item would go (see the comment added by\n> 171bdac). I think we don't have to deal with that here, because we are\n> just dealing with the initial \"mi\" selection. The actual binary search\n> is plain-vanilla, which handles that case just fine.\n>\n> I wonder if it is worth adding a test (you test only that \"not found\"\n> produces a negative index, but not which index). Like:\n\napi-sha1-array.txt says about sha1_array_lookup: \"If not found, returns \na negative integer\", and that's what the test checks.\n\nI actually like that the value is not specified for that case because no \nexisting caller actually uses it and it leaves room to implement the \nfunction e.g. using bsearch(3).\n\nI agree that adding a \"lookup non-existing entry with duplicates\" test \nwould make t0064 more complete, though.\n\n> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\n> index 3fcb8d8..7781129 100755\n> --- a/t/t0064-sha1-array.sh\n> +++ b/t/t0064-sha1-array.sh\n> @@ -42,12 +42,12 @@ test_expect_success 'lookup' '\n>   '\n>\n>   test_expect_success 'lookup non-existing entry' '\n> +\techo -1 >expect &&\n>   \t{\n>   \t\techo20 \"append \" 88 44 aa 55 &&\n>   \t\techo20 \"lookup \" 33\n>   \t} | test-sha1-array >actual &&\n> -\tn=$(cat actual) &&\n> -\ttest \"$n\" -lt 0\n> +\ttest_cmp expect actual\n>   '\n>\n>   test_expect_success 'lookup with duplicates' '\n> @@ -61,6 +61,17 @@ test_expect_success 'lookup with duplicates' '\n>   \ttest \"$n\" -le 3\n>   '\n>\n> +test_expect_success 'lookup non-existing entry with duplicates' '\n> +\techo -5 >expect &&\n> +\t{\n> +\t\techo20 \"append \" 88 44 aa 55 &&\n> +\t\techo20 \"append \" 88 44 aa 55 &&\n> +\t\techo20 \"lookup \" 66\n> +\t} | test-sha1-array >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> +\n>   test_expect_success 'lookup with almost duplicate values' '\n>   \t{\n>   \t\techo \"append 5555555555555555555555555555555555555555\" &&\n>\n"},{"id":"250111","messageId":"20141001123343.GA10772@peff.net","threadId":"37655","inReplyTo":"542BE114.9070300@web.de","subject":"Re: [PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-01T12:33:44Z","receivedAt":"2014-10-01T12:33:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 01, 2014 at 01:10:12PM +0200, René Scharfe wrote:\n\n> >I wonder if it is worth adding a test (you test only that \"not found\"\n> >produces a negative index, but not which index). Like:\n> \n> api-sha1-array.txt says about sha1_array_lookup: \"If not found, returns a\n> negative integer\", and that's what the test checks.\n\nHmm. I do not recall intentionally leaving the value unspecified; I\nthink it is more that I was simply not thorough when writing the\ndocumentation. That being said...\n\n> I actually like that the value is not specified for that case because no\n> existing caller actually uses it and it leaves room to implement the\n> function e.g. using bsearch(3).\n\nYeah, if no callers actually care right now, that is a reasonable\nargument for leaving the exact return value unspecified (and testing\nonly what the documentation claims).\n\n> I agree that adding a \"lookup non-existing entry with duplicates\" test would\n> make t0064 more complete, though.\n\nAgreed.\n\n-Peff\n"},{"id":"250114","messageId":"CAPig+cQxXccnQCwr7oVfccAQn3sTUpv=b=qHEX1H7abng--=0A@mail.gmail.com","threadId":"37655","inReplyTo":"542BCBFC.5000509@web.de","subject":"Re: [PATCH 1/2] sha1-array: add test-sha1-array and basic tests","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-10-01T14:11:04Z","receivedAt":"2014-10-01T14:11:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 1, 2014 at 5:40 AM, René Scharfe <l.s.r@web.de> wrote:\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\n> new file mode 100755\n> index 0000000..bd68789\n> --- /dev/null\n> +++ b/t/t0064-sha1-array.sh\n> @@ -0,0 +1,64 @@\n> +#!/bin/sh\n> +\n> +test_description='basic tests for the SHA1 array implementation'\n> +. ./test-lib.sh\n> +\n> +echo20() {\n> +       prefix=\"$1\"\n> +       shift\n> +       while test $# -gt 0\n> +       do\n> +               echo \"$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n\nEach caller of echo20() manually includes a space at the end of\n$prefix. Would it make sense to instead have echo20() do this on\nbehalf of the caller?\n\n    echo \"$prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n\n> +               shift\n> +       done\n> +}\n> +\n> +test_expect_success 'ordered enumeration' '\n> +       echo20 \"\" 44 55 88 aa >expect &&\n> +       {\n> +               echo20 \"append \" 88 44 aa 55 &&\n\nWhich would slightly reduce the burden on the caller and make it read\n(very slightly) nicer:\n\n    echo20 append 88 44 aa 55 &&\n\n> +               echo for_each_unique\n> +       } | test-sha1-array >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'ordered enumeration with duplicate suppression' '\n> +       echo20 \"\" 44 55 88 aa >expect &&\n> +       {\n> +               echo20 \"append \" 88 44 aa 55 &&\n> +               echo20 \"append \" 88 44 aa 55 &&\n> +               echo for_each_unique\n> +       } | test-sha1-array >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n> +test_expect_success 'lookup' '\n> +       {\n> +               echo20 \"append \" 88 44 aa 55 &&\n> +               echo20 \"lookup \" 55\n> +       } | test-sha1-array >actual &&\n> +       n=$(cat actual) &&\n> +       test \"$n\" -eq 1\n> +'\n> +\n> +test_expect_success 'lookup non-existing entry' '\n> +       {\n> +               echo20 \"append \" 88 44 aa 55 &&\n> +               echo20 \"lookup \" 33\n> +       } | test-sha1-array >actual &&\n> +       n=$(cat actual) &&\n> +       test \"$n\" -lt 0\n> +'\n> +\n> +test_expect_success 'lookup with duplicates' '\n> +       {\n> +               echo20 \"append \" 88 44 aa 55 &&\n> +               echo20 \"append \" 88 44 aa 55 &&\n> +               echo20 \"lookup \" 55\n> +       } | test-sha1-array >actual &&\n> +       n=$(cat actual) &&\n> +       test \"$n\" -ge 2 &&\n> +       test \"$n\" -le 3\n> +'\n> +\n> +test_done\n"},{"id":"250115","messageId":"CAPig+cQADVFECOev+ZW9W94=Xy5nHVd4je8kuTzS53MjhxaVEw@mail.gmail.com","threadId":"37655","inReplyTo":"542BCCB9.4050908@web.de","subject":"Re: [PATCH 2/2] sha1-lookup: fix handling of duplicates in sha1_pos()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-10-01T14:12:58Z","receivedAt":"2014-10-01T14:12:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Oct 1, 2014 at 5:43 AM, René Scharfe <l.s.r@web.de> wrote:\n> If the first 18 bytes of the SHA1's of all entries are the same then\n> sha1_pos() dies and reports that the lower and upper limits of the\n> binary search were the same that this wasn't supposed to happen.  This\n> is wrong because the remaining two bytes could still differ.\n>\n> Furthermore: It wouldn't be a problem if they actually were the same,\n> i.e. if all entries have the same SHA1.  The code already handles\n> duplicates just fine otherwise.  Simply remove the erroneous check.\n>\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  sha1-lookup.c         |  2 --\n>  t/t0064-sha1-array.sh | 20 ++++++++++++++++++++\n>  2 files changed, 20 insertions(+), 2 deletions(-)\n>\n> diff --git a/sha1-lookup.c b/sha1-lookup.c\n> index 2dd8515..5f06921 100644\n> --- a/sha1-lookup.c\n> +++ b/sha1-lookup.c\n> @@ -84,8 +84,6 @@ int sha1_pos(const unsigned char *sha1, void *table, size_t nr,\n>                                 die(\"BUG: assertion failed in binary search\");\n>                         }\n>                 }\n> -               if (18 <= ofs)\n> -                       die(\"cannot happen -- lo and hi are identical\");\n>         }\n>\n>         do {\n> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\n> index bd68789..3fcb8d8 100755\n> --- a/t/t0064-sha1-array.sh\n> +++ b/t/t0064-sha1-array.sh\n> @@ -61,4 +61,24 @@ test_expect_success 'lookup with duplicates' '\n>         test \"$n\" -le 3\n>  '\n>\n> +test_expect_success 'lookup with almost duplicate values' '\n> +       {\n> +               echo \"append 5555555555555555555555555555555555555555\" &&\n> +               echo \"append 555555555555555555555555555555555555555f\" &&\n> +               echo20 \"lookup \" 55\n> +       } | test-sha1-array >actual &&\n> +       n=$(cat actual) &&\n> +       test \"$n\" -eq 0\n> +'\n> +\n> +test_expect_success 'lookup with single duplicate value' '\n> +       {\n> +               echo20 \"append \" 55 55 &&\n> +               echo20 \"lookup \" 55\n> +       } | test-sha1-array >actual &&\n> +       n=$(cat actual) &&\n> +       test \"$n\" -ge 0 &&\n> +       test \"$n\" -le 1\n> +'\n\nAn alternative would be to introduce these two tests in patch 1/2 as\ntest_expect_failure and flip them to test_expect_success in this patch\nwhich fixes the problem.\n\n> +\n>  test_done\n> --\n> 2.1.2\n"},{"id":"250116","messageId":"542C0DE2.6060502@web.de","threadId":"37655","inReplyTo":"CAPig+cQxXccnQCwr7oVfccAQn3sTUpv=b=qHEX1H7abng--=0A@mail.gmail.com","subject":"Re: [PATCH 1/2] sha1-array: add test-sha1-array and basic tests","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2014-10-01T14:21:22Z","receivedAt":"2014-10-01T14:21:22Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.10.2014 um 16:11 schrieb Eric Sunshine:\n> On Wed, Oct 1, 2014 at 5:40 AM, René Scharfe <l.s.r@web.de> wrote:\n>> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n>> ---\n>> diff --git a/t/t0064-sha1-array.sh b/t/t0064-sha1-array.sh\n>> new file mode 100755\n>> index 0000000..bd68789\n>> --- /dev/null\n>> +++ b/t/t0064-sha1-array.sh\n>> @@ -0,0 +1,64 @@\n>> +#!/bin/sh\n>> +\n>> +test_description='basic tests for the SHA1 array implementation'\n>> +. ./test-lib.sh\n>> +\n>> +echo20() {\n>> +       prefix=\"$1\"\n>> +       shift\n>> +       while test $# -gt 0\n>> +       do\n>> +               echo \"$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n>\n> Each caller of echo20() manually includes a space at the end of\n> $prefix. Would it make sense to instead have echo20() do this on\n> behalf of the caller?\n>\n>      echo \"$prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n\nThat wouldn't work if the prefix is the empty string; we don't want a \nspace in that case (see the next echo20 call below).\n\nBut ${prefix:+$prefix } would do the trick.  Thanks for the idea. :)\n\n>\n>> +               shift\n>> +       done\n>> +}\n>> +\n>> +test_expect_success 'ordered enumeration' '\n>> +       echo20 \"\" 44 55 88 aa >expect &&\n>> +       {\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>\n> Which would slightly reduce the burden on the caller and make it read\n> (very slightly) nicer:\n>\n>      echo20 append 88 44 aa 55 &&\n>\n>> +               echo for_each_unique\n>> +       } | test-sha1-array >actual &&\n>> +       test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'ordered enumeration with duplicate suppression' '\n>> +       echo20 \"\" 44 55 88 aa >expect &&\n>> +       {\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>> +               echo for_each_unique\n>> +       } | test-sha1-array >actual &&\n>> +       test_cmp expect actual\n>> +'\n>> +\n>> +test_expect_success 'lookup' '\n>> +       {\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>> +               echo20 \"lookup \" 55\n>> +       } | test-sha1-array >actual &&\n>> +       n=$(cat actual) &&\n>> +       test \"$n\" -eq 1\n>> +'\n>> +\n>> +test_expect_success 'lookup non-existing entry' '\n>> +       {\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>> +               echo20 \"lookup \" 33\n>> +       } | test-sha1-array >actual &&\n>> +       n=$(cat actual) &&\n>> +       test \"$n\" -lt 0\n>> +'\n>> +\n>> +test_expect_success 'lookup with duplicates' '\n>> +       {\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>> +               echo20 \"append \" 88 44 aa 55 &&\n>> +               echo20 \"lookup \" 55\n>> +       } | test-sha1-array >actual &&\n>> +       n=$(cat actual) &&\n>> +       test \"$n\" -ge 2 &&\n>> +       test \"$n\" -le 3\n>> +'\n>> +\n>> +test_done\n"},{"id":"250117","messageId":"20141001142336.GA19992@peff.net","threadId":"37655","inReplyTo":"CAPig+cQxXccnQCwr7oVfccAQn3sTUpv=b=qHEX1H7abng--=0A@mail.gmail.com","subject":"Re: [PATCH 1/2] sha1-array: add test-sha1-array and basic tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-10-01T14:23:36Z","receivedAt":"2014-10-01T14:23:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 01, 2014 at 10:11:04AM -0400, Eric Sunshine wrote:\n\n> > +echo20() {\n> > +       prefix=\"$1\"\n> > +       shift\n> > +       while test $# -gt 0\n> > +       do\n> > +               echo \"$prefix$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n> \n> Each caller of echo20() manually includes a space at the end of\n> $prefix. Would it make sense to instead have echo20() do this on\n> behalf of the caller?\n> \n>     echo \"$prefix $1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1$1\"\n\nNot always. For example:\n\n> > +test_expect_success 'ordered enumeration' '\n> > +       echo20 \"\" 44 55 88 aa >expect &&\n\nThis does not.\n\n> > +       {\n> > +               echo20 \"append \" 88 44 aa 55 &&\n> \n> Which would slightly reduce the burden on the caller and make it read\n> (very slightly) nicer:\n> \n>     echo20 append 88 44 aa 55 &&\n\nI agree that is more readable. But you'd have to make echo20 more like:\n\n  if test -n \"$1\"; then\n          prefix=\"$1 \"\n  else\n          prefix=\"\"\n  fi\n\nwhich is not too bad.\n\n-Peff\n"}]}