{"thread":{"id":"62456","subject":"[PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","startedAt":"2024-11-05T19:05:13Z","lastAt":"2024-11-08T17:26:12Z","messageCount":19,"participants":["Taylor Blau","Junio C Hamano","brian m. carlson","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"506674","messageId":"cover.1730833506.git.me@ttaylorr.com","threadId":"62456","inReplyTo":null,"subject":"[PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-05T19:05:10Z","receivedAt":"2024-11-05T19:05:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"This series implements a new 'sha1-unsafe' test helper, similar to\n't/helper/test-tool sha1'.\n\nI have found such a helper to be really handy when debugging the new\nSHA1_UNSAFE build knobs, e.g., to easily compare the performance of the\nsafe versus unsafe routines, different unsafe variants, etc.\n\nThe first patch prepares us by setting up the existing cmd_hash_impl()\nfunction to be able to conditionally use the unsafe variant. The final\npatch introduces a new 'sha1-unsafe' test helper which calls the new\nvariant.\n\nThanks in advance for your review!\n\nTaylor Blau (2):\n t/helper/test-sha1: prepare for an unsafe mode\n t/helper/test-tool: implement sha1-unsafe helper\n\n t/helper/test-hash.c | 17 +++++++++++++----\n t/helper/test-sha1.c | 7 ++++++-\n t/helper/test-sha1.sh | 38 ++++++++++++++++++++++----------------\n t/helper/test-sha256.c | 2 +-\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 3 ++-\n 6 files changed, 45 insertions(+), 23 deletions(-)\n\nRange-diff against v1:\n1: 3b31db4d2df = 1: 0e2fcee6894 t/helper/test-sha1: prepare for an unsafe mode\n2: d343f5dc9e5 = 2: d8c1fc78b57 t/helper/test-tool: implement sha1-unsafe helper\n\nbase-commit: 8f8d6eee531b3fa1a8ef14f169b0cb5035f7a772\n-- \n2.47.0.231.gd8c1fc78b57\n"},{"id":"506675","messageId":"0e2fcee6894b7b16136ff09a69f199bea9f8c882.1730833507.git.me@ttaylorr.com","threadId":"62456","inReplyTo":"cover.1730833506.git.me@ttaylorr.com","subject":"[PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-05T19:05:14Z","receivedAt":"2024-11-05T19:05:16Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"With the new \"unsafe\" SHA-1 build knob, it would be convenient to have\na test-tool that can exercise Git's unsafe SHA-1 wrappers for testing,\nsimilar to 't/helper/test-tool sha1'.\n\nPrepare for such a helper by altering the implementation of that\ntest-tool (in cmd_hash_impl(), which is generic and parameterized over\ndifferent hash functions) to conditionally run the unsafe variants of\nthe chosen hash function.\n\nThe following commit will add a new test-tool which makes use of this\nnew parameter.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-hash.c | 17 +++++++++++++----\n t/helper/test-sha1.c | 2 +-\n t/helper/test-sha256.c | 2 +-\n t/helper/test-tool.h | 2 +-\n 4 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/t/helper/test-hash.c b/t/helper/test-hash.c\nindex 45d829c908f..d0ee668df95 100644\n--- a/t/helper/test-hash.c\n+++ b/t/helper/test-hash.c\n@@ -1,7 +1,7 @@\n #include \"test-tool.h\"\n #include \"hex.h\"\n \n-int cmd_hash_impl(int ac, const char **av, int algo)\n+int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)\n {\n \tgit_hash_ctx ctx;\n \tunsigned char hash[GIT_MAX_HEXSZ];\n@@ -27,7 +27,10 @@ int cmd_hash_impl(int ac, const char **av, int algo)\n \t\t\tdie(\"OOPS\");\n \t}\n \n-\talgop->init_fn(&ctx);\n+\tif (unsafe)\n+\t\talgop->unsafe_init_fn(&ctx);\n+\telse\n+\t\talgop->init_fn(&ctx);\n \n \twhile (1) {\n \t\tssize_t sz, this_sz;\n@@ -46,9 +49,15 @@ int cmd_hash_impl(int ac, const char **av, int algo)\n \t\t}\n \t\tif (this_sz == 0)\n \t\t\tbreak;\n-\t\talgop->update_fn(&ctx, buffer, this_sz);\n+\t\tif (unsafe)\n+\t\t\talgop->unsafe_update_fn(&ctx, buffer, this_sz);\n+\t\telse\n+\t\t\talgop->update_fn(&ctx, buffer, this_sz);\n \t}\n-\talgop->final_fn(hash, &ctx);\n+\tif (unsafe)\n+\t\talgop->unsafe_final_fn(hash, &ctx);\n+\telse\n+\t\talgop->final_fn(hash, &ctx);\n \n \tif (binary)\n \t\tfwrite(hash, 1, algop->rawsz, stdout);\ndiff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c\nindex e60d000c039..1c1272cc1f9 100644\n--- a/t/helper/test-sha1.c\n+++ b/t/helper/test-sha1.c\n@@ -3,7 +3,7 @@\n \n int cmd__sha1(int ac, const char **av)\n {\n-\treturn cmd_hash_impl(ac, av, GIT_HASH_SHA1);\n+\treturn cmd_hash_impl(ac, av, GIT_HASH_SHA1, 0);\n }\n \n int cmd__sha1_is_sha1dc(int argc UNUSED, const char **argv UNUSED)\ndiff --git a/t/helper/test-sha256.c b/t/helper/test-sha256.c\nindex 2fb20438f3c..7fd0aa1fcd3 100644\n--- a/t/helper/test-sha256.c\n+++ b/t/helper/test-sha256.c\n@@ -3,5 +3,5 @@\n \n int cmd__sha256(int ac, const char **av)\n {\n-\treturn cmd_hash_impl(ac, av, GIT_HASH_SHA256);\n+\treturn cmd_hash_impl(ac, av, GIT_HASH_SHA256, 0);\n }\ndiff --git a/t/helper/test-tool.h b/t/helper/test-tool.h\nindex 21802ac27da..f3524d9a0f6 100644\n--- a/t/helper/test-tool.h\n+++ b/t/helper/test-tool.h\n@@ -81,6 +81,6 @@ int cmd__windows_named_pipe(int argc, const char **argv);\n #endif\n int cmd__write_cache(int argc, const char **argv);\n \n-int cmd_hash_impl(int ac, const char **av, int algo);\n+int cmd_hash_impl(int ac, const char **av, int algo, int unsafe);\n \n #endif\n-- \n2.47.0.231.gd8c1fc78b57\n\n"},{"id":"506676","messageId":"d8c1fc78b57e02a140b5c363caaa14c2dc2bb274.1730833507.git.me@ttaylorr.com","threadId":"62456","inReplyTo":"cover.1730833506.git.me@ttaylorr.com","subject":"[PATCH v2 2/2] t/helper/test-tool: implement sha1-unsafe helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-05T19:05:17Z","receivedAt":"2024-11-05T19:05:19Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Add a new helper similar to 't/helper/test-tool sha1' called instead\n\"sha1-unsafe\" which uses the unsafe variant of Git's SHA-1 wrappers.\n\nWhile we're at it, modify the test-sha1.sh script to exercise both\nthe sha1 and sha1-unsafe test tools to ensure that both produce the\nexpected hash values.\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-sha1.c | 5 +++++\n t/helper/test-sha1.sh | 38 ++++++++++++++++++++++----------------\n t/helper/test-tool.c | 1 +\n t/helper/test-tool.h | 1 +\n 4 files changed, 29 insertions(+), 16 deletions(-)\n\ndiff --git a/t/helper/test-sha1.c b/t/helper/test-sha1.c\nindex 1c1272cc1f9..349540c4df8 100644\n--- a/t/helper/test-sha1.c\n+++ b/t/helper/test-sha1.c\n@@ -13,3 +13,8 @@ int cmd__sha1_is_sha1dc(int argc UNUSED, const char **argv UNUSED)\n #endif\n \treturn 1;\n }\n+\n+int cmd__sha1_unsafe(int ac, const char **av)\n+{\n+\treturn cmd_hash_impl(ac, av, GIT_HASH_SHA1, 1);\n+}\ndiff --git a/t/helper/test-sha1.sh b/t/helper/test-sha1.sh\nindex 84594885c70..bf387d3db14 100755\n--- a/t/helper/test-sha1.sh\n+++ b/t/helper/test-sha1.sh\n@@ -3,25 +3,31 @@\n dd if=/dev/zero bs=1048576 count=100 2>/dev/null |\n /usr/bin/time t/helper/test-tool sha1 >/dev/null\n \n+dd if=/dev/zero bs=1048576 count=100 2>/dev/null |\n+/usr/bin/time t/helper/test-tool sha1-unsafe >/dev/null\n+\n while read expect cnt pfx\n do\n \tcase \"$expect\" in '#'*) continue ;; esac\n-\tactual=$(\n-\t\t{\n-\t\t\ttest -z \"$pfx\" || echo \"$pfx\"\n-\t\t\tdd if=/dev/zero bs=1048576 count=$cnt 2>/dev/null |\n-\t\t\tperl -pe 'y/\\000/g/'\n-\t\t} | ./t/helper/test-tool sha1 $cnt\n-\t)\n-\tif test \"$expect\" = \"$actual\"\n-\tthen\n-\t\techo \"OK: $expect $cnt $pfx\"\n-\telse\n-\t\techo >&2 \"OOPS: $cnt\"\n-\t\techo >&2 \"expect: $expect\"\n-\t\techo >&2 \"actual: $actual\"\n-\t\texit 1\n-\tfi\n+\tfor sha1 in sha1 sha1-unsafe\n+\tdo\n+\t\tactual=$(\n+\t\t\t{\n+\t\t\t\ttest -z \"$pfx\" || echo \"$pfx\"\n+\t\t\t\tdd if=/dev/zero bs=1048576 count=$cnt 2>/dev/null |\n+\t\t\t\tperl -pe 'y/\\000/g/'\n+\t\t\t} | ./t/helper/test-tool $sha1 $cnt\n+\t\t)\n+\t\tif test \"$expect\" = \"$actual\"\n+\t\tthen\n+\t\t\techo \"OK ($sha1): $expect $cnt $pfx\"\n+\t\telse\n+\t\t\techo >&2 \"OOPS ($sha1): $cnt\"\n+\t\t\techo >&2 \"expect ($sha1): $expect\"\n+\t\t\techo >&2 \"actual ($sha1): $actual\"\n+\t\t\texit 1\n+\t\tfi\n+\tdone\n done < writes:\n\n> -int cmd_hash_impl(int ac, const char **av, int algo)\n> +int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)\n> {\n> \tgit_hash_ctx ctx;\n> \tunsigned char hash[GIT_MAX_HEXSZ];\n> @@ -27,7 +27,10 @@ int cmd_hash_impl(int ac, const char **av, int algo)\n> \t\t\tdie(\"OOPS\");\n> \t}\n> \n> -\talgop->init_fn(&ctx);\n> +\tif (unsafe)\n> +\t\talgop->unsafe_init_fn(&ctx);\n> +\telse\n> +\t\talgop->init_fn(&ctx);\n\nIt may be just me, and it would not matter all that much within the\ncontext of the project because this is merely a test helper, but\ngiving a pair of init/unsafe_init methods to algop looks unnerving.\nIt gives an impression that every design of hash algorithm must come\nwith normal and unsafe variant, which is not what you want to say.\n\nI would have expected that there are different algorighm instances,\nand one of them would be \"unsafe SHA-1\", among \"normal SHA-1\" and\n\"SHA-256\" (as the last one would not even have unsafe_init_fn\nmethod), and the callers that want to measure the performance of\neach algorithm would simply pick one of these instances and go\nthrough the usual \"init\", \"update\", \"final\" flow, regardless of the\n\"unsafe\" ness of the algorithm.\n\nIOW, ...\n\n> \twhile (1) {\n> \t\tssize_t sz, this_sz;\n> @@ -46,9 +49,15 @@ int cmd_hash_impl(int ac, const char **av, int algo)\n> \t\t}\n> \t\tif (this_sz == 0)\n> \t\t\tbreak;\n> -\t\talgop->update_fn(&ctx, buffer, this_sz);\n> +\t\tif (unsafe)\n> +\t\t\talgop->unsafe_update_fn(&ctx, buffer, this_sz);\n> +\t\telse\n> +\t\t\talgop->update_fn(&ctx, buffer, this_sz);\n> \t}\n> -\talgop->final_fn(hash, &ctx);\n> +\tif (unsafe)\n> +\t\talgop->unsafe_final_fn(hash, &ctx);\n> +\telse\n> +\t\talgop->final_fn(hash, &ctx);\n\n... I didn't expect any of these \"if (unsafe) .. else ..\" switches.\n\n"},{"id":"506766","messageId":"ZywOWn08cGBnBWM-@tapette.crustytoothpaste.net","threadId":"62456","inReplyTo":"xmqqcyj9qgyf.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-07T00:48:26Z","receivedAt":"2024-11-07T00:48:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-11-06 at 01:38:48, Junio C Hamano wrote:\n> Taylor Blau writes:\n> \n> > -int cmd_hash_impl(int ac, const char **av, int algo)\n> > +int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)\n> > {\n> > \tgit_hash_ctx ctx;\n> > \tunsigned char hash[GIT_MAX_HEXSZ];\n> > @@ -27,7 +27,10 @@ int cmd_hash_impl(int ac, const char **av, int algo)\n> > \t\t\tdie(\"OOPS\");\n> > \t}\n> > \n> > -\talgop->init_fn(&ctx);\n> > +\tif (unsafe)\n> > +\t\talgop->unsafe_init_fn(&ctx);\n> > +\telse\n> > +\t\talgop->init_fn(&ctx);\n> \n> It may be just me, and it would not matter all that much within the\n> context of the project because this is merely a test helper, but\n> giving a pair of init/unsafe_init methods to algop looks unnerving.\n> It gives an impression that every design of hash algorithm must come\n> with normal and unsafe variant, which is not what you want to say.\n> \n> I would have expected that there are different algorighm instances,\n> and one of them would be \"unsafe SHA-1\", among \"normal SHA-1\" and\n> \"SHA-256\" (as the last one would not even have unsafe_init_fn\n> method), and the callers that want to measure the performance of\n> each algorithm would simply pick one of these instances and go\n> through the usual \"init\", \"update\", \"final\" flow, regardless of the\n> \"unsafe\" ness of the algorithm.\n> \n> IOW, ...\n> \n> > \twhile (1) {\n> > \t\tssize_t sz, this_sz;\n> > @@ -46,9 +49,15 @@ int cmd_hash_impl(int ac, const char **av, int algo)\n> > \t\t}\n> > \t\tif (this_sz == 0)\n> > \t\t\tbreak;\n> > -\t\talgop->update_fn(&ctx, buffer, this_sz);\n> > +\t\tif (unsafe)\n> > +\t\t\talgop->unsafe_update_fn(&ctx, buffer, this_sz);\n> > +\t\telse\n> > +\t\t\talgop->update_fn(&ctx, buffer, this_sz);\n> > \t}\n> > -\talgop->final_fn(hash, &ctx);\n> > +\tif (unsafe)\n> > +\t\talgop->unsafe_final_fn(hash, &ctx);\n> > +\telse\n> > +\t\talgop->final_fn(hash, &ctx);\n> \n> ... I didn't expect any of these \"if (unsafe) .. else ..\" switches.\n\nI don't think we can remove those, and here's why. Certainly Taylor can\ncorrect me if I'm off base, though.\n\nIn the normal case, our hash struct is a union, and different\nimplementations can have a different layout. A SHA-1 implementation\nwill usually keep track of a 64-bit size, 5 32-bit words, and up to 64\nbytes of data that might need to be processed. Maybe SHA-1-DC, our safe\nimplementation, stores the size first, but our unsafe implementation\nis OpenSSL and it stores the 5 hash words first, or whatever.\n\nIf we use the same update and final functions, we'll end up with\nincorrect data because we'll have initialized the union contents with\ndata for one implementation but are trying to update or finalize it with\ndifferent data, which in the very best case will just produce broken\nresults, and might just cause a segfault (if one of the implementations\nstores a pointer instead of storing the data in the union).\n\nWe could certainly adopt a different hash algorithm structure for safe\nand unsafe code, but we have a lot of places that assume that there's\njust one structure per algorithm. It would require a substantial amount\nof refactoring to remove those assumptions and deal with the fact that\nwe now have two SHA-1 implementations. It would also be tricky to deal\nwith the fact that for SHA-1, we might use the safe or unsafe algorithm,\nbut for SHA-256, there's only one algorithm structure and we need to use\nit for both.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"506767","messageId":"20241107013915.GA961214@coredump.intra.peff.net","threadId":"62456","inReplyTo":"ZywOWn08cGBnBWM-@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-07T01:39:15Z","receivedAt":"2024-11-07T01:39:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 12:48:26AM +0000, brian m. carlson wrote:\n\n> > I would have expected that there are different algorighm instances,\n> > and one of them would be \"unsafe SHA-1\", among \"normal SHA-1\" and\n> > \"SHA-256\" (as the last one would not even have unsafe_init_fn\n> > method), and the callers that want to measure the performance of\n> > each algorithm would simply pick one of these instances and go\n> > through the usual \"init\", \"update\", \"final\" flow, regardless of the\n> > \"unsafe\" ness of the algorithm.\n> [...]\n> > ... I didn't expect any of these \"if (unsafe) .. else ..\" switches.\n> \n> I don't think we can remove those, and here's why. Certainly Taylor can\n> correct me if I'm off base, though.\n> \n> In the normal case, our hash struct is a union, and different\n> implementations can have a different layout. A SHA-1 implementation\n> will usually keep track of a 64-bit size, 5 32-bit words, and up to 64\n> bytes of data that might need to be processed. Maybe SHA-1-DC, our safe\n> implementation, stores the size first, but our unsafe implementation\n> is OpenSSL and it stores the 5 hash words first, or whatever.\n> \n> If we use the same update and final functions, we'll end up with\n> incorrect data because we'll have initialized the union contents with\n> data for one implementation but are trying to update or finalize it with\n> different data, which in the very best case will just produce broken\n> results, and might just cause a segfault (if one of the implementations\n> stores a pointer instead of storing the data in the union).\n\nI don't think Junio is proposing mixing the functions on a single data\ntype. Which, as you note, would be a disaster. I think the idea is for a\nseparate git_hash_algo struct entirely, that can be slotted in as a\npointer (since git_hash_algo is already essentially a virtual type).\n\nThat gets weird as you say here:\n\n> We could certainly adopt a different hash algorithm structure for safe\n> and unsafe code, but we have a lot of places that assume that there's\n> just one structure per algorithm. It would require a substantial amount\n> of refactoring to remove those assumptions and deal with the fact that\n> we now have two SHA-1 implementations. It would also be tricky to deal\n> with the fact that for SHA-1, we might use the safe or unsafe algorithm,\n> but for SHA-256, there's only one algorithm structure and we need to use\n> it for both.\n\nboth because we have two different algos for \"sha1\", but also because\nthey are not _really_ independent. If the_hash_algo is sha1, then\nwhichever implementation a given piece of code is using, it must still\nbe one of the sha1 variants.\n\nSo I think you wouldn't want to allocate an enum or a slot in the\nhash_algos array, because it is not really an independent algorithm.\nBut I think it _could_ work as a separate struct that the caller derives\nfrom the main hash algorithm. For example, imagine that the\ngit_hash_algo struct didn't have unsafe_init_fn, etc, but instead had:\n\n struct git_hash_algo *unsafe_implementation;\n\nwith a matching accessor like:\n\n struct git_hash_algo *unsafe_hash_algo(struct git_hash_algo *algo)\n {\n\t/* if we have a faster \"unsafe\" implementation, use that */\n\tif (algo->unsafe_implementation)\n\t\treturn algo->unsafe_implementation;\n\t/* otherwise just use the default one */\n\treturn algo;\n }\n\nAnd then csum-file.c, rather than calling:\n\n the_hash_algo->unsafe_init_fn(...);\n ...\n the_hash_algo->unsafe_final_fn(...);\n\nwould do this:\n\n struct git_hash_algo *algo = unsafe_hash_algo(the_hash_algo);\n algo->init_fn(...);\n ...\n algo->final_fn(...);\n\nAnd likewise this test helper would have a single conditional at the\nstart:\n\n if (unsafe)\n\talgo = unsafe_hash_algo(algo);\n\nand the rest of the code would just use that.\n\nAll that said, I do not think it buys us anything for \"real\" code. There\nthe decision between safe/unsafe is in the context of how we are using\nit, and not based on some conditional. So while the code in this test\nhelper is ugly, I don't think we'd ever write anything like that for\nreal. So it may not be worth the effort to refactor into a more virtual\nobject-oriented way.\n\n-Peff\n"},{"id":"506768","messageId":"20241107014737.GB961214@coredump.intra.peff.net","threadId":"62456","inReplyTo":"cover.1730833506.git.me@ttaylorr.com","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-07T01:47:37Z","receivedAt":"2024-11-07T01:47:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 05, 2024 at 02:05:10PM -0500, Taylor Blau wrote:\n\n> This series implements a new 'sha1-unsafe' test helper, similar to\n> 't/helper/test-tool sha1'.\n> \n> I have found such a helper to be really handy when debugging the new\n> SHA1_UNSAFE build knobs, e.g., to easily compare the performance of the\n> safe versus unsafe routines, different unsafe variants, etc.\n> \n> The first patch prepares us by setting up the existing cmd_hash_impl()\n> function to be able to conditionally use the unsafe variant. The final\n> patch introduces a new 'sha1-unsafe' test helper which calls the new\n> variant.\n\nI think this is a useful thing to have, and I didn't see anything wrong\nin the implementation. I did notice some oddities that existed before\nyour series:\n\n 1. Why do we have \"test-tool sha256\" at all? Nobody in the test suite\n calls it. It feels like the whole test-sha1/sha256/hash split is\n overly complicated. A single \"test-tool hash\" seems like it would\n be simpler, and it could take an \"--algo\" parameter (and an\n \"--unsafe\" one). I guess in the end we end up with the same options\n ,but the proliferation of top-level test-tool commands seems ugly\n to me (likewise \"sha1_is_sha1dc\").\n\n 2. You modified test-sha1.sh, but I've wondered if we should just\n delete that script. It is not ever invoked in the test suite AFAIK.\n If we want correctness tests, they should go into a real t[0-9]*.sh\n script (though one imagines we exercise the sha1 code quite a lot\n in the rest of the tests). And it starts with a weird ad-hoc\n performance test that doesn't really tell us much. A t/perf test\n would be better (if it is even worth measuring at all).\n\nSo I dunno. None of those are the fault of your series, but it is piling\non yet another test-tool command.\n\n-Peff\n"},{"id":"506769","messageId":"Zywcr2lMM_Ij8suu@tapette.crustytoothpaste.net","threadId":"62456","inReplyTo":"20241107013915.GA961214@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-07T01:49:35Z","receivedAt":"2024-11-07T01:49:38Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-11-07 at 01:39:15, Jeff King wrote:\n> So I think you wouldn't want to allocate an enum or a slot in the\n> hash_algos array, because it is not really an independent algorithm.\n> But I think it _could_ work as a separate struct that the caller derives\n> from the main hash algorithm. For example, imagine that the\n> git_hash_algo struct didn't have unsafe_init_fn, etc, but instead had:\n> \n> struct git_hash_algo *unsafe_implementation;\n> \n> with a matching accessor like:\n> \n> struct git_hash_algo *unsafe_hash_algo(struct git_hash_algo *algo)\n> {\n> \t/* if we have a faster \"unsafe\" implementation, use that */\n> \tif (algo->unsafe_implementation)\n> \t\treturn algo->unsafe_implementation;\n> \t/* otherwise just use the default one */\n> \treturn algo;\n> }\n> \n> And then csum-file.c, rather than calling:\n> \n> the_hash_algo->unsafe_init_fn(...);\n> ...\n> the_hash_algo->unsafe_final_fn(...);\n> \n> would do this:\n> \n> struct git_hash_algo *algo = unsafe_hash_algo(the_hash_algo);\n> algo->init_fn(...);\n> ...\n> algo->final_fn(...);\n> \n> And likewise this test helper would have a single conditional at the\n> start:\n> \n> if (unsafe)\n> \talgo = unsafe_hash_algo(algo);\n> \n> and the rest of the code would just use that.\n\nAh, yes, I think that approach would be simpler and nicer to work with\nthan a separate entry in the `hash_algos` array. We do, however, have\nsome places that assume that a `struct git_hash_algo *` points into the\n`hash_algos` array (notably `hash_algo_by_ptr`), so we'd have to adjust\nfor that, move the function pointers out into their own struct which\nwe'd use for `unsafe_hash_algo`, or be careful never to call the\nrelevant functions on our special `git_hash_algo` struct.\n\n> All that said, I do not think it buys us anything for \"real\" code. There\n> the decision between safe/unsafe is in the context of how we are using\n> it, and not based on some conditional. So while the code in this test\n> helper is ugly, I don't think we'd ever write anything like that for\n> real. So it may not be worth the effort to refactor into a more virtual\n> object-oriented way.\n\nYeah, I don't have a strong opinion one way or the other. I think, with\nthe limitation I mentioned above, it would probably require a decent\namount of refactoring if we took a different approach, and I'm fine with\ngoing with Taylor's current approach unless he wants to do that\nrefactoring (in which case, great).\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"506771","messageId":"ZywgZt419SfPUVkQ@tapette.crustytoothpaste.net","threadId":"62456","inReplyTo":"20241107014737.GB961214@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2024-11-07T02:05:26Z","receivedAt":"2024-11-07T02:05:29Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On 2024-11-07 at 01:47:37, Jeff King wrote:\n> I think this is a useful thing to have, and I didn't see anything wrong\n> in the implementation. I did notice some oddities that existed before\n> your series:\n> \n> 1. Why do we have \"test-tool sha256\" at all? Nobody in the test suite\n> calls it. It feels like the whole test-sha1/sha256/hash split is\n> overly complicated. A single \"test-tool hash\" seems like it would\n> be simpler, and it could take an \"--algo\" parameter (and an\n> \"--unsafe\" one). I guess in the end we end up with the same options\n> ,but the proliferation of top-level test-tool commands seems ugly\n> to me (likewise \"sha1_is_sha1dc\").\n\nI think we do in `pack_trailer` in `t/lib-pack.sh`, but not in a\ngreppable way:\n\n # Compute and append pack trailer to \"$1\"\n pack_trailer () {\n \ttest-tool $(test_oid algo) -b <\"$1\" >trailer.tmp &&\n \tcat trailer.tmp >>\"$1\" &&\n \trm -f trailer.tmp\n }\n\nWhen you posed the question above, I wasn't sure why I added this\nfunctionality: was it just to test my SHA-256 implementation adequately\nor did it have some actual utility in the testsuite? However, I knew if\nit didn't appear straightforwardly in `git grep`, any uses would involve\n`test_oid`, and boom, I was right.\n\nI think a single helper with `--algo` and `--unsafe` parameters would\nalso be fine and would probably be a little more tidy, as you mention.\n-- \nbrian m. carlson (they/them or he/him)\nToronto, Ontario, CA\n"},{"id":"506772","messageId":"20241107020810.GD961214@coredump.intra.peff.net","threadId":"62456","inReplyTo":"Zywcr2lMM_Ij8suu@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-07T02:08:10Z","receivedAt":"2024-11-07T02:08:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 01:49:35AM +0000, brian m. carlson wrote:\n\n> Ah, yes, I think that approach would be simpler and nicer to work with\n> than a separate entry in the `hash_algos` array. We do, however, have\n> some places that assume that a `struct git_hash_algo *` points into the\n> `hash_algos` array (notably `hash_algo_by_ptr`), so we'd have to adjust\n> for that, move the function pointers out into their own struct which\n> we'd use for `unsafe_hash_algo`, or be careful never to call the\n> relevant functions on our special `git_hash_algo` struct.\n\nYeah, I wondered if some code might be surprised by having a separate\nhash algo. Another weird thing is that the sub-implementation algo\nstruct will have its own rawsz, hexsz, etc, even though those _must_ be\nthe same its parent.\n\nIf all of the virtual implementation functions were in a git_hash_impl\nstruct, then each git_hash_algo struct could have one embedded for the\nmain implementation (which in sha1's case would be a collision detecting\none), and an optional pointer to another unsafe _impl struct.\n\nAnd then you get more type-safety, because you can never confuse the\n_impl struct for a parent git_hash_algo.\n\nThe downside is that every single caller, even if it doesn't care about\nthe unsafe variant, needs to refer to the_hash_algo->impl.init_fn(),\netc, due to the extra layer of indirection. Probably not worth it.\n\n> Yeah, I don't have a strong opinion one way or the other. I think, with\n> the limitation I mentioned above, it would probably require a decent\n> amount of refactoring if we took a different approach, and I'm fine with\n> going with Taylor's current approach unless he wants to do that\n> refactoring (in which case, great).\n\nMe too. If we were fixing something ugly or error-prone that we expected\nto come up in real code, it might be worth it. But it's probably not for\ntrying to accommodate a one-off test helper.\n\n-Peff\n"},{"id":"506777","messageId":"xmqqed3nvizn.fsf@gitster.g","threadId":"62456","inReplyTo":"20241107020810.GD961214@coredump.intra.peff.net","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-07T03:08:12Z","receivedAt":"2024-11-07T03:08:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Me too. If we were fixing something ugly or error-prone that we expected\n> to come up in real code, it might be worth it. But it's probably not for\n> trying to accommodate a one-off test helper.\n\nI 100% agree that it would not matter all that much within the\ncontext of the project because this is merely a test helper.\n\nIt looked odd not to have sha1-unsafe as a first class citizen next\nto sha1 and sha256, with an entry for it in the list enums and list\nof hash algos. If there is a good reason why adding the \"unsafe\"\nvariant as a first class citizen among algos, the approach posted\npatch took is fine.\n\nThanks.\n\n\n\n"},{"id":"506811","messageId":"Zy0xfVqtOXhEVDQB@nand.local","threadId":"62456","inReplyTo":"Zywcr2lMM_Ij8suu@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-07T21:30:37Z","receivedAt":"2024-11-07T21:30:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 07, 2024 at 01:49:35AM +0000, brian m. carlson wrote:\n> On 2024-11-07 at 01:39:15, Jeff King wrote:\n> > So I think you wouldn't want to allocate an enum or a slot in the\n> > hash_algos array, because it is not really an independent algorithm.\n> > But I think it _could_ work as a separate struct that the caller derives\n> > from the main hash algorithm. For example, imagine that the\n> > git_hash_algo struct didn't have unsafe_init_fn, etc, but instead had:\n> >\n> > struct git_hash_algo *unsafe_implementation;\n> >\n> > with a matching accessor like:\n> >\n> > struct git_hash_algo *unsafe_hash_algo(struct git_hash_algo *algo)\n> > {\n> > \t/* if we have a faster \"unsafe\" implementation, use that */\n> > \tif (algo->unsafe_implementation)\n> > \t\treturn algo->unsafe_implementation;\n> > \t/* otherwise just use the default one */\n> > \treturn algo;\n> > }\n> >\n> > And then csum-file.c, rather than calling:\n> >\n> > the_hash_algo->unsafe_init_fn(...);\n> > ...\n> > the_hash_algo->unsafe_final_fn(...);\n> >\n> > would do this:\n> >\n> > struct git_hash_algo *algo = unsafe_hash_algo(the_hash_algo);\n> > algo->init_fn(...);\n> > ...\n> > algo->final_fn(...);\n> >\n> > And likewise this test helper would have a single conditional at the\n> > start:\n> >\n> > if (unsafe)\n> > \talgo = unsafe_hash_algo(algo);\n> >\n> > and the rest of the code would just use that.\n>\n> Ah, yes, I think that approach would be simpler and nicer to work with\n> than a separate entry in the `hash_algos` array. We do, however, have\n> some places that assume that a `struct git_hash_algo *` points into the\n> `hash_algos` array (notably `hash_algo_by_ptr`), so we'd have to adjust\n> for that, move the function pointers out into their own struct which\n> we'd use for `unsafe_hash_algo`, or be careful never to call the\n> relevant functions on our special `git_hash_algo` struct.\n\nI agree that the approach suggested by Peff here is clean and would\nbutton up the test-helper code nicely. It may be worth doing in the\nfuture, but I agree that it doesn't seem entirely in scope here.\n\nPart of the reason that I did not initially add a separate sha1-unsafe\nstruct is that while it avoids this awkwardness, it creates new\nawkwardness when in a non-SHA-1 repository, where you have to keep\ntrack of whether you want to use the_unsafe_hash_algo or the SHA-256\nhash algo.\n\nIn that instance, it's not a matter of computing the same result two\ndifferent ways, but rather using the wrong hash function entirely. IOW,\nthe hashfile API would have to realize that when in SHA-256 mode, that\nit should *not* use the separate (hypothetical) unsafe SHA-1 struct.\n\n> > All that said, I do not think it buys us anything for \"real\" code. There\n> > the decision between safe/unsafe is in the context of how we are using\n> > it, and not based on some conditional. So while the code in this test\n> > helper is ugly, I don't think we'd ever write anything like that for\n> > real. So it may not be worth the effort to refactor into a more virtual\n> > object-oriented way.\n>\n> Yeah, I don't have a strong opinion one way or the other. I think, with\n> the limitation I mentioned above, it would probably require a decent\n> amount of refactoring if we took a different approach, and I'm fine with\n> going with Taylor's current approach unless he wants to do that\n> refactoring (in which case, great).\n\nI think it does buy you something for real code, which is that you don't\nhave to remember to consistently call the unsafe_ variants of all of the\nvarious function pointers.\n\nFor instance, if you do\n\n the_hash_algo->unsafe_init_fn(...);\n\nearly on, and then later on by mistake write:\n\n the_hash_algo->update_fn(...);\n\nThen your code is broken and will (as brian said) either in the best\ncase produce wrong results, or likely segfault.\n\nSo in that sense it is worth doing as it avoids the caller from having\nto keep track of what \"mode\" they are using the_hash_algo in. But let's\ntake it up later, I think.\n\nThanks,\nTaylor\n"},{"id":"506812","messageId":"Zy0yRLt5dM554yq1@nand.local","threadId":"62456","inReplyTo":"ZywgZt419SfPUVkQ@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-07T21:33:56Z","receivedAt":"2024-11-07T21:33:58Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 07, 2024 at 02:05:26AM +0000, brian m. carlson wrote:\n> On 2024-11-07 at 01:47:37, Jeff King wrote:\n> > I think this is a useful thing to have, and I didn't see anything wrong\n> > in the implementation. I did notice some oddities that existed before\n> > your series:\n> >\n> > 1. Why do we have \"test-tool sha256\" at all? Nobody in the test suite\n> > calls it. It feels like the whole test-sha1/sha256/hash split is\n> > overly complicated. A single \"test-tool hash\" seems like it would\n> > be simpler, and it could take an \"--algo\" parameter (and an\n> > \"--unsafe\" one). I guess in the end we end up with the same options\n> > ,but the proliferation of top-level test-tool commands seems ugly\n> > to me (likewise \"sha1_is_sha1dc\").\n>\n> I think we do in `pack_trailer` in `t/lib-pack.sh`, but not in a\n> greppable way:\n>\n> # Compute and append pack trailer to \"$1\"\n> pack_trailer () {\n> \ttest-tool $(test_oid algo) -b <\"$1\" >trailer.tmp &&\n> \tcat trailer.tmp >>\"$1\" &&\n> \trm -f trailer.tmp\n> }\n\nNice find. I think that it may be worth writing this as:\n\n case \"$(test_oid algo)\" in\n sha1)\n test-tool sha1 -b <\"$1\" >trailer.tmp\n ;;\n sha256)\n test-tool sha256 -b <\"$1\" >trailer.tmp\n ;;\n *)\n echo >&2 \"unknown algorithm: $(test_oid algo)\"\n exit 1\n ;;\n esac\n\nTo make it more greppable. Obviously the existing implementation is not\nwrong, but I do find it remarkably hard to search for ;-).\n\n> I think a single helper with `--algo` and `--unsafe` parameters would\n> also be fine and would probably be a little more tidy, as you mention.\n\nThat would be nice too.\n\nThanks,\nTaylor\n"},{"id":"506813","messageId":"Zy0yxDIgEmKtec/0@nand.local","threadId":"62456","inReplyTo":"20241107014737.GB961214@coredump.intra.peff.net","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-07T21:36:04Z","receivedAt":"2024-11-07T21:36:07Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Nov 06, 2024 at 08:47:37PM -0500, Jeff King wrote:\n> 2. You modified test-sha1.sh, but I've wondered if we should just\n> delete that script. It is not ever invoked in the test suite AFAIK.\n> If we want correctness tests, they should go into a real t[0-9]*.sh\n> script (though one imagines we exercise the sha1 code quite a lot\n> in the rest of the tests). And it starts with a weird ad-hoc\n> performance test that doesn't really tell us much. A t/perf test\n> would be better (if it is even worth measuring at all).\n\nYeah, I was sort of puzzled and thinking the same thing as I wrote the\npatch.\n\nI wouldn't be opposed to deleting it in the future, and certainly have\nno strong feelings about keeping it around in the meantime. It just\nseemed like the path of least resistance to bring it along for the\nsha1-unsafe ride instead of deleting it outright.\n\n> So I dunno. None of those are the fault of your series, but it is piling\n> on yet another test-tool command.\n\nYeah, I think there was a fair amount of interesting discussion about\npossible alternatives in this thread, which I am appreciative of.\n\nI think that if nobody has strong feelings or issues with the current\nimplementation to add the sha1-unsafe test-tool, that we should do so\nand take these patches.\n\nIn the future we can consider other things on top, like dropping the\ntest-sha1.sh script, having an unsafe pointer embedded within\nthe_hash_algo, or something else entirely. But those can be done on top,\nor not at all, and I don't want to hold up this series for them.\n\nThanks,\nTaylor\n"},{"id":"506816","messageId":"Zy1LQ7uJbut7JTHM@nand.local","threadId":"62456","inReplyTo":"Zy0xfVqtOXhEVDQB@nand.local","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-11-07T23:20:35Z","receivedAt":"2024-11-07T23:20:38Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Thu, Nov 07, 2024 at 04:30:37PM -0500, Taylor Blau wrote:\n> > > All that said, I do not think it buys us anything for \"real\" code. There\n> > > the decision between safe/unsafe is in the context of how we are using\n> > > it, and not based on some conditional. So while the code in this test\n> > > helper is ugly, I don't think we'd ever write anything like that for\n> > > real. So it may not be worth the effort to refactor into a more virtual\n> > > object-oriented way.\n> >\n> > Yeah, I don't have a strong opinion one way or the other. I think, with\n> > the limitation I mentioned above, it would probably require a decent\n> > amount of refactoring if we took a different approach, and I'm fine with\n> > going with Taylor's current approach unless he wants to do that\n> > refactoring (in which case, great).\n>\n> I think it does buy you something for real code, which is that you don't\n> have to remember to consistently call the unsafe_ variants of all of the\n> various function pointers.\n>\n> For instance, if you do\n>\n> the_hash_algo->unsafe_init_fn(...);\n>\n> early on, and then later on by mistake write:\n>\n> the_hash_algo->update_fn(...);\n>\n> Then your code is broken and will (as brian said) either in the best\n> case produce wrong results, or likely segfault.\n>\n> So in that sense it is worth doing as it avoids the caller from having\n> to keep track of what \"mode\" they are using the_hash_algo in. But let's\n> take it up later, I think.\n\nThe idea seemed too fun to play with, so I tried my hand at implementing\nit and was quite pleased with the result.\n\nThe WIP-quality patches are available in my tree under the\nwip/tb/unsafe-hash-algop[1] branch. The result, at least as it applies\nto the test-tool modified here, is quite pleasing IMHO:\n\n--- 8< ---\nSubject: [PATCH] t/helper/test-hash.c: use unsafe_hash_algo()\n\nSigned-off-by: Taylor Blau \n---\n t/helper/test-hash.c | 17 +++++------------\n 1 file changed, 5 insertions(+), 12 deletions(-)\n\ndiff --git a/t/helper/test-hash.c b/t/helper/test-hash.c\nindex d0ee668df95..aa82638c621 100644\n--- a/t/helper/test-hash.c\n+++ b/t/helper/test-hash.c\n@@ -9,6 +9,8 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)\n \tint binary = 0;\n \tchar *buffer;\n \tconst struct git_hash_algo *algop = &hash_algos[algo];\n+\tif (unsafe)\n+\t\talgop = unsafe_hash_algo(algop);\n\n \tif (ac == 2) {\n \t\tif (!strcmp(av[1], \"-b\"))\n@@ -27,10 +29,7 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)\n \t\t\tdie(\"OOPS\");\n \t}\n\n-\tif (unsafe)\n-\t\talgop->unsafe_init_fn(&ctx);\n-\telse\n-\t\talgop->init_fn(&ctx);\n+\talgop->init_fn(&ctx);\n\n \twhile (1) {\n \t\tssize_t sz, this_sz;\n@@ -49,15 +48,9 @@ int cmd_hash_impl(int ac, const char **av, int algo, int unsafe)\n \t\t}\n \t\tif (this_sz == 0)\n \t\t\tbreak;\n-\t\tif (unsafe)\n-\t\t\talgop->unsafe_update_fn(&ctx, buffer, this_sz);\n-\t\telse\n-\t\t\talgop->update_fn(&ctx, buffer, this_sz);\n+\t\talgop->update_fn(&ctx, buffer, this_sz);\n \t}\n-\tif (unsafe)\n-\t\talgop->unsafe_final_fn(hash, &ctx);\n-\telse\n-\t\talgop->final_fn(hash, &ctx);\n+\talgop->final_fn(hash, &ctx);\n\n \tif (binary)\n \t\tfwrite(hash, 1, algop->rawsz, stdout);\n\nbase-commit: d8c1fc78b57e02a140b5c363caaa14c2dc2bb274\nprerequisite-patch-id: 5df87a6ba8a596bc7834ce8b5a656a3c4f1a869f\nprerequisite-patch-id: c3f346e57f98b067eef8adf1e20c70ac23f41c36\nprerequisite-patch-id: c5d1ec4f5e9796c5c9232442a3fc3be79379b8c4\nprerequisite-patch-id: bdc2d6cbc23c467b24f184a889a5242e5c9fe774\n--\n2.47.0.238.g87615aecf12\n--- >8 ---\n\nTo be clear: I think that we should still take this series as-is, and\nI'll send the additional half dozen or so patches on top as a new series\ndependent on this one.\n\nThanks,\nTaylor\n\n[1]: https://github.com/ttaylorr/git/compare/tb/sha1-unsafe-helper...ttaylorr:git:wip/tb/unsafe-hash-algop\n"},{"id":"506877","messageId":"20241108172238.GB548990@coredump.intra.peff.net","threadId":"62456","inReplyTo":"ZywgZt419SfPUVkQ@tapette.crustytoothpaste.net","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-08T17:22:38Z","receivedAt":"2024-11-08T17:22:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 02:05:26AM +0000, brian m. carlson wrote:\n\n> > 1. Why do we have \"test-tool sha256\" at all? Nobody in the test suite\n> > calls it. It feels like the whole test-sha1/sha256/hash split is\n> > overly complicated. A single \"test-tool hash\" seems like it would\n> > be simpler, and it could take an \"--algo\" parameter (and an\n> > \"--unsafe\" one). I guess in the end we end up with the same options\n> > ,but the proliferation of top-level test-tool commands seems ugly\n> > to me (likewise \"sha1_is_sha1dc\").\n> \n> I think we do in `pack_trailer` in `t/lib-pack.sh`, but not in a\n> greppable way:\n> \n> # Compute and append pack trailer to \"$1\"\n> pack_trailer () {\n> \ttest-tool $(test_oid algo) -b <\"$1\" >trailer.tmp &&\n> \tcat trailer.tmp >>\"$1\" &&\n> \trm -f trailer.tmp\n> }\n> \n> When you posed the question above, I wasn't sure why I added this\n> functionality: was it just to test my SHA-256 implementation adequately\n> or did it have some actual utility in the testsuite? However, I knew if\n> it didn't appear straightforwardly in `git grep`, any uses would involve\n> `test_oid`, and boom, I was right.\n\nAh, good catch. Or good recall, I suppose. ;)\n\nIt feels a tiny bit hacky that we have to specify the algo here. If\nthere were a plumbing tool to work with the trailing hash of a\ncsum-file, then naturally it would use the repo's algorithm instead of\nhaving to specify it. And I have run into situations once or twice where\nthat might have been a useful debugging tool, rather than hacking\ntogether shell tools like dd. But since we have already done that\nhacking together in the test suite, and this is the only spot that has\nneeded it so far, it's probably worth letting sleeping dogs lie.\n\n> I think a single helper with `--algo` and `--unsafe` parameters would\n> also be fine and would probably be a little more tidy, as you mention.\n\nYup.\n\n-Peff\n"},{"id":"506878","messageId":"20241108172323.GC548990@coredump.intra.peff.net","threadId":"62456","inReplyTo":"Zy0yRLt5dM554yq1@nand.local","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-08T17:23:23Z","receivedAt":"2024-11-08T17:23:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 04:33:56PM -0500, Taylor Blau wrote:\n\n> > # Compute and append pack trailer to \"$1\"\n> > pack_trailer () {\n> > \ttest-tool $(test_oid algo) -b <\"$1\" >trailer.tmp &&\n> > \tcat trailer.tmp >>\"$1\" &&\n> > \trm -f trailer.tmp\n> > }\n> \n> Nice find. I think that it may be worth writing this as:\n> \n> case \"$(test_oid algo)\" in\n> sha1)\n> test-tool sha1 -b <\"$1\" >trailer.tmp\n> ;;\n> sha256)\n> test-tool sha256 -b <\"$1\" >trailer.tmp\n> ;;\n> *)\n> echo >&2 \"unknown algorithm: $(test_oid algo)\"\n> exit 1\n> ;;\n> esac\n\nIf it were \"test-tool hash --algo=$(test_oid algo)\", then presumably\ntest-tool would naturally do the same switch and error message\ninternally.\n\n-Peff\n"},{"id":"506879","messageId":"20241108172356.GD548990@coredump.intra.peff.net","threadId":"62456","inReplyTo":"Zy0yxDIgEmKtec/0@nand.local","subject":"Re: [PATCH v2 0/2] t/helper/test-tool: implement 'sha1-unsafe' helper","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-08T17:23:56Z","receivedAt":"2024-11-08T17:23:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 04:36:04PM -0500, Taylor Blau wrote:\n\n> > So I dunno. None of those are the fault of your series, but it is piling\n> > on yet another test-tool command.\n> \n> Yeah, I think there was a fair amount of interesting discussion about\n> possible alternatives in this thread, which I am appreciative of.\n> \n> I think that if nobody has strong feelings or issues with the current\n> implementation to add the sha1-unsafe test-tool, that we should do so\n> and take these patches.\n\nI'm OK with that direction.\n\n> In the future we can consider other things on top, like dropping the\n> test-sha1.sh script, having an unsafe pointer embedded within\n> the_hash_algo, or something else entirely. But those can be done on top,\n> or not at all, and I don't want to hold up this series for them.\n\nSounds good.\n\n-Peff\n"},{"id":"506881","messageId":"20241108172611.GE548990@coredump.intra.peff.net","threadId":"62456","inReplyTo":"Zy0xfVqtOXhEVDQB@nand.local","subject":"Re: [PATCH v2 1/2] t/helper/test-sha1: prepare for an unsafe mode","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-11-08T17:26:11Z","receivedAt":"2024-11-08T17:26:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2024 at 04:30:37PM -0500, Taylor Blau wrote:\n\n> > Yeah, I don't have a strong opinion one way or the other. I think, with\n> > the limitation I mentioned above, it would probably require a decent\n> > amount of refactoring if we took a different approach, and I'm fine with\n> > going with Taylor's current approach unless he wants to do that\n> > refactoring (in which case, great).\n> \n> I think it does buy you something for real code, which is that you don't\n> have to remember to consistently call the unsafe_ variants of all of the\n> various function pointers.\n> \n> For instance, if you do\n> \n> the_hash_algo->unsafe_init_fn(...);\n> \n> early on, and then later on by mistake write:\n> \n> the_hash_algo->update_fn(...);\n> \n> Then your code is broken and will (as brian said) either in the best\n> case produce wrong results, or likely segfault.\n\nYes, true. I sort of assume that all of those calls are happening within\none function (or at least a suite of related functions). Just because\nthere's an implicit context of \"I am computing the hash for an object\"\nversus \"I am computing a checksum\".\n\nAnd if we ever do move to splitting those further (to have crc32 or\nwhatever for the checksum), then having a git_hash_algo for that would\nseem even weirder.\n\n-Peff\n"}]}