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

+Shawn Pearce

On Thu, Jun 21, 2012 at 1:34 AM, Dmitry Ivankov [off-list ref] wrote:

Combinations of "reset", "commit" with "from" and/or "merge" commands
may make fast-import to produce bad objects (null_sha1 parents) or
accept bad inputs (ones asking for empty branches as parents).

I was trying to split the patch into small an obvious cases and found out that
there are 4 ways to make or refer to "empty" branches:
1. reset branch_name
2. reset branch_name \n from `0{40}`
3. refer to it as `0{40}`
4. commit branch_name for a new branch name. It's empty up to the
'commit' command end.
Note: I'll use 4 to denote a reference to a branch name from the
'commit' command to this very branch name.

And these branches can be used in 'from' and 'merge' commands.

In 'from' command:
- 1,2,3 are allowed in 'from', no bug of writing 0{40} parent here as
'from' parent is checked against null_sha1
- 4 is not allowed with message "Can't create a branch from itself"

In 'merge' command:
- 1,2 are allowed in 'merge' and lead to 0{40} being actually written
as a commit parent - BUG
- 3 is not allowed with message "Not a valid commit"
- 4 without 'from' is allowed, parent is previous branch tip. May be 0{40} - BUG
- 4 with 'from' is allowed, 'merge' parent is 'from' parent - probably
a bug, also may lead to 0{40} being written - BUG

BUG stuff obviously needs to be fixed, at least we can just skip 0{40}
parents in 'merge' command.

Then difference in 3 between 'from' and 'merge' should be dealt with.
'merge' behaviour  comes from
    tags/v1.5.0.4~21^2 2f6dc35d2ad0b.. 5 Mar 2007 fast-import: Fail if
a non-existant commit is used for merge
since then 'merge sha1' just looks up commit with this sha1. But
'from' command has it's story too - it was
a 'new_branch' long ago (up to tags/v1.5.0-rc4~14^2~64) and since the
very beginning (tags/v1.5.0-rc4~14^2~76)
is had a special case to allow 0{40} as a parent. So, should we just
allow 'merge 0{40}'?

And finally 4 in 'merge' should probably always refer to the previous
tip to cause less surprise (it was the largest
part of my patch, though it turns out not the most interesting one).

P.S. At first I thought that any reference to an empty branch in
'merge' of 'from' should be rejected, but given that we
allow sha1 0{40} to be used in these and in 2, I guess we should keep
it allowed.