{"thread":{"id":"30852","subject":"[PATCH/RFC] fast-import: disallow empty branches as parents","startedAt":"2012-06-20T19:34:00Z","lastAt":"2012-06-25T08:00:57Z","messageCount":5,"participants":["Dmitry Ivankov","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"193974","messageId":"1340220841-753-1-git-send-email-divanorama@gmail.com","threadId":"30852","inReplyTo":null,"subject":"[PATCH/RFC] fast-import: disallow empty branches as parents","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-20T19:34:00Z","receivedAt":"2012-06-20T19:34:00Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Combinations of \"reset\", \"commit\" with \"from\" and/or \"merge\" commands\nmay make fast-import to produce bad objects (null_sha1 parents) or\naccept bad inputs (ones asking for empty branches as parents).\n\nFix this and add some tests.\n\n\nOne RFC here: does following use case make any sense/should it be allowed?\n\n    commit refs/heads/master\n    ...\n    from something\n    merge refs/heads/master\n\n1. If \"from\" is omitted or equals refs/heads/master we end up with duplicated parents.\n2. And if something is not master we allow to pick a new first parent path.\n\n\"2\" seems quite legal, while \"1\" looks worse. Though \"1\" is not directly related to\nthis patch and can be reproduced via a simple \"merge X X\" command for example.\n\n\nDmitry Ivankov (1):\n  fast-import: disallow empty branches as parents\n\n fast-import.c          |   49 +++++++++++++++++++++++++++++------------------\n t/t9300-fast-import.sh |   48 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+), 19 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"193975","messageId":"1340220841-753-2-git-send-email-divanorama@gmail.com","threadId":"30852","inReplyTo":"1340220841-753-1-git-send-email-divanorama@gmail.com","subject":"[PATCH] fast-import: disallow empty branches as parents","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-20T19:34:01Z","receivedAt":"2012-06-20T19:34:01Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"Empty branches (either new or reset-ed) have null_sha1 in fast-import\ninternals. These null_sha1 heads can slip to the real commit objects.\n\n- parse_merge() had no check against null_sha1, so add it.\n- parse_from() didn't have it too. It doesn't cause null_sha1 to slip\n  because for the first parent there is a null_sha1 check. But still\n  the input with \"from empty_branch\" must be rejected, make it so via\n  adding the check to parse_from().\n\nThere is a special case with \"merge branch_itself\" command. When the\nbranch_itself is empty it must clearly be rejected. Though in a\npresence of \"from ...\" command it is not. parse_from() writes the new\nfirst parent sha1 to branch->sha1 as a temporary placeholder, which\nparse_merge() then picks up. Though this sha1 might have never been a\ntip of the branch_itself at all.\n\nMake parse_from() store the first parent sha1 in a temporary buffer\nto fix this special case.\n\nAdd some tests for all these fixes.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n fast-import.c          |   49 +++++++++++++++++++++++++++++------------------\n t/t9300-fast-import.sh |   48 +++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 78 insertions(+), 19 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex eed97c8..2e089a8 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2540,7 +2540,8 @@ static void file_change_deleteall(struct branch *b)\n \tb->num_notes = 0;\n }\n \n-static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n+static void parse_from_commit(struct branch *b, unsigned char *sha1,\n+\t\t\t\t\t\tchar *buf, unsigned long size)\n {\n \tif (!buf || size < 46)\n \t\tdie(\"Not a valid commit: %s\", sha1_to_hex(b->sha1));\n@@ -2551,29 +2552,31 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n \t\tb->branch_tree.versions[1].sha1);\n }\n \n-static void parse_from_existing(struct branch *b)\n+static void parse_from_existing(struct branch *b, unsigned char *sha1)\n {\n-\tif (is_null_sha1(b->sha1)) {\n+\tif (is_null_sha1(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 \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\tbuf = read_object_with_reference(sha1,\n+\t\t\tcommit_type, &size, sha1);\n+\t\tparse_from_commit(b, sha1, buf, size);\n \t\tfree(buf);\n \t}\n }\n \n-static int parse_from(struct branch *b)\n+static int parse_from(struct branch *b, unsigned char *sha1out)\n {\n \tconst char *from;\n \tstruct branch *s;\n \n-\tif (prefixcmp(command_buf.buf, \"from \"))\n+\tif (prefixcmp(command_buf.buf, \"from \")) {\n+\t\thashclr(sha1out);\n \t\treturn 0;\n+\t}\n \n \tif (b->branch_tree.tree) {\n \t\trelease_tree_content_recursive(b->branch_tree.tree);\n@@ -2586,7 +2589,10 @@ static int parse_from(struct branch *b)\n \t\tdie(\"Can't create a branch from itself: %s\", b->name);\n \telse if (s) {\n \t\tunsigned char *t = s->branch_tree.versions[1].sha1;\n-\t\thashcpy(b->sha1, s->sha1);\n+\t\tif (is_null_sha1(s->sha1))\n+\t\t\tdie(\"Can't create a branch from an empty branch:\"\n+\t\t\t\t\" %s from %s\", b->name, s->name);\n+\t\thashcpy(sha1out, s->sha1);\n \t\thashcpy(b->branch_tree.versions[0].sha1, t);\n \t\thashcpy(b->branch_tree.versions[1].sha1, t);\n \t} else if (*from == ':') {\n@@ -2594,16 +2600,16 @@ static int parse_from(struct branch *b)\n \t\tstruct object_entry *oe = find_mark(idnum);\n \t\tif (oe->type != OBJ_COMMIT)\n \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n-\t\thashcpy(b->sha1, oe->idx.sha1);\n+\t\thashcpy(sha1out, oe->idx.sha1);\n \t\tif (oe->pack_id != MAX_PACK_ID) {\n \t\t\tunsigned long size;\n \t\t\tchar *buf = gfi_unpack_entry(oe, &size);\n-\t\t\tparse_from_commit(b, buf, size);\n+\t\t\tparse_from_commit(b, sha1out, buf, size);\n \t\t\tfree(buf);\n \t\t} else\n-\t\t\tparse_from_existing(b);\n-\t} else if (!get_sha1(from, b->sha1))\n-\t\tparse_from_existing(b);\n+\t\t\tparse_from_existing(b, sha1out);\n+\t} else if (!get_sha1(from, sha1out))\n+\t\tparse_from_existing(b, sha1out);\n \telse\n \t\tdie(\"Invalid ref name or SHA1 expression: %s\", from);\n \n@@ -2622,9 +2628,11 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\tfrom = strchr(command_buf.buf, ' ') + 1;\n \t\tn = xmalloc(sizeof(*n));\n \t\ts = lookup_branch(from);\n-\t\tif (s)\n+\t\tif (s) {\n+\t\t\tif (is_null_sha1(s->sha1))\n+\t\t\t\tdie(\"Can't merge empty branch: %s\", s->name);\n \t\t\thashcpy(n->sha1, s->sha1);\n-\t\telse if (*from == ':') {\n+\t\t} else if (*from == ':') {\n \t\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n \t\t\tstruct object_entry *oe = find_mark(idnum);\n \t\t\tif (oe->type != OBJ_COMMIT)\n@@ -2656,6 +2664,7 @@ static void parse_new_commit(void)\n {\n \tstatic struct strbuf msg = STRBUF_INIT;\n \tstruct branch *b;\n+\tunsigned char sha1[20];\n \tchar *sp;\n \tchar *author = NULL;\n \tchar *committer = NULL;\n@@ -2683,7 +2692,7 @@ static void parse_new_commit(void)\n \t\tdie(\"Expected committer but didn't get one\");\n \tparse_data(&msg, 0, NULL);\n \tread_next_command();\n-\tparse_from(b);\n+\tparse_from(b, sha1);\n \tmerge_list = parse_merge(&merge_count);\n \n \t/* ensure the branch is active/loaded */\n@@ -2730,7 +2739,9 @@ static void parse_new_commit(void)\n \tstrbuf_reset(&new_data);\n \tstrbuf_addf(&new_data, \"tree %s\\n\",\n \t\tsha1_to_hex(b->branch_tree.versions[1].sha1));\n-\tif (!is_null_sha1(b->sha1))\n+\tif (!is_null_sha1(sha1))\n+\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(sha1));\n+\telse if (!is_null_sha1(b->sha1))\n \t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(b->sha1));\n \twhile (merge_list) {\n \t\tstruct hash_list *next = merge_list->next;\n@@ -2855,7 +2866,7 @@ static void parse_reset_branch(void)\n \telse\n \t\tb = new_branch(sp);\n \tread_next_command();\n-\tparse_from(b);\n+\tparse_from(b, b->sha1);\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 2aa1824..5f25c01 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -2895,6 +2895,54 @@ test_expect_success 'S: merge with garbage after mark must fail' '\n \ttest_i18ngrep \"after mark\" err\n '\n \n+test_expect_success 'S: empty branch as merge parent must fail' '\n+\ttest_must_fail git fast-import <<-EOF 2>err &&\n+\tcommit refs/heads/chicken\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tI am the chicken.\n+\tCOMMIT\n+\tmerge refs/heads/chicken\n+\tEOF\n+\tcat err &&\n+\ttest_must_fail git rev-parse --verify refs/heads/chicken^\n+'\n+\n+test_expect_success 'S: empty branch as merge parent must fail (2)' '\n+\ttest_must_fail git fast-import <<-EOF 2>err &&\n+\tcommit refs/heads/egg1\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tI am the egg N1.\n+\tCOMMIT\n+\n+\tcommit refs/heads/egg2\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tI am the egg N2.\n+\tCOMMIT\n+\tfrom refs/heads/egg1\n+\tmerge refs/heads/egg2\n+\tEOF\n+\tcat err &&\n+\ttest_must_fail git rev-parse --verify refs/heads/egg2^2\n+'\n+\n+test_expect_success 'S: empty branch as a parent must fail ' '\n+\ttest_must_fail git fast-import <<-EOF 2>err &&\n+\treset refs/heads/egg3\n+\n+\tcommit refs/heads/egg4\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tI am the egg N4.\n+\tCOMMIT\n+\tfrom refs/heads/egg3\n+\tEOF\n+\tcat err &&\n+\ttest_must_fail git rev-parse --verify refs/heads/egg4^2\n+'\n+\n #\n # tag, from markref\n #\n-- \n1.7.3.4\n"},{"id":"194014","messageId":"20120621035753.GA3842@burratino","threadId":"30852","inReplyTo":"1340220841-753-2-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH] fast-import: disallow empty branches as parents","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-21T03:57:53Z","receivedAt":"2012-06-21T03:57:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi Dmitry,\n\nDmitry Ivankov wrote:\n\n> Empty branches (either new or reset-ed) have null_sha1 in fast-import\n> internals. These null_sha1 heads can slip to the real commit objects.\n[... nice explanation snipped ...]\n\nVery nice, thanks much for this.\n\nWould it be possible to split this into multiple independent fixes?\nSee [*] below for one way.\n\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2540,7 +2540,8 @@ static void file_change_deleteall(struct branch *b)\n>  \tb->num_notes = 0;\n>  }\n>  \n> -static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n> +static void parse_from_commit(struct branch *b, unsigned char *sha1,\n> +\t\t\t\t\t\tchar *buf, unsigned long size)\n>  {\n\nWhat is happening here?  The new argument doesn't seem to be used.\n\n[...]\n> @@ -2551,29 +2552,31 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n>  \t\tb->branch_tree.versions[1].sha1);\n>  }\n>  \n> -static void parse_from_existing(struct branch *b)\n> +static void parse_from_existing(struct branch *b, unsigned char *sha1)\n>  {\n> -\tif (is_null_sha1(b->sha1)) {\n> +\tif (is_null_sha1(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>  \n> -\t\tbuf = read_object_with_reference(b->sha1,\n> -\t\t\tcommit_type, &size, b->sha1);\n> +\t\tbuf = read_object_with_reference(sha1,\n> +\t\t\tcommit_type, &size, sha1);\n\nThis seems to be about delaying the effect of \"from\" so it doesn't\ninterfere with a \"merge\" command referring to the same commit.\n\n[...]\n> -static int parse_from(struct branch *b)\n> +static int parse_from(struct branch *b, unsigned char *sha1out)\n>  {\n>  \tconst char *from;\n>  \tstruct branch *s;\n>  \n> -\tif (prefixcmp(command_buf.buf, \"from \"))\n> +\tif (prefixcmp(command_buf.buf, \"from \")) {\n> +\t\thashclr(sha1out);\n>  \t\treturn 0;\n> +\t}\n\nThis code path handles the case where there is no \"from\" after a\n\"reset\" or \"commit\" command.  We clear sha1out to make the calling\nconvention simple --- sha1out is always written to, and the caller\ndoes not have to worry about initializing it in advance.\n\nI guess this is part of the change that delays the effect of \"from\"?\n\n[...]\n> @@ -2586,7 +2589,10 @@ static int parse_from(struct branch *b)\n>  \t\tdie(\"Can't create a branch from itself: %s\", b->name);\n>  \telse if (s) {\n>  \t\tunsigned char *t = s->branch_tree.versions[1].sha1;\n> +\t\tif (is_null_sha1(s->sha1))\n> +\t\t\tdie(\"Can't create a branch from an empty branch:\"\n> +\t\t\t\t\" %s from %s\", b->name, s->name);\n\nThis seems to be about protecting against \"from\" with an unborn\nbranch.\n\n> -\t\thashcpy(b->sha1, s->sha1);\n> +\t\thashcpy(sha1out, s->sha1);\n\nDelaying the effect of \"from\", maybe.\n\n[...]\n> @@ -2594,16 +2600,16 @@ static int parse_from(struct branch *b)\n>  \t\tstruct object_entry *oe = find_mark(idnum);\n>  \t\tif (oe->type != OBJ_COMMIT)\n>  \t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n> -\t\thashcpy(b->sha1, oe->idx.sha1);\n> +\t\thashcpy(sha1out, oe->idx.sha1);\n\nDelaying \"from\" effect?\n\n>  \t\tif (oe->pack_id != MAX_PACK_ID) {\n>  \t\t\tunsigned long size;\n>  \t\t\tchar *buf = gfi_unpack_entry(oe, &size);\n> -\t\t\tparse_from_commit(b, buf, size);\n> +\t\t\tparse_from_commit(b, sha1out, buf, size);\n\nLikewise?\n\n>  \t\t\tfree(buf);\n>  \t\t} else\n> -\t\t\tparse_from_existing(b);\n> +\t\t\tparse_from_existing(b, sha1out);\n> -\t} else if (!get_sha1(from, b->sha1))\n> +\t} else if (!get_sha1(from, sha1out))\n> -\t\tparse_from_existing(b);\n> +\t\tparse_from_existing(b, sha1out);\n\nLikewise?\n\n[...]\n>  \telse\n>  \t\tdie(\"Invalid ref name or SHA1 expression: %s\", from);\n>  \n> @@ -2622,9 +2628,11 @@ static struct hash_list *parse_merge(unsigned int *count)\n>  \t\tfrom = strchr(command_buf.buf, ' ') + 1;\n>  \t\tn = xmalloc(sizeof(*n));\n>  \t\ts = lookup_branch(from);\n> -\t\tif (s)\n> +\t\tif (s) {\n> +\t\t\tif (is_null_sha1(s->sha1))\n> +\t\t\t\tdie(\"Can't merge empty branch: %s\", s->name);\n>  \t\t\thashcpy(n->sha1, s->sha1);\n> -\t\telse if (*from == ':') {\n> +\t\t} else if (*from == ':') {\n\nProtecting against \"merge\" with unknown branch.\n\n[...]\n> @@ -2656,6 +2664,7 @@ static void parse_new_commit(void)\n>  {\n>  \tstatic struct strbuf msg = STRBUF_INIT;\n>  \tstruct branch *b;\n> +\tunsigned char sha1[20];\n>  \tchar *sp;\n>  \tchar *author = NULL;\n>  \tchar *committer = NULL;\n> @@ -2683,7 +2692,7 @@ static void parse_new_commit(void)\n>  \t\tdie(\"Expected committer but didn't get one\");\n>  \tparse_data(&msg, 0, NULL);\n>  \tread_next_command();\n> -\tparse_from(b);\n> +\tparse_from(b, sha1);\n\nDelayed \"from\" effect?\n\n[...]\n> @@ -2730,7 +2739,9 @@ static void parse_new_commit(void)\n>  \tstrbuf_reset(&new_data);\n>  \tstrbuf_addf(&new_data, \"tree %s\\n\",\n>  \t\tsha1_to_hex(b->branch_tree.versions[1].sha1));\n> -\tif (!is_null_sha1(b->sha1))\n> +\tif (!is_null_sha1(sha1))\n> +\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(sha1));\n> +\telse if (!is_null_sha1(b->sha1))\n>  \t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(b->sha1));\n\nDelaying \"from\"?\n\n[...]\n> @@ -2855,7 +2866,7 @@ static void parse_reset_branch(void)\n>  \telse\n>  \t\tb = new_branch(sp);\n>  \tread_next_command();\n> -\tparse_from(b);\n> +\tparse_from(b, b->sha1);\n\nLikewise?\n\n[*]\nSo it looks like this would be easier to read as three patches:\n\n 1. protecting against \"from\" of unborn branch\n 2. protecting against \"merge\" of unborn branch\n 3. delaying the effect of \"from\" to avoid it confusingly changing\n    the effect of a \"merge\" in the same commit\n\n(1) and (2) would be no-brainers, while (3) seems more subtle ---\nmaybe it should be documented to help importers for other version\ncontrol systems to know to make the same change?\n\n[...]\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -2895,6 +2895,54 @@ test_expect_success 'S: merge with garbage after mark must fail' '\n>  \ttest_i18ngrep \"after mark\" err\n>  '\n>  \n> +test_expect_success 'S: empty branch as merge parent must fail' '\n> +\ttest_must_fail git fast-import <<-EOF 2>err &&\n> +\tcommit refs/heads/chicken\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata <<COMMIT\n> +\tI am the chicken.\n> +\tCOMMIT\n> +\tmerge refs/heads/chicken\n> +\tEOF\n> +\tcat err &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/chicken^\n\nThere would be no \"chicken\" branch after this import at all, right?\n\n[...]\n> +test_expect_success 'S: empty branch as merge parent must fail (2)' '\n> +\ttest_must_fail git fast-import <<-EOF 2>err &&\n> +\tcommit refs/heads/egg1\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata <<COMMIT\n> +\tI am the egg N1.\n> +\tCOMMIT\n> +\n> +\tcommit refs/heads/egg2\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata <<COMMIT\n> +\tI am the egg N2.\n> +\tCOMMIT\n> +\tfrom refs/heads/egg1\n> +\tmerge refs/heads/egg2\n> +\tEOF\n> +\tcat err &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/egg2^2\n\nLikewise for egg2.\n\n[...]\n> +test_expect_success 'S: empty branch as a parent must fail ' '\n> +\ttest_must_fail git fast-import <<-EOF 2>err &&\n> +\treset refs/heads/egg3\n> +\n> +\tcommit refs/heads/egg4\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +\tdata <<COMMIT\n> +\tI am the egg N4.\n> +\tCOMMIT\n> +\tfrom refs/heads/egg3\n> +\tEOF\n> +\tcat err &&\n> +\ttest_must_fail git rev-parse --verify refs/heads/egg4^2\n\nLikewise for egg4.\n\nIf this were split up as described above, I imagine it would be much\neasier to read and most of it would move into the \"obviously good\"\ncategory (I'm still uncertain about some details of the \"delayed from\"\nimplementation and haven't checked carefully whether it misses any\nspots).  What do you think?\n\nThanks for a pleasant read, and hope that helps,\nJonathan\n"},{"id":"194030","messageId":"CA+gfSn9+Wg9oA7q4=nWdNXuG98n-p8VHYaN_9nVeGUk8iHpeSA@mail.gmail.com","threadId":"30852","inReplyTo":"20120621035753.GA3842@burratino","subject":"Re: [PATCH] fast-import: disallow empty branches as parents","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-21T06:39:34Z","receivedAt":"2012-06-21T06:39:34Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"On Thu, Jun 21, 2012 at 9:57 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Hi Dmitry,\n>\n> Dmitry Ivankov wrote:\n>\n>> Empty branches (either new or reset-ed) have null_sha1 in fast-import\n>> internals. These null_sha1 heads can slip to the real commit objects.\n> [... nice explanation snipped ...]\n>\n> Very nice, thanks much for this.\n>\n> Would it be possible to split this into multiple independent fixes?\n> See [*] below for one way.\n>\n>> --- a/fast-import.c\n>> +++ b/fast-import.c\n>> @@ -2540,7 +2540,8 @@ static void file_change_deleteall(struct branch *b)\n>>       b->num_notes = 0;\n>>  }\n>>\n>> -static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n>> +static void parse_from_commit(struct branch *b, unsigned char *sha1,\n>> +                                             char *buf, unsigned long size)\n>>  {\n>\n> What is happening here?  The new argument doesn't seem to be used.\nOw, sorry. In fact it is here for die() messages.\nA bit like spaghetti code, maybe die()-s can be moved to the caller.\n\n>\n> [...]\n>> @@ -2551,29 +2552,31 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)\n>>               b->branch_tree.versions[1].sha1);\n>>  }\n>>\n>> -static void parse_from_existing(struct branch *b)\n>> +static void parse_from_existing(struct branch *b, unsigned char *sha1)\n>>  {\n>> -     if (is_null_sha1(b->sha1)) {\n>> +     if (is_null_sha1(sha1)) {\n>>               hashclr(b->branch_tree.versions[0].sha1);\n>>               hashclr(b->branch_tree.versions[1].sha1);\n>>       } else {\n>>               unsigned long size;\n>>               char *buf;\n>>\n>> -             buf = read_object_with_reference(b->sha1,\n>> -                     commit_type, &size, b->sha1);\n>> +             buf = read_object_with_reference(sha1,\n>> +                     commit_type, &size, sha1);\n>\n> This seems to be about delaying the effect of \"from\" so it doesn't\n> interfere with a \"merge\" command referring to the same commit.\n\nKind of, parse_from_*() arrange b->sha1 and b->branch_tree to correspond\nthe first parent of a new tip. With this patch b->sha1 is left alone\nto avoid the\ninterference. Oh, yes, luckily parse_merge() doesn't need b->branch_tree-s.\n\n\n>\n> [...]\n>> -static int parse_from(struct branch *b)\n>> +static int parse_from(struct branch *b, unsigned char *sha1out)\n>>  {\n>>       const char *from;\n>>       struct branch *s;\n>>\n>> -     if (prefixcmp(command_buf.buf, \"from \"))\n>> +     if (prefixcmp(command_buf.buf, \"from \")) {\n>> +             hashclr(sha1out);\n>>               return 0;\n>> +     }\n>\n> This code path handles the case where there is no \"from\" after a\n> \"reset\" or \"commit\" command.  We clear sha1out to make the calling\n> convention simple --- sha1out is always written to, and the caller\n> does not have to worry about initializing it in advance.\n>\n> I guess this is part of the change that delays the effect of \"from\"?\n>\n> [...]\n>> @@ -2586,7 +2589,10 @@ static int parse_from(struct branch *b)\n>>               die(\"Can't create a branch from itself: %s\", b->name);\n>>       else if (s) {\n>>               unsigned char *t = s->branch_tree.versions[1].sha1;\n>> +             if (is_null_sha1(s->sha1))\n>> +                     die(\"Can't create a branch from an empty branch:\"\n>> +                             \" %s from %s\", b->name, s->name);\n>\n> This seems to be about protecting against \"from\" with an unborn\n> branch.\n>\n>> -             hashcpy(b->sha1, s->sha1);\n>> +             hashcpy(sha1out, s->sha1);\n>\n> Delaying the effect of \"from\", maybe.\n>\n> [...]\n>> @@ -2594,16 +2600,16 @@ static int parse_from(struct branch *b)\n>>               struct object_entry *oe = find_mark(idnum);\n>>               if (oe->type != OBJ_COMMIT)\n>>                       die(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n>> -             hashcpy(b->sha1, oe->idx.sha1);\n>> +             hashcpy(sha1out, oe->idx.sha1);\n>\n> Delaying \"from\" effect?\n>\n>>               if (oe->pack_id != MAX_PACK_ID) {\n>>                       unsigned long size;\n>>                       char *buf = gfi_unpack_entry(oe, &size);\n>> -                     parse_from_commit(b, buf, size);\n>> +                     parse_from_commit(b, sha1out, buf, size);\n>\n> Likewise?\n>\n>>                       free(buf);\n>>               } else\n>> -                     parse_from_existing(b);\n>> +                     parse_from_existing(b, sha1out);\n>> -     } else if (!get_sha1(from, b->sha1))\n>> +     } else if (!get_sha1(from, sha1out))\n>> -             parse_from_existing(b);\n>> +             parse_from_existing(b, sha1out);\n>\n> Likewise?\n>\n> [...]\n>>       else\n>>               die(\"Invalid ref name or SHA1 expression: %s\", from);\n>>\n>> @@ -2622,9 +2628,11 @@ static struct hash_list *parse_merge(unsigned int *count)\n>>               from = strchr(command_buf.buf, ' ') + 1;\n>>               n = xmalloc(sizeof(*n));\n>>               s = lookup_branch(from);\n>> -             if (s)\n>> +             if (s) {\n>> +                     if (is_null_sha1(s->sha1))\n>> +                             die(\"Can't merge empty branch: %s\", s->name);\n>>                       hashcpy(n->sha1, s->sha1);\n>> -             else if (*from == ':') {\n>> +             } else if (*from == ':') {\n>\n> Protecting against \"merge\" with unknown branch.\n>\n> [...]\n>> @@ -2656,6 +2664,7 @@ static void parse_new_commit(void)\n>>  {\n>>       static struct strbuf msg = STRBUF_INIT;\n>>       struct branch *b;\n>> +     unsigned char sha1[20];\n>>       char *sp;\n>>       char *author = NULL;\n>>       char *committer = NULL;\n>> @@ -2683,7 +2692,7 @@ static void parse_new_commit(void)\n>>               die(\"Expected committer but didn't get one\");\n>>       parse_data(&msg, 0, NULL);\n>>       read_next_command();\n>> -     parse_from(b);\n>> +     parse_from(b, sha1);\n>\n> Delayed \"from\" effect?\n>\n> [...]\n>> @@ -2730,7 +2739,9 @@ static void parse_new_commit(void)\n>>       strbuf_reset(&new_data);\n>>       strbuf_addf(&new_data, \"tree %s\\n\",\n>>               sha1_to_hex(b->branch_tree.versions[1].sha1));\n>> -     if (!is_null_sha1(b->sha1))\n>> +     if (!is_null_sha1(sha1))\n>> +             strbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(sha1));\n>> +     else if (!is_null_sha1(b->sha1))\n>>               strbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(b->sha1));\n>\n> Delaying \"from\"?\n>\n> [...]\n>> @@ -2855,7 +2866,7 @@ static void parse_reset_branch(void)\n>>       else\n>>               b = new_branch(sp);\n>>       read_next_command();\n>> -     parse_from(b);\n>> +     parse_from(b, b->sha1);\n>\n> Likewise?\n>\n> [*]\n> So it looks like this would be easier to read as three patches:\n>\n>  1. protecting against \"from\" of unborn branch\n>  2. protecting against \"merge\" of unborn branch\n>  3. delaying the effect of \"from\" to avoid it confusingly changing\n>    the effect of a \"merge\" in the same commit\nThanks, will look into it. Without (3) we'll still have inputs asking\nfor (2) accepted, like:\n    commit new_tip\n    ...\n    from old_tip\n    merge new_tip\nnew_tip won't end up with null_sha1 parent written, so should be ok to\npostpone this case to (3).\n\n>\n> (1) and (2) would be no-brainers, while (3) seems more subtle ---\n> maybe it should be documented to help importers for other version\n> control systems to know to make the same change?\nWill add a note on this.\n\n>\n> [...]\n>> --- a/t/t9300-fast-import.sh\n>> +++ b/t/t9300-fast-import.sh\n>> @@ -2895,6 +2895,54 @@ test_expect_success 'S: merge with garbage after mark must fail' '\n>>       test_i18ngrep \"after mark\" err\n>>  '\n>>\n>> +test_expect_success 'S: empty branch as merge parent must fail' '\n>> +     test_must_fail git fast-import <<-EOF 2>err &&\n>> +     commit refs/heads/chicken\n>> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n>> +     data <<COMMIT\n>> +     I am the chicken.\n>> +     COMMIT\n>> +     merge refs/heads/chicken\n>> +     EOF\n>> +     cat err &&\n>> +     test_must_fail git rev-parse --verify refs/heads/chicken^\n>\n> There would be no \"chicken\" branch after this import at all, right?\nOh, yes, these rev-parse are all to illustrate the reason why import must fail.\nWith this one we can add a positive test where chicken is a born branch and\nimport succeeds.\n\n>\n> [...]\n>> +test_expect_success 'S: empty branch as merge parent must fail (2)' '\n>> +     test_must_fail git fast-import <<-EOF 2>err &&\n>> +     commit refs/heads/egg1\n>> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n>> +     data <<COMMIT\n>> +     I am the egg N1.\n>> +     COMMIT\n>> +\n>> +     commit refs/heads/egg2\n>> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n>> +     data <<COMMIT\n>> +     I am the egg N2.\n>> +     COMMIT\n>> +     from refs/heads/egg1\n>> +     merge refs/heads/egg2\n>> +     EOF\n>> +     cat err &&\n>> +     test_must_fail git rev-parse --verify refs/heads/egg2^2\n>\n> Likewise for egg2.\nAdding a positive one here too looks a good idea.\n\n>\n> [...]\n>> +test_expect_success 'S: empty branch as a parent must fail ' '\n>> +     test_must_fail git fast-import <<-EOF 2>err &&\n>> +     reset refs/heads/egg3\n>> +\n>> +     commit refs/heads/egg4\n>> +     committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n>> +     data <<COMMIT\n>> +     I am the egg N4.\n>> +     COMMIT\n>> +     from refs/heads/egg3\n>> +     EOF\n>> +     cat err &&\n>> +     test_must_fail git rev-parse --verify refs/heads/egg4^2\n>\n> Likewise for egg4.\nThere is a copy-paste typo \"egg4^2\" instead of \"egg4\". Will just drop\nrev-parse here as there should be a positive merge test somewhere\nalready.\n\n>\n> If this were split up as described above, I imagine it would be much\n> easier to read and most of it would move into the \"obviously good\"\n> category (I'm still uncertain about some details of the \"delayed from\"\n> implementation and haven't checked carefully whether it misses any\n> spots).  What do you think?\nThanks, good idea indeed. Will rearrange and resend.\n\n>\n> Thanks for a pleasant read, and hope that helps,\n> Jonathan\n"},{"id":"194188","messageId":"CA+gfSn9oWTxwhrsmE9NeD0awkghSknfNcm8CcVqRGveGg_g+Lw@mail.gmail.com","threadId":"30852","inReplyTo":"1340220841-753-1-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH/RFC] fast-import: disallow empty branches as parents","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-25T08:00:57Z","receivedAt":"2012-06-25T08:00:57Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"+Shawn Pearce\n\nOn Thu, Jun 21, 2012 at 1:34 AM, Dmitry Ivankov <divanorama@gmail.com> wrote:\n> Combinations of \"reset\", \"commit\" with \"from\" and/or \"merge\" commands\n> may make fast-import to produce bad objects (null_sha1 parents) or\n> accept bad inputs (ones asking for empty branches as parents).\n\nI was trying to split the patch into small an obvious cases and found out that\nthere are 4 ways to make or refer to \"empty\" branches:\n1. reset branch_name\n2. reset branch_name \\n from `0{40}`\n3. refer to it as `0{40}`\n4. commit branch_name for a new branch name. It's empty up to the\n'commit' command end.\nNote: I'll use 4 to denote a reference to a branch name from the\n'commit' command to this very branch name.\n\nAnd these branches can be used in 'from' and 'merge' commands.\n\nIn 'from' command:\n- 1,2,3 are allowed in 'from', no bug of writing 0{40} parent here as\n'from' parent is checked against null_sha1\n- 4 is not allowed with message \"Can't create a branch from itself\"\n\nIn 'merge' command:\n- 1,2 are allowed in 'merge' and lead to 0{40} being actually written\nas a commit parent - BUG\n- 3 is not allowed with message \"Not a valid commit\"\n- 4 without 'from' is allowed, parent is previous branch tip. May be 0{40} - BUG\n- 4 with 'from' is allowed, 'merge' parent is 'from' parent - probably\na bug, also may lead to 0{40} being written - BUG\n\nBUG stuff obviously needs to be fixed, at least we can just skip 0{40}\nparents in 'merge' command.\n\nThen difference in 3 between 'from' and 'merge' should be dealt with.\n'merge' behaviour  comes from\n    tags/v1.5.0.4~21^2 2f6dc35d2ad0b.. 5 Mar 2007 fast-import: Fail if\na non-existant commit is used for merge\nsince then 'merge sha1' just looks up commit with this sha1. But\n'from' command has it's story too - it was\na 'new_branch' long ago (up to tags/v1.5.0-rc4~14^2~64) and since the\nvery beginning (tags/v1.5.0-rc4~14^2~76)\nis had a special case to allow 0{40} as a parent. So, should we just\nallow 'merge 0{40}'?\n\nAnd finally 4 in 'merge' should probably always refer to the previous\ntip to cause less surprise (it was the largest\npart of my patch, though it turns out not the most interesting one).\n\nP.S. At first I thought that any reference to an empty branch in\n'merge' of 'from' should be rejected, but given that we\nallow sha1 0{40} to be used in these and in 2, I guess we should keep\nit allowed.\n"}]}