{"thread":{"id":"28423","subject":"[PATCH/RFC 0/2] fast-import: commit from null_sha1","startedAt":"2011-09-18T21:20:44Z","lastAt":"2011-09-18T21:40:43Z","messageCount":5,"participants":["Dmitry Ivankov","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"175747","messageId":"1316380846-15845-1-git-send-email-divanorama@gmail.com","threadId":"28423","inReplyTo":null,"subject":"[PATCH/RFC 0/2] fast-import: commit from null_sha1","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T21:20:44Z","receivedAt":"2011-09-18T21:20:44Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Not so sure how null_sha1 parent should be treated in fast-import.\nAbsent parent is represented as null_sha1 to the user in reflog,\nbut isn't allowed as an argument for porcelain nor shows in most\nplumbing commands afaik.\n\nThese patches make fast-import treat\n commit refs/heads/master\n ...\n from `null_sha1`\nlike any other missing parent sha1 - reject such input.\n\nNote: if we'll want this input to be valid, some other adjustments\nto fast-import logic may be needed for consistency.\n\nDmitry Ivankov (2):\n fast-import: add 'commit from 0{40}' failing test\n fast-import: fix 'from 0{40}' test\n\n fast-import.c | 17 ++++++-----------\n t/t9300-fast-import.sh | 12 ++++++++++++\n 2 files changed, 18 insertions(+), 11 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"175748","messageId":"1316380846-15845-2-git-send-email-divanorama@gmail.com","threadId":"28423","inReplyTo":"1316380846-15845-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 1/2] fast-import: add 'commit from 0{40}' failing test","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T21:20:45Z","receivedAt":"2011-09-18T21:20:45Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Following shouldn't be allowed, while it is:\n\ncommit refs/heads/some\ncommitter ...\ndata ...\nfrom `null_sha1`\n\nIt is treated as if 'from' was omitted. But it is allowed to just\nomit 'from' actually. And `null_sha1` being special in fast-import\nis an internal implementation detail.\n\nAdd a test as described.\n\nSigned-off-by: Dmitry Ivankov \n---\n t/t9300-fast-import.sh | 12 ++++++++++++\n 1 files changed, 12 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 1a6c066..8cc3f16 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -375,6 +375,18 @@ test_expect_success 'B: fail on invalid branch name \"bad[branch]name\"' '\n rm -f .git/objects/pack_* .git/objects/index_*\n \n cat >input < $GIT_COMMITTER_DATE\n+data 0\n+\n+from 0000000000000000000000000000000000000000\n+INPUT_END\n+test_expect_failure 'B: fail on \"from 0{40}\"' '\n+ test_must_fail git fast-import input < $GIT_COMMITTER_DATE\n data <\n---\n fast-import.c | 17 ++++++-----------\n t/t9300-fast-import.sh | 2 +-\n 2 files changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 742e7da..827434a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2488,18 +2488,13 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n \n static void parse_from_existing(struct branch *b)\n {\n-\tif (is_null_sha1(b->sha1)) {\n-\t\thashclr(b->branch_tree.versions[0].sha1);\n-\t\thashclr(b->branch_tree.versions[1].sha1);\n-\t} else {\n-\t\tunsigned long size;\n-\t\tchar *buf;\n+\tunsigned long size;\n+\tchar *buf;\n \n-\t\tbuf = read_object_with_reference(b->sha1,\n-\t\t\tcommit_type, &size, b->sha1);\n-\t\tparse_from_commit(b, buf, size);\n-\t\tfree(buf);\n-\t}\n+\tbuf = read_object_with_reference(b->sha1,\n+\t\tcommit_type, &size, b->sha1);\n+\tparse_from_commit(b, buf, size);\n+\tfree(buf);\n }\n \n static int parse_from(struct branch *b)\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 8cc3f16..0784d50 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -381,7 +381,7 @@ data 0\n \n from 0000000000000000000000000000000000000000\n INPUT_END\n-test_expect_failure 'B: fail on \"from 0{40}\"' '\n+test_expect_success 'B: fail on \"from 0{40}\"' '\n test_must_fail git fast-import These patches make fast-import treat\n> commit refs/heads/master\n> ...\n> from `null_sha1`\n> like any other missing parent sha1 - reject such input.\n\nAre you sure the existing support for \"from 0{40}\" is not deliberate\nand that no one is relying on it? If and only if you are, then this\nseems like a good idea (a single patch that both makes the behavior\nchange and adds a test for it should be easier to review).\n"},{"id":"175754","messageId":"CA+gfSn_WG_S+QJK5O_4D62KV78-77QO_gyy7PeFGRJYW4cQu8A@mail.gmail.com","threadId":"28423","inReplyTo":"20110918213050.GJ2308@elie","subject":"Re: [PATCH/RFC 0/2] fast-import: commit from null_sha1","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T21:40:43Z","receivedAt":"2011-09-18T21:40:43Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"On Mon, Sep 19, 2011 at 3:30 AM, Jonathan Nieder wrote:\n> Dmitry Ivankov wrote:\n>\n>> These patches make fast-import treat\n>>     commit refs/heads/master\n>>     ...\n>>     from `null_sha1`\n>> like any other missing parent sha1 - reject such input.\n>\n> Are you sure the existing support for \"from 0{40}\" is not deliberate\n> and that no one is relying on it?\nIt is hard to guess. There is no test for it in t/t9300-fast-import.sh, no\nmention in the Documentation, but sometimes a user can see null_sha1\nfrom git. I hope that it pops up only when something is read to simplify\nthe format and never accepted in 'write' commands or as an argument.\n\n>  If and only if you are, then this\n> seems like a good idea (a single patch that both makes the behavior\n> change and adds a test for it should be easier to review).\n>\n"}]}