Re: [PATCH v2 1/3] fast-import: do not write null_sha1 as a merge parent
Hi,
In June, Dmitry Ivankov wrote:
Show 8 quoted lines
> 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.
[...]
Show 9 quoted lines
> --- 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?
Show 27 quoted lines
> --- 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(+)
Show changes to t/t9300-fast-import.sh +36 −0
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
Dmitry Ivankov
