Re: [PATCH] Branches from a vendor branch

Jon Foster wrote:
> Part of my repository history is:
> - "cvs import" a new module, onto vendor branch VENDOR-BR
> - Create a branch FOO-BR from r1.1.1.1 of all the files
> 
> cvs2svn wouldn't believe me when I told it that FOO-BR's parent was
> trunk (using a symbol strategy rule).  It insisted that the parent
> had to be VENDOR-BR, and I couldn't exclude VENDOR-BR.  A minimal
> test case is included in the attached patch.

You are working in the neighborhood of an important and annoying problem
(see issues 54 [1] and 55 [2]).  But your patch only solves a very
specific variant of the problem in an indirect way, and will not work in
the general case.  (Not that we should hold it against your change that
it doesn't solve the general problem!)

Your results were very confusing to me, so I will think out loud about
what is happening...

Running your new test case (without your patch) results in the error:

   Cannot convert the following symbol to Branch('my-branch'): The
selected parent is not among the symbol's possible parents.
       'my-branch' is a tag in 0 files, a branch in 1 files, a trivial
import in 0 files, a pure import in 0 files, and has commits in 1 files

This error comes from a consistency check that the preferred parent
requested by the user is an allowed parent for the symbol in at least
*one* RCS file.  Which it isn't in your example, because (at the time
this check is made) the branch sprouts from the 1.1.1.1 revision and not
from trunk.

The consistency check is not essential (in terms of the correctness of
the conversion) because the preferred parent for a symbol is only a
hint.  If the preferred parent is not a possible parent in a particular
file, then the branch's natural parent (i.e., the one that the branch
appears, numerically, to branch from) is used for that file.  And if the
preferred parent is *never* an allowed parent, then it's bad luck--the
symbol will be created from the natural parent in every file, making the
branch filling more complicated than necessary--but the contents of the
files on each branch will nevertheless be correct.

> The attached patch allows me to migrate this branch/module
> successfully.  When calculating parents for a branch, it checks if
> the parent revision is a vendor revision like 1.1.1.1, and if so
> then it notes trunk as a possible parent.

Adding trunk as a new possible parent has two main effects:

1. It fools the consistency check mentioned above so that it doesn't
veto trunk as the parent of the symbol in CollateSymbolsPass

2. It affects the statistics that HeuristicPreferredParentRule uses to
choose the preferred parent for the branch, possibly affecting its decision.

It does not, in itself, make trunk a *possible* parent for the file in
question.  cvs2svn wouldn't know how to graft the symbol onto trunk.

What happens in your test is that you *also* exclude the vendor branch
from the conversion.  This causes (via
CVSFileItems.graft_ntdbr_to_trunk()) the vendor branch to be grafted to
trunk, which indirectly makes trunk a possible parent of the branch that
sprouted from the vendor branch.  (In fact, it makes trunk the branch's
natural parent.)  Later, when CVSFileItems._adjust_branch_parents() is
called for this branch, it actually has nothing to do because the
branch's parent is already trunk.

If you were not also excluding the vendor branch, your change to the way
possible parent statistics are calculated would be more wrong than
right.  Because in that case trunk would in fact *not* be a possible
parent of the branch, at least in the sense that cvs2svn is not smart
enough to know how to graft the branch onto trunk.

This does not endanger the correctness of the conversion, because when
the time comes to pick a specific parent for a specific file, cvs2svn
will not consider "impossible parents".  But it could fool it into
picking trunk as the preferred parent, even though that will require
more copies than using the vendor branch as the preferred parent.

All in all, vendor branches are misused far more often than they are
used for their real purpose, and the problem that I just described is
pretty harmless.  So we could probably live with your patch.


But this general class of problems has further dimensions.  For example,
when the vendor branch is not being excluded, each NTDB revision
triggers the creation of a "post-commit" that causes the same revisions
to be committed to trunk.  Thus even when the vendor branch is not being
excluded, the trunk post-commit revision should be considered a possible
parent for a symbol that sprouts from a NTDB revision.  This makes the
resulting repository more convoluted than it needs to be, and for the
DVCSs also makes it look like some branches have multiple parents when
in fact they could be created from a single parent.

However, this problem is tricky to fix, because the post-commit revision
is not represented in the dependency graph at all, so there is no object
onto which to graft the branch.  I think that the best solution would be
to create special "post-commit" revision objects in FilterSymbolsPass,
to incorporate them in the dependency graph, to allow other symbols to
be grafted onto them, and somehow to ensure that a post-commit revision
is always committed immediately following its corresponding vendor
branch revision.  Alternatively, it could be noted during
FilterSymbolsPass which CVSSymbols should sprout from the post-commit
rather than the vendor branch commit, and this notation should affect
the branch filling code.

> Since it changes the way possible parents are calculated, this patch
> can change the parent which cvs2svn selects for a branch.  In the
> cvs2svn test suite, this patch unexpectedly changes the behaviour of
> the existing tests 14, 15, 16, and 17.  These all check the parent of
> the B_FROM_INITIALS_BUT_ONE branch in the "main" test repository.
> From looking at the CVS history, I don't think it's possible to tell
> if the parent is really B_FROM_INITIALS, trunk, or "vendorbranch".
> The old code made B_FROM_INITIALS the parent; the new code uses
> trunk.  I think this is OK, and I've updated the tests accordingly.
> (Looking at the README file, it looks like trunk is the real
> parent).

This is all unsurprising.

I wonder why you changed register_branch_possible_parents() but not
register_tag_possible_parents().  It seems like the same phenomenon
could occur with tags.

Michael

[1] http://cvs2svn.tigris.org/issues/show_bug.cgi?id=54
[2] http://cvs2svn.tigris.org/issues/show_bug.cgi?id=55

------------------------------------------------------
http://cvs2svn.tigris.org/ds/viewMessage.do?dsForumId=1667&dsMessageId=2574228

To unsubscribe from this discussion, e-mail: [[email protected]].