Re: [PATCH v3 0/2] cvsimport: add test illustrating a bug in cvsps

Heiko Voigt [off-list ref] writes:

This is an updated version of the first patch and an addition to ensure
correct handling of branches in fixes. 

I've already merged the first one to 'next' so this needs to be turned
into an incremental update if we were to continue building on top in the
git project.

I however have a bigger issue with this, perhaps because I do not have the
feeling that I fully understand where these patches are going.

Your approach seems to me to:

 - add tests to git test suite that expose issues the current cvsimport
   that runs on an unpatched cvsps has;

 - diagnose and fix

   - the issues in cvsimport, if the problem is because cvsimport is
     mishandling correct output from cvsps; or

   - the issues in cvsps (and adjust cvsimport to patched cvsps if
     necessary), if the problem is because output from cvsps is incorrect.

That all feels sane, and having the tests to verify the end result would
help the people who collaborate on these tasks.

But how much of the actual fix will be made to cvsps, and how much to
cvsimport?  If the majority of the changes are to happen on cvsps (which
is not unexpected, given that many people who tried and wrote various cvs
importers put blame on the shortcomings of its output), I am afraid that
it would not help majority of git users until the fixes to cvsps that come
out of this effort hit their distros for me to keep these tests in the
git.git repository.  I do not build and install custom cvsps (because I
haven't had to work with complex history in CVS that your improvements to
cvsps are need to deal with correctly), and I suspect many others are in
the same boat.  In addition, if your tests are in the git.git repository,
they need to say test_expect_success for people with patched cvsps and
test_expect_failure for people without, and because I suspect that the
majority of git developers do not run bleeding edge cvsps, it does not do
anything but slowing down the test suite.

It feels as if you are scratching my feet through my shoes while I still
am wearing them.  I wonder if it would be more direct and simpler approach
to add tests to cvsps and handle these improvements as part of the cvsps
maintenance/development effort, not as part of cvsimport fixes, at least
initially.

I think it is great that you started actively working on identifying and
fixing issues with cvsps, that many others have gave up and gone to
different avenues, and I certainly do not mind keeping the new tests in
'pu' for wider exposure, in order to make it easier for other people who
use cvsimport and want to collaborate with you improving it through
improving cvsps.

But I am starting to think that it was a mistake on my part to have merged
the initial set of tests to 'next'.

Thoughts?

Junio C Hamano venit, vidit, dixit 18.03.2009 19:22:

Heiko Voigt [off-list ref] writes:

quoted

This is an updated version of the first patch and an addition to ensure
correct handling of branches in fixes. 

I've already merged the first one to 'next' so this needs to be turned
into an incremental update if we were to continue building on top in the
git project.

I however have a bigger issue with this, perhaps because I do not have the
feeling that I fully understand where these patches are going.

Your approach seems to me to:

 - add tests to git test suite that expose issues the current cvsimport
   that runs on an unpatched cvsps has;

 - diagnose and fix

   - the issues in cvsimport, if the problem is because cvsimport is
     mishandling correct output from cvsps; or

   - the issues in cvsps (and adjust cvsimport to patched cvsps if
     necessary), if the problem is because output from cvsps is incorrect.

That all feels sane, and having the tests to verify the end result would
help the people who collaborate on these tasks.

But how much of the actual fix will be made to cvsps, and how much to
cvsimport?  If the majority of the changes are to happen on cvsps (which
is not unexpected, given that many people who tried and wrote various cvs
importers put blame on the shortcomings of its output), I am afraid that
it would not help majority of git users until the fixes to cvsps that come
out of this effort hit their distros for me to keep these tests in the
git.git repository.  I do not build and install custom cvsps (because I
haven't had to work with complex history in CVS that your improvements to
cvsps are need to deal with correctly), and I suspect many others are in
the same boat.  In addition, if your tests are in the git.git repository,
they need to say test_expect_success for people with patched cvsps and
test_expect_failure for people without, and because I suspect that the
majority of git developers do not run bleeding edge cvsps, it does not do
anything but slowing down the test suite.

It feels as if you are scratching my feet through my shoes while I still
am wearing them.  I wonder if it would be more direct and simpler approach
to add tests to cvsps and handle these improvements as part of the cvsps
maintenance/development effort, not as part of cvsimport fixes, at least
initially.

I think it is great that you started actively working on identifying and
fixing issues with cvsps, that many others have gave up and gone to
different avenues, and I certainly do not mind keeping the new tests in
'pu' for wider exposure, in order to make it easier for other people who
use cvsimport and want to collaborate with you improving it through
improving cvsps.

But I am starting to think that it was a mistake on my part to have merged
the initial set of tests to 'next'.

Thoughts?

I second this, especially now that we've even got cvsps patches coming
to the git list. cvsps development should procede wherever all cvsps
developers can participate. It's not a central component of git.

In fact, the way git cvsimport uses cvsps may even mask some of cvsps's
behaviour. So, exposing cvsps bugs really only makes sense in a pure
cvsps test suite.

If git cvsimport is supposed to work around cvsps shortcomings then that
is supposed to be tested for in the git suite.

Right now this really seems to be going in the direction of: Make
everyone feel uneasy about working with git on cvs repos at all (aka
guerilla git), because git cvsimport still seems to be "recommended" or
"the standard", we have all this talk about how bad it is, and no clear
alternative.

I wish we had something saying: "If your cvs repo fulfills these
conditions you can use cvsimport. If not watch out for the following
(cvsps related) caveats. If you don't need two-way sync you're better of
with..."

Unfortunately, I'm not cvs(ps)-competent enough to spell out those
recommendations in detail.

Michael

Hi,

On Thu, 19 Mar 2009, Michael J Gruber wrote:

cvsps development should procede wherever all cvsps developers can 
participate. It's not a central component of git.

You are forgetting a central fact to this discussion: cvsps is not 
actively developed anymore.

Virtually all the "recent" patches going into cvsps (IIRC all patches 
between the last two releases) were coming directly from the Git 
community.

Ciao,
Dscho

Johannes Schindelin venit, vidit, dixit 19.03.2009 12:00:

Hi,

On Thu, 19 Mar 2009, Michael J Gruber wrote:

quoted

cvsps development should procede wherever all cvsps developers can 
participate. It's not a central component of git.

You are forgetting a central fact to this discussion: cvsps is not 
actively developed anymore.

Virtually all the "recent" patches going into cvsps (IIRC all patches 
between the last two releases) were coming directly from the Git 
community.

Well, then I guess "wherever all cvsps developers can participate" is de
facto here...

Still it's not in the git codebase. git cvsimport may expose cvsps short
comings, but testing cvsps in git's test suite does not seem right.

Michael

Michael J Gruber wrote:

[...]
Right now this really seems to be going in the direction of: Make
everyone feel uneasy about working with git on cvs repos at all (aka
guerilla git), because git cvsimport still seems to be "recommended" or
"the standard", we have all this talk about how bad it is, and no clear
alternative.

I wish we had something saying: "If your cvs repo fulfills these
conditions you can use cvsimport. If not watch out for the following
(cvsps related) caveats. If you don't need two-way sync you're better of
with..."

Good idea.  Let me try to summarize the situations I have found that can
result in cvsps/cvsimport giving objectively incorrect output.  I am
only including the most blatant problems that can cause the content
checked out of git to differ from that checked out of CVS.  I have
attached some scripts that illustrate some of the problems that I
haven't yet submitted to the test suite.


Problems known to occur in the conversion of CVS HEAD:

- If any files were ever "cvs import"ed more than once (e.g., import of
more than one vendor release) but not modified locally, then the HEAD
version is incorrect (t9601-cvsimport-vendor-branch.sh).

- If the CVS timestamps on subsequent revisions are out of order (e.g.,
because of clock skew) then file commits can be out of order in git
(e.g. b.txt<1.2> overwritten by b.txt<1.1>).  This is demonstrated by
t9603-cvsimport-time.sh.

- If two files ever had what look like cross-cross commits within
cvsps's 5-minute time window (e.g.,

a.txt<1.1> Log message 1
a.txt<1.2> Log message 2

b.txt<1.1> Log message 2
b.txt<1.2> Log message 1

) then the commits of one file will be out of order in git (e.g.
b.txt<1.2> overwritten by b.txt<1.1>).  I have a test for this that I
haven't yet submitted.


Problems known to occur in the conversion of CVS branches:

Any of the above problems can occur, plus:

- A branch does not appear in the git repository if no commits were made
on the branch (t01.sh)

- All files from the branching point are added to a branch, including
those that were never added to the branch in CVS.  (CVS allows part of a
source tree to be added to a branch/tag.) (t06.sh)

- A file that was added to the source branch *after* a daughter branch
was created but before the first commit on the branch will incorrectly
be added to the daughter branch in git. (t04.sh)


Problems known to occur in the conversion of CVS tags:

Any of the above problems can affect the content of the tag, plus:

- If multiple tags mark the same revision, only one of them is included
in the conversion. (t03.sh)


Michael

On Wed, Mar 18, 2009 at 11:22:43AM -0700, Junio C Hamano was talking about:

Heiko Voigt [off-list ref] writes:

quoted

This is an updated version of the first patch and an addition to ensure
correct handling of branches in fixes. 

I've already merged the first one to 'next' so this needs to be turned
into an incremental update if we were to continue building on top in the
git project.

I however have a bigger issue with this, perhaps because I do not have the
feeling that I fully understand where these patches are going.

To explain my intentions a little more: I recently imported a huge
repository with ~11 years of history from a RCS based system. 

The final step into git land was through CVS. Here I really struggled to
find the right tool and it costed me quite some time just to find a
robust tool for the job (and to figure out why the "standard" tool
failed). 

So my main goal is to save some time and hassle for everyone else going
this route. There are really nicer things to spend time on than
importing and repairing RCS files.

I think many people when starting with git get the feeling: Oh look at
this SHA1 based database and stuff these people have written a really
neat robust system. Then you look at getting your history (from CVS)
into it and it becomes a nightmare. The command git cvsimport just does
not fit with the rest of the git tools quality.

Your approach seems to me to:

 - add tests to git test suite that expose issues the current cvsimport
   that runs on an unpatched cvsps has;

 - diagnose and fix

   - the issues in cvsimport, if the problem is because cvsimport is
     mishandling correct output from cvsps; or

   - the issues in cvsps (and adjust cvsimport to patched cvsps if
     necessary), if the problem is because output from cvsps is incorrect.

That all feels sane, and having the tests to verify the end result would
help the people who collaborate on these tasks.

But how much of the actual fix will be made to cvsps, and how much to
cvsimport? 

I can not answer this question at the moment. One thing would be fixing
cvsps, which just for one test (mine) seems like a lot of work. I
haven't even looked into the other issues.

After writing my last email it came to my mind that it could be a
simpler approach to take parsecvs (only because its already written in C
otherwise probably cvs2svn/cvs2git) and change the interface so it
matches the one of git cvsimport and integrate it with git.

To let this happen the question is how important various features of the
current cvsimport are:

  * incremental import

  * keyword substitution

  * ...

If the majority of the changes are to happen on cvsps (which
is not unexpected, given that many people who tried and wrote various cvs
importers put blame on the shortcomings of its output), I am afraid that
it would not help majority of git users until the fixes to cvsps that come
out of this effort hit their distros for me to keep these tests in the
git.git repository.  I do not build and install custom cvsps (because I
haven't had to work with complex history in CVS that your improvements to
cvsps are need to deal with correctly), and I suspect many others are in
the same boat.  In addition, if your tests are in the git.git repository,
they need to say test_expect_success for people with patched cvsps and
test_expect_failure for people without, and because I suspect that the
majority of git developers do not run bleeding edge cvsps, it does not do
anything but slowing down the test suite.

It feels as if you are scratching my feet through my shoes while I still
am wearing them.  I wonder if it would be more direct and simpler approach
to add tests to cvsps and handle these improvements as part of the cvsps
maintenance/development effort, not as part of cvsimport fixes, at least
initially.

I think it is great that you started actively working on identifying and
fixing issues with cvsps, that many others have gave up and gone to
different avenues, and I certainly do not mind keeping the new tests in
'pu' for wider exposure, in order to make it easier for other people who
use cvsimport and want to collaborate with you improving it through
improving cvsps.

But I am starting to think that it was a mistake on my part to have merged
the initial set of tests to 'next'.

Thoughts?

In the long run I think at least a basic test for the current issues
should be in git.  Otherwise if cvsimport gets fixed you do not have a
way of making sure all tools (cvsps) in their right versions are
installed. Even with another non cvsps importer this property needs to
be ensured to handle non trivial repositories.
 
The only people who actually need to know about issues in cvsimport are
the ones who are trying to get away from CVS. So its probably best to
disable the "advanced" tests and have an environment variable e.g.:
"ALL_CVSIMPORT_TESTS" for enabling them.

cheers Heiko

Hallo,

I just started a very simple but hopefully effective testsuite to
collect all the issues we found with cvsps. Because I haven't heard
anything from Yann who where the last one to collect some patches I
forked his master and started from there. 

    http://repo.or.cz/w/cvsps-hv.git

The testsuite is on the pu branch so I can first get some feedback
before I will merge it to master.

Maybe Michael can add his tests as well. You should just need to
construct the expected cvsps output. I choose a similar naming
to the one from git. The suite is very simple:

for each test tXXXX-name should be a correspondingly named CVS module in
t/cvsroot and a directory t/tXXXX-name. In this directory a file named
'expect' contains the expected output of cvsps. If the test is known to
fail also an empty file: 'known_to_fail' is added to the test directory.
Thats it.

Hopefully we can collect some more specifics about the situations where
cvsps fails and most importantly the amount of coding needed for fixing.
From there on it should be easier to decide which route to follow for
a robust inbuild git cvsimport.  

cheers Heiko

On Mon, Mar 23, 2009 at 7:11 PM, Heiko Voigt [off-list ref] wrote:

I just started a very simple but hopefully effective testsuite to
collect all the issues we found with cvsps.

Excellent effort. One thing I found when I was debugging (and trying
to understand) corner cases in cvs imports was the test suite for the
cvs2svn importer. It had a number of tiny cvs repositories that were
broken in various ways.

The test cvs repos, straight from the svn repo for cvs2svn are in
http://cvs2svn.tigris.org/svn/cvs2svn/trunk/test-data/ - access with
user:guest and an empty password.

...

Hopefully we can collect some more specifics about the situations where
cvsps fails and most importantly the amount of coding needed for fixing.
From there on it should be easier to decide which route to follow for
a robust inbuild git cvsimport.

Some cvs repos are ambiguous. Others are "impossible", fruits of
direct file manipulation. cvs2svn has a fairly robust (and
inordinately slow!) approach, but you cannot rerun it to get
incremental imports.

It is certainly possible... and bloody hard!

cheers,


m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Heiko Voigt wrote:

I just started a very simple but hopefully effective testsuite to 
collect all the issues we found with cvsps. [...]

Maybe Michael can add his tests as well. You should just need to 
construct the expected cvsps output.

Please note that the cvsps output format itself is not rich enough to
represent all of the information needed, so it is not trivial to
construct the "expected" output.  The following is from an email that I
sent to the Mercurial mailing list [1]:

I spent some time looking into cvsps, and I don't think that its
output format, let alone the output itself, is suitable for
industrial-strength conversion from CVS.  For example, as far as I
can tell from my brief investigation:

* log messages are not unambiguously separable from the metadata

* it seems to only allow a single tag per changeset

* it seems to only apply tags to single changesets, whereas in CVS
  files can be added to a tag at different times.
...

* it seems to completely ignore a branch tag if no commits were
  ever made on that branch.

* it seems to ignore the effect that a second import to a vendor
  branch has on the main branch.

You might also be interested to know that there has been some work in
the Mercurial project on a Python-based "cvsps".  I believe they use
their own version for their cvsimport extension.  I don't know how
different their cvsps is from the original.

Michael

[1] http://selenic.com/pipermail/mercurial-devel/2008-February/004975.html

On Tue, Mar 24, 2009 at 05:50:03AM +0100, Michael Haggerty wrote:

Heiko Voigt wrote:

quoted

I just started a very simple but hopefully effective testsuite to 
collect all the issues we found with cvsps. [...]

Maybe Michael can add his tests as well. You should just need to 
construct the expected cvsps output.

Please note that the cvsps output format itself is not rich enough to
represent all of the information needed, so it is not trivial to
construct the "expected" output.  

But its possible ;) I have extracted some tests from the scripts you
send and pushed them into the repo. The output format definitely needs
extension. I already included that in the expected output. Maybe you
want to have a look. Tell me what you think.

The following is from an email that I
sent to the Mercurial mailing list [1]:

quoted

I spent some time looking into cvsps, and I don't think that its
output format, let alone the output itself, is suitable for
industrial-strength conversion from CVS.  For example, as far as I
can tell from my brief investigation:

* log messages are not unambiguously separable from the metadata

Thats one issue which could be addressed by escaping the sequences which
lead to the ambiguity.

All issues described below could be addressed by the examples I included in
the tests.

quoted

* it seems to only allow a single tag per changeset

* it seems to only apply tags to single changesets, whereas in CVS
  files can be added to a tag at different times.
...

* it seems to completely ignore a branch tag if no commits were
  ever made on that branch.

* it seems to ignore the effect that a second import to a vendor
  branch has on the main branch.

You might also be interested to know that there has been some work in
the Mercurial project on a Python-based "cvsps".  I believe they use
their own version for their cvsimport extension.  I don't know how
different their cvsps is from the original.

Thats indeed interesting I have to try it once the testsuite is ready.
Maybe we can then just replace the original.

cheers Heiko