git/list[1] front-page[2] threads[3] people[4] search[5] about
 

[PATCH] fast-import: disallow empty branches as parents

From
Dmitry Ivankov <divanorama@gmail.com>
Date
Jun 20, 2012, 19:34 UTC
Message-ID
<1340220841-753-2-git-send-email-divanorama@gmail.com>
In-Reply-To
<1340220841-753-1-git-send-email-divanorama@gmail.com>

Empty branches (either new or reset-ed) have null_sha1 in fast-import internals. These null_sha1 heads can slip to the real commit objects.

- parse_merge() had no check against null_sha1, so add it.
- parse_from() didn't have it too. It doesn't cause null_sha1 to slip
  because for the first parent there is a null_sha1 check. But still
  the input with "from empty_branch" must be rejected, make it so via
  adding the check to parse_from().

There is a special case with "merge branch_itself" command. When the branch_itself is empty it must clearly be rejected. Though in a presence of "from ..." command it is not. parse_from() writes the new first parent sha1 to branch->sha1 as a temporary placeholder, which parse_merge() then picks up. Though this sha1 might have never been a tip of the branch_itself at all.

Make parse_from() store the first parent sha1 in a temporary buffer to fix this special case.

Add some tests for all these fixes.
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |   49 +++++++++++++++++++++++++++++------------------
 t/t9300-fast-import.sh |   48 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 19 deletions(-)
diff --git a/fast-import.c b/fast-import.c
index eed97c8..2e089a8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2540,7 +2540,8 @@ static void file_change_deleteall(struct branch *b)
 	b->num_notes = 0;
 }
 
-static void parse_from_commit(struct branch *b, char *buf, unsigned long size)
+static void parse_from_commit(struct branch *b, unsigned char *sha1,
+						char *buf, unsigned long size)
 {
 	if (!buf || size < 46)
 		die("Not a valid commit: %s", sha1_to_hex(b->sha1));
@@ -2551,29 +2552,31 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)
 		b->branch_tree.versions[1].sha1);
 }
 
-static void parse_from_existing(struct branch *b)
+static void parse_from_existing(struct branch *b, unsigned char *sha1)
 {
-	if (is_null_sha1(b->sha1)) {
+	if (is_null_sha1(sha1)) {
 		hashclr(b->branch_tree.versions[0].sha1);
 		hashclr(b->branch_tree.versions[1].sha1);
 	} else {
 		unsigned long size;
 		char *buf;
 
-		buf = read_object_with_reference(b->sha1,
-			commit_type, &size, b->sha1);
-		parse_from_commit(b, buf, size);
+		buf = read_object_with_reference(sha1,
+			commit_type, &size, sha1);
+		parse_from_commit(b, sha1, buf, size);
 		free(buf);
 	}
 }
 
-static int parse_from(struct branch *b)
+static int parse_from(struct branch *b, unsigned char *sha1out)
 {
 	const char *from;
 	struct branch *s;
 
-	if (prefixcmp(command_buf.buf, "from "))
+	if (prefixcmp(command_buf.buf, "from ")) {
+		hashclr(sha1out);
 		return 0;
+	}
 
 	if (b->branch_tree.tree) {
 		release_tree_content_recursive(b->branch_tree.tree);
@@ -2586,7 +2589,10 @@ static int parse_from(struct branch *b)
 		die("Can't create a branch from itself: %s", b->name);
 	else if (s) {
 		unsigned char *t = s->branch_tree.versions[1].sha1;
-		hashcpy(b->sha1, s->sha1);
+		if (is_null_sha1(s->sha1))
+			die("Can't create a branch from an empty branch:"
+				" %s from %s", b->name, s->name);
+		hashcpy(sha1out, s->sha1);
 		hashcpy(b->branch_tree.versions[0].sha1, t);
 		hashcpy(b->branch_tree.versions[1].sha1, t);
 	} else if (*from == ':') {
@@ -2594,16 +2600,16 @@ static int parse_from(struct branch *b)
 		struct object_entry *oe = find_mark(idnum);
 		if (oe->type != OBJ_COMMIT)
 			die("Mark :%" PRIuMAX " not a commit", idnum);
-		hashcpy(b->sha1, oe->idx.sha1);
+		hashcpy(sha1out, oe->idx.sha1);
 		if (oe->pack_id != MAX_PACK_ID) {
 			unsigned long size;
 			char *buf = gfi_unpack_entry(oe, &size);
-			parse_from_commit(b, buf, size);
+			parse_from_commit(b, sha1out, buf, size);
 			free(buf);
 		} else
-			parse_from_existing(b);
-	} else if (!get_sha1(from, b->sha1))
-		parse_from_existing(b);
+			parse_from_existing(b, sha1out);
+	} else if (!get_sha1(from, sha1out))
+		parse_from_existing(b, sha1out);
 	else
 		die("Invalid ref name or SHA1 expression: %s", from);
 
@@ -2622,9 +2628,11 @@ static struct hash_list *parse_merge(unsigned int *count)
 		from = strchr(command_buf.buf, ' ') + 1;
 		n = xmalloc(sizeof(*n));
 		s = lookup_branch(from);
-		if (s)
+		if (s) {
+			if (is_null_sha1(s->sha1))
+				die("Can't merge empty branch: %s", s->name);
 			hashcpy(n->sha1, s->sha1);
-		else if (*from == ':') {
+		} else if (*from == ':') {
 			uintmax_t idnum = parse_mark_ref_eol(from);
 			struct object_entry *oe = find_mark(idnum);
 			if (oe->type != OBJ_COMMIT)
@@ -2656,6 +2664,7 @@ static void parse_new_commit(void)
 {
 	static struct strbuf msg = STRBUF_INIT;
 	struct branch *b;
+	unsigned char sha1[20];
 	char *sp;
 	char *author = NULL;
 	char *committer = NULL;
@@ -2683,7 +2692,7 @@ static void parse_new_commit(void)
 		die("Expected committer but didn't get one");
 	parse_data(&msg, 0, NULL);
 	read_next_command();
-	parse_from(b);
+	parse_from(b, sha1);
 	merge_list = parse_merge(&merge_count);
 
 	/* ensure the branch is active/loaded */
@@ -2730,7 +2739,9 @@ static void parse_new_commit(void)
 	strbuf_reset(&new_data);
 	strbuf_addf(&new_data, "tree %s\n",
 		sha1_to_hex(b->branch_tree.versions[1].sha1));
-	if (!is_null_sha1(b->sha1))
+	if (!is_null_sha1(sha1))
+		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(sha1));
+	else if (!is_null_sha1(b->sha1))
 		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
 	while (merge_list) {
 		struct hash_list *next = merge_list->next;
@@ -2855,7 +2866,7 @@ static void parse_reset_branch(void)
 	else
 		b = new_branch(sp);
 	read_next_command();
-	parse_from(b);
+	parse_from(b, b->sha1);
 	if (command_buf.len > 0)
 		unread_command_buf = 1;
 }
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2aa1824..5f25c01 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -2895,6 +2895,54 @@ test_expect_success 'S: merge with garbage after mark must fail' '
 	test_i18ngrep "after mark" err
 '
 
+test_expect_success 'S: empty branch as merge parent must fail' '
+	test_must_fail git fast-import <<-EOF 2>err &&
+	commit refs/heads/chicken
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	I am the chicken.
+	COMMIT
+	merge refs/heads/chicken
+	EOF
+	cat err &&
+	test_must_fail git rev-parse --verify refs/heads/chicken^
+'
+
+test_expect_success 'S: empty branch as merge parent must fail (2)' '
+	test_must_fail git fast-import <<-EOF 2>err &&
+	commit refs/heads/egg1
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	I am the egg N1.
+	COMMIT
+
+	commit refs/heads/egg2
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	I am the egg N2.
+	COMMIT
+	from refs/heads/egg1
+	merge refs/heads/egg2
+	EOF
+	cat err &&
+	test_must_fail git rev-parse --verify refs/heads/egg2^2
+'
+
+test_expect_success 'S: empty branch as a parent must fail ' '
+	test_must_fail git fast-import <<-EOF 2>err &&
+	reset refs/heads/egg3
+
+	commit refs/heads/egg4
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	I am the egg N4.
+	COMMIT
+	from refs/heads/egg3
+	EOF
+	cat err &&
+	test_must_fail git rev-parse --verify refs/heads/egg4^2
+'
+
 #
 # tag, from markref
 #

-- 
1.7.3.4
Previous: Dmitry IvankovNext: Jonathan Nieder
Message 2 of 5 in “fast-import: disallow empty branches as parents”
  1. fast-import: disallow empty branches as parentsDmitry Ivankov, Jun 20, 2012
  2. fast-import: disallow empty branches as parentsDmitry Ivankov, Jun 20, 2012
  3. Jonathan NiederJun 21, 2012
  4. Dmitry IvankovJun 21, 2012
  5. Dmitry IvankovJun 25, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.