{"thread":{"id":"30285","subject":"[PATCH nd/threaded-index-pack] index-pack: disable threading if NO_PREAD is defined","startedAt":"2012-04-19T14:05:29Z","lastAt":"2012-04-20T19:49:42Z","messageCount":4,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Johannes Sixt"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"189705","messageId":"1334844329-24557-1-git-send-email-pclouds@gmail.com","threadId":"30285","inReplyTo":null,"subject":"[PATCH nd/threaded-index-pack] index-pack: disable threading if NO_PREAD is defined","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-04-19T14:05:29Z","receivedAt":"2012-04-19T14:05:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"NO_PREAD simulates pread() as a sequence of seek, read, seek in\ncompat/pread.c. The simulation is not thread-safe because another\nthread could move the file offset away in the middle of pread\noperation. Do not allow threading in that case.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/index-pack.c |    5 +++++\n 1 files changed, 5 insertions(+), 0 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 847dbb3..c1c3c81 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -39,6 +39,11 @@ struct base_data {\n \tint ofs_first, ofs_last;\n };\n \n+#if !defined(NO_PTHREADS) && defined(NO_PREAD)\n+/* NO_PREAD uses compat/pread.c, which is not thread-safe. Disable threading. */\n+#define NO_PTHREADS\n+#endif\n+\n struct thread_local {\n #ifndef NO_PTHREADS\n \tpthread_t thread;\n-- \n1.7.8.36.g69ee2\n"},{"id":"189727","messageId":"xmqqhawfv4cv.fsf@junio.mtv.corp.google.com","threadId":"30285","inReplyTo":"1334844329-24557-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH nd/threaded-index-pack] index-pack: disable threading if NO_PREAD is defined","fromName":"Junio C Hamano","fromEmail":"jch@google.com","sentAt":"2012-04-19T20:44:32Z","receivedAt":"2012-04-19T20:44:32Z","isPatch":true,"sender":{"key":"jch@google.com","avatar":null},"body":"Thanks; will queue.\n"},{"id":"189740","messageId":"4F910145.5030102@viscovery.net","threadId":"30285","inReplyTo":"1334844329-24557-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH nd/threaded-index-pack] index-pack: disable threading if NO_PREAD is defined","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-04-20T06:25:09Z","receivedAt":"2012-04-20T06:25:09Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 4/19/2012 16:05, schrieb Nguyễn Thái Ngọc Duy:\n> NO_PREAD simulates pread() as a sequence of seek, read, seek in\n> compat/pread.c. The simulation is not thread-safe because another\n> thread could move the file offset away in the middle of pread\n> operation. Do not allow threading in that case.\n\nUnsurprisingly, this fixes the breakage for me.\n\nI used the attached patch to keep t9300 running when the breakage\nwas detected.\n\n--- 8< ---\nFrom: Johannes Sixt <j6t@kdbg.org>\nSubject: [PATCH] t9300-fast-import: avoid 'exit' in test_expect_success snippets\n\nExiting from a for-loop early using '|| break' does not propagate the\nfailure code, and for this reason, the tests used just 'exit'. But this\nends the test script with 'FATAL: Unexpected exit code 1' in the case of\na failed test.\n\nFix this by moving the loop into a shell function, from which we can\nsimply return early.\n\nWhile at it, modernize the style of the affected test cases.\n\nSigned-off-by: Johannes Sixt <j6t@kdbg.org>\n---\n t/t9300-fast-import.sh |   88 +++++++++++++++++++++++++++++-------------------\n 1 file changed, 54 insertions(+), 34 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 0f5b5e5..8d7be67 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -24,6 +24,13 @@ head_c () {\n \t' - \"$1\"\n }\n \n+verify_packs () {\n+\tfor p in .git/objects/pack/*.pack\n+\tdo\n+\t\tgit verify-pack \"$@\" \"$p\" || return\n+\tdone\n+}\n+\n file2_data='file2\n second line of EOF'\n \n@@ -105,9 +112,10 @@ test_expect_success \\\n     'A: create pack from stdin' \\\n     'git fast-import --export-marks=marks.out <input &&\n \t git whatchanged master'\n-test_expect_success \\\n-\t'A: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'A: verify pack' '\n+\tverify_packs\n+'\n \n cat >expect <<EOF\n author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n@@ -252,9 +260,11 @@ test_expect_success \\\n \t'A: verify marks import does not crash' \\\n \t'git fast-import --import-marks=marks.out <input &&\n \t git whatchanged verify--import-marks'\n-test_expect_success \\\n-\t'A: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'A: verify pack' '\n+\tverify_packs\n+'\n+\n cat >expect <<EOF\n :000000 100755 0000000000000000000000000000000000000000 7123f7f44e39be127c5eb701e5968176ee9d78b1 A\tcopy-of-file2\n EOF\n@@ -514,9 +524,11 @@ test_expect_success \\\n     'C: incremental import create pack from stdin' \\\n     'git fast-import <input &&\n \t git whatchanged branch'\n-test_expect_success \\\n-\t'C: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'C: verify pack' '\n+\tverify_packs\n+'\n+\n test_expect_success \\\n \t'C: validate reuse existing blob' \\\n \t'test $newf = `git rev-parse --verify branch:file2/newf` &&\n@@ -572,9 +584,10 @@ test_expect_success \\\n     'D: inline data in commit' \\\n     'git fast-import <input &&\n \t git whatchanged branch'\n-test_expect_success \\\n-\t'D: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'D: verify pack' '\n+\tverify_packs\n+'\n \n cat >expect <<EOF\n :000000 100755 0000000000000000000000000000000000000000 35a59026a33beac1569b1c7f66f3090ce9c09afc A\tnewdir/exec.sh\n@@ -618,9 +631,10 @@ test_expect_success 'E: rfc2822 date, --date-format=raw' '\n test_expect_success \\\n     'E: rfc2822 date, --date-format=rfc2822' \\\n     'git fast-import --date-format=rfc2822 <input'\n-test_expect_success \\\n-\t'E: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'E: verify pack' '\n+\tverify_packs\n+'\n \n cat >expect <<EOF\n author $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> 1170778938 -0500\n@@ -669,9 +683,10 @@ test_expect_success \\\n \t\tfi\n \t fi\n \t'\n-test_expect_success \\\n-\t'F: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'F: verify pack' '\n+\tverify_packs\n+'\n \n cat >expect <<EOF\n tree `git rev-parse branch~1^{tree}`\n@@ -705,9 +720,11 @@ INPUT_END\n test_expect_success \\\n     'G: non-fast-forward update forced' \\\n     'git fast-import --force <input'\n-test_expect_success \\\n-\t'G: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'G: verify pack' '\n+\tverify_packs\n+'\n+\n test_expect_success \\\n \t'G: branch changed, but logged' \\\n \t'test $old_branch != `git rev-parse --verify branch^0` &&\n@@ -742,9 +759,10 @@ test_expect_success \\\n     'H: deletall, add 1' \\\n     'git fast-import <input &&\n \t git whatchanged H'\n-test_expect_success \\\n-\t'H: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'H: verify pack' '\n+\tverify_packs\n+'\n \n cat >expect <<EOF\n :100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D\tfile2/newf\n@@ -1857,9 +1875,10 @@ test_expect_success \\\n \t'Q: commit notes' \\\n \t'git fast-import <input &&\n \t git whatchanged notes-test'\n-test_expect_success \\\n-\t'Q: verify pack' \\\n-\t'for p in .git/objects/pack/*.pack;do git verify-pack $p||exit;done'\n+\n+test_expect_success 'Q: verify pack' '\n+\tverify_packs\n+'\n \n commit1=$(git rev-parse notes-test~2)\n commit2=$(git rev-parse notes-test^)\n@@ -2616,13 +2635,14 @@ test_expect_success \\\n \t'R: blob bigger than threshold' \\\n \t'test_create_repo R &&\n \t git --git-dir=R/.git fast-import --big-file-threshold=1 <input'\n-test_expect_success \\\n-\t'R: verify created pack' \\\n-\t': >verify &&\n-\t for p in R/.git/objects/pack/*.pack;\n-\t do\n-\t   git verify-pack -v $p >>verify || exit;\n-\t done'\n+\n+test_expect_success 'R: verify created pack' '\n+\t(\n+\t\tcd R &&\n+\t\tverify_packs -v > ../verify\n+\t)\n+'\n+\n test_expect_success \\\n \t'R: verify written objects' \\\n \t'git --git-dir=R/.git cat-file blob big-file:big1 >actual &&\n-- \n1.7.10.1386.g7bd022\n"},{"id":"189772","messageId":"xmqqr4vimbe1.fsf@junio.mtv.corp.google.com","threadId":"30285","inReplyTo":"4F910145.5030102@viscovery.net","subject":"Re: [PATCH nd/threaded-index-pack] index-pack: disable threading if NO_PREAD is defined","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-20T19:49:42Z","receivedAt":"2012-04-20T19:49:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j.sixt@viscovery.net> writes:\n\n> Am 4/19/2012 16:05, schrieb Nguyễn Thái Ngọc Duy:\n>> NO_PREAD simulates pread() as a sequence of seek, read, seek in\n>> compat/pread.c. The simulation is not thread-safe because another\n>> thread could move the file offset away in the middle of pread\n>> operation. Do not allow threading in that case.\n>\n> Unsurprisingly, this fixes the breakage for me.\n>\n> I used the attached patch to keep t9300 running when the breakage\n> was detected.\n>\n> --- 8< ---\n> From: Johannes Sixt <j6t@kdbg.org>\n> Subject: [PATCH] t9300-fast-import: avoid 'exit' in test_expect_success snippets\n>\n> Exiting from a for-loop early using '|| break' does not propagate the\n> failure code, and for this reason, the tests used just 'exit'. But this\n> ends the test script with 'FATAL: Unexpected exit code 1' in the case of\n> a failed test.\n>\n> Fix this by moving the loop into a shell function, from which we can\n> simply return early.\n\nMakes sense.  If the original were written more readably, I may have\nsuggested to run the entire for loop in a subshell, but a helper\nfunction is equally readable and with many identical checks, it is the\nright way to do this.\n\nThanks.\n"}]}