{"thread":{"id":"51174","subject":"[PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","startedAt":"2019-05-24T23:10:17Z","lastAt":"2019-05-28T00:11:59Z","messageCount":6,"participants":["Eric Rannaud","Elijah Newren"],"isPatch":true,"patchVersion":4,"patchTotal":null},"messages":[{"id":"376210","messageId":"407a267d549bb83543e7046dc7cdf882a583b006.1558738737.git.e@nanocritical.com","threadId":"51174","inReplyTo":null,"subject":"[PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-24T23:09:56Z","receivedAt":"2019-05-24T23:10:17Z","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. fast-import never updates its internal view of branches in\nresponse to external changes.\n\nAlso, even for refs that fast-import has not been commanded to change\nfor a long time (or ever), at each checkpoint, we will systematically\nreset them to the last state this process knows about, a state which may\nhave been set before the previous checkpoint. Any changes made to these\nrefs by a different process since the last checkpoint will be\noverwritten.\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  # not changing master!\n\t1> from $C\n\t1> checkpoint\n\nAt this point master points again to $A. (With this patch applied,\nmaster points to $B.)\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  # not changing master!\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. (With this patch applied, master points to $A~1.)\n\nWith this patch, we keep track of whether a particular branch or tag has\nbeen changed by this fast-import process since our last checkpoint (or\nstartup). At the next checkpoint, only refs and tags that new commands\nhave changed are written to disk. To be clear, fast-import still does\nnot update its internal view of branches in response to external\nchanges, but now it avoids interfering with external changes unless\nthere was an explicit command to do so since the last checkpoint.\n\nNote, therefore, that fast-import will still overwrite external changes\nto some refs and tags. For example, with this patch applied, the change\nmade by process 2 will be overwritten:\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/master\n\t1> from $C\n\t1> checkpoint\n\nand master will point to $C.\n\nSigned-off-by: Eric Rannaud <e@nanocritical.com>\n---\n fast-import.c          |  14 ++++++\n t/t9300-fast-import.sh | 106 ++++++++++++++++++++++++++++++++++-------\n 2 files changed, 104 insertions(+), 16 deletions(-)\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":"376211","messageId":"CAH_=xoYQEvXKnhyZsUeZ=VTEMOpOdgbd-57f7-6M+2KQLSAAUQ@mail.gmail.com","threadId":"51174","inReplyTo":"407a267d549bb83543e7046dc7cdf882a583b006.1558738737.git.e@nanocritical.com","subject":"Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-25T02:18:15Z","receivedAt":"2019-05-25T02:18:30Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"(The patch is identical to v3 but I've tried to clarify the commit\nmessage and added the missing sign-off.)\n\nOn Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:\n>\n> At a checkpoint, fast-import resets all branches and tags on disk to the\n> OID that we have in memory. If --force is not given, only branch resets\n> that result in fast forwards with respect to the state on disk are\n> allowed. fast-import never updates its internal view of branches in\n> response to external changes.\n>\n> Also, even for refs that fast-import has not been commanded to change\n> for a long time (or ever), at each checkpoint, we will systematically\n> reset them to the last state this process knows about, a state which may\n> have been set before the previous checkpoint. Any changes made to these\n> refs by a different process since the last checkpoint will be\n> overwritten.\n>\n> 1> is one process, 2> is another:\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/tip  # not changing master!\n>         1> from $C\n>         1> checkpoint\n>\n> At this point master points again to $A. (With this patch applied,\n> master points to $B.)\n>\n> This problem is mitigated somewhat for branches when --force is not\n> specified (the default), as requiring a fast forward means in practice\n> that concurrent external changes are likely to be preserved. But it's\n> not foolproof either:\n>\n>         1> $ git fast-import\n>         1> reset refs/heads/master\n>         1> from $A\n>         1> checkpoint\n>         2> $ git branch -f refs/heads/master refs/heads/master~1\n>         1> reset refs/heads/tip  # not changing master!\n>         1> from $C\n>         1> checkpoint\n>\n> At this point, master points again to $A, not $A~1 as requested by the\n> second process. (With this patch applied, master points to $A~1.)\n>\n> With this patch, we keep track of whether a particular branch or tag has\n> been changed by this fast-import process since our last checkpoint (or\n> startup). At the next checkpoint, only refs and tags that new commands\n> have changed are written to disk. To be clear, fast-import still does\n> not update its internal view of branches in response to external\n> changes, but now it avoids interfering with external changes unless\n> there was an explicit command to do so since the last checkpoint.\n>\n> Note, therefore, that fast-import will still overwrite external changes\n> to some refs and tags. For example, with this patch applied, the change\n> made by process 2 will be overwritten:\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>\n> and master will point to $C.\n>\n> Signed-off-by: Eric Rannaud <e@nanocritical.com>\n> ---\n>  fast-import.c          |  14 ++++++\n>  t/t9300-fast-import.sh | 106 ++++++++++++++++++++++++++++++++++-------\n>  2 files changed, 104 insertions(+), 16 deletions(-)\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> 2.21.0\n>\n\n\n-- \nEric Rannaud <e@nanocritical.com>\nNanocritical, CEO\n"},{"id":"376254","messageId":"20190527211245.12292-1-newren@gmail.com","threadId":"51174","inReplyTo":"CAH_=xoYQEvXKnhyZsUeZ=VTEMOpOdgbd-57f7-6M+2KQLSAAUQ@mail.gmail.com","subject":"Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-05-27T21:12:45Z","receivedAt":"2019-05-27T21:12:56Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:\n>\n> With this patch, we keep track of whether a particular branch or tag has\n> been changed by this fast-import process since our last checkpoint (or\n> startup). At the next checkpoint, only refs and tags that new commands\n> have changed are written to disk. To be clear, fast-import still does\n> not update its internal view of branches in response to external\n> changes, but now it avoids interfering with external changes unless\n> there was an explicit command to do so since the last checkpoint.\n\nThis patch looks reasonable to me; you can view it one of two basic ways:\n  1) an optimization (only update what needs to be updated)\n  2) a way to have early access to a completed branch\n\nIn particular, if the fast-import stream is dealing with unrelated (or\nvery divergent) branches and it completes one branch before others,\nthen your change allows people to have early access to both read &\nupdate the completed branches while the import continues working on\nother branches.\n\nIf you changed your description to sell it this way, it'd be fine\nas-is.  But you use --force and make multiple mentions of concurrent\nupdates to branches in external processes during the fast-import\nprocess.  That kind of description makes it really clear we need to\ntighten up what happens with the checkpoint command when it hits a\nfailure (as mentioned in the commentary on V3).  Below is a patch that\ndoes this.\n\nI think we should either:\n\n  1) update your commit message to sell it without mentioning the\n     concurrent rewrites, and then I'll update my patch to not\n     conflict (we both add new tests at the same location to the same\n     file causing a simple conflict) by building on yours, OR\n     \n  2) update your patch to come after mine and add a comment to your\n     commit message about how checkpoint will abort if it hits an\n     error, suggesting that people should only update branches\n     fast-import will not be updating further or they should use\n     --force and deal with their changes being overwritten by\n     fast-import.  Then you can submit our two patches as a series.\n\nThoughts?\n\nElijah\n\n-- 8< --\nSubject: [PATCH] fast-import: remember to check failure flag when\n checkpointing\n\nfast-import, when finished, will flush out the current packfile and\nupdate branches, tags, and marks, returning whether it was successful.\nThe point of the 'checkpoint' command was to do all the same things, but\ncontinue processing the stream of input commands if it was successful.\nUnfortunately, the checkpoint code forgot to check the 'failure' flag to\nsee if there was an error in e.g. updating the branches, which meant it\nwould also continue if there was a failure.  Fix this oversight.\n\nSigned-off-by: Elijah Newren <newren@gmail.com>\n---\n fast-import.c          |  2 ++\n t/t9300-fast-import.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 44 insertions(+)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex f38d04fa58..d0e12b03a0 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3033,6 +3033,8 @@ static void checkpoint(void)\n \tdump_branches();\n \tdump_tags();\n \tdump_marks();\n+\tif (failure)\n+\t\texit(1);\n }\n \n static void parse_checkpoint(void)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 3668263c40..788a543b82 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3262,6 +3262,48 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n \tbackground_import_still_running\n '\n \n+background_fast_import_race () {\n+\toptions=$1\n+\tinput_file=$2\n+\texit_status=$3\n+\textra_cmd=\"$4\"\n+\n+\tmkfifo V.input\n+\texec 8<>V.input\n+\trm V.input\n+\n+\tgit fast-import $options <&8 &\n+\techo $! >V.pid &&\n+\t$extra_cmd &&\n+\n+\tcat $input_file >&8\n+\twait $(cat V.pid)\n+\ttest $? -eq $exit_status\n+}\n+\n+test_expect_success PIPE 'V: checkpoint fails if refs updated beforehand' '\n+\tgit checkout --orphan V3 &&\n+\tgit commit --allow-empty -m initial &&\n+\tINITIAL=$(git rev-parse HEAD) &&\n+\n+\tcat >input <<-INPUT_END &&\n+\tfeature done\n+\tcommit refs/heads/V3\n+\tmark :3\n+\tcommitter Me My <self@and.eye> 1234567890 +0123\n+\tdata 10\n+\tgenerated\n+\tfrom $INITIAL\n+\n+\tcheckpoint\n+\tdone\n+\tINPUT_END\n+\n+\tbackground_fast_import_race \"\" input 1 \"git commit --allow-empty -m conflicting\" &&\n+\tbackground_fast_import_race \"--force\" input 0 \"git commit --allow-empty -m conflicting\"\n+'\n+\n+\n ###\n ### series W (get-mark and empty orphan commits)\n ###\n-- \n2.22.0.rc1.dirty\n\n"},{"id":"376255","messageId":"CAH_=xoZ5Lt2nn50cfDbTA-ZVeshi8HZ91Od4btuUXpBGCanGng@mail.gmail.com","threadId":"51174","inReplyTo":"20190527211245.12292-1-newren@gmail.com","subject":"Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-27T22:45:49Z","receivedAt":"2019-05-27T22:46:03Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Mon, May 27, 2019 at 2:12 PM Elijah Newren <newren@gmail.com> wrote:\n> On Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:\n> >\n> > With this patch, we keep track of whether a particular branch or tag has\n> > been changed by this fast-import process since our last checkpoint (or\n> > startup). At the next checkpoint, only refs and tags that new commands\n> > have changed are written to disk. To be clear, fast-import still does\n> > not update its internal view of branches in response to external\n> > changes, but now it avoids interfering with external changes unless\n> > there was an explicit command to do so since the last checkpoint.\n>\n> This patch looks reasonable to me; you can view it one of two basic ways:\n>   1) an optimization (only update what needs to be updated)\n>   2) a way to have early access to a completed branch\n>\n> In particular, if the fast-import stream is dealing with unrelated (or\n> very divergent) branches and it completes one branch before others,\n> then your change allows people to have early access to both read &\n> update the completed branches while the import continues working on\n> other branches.\n\nThat's a good way to describe how people with a more standard use case\nthan mine can benefit from my patch.\n\n\n> If you changed your description to sell it this way, it'd be fine\n> as-is.  But you use --force and make multiple mentions of concurrent\n> updates to branches in external processes during the fast-import\n> process.  That kind of description makes it really clear we need to\n> tighten up what happens with the checkpoint command when it hits a\n> failure (as mentioned in the commentary on V3).  Below is a patch that\n> does this.\n>\n> I think we should either:\n>\n>   1) update your commit message to sell it without mentioning the\n>      concurrent rewrites, and then I'll update my patch to not\n>      conflict (we both add new tests at the same location to the same\n>      file causing a simple conflict) by building on yours, OR\n>\n>   2) update your patch to come after mine and add a comment to your\n>      commit message about how checkpoint will abort if it hits an\n>      error, suggesting that people should only update branches\n>      fast-import will not be updating further or they should use\n>      --force and deal with their changes being overwritten by\n>      fast-import.  Then you can submit our two patches as a series.\n>\n> Thoughts?\n\n(2) is probably the most logical order to do things. I have a few\ncomments on your patch, below.\n\n\n\n> -- 8< --\n> Subject: [PATCH] fast-import: remember to check failure flag when\n>  checkpointing\n>\n> fast-import, when finished, will flush out the current packfile and\n> update branches, tags, and marks, returning whether it was successful.\n> The point of the 'checkpoint' command was to do all the same things, but\n> continue processing the stream of input commands if it was successful.\n> Unfortunately, the checkpoint code forgot to check the 'failure' flag to\n> see if there was an error in e.g. updating the branches, which meant it\n> would also continue if there was a failure.  Fix this oversight.\n>\n> Signed-off-by: Elijah Newren <newren@gmail.com>\n> ---\n>  fast-import.c          |  2 ++\n>  t/t9300-fast-import.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n>  2 files changed, 44 insertions(+)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index f38d04fa58..d0e12b03a0 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -3033,6 +3033,8 @@ static void checkpoint(void)\n>         dump_branches();\n>         dump_tags();\n>         dump_marks();\n> +       if (failure)\n> +               exit(1);\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 3668263c40..788a543b82 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -3262,6 +3262,48 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n>         background_import_still_running\n>  '\n>\n> +background_fast_import_race () {\n> +       options=$1\n> +       input_file=$2\n> +       exit_status=$3\n> +       extra_cmd=\"$4\"\n> +\n> +       mkfifo V.input\n> +       exec 8<>V.input\n> +       rm V.input\n> +\n> +       git fast-import $options <&8 &\n> +       echo $! >V.pid &&\n> +       $extra_cmd &&\n> +\n> +       cat $input_file >&8\n> +       wait $(cat V.pid)\n> +       test $? -eq $exit_status\n> +}\n\nI think the test will not fail without the patch (and therefore won't\ncatch a regression): while checkpoint doesn't currently check the\nfailure flag, it will be checked when cmd_main() returns so\nexit_status will be 1.\n\nYou either want to:\n- (a) add more independent commands after the checkpoint and check\nthat they were not run,\n- or (b) run with --done, do not include a done command, and check\nthat fast-import does exit (but it's racy),\n- or (c) you can reuse background_import to have a more explicit\nsequence of events (in which case improvements to background_import\nfrom my patch would have to be committed first).\n\nSomething like: (I did not try the following code)\n\ncat >input <<-INPUT_END &&\ncommit refs/heads/V4\nmark :1\ncommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\ndata 0\nfrom refs/heads/U\n\ncommit refs/heads/V4_child\nmark :1\ncommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\ndata 0\nfrom refs/heads/V4^0\n\ncheckpoint\n\nreset refs/heads/V5\nfrom refs/heads/U\n\nprogress do: shell\nprogress git branch V5 V4\n\ncheckpoint\n\nreset refs/heads/V5\nfrom refs/heads/V4_child\n\nprogress do: checkpoint and stop\n\nINPUT_END\n\nbackground_import \"\" input &&\n\n# V5 remains equal to V4 as set by the external \"git branch\" command:\n#\n# V5 is not updated by the second checkpoint (as resetting V5 to U isn't a\n# fast-forward with respect to V4), and V5 is not updated by the reset to\n# V4_child after the second checkpoint (which would be a fast-forward with\n# respect to V4) as fast-import immediately exits on a failed checkpoint.\ntest \"$(git rev-parse --verify V5)\" = \"$(git rev-parse --verify V4)\" &&\n\n# fast-import is not running anymore (but do kill it if it is):\n! kill -9 $(cat V.pid) &> /dev/null &&\n# fast-import exited with an error:\ntest $(wait $(cat V.pid)) -eq 1\n\n\n\n> +test_expect_success PIPE 'V: checkpoint fails if refs updated beforehand' '\n> +       git checkout --orphan V3 &&\n> +       git commit --allow-empty -m initial &&\n> +       INITIAL=$(git rev-parse HEAD) &&\n> +\n> +       cat >input <<-INPUT_END &&\n> +       feature done\n> +       commit refs/heads/V3\n> +       mark :3\n> +       committer Me My <self@and.eye> 1234567890 +0123\n\nYou likely want to use:\ncommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n\n\n> +       data 10\n> +       generated\n> +       from $INITIAL\n> +\n> +       checkpoint\n> +       done\n> +       INPUT_END\n> +\n> +       background_fast_import_race \"\" input 1 \"git commit --allow-empty -m conflicting\" &&\n> +       background_fast_import_race \"--force\" input 0 \"git commit --allow-empty -m conflicting\"\n> +'\n> +\n> +\n>  ###\n>  ### series W (get-mark and empty orphan commits)\n>  ###\n> --\n> 2.22.0.rc1.dirty\n>\n"},{"id":"376256","messageId":"CABPp-BFkqyGT_YHReKPJKy-nX5eWYhb9AVE2UeD7M+nhKT0dqg@mail.gmail.com","threadId":"51174","inReplyTo":"CAH_=xoZ5Lt2nn50cfDbTA-ZVeshi8HZ91Od4btuUXpBGCanGng@mail.gmail.com","subject":"Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-05-27T23:27:13Z","receivedAt":"2019-05-27T23:27:27Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Eric,\n\nOn Mon, May 27, 2019 at 3:46 PM Eric Rannaud <e@nanocritical.com> wrote:\n> On Mon, May 27, 2019 at 2:12 PM Elijah Newren <newren@gmail.com> wrote:\n> > On Fri, May 24, 2019 at 4:10 PM Eric Rannaud <e@nanocritical.com> wrote:\n>\n> I think the test will not fail without the patch (and therefore won't\n> catch a regression): while checkpoint doesn't currently check the\n> failure flag, it will be checked when cmd_main() returns so\n> exit_status will be 1.\n\nOoh, good catch.\n\n> You either want to:\n> - (a) add more independent commands after the checkpoint and check\n> that they were not run,\n> - or (b) run with --done, do not include a done command, and check\n> that fast-import does exit (but it's racy),\n> - or (c) you can reuse background_import to have a more explicit\n> sequence of events (in which case improvements to background_import\n> from my patch would have to be committed first).\n\nThat sounds good...though it's taking my short patch and just about\namounts to completely rewriting it.  Would you like to take it over\nincluding authorship, and just add either a \"Original-patch-by:\" or\n\"Based-on-patch-by:\" for me in the commit message (these two tags\nappear to be the two most common attribution mechanism used in\ngit.git's history when someone does this)?\n\n> > +       cat >input <<-INPUT_END &&\n> > +       feature done\n> > +       commit refs/heads/V3\n> > +       mark :3\n> > +       committer Me My <self@and.eye> 1234567890 +0123\n>\n> You likely want to use:\n> committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n\nI see other tests in that testsuite using this, and using it here\ncertainly wouldn't hurt; I'm not opposed to it.  But I'm\ncurious...other than \"other tests in the same testcase use it a lot\" I\ndon't see why the choice of committer name/email/date matters at all.\nIs there an actual reason for this that I just missed?\n\nCheers,\nElijah\n"},{"id":"376259","messageId":"CAH_=xoY+3_uEJ0VRVwP6vqLKdWzzU8hjLosf6sjjJtqP4mW=ng@mail.gmail.com","threadId":"51174","inReplyTo":"CABPp-BFkqyGT_YHReKPJKy-nX5eWYhb9AVE2UeD7M+nhKT0dqg@mail.gmail.com","subject":"Re: [PATCH v4] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2019-05-28T00:11:45Z","receivedAt":"2019-05-28T00:11:59Z","isPatch":true,"sender":{"key":"e@nanocritical.com","avatar":"https://avatars.githubusercontent.com/u/925816?v=4"},"body":"On Mon, May 27, 2019 at 4:27 PM Elijah Newren <newren@gmail.com> wrote:\n> That sounds good...though it's taking my short patch and just about\n> amounts to completely rewriting it.  Would you like to take it over\n> including authorship, and just add either a \"Original-patch-by:\" or\n> \"Based-on-patch-by:\" for me in the commit message (these two tags\n> appear to be the two most common attribution mechanism used in\n> git.git's history when someone does this)?\n\nSure. Co-authored-by is an option too. I'll turn all of this in 3\npatches: improve background_import, your patch, the rest of mine.\n\n\n> > > +       cat >input <<-INPUT_END &&\n> > > +       feature done\n> > > +       commit refs/heads/V3\n> > > +       mark :3\n> > > +       committer Me My <self@and.eye> 1234567890 +0123\n> >\n> > You likely want to use:\n> > committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n>\n> I see other tests in that testsuite using this, and using it here\n> certainly wouldn't hurt; I'm not opposed to it.  But I'm\n> curious...other than \"other tests in the same testcase use it a lot\" I\n> don't see why the choice of committer name/email/date matters at all.\n> Is there an actual reason for this that I just missed?\n\nThere is no direct benefit here except consistency.\n\nThese vars are set the same on every test run so that the SHA1 of\ngenerated patches is the same as in a previous run (see also\ntest_tick() to increment the date by one minute). So in general this\nmakes it easier to debug tests as hashes can be compared between runs.\nBut your hand-picked values do give you that property.\n\nHowever, as normal git commands use the vars automatically, by using\nthem here, fast-import commands and direct git commands produce the\nsame hashes if they're doing the same work. That's the main benefit of\nusing them here: while the test does not currently rely on that\nproperty, maybe it will (or someone debugging down the road will\nexpect that).\n"}]}