Re: [PATCH] cvsimport: new -R option: generate revision-to-commit mapping

From: Jeff King <peff@peff.net>
Date: Jan 31, 2010, 13:24 UTC
Message-ID: <20100131132449.GA29470@coredump.intra.peff.net>
In-Reply-To: <bc341e101001310443x18e02281i2e4d18334ead700b@mail.gmail.com>

On Sun, Jan 31, 2010 at 12:43:44PM +0000, Aaron Crane wrote:

> Signed-off-by: Aaron Crane <git@aaroncrane.co.uk>
> ---

Please put a bit of the rationale into the commit message. Even a sentence or two can help later on when somebody is reading the output of "git log".

>  Documentation/git-cvsimport.txt |   15 ++++++++++++++-
>  git-cvsimport.perl              |   20 ++++++++++++++++----
>  2 files changed, 30 insertions(+), 5 deletions(-)

A basic test would be nice. You should be able to just use your new "-R" during the import in t9600, and then check that it generated the correct mapping.

Show 5 quoted lines

> +-R <revision-to-commit-file>::
> +	Generate a file containing a mapping from CVS revision numbers to
> +	newly-created Git commit IDs.  The generated file will contain one
> +	line for each (filename, revision) pair found by 'cvsps'; each line
> +	will look like

Is mentioning 'cvsps' right here? cvsps doesn't know about git commit id's.

> +open my $revision_map, '>', $opt_R
> +    or die "Can't open -R file $opt_R: $!\n"
> +	if defined $opt_R;

You need to use munge_user_filename here to handle relative paths. See commit f6fdbb6.

Also, should you perhaps be appending to the file instead of truncating it? Remember that cvsimport can be used incrementally. I wonder if it would be better to simply have "-R" without an argument to append the revision map to a file .git/cvs-revisions or something. And then the user can easily pull it from there after the import.

-Peff