{"thread":{"id":"49617","subject":"[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them","startedAt":"2018-10-19T16:31:47Z","lastAt":"2018-10-19T23:25:16Z","messageCount":2,"participants":["Eric Rannaud"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"360944","messageId":"2cc54cbb496404909b8600122b2c7e28164476fa.1539965934.git.e@nanocritical.com","threadId":"49617","inReplyTo":null,"subject":"[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2018-10-19T16:31:40Z","receivedAt":"2018-10-19T16:31:47Z","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 | 53 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 67 insertions(+)\n\n\nPlease let me know what you think of the change itself, before I spend\nsome quality time bashing out a way to avoid using sleep(1) in the\ntests.\n\nThanks.\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 95600c78e048..d25be5c83110 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -250,6 +250,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@@ -257,6 +258,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@@ -728,6 +730,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@@ -1715,6 +1718,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@@ -1780,6 +1787,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@@ -2813,6 +2824,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@@ -2894,6 +2906,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@@ -2914,6 +2927,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 40fe7e49767a..548994dfbeb3 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3123,6 +3123,9 @@ test_expect_success 'U: validate root delete result' '\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+# If input_file2 is specified, sleep 2 seconds before writing it to fast-import\n+# stdin.\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@@ -3131,6 +3134,7 @@ test_expect_success 'U: validate root delete result' '\n background_import_then_checkpoint () {\n \toptions=$1\n \tinput_file=$2\n+\tinput_file2=$3\n \n \tmkfifo V.input\n \texec 8<>V.input\n@@ -3155,6 +3159,7 @@ background_import_then_checkpoint () {\n \t# because there is no reader on &9 yet.\n \t(\n \t\tcat \"$input_file\"\n+\t\t[ -n \"$input_file2\" ] && sleep 2 && cat \"$input_file2\"\n \t\techo \"checkpoint\"\n \t\techo \"progress checkpoint\"\n \t) >&8 &\n@@ -3262,4 +3267,52 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\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+\tINPUT_END\n+\n+\tcat >input2 <<-INPUT_END &&\n+\treset refs/heads/V4\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_import_then_checkpoint \"\" input input2 &&\n+\tsleep 1 &&\n+\tgit branch -f V3 refs/heads/V &&\n+\tsleep 2 &&\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+\tINPUT_END\n+\n+\tcat >input2 <<-INPUT_END &&\n+\ttag Vtag3\n+\tfrom refs/heads/U\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 input2 &&\n+\tsleep 1 &&\n+\tgit tag -f Vtag2 refs/heads/V &&\n+\tsleep 2 &&\n+\ttest \"$(git rev-parse --verify Vtag2)\" = \"$(git rev-parse --verify V)\" &&\n+\tbackground_import_still_running\n+'\n+\n test_done\n-- \n2.19.1\n\n"},{"id":"360989","messageId":"b3b6b36debe9190e31c70e8fe6d623fd1ba1266b.1539991203.git.e@nanocritical.com","threadId":"49617","inReplyTo":"2cc54cbb496404909b8600122b2c7e28164476fa.1539965934.git.e@nanocritical.com","subject":"[PATCH 1/1] fast-import: checkpoint: only write out refs and tags if we changed them","fromName":"Eric Rannaud","fromEmail":"e@nanocritical.com","sentAt":"2018-10-19T23:25:08Z","receivedAt":"2018-10-19T23:25:16Z","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 | 55 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 69 insertions(+)\n\n\nChanged since v1:\n\nThe testcases failed to enforce the new behavior.\n\n- t9300: Fixed tests to properly interleave operations between the\n  fast-import in the background and the concurrent \"git branch\" and \"git\n  tag\" command.\n\n  I misremembered how background_import_then_checkpoint() works: it\n  return only after the whole set of commands provided in input_file\n  (and now input_file2) are run by fast-import.\n\n  In v2, the competing git commands are started first in the background\n  and are run after a delay of 1 second. input_file is sent right away\n  to fast-import, followed 2 seconds later by input_file2. That is:\n\n    second 0: input_file\n    second 1: git branch or git tag command\n    second 2: input_file2\n\n- t9300: Use --force for the branch testcase: without it, earlier\n  versions of Git don't fail this testcase (as resetting V3 to U is not\n  a FF, so the second checkpoint skips updating it in earlier versions\n  of Git, failing to demonstrate the problematic behavior).\n\nChecked that the new testcases do fail with earlier versions of Git.\n\nTODO: The testcases still rely on sleep(1).\n\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 95600c78e048..d25be5c83110 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -250,6 +250,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@@ -257,6 +258,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@@ -728,6 +730,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@@ -1715,6 +1718,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@@ -1780,6 +1787,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@@ -2813,6 +2824,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@@ -2894,6 +2906,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@@ -2914,6 +2927,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 40fe7e49767a..92e8c3d54112 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3123,6 +3123,9 @@ test_expect_success 'U: validate root delete result' '\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+# If input_file2 is specified, sleep 2 seconds before writing it to fast-import\n+# stdin.\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@@ -3131,6 +3134,7 @@ test_expect_success 'U: validate root delete result' '\n background_import_then_checkpoint () {\n \toptions=$1\n \tinput_file=$2\n+\tinput_file2=$3\n \n \tmkfifo V.input\n \texec 8<>V.input\n@@ -3155,6 +3159,7 @@ background_import_then_checkpoint () {\n \t# because there is no reader on &9 yet.\n \t(\n \t\tcat \"$input_file\"\n+\t\t[ -n \"$input_file2\" ] && sleep 2 && cat \"$input_file2\"\n \t\techo \"checkpoint\"\n \t\techo \"progress checkpoint\"\n \t) >&8 &\n@@ -3262,4 +3267,54 @@ test_expect_success PIPE 'V: checkpoint updates tags after tag' '\n \tbackground_import_still_running\n '\n \n+background_delayed () {\n+\t(sleep 1 && exec \"$@\") &\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+\tINPUT_END\n+\n+\tcat >input2 <<-INPUT_END &&\n+\treset refs/heads/V4\n+\tfrom refs/heads/U\n+\n+\tINPUT_END\n+\n+\tbackground_delayed git branch -f V3 V &&\n+\tbackground_import_then_checkpoint \"--force\" input input2 &&\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+\tINPUT_END\n+\n+\tcat >input2 <<-INPUT_END &&\n+\ttag Vtag3\n+\tfrom refs/heads/U\n+\ttagger $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata 0\n+\n+\tINPUT_END\n+\n+\tbackground_delayed git tag -f Vtag2 V &&\n+\tbackground_import_then_checkpoint \"\" input input2 &&\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 test_done\n-- \n2.19.1\n\n"}]}