[PATCH/RFC 0/2] fast-import: commit from null_sha1

[PATCH/RFC 0/2] fast-import: commit from null_sha1

[Dmitry Ivankov]

Not so sure how null_sha1 parent should be treated in fast-import.
Absent parent is represented as null_sha1 to the user in reflog,
but isn't allowed as an argument for porcelain nor shows in most
plumbing commands afaik.

These patches make fast-import treat
    commit refs/heads/master
    ...
    from `null_sha1`
like any other missing parent sha1 - reject such input.

Note: if we'll want this input to be valid, some other adjustments
to fast-import logic may be needed for consistency.

Dmitry Ivankov (2):
  fast-import: add 'commit from 0{40}' failing test
  fast-import: fix 'from 0{40}' test

 fast-import.c          |   17 ++++++-----------
 t/t9300-fast-import.sh |   12 ++++++++++++
 2 files changed, 18 insertions(+), 11 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] fast-import: add 'commit from 0{40}' failing test

[Dmitry Ivankov]

Following shouldn't be allowed, while it is:

commit refs/heads/some
committer ...
data ...
from `null_sha1`

It is treated as if 'from' was omitted. But it is allowed to just
omit 'from' actually. And `null_sha1` being special in fast-import
is an internal implementation detail.

Add a test as described.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 t/t9300-fast-import.sh |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 1a6c066..8cc3f16 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -375,6 +375,18 @@ test_expect_success 'B: fail on invalid branch name "bad[branch]name"' '
 rm -f .git/objects/pack_* .git/objects/index_*
 
 cat >input <<INPUT_END
+commit refs/heads/zeromaster
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data 0
+
+from 0000000000000000000000000000000000000000
+INPUT_END
+test_expect_failure 'B: fail on "from 0{40}"' '
+    test_must_fail git fast-import <input
+'
+rm -f .git/objects/pack_* .git/objects/index_*
+
+cat >input <<INPUT_END
 commit TEMP_TAG
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <<COMMIT
-- 
1.7.3.4

[PATCH 2/2] fast-import: fix 'from 0{40}' test

[Dmitry Ivankov]

parse_from_existing() has a special case for null_sha1 treating it
as a start of an orphaned branch. It is how null_sha1 parent is
treated in fast-import. For example parse_reset_branch() clears
sha1 of a branch but leaves it in a lookup table.

Looking at parse_from_existing() call sites, we can seen that it is
only called for sha1's that come from get_sha1() or an existing
object. So fast-import internals don't give it null_sha1 explicitly
and the only way for it to appear is direct '0{40}' in the input.

Don't treat null_sha1 as a magic sha1 in parse_from_existing thus
making 'from 0{40}' an invalid input. (Unless there is a commit
object having null_sha1, of course. And object with null_sha1 would
be a lot of trouble in fast-import regardless of this patch.)

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |   17 ++++++-----------
 t/t9300-fast-import.sh |    2 +-
 2 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 742e7da..827434a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2488,18 +2488,13 @@ static void parse_from_commit(struct branch *b, char *buf, unsigned long size)
 
 static void parse_from_existing(struct branch *b)
 {
-	if (is_null_sha1(b->sha1)) {
-		hashclr(b->branch_tree.versions[0].sha1);
-		hashclr(b->branch_tree.versions[1].sha1);
-	} else {
-		unsigned long size;
-		char *buf;
+	unsigned long size;
+	char *buf;
 
-		buf = read_object_with_reference(b->sha1,
-			commit_type, &size, b->sha1);
-		parse_from_commit(b, buf, size);
-		free(buf);
-	}
+	buf = read_object_with_reference(b->sha1,
+		commit_type, &size, b->sha1);
+	parse_from_commit(b, buf, size);
+	free(buf);
 }
 
 static int parse_from(struct branch *b)
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 8cc3f16..0784d50 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -381,7 +381,7 @@ data 0
 
 from 0000000000000000000000000000000000000000
 INPUT_END
-test_expect_failure 'B: fail on "from 0{40}"' '
+test_expect_success 'B: fail on "from 0{40}"' '
     test_must_fail git fast-import <input
 '
 rm -f .git/objects/pack_* .git/objects/index_*
-- 
1.7.3.4

Re: [PATCH/RFC 0/2] fast-import: commit from null_sha1

[Jonathan Nieder]

Dmitry Ivankov wrote:

> These patches make fast-import treat
>     commit refs/heads/master
>     ...
>     from `null_sha1`
> like any other missing parent sha1 - reject such input.

Are you sure the existing support for "from 0{40}" is not deliberate
and that no one is relying on it?  If and only if you are, then this
seems like a good idea (a single patch that both makes the behavior
change and adds a test for it should be easier to review).

Re: [PATCH/RFC 0/2] fast-import: commit from null_sha1

[Dmitry Ivankov]

On Mon, Sep 19, 2011 at 3:30 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Dmitry Ivankov wrote:
>
>> These patches make fast-import treat
>>     commit refs/heads/master
>>     ...
>>     from `null_sha1`
>> like any other missing parent sha1 - reject such input.
>
> Are you sure the existing support for "from 0{40}" is not deliberate
> and that no one is relying on it?
It is hard to guess. There is no test for it in t/t9300-fast-import.sh, no
mention in the Documentation, but sometimes a user can see null_sha1
from git. I hope that it pops up only when something is read to simplify
the format and never accepted in 'write' commands or as an argument.

>  If and only if you are, then this
> seems like a good idea (a single patch that both makes the behavior
> change and adds a test for it should be easier to review).
>