[PATCH/RFC v2 0/3] fast-import: disallow empty branches as parents

This is a rewrite of [1].

First of all the patch is split into several parts now.

1/3 prevents writing invalid commit objects (with null_sha1 parents)

For 2/3 and 3/3 I've changed my mind almost completely. The commands
in question are:
A 'from null_sha1'
B 'from empty_branch'
C 'from itself'
D 'merge null_sha1'
E 'merge empty_branch'
F 'merge itself'

Currently C is disallowed, D and E lead to 1/3 bug, F looks broken, A and B are allowed.

In [1] I kept A allowed, but made B, C, D, E disallowed and "fixed" F.
The idea was too keep A as legacy, fix F as it may have applications and disallow others
as they look like errors in import stream.

This time I keep A and B allowed, allow D and E, disallow F.
Now I think of null_sha1 as of a special feature, empty_branch things as a mix of legacy
and this feature (one can 'reset' branch to null_sha1, then use it's name and expect it
to work as if null_sha1 was used, and null_sha1 is allowed). "Fix" for F is dropped for
now and will later go separately with it's own set of tests and a new discussion I guess.

[1] http://thread.gmane.org/gmane.comp.version-control.git/200339

Dmitry Ivankov (3):
  fast-import: do not write null_sha1 as a merge parent
  fast-import: allow "merge $null_sha1" command
  fast-import: disallow "merge $itself" command

 fast-import.c          |   29 +++++++++++++++++++----------
 t/t9300-fast-import.sh |   35 +++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 10 deletions(-)

-- 
1.7.3.4

null_sha1 is used in fast-import to indicate "empty" branches and
should never be actually written out as a commit parent. 'merge'
command lacks is_null_sha1 checks and must be fixed.

It looks like using null_sha1 or empty branches in 'from' command
is legal and/or an intended option (it has been here from the very
beginning and survived). So leave it allowed for 'merge' command too,
and just like with 'from' command silently skip null_sha1 parents.

Add a simple test for null_sha1 merge parents.

Signed-off-by: Dmitry Ivankov <redacted>
---
 fast-import.c          |    3 ++-
 t/t9300-fast-import.sh |   21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index eed97c8..419e435 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2734,7 +2734,8 @@ static void parse_new_commit(void)
 		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(b->sha1));
 	while (merge_list) {
 		struct hash_list *next = merge_list->next;
-		strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));
+		if (!is_null_sha1(merge_list->sha1))
+			strbuf_addf(&new_data, "parent %s\n", sha1_to_hex(merge_list->sha1));
 		free(merge_list);
 		merge_list = next;
 	}

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index c17f52e..5716420 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh

@@ -850,6 +850,27 @@ INPUT_END
 test_expect_success \
 	'J: tag must fail on empty branch' \
 	'test_must_fail git fast-import <input'
+
+cat >input <<INPUT_END
+reset refs/heads/J3
+
+reset refs/heads/J4
+from 0000000000000000000000000000000000000000
+
+commit refs/heads/J5
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J3, J4 into fresh J5.
+COMMIT
+merge refs/heads/J3
+merge refs/heads/J4
+
+INPUT_END
+test_expect_success \
+	'J: allow merge with empty branch' \
+	'git fast-import <input &&
+	git rev-parse --verify J5 &&
+	test_must_fail git rev-parse --verify J5^'
 ###
 ### series K
 ###

-- 
1.7.3.4

"merge $itself" may be used to create commits with previous branch tip
being repeated as n-th parent or even moved from being 1-st to be just
n-th. This is not a documented use case and doesn't look like a common
one.

In presence of "from $some" command "merge $itself" acts the same as
"merge $some" would. Which is completely undocumented and looks like
a bug (caused by parse_from() temporarily rewriting b->sha1 with $some).

Just deny "merge $itself" for now. It was a bit broken and btw "from
$itself" was and is a forbidden command too.

Signed-off-by: Dmitry Ivankov <redacted>
---
 fast-import.c          |   12 +++++++++---
 t/t9300-fast-import.sh |   13 +++++++++++++
 2 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index f03da1e..781c614 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2611,7 +2611,7 @@ static int parse_from(struct branch *b)
 	return 1;
 }
 
-static struct hash_list *parse_merge(unsigned int *count)
+static struct hash_list *parse_merge(unsigned int *count, struct branch *b)
 {
 	struct hash_list *list = NULL, *n, *e = e;
 	const char *from;

@@ -2622,7 +2622,13 @@ 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 (b == s)
+			/*
+			 * Also if there were a 'from' command, b will point to
+			 * 'from' commit, because parse_from stores it there.
+			 */
+			die("Can't merge a branch with itself: %s", b->name);
+		else if (s)
 			hashcpy(n->sha1, s->sha1);
 		else if (*from == ':') {
 			uintmax_t idnum = parse_mark_ref_eol(from);

@@ -2686,7 +2692,7 @@ static void parse_new_commit(void)
 	parse_data(&msg, 0, NULL);
 	read_next_command();
 	parse_from(b);
-	merge_list = parse_merge(&merge_count);
+	merge_list = parse_merge(&merge_count, b);
 
 	/* ensure the branch is active/loaded */
 	if (!b->branch_tree.tree || !max_active_branches) {

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 6f4c988..79cb72a 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh

@@ -872,6 +872,19 @@ test_expect_success \
 	'git fast-import <input &&
 	git rev-parse --verify J5 &&
 	test_must_fail git rev-parse --verify J5^'
+
+cat >input <<INPUT_END
+commit refs/heads/J5
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J5 with itself.
+COMMIT
+merge refs/heads/J5
+
+INPUT_END
+test_expect_success \
+	'J: disallow merge with itself' \
+	'test_must_fail git fast-import <input'
 ###
 ### series K
 ###

-- 
1.7.3.4

"from $null_sha1" and "merge $empty_branch" are already allowed so
allow "merge $null_sha1" command too.

However such 'merge' has no effect on the import. It's made allowed
just to unify null_sha1 commits handling a little bit.

Signed-off-by: Dmitry Ivankov <redacted>
---
 fast-import.c          |   14 ++++++++------
 t/t9300-fast-import.sh |    1 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 419e435..f03da1e 100644
--- a/fast-import.c
+++ b/fast-import.c

@@ -2631,12 +2631,14 @@ static struct hash_list *parse_merge(unsigned int *count)
 				die("Mark :%" PRIuMAX " not a commit", idnum);
 			hashcpy(n->sha1, oe->idx.sha1);
 		} else if (!get_sha1(from, n->sha1)) {
-			unsigned long size;
-			char *buf = read_object_with_reference(n->sha1,
-				commit_type, &size, n->sha1);
-			if (!buf || size < 46)
-				die("Not a valid commit: %s", from);
-			free(buf);
+			if (!is_null_sha1(n->sha1)) {
+				unsigned long size;
+				char *buf = read_object_with_reference(n->sha1,
+					commit_type, &size, n->sha1);
+				if (!buf || size < 46)
+					die("Not a valid commit: %s", from);
+				free(buf);
+			}
 		} else
 			die("Invalid ref name or SHA1 expression: %s", from);
 

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 5716420..6f4c988 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh

@@ -864,6 +864,7 @@ Merge J3, J4 into fresh J5.
 COMMIT
 merge refs/heads/J3
 merge refs/heads/J4
+merge 0000000000000000000000000000000000000000
 
 INPUT_END
 test_expect_success \

-- 
1.7.3.4

Dmitry Ivankov wrote:

"merge $itself" may be used to create commits with previous branch tip
being repeated as n-th parent or even moved from being 1-st to be just
n-th. This is not a documented use case and doesn't look like a common
one.

[...]

Just deny "merge $itself" for now. It was a bit broken and btw "from
$itself" was and is a forbidden command too.

Lovely.  Thanks for a clear patch and clear explanation.

For what it's worth, this one is
Acked-by: Jonathan Nieder <redacted>

I'll think more about the other two and get back to you.

Dmitry Ivankov wrote:

null_sha1 is used in fast-import to indicate "empty" branches and
should never be actually written out as a commit parent. 'merge'
command lacks is_null_sha1 checks and must be fixed.

Yeah.

It looks like using null_sha1 or empty branches in 'from' command
is legal and/or an intended option (it has been here from the very
beginning and survived). So leave it allowed for 'merge' command too,
and just like with 'from' command silently skip null_sha1 parents.

Ok, fair enough.  Are there any tests in the test script for the
"create new branch from unborn branch" trick?  Is this worth
documenting so other backend authors know what they need to do to
support frontends that work with git fast-import?

[...]

Acked-by: Jonathan Nieder <redacted>

Dmitry Ivankov wrote:

"from $null_sha1" and "merge $empty_branch" are already allowed so
allow "merge $null_sha1" command too.

The reader might not realize that null_sha1 means
0000000000000000000000000000000000000000 until she reads the test
script.  Is it possible to help her save time?

[...]

Hm, ok.  Maybe the "peel onion" call guarded by this "if" could be a
separate function to make this cleaner (and avoid some duplication of
code with other functions while at it)?

e.g.,

	static int peel_to_commit(unsigned char sha1[20])
	{
		unsigned long size;

		char *buf = read_object_with_reference(...);
		if (!buf)
			return -1;
		free(buf);
		if (size < strlen("commit ") + 40)
			return -1;
		return 0;
	}

	...
		if (is_null_sha1(n->sha1))
			; /* ok */
		else if (peel_to_commit(n->sha1))
			die("Not a valid commit: %s", from);

I like the direction, but as it is, this patch feels kind of "meh" to
me.

Thanks again and hope that helps,
Jonathan

Hi,

In June, Dmitry Ivankov wrote:

null_sha1 is used in fast-import to indicate "empty" branches and
should never be actually written out as a commit parent. 'merge'
command lacks is_null_sha1 checks and must be fixed.

It looks like using null_sha1 or empty branches in 'from' command
is legal and/or an intended option (it has been here from the very
beginning and survived). So leave it allowed for 'merge' command too,
and just like with 'from' command silently skip null_sha1 parents.

As Junio mentioned, this might have just been an implementation
accident --- without a use case in mind, it is hard to say that
support for the 'from 0{40}' was really intended to be part of the
supported fast-import syntax.

On the other hand it seems possible and even likely that some frontend
has taken advantage of the feature to avoid having to use conditional
logic to decide whether to emit a "from" command, since it has been
around so long.  So you are right that it's safest not to remove it.

That means that adding the same support for the "merge" command could
be a pretty bad thing, since it would be making a new promise of
continued support and would place a new burden on other implementers
of backends.

[...]

Since these "merge" commands produced invalid results in the past,
would it be safe to do

		if (is_null_sha1(merge_list->sha1))
			die("cannot use unborn branch or all-zeroes hash as merge parent";

instead?

Thanks for the test --- in any case, we should test the behavior.  How
about this, for now?

-- >8 --
From: Dmitry Ivankov <redacted>
Subject: test: demonstrate fast-import bug that produces invalid commits with null parent

null_sha1 is used in fast-import to indicate "empty" branches and
should never be actually written out as a commit parent. 'merge'
command lacks is_null_sha1 checks and must be fixed.

[jn: extracted from a patch with a proposed fix; split into two tests]

Signed-off-by: Dmitry Ivankov <redacted>
Signed-off-by: Jonathan Nieder <redacted>
---
 t/t9300-fast-import.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2fcf2694..f13b85b8 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh

@@ -850,6 +850,42 @@ INPUT_END
 test_expect_success \
 	'J: tag must fail on empty branch' \
 	'test_must_fail git fast-import <input'
+
+cat >input <<INPUT_END
+reset refs/heads/J-unborn
+
+commit refs/heads/J-merge-unborn
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J-unborn into fresh J-merge-unborn.
+COMMIT
+merge refs/heads/J-unborn
+
+INPUT_END
+test_expect_failure \
+	'J: reject or ignore merge with unborn branch' \
+	'test_when_finished "git update-ref -d refs/heads/J-merge-unborn" &&
+	 test_might_fail git fast-import <input &&
+	 git fsck'
+
+cat >input <<INPUT_END
+reset refs/heads/J-null-sha1
+from 0000000000000000000000000000000000000000
+
+commit refs/heads/J-merge-null
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+Merge J-null-sha1 into fresh J-merge-null.
+COMMIT
+merge refs/heads/J-null-sha1
+
+INPUT_END
+test_expect_failure \
+	'J: reject or ignore merge with unborn branch' \
+	'test_when_finished "git update-ref -d refs/heads/J-merge-null" &&
+	 test_might_fail git fast-import <input &&
+	 git fsck'
+
 ###
 ### series K
 ###

-- 
1.7.10.4

Hi,

In June, Dmitry Ivankov wrote:

In presence of "from $some" command "merge $itself" acts the same as
"merge $some" would. Which is completely undocumented and looks like
a bug (caused by parse_from() temporarily rewriting b->sha1 with $some).

Could you give an example?

Just deny "merge $itself" for now. It was a bit broken and btw "from
$itself" was and is a forbidden command too.

Signed-off-by: Dmitry Ivankov <redacted>

Yes, this one still looks good.

[...]

Style: "if (s == b)" would make it clearer that b is known (the current
branch) and s unknown.  Giving the 'b' parameter a meaningful name
like 'this_branch' would help even more.

+			/*
+			 * Also if there were a 'from' command, b will point to
+			 * 'from' commit, because parse_from stores it there.
+			 */
+			die("Can't merge a branch with itself: %s", b->name);

It's not clear to me what the "Also" is referring to here.  How
about:

			/*
			 * If there was a 'from' command, b->sha1 refers to
			 * that commit instead of the previous commit on the
			 * current branch, which is probably what no one
			 * expected.
			 *
			 * Let's just reject attempts to merge a branch into
			 * itself.
			 */
			die("Can't merge a ...");

[...]

Looks sensible.

If the changes suggested above look good to you, I can amend locally.
Otherwise, I'll be happy to see what you come up with next.

Thanks,
Jonathan