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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 11, 2013, 16:31 UTC
Message-ID: <7va9sfd6lk.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1357875152-19899-1-git-send-email-gitster@pobox.com>

Show 25 quoted lines

> From: "Eric S. Raymond" <esr@thyrsus.com>
> ...
> diff --git a/git-cvsimport.perl b/git-cvsimport-fallback.perl
> similarity index 98%
> rename from git-cvsimport.perl
> rename to git-cvsimport-fallback.perl
> index 0a31ebd..4bc0717 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport-fallback.perl
> @@ -1,4 +1,8 @@
>  #!/usr/bin/perl
> +# This code became obsolete in January 2013, and is retained only as a
> +# fallback from git-cvsimport.py for users who have only cvsps-2.x.
> +# It (and the code in cvsimport.py that calls it) should be removed
> +# once the 3.x version has had a reasonable time to propagate.
>  
>  # This tool is copyright (c) 2005, Matthias Urlichs.
>  # It is released under the Gnu Public License, version 2.
> @@ -27,6 +31,10 @@
>  use POSIX qw(strftime tzset dup2 ENOENT);
>  use IPC::Open2;
>  
> +print(STDERR "You do not appear to have cvsps 3.x.\n");
> +print(STDERR "Falling back to unmaintained Perl cvsimport for cvsps 2.x.\n");
> +print(STDERR "Upgrade your cvsps for best results.\n");

I think the prevalent style in this script is to write "print" without parentheses:

	print STDERR "msg\n";

Show 24 quoted lines

> diff --git a/git-cvsimport.py b/git-cvsimport.py
> new file mode 100755
> index 0000000..129471e
> --- /dev/null
> +++ b/git-cvsimport.py
> @@ -0,0 +1,354 @@
> +#!/usr/bin/env python
> +#
> +# Import CVS history into git
> +#
> +# Intended to be a near-workalike of Matthias Urlichs's Perl implementation.
> ...
> +class cvsps:
> +    "Method class for cvsps back end."
> +    def __init__(self):
> +        self.opts = ""
> +        self.revmap = None
> +    def set_repo(self, val):
> +        "Set the repository root option."
> +        if not val.startswith(":"):
> +            if not val.startswith(os.sep):
> +                val = os.path.abspath(val)
> +            val = ":local:" + val
> +        self.opts += " --root '%s'" % val

This looks lazy and unsafe quoting. Is there anything that makes sure repository path does not contain a single quote?

Show 12 quoted lines

> +    def set_authormap(self, val):
> +        "Set the author-map file."
> +        self.opts += " -A '%s'" % val
> +    def set_fuzz(self, val):
> +        "Set the commit-similarity window."
> +        self.opts += " -z %s" % val
> +    def set_nokeywords(self):
> +        "Suppress CVS keyword expansion."
> +        self.opts += " -k"
> +    def add_opts(self, val):
> +        "Add options to the engine command line."
> +        self.opts += " " + val

... especially for callers of this method.

The same comment applies to many uses of "val" in the method implementations of this class and the cvs2git class.

> +    def command(self):
> +        "Emit the command implied by all previous options."
> +        return "(cvs2git --username=git-cvsimport --quiet --quiet --blobfile={0} --dumpfile={1} {2} {3} && cat {0} {1} && rm {0} {1})".format(tempfile.mkstemp()[1], tempfile.mkstemp()[1], self.opts, self.modulepath)

Could we do something better with this overlong source line?

Show 24 quoted lines

> +if __name__ == '__main__':
> ...
> +    for (opt, val) in options:
> +        if opt == '-v':
> +            verbose += 1
> +        elif opt == '-b':
> +            bare = True
> +        elif opt == '-e':
> +            for cls in (cvsps, cvs2git):
> +                if cls.__name__ == val:
> +                    backend = cls()
> +                    break
> +            else:
> +                sys.stderr.write("git cvsimport: unknown engine %s.\n" % val)
> +                sys.exit(1)
> +        elif opt == '-d':
> +            backend.set_repo(val)
> +        elif opt == '-C':
> +            outdir = val
> +        elif opt == '-r':
> +            remotize = True
> +        elif opt == '-o':
> +            sys.stderr.write("git cvsimport: -o is no longer supported.\n")
> +            sys.exit(1)

Isn't this a regression?

Show 15 quoted lines

> +        elif opt == '-i':
> +            import_only = True
> +        elif opt == '-k':
> +            backend.set_nokeywords()
> +        elif opt == '-u':
> +            underscore_to_dot = True
> +        elif opt == '-s':
> +            slashsubst = val
> +        elif opt == '-p':
> +            backend.add_opts(val.replace(",", " "))
> +        elif opt == '-z':
> ...
> +        elif opt == '-P':
> +            backend = filesource(val)
> +            sys.exit(1)

???

> +        elif opt in ('-m', '-M'):
> +            sys.stderr.write("git cvsimport: -m and -M are no longer supported: use reposurgeon instead.\n")
> +            sys.exit(1)

I wonder if it is better to ignore these options with a warning but still let the command continue; cvsps-3.x was supposed to get merges right without the help of these ad-hoc options, no?

Otherwise it looks like a regression to me.

Show 36 quoted lines

> +        elif opt == '-S':
> +            backend.set_exclusion(val)
> +        elif opt == '-a':
> +            sys.stderr.write("git cvsimport: -a is no longer supported.\n")
> +            sys.exit(1)
> +        elif opt == '-L':
> +            sys.stderr.write("git cvsimport: -L is no longer supported.\n")
> +            sys.exit(1)
> +        elif opt == '-A':
> +            authormap = os.path.abspath(val)
> +        elif opt == '-R':
> +            revisionmap = True
> +        else:
> +            print """\
> +git cvsimport [-A <author-conv-file>] [-C <git_repository>] [-b] [-d <CVSROOT>]
> +     [-e engine] [-h] [-i] [-k] [-p <options-for-cvsps>] [-P <source-file>]
> +     [-r <remote>] [-R] [-s <subst>] [-S <regex>] [-u] [-v] [-z <fuzz>]
> +     [<CVS_module>]
> +"""
> +    def metadata(fn, outdir='.'):
> +        if bare:
> +            return os.path.join(outdir, fn)
> +        else:
> +            return os.path.join(outdir, ".git", fn)
> +    # Ugly fallback code for people with only cvsps-2.x
> +    # Added January 2013 - should be removed after a decent interval.
> +    if backend.__class__.__name__ == "cvsps":
> +        try:
> +            subprocess.check_output("cvsps -V 2>/dev/null", shell=True)
> +        except subprocess.CalledProcessError as e:
> +            if e.returncode == 1:
> +                sys.stderr.write("cvsimport: falling back to old version...\n")
> +                sys.exit(os.system("git-cvsimport-fallback " + " ".join(sys.argv[1:])))
> +            else:
> +                sys.stderr.write("cvsimport: cannot execute cvsps.\n")
> +                sys.exit(1)

Having the code to die when it sees options the rewritten version does not yet support before it calls the fallback makes the fallback much less effective, no?

Show 8 quoted lines

> +    # Real mainline code begins here
> +    try:
> +        if outdir:
> +            try:
> +                # If the output directory does not exist, create it
> +                # and initialize it as a git repository.
> +                os.mkdir(outdir)
> +                do_or_die("git init --quiet " + outdir)

Did anything made sure outdir is without $IFS chars up to this point?

Not very impressed (yet). The advertised "fix major bugs" sounds more like "trade major bugs with different ones with a couple of feature removals" at this point.