patch, 4 partsStart a library for cvsimport-related tests

The test suite for "git cvsimport" is pretty limited, and I would like to improve the situation. This patch series contains the first of what I hope will eventually be several additions to the "git cvsimport" test suite.

I am the maintainer of cvs2svn/cvs2git. Most of the new tests will probably use fragments from the cvs2svn test suite. I should admit that part of my motivation for adding tests to the "git cvsimport" test suite is to document its weaknesses, which do not seem to be especially well known.

Patch 1 splits out some code into a library usable by multiple CVS-related tests.

Patch 2 changes the library to add the -f option when invoking cvs (to make it ignore the user's ~/.cvsrc file).

Patch 3 adds a new test to t9600, namely to compare the entire module as checked out by CVS vs. git.

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.

These ideas in the patches are logically independent of each other, but each patch assumes that the previous patches have been applied.

I would like to point out a few things about these patches that seem a little bit unprecedented in the git test suite. If other approaches would be preferred, please let me know.

The first is that I would like to introduce a library that can be used by the "git cvsimport" tests in the t96xx series, simply to avoid code duplication. I put this library in t/t96xx/cvs-lib.sh, to hopefully make its role clear. The library has to be sourced from the main test directory. (It sources test-lib.sh indirectly.)

The second is that the new test script uses a small CVS repository that is part of the test suite (i.e., the *,v files are committed directly into the git source tree). This is different than the approach of t9600, which creates its own test CVS repository using CVS commands. The reasons for this are:

- t9600 wants to test incremental import, so it *has to* create the
  repository dynamically.  That is not the case for t9601, which only
  tests a one-shot import.

- The repository for t9601 is derived from one that already exists as
  part of the cvs2svn test suite.  Reverse-engineering it into CVS
  commands would be extra work.

- The code to create CVS repositories via CVS commands is not very
  illuminating, and runs slowly, as CVS throttles commits to 1 per
  second (to ensure unique timestamps).

- Future tests may require even more complicated CVS repositories that
  are even more cumbersome to create, so it's good to set a precedent
  now :-)

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.

Cheers, Michael

For now the "library" just includes code (moved from t/t9600-cvsimport.sh) that checks whether the prerequisites for "git cvsimport" are installed.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t9600-cvsimport.sh |   29 +----------------------------
 t/t96xx/cvs-lib.sh   |   31 +++++++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 28 deletions(-)
 create mode 100644 t/t96xx/cvs-lib.sh

A user's ~/.cvsrc file can change the basic behavior of CVS commands. Therefore we should ignore it in order to ensure consistent results from the test suite.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t9600-cvsimport.sh |   16 ++++++++--------
 t/t96xx/cvs-lib.sh   |    3 +++
 2 files changed, 11 insertions(+), 8 deletions(-)

Test added for completeness (it passes).

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t9600-cvsimport.sh |    2 ++
 t/t96xx/cvs-lib.sh   |   25 +++++++++++++++++++++++++
 2 files changed, 27 insertions(+), 0 deletions(-)

CVS's handling of vendor branches is tricky; add some tests to check whether revisions added via "cvs imports" then imported to git via "git cvsimport" are reflected correctly on master.

One of these tests fail and is therefore marked "test_expect_failure". Cvsimport doesn't realize that subsequent changes on a vendor branch affect master as long as the vendor branch is the default branch.

The test CVS repository used for these tests is derived from cvs2svn's test suite.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 t/t9601-cvsimport-vendor-branch.sh                 |   86 ++++++++++++++++++++
 t/t9601/cvsroot/CVSROOT/.gitignore                 |    1 +
 t/t9601/cvsroot/module/added-imported.txt,v        |   44 ++++++++++
 t/t9601/cvsroot/module/imported-anonymously.txt,v  |   42 ++++++++++
 .../module/imported-modified-imported.txt,v        |   76 +++++++++++++++++
 t/t9601/cvsroot/module/imported-modified.txt,v     |   59 +++++++++++++
 t/t9601/cvsroot/module/imported-once.txt,v         |   43 ++++++++++
 t/t9601/cvsroot/module/imported-twice.txt,v        |   60 ++++++++++++++
 t/t96xx/cvs-lib.sh                                 |    6 ++
 9 files changed, 417 insertions(+), 0 deletions(-)
 create mode 100755 t/t9601-cvsimport-vendor-branch.sh
 create mode 100644 t/t9601/cvsroot/CVSROOT/.gitignore
 create mode 100644 t/t9601/cvsroot/module/added-imported.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-anonymously.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-modified-imported.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-modified.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-once.txt,v
 create mode 100644 t/t9601/cvsroot/module/imported-twice.txt,v

On Fri, Feb 20, 2009 at 06:18:09AM +0100, Michael Haggerty wrote:

Great. I agree the test suite is terrible for cvsimport; what little is there was added only after a regression where it was totally broken. ;)

I don't think it is a problem to document cvsimport's weakness. It is clear from list traffic that it has shortcomings, and IMHO documenting them clearly and rigorously with test cases is the first step to fixing them (or admitting that people should just use something else ;) ).

The only downside I see is that it bloats git's test suite a bit (and cvs tests are often slow to run). We can always make them optional, I suppose.

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 :) ).

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.

The code in t9600 (which gets moved to lib-cvs in your patch 1) sets HOME explicitly. So is this really a problem?

Sounds reasonable.

Cool. Are you volunteering to fix git-cvsimport, too? :)

I think that's fine. There are other places in the test suite where things that are a pain to produce are just included as content (e.g., see some of the SVN tests in the 9100 series).

And I think all of the reasons you gave are compelling.

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.,:

Jeff King <peff@peff.net> writes:

Thanks for a review.  Everything you said makes sense to me.

Also I noticed that [3/4] uses "diff -r -x" --- does it pretty much mean we require GNU diff to pass the test? Can this be made more portable?

Junio C Hamano wrote:

I can't think offhand of a more portable tool that could replace "diff -r -x" here (suggestions, anyone?). Of course one could re-implement recursive diff in shell or perl, and I will take a stab at it if you consider it important. Alternatively, one could hardcode the paths that the test scripts should test, but that would cost more per-test work that I'd rather avoid.

Michael

With this patch, it is possible to exclude files based on basename patterns. Example:

	$ git diff --no-index -x Makefile -x Makefile.in a/ b/

In this example, the recursive diff between a/ and b/ will be shown modulo changes in files named 'Makefile' or 'Makefile.in'.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

	Michael wrote:

	> I can't think offhand of a more portable tool that could replace 
	> "diff -r -x" here (suggestions, anyone?).

	Maybe something like this?

	Note: before it can be included in git.git, documentation and 
	tests have to be added; also, it might be a good idea to extend it 
	to the "non-no-index" case (maybe I can beat Peff in the number of 
	double negations one day...)

	So why only half a patch?  From time to time, I have to remember 
	that I work on Git for fun.  And this was the fun part, as far as 
	I am concerned.

 diff-no-index.c |   50 ++++++++++++++++++++++++++++++++++++++++++++------
 diff.c          |    9 +++++++++
 diff.h          |    6 ++++++
 3 files changed, 59 insertions(+), 6 deletions(-)

On Fri, Feb 20, 2009 at 03:12:28PM +0100, Johannes Schindelin wrote:

Great. Using "git diff" was my first thought, too.

Maybe a config option "diff.denyNonIndexExclude = false"? *ducks*

But more seriously, how would a user expect this to interact with .gitignore? I know gitignore is about ignoring untracked files, but I can't help but feel the two have something in common. But maybe not. I'm sick today and my brain is not working very well.

-Peff

Hi,

On Fri, 20 Feb 2009, Jeff King wrote:

Heh :-)

I have to admit that I had something pretty ugly involving find and grep in mind at first, then something equally appalling using GIT_WORKTREE and GIT_INDEX_FILE.

How about diffNoIndex.denyNonIndexNonExclude = !true?  *swans*

I think that the -x option with regular (not --no-index) diff would be a little different. .gitignore is for "git add" time, while "git diff" happily ignores .gitignore.

Besides, the -x option only works on the basenames (as I implemented it; no idea if GNU diff works the same way, but from Michael's patch it looks like it does).

BTW I just realized that I forgot to add a die() when !no_index && basename_excludes.

Ciao, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Info: (diff.info.gz)diff Options

`-x PATTERN'
`--exclude=PATTERN'
     When comparing directories, ignore files and subdirectories whose
     basenames match PATTERN.  *Note Comparing Directories::.

`-X FILE'
`--exclude-from=FILE'
     When comparing directories, ignore files and subdirectories whose
     basenames match any pattern contained in FILE.  *Note Comparing
     Directories::.

Hi,

On Fri, 20 Feb 2009, Jakub Narebski wrote:

Thanks for the clarification!

Ciao, Dscho

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

I agree that diff_options is the logical way to hook this information and diff_opt_parse() is the right place to add this, but why isn't this done at diff_{addremove,change,unmerge}() layer? That way you should be able to cover both no-index special case and the normal diffs, no?

Hi,

On Fri, 20 Feb 2009, Junio C Hamano wrote:

The principal aim of this patch was to support git diff --no-index -x, that is why (and in addition, avoiding unnecessary work when we can exclude stuff already early in the code path).

It is unlikely that I will work on this before next week.

Thanks, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Oh, that's something I'd welcome, as I'd like to see people not necessarily hunting for but definitely responding to issues in the soon to be tagged 1.6.2, instead of new features ;-)

Jeff King wrote:

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.

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

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.

Not unless you call cvs2git the fixed version :-)

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

On Fri, Feb 20, 2009 at 11:21:38AM +0100, Michael Haggerty wrote:

OK. I haven't looked at it and you have, so I will accept your judgement.

Yuck. But if that's the way it works, then I think your patch is the only way.

Heh.

Agreed. I have seen that advice given on the list several times, and it seems to be working for people. So it really is about the right tool for the job, IMHO.

-Peff

Michael Haggerty wrote:

Michael,

If I run cvs2git several times against a live cvs repo (using the same configuration), wouldn't it perform an incremental import? Is there anything that would make it produce different commits for the history?

I've just made a simple test here performing 2 imports (the 2nd with a dozen of new commits not in the 1st) and it seemed to work fine.

I know that it will take the same time/memory as the first import, but is there something that can break the repository or produce wrong data?

Thanks,

 - Samuel

Samuel Lucas Vaz de Mello wrote:

Cool, I'd never thought of that. It's certainly not by design, but as you've discovered, the interaction of cvs2git and git *almost* combine to give you an incremental import.

Alas, it is only "almost". There are many things that can happen in a CVS repository that would cause the overlapping part of the history to disagree between runs of cvs2svn. The nastiest are things that a VCS shouldn't really even allow, but are common in CVS, like

- Retroactively adding a file to a branch or tag.  (This is a
much-beloved feature of CVS.)  Since CVS doesn't record the timestamp
when a symbol is added to a file, cvs2git tries (subject to the
constraints of other timestamps) to group all such changes into a single
changeset.  So the creation of the symbol would look different in runs N
vs N+1 of cvs2git--containing different files and likely with a
different timestamp.

- Renaming a file "with history" by renaming or copying the associated
*,v file in the repository.  This retroactively changes the entire
history of that file and thus of all changesets that involved changes to
that file.

- Changing the "text vs binary" or keyword expansion mode of a file.
These properties apply to all revisions of a file, and therefore also
have a retroactive effect.

But even aside from these retroactive changes, the output of cvs2git is not deterministic in any practical sense (though I've tried to make it deterministic given *identical* input). The problem is that there are so many ambiguities in a CVS history (because CVS doesn't record enough information) that cvs2git has to use heuristics to decide what individual file events should be grouped together as commits. The trickiest part is that the graph of naively inferred changesets can have cycles in it, and cvs2git uses several heuristics to decide how to split up changesets so as to remove the cycles. (See our design notes [1] for all the hairy details.) The CVS commits made between runs N and N+1 could easily change some of the heuristics' decisions, giving different results even for the overlapping part of the history.

To add robust support for incremental commits to cvs2git would require run N+1 to know about the decisions made in run N, to avoid contradicting them.

I wonder what would happen if one would treat the results of cvs2git conversions N and N+1 as two separate repositories and merge them using git. In many cases the merge would probably be trivial, and most conflicts (except retroactive file renaming!) would probably tend to be in the recent past and therefore resolvable manually. At least the repository shouldn't silently become corrupted, which can happen with other incremental conversion tools.

The final problem is that cvs2git conversions of large CVS repositories are quite time-consuming, so using it for incremental conversions of large repositories would be painful. No doubt it could be speeded up considerably, especially if conversion N+1 was privy to the results of conversion N.

These are all challenging problems and I would welcome volunteers and be happy to get them started.

Michael

[1] http://cvs2svn.tigris.org/svn/cvs2svn/trunk/doc/design-notes.txt

        Hi,

 I'm actually working on coming up with a patch for a bug I hit that has to to do with safecrlf=true. Maybe now I should coordinate with you?

 See this thread
 http://article.gmane.org/gmane.comp.version-control.git/110152

 Ferry

 Michael Haggerty wrote:    The test suite for "git cvsimport" is pretty limited, and I would like to improve the situation.  This patch series contains the first of what I hope will eventually be
several additions to the "git cvsimport" test suite.  I am the maintainer of cvs2svn/cvs2git.  Most of the new tests will probably use fragments from the cvs2svn test suite.  I should admit that
part of my motivation for adding tests to the "git cvsimport" test suite is to document its weaknesses, which do not seem to be especially well known.  Patch 1 splits out some code into a library
usable by multiple CVS-related tests.  Patch 2 changes the library to add the -f option when invoking cvs (to make it ignore the user's ~/.cvsrc file).  Patch 3 adds a new test to t9600, namely to
compare the entire module as checked out by CVS vs. git.  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.  These ideas in the patches are logically independent of each other, but each patch assumes that the previous patches have been applied.  I would like to point out a few things about
these patches that seem a little bit unprecedented in the git test suite.  If other approaches would be preferred, please let me know.  The first is that I would like to introduce a library that can
be used by the "git cvsimport" tests in the t96xx series, simply to avoid code duplication.  I put this library in t/t96xx/cvs-lib.sh, to hopefully make its role clear.  The library has to be
sourced from the main test directory.  (It sources test-lib.sh indirectly.)  The second is that the new test script uses a small CVS repository that is part of the test suite (i.e., the *,v files
are committed directly into the git source tree).  This is different than the approach of t9600, which creates its own test CVS repository using CVS commands.  The reasons for this are:  - t9600
wants to test incremental import, so it *has to* create the   repository dynamically.  That is not the case for t9601, which only   tests a one-shot import.  - The repository for t9601 is derived
from one that already exists as   part of the cvs2svn test suite.  Reverse-engineering it into CVS   commands would be extra work.  - The code to create CVS repositories via CVS commands is not very
  illuminating, and runs slowly, as CVS throttles commits to 1 per   second (to ensure unique timestamps).  - Future tests may require even more complicated CVS repositories that   are even more
cumbersome to create, so it's good to set a precedent   now :-)  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.  Cheers, Michael -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@vger.kernel.org More majordomo info at 
http://vger.kernel.org/majordomo-info.html

Ferry Huberts (Pelagic) wrote:

I am only adding some tests of "git cvsimport"; I definitely don't plan to become a "git cvsimport" hacker. But we can certainly work together on the test infrastructure if it will help you.

Michael

Michael Haggerty wrote:

I'd like to add a couple of tests to t9600 (as per Johannes' suggestion) to test my patch

Ferry

Michael Haggerty <mhagger@alum.mit.edu> writes:

Thanks, both. I generally am not very fond of adding tests without intention to look into fixes, but if they make outstanding bugs more visible, they may have the effect of shaming the original authors badly enough to step in in the effort of fixing them ;-)