Re: [PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent

[Jonathan Nieder <jrnieder@gmail.com>]

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.

[...]
> --- 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));

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?

> --- 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^'

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

-- >8 --
From: Dmitry Ivankov <divanorama@gmail.com>
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 <divanorama@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 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