{"thread":{"id":"47181","subject":"[PATCH] Make t4201-shortlog.sh test more robust","startedAt":"2017-11-12T15:33:01Z","lastAt":"2017-11-13T03:48:05Z","messageCount":3,"participants":["Charles Bailey","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"332342","messageId":"20171112152523.7186-1-charles@hashpling.org","threadId":"47181","inReplyTo":null,"subject":"[PATCH] Make t4201-shortlog.sh test more robust","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2017-11-12T15:25:23Z","receivedAt":"2017-11-12T15:33:01Z","isPatch":true,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"From: Charles Bailey <cbailey32@bloomberg.net>\n\nThe test for '--abbrev' in t4201-shortlog.sh assumes that the commits\ngenerated in the test can always be uniquely abbreviated to 5 hex digits\nbut this is not always the case. If you were unlucky and happened to run\nthe test at (say) Thu Jun 22 03:04:49 2017 +0000, you would find that\nthe first commit generated would collide with a tree object created\nlater in the same test.\n\nThis can be simulated in the version of t4201-shortlog.sh prior to this\ncommit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689\nafter sourcing test-lib.sh.\n\nChange the test to test --abbrev=35 instead of --abbrev=5 to almost\ncompletely avoid the possibility of a partial collision and add a call\nto test_tick in the setup to make the test repeatable.\n\nSigned-off-by: Charles Bailey <cbailey32@bloomberg.net>\n---\n t/t4201-shortlog.sh | 5 +++--\n t/test-lib.sh       | 7 ++++---\n 2 files changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\nindex 9df054b..da10478 100755\n--- a/t/t4201-shortlog.sh\n+++ b/t/t4201-shortlog.sh\n@@ -9,6 +9,7 @@ test_description='git shortlog\n . ./test-lib.sh\n \n test_expect_success 'setup' '\n+\ttest_tick &&\n \techo 1 >a1 &&\n \tgit add a1 &&\n \ttree=$(git write-tree) &&\n@@ -59,7 +60,7 @@ fuzz() {\n \tfile=$1 &&\n \tsed \"\n \t\t\ts/$_x40/OBJECT_NAME/g\n-\t\t\ts/$_x05/OBJID/g\n+\t\t\ts/$_x35/OBJID/g\n \t\t\ts/^ \\{6\\}[CTa].*/      SUBJECT/g\n \t\t\ts/^ \\{8\\}[^ ].*/        CONTINUATION/g\n \t\t\" <\"$file\" >\"$file.fuzzy\" &&\n@@ -81,7 +82,7 @@ test_expect_success 'pretty format' '\n \n test_expect_success '--abbrev' '\n \tsed s/SUBJECT/OBJID/ expect.template >expect &&\n-\tgit shortlog --format=\"%h\" --abbrev=5 HEAD >log &&\n+\tgit shortlog --format=\"%h\" --abbrev=35 HEAD >log &&\n \tfuzz log >log.predictable &&\n \ttest_cmp expect log.predictable\n '\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 9b61f16..116bd6a 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -175,9 +175,10 @@ esac\n \n # Convenience\n #\n-# A regexp to match 5 and 40 hexdigits\n+# A regexp to match 5, 35 and 40 hexdigits\n _x05='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'\n-_x40=\"$_x05$_x05$_x05$_x05$_x05$_x05$_x05$_x05\"\n+_x35=\"$_x05$_x05$_x05$_x05$_x05$_x05$_x05\"\n+_x40=\"$_x35$_x05\"\n \n # Zero SHA-1\n _z40=0000000000000000000000000000000000000000\n@@ -193,7 +194,7 @@ LF='\n # when case-folding filenames\n u200c=$(printf '\\342\\200\\214')\n \n-export _x05 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB\n+export _x05 _x35 _x40 _z40 LF u200c EMPTY_TREE EMPTY_BLOB\n \n # Each test should start with something like this, after copyright notices:\n #\n-- \n2.10.2\n\n"},{"id":"332347","messageId":"20171112161132.au26ywjeeipxsor4@sigill.intra.peff.net","threadId":"47181","inReplyTo":"20171112152523.7186-1-charles@hashpling.org","subject":"Re: [PATCH] Make t4201-shortlog.sh test more robust","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-12T16:11:32Z","receivedAt":"2017-11-12T16:11:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 12, 2017 at 03:25:23PM +0000, Charles Bailey wrote:\n\n> From: Charles Bailey <cbailey32@bloomberg.net>\n> \n> The test for '--abbrev' in t4201-shortlog.sh assumes that the commits\n> generated in the test can always be uniquely abbreviated to 5 hex digits\n> but this is not always the case. If you were unlucky and happened to run\n> the test at (say) Thu Jun 22 03:04:49 2017 +0000, you would find that\n> the first commit generated would collide with a tree object created\n> later in the same test.\n> \n> This can be simulated in the version of t4201-shortlog.sh prior to this\n> commit by setting GIT_COMMITTER_DATE and GIT_AUTHOR_DATE to 1498100689\n> after sourcing test-lib.sh.\n> \n> Change the test to test --abbrev=35 instead of --abbrev=5 to almost\n> completely avoid the possibility of a partial collision and add a call\n> to test_tick in the setup to make the test repeatable.\n\nThis generally makes sense to me, but I think there's one thing unsaid\nin this last paragraph: why is it OK to switch from 5 to 35?\n\nThe answer is that the test is just checking that --abbrev is respected,\nand doesn't care whether it's making things larger or smaller. There are\nother cases of --abbrev=5 in the test suite which might fall into the\nsame boat.\n\nYou're also really doing two things to fix the problem here, either one\nof which would have been sufficient: increasing the abbreviation size\nand using test_tick to get a deterministic run.\n\nI'm OK with doing that here, but some of the other --abbrev=5 cases\nmight already be fine due to using test_tick appropriately.\n\n> diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh\n> index 9df054b..da10478 100755\n> --- a/t/t4201-shortlog.sh\n> +++ b/t/t4201-shortlog.sh\n> @@ -9,6 +9,7 @@ test_description='git shortlog\n>  . ./test-lib.sh\n>  \n>  test_expect_success 'setup' '\n> +\ttest_tick &&\n\nCharles and I had a discussion off-list whether it's appropriate to\ntest_tick once, but not for each commit (simply because it's a minor\npain to remember to do so before each commit).\n\nDoing it once is enough to make the test deterministic, and for this\nparticular case we don't actually care at all whether all of the commits\nhave the exact same timestamp. So I think it's fine.\n\nOne option is to try replacing the ad-hoc commits with test_commit, but\nit turns out to be quite ugly in this case, as the tests depend on a lot\nof oddly-formatted commit messages.\n\n-Peff\n"},{"id":"332387","messageId":"xmqqr2t2alg2.fsf@gitster.mtv.corp.google.com","threadId":"47181","inReplyTo":"20171112161132.au26ywjeeipxsor4@sigill.intra.peff.net","subject":"Re: [PATCH] Make t4201-shortlog.sh test more robust","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T03:47:57Z","receivedAt":"2017-11-13T03:48:05Z","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> You're also really doing two things to fix the problem here, either one\n> of which would have been sufficient: increasing the abbreviation size\n> and using test_tick to get a deterministic run.\n> ...\n> Doing it once is enough to make the test deterministic, and for this\n> particular case we don't actually care at all whether all of the commits\n> have the exact same timestamp. So I think it's fine.\n\n;-)\n"}]}