Re: [PATCH] cvsimport: rewrite to use cvsps 3.x to fix major bugs

["Eric S. Raymond" <esr@thyrsus.com>]

Junio C Hamano <gitster@pobox.com>:
> And here is what I got:

Hm. In my version of these tests, I only have one regression from the
old combo (in the pathological tags test, t9602).  You're seeing more
breakage than that, obviously.

> A funny thing was that without cvsps-3.7 on $PATH (which means I am
> getting distro packaged cvsps 2.1), I got identical errors.

That suggests that something in your test setup has gone bad and is
introducing spurious errors. Which would be consistent with the above.

>                                                     Looking
> at the log message, it seems that you meant to remove t960[123], so
> perhaps the patch simply forgot to remove 9601 and 9602?

Yes.
 
> As neither test runs "git cvsimport" with -o/-m/-M options, ideally
> we should be able to pass them with and without having cvsps-3.x.
> Not passing them without cvsps-3.x would mean that the fallback mode
> of rewritten cvsimport is not working as expected. Not passing them
> with cvsps-3.x may mean the tests were expecting a wrong conversion
> result, or they uncover bugs in the replacement cvsimport.

That's possible, but seems unlikely.  Because the new cvsimport is
such a thin wrapper around the conversion engine, bugs in it should
lead to obvious crashes or failure to run the engine rather than the 
sort of conversion error the t960* tests are designed to check.  Really
all it does is assemble options to pass to the conversion engines.

My test strategy is aimed at the engine, not the wrapper. I took the
repos from t960*  and wrote a small Python framework to check the same 
assertions as the git-tree tests do, but using the engine.  For example,
here's how my t9602 looks:

import os, cvspstest

cc = cvspstest.ConvertComparison("t9602", "module")
cc.cmp_branch_tree("test of branch", "master", True)
cc.cmp_branch_tree("test of branch", "vendorbranch", True)
cc.cmp_branch_tree("test of branch", "B_FROM_INITIALS", False)
cc.cmp_branch_tree("test of branch", "B_FROM_INITIALS_BUT_ONE", False)
cc.cmp_branch_tree("test of branch", "B_MIXED", False)
cc.cmp_branch_tree("test of branch", "B_SPLIT", True)
cc.cmp_branch_tree("test of tag", "vendortag", False)
# This is the only test new cvsps fails that old git-cvsimport passed.
cc.cmp_branch_tree("test of tag", "T_ALL_INITIAL_FILES", True)
cc.cmp_branch_tree("test of tag", "T_ALL_INITIAL_FILES_BUT_ONE", False)
cc.cmp_branch_tree("test of tag", "T_MIXED", False)
cc.cleanup()
 
> t9600 fails with "-a is no longer supported", even without having
> cvsps-3.x on the $PATH (i.e. attempting to use the fallback).  I
> wonder if this is an option the updated cvsimport would want to
> simply ignore?

Probably.  But I don't think you should keep these tests in the git tree.
That wasn't a great idea even when you were supporting just one engine;
with two (and soon three) it's going to be just silly.  Let sanity-checking
the engines be *my* problem, since I have to do it anyway.

(I'm working towards the generalized test suite as fast as I can.  First
results probably in four days or so.)

> It is a way to tell the old cvsps/cvsimport to disable its
> heuristics to ignore commits made within the last 10 minutes (this
> is done in the hope of waiting for the per-file nature of CVS
> commits to stabilize, IIUC); the user tells the command that he
> knows that the CVS repository is now quiescent and it is safe to
> import the whole thing.

Yes, that's just what -a is supposed to do.  But is should be
irrelevant for testing - in the test framework CVS is running locally, 
so there's no network lag.

> So... does this mean that we now set the minimum required version of
> Python to 2.7?  I dunno.

That would be bad, IMO.  I'll put backporting to 2.6 high on my to-do list.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>