{"thread":{"id":"46834","subject":"[PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","startedAt":"2017-09-26T03:31:42Z","lastAt":"2017-09-29T09:35:32Z","messageCount":20,"participants":["Eric Rannaud","Junio C Hamano","Ramsay Jones","Adam Dinwoodie"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"328922","messageId":"598f0addb8b1a5dd535b5bea5f2685ab4c3eb2b0.1506396652.git.e@nanocritical.com","threadId":"46834","inReplyTo":null,"subject":"[PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-26T03:30:52Z","receivedAt":"2017-09-26T03:31:42Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"The checkpoint command cycles packfiles if object_count != 0, a sensible\ntest or there would be no pack files to write. Since 820b931012, the\ncommand also dumps branches, tags and marks, but still conditionally.\nHowever, it is possible for a command stream to modify refs or create\nmarks without creating any new objects.\n\nFor example, reset a branch:\n\n\t$ git fast-import\n\treset refs/heads/master\n\tfrom refs/heads/master^\n\n\tcheckpoint\n\nbut refs/heads/master remains unchanged.\n\nOther example: a commit command that re-creates an object that already\nexists in the object database.\n\nThis fix unconditionally calls dump_{branches,tags,marks}() for all\ncheckpoint commands. dump_branches() and dump_tags() are cheap to call\nin the case of a no-op.\n\nSigned-off-by: Eric Rannaud <e@nanocritical.com>\n---\n fast-import.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 35bf671f12c4..d5e4cf0bad41 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3189,10 +3189,10 @@ static void checkpoint(void)\n \tcheckpoint_requested = 0;\n \tif (object_count) {\n \t\tcycle_packfile();\n-\t\tdump_branches();\n-\t\tdump_tags();\n-\t\tdump_marks();\n \t}\n+\tdump_branches();\n+\tdump_tags();\n+\tdump_marks();\n }\n \n static void parse_checkpoint(void)\n-- \n2.14.1\n\n"},{"id":"328925","messageId":"xmqqr2uuaxy1.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"598f0addb8b1a5dd535b5bea5f2685ab4c3eb2b0.1506396652.git.e@nanocritical.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-26T04:25:10Z","receivedAt":"2017-09-26T04:25:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Rannaud\" <e@nanocritical.com> writes:\n\n> The checkpoint command cycles packfiles if object_count != 0, a sensible\n> test or there would be no pack files to write. Since 820b931012, the\n> command also dumps branches, tags and marks, but still conditionally.\n> However, it is possible for a command stream to modify refs or create\n> marks without creating any new objects.\n\nThat reasoning sounds sensible.  Especially given the discussion of\n\"checkpoint\" and \"progress\" we can see in \"git fast-import --help\"\ndocumentation. E.g.\n\n    Placing a progress command immediately after a checkpoint will\n    inform the reader when the checkpoint has been completed and it\n    can safely access the refs that fast-import updated.\n\nwould not be true without this change, I suspect.\n\nCan we also add a new test or two that protect this from future\nbreakages?\n\nThanks.\n"},{"id":"328936","messageId":"eedb545ccb43a14820802bab10f59ab8ab9557a0.1506419307.git.e@nanocritical.com","threadId":"46834","inReplyTo":"xmqqr2uuaxy1.fsf@gitster.mtv.corp.google.com","subject":"[PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-26T09:53:04Z","receivedAt":"2017-09-26T09:53:27Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"The checkpoint command cycles packfiles if object_count != 0, a sensible\ntest or there would be no pack files to write. Since 820b931012, the\ncommand also dumps branches, tags and marks, but still conditionally.\nHowever, it is possible for a command stream to modify refs or create\nmarks without creating any new objects.\n\nFor example, reset a branch (and keep fast-import running):\n\n\t$ git fast-import\n\treset refs/heads/master\n\tfrom refs/heads/master^\n\n\tcheckpoint\n\nbut refs/heads/master remains unchanged.\n\nOther example: a commit command that re-creates an object that already\nexists in the object database.\n\nThe man page also states that checkpoint \"updates the refs\" and that\n\"placing a progress command immediately after a checkpoint will inform\nthe reader when the checkpoint has been completed and it can safely\naccess the refs that fast-import updated\". This wasn't always true\nwithout this patch.\n\nThis fix unconditionally calls dump_{branches,tags,marks}() for all\ncheckpoint commands. dump_branches() and dump_tags() are cheap to call\nin the case of a no-op.\n\nAdd tests to t9300 that observe the (non-packfiles) effects of\ncheckpoint.\n\nSigned-off-by: Eric Rannaud <e@nanocritical.com>\n---\n fast-import.c          |   6 +--\n t/t9300-fast-import.sh | 117 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 120 insertions(+), 3 deletions(-)\n\n\nAny comments on the testing strategy with a background fast-import?\n\nTo summarize:\n- fast-import is started in the background with a command stream that\n  ends with \"checkpoint\\nprogress checkpoint\\n\". fast-import keeps\n  running after reaching the last command (we don't want fast-import to\n  terminate).\n- In the meantime, the test is waiting to read \"progress checkpoint\" in\n  the output of fast-import, then it checks the testing conditions.\n- Finally, the test ensures that fast-import is still running in the\n  background (and thus that it has just observed the effect of\n  checkpoint, and not the side effects of fast-import terminating).\n- After 10 sec, no matter what, the background fast-import is sent\n  \"done\" and terminates.\n\nI tried to make sure that the test runs quickly and does not (typically) sleep.\nUpon failure, the test may take up to 10 sec to fully terminate.\n\nHowever, the test could break under (very heavy) load if fast-import is unable\nto make progress in a reasonable amount of time (either \"progress checkpoint\"\nis not read within 5 sec, or fast-import receives \"done\" before the testing\nconditions are checked). Let me know if this is not acceptable.\n\nShould these test cases be at least separated to a new t9304 to\ncircumscribe any such nuisance? Alternatively, a FIFO-based approach\ncould be considered.\n\n(Note: added cases 2 and 4 pass without this patch, but 1 and 3 do not.)\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 35bf671f12c4..d5e4cf0bad41 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3189,10 +3189,10 @@ static void checkpoint(void)\n \tcheckpoint_requested = 0;\n \tif (object_count) {\n \t\tcycle_packfile();\n-\t\tdump_branches();\n-\t\tdump_tags();\n-\t\tdump_marks();\n \t}\n+\tdump_branches();\n+\tdump_tags();\n+\tdump_marks();\n }\n \n static void parse_checkpoint(void)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 67b8c50a5ab4..b410bf3a3a7a 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3120,4 +3120,121 @@ test_expect_success 'U: validate root delete result' '\n \tcompare_diff_raw expect actual\n '\n \n+###\n+### series V (checkpoint)\n+###\n+\n+# To make sure you're observing the side effects of checkpoint *before*\n+# fast-import terminates (and thus writes out its state), check that the\n+# fast-import process is still running using background_import_still_running\n+# *after* evaluating the test conditions.\n+background_import_until_checkpoint () {\n+\toptions=$1\n+\tinput_file=$2\n+\t( cat $input_file; sleep 10; echo done ) | git fast-import $options >V.output &\n+\techo $! >V.pid\n+\n+\t# The loop will poll for approximately 5 seconds before giving up.\n+\tn=0\n+\twhile ! test \"$(cat V.output)\" = \"progress checkpoint\"; do\n+\t\tif test $n -gt 55\n+\t\tthen\n+\t\t\techo >&2 \"no progress checkpoint received\"\n+\t\t\texit 1\n+\t\tfi\n+\n+\t\t# Try to avoid sleeping in the first iterations and poll\n+\t\t# aggressively.\n+\t\tif test $n -ge 50\n+\t\tthen\n+\t\t\tsleep 1\n+\t\tfi\n+\n+\t\tn=$(($n + 1))\n+\tdone\n+}\n+\n+background_import_still_running () {\n+\tif ! ps --pid \"$(cat V.pid)\"\n+\tthen\n+\t\techo >&2 \"background fast-import terminated too early\"\n+\t\texit 1\n+\tfi\n+\tkill $(cat V.pid)\n+}\n+\n+test_expect_success 'V: checkpoint updates refs after reset' '\n+\tcat >input <<-INPUT_END &&\n+\treset refs/heads/V\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"\" input &&\n+\ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success 'V: checkpoint updates refs and marks after commit' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V\n+\tmark :1\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V^)\" = \"$(git rev-parse --verify U)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+# Re-create the exact same commit, but on a different branch: no new object is\n+# created in the database, but the refs and marks still need to be updated.\n+test_expect_success 'V: checkpoint updates refs and marks after commit (no new objects)' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V2\n+\tmark :2\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V2)\" = \"$(git rev-parse --verify V)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success 'V: checkpoint updates tags after tag' '\n+\tcat >input <<-INPUT_END &&\n+\ttag Vtag\n+\tfrom refs/heads/V\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"\" input &&\n+\tgit show-ref -d Vtag &&\n+\tbackground_import_still_running\n+'\n+\n test_done\n-- \n2.14.1\n\n"},{"id":"328996","messageId":"xmqqefqs7qx8.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"eedb545ccb43a14820802bab10f59ab8ab9557a0.1506419307.git.e@nanocritical.com","subject":"Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-27T03:37:23Z","receivedAt":"2017-09-27T03:37:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Rannaud\" <e@nanocritical.com> writes:\n\n> Any comments on the testing strategy with a background fast-import?\n>\n> To summarize:\n> - fast-import is started in the background with a command stream that\n>   ends with \"checkpoint\\nprogress checkpoint\\n\". fast-import keeps\n>   running after reaching the last command (we don't want fast-import to\n>   terminate).\n> - In the meantime, the test is waiting to read \"progress checkpoint\" in\n>   the output of fast-import, then it checks the testing conditions.\n> - Finally, the test ensures that fast-import is still running in the\n>   background (and thus that it has just observed the effect of\n>   checkpoint, and not the side effects of fast-import terminating).\n> - After 10 sec, no matter what, the background fast-import is sent\n>   \"done\" and terminates.\n>\n> I tried to make sure that the test runs quickly and does not (typically) sleep.\n> Upon failure, the test may take up to 10 sec to fully terminate.\n\nHmmmm, it certainly looks a bit too brittle with many tweakables\nlike 10, 50, 55, etc. that heavily depend on the load on the system.\n\nSorry for asking you to go through the hoops.\n\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 67b8c50a5ab4..b410bf3a3a7a 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -3120,4 +3120,121 @@ test_expect_success 'U: validate root delete result' '\n>  \tcompare_diff_raw expect actual\n>  '\n>  \n> +###\n> +### series V (checkpoint)\n> +###\n> +\n> +# To make sure you're observing the side effects of checkpoint *before*\n> +# fast-import terminates (and thus writes out its state), check that the\n> +# fast-import process is still running using background_import_still_running\n> +# *after* evaluating the test conditions.\n> +background_import_until_checkpoint () {\n> +\toptions=$1\n> +\tinput_file=$2\n> +\t( cat $input_file; sleep 10; echo done ) | git fast-import $options >V.output &\n> +\techo $! >V.pid\n> +\n> +\t# The loop will poll for approximately 5 seconds before giving up.\n> +\tn=0\n> +\twhile ! test \"$(cat V.output)\" = \"progress checkpoint\"; do\n\nMicronit.  Just like you have if/then on different lines, have\nwhile/do on different lines, i.e.\n\n\twhile test \"$(cat V.output)\" != \"progress checkpoint\"\n\tdo\n\n> +\t\tif test $n -gt 55\n> +\t\tthen\n> +...\n\n> +background_import_still_running () {\n> +\tif ! ps --pid \"$(cat V.pid)\"\n\n\"ps --pid\" is probably not portable (I think we'd do \"kill -0 $pid\"\ninstead in our existing tests---and it should be kosher according to\nPOSIX [*1*, *2*]).\n\n> +test_expect_success 'V: checkpoint updates refs after reset' '\n> +\tcat >input <<-INPUT_END &&\n\nIt is not wrong per-se but we quote INPUT_END when there is no\ninterpolation necessary in the body of here document to help\nreaders, like so:\n\n\tcat >input <<-\\INPUT_END &&\n\n> +\treset refs/heads/V\n> +\tfrom refs/heads/U\n> +\n> +\tcheckpoint\n> +\tprogress checkpoint\n> +\tINPUT_END\n\n> +test_expect_success 'V: checkpoint updates refs and marks after commit' '\n> +\tcat >input <<-INPUT_END &&\n\nThis we do want interpolation and the above is correct.\n\n\n[Footnotes]\n\n*1* http://pubs.opengroup.org/onlinepubs/9699919799/utilities/kill.html \n    lists '0' as an allowed signal number to be sent, and defers to the\n    description of the kill() function to explain what happens.\n\n*2* http://pubs.opengroup.org/onlinepubs/9699919799/functions/kill.html\n    says \"\"\"If sig is 0 (the null signal), error checking is\n    performed but no signal is actually sent. The null signal can be\n    used to check the validity of pid.\"\"\"\n"},{"id":"329054","messageId":"cccb06b75df3cad9f013d5a9ab0371f0a2d9c2ce.1506541322.git.e@nanocritical.com","threadId":"46834","inReplyTo":"xmqqefqs7qx8.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-27T19:46:26Z","receivedAt":"2017-09-27T19:46:40Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"The checkpoint command cycles packfiles if object_count != 0, a sensible\ntest or there would be no pack files to write. Since 820b931012, the\ncommand also dumps branches, tags and marks, but still conditionally.\nHowever, it is possible for a command stream to modify refs or create\nmarks without creating any new objects.\n\nFor example, reset a branch (and keep fast-import running):\n\n\t$ git fast-import\n\treset refs/heads/master\n\tfrom refs/heads/master^\n\n\tcheckpoint\n\nbut refs/heads/master remains unchanged.\n\nOther example: a commit command that re-creates an object that already\nexists in the object database.\n\nThe man page also states that checkpoint \"updates the refs\" and that\n\"placing a progress command immediately after a checkpoint will inform\nthe reader when the checkpoint has been completed and it can safely\naccess the refs that fast-import updated\". This wasn't always true\nwithout this patch.\n\nThis fix unconditionally calls dump_{branches,tags,marks}() for all\ncheckpoint commands. dump_branches() and dump_tags() are cheap to call\nin the case of a no-op.\n\nAdd tests to t9300 that observe the (non-packfiles) effects of\ncheckpoint.\n\nSigned-off-by: Eric Rannaud <e@nanocritical.com>\n---\n fast-import.c          |   6 +--\n t/t9300-fast-import.sh | 129 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 132 insertions(+), 3 deletions(-)\n\n\nUse named pipes instead of the polling approach. Also incorporate your other\ncomments.\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 35bf671f12c4..d5e4cf0bad41 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3189,10 +3189,10 @@ static void checkpoint(void)\n \tcheckpoint_requested = 0;\n \tif (object_count) {\n \t\tcycle_packfile();\n-\t\tdump_branches();\n-\t\tdump_tags();\n-\t\tdump_marks();\n \t}\n+\tdump_branches();\n+\tdump_tags();\n+\tdump_marks();\n }\n \n static void parse_checkpoint(void)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 67b8c50a5ab4..9aa3470d895b 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3120,4 +3120,133 @@ test_expect_success 'U: validate root delete result' '\n \tcompare_diff_raw expect actual\n '\n \n+###\n+### series V (checkpoint)\n+###\n+\n+# To make sure you're observing the side effects of checkpoint *before*\n+# fast-import terminates (and thus writes out its state), check that the\n+# fast-import process is still running using background_import_still_running\n+# *after* evaluating the test conditions.\n+background_import_until_checkpoint () {\n+\toptions=$1\n+\tinput_file=$2\n+\n+\tmkfifo V.input\n+\texec 8<>V.input\n+\trm V.input\n+\n+\tmkfifo V.output\n+\texec 9<>V.output\n+\trm V.output\n+\n+\tcat $input_file >&8\n+\tgit fast-import $options <&8 >&9 &\n+\techo $! >V.pid\n+\ttest_when_finished \"kill $(cat V.pid) || true\"\n+\n+\terror=0\n+\tif read output <&9\n+\tthen\n+\t\tif ! test \"$output\" = \"progress checkpoint\"\n+\t\tthen\n+\t\t\techo >&2 \"no progress checkpoint received: $output\"\n+\t\t\terror=1\n+\t\tfi\n+\telse\n+\t\techo >&2 \"failed to read fast-import output\"\n+\t\terror=1\n+\tfi\n+\n+\texec 8>&-\n+\texec 9>&-\n+\n+\tif test $error -eq 1\n+\tthen\n+\t\texit 1\n+\tfi\n+}\n+\n+background_import_still_running () {\n+\tif ! kill -0 \"$(cat V.pid)\"\n+\tthen\n+\t\techo >&2 \"background fast-import terminated too early\"\n+\t\texit 1\n+\tfi\n+}\n+\n+test_expect_success 'V: checkpoint updates refs after reset' '\n+\tcat >input <<-\\INPUT_END &&\n+\treset refs/heads/V\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"\" input &&\n+\ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success 'V: checkpoint updates refs and marks after commit' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V\n+\tmark :1\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V^)\" = \"$(git rev-parse --verify U)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+# Re-create the exact same commit, but on a different branch: no new object is\n+# created in the database, but the refs and marks still need to be updated.\n+test_expect_success 'V: checkpoint updates refs and marks after commit (no new objects)' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V2\n+\tmark :2\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V2)\" = \"$(git rev-parse --verify V)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success 'V: checkpoint updates tags after tag' '\n+\tcat >input <<-INPUT_END &&\n+\ttag Vtag\n+\tfrom refs/heads/V\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tcheckpoint\n+\tprogress checkpoint\n+\tINPUT_END\n+\n+\tbackground_import_until_checkpoint \"\" input &&\n+\tgit show-ref -d Vtag &&\n+\tbackground_import_still_running\n+'\n+\n test_done\n-- \n2.14.1\n\n"},{"id":"329077","messageId":"2a9cc69b-2e9f-7fe9-3c55-66ffe3060732@ramsayjones.plus.com","threadId":"46834","inReplyTo":"cccb06b75df3cad9f013d5a9ab0371f0a2d9c2ce.1506541322.git.e@nanocritical.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-09-27T23:19:36Z","receivedAt":"2017-09-27T23:19:44Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 27/09/17 20:46, Eric Rannaud wrote:\n> The checkpoint command cycles packfiles if object_count != 0, a sensible\n> test or there would be no pack files to write. Since 820b931012, the\n> command also dumps branches, tags and marks, but still conditionally.\n> However, it is possible for a command stream to modify refs or create\n> marks without creating any new objects.\n> \n> For example, reset a branch (and keep fast-import running):\n> \n> \t$ git fast-import\n> \treset refs/heads/master\n> \tfrom refs/heads/master^\n> \n> \tcheckpoint\n> \n> but refs/heads/master remains unchanged.\n> \n> Other example: a commit command that re-creates an object that already\n> exists in the object database.\n> \n> The man page also states that checkpoint \"updates the refs\" and that\n> \"placing a progress command immediately after a checkpoint will inform\n> the reader when the checkpoint has been completed and it can safely\n> access the refs that fast-import updated\". This wasn't always true\n> without this patch.\n> \n> This fix unconditionally calls dump_{branches,tags,marks}() for all\n> checkpoint commands. dump_branches() and dump_tags() are cheap to call\n> in the case of a no-op.\n> \n> Add tests to t9300 that observe the (non-packfiles) effects of\n> checkpoint.\n> \n> Signed-off-by: Eric Rannaud <e@nanocritical.com>\n> ---\n>  fast-import.c          |   6 +--\n>  t/t9300-fast-import.sh | 129 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 132 insertions(+), 3 deletions(-)\n> \n> \n> Use named pipes instead of the polling approach. Also incorporate your other\n> comments.\n> \n> \n> diff --git a/fast-import.c b/fast-import.c\n> index 35bf671f12c4..d5e4cf0bad41 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -3189,10 +3189,10 @@ static void checkpoint(void)\n>  \tcheckpoint_requested = 0;\n>  \tif (object_count) {\n>  \t\tcycle_packfile();\n> -\t\tdump_branches();\n> -\t\tdump_tags();\n> -\t\tdump_marks();\n>  \t}\n> +\tdump_branches();\n> +\tdump_tags();\n> +\tdump_marks();\n>  }\n>  \n>  static void parse_checkpoint(void)\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 67b8c50a5ab4..9aa3470d895b 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -3120,4 +3120,133 @@ test_expect_success 'U: validate root delete result' '\n>  \tcompare_diff_raw expect actual\n>  '\n>  \n> +###\n> +### series V (checkpoint)\n> +###\n> +\n> +# To make sure you're observing the side effects of checkpoint *before*\n> +# fast-import terminates (and thus writes out its state), check that the\n> +# fast-import process is still running using background_import_still_running\n> +# *after* evaluating the test conditions.\n> +background_import_until_checkpoint () {\n> +\toptions=$1\n> +\tinput_file=$2\n> +\n> +\tmkfifo V.input\n\nSince you are using mkfifo here ...\n\n> +\texec 8<>V.input\n> +\trm V.input\n> +\n> +\tmkfifo V.output\n> +\texec 9<>V.output\n> +\trm V.output\n> +\n> +\tcat $input_file >&8\n> +\tgit fast-import $options <&8 >&9 &\n> +\techo $! >V.pid\n> +\ttest_when_finished \"kill $(cat V.pid) || true\"\n> +\n> +\terror=0\n> +\tif read output <&9\n> +\tthen\n> +\t\tif ! test \"$output\" = \"progress checkpoint\"\n> +\t\tthen\n> +\t\t\techo >&2 \"no progress checkpoint received: $output\"\n> +\t\t\terror=1\n> +\t\tfi\n> +\telse\n> +\t\techo >&2 \"failed to read fast-import output\"\n> +\t\terror=1\n> +\tfi\n> +\n> +\texec 8>&-\n> +\texec 9>&-\n> +\n> +\tif test $error -eq 1\n> +\tthen\n> +\t\texit 1\n> +\tfi\n> +}\n> +\n> +background_import_still_running () {\n> +\tif ! kill -0 \"$(cat V.pid)\"\n> +\tthen\n> +\t\techo >&2 \"background fast-import terminated too early\"\n> +\t\texit 1\n> +\tfi\n> +}\n> +\n\n... you need to set the PIPE prerequisite on all of your new tests.\n\n> +test_expect_success 'V: checkpoint updates refs after reset' '\n> +\tcat >input <<-\\INPUT_END &&\n> +\treset refs/heads/V\n> +\tfrom refs/heads/U\n> +\n> +\tcheckpoint\n> +\tprogress checkpoint\n> +\tINPUT_END\n> +\n> +\tbackground_import_until_checkpoint \"\" input &&\n> +\ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n> +\tbackground_import_still_running\n> +'\n\n[snip]\n\nATB,\nRamsay Jones\n\n\n"},{"id":"329087","messageId":"xmqqh8vn32mn.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"cccb06b75df3cad9f013d5a9ab0371f0a2d9c2ce.1506541322.git.e@nanocritical.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T03:48:00Z","receivedAt":"2017-09-28T03:48:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Rannaud\" <e@nanocritical.com> writes:\n\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 67b8c50a5ab4..9aa3470d895b 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -3120,4 +3120,133 @@ test_expect_success 'U: validate root delete result' '\n>  \tcompare_diff_raw expect actual\n>  '\n>  \n> +###\n> +### series V (checkpoint)\n> +###\n> +\n> +# To make sure you're observing the side effects of checkpoint *before*\n> +# fast-import terminates (and thus writes out its state), check that the\n> +# fast-import process is still running using background_import_still_running\n> +# *after* evaluating the test conditions.\n> +background_import_until_checkpoint () {\n> +\toptions=$1\n> +\tinput_file=$2\n> +\n> +\tmkfifo V.input\n> +\texec 8<>V.input\n> +\trm V.input\n> +\n> +\tmkfifo V.output\n> +\texec 9<>V.output\n> +\trm V.output\n> +\n> +\tcat $input_file >&8\n\nIt probably is a good idea to quote \"$input_file\" in case other\npeople later use a full path to the file or something; for now this\nis OK.\n\nfd#8 at this point does not have a reader; unless the contents of\nthe $input_file is small enough, wouldn't this \"cat\" block until\nsomebody else comes and reads from it to drain?  Should we instead\nstart fast-import first in the background, arrange it to be killed\nwhen we are done with it, and then start feeding the input?\n\n> +\tgit fast-import $options <&8 >&9 &\n> +\techo $! >V.pid\n> +\ttest_when_finished \"kill $(cat V.pid) || true\"\n\nThis '|| true' is here because the process might already have died\non its own, which sounds like a sensible precaution.\n\n> +\terror=0\n> +\tif read output <&9\n> +\tthen\n> +\t\tif ! test \"$output\" = \"progress checkpoint\"\n> +\t\tthen\n> +\t\t\techo >&2 \"no progress checkpoint received: $output\"\n> +\t\t\terror=1\n> +\t\tfi\n> +\telse\n> +\t\techo >&2 \"failed to read fast-import output\"\n> +\t\terror=1\n> +\tfi\n\nAnd we expect \"progress checkpoint\" would be the first and only\noutput after fast-import consumes all the input stream up to the\n\"progress\" thing we feed, so this is not \"read and discard until\nwe see 'progress checkpoint'\" but is \"read one and that must be\n'progress checkpoint'\".  Makes sense to me.\n\nIf this script is (and will be in the future) all about issuing a\ncheckpoint command and observing its effect, we can reasonably\nexpect that the input file _must_ end with \"checkpoint\" followed by\n\"progress checkpoint\", no?  If that is the case, perhaps feeding\nthese two from this helper function to >&8, instead of forcing the\ncaller to prepare the input file to always end with these two, may\nbe a better organization.\n\n> +\texec 8>&-\n> +\texec 9>&-\n\nThese are to make sure that nobody (after fast-import dies) has\nthese file descriptors hanging open for writing.  Makes one wonder\nwhat happens to the reader side of the file descriptor, though ;-)\n\nBefore we return from this function, we expect (as the comment\nbefore the function says) that fast-import is still running, waiting\nfurther input.  Wouldn't closing the other side of the pipe here\nlike these make it notice that there is no more data by causing\nread_next_command() find EOF?  IOW, is \"use import_until_checkout,\ntest the outcome and then make sure import_still_running reports that\nthe outcome was not due to the process terminating and flushing\"\nsomewhat racy?\n\nOr are we closing these file descriptors for different reason\n(i.e. not to tell fast-import we are done feeding it input) and I am\nreading the code incorrectly?  Puzzled.\n\n> +\tif test $error -eq 1\n> +\tthen\n> +\t\texit 1\n> +\tfi\n> +}\n> +\n> +background_import_still_running () {\n> +\tif ! kill -0 \"$(cat V.pid)\"\n> +\tthen\n> +\t\techo >&2 \"background fast-import terminated too early\"\n> +\t\texit 1\n> +\tfi\n> +}\n\nI suspect these \"exit 1\" above should be \"false\", to give the calling\ntest_expect_success a chance to notice the failure and react to it.\n\n> +test_expect_success 'V: checkpoint updates refs after reset' '\n> +\tcat >input <<-\\INPUT_END &&\n> +\treset refs/heads/V\n> +\tfrom refs/heads/U\n> +\n> +\tcheckpoint\n> +\tprogress checkpoint\n> +\tINPUT_END\n> +\n> +\tbackground_import_until_checkpoint \"\" input &&\n> +\ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n> +\tbackground_import_still_running\n> +'\n"},{"id":"329094","messageId":"CAH_=xoZfuJDE515zZbry1DdGV+YeySy+5T9QOBefO-MMTSvXSA@mail.gmail.com","threadId":"46834","inReplyTo":"xmqqh8vn32mn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-28T04:56:14Z","receivedAt":"2017-09-28T04:56:39Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Wed, Sep 27, 2017 at 8:48 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> +     cat $input_file >&8\n>\n> It probably is a good idea to quote \"$input_file\" in case other\n> people later use a full path to the file or something; for now this\n> is OK.\n\nRight.\n\n\n> fd#8 at this point does not have a reader; unless the contents of\n> the $input_file is small enough, wouldn't this \"cat\" block until\n> somebody else comes and reads from it to drain?  Should we instead\n> start fast-import first in the background, arrange it to be killed\n> when we are done with it, and then start feeding the input?\n\nGood point, I will swap the order.\n\n\n>> +     git fast-import $options <&8 >&9 &\n>> +     echo $! >V.pid\n>> +     test_when_finished \"kill $(cat V.pid) || true\"\n>\n> This '|| true' is here because the process might already have died\n> on its own, which sounds like a sensible precaution.\n\nI added a comment.\n\n\n>> +     error=0\n>> +     if read output <&9\n>> +     then\n>> +             if ! test \"$output\" = \"progress checkpoint\"\n>> +             then\n>> +                     echo >&2 \"no progress checkpoint received: $output\"\n>> +                     error=1\n>> +             fi\n>> +     else\n>> +             echo >&2 \"failed to read fast-import output\"\n>> +             error=1\n>> +     fi\n>\n> And we expect \"progress checkpoint\" would be the first and only\n> output after fast-import consumes all the input stream up to the\n> \"progress\" thing we feed, so this is not \"read and discard until\n> we see 'progress checkpoint'\" but is \"read one and that must be\n> 'progress checkpoint'\".  Makes sense to me.\n>\n> If this script is (and will be in the future) all about issuing a\n> checkpoint command and observing its effect, we can reasonably\n> expect that the input file _must_ end with \"checkpoint\" followed by\n> \"progress checkpoint\", no?  If that is the case, perhaps feeding\n> these two from this helper function to >&8, instead of forcing the\n> caller to prepare the input file to always end with these two, may\n> be a better organization.\n\nAgreed. Renamed the function background_import_then_checkpoint to\nreflect the change.\n\n\n>> +     exec 8>&-\n>> +     exec 9>&-\n>\n> These are to make sure that nobody (after fast-import dies) has\n> these file descriptors hanging open for writing.  Makes one wonder\n> what happens to the reader side of the file descriptor, though ;-)\n>\n> Before we return from this function, we expect (as the comment\n> before the function says) that fast-import is still running, waiting\n> further input.  Wouldn't closing the other side of the pipe here\n> like these make it notice that there is no more data by causing\n> read_next_command() find EOF?  IOW, is \"use import_until_checkout,\n> test the outcome and then make sure import_still_running reports that\n> the outcome was not due to the process terminating and flushing\"\n> somewhat racy?\n>\n> Or are we closing these file descriptors for different reason\n> (i.e. not to tell fast-import we are done feeding it input) and I am\n> reading the code incorrectly?  Puzzled.\n\nClosing 8 and 9 was just housekeeping on my part. But you raise a good\npoint: what happens then to the stdin of fast-import?\n\nDoesn't fast-import get a copy of 8 (open for both reading and\nwriting), as a child process, and exec 8>&- only closes the copy of\nthe file descriptor in the parent shell, so the named pipe remains\nopen for writing somewhere (in the fast-import process itself, in\nfact), therefore fast-import will not find EOF on its stdin?\n\nBut in any case, it is sensible to delay the closing of 8 and 9 to\ntest_when_finished.\n\n\n>> +     if test $error -eq 1\n>> +     then\n>> +             exit 1\n>> +     fi\n>> +}\n>> +\n>> +background_import_still_running () {\n>> +     if ! kill -0 \"$(cat V.pid)\"\n>> +     then\n>> +             echo >&2 \"background fast-import terminated too early\"\n>> +             exit 1\n>> +     fi\n>> +}\n>\n> I suspect these \"exit 1\" above should be \"false\", to give the calling\n> test_expect_success a chance to notice the failure and react to it.\n\nTrue.\n\nWill follow-up with an updated patch.\n"},{"id":"329096","messageId":"5b6001e7db3c57fdd9e6354ed4294e2192524061.1506574951.git.e@nanocritical.com","threadId":"46834","inReplyTo":"CAH_=xoZfuJDE515zZbry1DdGV+YeySy+5T9QOBefO-MMTSvXSA@mail.gmail.com","subject":"[PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-28T05:07:41Z","receivedAt":"2017-09-28T05:07:50Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"The checkpoint command cycles packfiles if object_count != 0, a sensible\ntest or there would be no pack files to write. Since 820b931012, the\ncommand also dumps branches, tags and marks, but still conditionally.\nHowever, it is possible for a command stream to modify refs or create\nmarks without creating any new objects.\n\nFor example, reset a branch (and keep fast-import running):\n\n\t$ git fast-import\n\treset refs/heads/master\n\tfrom refs/heads/master^\n\n\tcheckpoint\n\nbut refs/heads/master remains unchanged.\n\nOther example: a commit command that re-creates an object that already\nexists in the object database.\n\nThe man page also states that checkpoint \"updates the refs\" and that\n\"placing a progress command immediately after a checkpoint will inform\nthe reader when the checkpoint has been completed and it can safely\naccess the refs that fast-import updated\". This wasn't always true\nwithout this patch.\n\nThis fix unconditionally calls dump_{branches,tags,marks}() for all\ncheckpoint commands. dump_branches() and dump_tags() are cheap to call\nin the case of a no-op.\n\nAdd tests to t9300 that observe the (non-packfiles) effects of\ncheckpoint.\n\nSigned-off-by: Eric Rannaud <e@nanocritical.com>\n---\n fast-import.c          |   6 +--\n t/t9300-fast-import.sh | 126 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 129 insertions(+), 3 deletions(-)\n\n\nUpdated to include Junio's latest remarks.\n\nAlso adding the necessary PIPE prereq, as pointed out by Ramsay Jones.\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 35bf671f12c4..d5e4cf0bad41 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3189,10 +3189,10 @@ static void checkpoint(void)\n \tcheckpoint_requested = 0;\n \tif (object_count) {\n \t\tcycle_packfile();\n-\t\tdump_branches();\n-\t\tdump_tags();\n-\t\tdump_marks();\n \t}\n+\tdump_branches();\n+\tdump_tags();\n+\tdump_marks();\n }\n \n static void parse_checkpoint(void)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 67b8c50a5ab4..b8d394548520 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3120,4 +3120,130 @@ test_expect_success 'U: validate root delete result' '\n \tcompare_diff_raw expect actual\n '\n \n+###\n+### series V (checkpoint)\n+###\n+\n+# The commands in input_file should not produce any output on the file\n+# descriptor set with --cat-blob-fd (or stdout if unspecified).\n+#\n+# To make sure you're observing the side effects of checkpoint *before*\n+# fast-import terminates (and thus writes out its state), check that the\n+# fast-import process is still running using background_import_still_running\n+# *after* evaluating the test conditions.\n+background_import_then_checkpoint () {\n+\toptions=$1\n+\tinput_file=$2\n+\n+\tmkfifo V.input\n+\texec 8<>V.input\n+\trm V.input\n+\n+\tmkfifo V.output\n+\texec 9<>V.output\n+\trm V.output\n+\n+\tgit fast-import $options <&8 >&9 &\n+\techo $! >V.pid\n+\t# We don't mind if fast-import has already died by the time the test\n+\t# ends.\n+\ttest_when_finished \"exec 8>&-; exec 9>&-; kill $(cat V.pid) || true\"\n+\n+\tcat \"$input_file\" >&8\n+\techo \"checkpoint\" >&8\n+\techo \"progress checkpoint\" >&8\n+\n+\terror=0\n+\tif read output <&9\n+\tthen\n+\t\tif ! test \"$output\" = \"progress checkpoint\"\n+\t\tthen\n+\t\t\techo >&2 \"no progress checkpoint received: $output\"\n+\t\t\terror=1\n+\t\tfi\n+\telse\n+\t\techo >&2 \"failed to read fast-import output\"\n+\t\terror=1\n+\tfi\n+\n+\tif test $error -eq 1\n+\tthen\n+\t\tfalse\n+\tfi\n+}\n+\n+background_import_still_running () {\n+\tif ! kill -0 \"$(cat V.pid)\"\n+\tthen\n+\t\techo >&2 \"background fast-import terminated too early\"\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success PIPE 'V: checkpoint updates refs after reset' '\n+\tcat >input <<-\\INPUT_END &&\n+\treset refs/heads/V\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"\" input &&\n+\ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V\n+\tmark :1\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V^)\" = \"$(git rev-parse --verify U)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+# Re-create the exact same commit, but on a different branch: no new object is\n+# created in the database, but the refs and marks still need to be updated.\n+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no new objects)' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V2\n+\tmark :2\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V2)\" = \"$(git rev-parse --verify V)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n+\tcat >input <<-INPUT_END &&\n+\ttag Vtag\n+\tfrom refs/heads/V\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"\" input &&\n+\tgit show-ref -d Vtag &&\n+\tbackground_import_still_running\n+'\n+\n test_done\n-- \n2.14.1\n\n"},{"id":"329099","messageId":"xmqqvak31hts.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"CAH_=xoZfuJDE515zZbry1DdGV+YeySy+5T9QOBefO-MMTSvXSA@mail.gmail.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T06:02:39Z","receivedAt":"2017-09-28T06:02:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Rannaud <e@nanocritical.com> writes:\n\n> Doesn't fast-import get a copy of 8 (open for both reading and\n> writing), as a child process, and exec 8>&- only closes the copy of\n> the file descriptor in the parent shell, so the named pipe remains\n> open for writing somewhere (in the fast-import process itself, in\n> fact), therefore fast-import will not find EOF on its stdin?\n\nAHHHHhhhh.  If that was done intentionally, well, I really have to\nmarvel at the cleverness of the solution!  It makes sense now to me.\n"},{"id":"329100","messageId":"CAH_=xoaQKf2NUaFdqnMPejbj4e_cXQ9CkVVRJYtwGEBFPqca3Q@mail.gmail.com","threadId":"46834","inReplyTo":"xmqqvak31hts.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-28T06:44:24Z","receivedAt":"2017-09-28T06:44:50Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Wed, Sep 27, 2017 at 11:02 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Rannaud <e@nanocritical.com> writes:\n>\n>> Doesn't fast-import get a copy of 8 (open for both reading and\n>> writing), as a child process, and exec 8>&- only closes the copy of\n>> the file descriptor in the parent shell, so the named pipe remains\n>> open for writing somewhere (in the fast-import process itself, in\n>> fact), therefore fast-import will not find EOF on its stdin?\n>\n> AHHHHhhhh.  If that was done intentionally, well, I really have to\n> marvel at the cleverness of the solution!  It makes sense now to me.\n\nI plead the Fifth.\n"},{"id":"329102","messageId":"xmqqo9pv156t.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"5b6001e7db3c57fdd9e6354ed4294e2192524061.1506574951.git.e@nanocritical.com","subject":"Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T10:35:38Z","receivedAt":"2017-09-28T10:35:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Rannaud\" <e@nanocritical.com> writes:\n\n> +# The commands in input_file should not produce any output on the file\n> +# descriptor set with --cat-blob-fd (or stdout if unspecified).\n\nThanks for documenting this.  Swapping the order of starting\nfast-import and feeding its input (which is one change in this\nversion relative to the previous one) alone would not help, because\nin the updated order in this patch, nobody is reading from\nfast-import until the parent process finishes feeding it.\n"},{"id":"329106","messageId":"20170928125916.GB9439@dinwoodie.org","threadId":"46834","inReplyTo":"5b6001e7db3c57fdd9e6354ed4294e2192524061.1506574951.git.e@nanocritical.com","subject":"Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Adam Dinwoodie","fromEmail":"adam@dinwoodie.org","sentAt":"2017-09-28T12:59:16Z","receivedAt":"2017-09-28T12:59:28Z","isPatch":true,"sender":{"key":"adam@dinwoodie.org","avatar":"https://avatars.githubusercontent.com/u/1397507?v=4"},"body":"On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:\n> The checkpoint command cycles packfiles if object_count != 0, a sensible\n> test or there would be no pack files to write. Since 820b931012, the\n> command also dumps branches, tags and marks, but still conditionally.\n> However, it is possible for a command stream to modify refs or create\n> marks without creating any new objects.\n> \n> For example, reset a branch (and keep fast-import running):\n> \n> \t$ git fast-import\n> \treset refs/heads/master\n> \tfrom refs/heads/master^\n> \n> \tcheckpoint\n> \n> but refs/heads/master remains unchanged.\n> \n> Other example: a commit command that re-creates an object that already\n> exists in the object database.\n> \n> The man page also states that checkpoint \"updates the refs\" and that\n> \"placing a progress command immediately after a checkpoint will inform\n> the reader when the checkpoint has been completed and it can safely\n> access the refs that fast-import updated\". This wasn't always true\n> without this patch.\n> \n> This fix unconditionally calls dump_{branches,tags,marks}() for all\n> checkpoint commands. dump_branches() and dump_tags() are cheap to call\n> in the case of a no-op.\n> \n> Add tests to t9300 that observe the (non-packfiles) effects of\n> checkpoint.\n> \n> Signed-off-by: Eric Rannaud <e@nanocritical.com>\n> ---\n>  fast-import.c          |   6 +--\n>  t/t9300-fast-import.sh | 126 +++++++++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 129 insertions(+), 3 deletions(-)\n> \n> \n> Updated to include Junio's latest remarks.\n> \n> Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.\n\nCygwin doesn't have the PIPE prereq; I've just confirmed that the\nprevious version of this patch has t9300 failing on Cygwin, but this\nversion passes.\n"},{"id":"329118","messageId":"CAH_=xoY_WihdNEeZTcUyYR8jFu17L0gnS8umLToOR6ER0A=RSQ@mail.gmail.com","threadId":"46834","inReplyTo":"xmqqo9pv156t.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-28T20:30:25Z","receivedAt":"2017-09-28T20:30:52Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Thu, Sep 28, 2017 at 3:35 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> \"Eric Rannaud\" <e@nanocritical.com> writes:\n>\n>> +# The commands in input_file should not produce any output on the file\n>> +# descriptor set with --cat-blob-fd (or stdout if unspecified).\n>\n> Thanks for documenting this.  Swapping the order of starting\n> fast-import and feeding its input (which is one change in this\n> version relative to the previous one) alone would not help, because\n> in the updated order in this patch, nobody is reading from\n> fast-import until the parent process finishes feeding it.\n\nDarn. That's correct, on a platform with blocking pipes it would not\nwork. I will start the input cat in the background (so it may block\nwhile writing), before reading from the output of fast-import.\n"},{"id":"329120","messageId":"20170928210358.11596-1-e@nanocritical.com","threadId":"46834","inReplyTo":"20170928125916.GB9439@dinwoodie.org","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-28T21:03:58Z","receivedAt":"2017-09-28T21:04:05Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Thu, Sep 28, 2017 at 5:59 AM, Adam Dinwoodie <adam@dinwoodie.org> wrote:\n> On Wed, Sep 27, 2017 at 10:07:41PM -0700, Eric Rannaud wrote:\n>>\n>> Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.\n>\n> Cygwin doesn't have the PIPE prereq; I've just confirmed that the\n> previous version of this patch has t9300 failing on Cygwin, but this\n> version passes.\n\nWhat's the preferred solution here? I can avoid using named pipes entirely:\n\n\tread_checkpoint () {\n\t\tif read output\n\t\tthen\n\t\t\tif ! test \"$output\" = \"progress checkpoint\"\n\t\t\tthen\n\t\t\t\techo >&2 \"no progress checkpoint received: $output\"\n\t\t\t\techo 1 > V.result\n\t\t\telse\n\t\t\t\techo 0 > V.result\n\t\t\tfi\n\t\telse\n\t\t\techo >&2 \"failed to read fast-import output\"\n\t\t\techo 1 > V.result\n\t\tfi\n\t}\n\t\n\t# The commands in input_file should not produce any output on the file\n\t# descriptor set with --cat-blob-fd (or stdout if unspecified).\n\t#\n\t# To make sure you're observing the side effects of checkpoint *before*\n\t# fast-import terminates (and thus writes out its state), check that the\n\t# fast-import process is still running using background_import_still_running\n\t# *after* evaluating the test conditions.\n\tbackground_import_then_checkpoint () {\n\t\toptions=$1\n\t\tinput_file=$2\n\t\n\t\trm -f V.result\n\t\n\t\t( cat \"$input_file\"\n\t\techo \"checkpoint\"\n\t\techo \"progress checkpoint\"\n\t\tsleep 3600 &\n\t\techo $! >V.pid\n\t\twait ) | git fast-import $options | read_checkpoint &\n\t\n\t\t# We don't mind if the pipeline has already died by the time the test\n\t\t# ends.\n\t\ttest_when_finished \"kill $(cat V.pid) || true\"\n\t\n\t\twhile ! test -f V.result\n\t\tdo\n\t\t\t# Try to sleep less than a second, if supported.\n\t\t\tsleep .1 2>/dev/null || sleep 1\n\t\tdone\n\t\treturn $(cat V.result)\n\t}\n\nDo we like this better?\n"},{"id":"329141","messageId":"xmqq3776z0jg.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"20170928125916.GB9439@dinwoodie.org","subject":"Re: [PATCH 1/1] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-29T02:44:19Z","receivedAt":"2017-09-29T02:44:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Adam Dinwoodie <adam@dinwoodie.org> writes:\n\n>> Also adding the necessary PIPE prereq, as pointed out by Ramsay Jones.\n>\n> Cygwin doesn't have the PIPE prereq; I've just confirmed that the\n> previous version of this patch has t9300 failing on Cygwin, but this\n> version passes.\n\nThanks for a report.  So the patch should be good to go, it seems to\nme.\n\nEric, thanks again for working on this one.\n"},{"id":"329142","messageId":"0cb786584bd2669763c303f80072baa3693efc33.1506654123.git.e@nanocritical.com","threadId":"46834","inReplyTo":"xmqq3776z0jg.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-29T03:09:36Z","receivedAt":"2017-09-29T03:09:48Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"The checkpoint command cycles packfiles if object_count != 0, a sensible\ntest or there would be no pack files to write. Since 820b931012, the\ncommand also dumps branches, tags and marks, but still conditionally.\nHowever, it is possible for a command stream to modify refs or create\nmarks without creating any new objects.\n\nFor example, reset a branch (and keep fast-import running):\n\n\t$ git fast-import\n\treset refs/heads/master\n\tfrom refs/heads/master^\n\n\tcheckpoint\n\nbut refs/heads/master remains unchanged.\n\nOther example: a commit command that re-creates an object that already\nexists in the object database.\n\nThe man page also states that checkpoint \"updates the refs\" and that\n\"placing a progress command immediately after a checkpoint will inform\nthe reader when the checkpoint has been completed and it can safely\naccess the refs that fast-import updated\". This wasn't always true\nwithout this patch.\n\nThis fix unconditionally calls dump_{branches,tags,marks}() for all\ncheckpoint commands. dump_branches() and dump_tags() are cheap to call\nin the case of a no-op.\n\nAdd tests to t9300 that observe the (non-packfiles) effects of\ncheckpoint.\n\nSigned-off-by: Eric Rannaud <e@nanocritical.com>\n---\n fast-import.c          |   6 +--\n t/t9300-fast-import.sh | 130 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 133 insertions(+), 3 deletions(-)\n\n\nJunio, this last version addresses your last remark regarding the\npotential for the cat $input_file sequence to block when the FIFOs are\nunbuffered.\n\nThe concern is mainly theoretical (*if* the shell function is used\ncorrectly): fast-import will not start writing to its own output until\nit has fully consumed its input (as the only command generating output\nshould be the last one). Nevertheless, in this version the write is done\nin the background.\n\nThanks!\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 35bf671f12c4..d5e4cf0bad41 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3189,10 +3189,10 @@ static void checkpoint(void)\n \tcheckpoint_requested = 0;\n \tif (object_count) {\n \t\tcycle_packfile();\n-\t\tdump_branches();\n-\t\tdump_tags();\n-\t\tdump_marks();\n \t}\n+\tdump_branches();\n+\tdump_tags();\n+\tdump_marks();\n }\n \n static void parse_checkpoint(void)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 67b8c50a5ab4..8f583e8a22c1 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3120,4 +3120,134 @@ test_expect_success 'U: validate root delete result' '\n \tcompare_diff_raw expect actual\n '\n \n+###\n+### series V (checkpoint)\n+###\n+\n+# The commands in input_file should not produce any output on the file\n+# descriptor set with --cat-blob-fd (or stdout if unspecified).\n+#\n+# To make sure you're observing the side effects of checkpoint *before*\n+# fast-import terminates (and thus writes out its state), check that the\n+# fast-import process is still running using background_import_still_running\n+# *after* evaluating the test conditions.\n+background_import_then_checkpoint () {\n+\toptions=$1\n+\tinput_file=$2\n+\n+\tmkfifo V.input\n+\texec 8<>V.input\n+\trm V.input\n+\n+\tmkfifo V.output\n+\texec 9<>V.output\n+\trm V.output\n+\n+\tgit fast-import $options <&8 >&9 &\n+\techo $! >V.pid\n+\t# We don't mind if fast-import has already died by the time the test\n+\t# ends.\n+\ttest_when_finished \"exec 8>&-; exec 9>&-; kill $(cat V.pid) || true\"\n+\n+\t# Start in the background to ensure we adhere strictly to (blocking)\n+\t# pipes writing sequence. We want to assume that the write below could\n+\t# block, e.g. if fast-import blocks writing its own output to &9\n+\t# because there is no reader on &9 yet.\n+\t( cat \"$input_file\"\n+\techo \"checkpoint\"\n+\techo \"progress checkpoint\" ) >&8 &\n+\n+\terror=0\n+\tif read output <&9\n+\tthen\n+\t\tif ! test \"$output\" = \"progress checkpoint\"\n+\t\tthen\n+\t\t\techo >&2 \"no progress checkpoint received: $output\"\n+\t\t\terror=1\n+\t\tfi\n+\telse\n+\t\techo >&2 \"failed to read fast-import output\"\n+\t\terror=1\n+\tfi\n+\n+\tif test $error -eq 1\n+\tthen\n+\t\tfalse\n+\tfi\n+}\n+\n+background_import_still_running () {\n+\tif ! kill -0 \"$(cat V.pid)\"\n+\tthen\n+\t\techo >&2 \"background fast-import terminated too early\"\n+\t\tfalse\n+\tfi\n+}\n+\n+test_expect_success PIPE 'V: checkpoint updates refs after reset' '\n+\tcat >input <<-\\INPUT_END &&\n+\treset refs/heads/V\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"\" input &&\n+\ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V\n+\tmark :1\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V^)\" = \"$(git rev-parse --verify U)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+# Re-create the exact same commit, but on a different branch: no new object is\n+# created in the database, but the refs and marks still need to be updated.\n+test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no new objects)' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/V2\n+\tmark :2\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n+\n+\techo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n+\n+\ttest \"$(git rev-parse --verify V2)\" = \"$(git rev-parse --verify V)\" &&\n+\ttest_cmp marks.expected marks.actual &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n+\tcat >input <<-INPUT_END &&\n+\ttag Vtag\n+\tfrom refs/heads/V\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"\" input &&\n+\tgit show-ref -d Vtag &&\n+\tbackground_import_still_running\n+'\n+\n test_done\n-- \n2.14.1\n\n"},{"id":"329145","messageId":"xmqqy3oyxiuo.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"0cb786584bd2669763c303f80072baa3693efc33.1506654123.git.e@nanocritical.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-29T03:51:43Z","receivedAt":"2017-09-29T03:51:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Eric Rannaud\" <e@nanocritical.com> writes:\n\n> Junio, this last version addresses your last remark regarding the\n> potential for the cat $input_file sequence to block when the FIFOs are\n> unbuffered.\n>\n> The concern is mainly theoretical (*if* the shell function is used\n> correctly): fast-import will not start writing to its own output until\n> it has fully consumed its input (as the only command generating output\n> should be the last one). Nevertheless, in this version the write is done\n> in the background.\n\nI agree that the concern is theoretical.  Without this fix, we may\nnot be able to feed the input fully up to the final 'progress\ncheckpoint' command---because the other side is not reading, we may\nget stuck while attempting to write \"checkpoint\" or \"progress\ncheckpoint\", and may never get to read what fast-import says to get\nus unstuck.\n\nBut if we wanted to solve the theoretical issue (i.e. the command\nsequence the user of this helper shell function gives us _might_\ntrigger an output from fast-import) fully, I do not think\nbackgrounding the feeding side is sufficient.  The code that reads\nfd#9 would need to become a while loop that reads and discards extra\noutput until we see the \"progress checkpoint\", at least, perhaps\nlike the attached patch.\n\nBut even with such a fix, we are still at the mercy of the caller of\nthe helper---the helper will get broken if the input happened to\nhave a \"progress checkpoint\" command itself.  There has to be a\n\"good enough\" place to stop.\n\nI think that your patch the last round that feeds fd#8 in the\nforeground (i.e. fully trusting that the caller is sensibly giving\ninput that produces no output) is already a good place to stop.\n\nYour patch this round that feeds fd#8 in the background, plus the\nattached patch (i.e. not trusting the caller as much and allowing it\nto use commands that outputs something, within reason), would also\nbe a good place to stop.\n\nBut I am not sure your patch this round alone is a good place to\nstop.  It somehow feels halfway either way.\n\n t/t9300-fast-import.sh | 30 ++++++++++++++++++++----------\n 1 file changed, 20 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 74ba70874b..d47560b634 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3159,18 +3159,17 @@ background_import_then_checkpoint () {\n \t\techo \"progress checkpoint\"\n \t) >&8 &\n \n-\terror=0\n-\tif read output <&9\n-\tthen\n-\t\tif ! test \"$output\" = \"progress checkpoint\"\n+\terror=1 ;# assume the worst\n+\twhile read output <&9\n+\tdo\n+\t\tif test \"$output\" = \"progress checkpoint\"\n \t\tthen\n-\t\t\techo >&2 \"no progress checkpoint received: $output\"\n-\t\t\terror=1\n+\t\t\terror=0\n+\t\t\tbreak\n \t\tfi\n-\telse\n-\t\techo >&2 \"failed to read fast-import output\"\n-\t\terror=1\n-\tfi\n+\t\t# otherwise ignore cruft\n+\t\techo >&2 \"cruft: $output\"\n+\tdone\n \n \tif test $error -eq 1\n \tthen\n@@ -3186,6 +3185,17 @@ background_import_still_running () {\n \tfi\n }\n \n+test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra output' '\n+\tcat >input <<-INPUT_END &&\n+\tprogress foo\n+\tprogress bar\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"\" input &&\n+\tbackground_import_still_running\n+'\n+\n test_expect_success PIPE 'V: checkpoint updates refs after reset' '\n \tcat >input <<-\\INPUT_END &&\n \treset refs/heads/V\n-- \n2.14.2-820-gd7428e091c\n\n\n\n"},{"id":"329152","messageId":"CAH_=xobLo+K74HJ3a+XK4A3ok-FZJkAkO-9griaETkH50Qtp-A@mail.gmail.com","threadId":"46834","inReplyTo":"xmqqy3oyxiuo.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2017-09-29T05:40:29Z","receivedAt":"2017-09-29T05:40:56Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Thu, Sep 28, 2017 at 8:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> I think that your patch the last round that feeds fd#8 in the\n> foreground (i.e. fully trusting that the caller is sensibly giving\n> input that produces no output) is already a good place to stop.\n>\n> Your patch this round that feeds fd#8 in the background, plus the\n> attached patch (i.e. not trusting the caller as much and allowing it\n> to use commands that outputs something, within reason), would also\n> be a good place to stop.\n>\n> But I am not sure your patch this round alone is a good place to\n> stop.  It somehow feels halfway either way.\n\nI agree. If we're coding defensively against the caller, we do have to\ninclude your patch to be effective, you're right. I reckon we likely\ndon't need to be quite so paranoid, at least until this has more\nusers.\n\nThanks.\n"},{"id":"329165","messageId":"xmqqbmltx2xv.fsf@gitster.mtv.corp.google.com","threadId":"46834","inReplyTo":"CAH_=xobLo+K74HJ3a+XK4A3ok-FZJkAkO-9griaETkH50Qtp-A@mail.gmail.com","subject":"Re: [PATCH] fast-import: checkpoint: dump branches/tags/marks even if object_count==0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-29T09:35:24Z","receivedAt":"2017-09-29T09:35:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Rannaud <e@nanocritical.com> writes:\n\n> On Thu, Sep 28, 2017 at 8:51 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> I think that your patch the last round that feeds fd#8 in the\n>> foreground (i.e. fully trusting that the caller is sensibly giving\n>> input that produces no output) is already a good place to stop.\n>>\n>> Your patch this round that feeds fd#8 in the background, plus the\n>> attached patch (i.e. not trusting the caller as much and allowing it\n>> to use commands that outputs something, within reason), would also\n>> be a good place to stop.\n>>\n>> But I am not sure your patch this round alone is a good place to\n>> stop.  It somehow feels halfway either way.\n>\n> I agree. If we're coding defensively against the caller, we do have to\n> include your patch to be effective, you're right. I reckon we likely\n> don't need to be quite so paranoid, at least until this has more\n> users.\n\nOK, let's then pick the (not too excessively) defensive version by\ntaking your last one and suggested \"while\" loop squashed into it.\n\nThanks.\n"}]}