{"thread":{"id":"51272","subject":"[PATCH 0/3] Test oidmap","startedAt":"2019-06-09T04:49:33Z","lastAt":"2019-06-14T10:36:18Z","messageCount":14,"participants":["Christian Couder","SZEDER Gábor","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"376871","messageId":"20190609044907.32477-1-chriscool@tuxfamily.org","threadId":"51272","inReplyTo":null,"subject":"[PATCH 0/3] Test oidmap","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-09T04:49:04Z","receivedAt":"2019-06-09T04:49:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nUnlike hashmap that has t/helper/test-hashmap.c and t/t0011-hashmap.sh\noidmap has no specific test. The goal of this small patch series is to\nchange that and also improve oidmap a bit while at it.\n\nChristian Couder (3):\n  t/helper: add test-oidmap.c\n  t: add t0016-oidmap.sh\n  oidmap: use sha1hash() instead of static hash() function\n\n Makefile               |   1 +\n oidmap.c               |  13 +---\n t/helper/test-oidmap.c | 134 +++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c   |   1 +\n t/helper/test-tool.h   |   1 +\n t/t0016-oidmap.sh      | 100 ++++++++++++++++++++++++++++++\n 6 files changed, 240 insertions(+), 10 deletions(-)\n create mode 100644 t/helper/test-oidmap.c\n create mode 100755 t/t0016-oidmap.sh\n\n-- \n2.22.0.14.g9023ccb50a\n\n"},{"id":"376872","messageId":"20190609044907.32477-2-chriscool@tuxfamily.org","threadId":"51272","inReplyTo":"20190609044907.32477-1-chriscool@tuxfamily.org","subject":"[PATCH 1/3] t/helper: add test-oidmap.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-09T04:49:05Z","receivedAt":"2019-06-09T04:49:33Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"This new helper is very similar to \"test-hashmap.c\" and will help\ntest how `struct oidmap` from oidmap.{c,h} can be used.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n Makefile               |   1 +\n t/helper/test-oidmap.c | 134 +++++++++++++++++++++++++++++++++++++++++\n t/helper/test-tool.c   |   1 +\n t/helper/test-tool.h   |   1 +\n 4 files changed, 137 insertions(+)\n create mode 100644 t/helper/test-oidmap.c\n\ndiff --git a/Makefile b/Makefile\nindex 8a7e235352..5efc7700ed 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -727,6 +727,7 @@ TEST_BUILTINS_OBJS += test-lazy-init-name-hash.o\n TEST_BUILTINS_OBJS += test-match-trees.o\n TEST_BUILTINS_OBJS += test-mergesort.o\n TEST_BUILTINS_OBJS += test-mktemp.o\n+TEST_BUILTINS_OBJS += test-oidmap.o\n TEST_BUILTINS_OBJS += test-online-cpus.o\n TEST_BUILTINS_OBJS += test-parse-options.o\n TEST_BUILTINS_OBJS += test-path-utils.o\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nnew file mode 100644\nindex 0000000000..0ba122a264\n--- /dev/null\n+++ b/t/helper/test-oidmap.c\n@@ -0,0 +1,134 @@\n+#include \"test-tool.h\"\n+#include \"cache.h\"\n+#include \"oidmap.h\"\n+#include \"strbuf.h\"\n+\n+/* key is an oid and value is a name (could be a refname for example) */\n+struct test_entry {\n+\tstruct oidmap_entry entry;\n+\tchar name[FLEX_ARRAY];\n+};\n+\n+#define DELIM \" \\t\\r\\n\"\n+\n+/*\n+ * Read stdin line by line and print result of commands to stdout:\n+ *\n+ * hash oidkey -> sha1hash(oidkey)\n+ * put oidkey namevalue -> NULL / old namevalue\n+ * get oidkey -> NULL / namevalue\n+ * remove oidkey -> NULL / old namevalue\n+ * iterate -> oidkey1 namevalue1\\noidkey2 namevalue2\\n...\n+ *\n+ */\n+int cmd__oidmap(int argc, const char **argv)\n+{\n+\tstruct strbuf line = STRBUF_INIT;\n+\tstruct oidmap map = OIDMAP_INIT;\n+\n+\tsetup_git_directory();\n+\n+\t/* init oidmap */\n+\toidmap_init(&map, 0);\n+\n+\t/* process commands from stdin */\n+\twhile (strbuf_getline(&line, stdin) != EOF) {\n+\t\tchar *cmd, *p1 = NULL, *p2 = NULL;\n+\t\tstruct test_entry *entry;\n+\t\tstruct object_id oid;\n+\n+\t\t/* break line into command and up to two parameters */\n+\t\tcmd = strtok(line.buf, DELIM);\n+\t\t/* ignore empty lines */\n+\t\tif (!cmd || *cmd == '#')\n+\t\t\tcontinue;\n+\n+\t\tp1 = strtok(NULL, DELIM);\n+\t\tif (p1)\n+\t\t\tp2 = strtok(NULL, DELIM);\n+\n+\t\tif (!strcmp(\"hash\", cmd) && p1) {\n+\n+\t\t\t/* print hash of oid */\n+\t\t\tif (!get_oid(p1, &oid))\n+\t\t\t\tprintf(\"%u\\n\", sha1hash(oid.hash));\n+\t\t\telse\n+\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n+\n+\t\t} else if (!strcmp(\"add\", cmd) && p1 && p2) {\n+\n+\t\t\tif (get_oid(p1, &oid)) {\n+\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\t/* create entry with oidkey from p1, value = p2 */\n+\t\t\tFLEX_ALLOC_STR(entry, name, p2);\n+\t\t\toidcpy(&entry->entry.oid, &oid);\n+\n+\t\t\t/* add to oidmap */\n+\t\t\toidmap_put(&map, entry);\n+\n+\t\t} else if (!strcmp(\"put\", cmd) && p1 && p2) {\n+\n+\t\t\tif (get_oid(p1, &oid)) {\n+\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\t/* create entry with oid_key = p1, name_value = p2 */\n+\t\t\tFLEX_ALLOC_STR(entry, name, p2);\n+\t\t\toidcpy(&entry->entry.oid, &oid);\n+\n+\t\t\t/* add / replace entry */\n+\t\t\tentry = oidmap_put(&map, entry);\n+\n+\t\t\t/* print and free replaced entry, if any */\n+\t\t\tputs(entry ? entry->name : \"NULL\");\n+\t\t\tfree(entry);\n+\n+\t\t} else if (!strcmp(\"get\", cmd) && p1) {\n+\n+\t\t\tif (get_oid(p1, &oid)) {\n+\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\t/* lookup entry in oidmap */\n+\t\t\tentry = oidmap_get(&map, &oid);\n+\n+\t\t\t/* print result */\n+\t\t\tputs(entry ? entry->name : \"NULL\");\n+\n+\t\t} else if (!strcmp(\"remove\", cmd) && p1) {\n+\n+\t\t\tif (get_oid(p1, &oid)) {\n+\t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\n+\t\t\t/* remove entry from oidmap */\n+\t\t\tentry = oidmap_remove(&map, &oid);\n+\n+\t\t\t/* print result and free entry*/\n+\t\t\tputs(entry ? entry->name : \"NULL\");\n+\t\t\tfree(entry);\n+\n+\t\t} else if (!strcmp(\"iterate\", cmd)) {\n+\n+\t\t\tstruct oidmap_iter iter;\n+\t\t\toidmap_iter_init(&map, &iter);\n+\t\t\twhile ((entry = oidmap_iter_next(&iter)))\n+\t\t\t\tprintf(\"%s %s\\n\", oid_to_hex(&entry->entry.oid), entry->name);\n+\n+\t\t} else {\n+\n+\t\t\tprintf(\"Unknown command %s\\n\", cmd);\n+\n+\t\t}\n+\t}\n+\n+\tstrbuf_release(&line);\n+\toidmap_free(&map, 1);\n+\treturn 0;\n+}\ndiff --git a/t/helper/test-tool.c b/t/helper/test-tool.c\nindex 087a8c0cc9..1eac25233f 100644\n--- a/t/helper/test-tool.c\n+++ b/t/helper/test-tool.c\n@@ -35,6 +35,7 @@ static struct test_cmd cmds[] = {\n \t{ \"match-trees\", cmd__match_trees },\n \t{ \"mergesort\", cmd__mergesort },\n \t{ \"mktemp\", cmd__mktemp },\n+\t{ \"oidmap\", cmd__oidmap },\n \t{ \"online-cpus\", cmd__online_cpus },\n \t{ \"parse-options\", cmd__parse_options },\n \t{ \"path-utils\", cmd__path_utils },\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 7e703f3038..c7a46dc320 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -25,6 +25,7 @@ int cmd__lazy_init_name_hash(int argc, const char **argv);\n int cmd__match_trees(int argc, const char **argv);\n int cmd__mergesort(int argc, const char **argv);\n int cmd__mktemp(int argc, const char **argv);\n+int cmd__oidmap(int argc, const char **argv);\n int cmd__online_cpus(int argc, const char **argv);\n int cmd__parse_options(int argc, const char **argv);\n int cmd__path_utils(int argc, const char **argv);\n-- \n2.22.0.14.g9023ccb50a\n\n"},{"id":"376873","messageId":"20190609044907.32477-3-chriscool@tuxfamily.org","threadId":"51272","inReplyTo":"20190609044907.32477-1-chriscool@tuxfamily.org","subject":"[PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-09T04:49:06Z","receivedAt":"2019-06-09T04:49:36Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nAdd actual tests for operations using `struct oidmap` from oidmap.{c,h}.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n t/t0016-oidmap.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 100 insertions(+)\n create mode 100755 t/t0016-oidmap.sh\n\ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\nnew file mode 100755\nindex 0000000000..3a8e8bdb3d\n--- /dev/null\n+++ b/t/t0016-oidmap.sh\n@@ -0,0 +1,100 @@\n+#!/bin/sh\n+\n+test_description='test oidmap'\n+. ./test-lib.sh\n+\n+# This purposefully is very similar to t0011-hashmap.sh\n+\n+test_oidmap() {\n+\techo \"$1\" | test-tool oidmap $3 > actual &&\n+\techo \"$2\" > expect &&\n+\ttest_cmp expect actual\n+}\n+\n+\n+test_expect_success 'setup' '\n+\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three &&\n+\ttest_commit four\n+\n+'\n+\n+test_oidhash() {\n+\tgit rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n+}\n+\n+test_expect_success PERL 'hash' '\n+\n+test_oidmap \"hash one\n+hash two\n+hash invalidOid\n+hash three\" \"$(test_oidhash one)\n+$(test_oidhash two)\n+Unknown oid: invalidOid\n+$(test_oidhash three)\"\n+\n+'\n+\n+test_expect_success 'put' '\n+\n+test_oidmap \"put one 1\n+put two 2\n+put invalidOid 4\n+put three 3\" \"NULL\n+NULL\n+Unknown oid: invalidOid\n+NULL\"\n+\n+'\n+\n+test_expect_success 'replace' '\n+\n+test_oidmap \"put one 1\n+put two 2\n+put three 3\n+put invalidOid 4\n+put two deux\n+put one un\" \"NULL\n+NULL\n+NULL\n+Unknown oid: invalidOid\n+2\n+1\"\n+\n+'\n+\n+test_expect_success 'get' '\n+\n+test_oidmap \"put one 1\n+put two 2\n+put three 3\n+get two\n+get four\n+get invalidOid\n+get one\" \"NULL\n+NULL\n+NULL\n+2\n+NULL\n+Unknown oid: invalidOid\n+1\"\n+\n+'\n+\n+test_expect_success 'iterate' '\n+\n+test_oidmap \"put one 1\n+put two 2\n+put three 3\n+iterate\" \"NULL\n+NULL\n+NULL\n+$(git rev-parse two) 2\n+$(git rev-parse one) 1\n+$(git rev-parse three) 3\"\n+\n+'\n+\n+test_done\n-- \n2.22.0.14.g9023ccb50a\n\n"},{"id":"376874","messageId":"20190609044907.32477-4-chriscool@tuxfamily.org","threadId":"51272","inReplyTo":"20190609044907.32477-1-chriscool@tuxfamily.org","subject":"[PATCH 3/3] oidmap: use sha1hash() instead of static hash() function","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-09T04:49:07Z","receivedAt":"2019-06-09T04:49:45Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"From: Christian Couder <christian.couder@gmail.com>\n\nGet rid of the static hash() function in oidmap.c which is redundant\nwith sha1hash(). Use sha1hash() directly instead.\n\nLet's be more consistent and not use several hash functions doing\nnearly exactly the same thing.\n\nSigned-off-by: Christian Couder <chriscool@tuxfamily.org>\n---\n oidmap.c | 13 +++----------\n 1 file changed, 3 insertions(+), 10 deletions(-)\n\ndiff --git a/oidmap.c b/oidmap.c\nindex b0841a0f58..01c206aaef 100644\n--- a/oidmap.c\n+++ b/oidmap.c\n@@ -12,13 +12,6 @@ static int oidmap_neq(const void *hashmap_cmp_fn_data,\n \t\t      &((const struct oidmap_entry *) entry_or_key)->oid);\n }\n \n-static int hash(const struct object_id *oid)\n-{\n-\tint hash;\n-\tmemcpy(&hash, oid->hash, sizeof(hash));\n-\treturn hash;\n-}\n-\n void oidmap_init(struct oidmap *map, size_t initial_size)\n {\n \thashmap_init(&map->map, oidmap_neq, NULL, initial_size);\n@@ -36,7 +29,7 @@ void *oidmap_get(const struct oidmap *map, const struct object_id *key)\n \tif (!map->map.cmpfn)\n \t\treturn NULL;\n \n-\treturn hashmap_get_from_hash(&map->map, hash(key), key);\n+\treturn hashmap_get_from_hash(&map->map, sha1hash(key->hash), key);\n }\n \n void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n@@ -46,7 +39,7 @@ void *oidmap_remove(struct oidmap *map, const struct object_id *key)\n \tif (!map->map.cmpfn)\n \t\toidmap_init(map, 0);\n \n-\thashmap_entry_init(&entry, hash(key));\n+\thashmap_entry_init(&entry, sha1hash(key->hash));\n \treturn hashmap_remove(&map->map, &entry, key);\n }\n \n@@ -57,6 +50,6 @@ void *oidmap_put(struct oidmap *map, void *entry)\n \tif (!map->map.cmpfn)\n \t\toidmap_init(map, 0);\n \n-\thashmap_entry_init(&to_put->internal_entry, hash(&to_put->oid));\n+\thashmap_entry_init(&to_put->internal_entry, sha1hash(to_put->oid.hash));\n \treturn hashmap_put(&map->map, to_put);\n }\n-- \n2.22.0.14.g9023ccb50a\n\n"},{"id":"376879","messageId":"20190609092259.GB24208@szeder.dev","threadId":"51272","inReplyTo":"20190609044907.32477-3-chriscool@tuxfamily.org","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-09T09:22:59Z","receivedAt":"2019-06-09T09:23:06Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Jun 09, 2019 at 06:49:06AM +0200, Christian Couder wrote:\n> From: Christian Couder <christian.couder@gmail.com>\n> \n> Add actual tests for operations using `struct oidmap` from oidmap.{c,h}.\n> \n> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>\n> ---\n>  t/t0016-oidmap.sh | 100 ++++++++++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 100 insertions(+)\n>  create mode 100755 t/t0016-oidmap.sh\n> \n> diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\n> new file mode 100755\n> index 0000000000..3a8e8bdb3d\n> --- /dev/null\n> +++ b/t/t0016-oidmap.sh\n> @@ -0,0 +1,100 @@\n> +#!/bin/sh\n> +\n> +test_description='test oidmap'\n> +. ./test-lib.sh\n> +\n> +# This purposefully is very similar to t0011-hashmap.sh\n> +\n> +test_oidmap() {\n> +\techo \"$1\" | test-tool oidmap $3 > actual &&\n> +\techo \"$2\" > expect &&\n\nStyle nit: space between redirection op and filename.\n\n> +\ttest_cmp expect actual\n> +}\n> +\n> +\n> +test_expect_success 'setup' '\n> +\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three &&\n> +\ttest_commit four\n> +\n> +'\n> +\n> +test_oidhash() {\n> +\tgit rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n\nNew Perl dependencies always make Dscho sad... :)\n\nSo, 'test oidmap' from the previous patch prints the value we want to\ncheck with:\n\n    printf(\"%u\\n\", sha1hash(oid.hash));\n\nFirst, since object ids inherently make more sense as hex values, it\nwould be more appropriate to print that hash with the '%x' format\nspecifier, and then we wouldn't need Perl's hex() anymore, and thus\ncould swap the order of the first four bytes in oidmap's hash without\nrelying on Perl, e.g. with:\n\n  sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n\nSecond, and more importantly, the need for swapping the byte order\nindicates that this test would fail on big-endian systems, I'm afraid.\nSo I think we need an additional bswap32() on the printing side, and\nthen could further simplify 'test_oidhash':\n\n\ndiff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\nindex 0ba122a264..4177912f9a 100644\n--- a/t/helper/test-oidmap.c\n+++ b/t/helper/test-oidmap.c\n@@ -51,7 +51,7 @@ int cmd__oidmap(int argc, const char **argv)\n \n \t\t\t/* print hash of oid */\n \t\t\tif (!get_oid(p1, &oid))\n-\t\t\t\tprintf(\"%u\\n\", sha1hash(oid.hash));\n+\t\t\t\tprintf(\"%x\\n\", bswap32(sha1hash(oid.hash)));\n \t\t\telse\n \t\t\t\tprintf(\"Unknown oid: %s\\n\", p1);\n \ndiff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\nindex 3a8e8bdb3d..9c0d88a316 100755\n--- a/t/t0016-oidmap.sh\n+++ b/t/t0016-oidmap.sh\n@@ -22,10 +22,10 @@ test_expect_success 'setup' '\n '\n \n test_oidhash() {\n-\tgit rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n+\tgit rev-parse \"$1\" | cut -c1-8\n }\n \n-test_expect_success PERL 'hash' '\n+test_expect_success 'hash' '\n \n test_oidmap \"hash one\n hash two\n\n\n> +}\n> +\n> +test_expect_success PERL 'hash' '\n> +\n> +test_oidmap \"hash one\n> +hash two\n> +hash invalidOid\n> +hash three\" \"$(test_oidhash one)\n> +$(test_oidhash two)\n> +Unknown oid: invalidOid\n> +$(test_oidhash three)\"\n> +\n> +'\n> +\n> +test_expect_success 'put' '\n> +\n> +test_oidmap \"put one 1\n> +put two 2\n> +put invalidOid 4\n> +put three 3\" \"NULL\n> +NULL\n> +Unknown oid: invalidOid\n> +NULL\"\n> +\n> +'\n> +\n> +test_expect_success 'replace' '\n> +\n> +test_oidmap \"put one 1\n> +put two 2\n> +put three 3\n> +put invalidOid 4\n> +put two deux\n> +put one un\" \"NULL\n> +NULL\n> +NULL\n> +Unknown oid: invalidOid\n> +2\n> +1\"\n> +\n> +'\n> +\n> +test_expect_success 'get' '\n> +\n> +test_oidmap \"put one 1\n> +put two 2\n> +put three 3\n> +get two\n> +get four\n> +get invalidOid\n> +get one\" \"NULL\n> +NULL\n> +NULL\n> +2\n> +NULL\n> +Unknown oid: invalidOid\n> +1\"\n> +\n> +'\n> +\n> +test_expect_success 'iterate' '\n> +\n> +test_oidmap \"put one 1\n> +put two 2\n> +put three 3\n> +iterate\" \"NULL\n> +NULL\n> +NULL\n> +$(git rev-parse two) 2\n> +$(git rev-parse one) 1\n> +$(git rev-parse three) 3\"\n> +\n> +'\n> +\n> +test_done\n> -- \n> 2.22.0.14.g9023ccb50a\n> \n"},{"id":"376894","messageId":"CAP8UFD2AD9NzOUcLfN+NuWp_9JzwdV9oUo9rGAPXt3EP95=_og@mail.gmail.com","threadId":"51272","inReplyTo":"20190609092259.GB24208@szeder.dev","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-09T20:24:55Z","receivedAt":"2019-06-09T20:25:10Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Jun 9, 2019 at 11:23 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>\n> On Sun, Jun 09, 2019 at 06:49:06AM +0200, Christian Couder wrote:\n> > +\n> > +test_oidmap() {\n> > +     echo \"$1\" | test-tool oidmap $3 > actual &&\n> > +     echo \"$2\" > expect &&\n>\n> Style nit: space between redirection op and filename.\n\nThanks for spotting this. It's fixed in my current version.\n\n> > +test_oidhash() {\n> > +     git rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n>\n> New Perl dependencies always make Dscho sad... :)\n\nYeah, I was not sure how to do it properly in shell so I was hoping I\nwould get suggestions about this. Thanks for looking at this!\n\nI could have hardcoded the values as it is done in t0011-hashmap.sh,\nbut I thought it was better to find a function that does he job.\n\n> So, 'test oidmap' from the previous patch prints the value we want to\n> check with:\n>\n>     printf(\"%u\\n\", sha1hash(oid.hash));\n\nYeah, I did it this way because \"test-hashmap.c\" does the same kind of\nthing to print hashes:\n\n            printf(\"%u %u %u %u\\n\",\n                   strhash(p1), memhash(p1, strlen(p1)),\n                   strihash(p1), memihash(p1, strlen(p1)));\n\n> First, since object ids inherently make more sense as hex values, it\n> would be more appropriate to print that hash with the '%x' format\n> specifier,\n\nI would be ok with that, but then I think it would make sense to also\nprint hex values in \"test-hashmap.c\".\n\n> and then we wouldn't need Perl's hex() anymore, and thus\n> could swap the order of the first four bytes in oidmap's hash without\n> relying on Perl, e.g. with:\n>\n>   sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n>\n> Second, and more importantly, the need for swapping the byte order\n> indicates that this test would fail on big-endian systems, I'm afraid.\n> So I think we need an additional bswap32() on the printing side,\n\nOk, but then shouldn't we also use bswap32() in \"test-hashmap.c\"?\n\nBy the way it seems that we use ntohl() or htonl() instead of\nbswap32() in the source code.\n\n> and then could further simplify 'test_oidhash':\n>\n> diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\n> index 0ba122a264..4177912f9a 100644\n> --- a/t/helper/test-oidmap.c\n> +++ b/t/helper/test-oidmap.c\n> @@ -51,7 +51,7 @@ int cmd__oidmap(int argc, const char **argv)\n>\n>                         /* print hash of oid */\n>                         if (!get_oid(p1, &oid))\n> -                               printf(\"%u\\n\", sha1hash(oid.hash));\n> +                               printf(\"%x\\n\", bswap32(sha1hash(oid.hash)));\n>                         else\n>                                 printf(\"Unknown oid: %s\\n\", p1);\n>\n> diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\n> index 3a8e8bdb3d..9c0d88a316 100755\n> --- a/t/t0016-oidmap.sh\n> +++ b/t/t0016-oidmap.sh\n> @@ -22,10 +22,10 @@ test_expect_success 'setup' '\n>  '\n>\n>  test_oidhash() {\n> -       git rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n> +       git rev-parse \"$1\" | cut -c1-8\n>  }\n>\n> -test_expect_success PERL 'hash' '\n> +test_expect_success 'hash' '\n\nYeah, I agree that it seems better to me this way.\n"},{"id":"376895","messageId":"20190609212130.GC24208@szeder.dev","threadId":"51272","inReplyTo":"CAP8UFD2AD9NzOUcLfN+NuWp_9JzwdV9oUo9rGAPXt3EP95=_og@mail.gmail.com","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-09T21:21:30Z","receivedAt":"2019-06-09T21:21:37Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Sun, Jun 09, 2019 at 10:24:55PM +0200, Christian Couder wrote:\n> On Sun, Jun 9, 2019 at 11:23 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> >\n> > On Sun, Jun 09, 2019 at 06:49:06AM +0200, Christian Couder wrote:\n> > > +\n> > > +test_oidmap() {\n> > > +     echo \"$1\" | test-tool oidmap $3 > actual &&\n> > > +     echo \"$2\" > expect &&\n> >\n> > Style nit: space between redirection op and filename.\n> \n> Thanks for spotting this. It's fixed in my current version.\n> \n> > > +test_oidhash() {\n> > > +     git rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n> >\n> > New Perl dependencies always make Dscho sad... :)\n> \n> Yeah, I was not sure how to do it properly in shell so I was hoping I\n> would get suggestions about this. Thanks for looking at this!\n> \n> I could have hardcoded the values as it is done in t0011-hashmap.sh,\n> but I thought it was better to find a function that does he job.\n\nWell, I'm fine with hardcoding the expected hash values (in network\nbyte order) as well, because then we won't add another git process\nupstream of a pipe that would pop up during audit later...\n\n> > So, 'test oidmap' from the previous patch prints the value we want to\n> > check with:\n> >\n> >     printf(\"%u\\n\", sha1hash(oid.hash));\n> \n> Yeah, I did it this way because \"test-hashmap.c\" does the same kind of\n> thing to print hashes:\n> \n>             printf(\"%u %u %u %u\\n\",\n>                    strhash(p1), memhash(p1, strlen(p1)),\n>                    strihash(p1), memihash(p1, strlen(p1)));\n> \n> > First, since object ids inherently make more sense as hex values, it\n> > would be more appropriate to print that hash with the '%x' format\n> > specifier,\n> \n> I would be ok with that, but then I think it would make sense to also\n> print hex values in \"test-hashmap.c\".\n> \n> > and then we wouldn't need Perl's hex() anymore, and thus\n> > could swap the order of the first four bytes in oidmap's hash without\n> > relying on Perl, e.g. with:\n> >\n> >   sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n> >\n> > Second, and more importantly, the need for swapping the byte order\n> > indicates that this test would fail on big-endian systems, I'm afraid.\n> > So I think we need an additional bswap32() on the printing side,\n> \n> Ok, but then shouldn't we also use bswap32() in \"test-hashmap.c\"?\n\nNo.  The two test scripts/helpers work with different hashes.  t0011\nand 'test-hashmap.c' uses the various FNV-1-based hash functions\n(strhash(), memhash(), ...) to calculate an unsigned int hash of the\nitems stored in the hashmap, therefore their hashes will be the same\nregardless of endianness.  In an oidmap, however, the hash is simply\nthe first four bytes of the object id as an unsigned int as is, and\nlook at how sha1hash() does it, and indeed at the last sentence of the\ncomment in front of it:\n\n * [...] Note that\n * the results will be different on big-endian and little-endian\n * platforms, so they should not be stored or transferred over the net.\n */\nstatic inline unsigned int sha1hash(const unsigned char *sha1)\n{\n        /*\n         * Equivalent to 'return *(unsigned int *)sha1;', but safe on\n         * platforms that don't support unaligned reads.\n         */\n        unsigned int hash;\n        memcpy(&hash, sha1, sizeof(hash));\n        return hash;\n}\n\n> By the way it seems that we use ntohl() or htonl() instead of\n> bswap32() in the source code.\n\nOK.\n\n> > and then could further simplify 'test_oidhash':\n> >\n> > diff --git a/t/helper/test-oidmap.c b/t/helper/test-oidmap.c\n> > index 0ba122a264..4177912f9a 100644\n> > --- a/t/helper/test-oidmap.c\n> > +++ b/t/helper/test-oidmap.c\n> > @@ -51,7 +51,7 @@ int cmd__oidmap(int argc, const char **argv)\n> >\n> >                         /* print hash of oid */\n> >                         if (!get_oid(p1, &oid))\n> > -                               printf(\"%u\\n\", sha1hash(oid.hash));\n> > +                               printf(\"%x\\n\", bswap32(sha1hash(oid.hash)));\n> >                         else\n> >                                 printf(\"Unknown oid: %s\\n\", p1);\n> >\n> > diff --git a/t/t0016-oidmap.sh b/t/t0016-oidmap.sh\n> > index 3a8e8bdb3d..9c0d88a316 100755\n> > --- a/t/t0016-oidmap.sh\n> > +++ b/t/t0016-oidmap.sh\n> > @@ -22,10 +22,10 @@ test_expect_success 'setup' '\n> >  '\n> >\n> >  test_oidhash() {\n> > -       git rev-parse \"$1\" | perl -ne 'print hex(\"$4$3$2$1\") . \"\\n\" if m/^(..)(..)(..)(..).*/;'\n> > +       git rev-parse \"$1\" | cut -c1-8\n> >  }\n> >\n> > -test_expect_success PERL 'hash' '\n> > +test_expect_success 'hash' '\n> \n> Yeah, I agree that it seems better to me this way.\n"},{"id":"376896","messageId":"CAP8UFD3iDY8gAm09OK2uQzG6bsB+06wVcGC6pjOgttb4sZ9F+w@mail.gmail.com","threadId":"51272","inReplyTo":"20190609212130.GC24208@szeder.dev","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-09T21:51:35Z","receivedAt":"2019-06-09T21:51:49Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Jun 9, 2019 at 11:21 PM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n>\n> On Sun, Jun 09, 2019 at 10:24:55PM +0200, Christian Couder wrote:\n> > On Sun, Jun 9, 2019 at 11:23 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:\n> > >\n> > > New Perl dependencies always make Dscho sad... :)\n> >\n> > Yeah, I was not sure how to do it properly in shell so I was hoping I\n> > would get suggestions about this. Thanks for looking at this!\n> >\n> > I could have hardcoded the values as it is done in t0011-hashmap.sh,\n> > but I thought it was better to find a function that does he job.\n>\n> Well, I'm fine with hardcoding the expected hash values (in network\n> byte order) as well, because then we won't add another git process\n> upstream of a pipe that would pop up during audit later...\n\nOk, I think I will do that then.\n\n> > > So, 'test oidmap' from the previous patch prints the value we want to\n> > > check with:\n> > >\n> > >     printf(\"%u\\n\", sha1hash(oid.hash));\n> >\n> > Yeah, I did it this way because \"test-hashmap.c\" does the same kind of\n> > thing to print hashes:\n> >\n> >             printf(\"%u %u %u %u\\n\",\n> >                    strhash(p1), memhash(p1, strlen(p1)),\n> >                    strihash(p1), memihash(p1, strlen(p1)));\n> >\n> > > First, since object ids inherently make more sense as hex values, it\n> > > would be more appropriate to print that hash with the '%x' format\n> > > specifier,\n> >\n> > I would be ok with that, but then I think it would make sense to also\n> > print hex values in \"test-hashmap.c\".\n> >\n> > > and then we wouldn't need Perl's hex() anymore, and thus\n> > > could swap the order of the first four bytes in oidmap's hash without\n> > > relying on Perl, e.g. with:\n> > >\n> > >   sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n> > >\n> > > Second, and more importantly, the need for swapping the byte order\n> > > indicates that this test would fail on big-endian systems, I'm afraid.\n> > > So I think we need an additional bswap32() on the printing side,\n> >\n> > Ok, but then shouldn't we also use bswap32() in \"test-hashmap.c\"?\n>\n> No.  The two test scripts/helpers work with different hashes.  t0011\n> and 'test-hashmap.c' uses the various FNV-1-based hash functions\n> (strhash(), memhash(), ...) to calculate an unsigned int hash of the\n> items stored in the hashmap, therefore their hashes will be the same\n> regardless of endianness.\n\nI see. Thanks for explaining that.\n\n>  In an oidmap, however, the hash is simply\n> the first four bytes of the object id as an unsigned int as is,\n\nYeah, I had realized that.\n\nThanks,\nChristian.\n"},{"id":"376947","messageId":"xmqqftoho0bg.fsf@gitster-ct.c.googlers.com","threadId":"51272","inReplyTo":"20190609092259.GB24208@szeder.dev","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-10T16:46:27Z","receivedAt":"2019-06-10T16:46:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor <szeder.dev@gmail.com> writes:\n\n> So, 'test oidmap' from the previous patch prints the value we want to\n> check with:\n>\n>     printf(\"%u\\n\", sha1hash(oid.hash));\n>\n> First, since object ids inherently make more sense as hex values, it\n> would be more appropriate to print that hash with the '%x' format\n> specifier, and then we wouldn't need Perl's hex() anymore, and thus\n> could swap the order of the first four bytes in oidmap's hash without\n> relying on Perl, e.g. with:\n>\n>   sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n>\n> Second, and more importantly, the need for swapping the byte order\n> indicates that this test would fail on big-endian systems, I'm afraid.\n> So I think we need an additional bswap32() on the printing side, and\n> then could further simplify 'test_oidhash':\n\nYup, if we are doing an ad-hoc t/helper/ command, we should strive\nto make it help the driving scripts around it to become simpler, and\nyour suggestion to do s/%u/%x/ is a good example of doing so.\n\nThanks for a dose of sanity.  The goal of the series may be\nworthwhile, and helping hands in improving its execution is very\nmuch appreciated.\n"},{"id":"377156","messageId":"20190613171913.GA14055@sigill.intra.peff.net","threadId":"51272","inReplyTo":"20190609092259.GB24208@szeder.dev","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-13T17:19:13Z","receivedAt":"2019-06-13T17:19:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jun 09, 2019 at 11:22:59AM +0200, SZEDER Gábor wrote:\n\n> So, 'test oidmap' from the previous patch prints the value we want to\n> check with:\n> \n>     printf(\"%u\\n\", sha1hash(oid.hash));\n> \n> First, since object ids inherently make more sense as hex values, it\n> would be more appropriate to print that hash with the '%x' format\n> specifier, and then we wouldn't need Perl's hex() anymore, and thus\n> could swap the order of the first four bytes in oidmap's hash without\n> relying on Perl, e.g. with:\n> \n>   sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n> \n> Second, and more importantly, the need for swapping the byte order\n> indicates that this test would fail on big-endian systems, I'm afraid.\n> So I think we need an additional bswap32() on the printing side, and\n> then could further simplify 'test_oidhash':\n\nI agree with all your points about using hex and pushing the logic into\ntest-oidmap.c. BUT.\n\nAt the point where we are normalizing byte order of the hashes, I have\nto wonder: why do we care about testing the hash value in the first\nplace? We care that oidmap can store and retrieve values, and that it\nperforms well. But as long as it does those things, I don't think\nanybody cares if it uses the first 4 bytes of the sha1 or the last 4.\n\nI know there are testing philosophies that go to this level of white-box\ntesting, but I don't think we usually do in Git. A unit test of oidmap's\nexternally visible behavior seems like the right level to me.\n\n-Peff\n"},{"id":"377164","messageId":"20190613175236.GF31952@szeder.dev","threadId":"51272","inReplyTo":"20190613171913.GA14055@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-06-13T17:52:36Z","receivedAt":"2019-06-13T17:52:42Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Thu, Jun 13, 2019 at 01:19:13PM -0400, Jeff King wrote:\n> On Sun, Jun 09, 2019 at 11:22:59AM +0200, SZEDER Gábor wrote:\n> \n> > So, 'test oidmap' from the previous patch prints the value we want to\n> > check with:\n> > \n> >     printf(\"%u\\n\", sha1hash(oid.hash));\n> > \n> > First, since object ids inherently make more sense as hex values, it\n> > would be more appropriate to print that hash with the '%x' format\n> > specifier, and then we wouldn't need Perl's hex() anymore, and thus\n> > could swap the order of the first four bytes in oidmap's hash without\n> > relying on Perl, e.g. with:\n> > \n> >   sed -e 's/^\\(..\\)\\(..\\)\\(..\\)\\(..\\).*/\\4\\3\\2\\1/'\n> > \n> > Second, and more importantly, the need for swapping the byte order\n> > indicates that this test would fail on big-endian systems, I'm afraid.\n> > So I think we need an additional bswap32() on the printing side, and\n> > then could further simplify 'test_oidhash':\n> \n> I agree with all your points about using hex and pushing the logic into\n> test-oidmap.c. BUT.\n> \n> At the point where we are normalizing byte order of the hashes, I have\n> to wonder: why do we care about testing the hash value in the first\n> place? We care that oidmap can store and retrieve values, and that it\n> performs well. But as long as it does those things, I don't think\n> anybody cares if it uses the first 4 bytes of the sha1 or the last 4.\n> \n> I know there are testing philosophies that go to this level of\n> white-box testing, but I don't think we usually do in Git. A unit\n> test of oidmap's externally visible behavior seems like the right\n> level to me.\n\nThat's a good point...  but then why does 't0011-hashmap.sh' do it in\nthe first place?  As far as I understood this t0016 mainly follows\nsuit of t0011.\n\n"},{"id":"377177","messageId":"20190613190229.GB27217@sigill.intra.peff.net","threadId":"51272","inReplyTo":"20190613175236.GF31952@szeder.dev","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-06-13T19:02:30Z","receivedAt":"2019-06-13T19:02:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jun 13, 2019 at 07:52:36PM +0200, SZEDER Gábor wrote:\n\n> > At the point where we are normalizing byte order of the hashes, I have\n> > to wonder: why do we care about testing the hash value in the first\n> > place? We care that oidmap can store and retrieve values, and that it\n> > performs well. But as long as it does those things, I don't think\n> > anybody cares if it uses the first 4 bytes of the sha1 or the last 4.\n> > \n> > I know there are testing philosophies that go to this level of\n> > white-box testing, but I don't think we usually do in Git. A unit\n> > test of oidmap's externally visible behavior seems like the right\n> > level to me.\n> \n> That's a good point...  but then why does 't0011-hashmap.sh' do it in\n> the first place?  As far as I understood this t0016 mainly follows\n> suit of t0011.\n\nI'd make the same argument against t0011. :)\n\nI think there it at least made a little more sense because we truly are\nhashing ourselves, rather than just copying out some sha1 bytes. But I\nthink I'd still argue that if I updated strhash() to use a different\nhash, I should not have to be updating t0011 to change out the hashes.\n\n-Peff\n"},{"id":"377203","messageId":"xmqqh88tunvt.fsf@gitster-ct.c.googlers.com","threadId":"51272","inReplyTo":"20190613190229.GB27217@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-06-13T22:22:14Z","receivedAt":"2019-06-13T22:22:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> > I know there are testing philosophies that go to this level of\n>> > white-box testing, but I don't think we usually do in Git. A unit\n>> > test of oidmap's externally visible behavior seems like the right\n>> > level to me.\n>> \n>> That's a good point...  but then why does 't0011-hashmap.sh' do it in\n>> the first place?  As far as I understood this t0016 mainly follows\n>> suit of t0011.\n>\n> I'd make the same argument against t0011. :)\n\nYeah, I tend to agree.  It is not a good excuse that somebody else\nalerady has made a mistake.\n\n> I think there it at least made a little more sense because we truly are\n> hashing ourselves, rather than just copying out some sha1 bytes. But I\n> think I'd still argue that if I updated strhash() to use a different\n> hash, I should not have to be updating t0011 to change out the hashes.\n\nTrue, too.\n"},{"id":"377230","messageId":"CAP8UFD3Q2rQwq82LNSXuLSEUMHABVJVgy8UJ7Fvgxug1rZGMFQ@mail.gmail.com","threadId":"51272","inReplyTo":"xmqqh88tunvt.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/3] t: add t0016-oidmap.sh","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-06-14T10:36:02Z","receivedAt":"2019-06-14T10:36:18Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Jun 14, 2019 at 12:22 AM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Jeff King <peff@peff.net> writes:\n>\n> >> > I know there are testing philosophies that go to this level of\n> >> > white-box testing, but I don't think we usually do in Git. A unit\n> >> > test of oidmap's externally visible behavior seems like the right\n> >> > level to me.\n> >>\n> >> That's a good point...  but then why does 't0011-hashmap.sh' do it in\n> >> the first place?  As far as I understood this t0016 mainly follows\n> >> suit of t0011.\n> >\n> > I'd make the same argument against t0011. :)\n>\n> Yeah, I tend to agree.  It is not a good excuse that somebody else\n> alerady has made a mistake.\n\nOk, I will remove the \"hash\" test in t0016 and the corresponding code\nin test-oidmap.c.\n\n> > I think there it at least made a little more sense because we truly are\n> > hashing ourselves, rather than just copying out some sha1 bytes. But I\n> > think I'd still argue that if I updated strhash() to use a different\n> > hash, I should not have to be updating t0011 to change out the hashes.\n>\n> True, too.\n\nI will also send an additional patch to remove similar code in t00161\nand test-hashmap.c.\n"}]}