From: Junio C Hamano <gitster@pobox.com>
To: "Eric S. Raymond" <esr@thyrsus.com>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] cvsimport: rewrite to use cvsps 3.x to fix major bugs
Date: Fri, 11 Jan 2013 08:31:19 -0800 [thread overview]
Message-ID: <7va9sfd6lk.fsf@alter.siamese.dyndns.org> (raw)
In-Reply-To: <1357875152-19899-1-git-send-email-gitster@pobox.com> (Junio C. Hamano's message of "Thu, 10 Jan 2013 19:32:32 -0800")
> 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";
> 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?
> + 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?
> +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?
> + 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.
> + 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?
> + # 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.
next prev parent reply other threads:[~2013-01-11 16:31 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-01-11 3:32 [PATCH] cvsimport: rewrite to use cvsps 3.x to fix major bugs Junio C Hamano
2013-01-11 16:31 ` Junio C Hamano [this message]
2013-01-11 18:58 ` Eric S. Raymond
2013-01-11 19:17 ` Junio C Hamano
2013-01-11 19:27 ` Junio C Hamano
2013-01-12 5:04 ` Eric S. Raymond
2013-01-12 5:20 ` Junio C Hamano
2013-01-12 5:38 ` [PATCH] t/t960[123]: remove leftover scripts Junio C Hamano
2013-01-12 6:06 ` Chris Rorvick
2013-01-12 8:40 ` [PATCH] t/lib-cvs: cvsimport no longer works without Python >= 2.7 Junio C Hamano
2013-01-12 15:27 ` Michael Haggerty
2013-01-13 17:17 ` John Keeping
2013-01-12 15:47 ` [PATCH] cvsimport: rewrite to use cvsps 3.x to fix major bugs Eric S. Raymond
2013-01-12 15:13 ` Michael Haggerty
2013-01-12 16:11 ` Eric S. Raymond
2013-01-12 18:16 ` Jonathan Nieder
2013-01-12 18:26 ` Jonathan Nieder
2013-01-13 22:20 ` Junio C Hamano
2013-01-13 23:27 ` Junio C Hamano
2013-01-14 1:40 ` [PATCH 0/3] A smoother transition plan for cvsimport Junio C Hamano
2013-01-14 1:40 ` [PATCH 1/3] cvsimport: allow setting a custom cvsps (2.x) program name Junio C Hamano
2013-01-14 1:40 ` [PATCH 2/3] cvsimport: introduce a version-switch wrapper Junio C Hamano
2013-01-14 1:47 ` Junio C Hamano
2013-01-14 1:40 ` [PATCH 3/3] cvsimport: start adding cvsps 3.x support Junio C Hamano
2013-01-15 6:19 ` Chris Rorvick
2013-01-15 6:44 ` Junio C Hamano
2013-01-14 5:12 ` [PATCH] cvsimport: rewrite to use cvsps 3.x to fix major bugs Michael Haggerty
2013-01-14 7:25 ` [PATCH v2 0/6] A smoother transition plan for cvsimport Junio C Hamano
2013-01-14 7:25 ` [PATCH v2 1/6] Makefile: add description on PERL/PYTHON_PATH Junio C Hamano
2013-01-14 7:25 ` [PATCH v2 2/6] cvsimport: allow setting a custom cvsps (2.x) program name Junio C Hamano
2013-01-14 7:25 ` [PATCH v2 3/6] cvsimport: introduce a version-switch wrapper Junio C Hamano
2013-01-14 7:25 ` [PATCH v2 4/6] cvsimport: start adding cvsps 3.x support Junio C Hamano
2013-01-14 7:25 ` [PATCH v2 5/6] cvsimport: make tests reusable for cvsimport-3 Junio C Hamano
2013-01-14 7:25 ` [PATCH v2 6/6] cvsimport-3: add a sample test Junio C Hamano
2013-01-14 7:47 ` [PATCH v2 0/6] A smoother transition plan for cvsimport Jonathan Nieder
2013-01-14 7:48 ` [PATCH v2 7/6] t9600: further prepare for sharing Junio C Hamano
2013-01-14 7:52 ` [PATCH v2 8/6] t9600: adjust for new cvsimport Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7va9sfd6lk.fsf@alter.siamese.dyndns.org \
--to=gitster@pobox.com \
--cc=esr@thyrsus.com \
--cc=git@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).