{"thread":{"id":"29008","subject":"Incremental use of fast-import may cause conflicting notes","startedAt":"2011-11-23T12:09:34Z","lastAt":"2011-11-25T00:09:47Z","messageCount":7,"participants":["Henrik Grubbström","Jonathan Nieder","Johan Herland"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"179882","messageId":"Pine.GSO.4.63.1111231137350.5099@shipon.roxen.com","threadId":"29008","inReplyTo":null,"subject":"Incremental use of fast-import may cause conflicting notes","fromName":"Henrik Grubbström","fromEmail":"grubba@grubba.org","sentAt":"2011-11-23T12:09:34Z","receivedAt":"2011-11-23T12:09:34Z","isPatch":false,"sender":{"key":"grubba@grubba.org","avatar":"https://avatars.githubusercontent.com/u/1169458?v=4"},"body":"Hi.\n\nBackground: I have an incremental repository-walker creating a \ncorresponding documentation repository from a source repository\nthat uses git-notes to store its state, a use for which notes\nseem very suitable.\n\nProblem: When the number of notes in the root of the notes branch\nincreases beyond a threshold, fast-import changes the fanout. This \nis as designed, but the problem is that when fast-import is restarted\nit won't remember the fanout, and will start writing files in the root \nagain. This means that there may be multiple notes-files for the same \ncommit, eg both de/adbeef and deadbeef.\n\nThis is not what the user expects, and is not good practice, even if it in \nthis case actually works, since the latter is defined to have priority.\nI'm however not sure if eg fast_import.c:do_change_note_fanout() will do \nthe right thing if/when the fanout is changed again.\n\nThe problem is probably due to b->num_notes not being initialized properly \nwhen the old non-empty root commit for the notes branch is loaded in \nparse_from()/parse_new_commit().\n\nMy workaround for now is to use filedeleteall and restore all the notes\nby hand in the first new commit on the notes branch.\n\nVersion of git: 1.7.6.4 (gentoo)\n\nThanks,\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@grubba.org\nRoxen Internet Software AB\t\t\t\tgrubba@roxen.com"},{"id":"179883","messageId":"Pine.GSO.4.63.1111231310090.5099@shipon.roxen.com","threadId":"29008","inReplyTo":"Pine.GSO.4.63.1111231137350.5099@shipon.roxen.com","subject":"Re: Incremental use of fast-import may cause conflicting notes","fromName":"Henrik Grubbström","fromEmail":"grubba@roxen.com","sentAt":"2011-11-23T12:10:57Z","receivedAt":"2011-11-23T12:10:57Z","isPatch":false,"sender":{"key":"grubba@roxen.com","avatar":null},"body":"On Wed, 23 Nov 2011, Henrik Grubbström wrote:\n\n> Hi.\n>\n> Background: I have an incremental repository-walker creating a corresponding \n> documentation repository from a source repository\n> that uses git-notes to store its state, a use for which notes\n> seem very suitable.\n>\n> Problem: When the number of notes in the root of the notes branch\n> increases beyond a threshold, fast-import changes the fanout. This is as \n> designed, but the problem is that when fast-import is restarted\n> it won't remember the fanout, and will start writing files in the root again. \n> This means that there may be multiple notes-files for the same commit, eg \n> both de/adbeef and deadbeef.\n>\n> This is not what the user expects, and is not good practice, even if it in \n> this case actually works, since the latter is defined to have priority.\n> I'm however not sure if eg fast_import.c:do_change_note_fanout() will do the \n> right thing if/when the fanout is changed again.\n>\n> The problem is probably due to b->num_notes not being initialized properly \n> when the old non-empty root commit for the notes branch is loaded in \n> parse_from()/parse_new_commit().\n>\n> My workaround for now is to use filedeleteall and restore all the notes\n> by hand in the first new commit on the notes branch.\n>\n> Version of git: 1.7.6.4 (gentoo)\n\nOops, wrong machine... 1.7.8.rc3 (gentoo)\n\n> Thanks,\n\n--\nHenrik Grubbström\t\t\t\t\tgrubba@roxen.com\nRoxen Internet Software AB"},{"id":"179955","messageId":"20111124230917.GC27586@elie.hsd1.il.comcast.net","threadId":"29008","inReplyTo":"Pine.GSO.4.63.1111231137350.5099@shipon.roxen.com","subject":"Re: Incremental use of fast-import may cause conflicting notes","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-24T23:09:17Z","receivedAt":"2011-11-24T23:09:17Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Henrik,\n\nHenrik Grubbström wrote:\n\n> Background: I have an incremental repository-walker creating a corresponding\n> documentation repository from a source repository\n> that uses git-notes to store its state, a use for which notes\n> seem very suitable.\n\nNice.\n\n[...]\n> when fast-import is restarted\n> it won't remember the fanout, and will start writing files in the root\n> again. This means that there may be multiple notes-files for the same\n> commit, eg both de/adbeef and deadbeef.\n[...]\n> The problem is probably due to b->num_notes not being initialized properly\n> when the old non-empty root commit for the notes branch is loaded in\n> parse_from()/parse_new_commit().\n\nSounds like a bug. Can you suggest a reproduction recipe (ideally as\na patch to t/t9301-fast-import-notes.sh), a fix, or both?\n\nThanks.\n\nRegards,\nJonathan\n"},{"id":"179958","messageId":"1322179787-4422-1-git-send-email-johan@herland.net","threadId":"29008","inReplyTo":"Pine.GSO.4.63.1111231137350.5099@shipon.roxen.com","subject":"[RFC/PATCH 0/3] fast-import: Fix incremental use of notes","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-11-25T00:09:44Z","receivedAt":"2011-11-25T00:09:44Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"Hi,\n\nThis is a first attempt at fixing the bug reported by Henrik.\n\nThe first two patches provide testcases for the bug, and the last patch\nprovides a fix.\n\nHave fun! :)\n\n...Johan\n\n\nJohan Herland (3):\n t9301: Fix testcase covering up a bug in fast-import's notes fanout handling\n t9301: Add 2nd testcase exposing bugs in fast-import's notes fanout handling\n fast-import: Fix incorrect fanout level when modifying existing notes refs\n\n fast-import.c | 28 ++++++++++++++++--\n t/t9301-fast-import-notes.sh | 63 ++++++++++++++++++++++++++++++++++++++---\n 2 files changed, 83 insertions(+), 8 deletions(-)\n\n-- \n1.7.5.rc1.3.g4d7b\n"},{"id":"179956","messageId":"1322179787-4422-2-git-send-email-johan@herland.net","threadId":"29008","inReplyTo":"1322179787-4422-1-git-send-email-johan@herland.net","subject":"[RFC/PATCH 1/3] t9301: Fix testcase covering up a bug in fast-import's notes fanout handling","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-11-25T00:09:45Z","receivedAt":"2011-11-25T00:09:45Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"There is a bug in fast-import where the fanout levels of an existing notes\ntree being loaded into the fast-import machinery is disregarded. Instead, any\ntree loaded is assumed to have a fanout level of 0. If the true fanout level\nis deeper, any attempt to remove a note from that tree will silently fail\n(as the note will not be found at fanout level 0).\n\nHowever, this bug was covered up by the way in which the t9301 testcase was\nwritten: When generating the fast-import commands to test mass removal of\nnotes, we appended these commands to an already existing 'input' file which\nhappened to already contain the fast-import commands used in the previous\nsubtest to generate the very same notes tree. This would normally be harmless\n(but suboptimal) as the notes created were identical to the notes already\npresent in the notes tree. But the act of repeating all the notes additions\ncaused the internal fast-import data structures to recalculate the fanout,\ninstead of hanging on to the initial (incorrect) fanout (that causes the bug\ndescribed above). Thus, the subsequent removal of notes in the same 'input'\nfile would succeed, thereby covering up the bug described above.\n\nThis patch creates a new 'input' file instead of appending to the file from\nthe previous subtest. Thus, we end up properly testing removal of notes that\nwere added by a previous fast-import command. As a side effect, the notes\nremoval can no longer refer to commits using the marks set by the previous\nfast-import run, instead the commits names must be referenced directly.\n\nThe underlying fast-import bug is still present after this patch, but now we\nhave at least uncovered it. Therefore, the affected subtests are labeled as\nexpected failures until the underlying bug is fixed.\n\nSigned-off-by: Johan Herland \n---\n t/t9301-fast-import-notes.sh | 13 ++++++-------\n 1 files changed, 6 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh\nindex 463254c..fd08161 100755\n--- a/t/t9301-fast-import-notes.sh\n+++ b/t/t9301-fast-import-notes.sh\n@@ -507,7 +507,7 @@ test_expect_success 'verify that non-notes are untouched by a fanout change' '\n '\n remaining_notes=10\n test_tick\n-cat >>input <input < $GIT_COMMITTER_DATE\n data <>input <\nSigned-off-by: Johan Herland \n---\n t/t9301-fast-import-notes.sh | 54 ++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 54 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh\nindex fd08161..57d85a6 100755\n--- a/t/t9301-fast-import-notes.sh\n+++ b/t/t9301-fast-import-notes.sh\n@@ -505,6 +505,60 @@ test_expect_success 'verify that non-notes are untouched by a fanout change' '\n \ttest_cmp expect_non-note3 actual\n \n '\n+\n+# Change the notes for the three top commits\n+test_tick\n+cat >input < $GIT_COMMITTER_DATE\n+data <>input <>expect < actual &&\n+\ttest_cmp expect actual\n+\n+'\n+\n+test_expect_failure 'verify that changing notes respect existing fanout' '\n+\n+\t# None of the entries in the top-level notes tree should be a full SHA1\n+\tgit ls-tree --name-only refs/notes/many_notes |\n+\twhile read path\n+\tdo\n+\t\tif test $(expr length \"$path\") -ge 40\n+\t\tthen\n+\t\t\treturn 1\n+\t\tfi\n+\tdone\n+\n+'\n+\n remaining_notes=10\n test_tick\n cat >input < 0.\nManipulating the notes tree at an incorrect fanout level causes removals to\nsilently fail, and modifications of existing notes to instead produce an\nadditional note (leaving the old object in place at a different fanout level).\n\nThis patch fixes the bug by explicitly counting the number of notes in the\nnotes tree whenever it looks like the num_notes counter could be wrong (when\nnum_notes == 0). There may be false positives (i.e. triggering the counting\nwhen the notes tree is truly empty), but in those cases, the counting should\nnot take long.\n\nSigned-off-by: Johan Herland \n---\n fast-import.c | 28 +++++++++++++++++++++++++---\n t/t9301-fast-import-notes.sh | 8 ++++----\n 2 files changed, 29 insertions(+), 7 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 8d8ea3c..f4bfe0f 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2173,6 +2173,11 @@ static uintmax_t do_change_note_fanout(\n \n \t\tif (tmp_hex_sha1_len == 40 && !get_sha1_hex(hex_sha1, sha1)) {\n \t\t\t/* This is a note entry */\n+\t\t\tif (fanout == 0xff) {\n+\t\t\t\t/* Counting mode, no rename */\n+\t\t\t\tnum_notes++;\n+\t\t\t\tcontinue;\n+\t\t\t}\n \t\t\tconstruct_path_with_fanout(hex_sha1, fanout, realpath);\n \t\t\tif (!strcmp(fullpath, realpath)) {\n \t\t\t\t/* Note entry is in correct location */\n@@ -2379,7 +2384,7 @@ static void file_change_cr(struct branch *b, int rename)\n \t\tleaf.tree);\n }\n \n-static void note_change_n(struct branch *b, unsigned char old_fanout)\n+static void note_change_n(struct branch *b, unsigned char *old_fanout)\n {\n \tconst char *p = command_buf.buf + 2;\n \tstatic struct strbuf uq = STRBUF_INIT;\n@@ -2390,6 +2395,23 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)\n \tuint16_t inline_data = 0;\n \tunsigned char new_fanout;\n \n+\t/*\n+\t * When loading a branch, we don't traverse its tree to count the real\n+\t * number of notes (too expensive to do this for all non-note refs).\n+\t * This means that recently loaded notes refs might incorrectly have\n+\t * b->num_notes == 0, and consequently, old_fanout might be wrong.\n+\t *\n+\t * Fix this by traversing the tree and counting the number of notes\n+\t * when b->num_notes == 0. If the notes tree is truly empty, the\n+\t * calculation should not take long.\n+\t */\n+\tif (b->num_notes == 0 && *old_fanout == 0) {\n+\t\t/* Invoke change_note_fanout() in \"counting mode\". */\n+\t\tb->num_notes = change_note_fanout(&b->branch_tree, 0xff);\n+\t\t*old_fanout = convert_num_notes_to_fanout(b->num_notes);\n+\t}\n+\n+\t/* Now parse the notemodify command. */\n \t/* or 'inline' */\n \tif (*p == ':') {\n \t\tchar *x;\n@@ -2450,7 +2472,7 @@ static void note_change_n(struct branch *b, unsigned char old_fanout)\n \t\t\t typename(type), command_buf.buf);\n \t}\n \n-\tconstruct_path_with_fanout(sha1_to_hex(commit_sha1), old_fanout, path);\n+\tconstruct_path_with_fanout(sha1_to_hex(commit_sha1), *old_fanout, path);\n \tif (tree_content_remove(&b->branch_tree, path, NULL))\n \t\tb->num_notes--;\n \n@@ -2637,7 +2659,7 @@ static void parse_new_commit(void)\n \t\telse if (!prefixcmp(command_buf.buf, \"C \"))\n \t\t\tfile_change_cr(b, 0);\n \t\telse if (!prefixcmp(command_buf.buf, \"N \"))\n-\t\t\tnote_change_n(b, prev_fanout);\n+\t\t\tnote_change_n(b, &prev_fanout);\n \t\telse if (!strcmp(\"deleteall\", command_buf.buf))\n \t\t\tfile_change_deleteall(b);\n \t\telse if (!prefixcmp(command_buf.buf, \"ls \"))\ndiff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh\nindex 57d85a6..83acf68 100755\n--- a/t/t9301-fast-import-notes.sh\n+++ b/t/t9301-fast-import-notes.sh\n@@ -536,7 +536,7 @@ EXPECT_END\n \tj=$(($j + 1))\n done\n \n-test_expect_failure 'change a few existing notes' '\n+test_expect_success 'change a few existing notes' '\n \n \tgit fast-import