Re: [PATCH 0/4] Add more tests of cvsimport

From: Michael Haggerty <mhagger@alum.mit.edu>
Date: Feb 20, 2009, 10:21 UTC
Message-ID: <499E8432.9010806@alum.mit.edu>
In-Reply-To: <20090220062543.GA27837@coredump.intra.peff.net>

Jeff King wrote:

Show 8 quoted lines

> On Fri, Feb 20, 2009 at 06:18:09AM +0100, Michael Haggerty wrote:
> [...]
> I do wonder, though, whether it would be simpler to make a "cvs import
> test suite" that could pluggably test cvs2svn, git-cvsimport, or other
> converters. Then you could test each on the exact same set of test
> repos. And abstracting "OK, now make a repository from this cvsroot"
> wouldn't be that hard for each command (I wouldn't think, but obviously
> I haven't tried it :) ).

Many tests only need to compare the contents of branches/tags checked out of CVS with those checked out of the converted repository. Such tests could pretty easily be made agnostic about what conversion tool they are testing and also, for that matter, the type of the target VCS (git, SVN, hg, ...). Testing incremental conversions at that level of detail would not be much harder.

But other tests would be harder to write in a neutral fashion. For example, the cvs2svn test suite has tests of log messages, character-set conversions of metadata, correct commit ordering, branching topology, etc.

In fact, one of the many things I haven't gotten around to yet is writing a nontrivial test suite for cvs2git. I'd like to share as much as possible with the cvs2svn test suite, but there is a lot there that is SVN-specific.

Show 5 quoted lines

>> Patch 1 splits out some code into a library usable by multiple
>> CVS-related tests.
> 
> That is definitely a good first step, though the usual naming convention
> is t/lib-cvs.sh. See t/lib-{git-svn,httpd,rebase}.sh, for example.

Thanks, I hadn't noticed that.  I'll change it the v2 of the patch.

Show 5 quoted lines

>> Patch 2 changes the library to add the -f option when invoking cvs (to
>> make it ignore the user's ~/.cvsrc file).
> 
> The code in t9600 (which gets moved to lib-cvs in your patch 1) sets
> HOME explicitly. So is this really a problem?

That's a good question. I just checked, and empirically cvs uses .cvsrc from my true home directory even if HOME is set differently. So I think that the -f option is indeed necessary.

Show 5 quoted lines

>> Patch 4 adds a new test script t9601 that tests "git cvsimport"'s
>> handling of CVS vendor branches.  One of these tests fails due to an
>> actual bug.
> 
> Cool. Are you volunteering to fix git-cvsimport, too? :)

Not unless you call cvs2git the fixed version :-)

Show 18 quoted lines

>> Finally, the *,v files comprising the CVS repository have blank
>> trailing lines, triggering a warning from "git diff --check".  I don't
>> think that CVS strictly requires the blank lines, but they are always
>> generated by CVS, so I left them in.  But if the "git diff --check"
>> warnings are considered a serious problem, the blank lines could
>> probably be removed.
> 
> It's best to leave them in, I think, to create as realistic a test as
> possible. But you should mark the paths as "we don't care about
> whitespace" using gitattributes. I.e.,:
> 
> diff --git a/t/t9601/.gitattributes b/t/t9601/.gitattributes
> new file mode 100644
> index 0000000..562b12e
> --- /dev/null
> +++ b/t/t9601/.gitattributes
> @@ -0,0 +1 @@
> +* -whitespace

Cool, I didn't know about that feature.  I'll include that in v2 as well.

Thanks for the feedback!

BTW, I don't want to trash "git cvsimport". I'm not brave enough even to try to implement incremental conversions in cvs2git. So the fact that cvsimport sometimes works is already impressive :-) I also understand that its limitations come from those of cvsps, another impressive but flawed tool (which in turn is being used outside of its design limits). But I hope to raise awareness that cvsps-based tools are not the best choice for "one-shot" conversions, and maybe work against people's tendency to use the "default" tool unless it obviously blows up.

Michael