{"thread":{"id":"46480","subject":"[RFD PATCH 0/2] How to migrate our test infrastructure away from sha1","startedAt":"2017-07-28T17:18:30Z","lastAt":"2017-07-31T23:55:10Z","messageCount":15,"participants":["Stefan Beller","Junio C Hamano","brian m. carlson","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"325222","messageId":"20170728171817.21458-1-sbeller@google.com","threadId":"46480","inReplyTo":null,"subject":"[RFD PATCH 0/2] How to migrate our test infrastructure away from sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-28T17:18:15Z","receivedAt":"2017-07-28T17:18:30Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Brian got the ball rolling with his object-id patches\nto fixup the code base, but we also need to discuss how\nwe approach our test infrastructure eventually.\n\nC.f.:\n* jb/t8008-cleanup (2017-07-26) 1 commit\n - t8008: rely on rev-parse'd HEAD instead of sha1 value\n* sb/t4005-modernize (Remove hard coded sha1 values.)\n\nThis presents an alternative approach in the second patch,\nwhich just marks tests as \"SHA1\" required instead of making\nthe test hash-agnostic.\n\nThe first patch is just an aid to show which tests need\naddressing, although there are some false positives (i.e.\nthe test fails for other reasons than the hash dependency,\nsuch as corruption of data.)\n\nStefan \n\nStefan Beller (2):\n  alter hash function: expose test dependencies on sha1\n  t6500: mark tests as SHA1 reliant\n\n sha1dc/sha1.c | 2 +-\n t/t6500-gc.sh | 6 +++---\n t/test-lib.sh | 4 ++++\n 3 files changed, 8 insertions(+), 4 deletions(-)\n\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"325223","messageId":"20170728171817.21458-2-sbeller@google.com","threadId":"46480","inReplyTo":"20170728171817.21458-1-sbeller@google.com","subject":"[PATCH 1/2] alter hash function: expose test dependencies on sha1","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-28T17:18:16Z","receivedAt":"2017-07-28T17:18:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"DO NOT APPLY.\n\nAlter the hash function such that with this patch\nany dependency on sha1 in tests will make the test\nfail. This patch applied on master yields this list:\n\n./t0000-basic.sh\n./t0002-gitfile.sh\n./t0005-signals.sh\n./t0013-sha1dc.sh\n./t0021-conversion.sh\n./t0090-cache-tree.sh\n./t1001-read-tree-m-2way.sh\n./t1007-hash-object.sh\n./t1011-read-tree-sparse-checkout.sh\n./t1013-read-tree-submodule.sh\n./t1100-commit-tree-options.sh\n./t1200-tutorial.sh\n./t1300-repo-config.sh\n./t1304-default-acl.sh\n./t1400-update-ref.sh\n./t1411-reflog-show.sh\n./t1450-fsck.sh\n./t1507-rev-parse-upstream.sh\n./t1512-rev-parse-disambiguation.sh\n./t1700-split-index.sh\n./t2011-checkout-invalid-head.sh\n./t2013-checkout-submodule.sh\n./t2015-checkout-unborn.sh\n./t2017-checkout-orphan.sh\n./t2022-checkout-paths.sh\n./t2101-update-index-reupdate.sh\n./t2107-update-index-basic.sh\n./t2203-add-intent.sh\n./t3033-merge-toplevel.sh\n./t3102-ls-tree-wildcards.sh\n./t3103-ls-tree-misc.sh\n./t3201-branch-contains.sh\n./t3301-notes.sh\n./t3305-notes-fanout.sh\n./t3306-notes-prune.sh\n./t3308-notes-merge.sh\n./t3309-notes-merge-auto-resolve.sh\n./t3310-notes-merge-manual-resolve.sh\n./t3311-notes-merge-fanout.sh\n./t3400-rebase.sh\n./t3404-rebase-interactive.sh\n./t3405-rebase-malformed.sh\n./t3408-rebase-multi-line.sh\n./t3415-rebase-autosquash.sh\n./t3419-rebase-patch-id.sh\n./t3421-rebase-topology-linear.sh\n./t3501-revert-cherry-pick.sh\n./t3502-cherry-pick-merge.sh\n./t3503-cherry-pick-root.sh\n./t3506-cherry-pick-ff.sh\n./t3509-cherry-pick-merge-df.sh\n./t3600-rm.sh\n./t3700-add.sh\n./t3701-add-interactive.sh\n./t3702-add-edit.sh\n./t3903-stash.sh\n./t3905-stash-include-untracked.sh\n./t4002-diff-basic.sh\n./t4007-rename-3.sh\n./t4008-diff-break-rewrite.sh\n./t4010-diff-pathspec.sh\n./t4011-diff-symlink.sh\n./t4013-diff-various.sh\n./t4014-format-patch.sh\n./t4015-diff-whitespace.sh\n./t4020-diff-external.sh\n./t4022-diff-rewrite.sh\n./t4029-diff-trailing-space.sh\n./t4030-diff-textconv.sh\n./t4033-diff-patience.sh\n./t4034-diff-words.sh\n./t4039-diff-assume-unchanged.sh\n./t4042-diff-textconv-caching.sh\n./t4044-diff-index-unique-abbrev.sh\n./t4045-diff-relative.sh\n./t4048-diff-combined-binary.sh\n./t4050-diff-histogram.sh\n./t4052-stat-output.sh\n./t4054-diff-bogus-tree.sh\n./t4060-diff-submodule-option-diff-format.sh\n./t4126-apply-empty.sh\n./t4151-am-abort.sh\n./t4202-log.sh\n./t4205-log-pretty-formats.sh\n./t4208-log-magic-pathspec.sh\n./t4211-line-log.sh\n./t4300-merge-tree.sh\n./t5150-request-pull.sh\n./t5300-pack-object.sh\n./t5306-pack-nobase.sh\n./t5308-pack-detect-duplicates.sh\n./t5309-pack-delta-cycles.sh\n./t5313-pack-bounds-checks.sh\n./t5512-ls-remote.sh\n./t5515-fetch-merge-logic.sh\n./t5516-fetch-push.sh\n./t5520-pull.sh\n./t5521-pull-options.sh\n./t6000-rev-list-misc.sh\n./t6012-rev-list-simplify.sh\n./t6020-merge-df.sh\n./t6022-merge-rename.sh\n./t6024-recursive-merge.sh\n./t6030-bisect-porcelain.sh\n./t6031-merge-filemode.sh\n./t6035-merge-dir-to-symlink.sh\n./t6300-for-each-ref.sh\n./t6500-gc.sh\n./t7003-filter-branch.sh\n./t7012-skip-worktree-writing.sh\n./t7063-status-untracked-cache.sh\n./t7102-reset.sh\n./t7106-reset-unborn-branch.sh\n./t7112-reset-submodule.sh\n./t7201-co.sh\n./t7400-submodule-basic.sh\n./t7506-status-submodule.sh\n./t7507-commit-verbose.sh\n./t7508-status.sh\n./t7600-merge.sh\n./t7607-merge-overwrite.sh\n./t7609-merge-co-error-msgs.sh\n./t8008-blame-formats.sh\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n sha1dc/sha1.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\nindex 25eded1399..e18acee9ca 100644\n--- a/sha1dc/sha1.c\n+++ b/sha1dc/sha1.c\n@@ -1756,7 +1756,7 @@ static void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])\n void SHA1DCInit(SHA1_CTX* ctx)\n {\n \tctx->total = 0;\n-\tctx->ihv[0] = 0x67452301;\n+\tctx->ihv[0] = 0x07452301;\n \tctx->ihv[1] = 0xEFCDAB89;\n \tctx->ihv[2] = 0x98BADCFE;\n \tctx->ihv[3] = 0x10325476;\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"325224","messageId":"20170728171817.21458-3-sbeller@google.com","threadId":"46480","inReplyTo":"20170728171817.21458-1-sbeller@google.com","subject":"[PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-28T17:18:17Z","receivedAt":"2017-07-28T17:18:36Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The first test marked relies on hard coded sha1:\n\n\t# We need to create two object whose sha1s start with 17\n\t# since this is what git gc counts.  As it happens, these\n\t# two blobs will do so.\n\ttest_commit 263 &&\n\ttest_commit 410 &&\n\nThe next two seem to rely on state from the first one, I did not\ninvestigate.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n t/t6500-gc.sh | 6 +++---\n t/test-lib.sh | 4 ++++\n 2 files changed, 7 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\nindex 41b0be575d..3900baa01d 100755\n--- a/t/t6500-gc.sh\n+++ b/t/t6500-gc.sh\n@@ -43,7 +43,7 @@ test_expect_success 'gc is not aborted due to a stale symref' '\n \t)\n '\n \n-test_expect_success 'auto gc with too many loose objects does not attempt to create bitmaps' '\n+test_expect_success SHA1 'auto gc with too many loose objects does not attempt to create bitmaps' '\n \ttest_config gc.auto 3 &&\n \ttest_config gc.autodetach false &&\n \ttest_config pack.writebitmaps true &&\n@@ -77,7 +77,7 @@ run_and_wait_for_auto_gc () {\n \tdoesnt_matter=$(git gc --auto 9>&1)\n }\n \n-test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n+test_expect_success SHA1 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n \ttest_commit foo &&\n \ttest_commit bar &&\n \tgit repack &&\n@@ -95,7 +95,7 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n \ttest_line_count = 1 packs\n '\n \n-test_expect_success 'background auto gc respects lock for all operations' '\n+test_expect_success SHA1 'background auto gc respects lock for all operations' '\n \t# make sure we run a background auto-gc\n \ttest_commit make-pack &&\n \tgit repack &&\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 1b6e53f78a..a5a54c6d4a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1127,6 +1127,10 @@ test_lazy_prereq JGIT '\n \ttype jgit\n '\n \n+test_lazy_prereq SHA1 '\n+\tfalse\n+'\n+\n # SANITY is about \"can you correctly predict what the filesystem would\n # do by only looking at the permission bits of the files and\n # directories?\"  A typical example of !SANITY is running the test\n-- \n2.14.0.rc0.3.g6c2e499285\n\n"},{"id":"325225","messageId":"CAPc5daXgXpwOKB2J7cz7fFYNd3GuRK7fqeROhwouyaCCGU1ToA@mail.gmail.com","threadId":"46480","inReplyTo":"20170728171817.21458-3-sbeller@google.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-28T17:59:07Z","receivedAt":"2017-07-28T17:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"There is another one that is better suited for this demonstration patch,\nwhose sole point is about creating bunch of objects that prefix conflicts\nwith each other to test the abbreviation machinery.\n\n\n\nOn Fri, Jul 28, 2017 at 10:18 AM, Stefan Beller <sbeller@google.com> wrote:\n> The first test marked relies on hard coded sha1:\n>\n>         # We need to create two object whose sha1s start with 17\n>         # since this is what git gc counts.  As it happens, these\n>         # two blobs will do so.\n>         test_commit 263 &&\n>         test_commit 410 &&\n>\n> The next two seem to rely on state from the first one, I did not\n> investigate.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  t/t6500-gc.sh | 6 +++---\n>  t/test-lib.sh | 4 ++++\n>  2 files changed, 7 insertions(+), 3 deletions(-)\n>\n> diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh\n> index 41b0be575d..3900baa01d 100755\n> --- a/t/t6500-gc.sh\n> +++ b/t/t6500-gc.sh\n> @@ -43,7 +43,7 @@ test_expect_success 'gc is not aborted due to a stale symref' '\n>         )\n>  '\n>\n> -test_expect_success 'auto gc with too many loose objects does not attempt to create bitmaps' '\n> +test_expect_success SHA1 'auto gc with too many loose objects does not attempt to create bitmaps' '\n>         test_config gc.auto 3 &&\n>         test_config gc.autodetach false &&\n>         test_config pack.writebitmaps true &&\n> @@ -77,7 +77,7 @@ run_and_wait_for_auto_gc () {\n>         doesnt_matter=$(git gc --auto 9>&1)\n>  }\n>\n> -test_expect_success 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n> +test_expect_success SHA1 'background auto gc does not run if gc.log is present and recent but does if it is old' '\n>         test_commit foo &&\n>         test_commit bar &&\n>         git repack &&\n> @@ -95,7 +95,7 @@ test_expect_success 'background auto gc does not run if gc.log is present and re\n>         test_line_count = 1 packs\n>  '\n>\n> -test_expect_success 'background auto gc respects lock for all operations' '\n> +test_expect_success SHA1 'background auto gc respects lock for all operations' '\n>         # make sure we run a background auto-gc\n>         test_commit make-pack &&\n>         git repack &&\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 1b6e53f78a..a5a54c6d4a 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1127,6 +1127,10 @@ test_lazy_prereq JGIT '\n>         type jgit\n>  '\n>\n> +test_lazy_prereq SHA1 '\n> +       false\n> +'\n> +\n>  # SANITY is about \"can you correctly predict what the filesystem would\n>  # do by only looking at the permission bits of the files and\n>  # directories?\"  A typical example of !SANITY is running the test\n> --\n> 2.14.0.rc0.3.g6c2e499285\n>\n"},{"id":"325229","messageId":"CAGZ79kZxVhZjFmTs5oAXh_vBNix4G=LtL8zCjrFCnqS+5pkDwQ@mail.gmail.com","threadId":"46480","inReplyTo":"CAPc5daXgXpwOKB2J7cz7fFYNd3GuRK7fqeROhwouyaCCGU1ToA@mail.gmail.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-28T21:43:49Z","receivedAt":"2017-07-28T21:43:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Jul 28, 2017 at 10:59 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> There is another one that is better suited for this demonstration patch,\n> whose sole point is about creating bunch of objects that prefix conflicts\n> with each other to test the abbreviation machinery.\n\n'Another one' meaning in another test file? I'll go look for these\ntests.\n\nThe first patch produces lots of false positives, so maybe I'll need\nto work on that (is there another code path for sha1?).\n\nThanks,\nStefan\n"},{"id":"325230","messageId":"xmqq7eysmdoq.fsf@gitster.mtv.corp.google.com","threadId":"46480","inReplyTo":"20170728171817.21458-2-sbeller@google.com","subject":"Re: [PATCH 1/2] alter hash function: expose test dependencies on sha1","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-28T21:52:53Z","receivedAt":"2017-07-28T21:53:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> DO NOT APPLY.\n>\n> Alter the hash function such that with this patch\n> any dependency on sha1 in tests will make the test\n> fail. This patch applied on master yields this list:\n>\n> ./t0000-basic.sh\n> ....\n> ./t8008-blame-formats.sh\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  sha1dc/sha1.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nEverybody knew and said that we need to make tests less dependent on\nthe exact hash and in such a way that the ones that currently do\nwould test the morally equivalent thing with a new hash before we\ncan go forward.  And everybody said that one way to start that is to\nbuild a Git with different hash function and trying to run tests.\n\nThis is the first report of doing so, and the large list above is\nthe biggest contribution this message makes to the \"let's wean\nourselves off of SHA-1\" journey.\n\nThanks for starting this.\n\n>\n> diff --git a/sha1dc/sha1.c b/sha1dc/sha1.c\n> index 25eded1399..e18acee9ca 100644\n> --- a/sha1dc/sha1.c\n> +++ b/sha1dc/sha1.c\n> @@ -1756,7 +1756,7 @@ static void sha1_process(SHA1_CTX* ctx, const uint32_t block[16])\n>  void SHA1DCInit(SHA1_CTX* ctx)\n>  {\n>  \tctx->total = 0;\n> -\tctx->ihv[0] = 0x67452301;\n> +\tctx->ihv[0] = 0x07452301;\n>  \tctx->ihv[1] = 0xEFCDAB89;\n>  \tctx->ihv[2] = 0x98BADCFE;\n>  \tctx->ihv[3] = 0x10325476;\n"},{"id":"325232","messageId":"xmqq379gmco6.fsf@gitster.mtv.corp.google.com","threadId":"46480","inReplyTo":"20170728171817.21458-3-sbeller@google.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-28T22:14:49Z","receivedAt":"2017-07-28T22:14:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> The first test marked relies on hard coded sha1:\n>\n> \t# We need to create two object whose sha1s start with 17\n> \t# since this is what git gc counts.  As it happens, these\n> \t# two blobs will do so.\n> \ttest_commit 263 &&\n> \ttest_commit 410 &&\n>\n> The next two seem to rely on state from the first one, I did not\n> investigate.\n\nI am moderately negative on this approach, if it is meant to suggest\nthe final shape of our test suite patch 1/2 started.\n\nThis script may be a good example you can use to demonstrate a much\nbetter approach.  As the above comment in the test shows, we want to\ncreate two objects whose object names begin with \"17\", and running\ntest_commit with 263 and 410 at this point in the test was a way to\nachieve that when Git uses SHA-1 as its hash.\n\nWhen we use a hash different from SHA-1, the exact strings 263 and\n410 may change, but we should be able to find two other strings that\nhas the same property (i.e. they results in objects that share the\nprefix \"17\").  Perhaps a better way forward for this kind of test is\nto parameterize these hardcoded constants and make it easier to use\ndifferent values without having to change the rest of the script\nwhen we switch the hash function?  So perhaps have something like\n\n\tcase \"$GIT_HASH_FUNCTION\" in\n\tSHA-1)\t\n\t\tTEST_17_1=\"263 410\" ;;\n\tCORRUPT-SHA-1)\t\n\t\tTEST_17_1=\"something else\" ;;\n        esac\n\nnear the top of the script and update the above two with:\n\n\tfor token in $TEST_17_1\n\tdo\n\t\ttest_commit \"$token\" || return 1\n\tdone &&\n\nwould prepare us to switch to SHA-256 or whatever hash function we\nchoose to use in the future?\n\nt1512 is another example with full of such tests that we would want\nto keep moral equivalents in the world with a different hash,\ninstead of reducing the test coverage.\n"},{"id":"325249","messageId":"20170729175833.4idan3befldn5vgp@genre.crustytoothpaste.net","threadId":"46480","inReplyTo":"xmqq379gmco6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-07-29T17:58:33Z","receivedAt":"2017-07-29T17:58:43Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Fri, Jul 28, 2017 at 03:14:49PM -0700, Junio C Hamano wrote:\n> Stefan Beller <sbeller@google.com> writes:\n> \n> > The first test marked relies on hard coded sha1:\n> >\n> > \t# We need to create two object whose sha1s start with 17\n> > \t# since this is what git gc counts.  As it happens, these\n> > \t# two blobs will do so.\n> > \ttest_commit 263 &&\n> > \ttest_commit 410 &&\n> >\n> > The next two seem to rely on state from the first one, I did not\n> > investigate.\n> \n> I am moderately negative on this approach, if it is meant to suggest\n> the final shape of our test suite patch 1/2 started.\n> \n> This script may be a good example you can use to demonstrate a much\n> better approach.  As the above comment in the test shows, we want to\n> create two objects whose object names begin with \"17\", and running\n> test_commit with 263 and 410 at this point in the test was a way to\n> achieve that when Git uses SHA-1 as its hash.\n> \n> When we use a hash different from SHA-1, the exact strings 263 and\n> 410 may change, but we should be able to find two other strings that\n> has the same property (i.e. they results in objects that share the\n> prefix \"17\").  Perhaps a better way forward for this kind of test is\n> to parameterize these hardcoded constants and make it easier to use\n> different values without having to change the rest of the script\n> when we switch the hash function?  So perhaps have something like\n> \n> \tcase \"$GIT_HASH_FUNCTION\" in\n> \tSHA-1)\t\n> \t\tTEST_17_1=\"263 410\" ;;\n> \tCORRUPT-SHA-1)\t\n> \t\tTEST_17_1=\"something else\" ;;\n>         esac\n\nOne approach I had considered taking is having a helper of some sort\nthat wrapped a simple key/value store.  We could pass the wrapper the\nSHA-1 value (or, if necessary, an arbitrary key) and have it return the\nproper value based on the given hash function.\n\nThat does have the downsides that the values may not present in the\ntests themselves, and that people adding new tests will of course need\nto run the test suite twice.  But it does make the tests easier to read.\n\nOpinions on the desirability of this approach are of course welcome.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"325283","messageId":"xmqqini9k4cx.fsf@gitster.mtv.corp.google.com","threadId":"46480","inReplyTo":"20170729175833.4idan3befldn5vgp@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-30T21:21:50Z","receivedAt":"2017-07-30T21:22:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n\n> One approach I had considered taking is having a helper of some sort\n> that wrapped a simple key/value store.  We could pass the wrapper the\n> SHA-1 value (or, if necessary, an arbitrary key) and have it return the\n> proper value based on the given hash function.\n>\n> That does have the downsides that the values may not present in the\n> tests themselves, and that people adding new tests will of course need\n> to run the test suite twice.  But it does make the tests easier to read.\n>\n> Opinions on the desirability of this approach are of course welcome.\n\nI am not quite sure if I follow.  There was a proposal to tweak the\ncommit format that uses the new hash in such a way that we can tell\nwhat SHA-1 would have been used if everything were SHA-1 (I think it\nwas from Jonathan, but I may be mistaken), and I recall that\ngenerally the list were receptive to the idea.  But I have a feeling\nthat your \"helper of some sort\" is something else.\n\nIf your <key,value> is about letting us store something like\n\n - If you hash \"hello\\n\" the resulting blob in SHA-1 world has this\n   object name, and with that, you can find out the equivalent\n   object name in SHA-256 world.\n\n - If you have a tree with the above blob at path P and nothing\n   else, then the object name of that tree in the SHA-1 world and\n   SHA-256 world are different and we can map between them.\n\n - Likewise for a commit that points at the above tree with fixed\n   date, author and message.\n\nI am not sure how much it would help.  Are you aiming to make it\neasier and more structured to create a patch like what Stefan did\nrecently for t8008 in 0ba9c9a0 (\"t8008: rely on rev-parse'd HEAD\ninstead of sha1 value\", 2017-07-26)?\n\nI also suspect that tests like t1512 and t6500 would not benefit\nthat much from such a mapping.  In these tests, the object names by\nthemselves are not interesting.  These tests are about what Git does\nwhen the names of the objects involved in them happen to share a\ncertain prefix.  We are not interested in using the same payload in\nthese tests using different hash, which is likely to destroy the\naspect of the object names that these tests are interested in,\nnamely, they share the same prefix.  When updating these tests to\nadjust for the SHA-256 world, we want to preserve that the resulting\nobject names happen to share the same prefix by tweaking the payload\nstrings (i.e. \"263 and 410\" in t6500 are chosen to cause the\nresulting objects to share \"17/\" prefix and fall inside a same\nfan-out directory as loose objects.  We want to choose different\nstrings so that the names of the resulting objects share the same\nprefix, not necessarily \"17/\" but preferrably so, in the SHA-256\nworld.  Similarly, Random-looking strings like \"a2onsxbvj\" in t1512\nare chosen to cause blobs, trees, commits and tags that are involved\nin the test to all share the same prefix \"000000...\"; we want to\nchoose different set of such random-looking strings that cause all\nobjects involved to hash to the same prefix, not necessarily but\npreferrably \"000000...\").\n\n\n"},{"id":"325285","messageId":"20170730230019.ch74ska6wgbrh7ql@genre.crustytoothpaste.net","threadId":"46480","inReplyTo":"xmqqini9k4cx.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-07-30T23:00:19Z","receivedAt":"2017-07-30T23:00:31Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jul 30, 2017 at 02:21:50PM -0700, Junio C Hamano wrote:\n> \"brian m. carlson\" <sandals@crustytoothpaste.net> writes:\n> \n> > One approach I had considered taking is having a helper of some sort\n> > that wrapped a simple key/value store.  We could pass the wrapper the\n> > SHA-1 value (or, if necessary, an arbitrary key) and have it return the\n> > proper value based on the given hash function.\n> >\n> > That does have the downsides that the values may not present in the\n> > tests themselves, and that people adding new tests will of course need\n> > to run the test suite twice.  But it does make the tests easier to read.\n> >\n> > Opinions on the desirability of this approach are of course welcome.\n> \n> I am not quite sure if I follow.  There was a proposal to tweak the\n> commit format that uses the new hash in such a way that we can tell\n> what SHA-1 would have been used if everything were SHA-1 (I think it\n> was from Jonathan, but I may be mistaken), and I recall that\n> generally the list were receptive to the idea.  But I have a feeling\n> that your \"helper of some sort\" is something else.\n> \n> If your <key,value> is about letting us store something like\n> \n>  - If you hash \"hello\\n\" the resulting blob in SHA-1 world has this\n>    object name, and with that, you can find out the equivalent\n>    object name in SHA-256 world.\n> \n>  - If you have a tree with the above blob at path P and nothing\n>    else, then the object name of that tree in the SHA-1 world and\n>    SHA-256 world are different and we can map between them.\n> \n>  - Likewise for a commit that points at the above tree with fixed\n>    date, author and message.\n> \n> I am not sure how much it would help.  Are you aiming to make it\n> easier and more structured to create a patch like what Stefan did\n> recently for t8008 in 0ba9c9a0 (\"t8008: rely on rev-parse'd HEAD\n> instead of sha1 value\", 2017-07-26)?\n\nYes, basically, but a bit more generally.  There will always be cases in\nwhich we need to specify an object ID or an arbitrary string and the\nbehavior will need to vary based on the hash.  That can be something\nlike, in this case, the two blob contents that would have the similar\nprefix.\n\nSo in this case, we pass the helper the string \"263 410\" and get back a\nvalue for either the hacked SHA-1 hash or the SHA-256 or whatever we're\nusing.\n\nThis was basically a nicer way of wrapping the case statement that you\nhad given as an example.  Of course, it doesn't relieve us of doing the\nhard work of analyzing the tests, which Stefan is doing, and with which\nI don't want to interfere.\n\nIt was simply a proposal for a future direction which we could take if\nwe found ourselves needing to write a large number of hash-specific case\nstatements.  I'm happy to wait to actually implement that code until we\ndecide we need such a thing.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"325286","messageId":"20170730232427.vrqrrhlsap55ax7t@genre.crustytoothpaste.net","threadId":"46480","inReplyTo":"20170730230019.ch74ska6wgbrh7ql@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-07-30T23:24:27Z","receivedAt":"2017-07-30T23:27:48Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:\n> Yes, basically, but a bit more generally.  There will always be cases in\n> which we need to specify an object ID or an arbitrary string and the\n> behavior will need to vary based on the hash.  That can be something\n> like, in this case, the two blob contents that would have the similar\n> prefix.\n> \n> So in this case, we pass the helper the string \"263 410\" and get back a\n> value for either the hacked SHA-1 hash or the SHA-256 or whatever we're\n> using.\n\nI realize this was worded poorly.  So for my example, in this case, we'd\ndo:\n\ntest-helper-hash-string \"263 410\"\n\nFor SHA-1, we'd get \"263 410\".  For SHA-256, we'd get \"313 481\" (which,\nas SHA-256 blobs, both start with \"17\" in their hex representation).\nPresumably we'd read some environment variable to determine the proper\nvalue.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"325313","messageId":"CACBZZX43JFOAOTffWVEMT1fPuzAiZnYi4JoE55QWquZ4kLA2Hg@mail.gmail.com","threadId":"46480","inReplyTo":"20170730232427.vrqrrhlsap55ax7t@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-07-31T20:17:52Z","receivedAt":"2017-07-31T20:18:19Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, Jul 31, 2017 at 1:24 AM, brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:\n>> Yes, basically, but a bit more generally.  There will always be cases in\n>> which we need to specify an object ID or an arbitrary string and the\n>> behavior will need to vary based on the hash.  That can be something\n>> like, in this case, the two blob contents that would have the similar\n>> prefix.\n>>\n>> So in this case, we pass the helper the string \"263 410\" and get back a\n>> value for either the hacked SHA-1 hash or the SHA-256 or whatever we're\n>> using.\n>\n> I realize this was worded poorly.  So for my example, in this case, we'd\n> do:\n>\n> test-helper-hash-string \"263 410\"\n>\n> For SHA-1, we'd get \"263 410\".  For SHA-256, we'd get \"313 481\" (which,\n> as SHA-256 blobs, both start with \"17\" in their hex representation).\n> Presumably we'd read some environment variable to determine the proper\n> value.\n\nI've been mostly out of the loop on this hash transition plan, but\ndon't we expect to be compiling a git that knows about both SHA-1 and\nwhatever the $newhash is? If so it seems better to just test all N\nhashes we have:\n\n    test_expect_success_hash $desc_description '\n        hash_value=$(test-helper-hash-string $CURRENT_HASH)\n        ....\n    '\n\nThen test_expect_success_hash would run N times for the N hashes we have.\n\nThis would obviously be slightly more hassle to write & convert, but I\nthink it would be worth it, particularly with something like Travis\nwhere we can test all hashes, instead of being in some mode where we\nfragment on all of hashes/gettext poison and whatever other\ncompilation option we have that really requires compiling a new git\nversion...\n"},{"id":"325315","messageId":"CAGZ79kaBUpVwymjmzmOtHgfiWyxPQZCtFOX_Zm5v7N-H+C9cNA@mail.gmail.com","threadId":"46480","inReplyTo":"20170730232427.vrqrrhlsap55ax7t@genre.crustytoothpaste.net","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-31T20:26:40Z","receivedAt":"2017-07-31T20:26:47Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sun, Jul 30, 2017 at 4:24 PM, brian m. carlson\n<sandals@crustytoothpaste.net> wrote:\n> On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:\n>> Yes, basically, but a bit more generally.  There will always be cases in\n>> which we need to specify an object ID or an arbitrary string and the\n>> behavior will need to vary based on the hash.  That can be something\n>> like, in this case, the two blob contents that would have the similar\n>> prefix.\n>>\n>> So in this case, we pass the helper the string \"263 410\" and get back a\n>> value for either the hacked SHA-1 hash or the SHA-256 or whatever we're\n>> using.\n>\n> I realize this was worded poorly.  So for my example, in this case, we'd\n> do:\n>\n> test-helper-hash-string \"263 410\"\n>\n> For SHA-1, we'd get \"263 410\".  For SHA-256, we'd get \"313 481\" (which,\n> as SHA-256 blobs, both start with \"17\" in their hex representation).\n> Presumably we'd read some environment variable to determine the proper\n> value.\n\nThis is what Junio proposed in the first message, except that we defer that\nto a shell script as for each test we may need different things, so a helper may\nbe of little value?\n"},{"id":"325317","messageId":"CAGZ79kZFkNu5zOZk_+ckkVNrU=7chHezPy0rA_A0-CjxFhucAw@mail.gmail.com","threadId":"46480","inReplyTo":"CACBZZX43JFOAOTffWVEMT1fPuzAiZnYi4JoE55QWquZ4kLA2Hg@mail.gmail.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-07-31T20:30:21Z","receivedAt":"2017-07-31T20:30:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Jul 31, 2017 at 1:17 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Mon, Jul 31, 2017 at 1:24 AM, brian m. carlson\n> <sandals@crustytoothpaste.net> wrote:\n>> On Sun, Jul 30, 2017 at 11:00:19PM +0000, brian m. carlson wrote:\n>>> Yes, basically, but a bit more generally.  There will always be cases in\n>>> which we need to specify an object ID or an arbitrary string and the\n>>> behavior will need to vary based on the hash.  That can be something\n>>> like, in this case, the two blob contents that would have the similar\n>>> prefix.\n>>>\n>>> So in this case, we pass the helper the string \"263 410\" and get back a\n>>> value for either the hacked SHA-1 hash or the SHA-256 or whatever we're\n>>> using.\n>>\n>> I realize this was worded poorly.  So for my example, in this case, we'd\n>> do:\n>>\n>> test-helper-hash-string \"263 410\"\n>>\n>> For SHA-1, we'd get \"263 410\".  For SHA-256, we'd get \"313 481\" (which,\n>> as SHA-256 blobs, both start with \"17\" in their hex representation).\n>> Presumably we'd read some environment variable to determine the proper\n>> value.\n>\n> I've been mostly out of the loop on this hash transition plan, but\n> don't we expect to be compiling a git that knows about both SHA-1 and\n> whatever the $newhash is?\n\nThat is my understanding as well.\n\n> If so it seems better to just test all N\n> hashes we have:\n>\n>     test_expect_success_hash $desc_description '\n>         hash_value=$(test-helper-hash-string $CURRENT_HASH)\n>         ....\n>     '\n>\n> Then test_expect_success_hash would run N times for the N hashes we have.\n\nI think that is just adding more workload without furthering the stated goal\nwhich is usually reached with just one hash function. The tests we're talking\nabout here are not trying to test correctness of hashes but some other\nfunctionality\n(correct abbreviation length, collisions in prefix, etc.) that would not change\ndepending on the hash function used, I imagine.\n\nFor t0000 we want to have multiple versions, one for each hash.\n\n> This would obviously be slightly more hassle to write & convert, but I\n> think it would be worth it, particularly with something like Travis\n> where we can test all hashes, instead of being in some mode where we\n> fragment on all of hashes/gettext poison and whatever other\n> compilation option we have that really requires compiling a new git\n> version...\n"},{"id":"325360","messageId":"20170731235459.stlzm6wpi6adf7v2@genre.crustytoothpaste.net","threadId":"46480","inReplyTo":"CAGZ79kaBUpVwymjmzmOtHgfiWyxPQZCtFOX_Zm5v7N-H+C9cNA@mail.gmail.com","subject":"Re: [PATCH 2/2] t6500: mark tests as SHA1 reliant","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-07-31T23:54:59Z","receivedAt":"2017-07-31T23:55:10Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Jul 31, 2017 at 01:26:40PM -0700, Stefan Beller wrote:\n> On Sun, Jul 30, 2017 at 4:24 PM, brian m. carlson\n> <sandals@crustytoothpaste.net> wrote:\n> > I realize this was worded poorly.  So for my example, in this case, we'd\n> > do:\n> >\n> > test-helper-hash-string \"263 410\"\n> >\n> > For SHA-1, we'd get \"263 410\".  For SHA-256, we'd get \"313 481\" (which,\n> > as SHA-256 blobs, both start with \"17\" in their hex representation).\n> > Presumably we'd read some environment variable to determine the proper\n> > value.\n> \n> This is what Junio proposed in the first message, except that we defer that\n> to a shell script as for each test we may need different things, so a helper may\n> be of little value?\n\nI think a shell script may end up being a fine helper for our needs.  In\nany case, I think we're in violent agreement.  My proposal was just an\nidea I had considered when thinking about how to do this, and any\nsuitable solution is fine with me.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"}]}