Re: [PATCH 0/3] fixup remaining cvsimport tests

From: Eric S. Raymond <esr@thyrsus.com>
Date: Jan 21, 2013, 02:43 UTC
Message-ID: <20130121024314.GA27799@thyrsus.com>
In-Reply-To: <CAEUsAPYdpsbhCZfp-1w91ZiyqgEa=8TNf2MJihMViqVZmW3sRw@mail.gmail.com>

Show 21 quoted lines

> > I probably won't be sending any more patches on this.  My hope was to
> > get cvsimport-3 (w/ cvsps as the engine) in a state such that one
> > could transition from the previous version seamlessly.  But the break
> > in t9605 has convinced me this is not worth the effort--even in this
> > trivial case cvsps is broken.  The fuzzing logic aggregates commits
> > into patch sets that have timestamps within a specified window and
> > otherwise matching attributes.  This aggregation causes file-level
> > commit timestamps to be lost and we are left with a single timestamp
> > for the patch set: the minimum for all contained CVS commits.  When
> > all commits have been processed, the patch sets are ordered
> > chronologically and printed.
> >
> > The problem is that is that a CVS commit is rolled into a patch set
> > regardless of whether the patch set's timestamp falls within the
> > adjacent CVS file-level commits.  Even worse, since the patch set
> > timestamp changes as subsequent commits are added (i.e., it's always
> > picking the earliest) it is potentially indeterminate at the time a
> > commit is added.  The result is that file revisions can be reordered
> > in resulting Git import (see t9605.)  I spent some time last week
> > trying to solve this but I coudln't think of anything that wasn't a
> > substantial re-work of the code.

I've lost who was who in the comment thread, but I think it is rather likely that the above diagnosis is correct in every respect.

I won't know for certain until I finish the test suite and apply it to all three tools (cvsps, cvs2git, cvs-fast-export) but what I've seen of their code indicates that cvsps has the weakest changeset analysis of the three, even after my fixes.

> > I have never used cvs2git, but I suspect Eric's efforts in making it a
> > potential backend for cvsimport are a better use of time.

Agreed. I didn't add multiengine support to csvsimport at random or just because Heiko Vogt was bugging me about parsecvs. I was half-expecting cvsps to manifest a showstopper like this - hoping it wouldn't, but hedging against the possibility by making alternate engines easy to plug into git-cvsimport seemed like a *really good idea* from the beginning of my work on it. Sometimes being that kind of right really sucks.

While I am going to have a try at modifying cvsps to make Chris's t9605 case work, I'm going to strictly limit the amount of time I spend on that effort since (as you imply) it is fairly likely this would be throwing good money after bad.

> Fixing this seemed like it would require splitting the processing out
> into a couple phases and would be a fair amount of work, but maybe I'm
> just not looking at the problem right.

Actually I think you've called it *exactly* right. The job has to be done in multiple clique-spitting phases - that's why cvs2git has 7 passes (though a few of those, perhaps as many as 3, are artifactual).

This is why the next step in my current work plan for CVS-related stuff will be unbundling my test suite from the cvsps tree and running it to see if cvs-fast-export dominates cvsps.

I'm expecting that it will, in which case my plan will be to salvage the CVS client code out of cvsps (*that* part is quite good - fast, clean, effective) gluing it to the better analysis stage in cvs-fast-export, and then shooting cvsps through the head and burying it behind the barn.

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