Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.

From: Eric S. Raymond <esr@thyrsus.com>
Date: Jan 2, 2013, 00:33 UTC
Message-ID: <20130102003344.GA9651@thyrsus.com>
In-Reply-To: <7vfw2k8t7k.fsf@alter.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com>:

Show 5 quoted lines

> So..., is this a flag-day patch?
> 
> After this is merged, users who have been interoperating with CVS
> repositories with the older cvsps have to install the updated cvsps
> before using a new version of Git that ships with it?

Yes, they must install an updated cvsps. But this is hardly a loss, as the old version was perilously broken.

There was an error or typo in the branch-analysis code, dating from 2006 and possibly earlier, that meant that branch root points would almost always be attributed to parent patchsets one patchset earlier than they should have been. Shocked me when I found it - how was this missed for six years?

Because of the way the analysis is done, this fundamental bug would also cause secondary damage like file changes near the root point getting attributed to the wrong branch. In fact, this is how I first spotted the problem; my test suite exhibited this symptom.

And mind you this is on top of ancestry-branch tracking not working - two separate bugs that could interact in ways I'd really rather not think about. The bottom line is that every import of a branchy CVS repo with a pre-3.x version of cvsps is probably wrong.

The old git-cvsimport code was doing its part to screw things up, too. At least three of the bugs on its manual page are problems I couldn't reproduce using a bare cvsps instance, even the old broken version.

>                                                    As long as
> they update both cvsps and cvsimport, they can continue using the
> existing repository to get updates from the same upstream CVS
> repository without losing hisory continuity?

Yes, but in that case I would strongly advise re-importing the entire CVS history, as the portion analyzed with 2.2b1 and earlier versions of cvsps will almost certainly have been somewhat garbled if it contains any branches.

Show 11 quoted lines

> I would have preferred an addition of "git cvsimport-new" (or rename
> of the existing one to "git cvsimport-old"), with additional tests
> that compare the results of these two implemenations on simple CVS
> history that cvsimport-old did *not* screw up, to ensure that (1)
> people with existing set-up can choose to keep using the old one,
> perhaps by tweaking their process to use cvsimport-old, and (2) the
> updated one will give these people the identical conversion results,
> as long as the history they have been interacting with do not have
> the corner cases that trigger bugs in older cvsps.
> 
> Or am I being too conservative?

I think you are being too conservative. This patch is *not* a mere feature upgrade. The branch-analysis bug I found three days ago is not a minor problem, it is a big ugly showstopper for any case beside the simplest linear histories. Only linear histories will not break.

'People with existing set-ups' should absolutely *not* 'keep using the old one'; we should yank that choice away from them and get the old cvsimport/cvsps pair out of use *as fast as possible*, because it silently mangles branchy imports.

Accordingly, giving people the idea that it's OK to use old and new versions in parallel would be an extremely bad idea. I would go so far as to call it irresponsible.

Here is what I have done to ease the transition:

If you try to use old git-cvsimport with new cvsps, new cvsps will detect this and ship a message to stderr telling you to upgrade

If you try to use new git-cvsimport with old cvsps, old cvsps will complain of an invalid argument and git-cvsimport will quit.

As for testing...cvsps now has several dozen self-tests on five different CVS repositories, including improved versions of the t960[123] tests. I will keep developing these as I work on bringing parsecvs up to snuff.

I don't think there is a lot of point in git-cvsimport having its own tests any more. If you read it I think you'll see why; it's a much thinner wrapper around the conversion engine(s) than it used to be. In particular, it no longer does its own protocol transactions to the CVS server.

-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>