{"thread":{"id":"37437","subject":"[PATCH 0/2] fast-import: fix segfault and tests","startedAt":"2014-08-28T14:54:32Z","lastAt":"2014-08-29T11:40:00Z","messageCount":7,"participants":["Maxim Bublis","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"248490","messageId":"1409237674-74185-1-git-send-email-satori@yandex-team.ru","threadId":"37437","inReplyTo":null,"subject":"[PATCH 0/2] fast-import: fix segfault and tests","fromName":"Maxim Bublis","fromEmail":"satori@yandex-team.ru","sentAt":"2014-08-28T14:54:32Z","receivedAt":"2014-08-28T14:54:32Z","isPatch":true,"sender":{"key":"satori@yandex-team.ru","avatar":null},"body":"Removing root tree with filedelete command would lead to segmentation fault\nin store_tree(). First patch from patch series adds test to show incorrect\nbehaviour and second one fixes bug by sanity check and load_tree() usage.\n\nMaxim Bublis (2):\n t9300: test filedelete root\n fast-import: fix segfault in store_tree()\n\n fast-import.c | 6 +++++-\n t/t9300-fast-import.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 51 insertions(+), 1 deletion(-)\n\n-- \n1.8.5.2 (Apple Git-48)\n"},{"id":"248492","messageId":"1409237674-74185-2-git-send-email-satori@yandex-team.ru","threadId":"37437","inReplyTo":"1409237674-74185-1-git-send-email-satori@yandex-team.ru","subject":"[PATCH 1/2] t9300: test filedelete root","fromName":"Maxim Bublis","fromEmail":"satori@yandex-team.ru","sentAt":"2014-08-28T14:54:33Z","receivedAt":"2014-08-28T14:54:33Z","isPatch":true,"sender":{"key":"satori@yandex-team.ru","avatar":null},"body":"Add new fast-import test series for filedelete command.\n\nSigned-off-by: Maxim Bublis \n---\n t/t9300-fast-import.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 46 insertions(+)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 5fc9ef2..3d557b3 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -3017,4 +3017,50 @@ test_expect_success 'T: empty reset doesnt delete branch' '\n \tgit rev-parse --verify refs/heads/not-to-delete\n '\n \n+###\n+### series U (filedelete)\n+###\n+\n+cat >input < $GIT_COMMITTER_DATE\n+data <input < $GIT_COMMITTER_DATE\n+data <expect <actual\n+\n+test_expect_success 'U: validate filedelete result' '\n+\tcompare_diff_raw expect actual\n+'\n+\n test_done\n-- \n1.8.5.2 (Apple Git-48)\n"},{"id":"248491","messageId":"1409237674-74185-3-git-send-email-satori@yandex-team.ru","threadId":"37437","inReplyTo":"1409237674-74185-1-git-send-email-satori@yandex-team.ru","subject":"[PATCH 2/2] fast-import: fix segfault in store_tree()","fromName":"Maxim Bublis","fromEmail":"satori@yandex-team.ru","sentAt":"2014-08-28T14:54:34Z","receivedAt":"2014-08-28T14:54:34Z","isPatch":true,"sender":{"key":"satori@yandex-team.ru","avatar":null},"body":"Branch tree is NULLified by filedelete command if we are trying\nto delete root tree. Add sanity check and use load_tree() in that case.\n\nSigned-off-by: Maxim Bublis \n---\n fast-import.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex d73f58c..b77f12c 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1419,7 +1419,7 @@ static void mktree(struct tree_content *t, int v, struct strbuf *b)\n \n static void store_tree(struct tree_entry *root)\n {\n-\tstruct tree_content *t = root->tree;\n+\tstruct tree_content *t;\n \tunsigned int i, j, del;\n \tstruct last_object lo = { STRBUF_INIT, 0, 0, /* no_swap */ 1 };\n \tstruct object_entry *le = NULL;\n@@ -1427,6 +1427,10 @@ static void store_tree(struct tree_entry *root)\n \tif (!is_null_sha1(root->versions[1].sha1))\n \t\treturn;\n \n+\tif (!root->tree)\n+\t\tload_tree(root)\n+\tt = root->tree;\n+\n \tfor (i = 0; i < t->entry_count; i++) {\n \t\tif (t->entries[i]->tree)\n \t\t\tstore_tree(t->entries[i]);\n-- \n1.8.5.2 (Apple Git-48)\n"},{"id":"248528","messageId":"xmqqa96o8cxa.fsf@gitster.dls.corp.google.com","threadId":"37437","inReplyTo":"1409237674-74185-2-git-send-email-satori@yandex-team.ru","subject":"Re: [PATCH 1/2] t9300: test filedelete root","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T22:30:41Z","receivedAt":"2014-08-28T22:30:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maxim Bublis writes:\n\n> Add new fast-import test series for filedelete command.\n>\n> Signed-off-by: Maxim Bublis \n> ---\n\nYou may have been concentrating on the \"delete root\" case, but as\nlong as you claim \"We add a series to test filedelete command\", it\nwould be sensible to test more typical cases of deleting files, not\nthe entire tree as well, no? Perhaps add three paths in the initial\ncommit e.g. hello.c, good/night.txt and good/bye.txt, first remove\ngood/night.txt and validate the result, then remove good/ directory\nand validate the result, and finally remove the whole thing and\nvalidate the result, or something like that?\n\nIn a patch series that introduces a demonstration of existing\nbreakage and then fixes the breakage in a separate patch, mark the\ntest that shows the known breakage with test_expect_failure and then\nturn that line into test_expect_success in the later patch that\nfixes the breakage.\n\nWhat the test checks and the fix in 2/2 both looked sensible from a\ncursory read.\n\nThanks.\n\n> t/t9300-fast-import.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n> 1 file changed, 46 insertions(+)\n>\n> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\n> index 5fc9ef2..3d557b3 100755\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -3017,4 +3017,50 @@ test_expect_success 'T: empty reset doesnt delete branch' '\n> \tgit rev-parse --verify refs/heads/not-to-delete\n> '\n> \n> +###\n> +### series U (filedelete)\n> +###\n> +\n> +cat >input < +commit refs/heads/U\n> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +data < +test setup\n> +COMMIT\n> +M 100644 inline hello.c\n> +data < +blob 1\n> +BLOB\n> +\n> +INPUT_END\n> +\n> +test_expect_success 'U: initialize for U tests' '\n> +\tgit fast-import +'\n> +\n> +cat >input < +commit refs/heads/U\n> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +data < +must succeed\n> +COMMIT\n> +from refs/heads/U^0\n> +D \"\"\n> +\n> +INPUT_END\n> +\n> +test_expect_success 'U: filedelete root succeeds' '\n> + git fast-import +'\n> +\n> +cat >expect < +:100644 000000 c18147dc648481eeb65dc5e66628429a64843327 0000000000000000000000000000000000000000 D\thello.c\n> +EOF\n> +\n> +git diff-tree -M -r U^1 U >actual\n> +\n> +test_expect_success 'U: validate filedelete result' '\n> +\tcompare_diff_raw expect actual\n> +'\n> +\n> test_done\n"},{"id":"248532","messageId":"xmqqwq9s6w8z.fsf@gitster.dls.corp.google.com","threadId":"37437","inReplyTo":"1409237674-74185-3-git-send-email-satori@yandex-team.ru","subject":"Re: [PATCH 2/2] fast-import: fix segfault in store_tree()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-08-28T23:16:12Z","receivedAt":"2014-08-28T23:16:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Maxim Bublis writes:\n\n> Branch tree is NULLified by filedelete command if we are trying\n> to delete root tree. Add sanity check and use load_tree() in that case.\n>\n> Signed-off-by: Maxim Bublis \n> ---\n> fast-import.c | 6 +++++-\n> 1 file changed, 5 insertions(+), 1 deletion(-)\n>\n> diff --git a/fast-import.c b/fast-import.c\n> index d73f58c..b77f12c 100644\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -1419,7 +1419,7 @@ static void mktree(struct tree_content *t, int v, struct strbuf *b)\n> \n> static void store_tree(struct tree_entry *root)\n> {\n> -\tstruct tree_content *t = root->tree;\n> +\tstruct tree_content *t;\n> \tunsigned int i, j, del;\n> \tstruct last_object lo = { STRBUF_INIT, 0, 0, /* no_swap */ 1 };\n> \tstruct object_entry *le = NULL;\n> @@ -1427,6 +1427,10 @@ static void store_tree(struct tree_entry *root)\n> \tif (!is_null_sha1(root->versions[1].sha1))\n> \t\treturn;\n> \n> +\tif (!root->tree)\n> +\t\tload_tree(root)\n\nMissing ';'\n\n> +\tt = root->tree;\n> +\n> \tfor (i = 0; i < t->entry_count; i++) {\n> \t\tif (t->entries[i]->tree)\n> \t\t\tstore_tree(t->entries[i]);\n"},{"id":"248539","messageId":"2F10795B-457A-493F-BF0F-CCCF0E64CF9C@yandex-team.ru","threadId":"37437","inReplyTo":"xmqqa96o8cxa.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/2] t9300: test filedelete root","fromName":"Maxim Bublis","fromEmail":"satori@yandex-team.ru","sentAt":"2014-08-29T11:37:25Z","receivedAt":"2014-08-29T11:37:25Z","isPatch":true,"sender":{"key":"satori@yandex-team.ru","avatar":null},"body":"On 29 авг. 2014 г., at 2:30, Junio C Hamano wrote:\n\n> You may have been concentrating on the \"delete root\" case, but as\n> long as you claim \"We add a series to test filedelete command\", it\n> would be sensible to test more typical cases of deleting files, not\n> the entire tree as well, no? Perhaps add three paths in the initial\n> commit e.g. hello.c, good/night.txt and good/bye.txt, first remove\n> good/night.txt and validate the result, then remove good/ directory\n> and validate the result, and finally remove the whole thing and\n> validate the result, or something like that?\n\nSure, I’ll add more tests for filedelete command.\n\n> In a patch series that introduces a demonstration of existing\n> breakage and then fixes the breakage in a separate patch, mark the\n> test that shows the known breakage with test_expect_failure and then\n> turn that line into test_expect_success in the later patch that\n> fixes the breakage.\n\nOk, thanks."},{"id":"248540","messageId":"F3FA18F7-324F-4E57-A936-5A8FA986E1BC@yandex-team.ru","threadId":"37437","inReplyTo":"xmqqwq9s6w8z.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 2/2] fast-import: fix segfault in store_tree()","fromName":"Maxim Bublis","fromEmail":"satori@yandex-team.ru","sentAt":"2014-08-29T11:40:00Z","receivedAt":"2014-08-29T11:40:00Z","isPatch":true,"sender":{"key":"satori@yandex-team.ru","avatar":null},"body":"\nOn 29 авг. 2014 г., at 3:16, Junio C Hamano wrote:\n\n> Missing ‘;'\n\nThanks, I’ll fix it. What a stupid mistype, I was writing some amount of Go code recently and it doesn’t use semicolons.\n"}]}