{"thread":{"id":"30918","subject":"[PATCH/RFC v2 0/3] fast-import: disallow empty branches as parents","startedAt":"2012-06-27T17:40:22Z","lastAt":"2012-07-24T19:40:47Z","messageCount":12,"participants":["Dmitry Ivankov","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":3},"messages":[{"id":"194359","messageId":"1340818825-13754-1-git-send-email-divanorama@gmail.com","threadId":"30918","inReplyTo":null,"subject":"[PATCH/RFC v2 0/3] fast-import: disallow empty branches as parents","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-27T17:40:22Z","receivedAt":"2012-06-27T17:40:22Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"This is a rewrite of [1].\n\nFirst of all the patch is split into several parts now.\n\n1/3 prevents writing invalid commit objects (with null_sha1 parents)\n\nFor 2/3 and 3/3 I've changed my mind almost completely. The commands\nin question are:\nA 'from null_sha1'\nB 'from empty_branch'\nC 'from itself'\nD 'merge null_sha1'\nE 'merge empty_branch'\nF 'merge itself'\n\nCurrently C is disallowed, D and E lead to 1/3 bug, F looks broken, A and B are allowed.\n\nIn [1] I kept A allowed, but made B, C, D, E disallowed and \"fixed\" F.\nThe idea was too keep A as legacy, fix F as it may have applications and disallow others\nas they look like errors in import stream.\n\nThis time I keep A and B allowed, allow D and E, disallow F.\nNow I think of null_sha1 as of a special feature, empty_branch things as a mix of legacy\nand this feature (one can 'reset' branch to null_sha1, then use it's name and expect it\nto work as if null_sha1 was used, and null_sha1 is allowed). \"Fix\" for F is dropped for\nnow and will later go separately with it's own set of tests and a new discussion I guess.\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/200339\n\nDmitry Ivankov (3):\n fast-import: do not write null_sha1 as a merge parent\n fast-import: allow \"merge $null_sha1\" command\n fast-import: disallow \"merge $itself\" command\n\n fast-import.c | 29 +++++++++++++++++++----------\n t/t9300-fast-import.sh | 35 +++++++++++++++++++++++++++++++++++\n 2 files changed, 54 insertions(+), 10 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"194360","messageId":"1340818825-13754-2-git-send-email-divanorama@gmail.com","threadId":"30918","inReplyTo":"1340818825-13754-1-git-send-email-divanorama@gmail.com","subject":"[PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-27T17:40:23Z","receivedAt":"2012-06-27T17:40:23Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"null_sha1 is used in fast-import to indicate \"empty\" branches and\nshould never be actually written out as a commit parent. 'merge'\ncommand lacks is_null_sha1 checks and must be fixed.\n\nIt looks like using null_sha1 or empty branches in 'from' command\nis legal and/or an intended option (it has been here from the very\nbeginning and survived). So leave it allowed for 'merge' command too,\nand just like with 'from' command silently skip null_sha1 parents.\n\nAdd a simple test for null_sha1 merge parents.\n\nSigned-off-by: Dmitry Ivankov \n---\n fast-import.c | 3 ++-\n t/t9300-fast-import.sh | 21 +++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex eed97c8..419e435 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2734,7 +2734,8 @@ static void parse_new_commit(void)\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-\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(merge_list->sha1));\n+\t\tif (!is_null_sha1(merge_list->sha1))\n+\t\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(merge_list->sha1));\n \t\tfree(merge_list);\n \t\tmerge_list = next;\n \t}\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex c17f52e..5716420 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -850,6 +850,27 @@ INPUT_END\n test_expect_success \\\n \t'J: tag must fail on empty branch' \\\n \t'test_must_fail git fast-import input < $GIT_COMMITTER_DATE\n+data <\n---\n fast-import.c | 14 ++++++++------\n t/t9300-fast-import.sh | 1 +\n 2 files changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 419e435..f03da1e 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2631,12 +2631,14 @@ static struct hash_list *parse_merge(unsigned int *count)\n \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n \t\t\thashcpy(n->sha1, oe->idx.sha1);\n \t\t} else if (!get_sha1(from, n->sha1)) {\n-\t\t\tunsigned long size;\n-\t\t\tchar *buf = read_object_with_reference(n->sha1,\n-\t\t\t\tcommit_type, &size, n->sha1);\n-\t\t\tif (!buf || size < 46)\n-\t\t\t\tdie(\"Not a valid commit: %s\", from);\n-\t\t\tfree(buf);\n+\t\t\tif (!is_null_sha1(n->sha1)) {\n+\t\t\t\tunsigned long size;\n+\t\t\t\tchar *buf = read_object_with_reference(n->sha1,\n+\t\t\t\t\tcommit_type, &size, n->sha1);\n+\t\t\t\tif (!buf || size < 46)\n+\t\t\t\t\tdie(\"Not a valid commit: %s\", from);\n+\t\t\t\tfree(buf);\n+\t\t\t}\n \t\t} else\n \t\t\tdie(\"Invalid ref name or SHA1 expression: %s\", from);\n \ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 5716420..6f4c988 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -864,6 +864,7 @@ Merge J3, J4 into fresh J5.\n COMMIT\n merge refs/heads/J3\n merge refs/heads/J4\n+merge 0000000000000000000000000000000000000000\n \n INPUT_END\n test_expect_success \\\n-- \n1.7.3.4\n"},{"id":"194361","messageId":"1340818825-13754-4-git-send-email-divanorama@gmail.com","threadId":"30918","inReplyTo":"1340818825-13754-1-git-send-email-divanorama@gmail.com","subject":"[PATCH v2 3/3] fast-import: disallow \"merge $itself\" command","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2012-06-27T17:40:25Z","receivedAt":"2012-06-27T17:40:25Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"\"merge $itself\" may be used to create commits with previous branch tip\nbeing repeated as n-th parent or even moved from being 1-st to be just\nn-th. This is not a documented use case and doesn't look like a common\none.\n\nIn presence of \"from $some\" command \"merge $itself\" acts the same as\n\"merge $some\" would. Which is completely undocumented and looks like\na bug (caused by parse_from() temporarily rewriting b->sha1 with $some).\n\nJust deny \"merge $itself\" for now. It was a bit broken and btw \"from\n$itself\" was and is a forbidden command too.\n\nSigned-off-by: Dmitry Ivankov \n---\n fast-import.c | 12 +++++++++---\n t/t9300-fast-import.sh | 13 +++++++++++++\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex f03da1e..781c614 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2611,7 +2611,7 @@ static int parse_from(struct branch *b)\n \treturn 1;\n }\n \n-static struct hash_list *parse_merge(unsigned int *count)\n+static struct hash_list *parse_merge(unsigned int *count, struct branch *b)\n {\n \tstruct hash_list *list = NULL, *n, *e = e;\n \tconst char *from;\n@@ -2622,7 +2622,13 @@ 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 (b == s)\n+\t\t\t/*\n+\t\t\t * Also if there were a 'from' command, b will point to\n+\t\t\t * 'from' commit, because parse_from stores it there.\n+\t\t\t */\n+\t\t\tdie(\"Can't merge a branch with itself: %s\", b->name);\n+\t\telse if (s)\n \t\t\thashcpy(n->sha1, s->sha1);\n \t\telse if (*from == ':') {\n \t\t\tuintmax_t idnum = parse_mark_ref_eol(from);\n@@ -2686,7 +2692,7 @@ static void parse_new_commit(void)\n \tparse_data(&msg, 0, NULL);\n \tread_next_command();\n \tparse_from(b);\n-\tmerge_list = parse_merge(&merge_count);\n+\tmerge_list = parse_merge(&merge_count, b);\n \n \t/* ensure the branch is active/loaded */\n \tif (!b->branch_tree.tree || !max_active_branches) {\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 6f4c988..79cb72a 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -872,6 +872,19 @@ test_expect_success \\\n \t'git fast-import input < $GIT_COMMITTER_DATE\n+data < \"merge $itself\" may be used to create commits with previous branch tip\n> being repeated as n-th parent or even moved from being 1-st to be just\n> n-th. This is not a documented use case and doesn't look like a common\n> one.\n[...]\n> Just deny \"merge $itself\" for now. It was a bit broken and btw \"from\n> $itself\" was and is a forbidden command too.\n\nLovely. Thanks for a clear patch and clear explanation.\n\nFor what it's worth, this one is\nAcked-by: Jonathan Nieder \n\nI'll think more about the other two and get back to you.\n"},{"id":"194383","messageId":"20120627212531.GN12774@burratino","threadId":"30918","inReplyTo":"1340818825-13754-2-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-27T21:25:31Z","receivedAt":"2012-06-27T21:25:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n> null_sha1 is used in fast-import to indicate \"empty\" branches and\n> should never be actually written out as a commit parent. 'merge'\n> command lacks is_null_sha1 checks and must be fixed.\n\nYeah.\n\n> It looks like using null_sha1 or empty branches in 'from' command\n> is legal and/or an intended option (it has been here from the very\n> beginning and survived). So leave it allowed for 'merge' command too,\n> and just like with 'from' command silently skip null_sha1 parents.\n\nOk, fair enough. Are there any tests in the test script for the\n\"create new branch from unborn branch\" trick? Is this worth\ndocumenting so other backend authors know what they need to do to\nsupport frontends that work with git fast-import?\n\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2734,7 +2734,8 @@ static void parse_new_commit(void)\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> -\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(merge_list->sha1));\n> +\t\tif (!is_null_sha1(merge_list->sha1))\n> +\t\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(merge_list->sha1));\n\nAcked-by: Jonathan Nieder \n"},{"id":"194385","messageId":"20120627213355.GO12774@burratino","threadId":"30918","inReplyTo":"1340818825-13754-3-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH v2 2/3] fast-import: allow \"merge $null_sha1\" command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-27T21:33:55Z","receivedAt":"2012-06-27T21:33:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n> \"from $null_sha1\" and \"merge $empty_branch\" are already allowed so\n> allow \"merge $null_sha1\" command too.\n\nThe reader might not realize that null_sha1 means\n0000000000000000000000000000000000000000 until she reads the test\nscript. Is it possible to help her save time?\n\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2631,12 +2631,14 @@ static struct hash_list *parse_merge(unsigned int *count)\n> \t\t\t\tdie(\"Mark :%\" PRIuMAX \" not a commit\", idnum);\n> \t\t\thashcpy(n->sha1, oe->idx.sha1);\n> \t\t} else if (!get_sha1(from, n->sha1)) {\n> -\t\t\tunsigned long size;\n> -\t\t\tchar *buf = read_object_with_reference(n->sha1,\n> -\t\t\t\tcommit_type, &size, n->sha1);\n> -\t\t\tif (!buf || size < 46)\n> -\t\t\t\tdie(\"Not a valid commit: %s\", from);\n> -\t\t\tfree(buf);\n> +\t\t\tif (!is_null_sha1(n->sha1)) {\n> +\t\t\t\tunsigned long size;\n> +\t\t\t\tchar *buf = read_object_with_reference(n->sha1,\n> +\t\t\t\t\tcommit_type, &size, n->sha1);\n> +\t\t\t\tif (!buf || size < 46)\n> +\t\t\t\t\tdie(\"Not a valid commit: %s\", from);\n> +\t\t\t\tfree(buf);\n> +\t\t\t}\n\nHm, ok. Maybe the \"peel onion\" call guarded by this \"if\" could be a\nseparate function to make this cleaner (and avoid some duplication of\ncode with other functions while at it)?\n\ne.g.,\n\n\tstatic int peel_to_commit(unsigned char sha1[20])\n\t{\n\t\tunsigned long size;\n\n\t\tchar *buf = read_object_with_reference(...);\n\t\tif (!buf)\n\t\t\treturn -1;\n\t\tfree(buf);\n\t\tif (size < strlen(\"commit \") + 40)\n\t\t\treturn -1;\n\t\treturn 0;\n\t}\n\n\t...\n\t\tif (is_null_sha1(n->sha1))\n\t\t\t; /* ok */\n\t\telse if (peel_to_commit(n->sha1))\n\t\t\tdie(\"Not a valid commit: %s\", from);\n\nI like the direction, but as it is, this patch feels kind of \"meh\" to\nme.\n\nThanks again and hope that helps,\nJonathan\n"},{"id":"194393","messageId":"7v395g75gg.fsf@alter.siamese.dyndns.org","threadId":"30918","inReplyTo":"1340818825-13754-3-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH v2 2/3] fast-import: allow \"merge $null_sha1\" command","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-06-27T22:30:55Z","receivedAt":"2012-06-27T22:30:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Dmitry Ivankov writes:\n\n> \"from $null_sha1\" and \"merge $empty_branch\" are already allowed so\n> allow \"merge $null_sha1\" command too.\n\nWould accepting such a \"merge oops-do-not-do-anything\" allow\nexporters' job to be simpler?\n\nWithout a convincing \"it makes sense to treat this nonsense request\nas a no-op\" argument, I fail to see why this is a change in the\nright direction. If there are two other nonsense request that\nsilently become no-op, shouldn't they be diagnosed as bugs in the\ninput stream, or do these two have valid uses?\n\nVery confused.\n"},{"id":"194396","messageId":"20120627233931.GA3014@burratino","threadId":"30918","inReplyTo":"7v395g75gg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/3] fast-import: allow \"merge $null_sha1\" command","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-06-27T23:39:31Z","receivedAt":"2012-06-27T23:39:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Dmitry Ivankov writes:\n\n>> \"from $null_sha1\" and \"merge $empty_branch\" are already allowed so\n>> allow \"merge $null_sha1\" command too.\n>\n> Would accepting such a \"merge oops-do-not-do-anything\" allow\n> exporters' job to be simpler?\n\nGood question.\n\nI was uncomfortable with the patch and couldn't pin down why and I\nthink you've hit it.\n\nI can imagine an importer that does\n\n\tcat <> Dmitry Ivankov writes:\n\n>>> \"from $null_sha1\" and \"merge $empty_branch\" are already allowed so\n>>> allow \"merge $null_sha1\" command too.\n>>\n>> Would accepting such a \"merge oops-do-not-do-anything\" allow\n>> exporters' job to be simpler?\n>\n> Good question.\n\nI think this patch series had some good parts and I would like to pass\nthem to Junio when they're ready.\n\nCan you send me the current version of the patches and remind me of\ntheir status and what's left to be done?\n\nThanks much,\nJonathan\n"},{"id":"195650","messageId":"20120724193040.GC5210@burratino","threadId":"30918","inReplyTo":"1340818825-13754-2-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-07-24T19:30:40Z","receivedAt":"2012-07-24T19:30:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nIn June, Dmitry Ivankov wrote:\n\n> null_sha1 is used in fast-import to indicate \"empty\" branches and\n> should never be actually written out as a commit parent. 'merge'\n> command lacks is_null_sha1 checks and must be fixed.\n>\n> It looks like using null_sha1 or empty branches in 'from' command\n> is legal and/or an intended option (it has been here from the very\n> beginning and survived). So leave it allowed for 'merge' command too,\n> and just like with 'from' command silently skip null_sha1 parents.\n\nAs Junio mentioned, this might have just been an implementation\naccident --- without a use case in mind, it is hard to say that\nsupport for the 'from 0{40}' was really intended to be part of the\nsupported fast-import syntax.\n\nOn the other hand it seems possible and even likely that some frontend\nhas taken advantage of the feature to avoid having to use conditional\nlogic to decide whether to emit a \"from\" command, since it has been\naround so long. So you are right that it's safest not to remove it.\n\nThat means that adding the same support for the \"merge\" command could\nbe a pretty bad thing, since it would be making a new promise of\ncontinued support and would place a new burden on other implementers\nof backends.\n\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2734,7 +2734,8 @@ static void parse_new_commit(void)\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> -\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(merge_list->sha1));\n> +\t\tif (!is_null_sha1(merge_list->sha1))\n> +\t\t\tstrbuf_addf(&new_data, \"parent %s\\n\", sha1_to_hex(merge_list->sha1));\n\nSince these \"merge\" commands produced invalid results in the past,\nwould it be safe to do\n\n\t\tif (is_null_sha1(merge_list->sha1))\n\t\t\tdie(\"cannot use unborn branch or all-zeroes hash as merge parent\";\n\ninstead?\n\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -850,6 +850,27 @@ INPUT_END\n> test_expect_success \\\n> \t'J: tag must fail on empty branch' \\\n> \t'test_must_fail git fast-import +\n> +cat >input < +reset refs/heads/J3\n> +\n> +reset refs/heads/J4\n> +from 0000000000000000000000000000000000000000\n> +\n> +commit refs/heads/J5\n> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +data < +Merge J3, J4 into fresh J5.\n> +COMMIT\n> +merge refs/heads/J3\n> +merge refs/heads/J4\n> +\n> +INPUT_END\n> +test_expect_success \\\n> +\t'J: allow merge with empty branch' \\\n> +\t'git fast-import +\tgit rev-parse --verify J5 &&\n> +\ttest_must_fail git rev-parse --verify J5^'\n\nThanks for the test --- in any case, we should test the behavior. How\nabout this, for now?\n\n-- >8 --\nFrom: Dmitry Ivankov \nSubject: test: demonstrate fast-import bug that produces invalid commits with null parent\n\nnull_sha1 is used in fast-import to indicate \"empty\" branches and\nshould never be actually written out as a commit parent. 'merge'\ncommand lacks is_null_sha1 checks and must be fixed.\n\n[jn: extracted from a patch with a proposed fix; split into two tests]\n\nSigned-off-by: Dmitry Ivankov \nSigned-off-by: Jonathan Nieder \n---\n t/t9300-fast-import.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 2fcf2694..f13b85b8 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -850,6 +850,42 @@ INPUT_END\n test_expect_success \\\n \t'J: tag must fail on empty branch' \\\n \t'test_must_fail git fast-import input < $GIT_COMMITTER_DATE\n+data <input < $GIT_COMMITTER_DATE\n+data < In presence of \"from $some\" command \"merge $itself\" acts the same as\n> \"merge $some\" would. Which is completely undocumented and looks like\n> a bug (caused by parse_from() temporarily rewriting b->sha1 with $some).\n\nCould you give an example?\n\n> Just deny \"merge $itself\" for now. It was a bit broken and btw \"from\n> $itself\" was and is a forbidden command too.\n>\n> Signed-off-by: Dmitry Ivankov \n\nYes, this one still looks good.\n\n[...]\n> --- a/fast-import.c\n> +++ b/fast-import.c\n> @@ -2611,7 +2611,7 @@ static int parse_from(struct branch *b)\n> \treturn 1;\n> }\n> \n> -static struct hash_list *parse_merge(unsigned int *count)\n> +static struct hash_list *parse_merge(unsigned int *count, struct branch *b)\n> {\n> \tstruct hash_list *list = NULL, *n, *e = e;\n> \tconst char *from;\n> @@ -2622,7 +2622,13 @@ 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 (b == s)\n\nStyle: \"if (s == b)\" would make it clearer that b is known (the current\nbranch) and s unknown. Giving the 'b' parameter a meaningful name\nlike 'this_branch' would help even more.\n\n> +\t\t\t/*\n> +\t\t\t * Also if there were a 'from' command, b will point to\n> +\t\t\t * 'from' commit, because parse_from stores it there.\n> +\t\t\t */\n> +\t\t\tdie(\"Can't merge a branch with itself: %s\", b->name);\n\nIt's not clear to me what the \"Also\" is referring to here. How\nabout:\n\n\t\t\t/*\n\t\t\t * If there was a 'from' command, b->sha1 refers to\n\t\t\t * that commit instead of the previous commit on the\n\t\t\t * current branch, which is probably what no one\n\t\t\t * expected.\n\t\t\t *\n\t\t\t * Let's just reject attempts to merge a branch into\n\t\t\t * itself.\n\t\t\t */\n\t\t\tdie(\"Can't merge a ...\");\n\n[...]\n> --- a/t/t9300-fast-import.sh\n> +++ b/t/t9300-fast-import.sh\n> @@ -871,6 +871,19 @@ test_expect_success \\\n> \t'git fast-import \tgit rev-parse --verify J5 &&\n> \ttest_must_fail git rev-parse --verify J5^'\n> +\n> +cat >input < +commit refs/heads/J5\n> +committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n> +data < +Merge J5 with itself.\n> +COMMIT\n> +merge refs/heads/J5\n> +\n> +INPUT_END\n> +test_expect_success \\\n> +\t'J: disallow merge with itself' \\\n> +\t'test_must_fail git fast-import