{"thread":{"id":"51066","subject":"[PATCH v3] fast-import: checkpoint: only write out refs and tags if we changed them","startedAt":"2019-05-10T08:41:31Z","lastAt":"2019-05-16T05:54:34Z","messageCount":5,"participants":["Eric Rannaud","Junio C Hamano","Elijah Newren"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"375283","messageId":"ad63393c24c7346c5b606b29579dfacad4d307f3.1557477247.git.e@nanocritical.com","threadId":"51066","inReplyTo":null,"subject":"[PATCH v3] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-10T08:41:15Z","receivedAt":"2019-05-10T08:41:31Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"At a checkpoint, fast-import resets all branches and tags on disk to the\nOID that we have in memory. If --force is not given, only branch resets\nthat result in fast forwards with respect to the state on disk are\nallowed.\n\nWith this approach, even for refs that fast-import has not been\ncommanded to change for a long time (or ever), at each checkpoint, we\nwill systematically reset them to the last state this process knows\nabout, a state which may have been set before the previous checkpoint.\nAny changes made to these refs by a different process since the last\ncheckpoint will be overwritten.\n\n1> is one process, 2> is another:\n\n\t1> $ git fast-import --force\n\t1> reset refs/heads/master\n\t1> from $A\n\t1> checkpoint\n\t2> $ git branch -f refs/heads/master $B\n\t1> reset refs/heads/tip\n\t1> from $C\n\t1> checkpoint\n\nAt this point master points again to $A.\n\nThis problem is mitigated somewhat for branches when --force is not\nspecified (the default), as requiring a fast forward means in practice\nthat concurrent external changes are likely to be preserved. But it's\nnot foolproof either:\n\n\t1> $ git fast-import\n\t1> reset refs/heads/master\n\t1> from $A\n\t1> checkpoint\n\t2> $ git branch -f refs/heads/master refs/heads/master~1\n\t1> reset refs/heads/tip\n\t1> from $C\n\t1> checkpoint\n\nAt this point, master points again to $A, not $A~1 as requested by the\nsecond process.\n\nWe now keep track of whether branches and tags have been changed by this\nfast-import process since our last checkpoint (or startup). At the next\ncheckpoint, only refs and tags that new commands have changed are\nwritten to disk.\n---\n fast-import.c          |  14 ++++++\n t/t9300-fast-import.sh | 106 ++++++++++++++++++++++++++++++++++-------\n 2 files changed, 104 insertions(+), 16 deletions(-)\n\n\nv2 and v1 were sent in Oct 2018. Only difference since is that the new\ntest cases in t9300 don't use sleep to implement the asynchronous dance.\n\nThanks.\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex f38d04fa5851..e9c3ea23ae42 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -103,6 +103,7 @@ struct branch {\n \tuintmax_t num_notes;\n \tunsigned active : 1;\n \tunsigned delete : 1;\n+\tunsigned changed : 1;\n \tunsigned pack_id : PACK_ID_BITS;\n \tstruct object_id oid;\n };\n@@ -110,6 +111,7 @@ struct branch {\n struct tag {\n \tstruct tag *next_tag;\n \tconst char *name;\n+\tunsigned changed : 1;\n \tunsigned int pack_id;\n \tstruct object_id oid;\n };\n@@ -581,6 +583,7 @@ static struct branch *new_branch(const char *name)\n \tb->branch_tree.versions[1].mode = S_IFDIR;\n \tb->num_notes = 0;\n \tb->active = 0;\n+\tb->changed = 0;\n \tb->pack_id = MAX_PACK_ID;\n \tbranch_table[hc] = b;\n \tbranch_count++;\n@@ -1571,6 +1574,10 @@ static int update_branch(struct branch *b)\n \tstruct object_id old_oid;\n \tstruct strbuf err = STRBUF_INIT;\n \n+\tif (!b->changed)\n+\t\treturn 0;\n+\tb->changed = 0;\n+\n \tif (is_null_oid(&b->oid)) {\n \t\tif (b->delete)\n \t\t\tdelete_ref(NULL, b->name, NULL, 0);\n@@ -1636,6 +1643,10 @@ static void dump_tags(void)\n \t\tgoto cleanup;\n \t}\n \tfor (t = first_tag; t; t = t->next_tag) {\n+\t\tif (!t->changed)\n+\t\t\tcontinue;\n+\t\tt->changed = 0;\n+\n \t\tstrbuf_reset(&ref_name);\n \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n \n@@ -2679,6 +2690,7 @@ static void parse_new_commit(const char *arg)\n \n \tif (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))\n \t\tb->pack_id = pack_id;\n+\tb->changed = 1;\n \tb->last_commit = object_count_by_type[OBJ_COMMIT];\n }\n \n@@ -2763,6 +2775,7 @@ static void parse_new_tag(const char *arg)\n \t\tt->pack_id = MAX_PACK_ID;\n \telse\n \t\tt->pack_id = pack_id;\n+\tt->changed = 1;\n }\n \n static void parse_reset_branch(const char *arg)\n@@ -2783,6 +2796,7 @@ static void parse_reset_branch(const char *arg)\n \t\tb = new_branch(arg);\n \tread_next_command();\n \tparse_from(b);\n+\tb->changed = 1;\n \tif (command_buf.len > 0)\n \t\tunread_command_buf = 1;\n }\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 3668263c4046..12abaebb22b6 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3121,14 +3121,23 @@ test_expect_success 'U: validate root delete result' '\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+# Instructions can be sent from $input_file to background_import() loop via the\n+# fast-import progress command.\n+# \n+# \tprogress do: shell\n+# Parse the next progress line and invoke it as a shell command.\n+# \n+# \tprogress do: checkpoint and stop\n+# Send checkpoint and wait for its completion.\n+# \n+# \tprogress do: stop\n+# Internal instruction.\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+background_import () {\n \toptions=$1\n \tinput_file=$2\n \n@@ -3153,22 +3162,30 @@ background_import_then_checkpoint () {\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(\n-\t\tcat \"$input_file\"\n-\t\techo \"checkpoint\"\n-\t\techo \"progress checkpoint\"\n-\t) >&8 &\n+\tcat \"$input_file\" >&8 &\n \n \terror=1 ;# assume the worst\n \twhile read output <&9\n \tdo\n-\t\tif test \"$output\" = \"progress checkpoint\"\n+\t\tif test \"$output\" = \"progress do: shell\"\n+\t\tthen\n+\t\t\tread output <&9\n+\t\t\tshell_cmd=\"$(echo $output |sed -e 's/^progress //')\"\n+\t\t\t$shell_cmd\n+\t\telif test \"$output\" = \"progress do: checkpoint and stop\"\n+\t\tthen\n+\t\t\t(\n+\t\t\t\techo \"checkpoint\"\n+\t\t\t\techo \"progress do: stop\"\n+\t\t\t) >&8 &\n+\t\telif test \"$output\" = \"progress do: stop\"\n \t\tthen\n \t\t\terror=0\n \t\t\tbreak\n+\t\telse\n+\t\t\t# otherwise ignore cruft\n+\t\t\techo >&2 \"cruft: $output\"\n \t\tfi\n-\t\t# otherwise ignore cruft\n-\t\techo >&2 \"cruft: $output\"\n \tdone\n \n \tif test $error -eq 1\n@@ -3189,10 +3206,11 @@ test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra out\n \tcat >input <<-INPUT_END &&\n \tprogress foo\n \tprogress bar\n+\tprogress do: checkpoint and stop\n \n \tINPUT_END\n \n-\tbackground_import_then_checkpoint \"\" input &&\n+\tbackground_import \"\" input &&\n \tbackground_import_still_running\n '\n \n@@ -3201,9 +3219,11 @@ test_expect_success PIPE 'V: checkpoint updates refs after reset' '\n \treset refs/heads/V\n \tfrom refs/heads/U\n \n+\tprogress do: checkpoint and stop\n+\n \tINPUT_END\n \n-\tbackground_import_then_checkpoint \"\" input &&\n+\tbackground_import \"\" input &&\n \ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n \tbackground_import_still_running\n '\n@@ -3216,9 +3236,11 @@ test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '\n \tdata 0\n \tfrom refs/heads/U\n \n+\tprogress do: checkpoint and stop\n+\n \tINPUT_END\n \n-\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n+\tbackground_import \"--export-marks=marks.actual\" input &&\n \n \techo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n \n@@ -3237,9 +3259,11 @@ test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no\n \tdata 0\n \tfrom refs/heads/U\n \n+\tprogress do: checkpoint and stop\n+\n \tINPUT_END\n \n-\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n+\tbackground_import \"--export-marks=marks.actual\" input &&\n \n \techo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n \n@@ -3255,13 +3279,63 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n \ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n \tdata 0\n \n+\tprogress do: checkpoint and stop\n+\n \tINPUT_END\n \n-\tbackground_import_then_checkpoint \"\" input &&\n+\tbackground_import \"\" input &&\n \tgit show-ref -d Vtag &&\n \tbackground_import_still_running\n '\n \n+test_expect_success PIPE 'V: checkpoint only resets changed branches' '\n+\tcat >input <<-INPUT_END &&\n+\treset refs/heads/V3\n+\tfrom refs/heads/U\n+\n+\tcheckpoint\n+\n+\tprogress do: shell\n+\tprogress git branch -f V3 V\n+\n+\treset refs/heads/V4\n+\tfrom refs/heads/U\n+\n+\tprogress do: checkpoint and stop\n+\n+\tINPUT_END\n+\n+\tbackground_import \"--force\" input &&\n+\ttest \"$(git rev-parse --verify V3)\" = \"$(git rev-parse --verify V)\" &&\n+\tbackground_import_still_running\n+'\n+\n+test_expect_success PIPE 'V: checkpoint only updates changed tags' '\n+\tcat >input <<-INPUT_END &&\n+\ttag Vtag2\n+\tfrom refs/heads/U\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tcheckpoint\n+\n+\tprogress do: shell\n+\tprogress git tag -f Vtag2 V\n+\n+\ttag Vtag3\n+\tfrom refs/heads/U\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tprogress do: checkpoint and stop\n+\n+\tINPUT_END\n+\n+\tbackground_import \"\" input &&\n+\ttest \"$(git show-ref -d Vtag2 |tail -1 |awk \\{print\\ \\$1\\})\" = \"$(git rev-parse --verify V)\" &&\n+\tbackground_import_still_running\n+'\n+\n ###\n ### series W (get-mark and empty orphan commits)\n ###\n-- \n2.21.0\n\n"},{"id":"375596","messageId":"xmqq8sv8709a.fsf@gitster-ct.c.googlers.com","threadId":"51066","inReplyTo":"ad63393c24c7346c5b606b29579dfacad4d307f3.1557477247.git.e@nanocritical.com","subject":"Re: [PATCH v3] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-05-15T05:30:25Z","receivedAt":"2019-05-15T05:30:33Z","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> We now keep track of whether branches and tags have been changed by this\n> fast-import process since our last checkpoint (or startup). At the next\n> checkpoint, only refs and tags that new commands have changed are\n> written to disk.\n\nAnd when we notice that we have been competing with some other\nprocess and the ref that we wanted to update has already been\nupdated to a different value, what happens?  That should be part of\nthe description of the new behaviour (aka \"fix\") written here.\n\n> ---\n\nMissing sign-off.\n\nIt sounds like a worthwhile thing to do.\n\nIt seems that Elijah has been active in fast-import/export area in\nthe last 12 months, so let's borrow a second set of eyes from him.\n\nThanks.\n\n>  fast-import.c          |  14 ++++++\n>  t/t9300-fast-import.sh | 106 ++++++++++++++++++++++++++++++++++-------\n>  2 files changed, 104 insertions(+), 16 deletions(-)\n>\n>\n> v2 and v1 were sent in Oct 2018. Only difference since is that the new\n> test cases in t9300 don't use sleep to implement the asynchronous dance.\n>\n> Thanks.\n>\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index f38d04fa5851..e9c3ea23ae42 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -103,6 +103,7 @@ struct branch {\n>  \tuintmax_t num_notes;\n>  \tunsigned active : 1;\n>  \tunsigned delete : 1;\n> +\tunsigned changed : 1;\n>  \tunsigned pack_id : PACK_ID_BITS;\n>  \tstruct object_id oid;\n>  };\n> @@ -110,6 +111,7 @@ struct branch {\n>  struct tag {\n>  \tstruct tag *next_tag;\n>  \tconst char *name;\n> +\tunsigned changed : 1;\n>  \tunsigned int pack_id;\n>  \tstruct object_id oid;\n>  };\n> @@ -581,6 +583,7 @@ static struct branch *new_branch(const char *name)\n>  \tb->branch_tree.versions[1].mode = S_IFDIR;\n>  \tb->num_notes = 0;\n>  \tb->active = 0;\n> +\tb->changed = 0;\n>  \tb->pack_id = MAX_PACK_ID;\n>  \tbranch_table[hc] = b;\n>  \tbranch_count++;\n> @@ -1571,6 +1574,10 @@ static int update_branch(struct branch *b)\n>  \tstruct object_id old_oid;\n>  \tstruct strbuf err = STRBUF_INIT;\n>  \n> +\tif (!b->changed)\n> +\t\treturn 0;\n> +\tb->changed = 0;\n> +\n>  \tif (is_null_oid(&b->oid)) {\n>  \t\tif (b->delete)\n>  \t\t\tdelete_ref(NULL, b->name, NULL, 0);\n> @@ -1636,6 +1643,10 @@ static void dump_tags(void)\n>  \t\tgoto cleanup;\n>  \t}\n>  \tfor (t = first_tag; t; t = t->next_tag) {\n> +\t\tif (!t->changed)\n> +\t\t\tcontinue;\n> +\t\tt->changed = 0;\n> +\n>  \t\tstrbuf_reset(&ref_name);\n>  \t\tstrbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n>  \n> @@ -2679,6 +2690,7 @@ static void parse_new_commit(const char *arg)\n>  \n>  \tif (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))\n>  \t\tb->pack_id = pack_id;\n> +\tb->changed = 1;\n>  \tb->last_commit = object_count_by_type[OBJ_COMMIT];\n>  }\n>  \n> @@ -2763,6 +2775,7 @@ static void parse_new_tag(const char *arg)\n>  \t\tt->pack_id = MAX_PACK_ID;\n>  \telse\n>  \t\tt->pack_id = pack_id;\n> +\tt->changed = 1;\n>  }\n>  \n>  static void parse_reset_branch(const char *arg)\n> @@ -2783,6 +2796,7 @@ static void parse_reset_branch(const char *arg)\n>  \t\tb = new_branch(arg);\n>  \tread_next_command();\n>  \tparse_from(b);\n> +\tb->changed = 1;\n>  \tif (command_buf.len > 0)\n>  \t\tunread_command_buf = 1;\n>  }\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 3668263c4046..12abaebb22b6 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -3121,14 +3121,23 @@ test_expect_success 'U: validate root delete result' '\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> +# Instructions can be sent from $input_file to background_import() loop via the\n> +# fast-import progress command.\n> +# \n> +# \tprogress do: shell\n> +# Parse the next progress line and invoke it as a shell command.\n> +# \n> +# \tprogress do: checkpoint and stop\n> +# Send checkpoint and wait for its completion.\n> +# \n> +# \tprogress do: stop\n> +# Internal instruction.\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> +background_import () {\n>  \toptions=$1\n>  \tinput_file=$2\n>  \n> @@ -3153,22 +3162,30 @@ background_import_then_checkpoint () {\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(\n> -\t\tcat \"$input_file\"\n> -\t\techo \"checkpoint\"\n> -\t\techo \"progress checkpoint\"\n> -\t) >&8 &\n> +\tcat \"$input_file\" >&8 &\n>  \n>  \terror=1 ;# assume the worst\n>  \twhile read output <&9\n>  \tdo\n> -\t\tif test \"$output\" = \"progress checkpoint\"\n> +\t\tif test \"$output\" = \"progress do: shell\"\n> +\t\tthen\n> +\t\t\tread output <&9\n> +\t\t\tshell_cmd=\"$(echo $output |sed -e 's/^progress //')\"\n> +\t\t\t$shell_cmd\n> +\t\telif test \"$output\" = \"progress do: checkpoint and stop\"\n> +\t\tthen\n> +\t\t\t(\n> +\t\t\t\techo \"checkpoint\"\n> +\t\t\t\techo \"progress do: stop\"\n> +\t\t\t) >&8 &\n> +\t\telif test \"$output\" = \"progress do: stop\"\n>  \t\tthen\n>  \t\t\terror=0\n>  \t\t\tbreak\n> +\t\telse\n> +\t\t\t# otherwise ignore cruft\n> +\t\t\techo >&2 \"cruft: $output\"\n>  \t\tfi\n> -\t\t# otherwise ignore cruft\n> -\t\techo >&2 \"cruft: $output\"\n>  \tdone\n>  \n>  \tif test $error -eq 1\n> @@ -3189,10 +3206,11 @@ test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra out\n>  \tcat >input <<-INPUT_END &&\n>  \tprogress foo\n>  \tprogress bar\n> +\tprogress do: checkpoint and stop\n>  \n>  \tINPUT_END\n>  \n> -\tbackground_import_then_checkpoint \"\" input &&\n> +\tbackground_import \"\" input &&\n>  \tbackground_import_still_running\n>  '\n>  \n> @@ -3201,9 +3219,11 @@ test_expect_success PIPE 'V: checkpoint updates refs after reset' '\n>  \treset refs/heads/V\n>  \tfrom refs/heads/U\n>  \n> +\tprogress do: checkpoint and stop\n> +\n>  \tINPUT_END\n>  \n> -\tbackground_import_then_checkpoint \"\" input &&\n> +\tbackground_import \"\" input &&\n>  \ttest \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n>  \tbackground_import_still_running\n>  '\n> @@ -3216,9 +3236,11 @@ test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '\n>  \tdata 0\n>  \tfrom refs/heads/U\n>  \n> +\tprogress do: checkpoint and stop\n> +\n>  \tINPUT_END\n>  \n> -\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n> +\tbackground_import \"--export-marks=marks.actual\" input &&\n>  \n>  \techo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n>  \n> @@ -3237,9 +3259,11 @@ test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no\n>  \tdata 0\n>  \tfrom refs/heads/U\n>  \n> +\tprogress do: checkpoint and stop\n> +\n>  \tINPUT_END\n>  \n> -\tbackground_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n> +\tbackground_import \"--export-marks=marks.actual\" input &&\n>  \n>  \techo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n>  \n> @@ -3255,13 +3279,63 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n>  \ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n>  \tdata 0\n>  \n> +\tprogress do: checkpoint and stop\n> +\n>  \tINPUT_END\n>  \n> -\tbackground_import_then_checkpoint \"\" input &&\n> +\tbackground_import \"\" input &&\n>  \tgit show-ref -d Vtag &&\n>  \tbackground_import_still_running\n>  '\n>  \n> +test_expect_success PIPE 'V: checkpoint only resets changed branches' '\n> +\tcat >input <<-INPUT_END &&\n> +\treset refs/heads/V3\n> +\tfrom refs/heads/U\n> +\n> +\tcheckpoint\n> +\n> +\tprogress do: shell\n> +\tprogress git branch -f V3 V\n> +\n> +\treset refs/heads/V4\n> +\tfrom refs/heads/U\n> +\n> +\tprogress do: checkpoint and stop\n> +\n> +\tINPUT_END\n> +\n> +\tbackground_import \"--force\" input &&\n> +\ttest \"$(git rev-parse --verify V3)\" = \"$(git rev-parse --verify V)\" &&\n> +\tbackground_import_still_running\n> +'\n> +\n> +test_expect_success PIPE 'V: checkpoint only updates changed tags' '\n> +\tcat >input <<-INPUT_END &&\n> +\ttag Vtag2\n> +\tfrom refs/heads/U\n> +\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata 0\n> +\n> +\tcheckpoint\n> +\n> +\tprogress do: shell\n> +\tprogress git tag -f Vtag2 V\n> +\n> +\ttag Vtag3\n> +\tfrom refs/heads/U\n> +\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata 0\n> +\n> +\tprogress do: checkpoint and stop\n> +\n> +\tINPUT_END\n> +\n> +\tbackground_import \"\" input &&\n> +\ttest \"$(git show-ref -d Vtag2 |tail -1 |awk \\{print\\ \\$1\\})\" = \"$(git rev-parse --verify V)\" &&\n> +\tbackground_import_still_running\n> +'\n> +\n>  ###\n>  ### series W (get-mark and empty orphan commits)\n>  ###\n"},{"id":"375603","messageId":"CAH_=xoZfx3aLpc+HZQ0FLZnRTkYW1wW7XN0huuOsT9bRJKzOKg@mail.gmail.com","threadId":"51066","inReplyTo":"xmqq8sv8709a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-15T06:57:40Z","receivedAt":"2019-05-15T06:57:55Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Tue, May 14, 2019 at 10:30 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Eric Rannaud\" <e@nanocritical.com> writes:\n>\n> > We now keep track of whether branches and tags have been changed by this\n> > fast-import process since our last checkpoint (or startup). At the next\n> > checkpoint, only refs and tags that new commands have changed are\n> > written to disk.\n>\n> And when we notice that we have been competing with some other\n> process and the ref that we wanted to update has already been\n> updated to a different value, what happens?  That should be part of\n> the description of the new behaviour (aka \"fix\") written here.\n\nThis isn't addressed by this change, that behavior remains the same as\nbefore. This change does not introduce a new failure mode. That is, we\nstill overwrite external changes if fast-import has itself been\ninstructed to change the ref since the last checkpoint (but without\n--force it must be a fast forward wrt the current state on disk).\n\nWith this change applied:\n\n        1> $ git fast-import --force\n        1> reset refs/heads/master\n        1> from $A\n        1> checkpoint\n        2> $ git branch -f refs/heads/master $B\n        1> reset refs/heads/master\n        1> from $C\n        1> checkpoint\n\nmaster points to $C.\n\nI can update the commit message with more details.\n\nWe could provide more complete transactional semantics, I suppose,\ni.e. only update a ref if:\n    - (1) fast-import has been instructed to change it (changed == 1)\nsince the last checkpoint (this patch),\n    - and (2) the ref on disk is identical to the one fast-import\nwrote at the previous checkpoint (or is identical to the one loaded\nfrom disk if we never modified it).\nWould we also need a command to get fast-import to read some/all refs\nfrom disk and update its internal state? But maybe this is going too\nfar.\n\nIn any case, the sequence above would then fail. But how should we\nfail? The same way we do for checkpoints which would result in non-FF\nwhen --force is not supplied? That is: don't update this ref, print a\nwarning, update other refs, continue processing the input, then\neventually exit with 1.\n\nOne problem with the current warning is that it goes to stderr and\nthere is no reliable way to detect on --cat-blob-fd that a checkpoint\nfailed. It is typical to use things like:\n\n        checkpoint\n        progress checkpoint\n\nand wait to read \"progress checkpoint\" on --cat-blob-fd to know when a\ncheckpoint is complete. It works fine if checkpoints cannot fail. For\na long-running fast-import process, an error would not necessarily be\nnoticed until EOF or \"done\", and you wouldn't easily be able to tell\nwhich checkpoint actually failed.\n\nIf we want to improve things beyond what this patch does, I think that\na reasonable balance would be:\n\n    - (a) improve the existing FF-only transaction by providing an\nimmediate error on --cat-blob-fd if a checkpoint fails (the error\nshould specify the list of refs that were not updated); for simplicity\nof processing the output of fast-import, we likely want to also print\nsomething for successful checkpoints: \"git fast-import\n--checkpoint-success\", and checkpoint prints on --cat-blob-fd:\n\n        [checkpoint failed <ref> LF]*\n        checkpoint complete LF\n\n    - (b) add a --force option to the \"checkpoint\" command, bypassing\nthe FF-only check on demand so that an interactive program can retry a\ncheckpoint after reading the state on disk, and maybe eventually give\nup and force the update.\n\n\n> > ---\n>\n> Missing sign-off.\n\n\nThanks. I will resend with a better explanation and sign-off.\n\n\nOn Tue, May 14, 2019 at 10:30 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Eric Rannaud\" <e@nanocritical.com> writes:\n>\n> > We now keep track of whether branches and tags have been changed by this\n> > fast-import process since our last checkpoint (or startup). At the next\n> > checkpoint, only refs and tags that new commands have changed are\n> > written to disk.\n>\n> And when we notice that we have been competing with some other\n> process and the ref that we wanted to update has already been\n> updated to a different value, what happens?  That should be part of\n> the description of the new behaviour (aka \"fix\") written here.\n>\n> > ---\n>\n> Missing sign-off.\n>\n> It sounds like a worthwhile thing to do.\n>\n> It seems that Elijah has been active in fast-import/export area in\n> the last 12 months, so let's borrow a second set of eyes from him.\n>\n> Thanks.\n>\n> >  fast-import.c          |  14 ++++++\n> >  t/t9300-fast-import.sh | 106 ++++++++++++++++++++++++++++++++++-------\n> >  2 files changed, 104 insertions(+), 16 deletions(-)\n> >\n> >\n> > v2 and v1 were sent in Oct 2018. Only difference since is that the new\n> > test cases in t9300 don't use sleep to implement the asynchronous dance.\n> >\n> > Thanks.\n> >\n> >\n> > diff --git a/fast-import.c b/fast-import.c\n> > index f38d04fa5851..e9c3ea23ae42 100644\n> > --- a/fast-import.c\n> > +++ b/fast-import.c\n> > @@ -103,6 +103,7 @@ struct branch {\n> >       uintmax_t num_notes;\n> >       unsigned active : 1;\n> >       unsigned delete : 1;\n> > +     unsigned changed : 1;\n> >       unsigned pack_id : PACK_ID_BITS;\n> >       struct object_id oid;\n> >  };\n> > @@ -110,6 +111,7 @@ struct branch {\n> >  struct tag {\n> >       struct tag *next_tag;\n> >       const char *name;\n> > +     unsigned changed : 1;\n> >       unsigned int pack_id;\n> >       struct object_id oid;\n> >  };\n> > @@ -581,6 +583,7 @@ static struct branch *new_branch(const char *name)\n> >       b->branch_tree.versions[1].mode = S_IFDIR;\n> >       b->num_notes = 0;\n> >       b->active = 0;\n> > +     b->changed = 0;\n> >       b->pack_id = MAX_PACK_ID;\n> >       branch_table[hc] = b;\n> >       branch_count++;\n> > @@ -1571,6 +1574,10 @@ static int update_branch(struct branch *b)\n> >       struct object_id old_oid;\n> >       struct strbuf err = STRBUF_INIT;\n> >\n> > +     if (!b->changed)\n> > +             return 0;\n> > +     b->changed = 0;\n> > +\n> >       if (is_null_oid(&b->oid)) {\n> >               if (b->delete)\n> >                       delete_ref(NULL, b->name, NULL, 0);\n> > @@ -1636,6 +1643,10 @@ static void dump_tags(void)\n> >               goto cleanup;\n> >       }\n> >       for (t = first_tag; t; t = t->next_tag) {\n> > +             if (!t->changed)\n> > +                     continue;\n> > +             t->changed = 0;\n> > +\n> >               strbuf_reset(&ref_name);\n> >               strbuf_addf(&ref_name, \"refs/tags/%s\", t->name);\n> >\n> > @@ -2679,6 +2690,7 @@ static void parse_new_commit(const char *arg)\n> >\n> >       if (!store_object(OBJ_COMMIT, &new_data, NULL, &b->oid, next_mark))\n> >               b->pack_id = pack_id;\n> > +     b->changed = 1;\n> >       b->last_commit = object_count_by_type[OBJ_COMMIT];\n> >  }\n> >\n> > @@ -2763,6 +2775,7 @@ static void parse_new_tag(const char *arg)\n> >               t->pack_id = MAX_PACK_ID;\n> >       else\n> >               t->pack_id = pack_id;\n> > +     t->changed = 1;\n> >  }\n> >\n> >  static void parse_reset_branch(const char *arg)\n> > @@ -2783,6 +2796,7 @@ static void parse_reset_branch(const char *arg)\n> >               b = new_branch(arg);\n> >       read_next_command();\n> >       parse_from(b);\n> > +     b->changed = 1;\n> >       if (command_buf.len > 0)\n> >               unread_command_buf = 1;\n> >  }\n> > diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> > index 3668263c4046..12abaebb22b6 100755\n> > --- a/t/t9300-fast-import.sh\n> > +++ b/t/t9300-fast-import.sh\n> > @@ -3121,14 +3121,23 @@ test_expect_success 'U: validate root delete result' '\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> > +# Instructions can be sent from $input_file to background_import() loop via the\n> > +# fast-import progress command.\n> > +#\n> > +#    progress do: shell\n> > +# Parse the next progress line and invoke it as a shell command.\n> > +#\n> > +#    progress do: checkpoint and stop\n> > +# Send checkpoint and wait for its completion.\n> > +#\n> > +#    progress do: stop\n> > +# Internal instruction.\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> > +background_import () {\n> >       options=$1\n> >       input_file=$2\n> >\n> > @@ -3153,22 +3162,30 @@ background_import_then_checkpoint () {\n> >       # pipes writing sequence. We want to assume that the write below could\n> >       # block, e.g. if fast-import blocks writing its own output to &9\n> >       # because there is no reader on &9 yet.\n> > -     (\n> > -             cat \"$input_file\"\n> > -             echo \"checkpoint\"\n> > -             echo \"progress checkpoint\"\n> > -     ) >&8 &\n> > +     cat \"$input_file\" >&8 &\n> >\n> >       error=1 ;# assume the worst\n> >       while read output <&9\n> >       do\n> > -             if test \"$output\" = \"progress checkpoint\"\n> > +             if test \"$output\" = \"progress do: shell\"\n> > +             then\n> > +                     read output <&9\n> > +                     shell_cmd=\"$(echo $output |sed -e 's/^progress //')\"\n> > +                     $shell_cmd\n> > +             elif test \"$output\" = \"progress do: checkpoint and stop\"\n> > +             then\n> > +                     (\n> > +                             echo \"checkpoint\"\n> > +                             echo \"progress do: stop\"\n> > +                     ) >&8 &\n> > +             elif test \"$output\" = \"progress do: stop\"\n> >               then\n> >                       error=0\n> >                       break\n> > +             else\n> > +                     # otherwise ignore cruft\n> > +                     echo >&2 \"cruft: $output\"\n> >               fi\n> > -             # otherwise ignore cruft\n> > -             echo >&2 \"cruft: $output\"\n> >       done\n> >\n> >       if test $error -eq 1\n> > @@ -3189,10 +3206,11 @@ test_expect_success PIPE 'V: checkpoint helper does not get stuck with extra out\n> >       cat >input <<-INPUT_END &&\n> >       progress foo\n> >       progress bar\n> > +     progress do: checkpoint and stop\n> >\n> >       INPUT_END\n> >\n> > -     background_import_then_checkpoint \"\" input &&\n> > +     background_import \"\" input &&\n> >       background_import_still_running\n> >  '\n> >\n> > @@ -3201,9 +3219,11 @@ test_expect_success PIPE 'V: checkpoint updates refs after reset' '\n> >       reset refs/heads/V\n> >       from refs/heads/U\n> >\n> > +     progress do: checkpoint and stop\n> > +\n> >       INPUT_END\n> >\n> > -     background_import_then_checkpoint \"\" input &&\n> > +     background_import \"\" input &&\n> >       test \"$(git rev-parse --verify V)\" = \"$(git rev-parse --verify U)\" &&\n> >       background_import_still_running\n> >  '\n> > @@ -3216,9 +3236,11 @@ test_expect_success PIPE 'V: checkpoint updates refs and marks after commit' '\n> >       data 0\n> >       from refs/heads/U\n> >\n> > +     progress do: checkpoint and stop\n> > +\n> >       INPUT_END\n> >\n> > -     background_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n> > +     background_import \"--export-marks=marks.actual\" input &&\n> >\n> >       echo \":1 $(git rev-parse --verify V)\" >marks.expected &&\n> >\n> > @@ -3237,9 +3259,11 @@ test_expect_success PIPE 'V: checkpoint updates refs and marks after commit (no\n> >       data 0\n> >       from refs/heads/U\n> >\n> > +     progress do: checkpoint and stop\n> > +\n> >       INPUT_END\n> >\n> > -     background_import_then_checkpoint \"--export-marks=marks.actual\" input &&\n> > +     background_import \"--export-marks=marks.actual\" input &&\n> >\n> >       echo \":2 $(git rev-parse --verify V2)\" >marks.expected &&\n> >\n> > @@ -3255,13 +3279,63 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n> >       tagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> >       data 0\n> >\n> > +     progress do: checkpoint and stop\n> > +\n> >       INPUT_END\n> >\n> > -     background_import_then_checkpoint \"\" input &&\n> > +     background_import \"\" input &&\n> >       git show-ref -d Vtag &&\n> >       background_import_still_running\n> >  '\n> >\n> > +test_expect_success PIPE 'V: checkpoint only resets changed branches' '\n> > +     cat >input <<-INPUT_END &&\n> > +     reset refs/heads/V3\n> > +     from refs/heads/U\n> > +\n> > +     checkpoint\n> > +\n> > +     progress do: shell\n> > +     progress git branch -f V3 V\n> > +\n> > +     reset refs/heads/V4\n> > +     from refs/heads/U\n> > +\n> > +     progress do: checkpoint and stop\n> > +\n> > +     INPUT_END\n> > +\n> > +     background_import \"--force\" input &&\n> > +     test \"$(git rev-parse --verify V3)\" = \"$(git rev-parse --verify V)\" &&\n> > +     background_import_still_running\n> > +'\n> > +\n> > +test_expect_success PIPE 'V: checkpoint only updates changed tags' '\n> > +     cat >input <<-INPUT_END &&\n> > +     tag Vtag2\n> > +     from refs/heads/U\n> > +     tagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> > +     data 0\n> > +\n> > +     checkpoint\n> > +\n> > +     progress do: shell\n> > +     progress git tag -f Vtag2 V\n> > +\n> > +     tag Vtag3\n> > +     from refs/heads/U\n> > +     tagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> > +     data 0\n> > +\n> > +     progress do: checkpoint and stop\n> > +\n> > +     INPUT_END\n> > +\n> > +     background_import \"\" input &&\n> > +     test \"$(git show-ref -d Vtag2 |tail -1 |awk \\{print\\ \\$1\\})\" = \"$(git rev-parse --verify V)\" &&\n> > +     background_import_still_running\n> > +'\n> > +\n> >  ###\n> >  ### series W (get-mark and empty orphan commits)\n> >  ###\n\n\n\n-- \nEric Rannaud <e@nanocritical.com>\nNanocritical, CEO\n"},{"id":"375675","messageId":"CABPp-BGZQz=4KVpZ6v+p12tDPMBJ8eFWuOH1mjQQrhpksU8NzQ@mail.gmail.com","threadId":"51066","inReplyTo":"xmqq8sv8709a.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-05-16T00:35:52Z","receivedAt":"2019-05-16T01:50:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, May 14, 2019 at 10:30 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> \"Eric Rannaud\" <e@nanocritical.com> writes:\n>\n> > We now keep track of whether branches and tags have been changed by this\n> > fast-import process since our last checkpoint (or startup). At the next\n> > checkpoint, only refs and tags that new commands have changed are\n> > written to disk.\n>\n> And when we notice that we have been competing with some other\n> process and the ref that we wanted to update has already been\n> updated to a different value, what happens?  That should be part of\n> the description of the new behaviour (aka \"fix\") written here.\n>\n> > ---\n>\n> Missing sign-off.\n>\n> It sounds like a worthwhile thing to do.\n>\n> It seems that Elijah has been active in fast-import/export area in\n> the last 12 months, so let's borrow a second set of eyes from him.\n\nI glanced over the fast-import.c portion of the code and it seemed\nsane, modulo this question about handling of failure to update\nbranches/tags during a checkpoint.  Didn't read the testcase portion\nof the patch as closely yet, but I think the big question is failures\nduring checkpoints.\n\nFirst, I have to admit to having never used checkpoints, and I'm\nstruggling a little bit to understand the usecase here.\n\nFor context on my angle, I've been always using --force and 'feature\ndone', which ensures that either all branches and tags successfully\nupdate, or fast-import terminates with having made no changes (other\nthan perhaps having written a garbage packfile that can be pruned).\nI'm particularly worried about the frontend hitting errors and not\ncompleting its output.  With that in mind, I was surprised to notice\nthat explicit checkpoints update branches and tags; it seemed\nincompatible with --done to me, though it turns out to only be\nincompatible with my intended use (no partial updates).\n\nIn your case, you are clearly interested in partial updates for some\npurpose.  It's easiest to figure out the 'right' behavior for partial\nupdates in conjunction with --force: as you state, if someone else\nupdates some ref after a checkpoint and the input stream continues to\nchange the ref, then tough luck for the external party; we were told\nto `--force` so we overwrite.  But the case without --force is most\ninteresting, as Junio highlighted.  I don't think that case has been\nthought about deeply in the past, for two reasons: (1) the fast-import\ndocumentation is somewhat dismissive of checkpoints (\"given that a 30\nGiB Subversion repository can be loaded into Git through fast-import\nin about 3 hours, explicit checkpointing may not be necessary\") , (2)\nI think most folks assume the repository isn't going to be interacted\nwith during the fast-import process.  However, the whole reason for\nyour patch is because checkpoints are in use *and* people are updating\nthe repo during the fast-import process, so this area that was likely\noverlooked in the past now is pretty important to get right.\n\nLooking at the fast-import code, it appears that update_branch and\nupdate_tag both set a global 'failure' flag whenever a branch cannot\nbe updated due to it not being a fast-forward.  It prints a \"warning\"\nbut this is likely because it wants to update whatever branches and\ntags it can and report issues on any it can't, and then after that's\ndone then exit.  Since branches and tags are typically just updated at\nthe very end of the process (again, based on the documentation\nsuggesting checkpoints aren't necessary), the end of program exit can\nalso be thought of as the \"exit immediately after failing branch/tag\nupdates\".  My current suspicion is that the current checkpointing code\nnot checking the 'failure' flag was an oversight.  If the user doesn't\nwant branches/tags forcibly updated, and they do want checkpoints when\nthe branches/tags get updated as input comes in, and there is the\npossibility that external folks modify thing, then I think it makes\nsense to abort the import once the checkpoint is finished (i.e. update\nwhatever branches/tags we can and then error out); continuing to\nprocess the input stream seems wrong, because:\n  * the reason to use checkpoints is because the fast-import process\nis taking a long time; if it was quick, there'd be no reason to\nmanually checkpoint.  Therefore...\n  * the user may be forced to backup and redo the long-lived\nfast-import process anyway; we've cost them time by not erroring out\nimmediately\n  * we allow errors to accumulate (other branches could have failures\nto update later in the fast-import process), causing users to have to\nfigure out whether one or many failures were reported and then have to\ntry to track down if they have one or multiple causes\n  * the users are less likely to notice early \"warning\" without\nerroring out, but the cause (someone else updating a ref) may be\nharder to diagnose the longer it has been since the external ref\nupdate.\n\nThat's my impression looking over your change and reading over the\nrelevant bits of fast-import.c, but as I noted before I don't\nunderstand the usecase and perhaps there's something important I'm not\ngrasping based on that.  It'd be great if you could provide more\ndetails to help us determine if we need to update the checkpoint code\nto appropriately handle errors.\n\n\nHope that helps,\nElijah\n"},{"id":"375702","messageId":"CAH_=xobEd6ZiifEJOnvW8KdN78zLWv5UirFTiEk722YyP_b5Kw@mail.gmail.com","threadId":"51066","inReplyTo":"CABPp-BGZQz=4KVpZ6v+p12tDPMBJ8eFWuOH1mjQQrhpksU8NzQ@mail.gmail.com","subject":"Re: [PATCH v3] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-16T05:54:19Z","receivedAt":"2019-05-16T05:54:34Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Wed, May 15, 2019 at 5:36 PM Elijah Newren <newren@gmail.com> wrote:\n> with during the fast-import process.  However, the whole reason for\n> your patch is because checkpoints are in use *and* people are updating\n> the repo during the fast-import process, so this area that was likely\n> overlooked in the past now is pretty important to get right.\n\nA little bit of background on our particular use case: fast-import is\nused to perform frequent history (re)writing and is a great (and fast)\nway to modify many refs without having to touch a checkout. For us, an\nalternative would be to use libgit but this is simpler, multiprocess\nand non-blocking, and doesn't have any additional dependencies.\n\nWe use --force: our fast-import frontend independently watches the\nrepo for updates so we are generally aware of external updates to the\nrepo. (And if we race with an external change, we can live with that\n-- although being able to finely control which ref updates we want to\nforce and which ones we expect to be FF would be great, but that's for\nanother patch.)\n\nWe would like to use checkpoints to keep fast-import running, which\nhas the advantage of keeping some Git state in memory. Restarting git\nfast-import every time we want to make a change is doable, but more\nexpensive.\n\nWith the current behavior, we have to shut down fast-import after\nevery checkpoint (in other words, we can't really use them) and\nrestart when needed, ensuring we don't overwrite refs we didn't intend\nto change this time. Alternatively, we could maintain an up-to-date\n(wrt .git/ on disk) mapping ref -> sha1 for all refs we've ever\nchanged, and make sure to precede any checkpoint with a bunch of\nintended-to-be-no-op reset commands, which effectively let fast-import\nknow about external changes to refs that we do not wish to overwrite.\n\nBut this patch seemed like a better path forward.\n\n\n> Looking at the fast-import code, it appears that update_branch and\n> update_tag both set a global 'failure' flag whenever a branch cannot\n> be updated due to it not being a fast-forward.  It prints a \"warning\"\n> but this is likely because it wants to update whatever branches and\n> tags it can and report issues on any it can't, and then after that's\n> done then exit.  Since branches and tags are typically just updated at\n> the very end of the process (again, based on the documentation\n> suggesting checkpoints aren't necessary), the end of program exit can\n> also be thought of as the \"exit immediately after failing branch/tag\n> updates\".  My current suspicion is that the current checkpointing code\n> not checking the 'failure' flag was an oversight.  If the user doesn't\n> want branches/tags forcibly updated, and they do want checkpoints when\n> the branches/tags get updated as input comes in, and there is the\n> possibility that external folks modify thing, then I think it makes\n> sense to abort the import once the checkpoint is finished (i.e. update\n> whatever branches/tags we can and then error out); continuing to\n> process the input stream seems wrong, because:\n>   * the reason to use checkpoints is because the fast-import process\n> is taking a long time; if it was quick, there'd be no reason to\n> manually checkpoint.  Therefore...\n>   * the user may be forced to backup and redo the long-lived\n> fast-import process anyway; we've cost them time by not erroring out\n> immediately\n>   * we allow errors to accumulate (other branches could have failures\n> to update later in the fast-import process), causing users to have to\n> figure out whether one or many failures were reported and then have to\n> try to track down if they have one or multiple causes\n>   * the users are less likely to notice early \"warning\" without\n> erroring out, but the cause (someone else updating a ref) may be\n> harder to diagnose the longer it has been since the external ref\n> update.\n\nWe use --force so I don't have an immediate interest in how checkpoint\nerrors are handled.\n\nIn a separate response to Junio, I suggested an alternative mechanism\nto signal checkpoint conflicts on --cat-blob-fd, but I think it would\ngenerally be fine if fast-import were to simply exit on such an error.\nTo reliably figure out which updates made it to disk and which didn't,\none can obtain expected OIDs with get-mark before initiating a\ncheckpoint, and compare with refs on disk after an error.\n\nI'm not sure if such a change should be behind a flag or not. I think\nanyone running without --force does care if a ref update is rejected;\nand we're unlikely to break much by erroring out early. But maybe some\nusers out there don't check the exit status of fast-import and haven't\nnoticed that some branch they don't really care about is not being\nupdated successfully while everything else gets updated fine?\n\nThanks for the thoughtful analysis.\n\nEric\n"}]}