{"thread":{"id":"44733","subject":"[PATCH] fast-import: properly fanout notes when tree is imported","startedAt":"2016-12-19T02:45:48Z","lastAt":"2016-12-20T21:51:08Z","messageCount":8,"participants":["Mike Hommey","Johan Herland","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"308005","messageId":"20161219021212.15978-1-mh@glandium.org","threadId":"44733","inReplyTo":null,"subject":"[PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-12-19T02:12:12Z","receivedAt":"2016-12-19T02:45:48Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"In typical uses of fast-import, trees are inherited from a parent\ncommit. In that case, the tree_entry for the branch looks like:\n\n  .versions[1].sha1 = $some_sha1\n  .tree = <tree structure loaded from $some_sha1>\n\nHowever, when trees are imported, rather than inherited, that is not the\ncase. One can import a tree with a filemodify command, replacing the\nroot tree object.\n\ne.g.\n  \"M 040000 $some_sha1 \\n\"\n\nIn this case, the tree_entry for the branch looks like:\n\n  .versions[1].sha1 = $some_sha1\n  .tree = NULL\n\nWhen adding new notes with the notemodify command, do_change_note_fanout\nis called to get a notes count, and to do so, it loops over the\ntree_entry->tree, but doesn't do anything when the tree is NULL.\n\nIn the latter case above, it means do_change_note_fanout thinks the tree\ncontains no note, and new notes are added with no fanout.\n\nInterestingly, do_change_note_fanout does check whether subdirectories\nhave a NULL .tree, in which case it uses load_tree(). Which means the\nright behaviour happens when using the filemodify command to import\nsubdirectories.\n\nThis change makes do_change_note_fanount call load_tree() whenever the\ntree_entry it is given has no tree loaded, making all cases handled\nequally.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n\nThis is something I should have submitted a patch for a long time ago, back\nwhen this was discussed in the thread starting from\nhttps://www.spinics.net/lists/git/msg242426.html. The message most relevant to\nthis patch in the thread is https://www.spinics.net/lists/git/msg242448.html.\n\n fast-import.c                |  8 +++++---\n t/t9301-fast-import-notes.sh | 41 +++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 46 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex cb545d7df5..5e528b1999 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(\n \t\tchar *fullpath, unsigned int fullpath_len,\n \t\tunsigned char fanout)\n {\n-\tstruct tree_content *t = root->tree;\n+\tstruct tree_content *t;\n \tstruct tree_entry *e, leaf;\n \tunsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;\n \tuintmax_t num_notes = 0;\n \tunsigned char sha1[20];\n \tchar realpath[60];\n \n+\tif (!root->tree)\n+\t\tload_tree(root);\n+\tt = root->tree;\n+\n \tfor (i = 0; t && i < t->entry_count; i++) {\n \t\te = t->entries[i];\n \t\ttmp_hex_sha1_len = hex_sha1_len + e->name->str_len;\n@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(\n \t\t\t\tleaf.tree);\n \t\t} else if (S_ISDIR(e->versions[1].mode)) {\n \t\t\t/* This is a subdir that may contain note entries */\n-\t\t\tif (!e->tree)\n-\t\t\t\tload_tree(e);\n \t\t\tnum_notes += do_change_note_fanout(orig_root, e,\n \t\t\t\thex_sha1, tmp_hex_sha1_len,\n \t\t\t\tfullpath, tmp_fullpath_len, fanout);\ndiff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh\nindex 83acf68bc3..b408b2b32d 100755\n--- a/t/t9301-fast-import-notes.sh\n+++ b/t/t9301-fast-import-notes.sh\n@@ -483,6 +483,47 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '\n \n '\n \n+# Create another notes tree from the one above\n+cat >>input <<INPUT_END\n+commit refs/heads/other_commits\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+commit #$(($num_commit + 1))\n+COMMIT\n+\n+from refs/heads/many_commits\n+M 644 inline file\n+data <<EOF\n+file contents in commit #$(($num_commit + 1))\n+EOF\n+\n+commit refs/notes/other_notes\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+committing one more note on a tree imported from a previous notes tree\n+COMMIT\n+\n+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) \n+N inline :$(($num_commit + 1))\n+data <<EOF\n+note for commit #$(($num_commit + 1))\n+EOF\n+INPUT_END\n+\n+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '\n+\tgit fast-import <input &&\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/other_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 cat >>expect_non-note1 << EOF\n This is not a note, but rather a regular file residing in a notes tree\n EOF\n-- \n2.11.0.dirty\n\n"},{"id":"308106","messageId":"CALKQrgepfK3Unk-pq4ZpU2EmoPg6u8KYPkgEAKU1F1D1N9WtqA@mail.gmail.com","threadId":"44733","inReplyTo":"20161219021212.15978-1-mh@glandium.org","subject":"Re: [PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2016-12-19T22:05:17Z","receivedAt":"2016-12-19T22:30:23Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Mon, Dec 19, 2016 at 3:12 AM, Mike Hommey <mh@glandium.org> wrote:\n> In typical uses of fast-import, trees are inherited from a parent\n> commit. In that case, the tree_entry for the branch looks like:\n>\n>   .versions[1].sha1 = $some_sha1\n>   .tree = <tree structure loaded from $some_sha1>\n>\n> However, when trees are imported, rather than inherited, that is not the\n> case. One can import a tree with a filemodify command, replacing the\n> root tree object.\n>\n> e.g.\n>   \"M 040000 $some_sha1 \\n\"\n>\n> In this case, the tree_entry for the branch looks like:\n>\n>   .versions[1].sha1 = $some_sha1\n>   .tree = NULL\n>\n> When adding new notes with the notemodify command, do_change_note_fanout\n> is called to get a notes count, and to do so, it loops over the\n> tree_entry->tree, but doesn't do anything when the tree is NULL.\n>\n> In the latter case above, it means do_change_note_fanout thinks the tree\n> contains no note, and new notes are added with no fanout.\n\ns/note,/notes,/\n\n>\n> Interestingly, do_change_note_fanout does check whether subdirectories\n> have a NULL .tree, in which case it uses load_tree(). Which means the\n> right behaviour happens when using the filemodify command to import\n> subdirectories.\n>\n> This change makes do_change_note_fanount call load_tree() whenever the\n> tree_entry it is given has no tree loaded, making all cases handled\n> equally.\n>\n> Signed-off-by: Mike Hommey <mh@glandium.org>\n\nAcked-by: Johan Herland <johan@herland.net>\n"},{"id":"308165","messageId":"xmqqk2aujsyb.fsf@gitster.mtv.corp.google.com","threadId":"44733","inReplyTo":"20161219021212.15978-1-mh@glandium.org","subject":"Re: [PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-20T19:34:04Z","receivedAt":"2016-12-20T19:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> In typical uses of fast-import, trees are inherited from a parent\n> commit. In that case, the tree_entry for the branch looks like:\n> ...\n> +# Create another notes tree from the one above\n> +cat >>input <<INPUT_END\n> +...\n> +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) \n\nThere is a trailing SP that cannot be seen by anybody.\n\nDon't do this.  It makes it very easy to miss what is going on and\nwastes reviewers' time.\n\nProtect it by doing something like:\n\n\tsed -e 's/Z$//' >>input <<INPUT_END\n\t...\n\tM 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z\n\nThanks.\n"},{"id":"308168","messageId":"20161220204841.awvabgwsxudxfzca@glandium.org","threadId":"44733","inReplyTo":"xmqqk2aujsyb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-12-20T20:48:41Z","receivedAt":"2016-12-20T20:48:53Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:\n> Mike Hommey <mh@glandium.org> writes:\n> \n> > In typical uses of fast-import, trees are inherited from a parent\n> > commit. In that case, the tree_entry for the branch looks like:\n> > ...\n> > +# Create another notes tree from the one above\n> > +cat >>input <<INPUT_END\n> > +...\n> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) \n> \n> There is a trailing SP that cannot be seen by anybody.\n> \n> Don't do this.  It makes it very easy to miss what is going on and\n> wastes reviewers' time.\n> \n> Protect it by doing something like:\n> \n> \tsed -e 's/Z$//' >>input <<INPUT_END\n> \t...\n> \tM 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z\n\nHow about\nEMPTY=\n...\nM 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY\n\n?\n\nMike\n"},{"id":"308170","messageId":"xmqqbmw6jp59.fsf@gitster.mtv.corp.google.com","threadId":"44733","inReplyTo":"20161220204841.awvabgwsxudxfzca@glandium.org","subject":"Re: [PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-20T20:56:18Z","receivedAt":"2016-12-20T20:56:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> On Tue, Dec 20, 2016 at 11:34:04AM -0800, Junio C Hamano wrote:\n>> Mike Hommey <mh@glandium.org> writes:\n>> \n>> > In typical uses of fast-import, trees are inherited from a parent\n>> > commit. In that case, the tree_entry for the branch looks like:\n>> > ...\n>> > +# Create another notes tree from the one above\n>> > +cat >>input <<INPUT_END\n>> > +...\n>> > +M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) \n>> \n>> There is a trailing SP that cannot be seen by anybody.\n>> \n>> Don't do this.  It makes it very easy to miss what is going on and\n>> wastes reviewers' time.\n>> \n>> Protect it by doing something like:\n>> \n>> \tsed -e 's/Z$//' >>input <<INPUT_END\n>> \t...\n>> \tM 040000 $(git log --no-walk --format=%T refs/notes/many_notes) Z\n>\n> How about\n> EMPTY=\n> ...\n> M 040000 $(git log --no-walk --format=%T refs/notes/many_notes) $EMPTY\n>\n> ?\n\nNotice I said \"something like\" ;-)\n\nI think you are bringing that up to avoid sed, but if you want to go\nthat route, the long string $EMPTY is distracting, and makes readers\nwonder why something that is loud but expands to nothing has to be\nthere.  It hides the true intention, which is that the SP that comes\nbefore it is the most important thing on that line.\n\nI would think a lot more understandable variant would be to do this\ninstead:\n\n\tSP=\" \"\n\t...\n\tM a lot of garbage $(and command)$SP\n\n"},{"id":"308171","messageId":"20161220210448.32213-1-mh@glandium.org","threadId":"44733","inReplyTo":"xmqqbmw6jp59.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-12-20T21:04:48Z","receivedAt":"2016-12-20T21:05:01Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"In typical uses of fast-import, trees are inherited from a parent\ncommit. In that case, the tree_entry for the branch looks like:\n\n  .versions[1].sha1 = $some_sha1\n  .tree = <tree structure loaded from $some_sha1>\n\nHowever, when trees are imported, rather than inherited, that is not the\ncase. One can import a tree with a filemodify command, replacing the\nroot tree object.\n\ne.g.\n  \"M 040000 $some_sha1 \\n\"\n\nIn this case, the tree_entry for the branch looks like:\n\n  .versions[1].sha1 = $some_sha1\n  .tree = NULL\n\nWhen adding new notes with the notemodify command, do_change_note_fanout\nis called to get a notes count, and to do so, it loops over the\ntree_entry->tree, but doesn't do anything when the tree is NULL.\n\nIn the latter case above, it means do_change_note_fanout thinks the tree\ncontains no notes, and new notes are added with no fanout.\n\nInterestingly, do_change_note_fanout does check whether subdirectories\nhave a NULL .tree, in which case it uses load_tree(). Which means the\nright behaviour happens when using the filemodify command to import\nsubdirectories.\n\nThis change makes do_change_note_fanount call load_tree() whenever the\ntree_entry it is given has no tree loaded, making all cases handled\nequally.\n\nSigned-off-by: Mike Hommey <mh@glandium.org>\n---\n fast-import.c                |  8 +++++---\n t/t9301-fast-import-notes.sh | 42 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex cb545d7df5..5e528b1999 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2220,13 +2220,17 @@ static uintmax_t do_change_note_fanout(\n \t\tchar *fullpath, unsigned int fullpath_len,\n \t\tunsigned char fanout)\n {\n-\tstruct tree_content *t = root->tree;\n+\tstruct tree_content *t;\n \tstruct tree_entry *e, leaf;\n \tunsigned int i, tmp_hex_sha1_len, tmp_fullpath_len;\n \tuintmax_t num_notes = 0;\n \tunsigned char sha1[20];\n \tchar realpath[60];\n \n+\tif (!root->tree)\n+\t\tload_tree(root);\n+\tt = root->tree;\n+\n \tfor (i = 0; t && i < t->entry_count; i++) {\n \t\te = t->entries[i];\n \t\ttmp_hex_sha1_len = hex_sha1_len + e->name->str_len;\n@@ -2278,8 +2282,6 @@ static uintmax_t do_change_note_fanout(\n \t\t\t\tleaf.tree);\n \t\t} else if (S_ISDIR(e->versions[1].mode)) {\n \t\t\t/* This is a subdir that may contain note entries */\n-\t\t\tif (!e->tree)\n-\t\t\t\tload_tree(e);\n \t\t\tnum_notes += do_change_note_fanout(orig_root, e,\n \t\t\t\thex_sha1, tmp_hex_sha1_len,\n \t\t\t\tfullpath, tmp_fullpath_len, fanout);\ndiff --git a/t/t9301-fast-import-notes.sh b/t/t9301-fast-import-notes.sh\nindex 83acf68bc3..dadc70b7d5 100755\n--- a/t/t9301-fast-import-notes.sh\n+++ b/t/t9301-fast-import-notes.sh\n@@ -483,6 +483,48 @@ test_expect_success 'verify that lots of notes trigger a fanout scheme' '\n \n '\n \n+# Create another notes tree from the one above\n+SP=\" \"\n+cat >>input <<INPUT_END\n+commit refs/heads/other_commits\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+commit #$(($num_commit + 1))\n+COMMIT\n+\n+from refs/heads/many_commits\n+M 644 inline file\n+data <<EOF\n+file contents in commit #$(($num_commit + 1))\n+EOF\n+\n+commit refs/notes/other_notes\n+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+data <<COMMIT\n+committing one more note on a tree imported from a previous notes tree\n+COMMIT\n+\n+M 040000 $(git log --no-walk --format=%T refs/notes/many_notes)$SP\n+N inline :$(($num_commit + 1))\n+data <<EOF\n+note for commit #$(($num_commit + 1))\n+EOF\n+INPUT_END\n+\n+test_expect_success 'verify that importing a notes tree respects the fanout scheme' '\n+\tgit fast-import <input &&\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/other_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 cat >>expect_non-note1 << EOF\n This is not a note, but rather a regular file residing in a notes tree\n EOF\n-- \n2.11.0\n\n"},{"id":"308172","messageId":"20161220210621.5gubonuscn6t2de7@glandium.org","threadId":"44733","inReplyTo":"20161220210448.32213-1-mh@glandium.org","subject":"Re: [PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2016-12-20T21:06:21Z","receivedAt":"2016-12-20T21:06:31Z","isPatch":true,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Sorry, I forgot the v2 in the subject.\n\nMike\n"},{"id":"308174","messageId":"xmqq7f6ujmmv.fsf@gitster.mtv.corp.google.com","threadId":"44733","inReplyTo":"20161220210448.32213-1-mh@glandium.org","subject":"Re: [PATCH] fast-import: properly fanout notes when tree is imported","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-20T21:50:32Z","receivedAt":"2016-12-20T21:51:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey <mh@glandium.org> writes:\n\n> In typical uses of fast-import, trees are inherited from a parent\n> commit. In that case, the tree_entry for the branch looks like:\n> ...\n> This change makes do_change_note_fanount call load_tree() whenever the\n> tree_entry it is given has no tree loaded, making all cases handled\n> equally.\n\nThanks.\n"}]}