Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
On 01/03/2013 04:22 PM, Junio C Hamano wrote:
Antoine Pelisse apeli...@gmail.com writes:
Doesn't Python come with a standard subprocess module that lets you
spawn external programs safely, similar to the way Perl's list form
open(), e.g. open($fh, -|, 'git', @args), works?
... and of course a more boring system('git', $subcmd, @args), as well.
Python's os.system() takes exactly one argument, which must be a string,
and executes it in a subshell. subprocess is indeed the way to go.
You mean something like this:
p1 = subprocess.Popen([backend.command()], stdout=subprocess.PIPE)
subprocess.Popen([git, fast-import, --quiet] + gitopts,
cwd=outdir, stdin=p1.stdout)
Assuming gitopts is a list rather than a string. (care must be taken
with backend.command() also)
Yes.
I vaguely recall that the subprocess module once used to be one
portability issue but that was between Python 2.3 and 2.4 or some
ancient history, and it should no longer be relevant.
subprocess was added in Python 2.4, and the above example should work
fine in any version = 2.4. But please note that other functions have
been added to the module since then, like check_call() (v2.5),
check_output (v2.7), and some methods were added to the Popen object in
v2.6.
Such things are documented pretty reliably in the Python library
documentation [1]; when in doubt, one can view older versions of the
library documentation, which are all available online [2].
Michael
[1] http://docs.python.org/2/library/
[2] http://www.python.org/doc/versions/
--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Hi, Eric S. Raymond wrote: Junio C Hamano gits...@pobox.com: So..., is this a flag-day patch? After this is merged, users who have been interoperating with CVS repositories with the older cvsps have to install the updated cvsps before using a new version of Git that ships with it? Yes, they must install an updated cvsps. But this is hardly a loss, as the old version was perilously broken. Speaking with my Debian packager hat on: the updated cvsps is not available in Debian. git cvsimport is, and it has users that report bugs from time to time. With this change, I would either have to take on responsibility for maintenance of the cvsps package (not going to happen) or drop git cvsimport. That's a serious regression. The moment someone takes care of packaging the updated cvsps, I'll stop minding, though. ;-) I wouldn't be surprised if the situation on other OSes is similar. This is too early to require such a dependency. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Jonathan Nieder jrnie...@gmail.com: Speaking with my Debian packager hat on: the updated cvsps is not available in Debian. git cvsimport is, and it has users that report bugs from time to time. With this change, I would either have to take on responsibility for maintenance of the cvsps package (not going to happen) or drop git cvsimport. That's a serious regression. How does going from it silently damages imports to it fails with an error message constitute a regression? The moment someone takes care of packaging the updated cvsps, I'll stop minding, though. ;-) I'll ping the Debian QA group. -- a href=http://www.catb.org/~esr/;Eric S. Raymond/a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Jonathan Nieder jrnie...@gmail.com: The former is already loudly advertised in the package description and manpage, at least lets you get work done, and works fine for simple repositories with linear history. Two of the three claims in this paragraph are false. The manual page does not tell you what is true, which is that old cvsps will fuck up every branch by putting the root point at the wrong place. And if you call silently and randomly damaging imports getting work done, your definitions of work and done are broken. Taking away a command that people have been using in everyday work is pretty much a textbook example of a regression, no? That would be, but we are talking about replacing total breakage with a git-cvsimport that actually works and that you invoke in pretty much the same way as the old one. Nothing is or will be taken away. In any case, once the distros package cvsps 3.x, old cvsimport will terminate with an error return, because cvsps-3.x sees an obsolete option that git-cvsimport tries to use as a command to treminate after displaying a prompt to upgrade. The most we can accomplish by being conservative is to lengthen the window during which people will falsely believe that their conversion process is working. -- a href=http://www.catb.org/~esr/;Eric S. Raymond/a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond wrote: Jonathan Nieder jrnie...@gmail.com: The former is already loudly advertised in the package description and manpage, at least lets you get work done, and works fine for simple repositories with linear history. Two of the three claims in this paragraph are false. Give me a break. Are you telling me that when multiple users read a manpage that states | WARNING: for certain situations the import leads to incorrect | results. Please see the section ISSUES for further reference. [...] | Problems related to timestamps: [...] | Problems related to branches: [...] | Problems related to tags: [...] | consider using these alternative tools which proved to be more | stable in practice: and a package description that states | The git cvsimport tool can incrementally import from a repository | that is being actively developed and only requires remote access | over CVS protocol. Unfortunately, in many situations the import | leads to incorrect results. For reliable, one-shot imports, cvs2git | from the cvs2svn package or parsecvs may be a better fit. and decide to use the tool anyway, this is not evidence that the tool is invaluable to them, despite its shortcomings? Perhaps the users reporting bugs didn't read the manpage and package description (despite quoting the same passages and explaining why they used the command nonetheless) or I should ignore the judgement calls they make. Consider the following workflow: 1. Update imported project periodically using git-cvsimport 2. Hack, do code archaeology using git log -S and git bisect, etc. 3. Fall back to a web browser and cvsweb to confirm conclusions. You are telling me that it is not a regression to change the workflow to the following: 1. Try to use git-cvsimport. 2. Wonder where that command went. Meanwhile Junio has already suggested a way out. Just rename the command. Hope that helps, Jonathan -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
First of all, I am at the same time a sad, nostalgic, and very happy that old cvsimport is getting replaced. On Wed, Jan 2, 2013 at 11:18 AM, Eric S. Raymond e...@thyrsus.com wrote: Two of the three claims in this paragraph are false. The manual page does not tell you what is true, which is that old cvsps will fuck up every branch by putting the root point at the wrong place. And if you call silently and randomly damaging imports getting work done, your definitions of work and done are broken. The existing cvsps/cvsimport combo work for CVS repos with simple branches, and can track those over time. Replacement with something more solid is welcome, but until you are extremely confident of its handling of legacy setups... I would still provide the old cvsimport, perhaps in contrib. cheers, m -- martin.langh...@gmail.com mar...@laptop.org -- Software Architect - OLPC - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Martin Langhoff martin.langh...@gmail.com: Replacement with something more solid is welcome, but until you are extremely confident of its handling of legacy setups... I would still provide the old cvsimport, perhaps in contrib. I am extremely confident. I built a test suite so I could be. -- a href=http://www.catb.org/~esr/;Eric S. Raymond/a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond e...@thyrsus.com writes: Two of the three claims in this paragraph are false. The manual page does not tell you what is true, which is that old cvsps will fuck up every branch by putting the root point at the wrong place. That doesn't look like being a widespread problem, or more people would have complained. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
On Wed, Jan 2, 2013 at 5:41 PM, Eric S. Raymond e...@thyrsus.com wrote: Martin Langhoff martin.langh...@gmail.com: Replacement with something more solid is welcome, but until you are extremely confident of its handling of legacy setups... I would still provide the old cvsimport, perhaps in contrib. I am extremely confident. I built a test suite so I could be. I too am glad to see some work go into the cvsimport script. So just to clear things up, previously you said this: Yes, they must install an updated cvsps. This is the problem, and one that is easily solved by just keeping a copy of the old command. Remember that for many users of these tools it doesn't matter if the history is correct or not, as long as the head checkout contains the right files and they are able to submit new changes. With this definition of works git-cvsimport is not that broken I think. Cheers, - Thomas -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond e...@thyrsus.com writes: If you try to use new git-cvsimport with old cvsps, old cvsps will complain of an invalid argument and git-cvsimport will quit. I see an opening for smoother transition here. Like it or not, you cannot force distros to ship with cvsps 3.0 when we ship our 1.8.2 (or 2.0 or whatever) that includes a cvsimport that requires cvsps 3.0. The best we can do is to make it capable of working with cvsps 3.0 for a better result (when available), and working with cvsps 2.0 in a limited way as ever (linear history only, etc. etc.) when cvsps 3.0 is not available. As your version already knows how to detect the case where cvsps is too old to operate with it, I imagine it to be straight-forward to ship the old cvsimport under obscure name, git cvsimport--old or something, and spawn it from your version when necessary, perhaps after issuing a warning cvsps 3.0 not found; switching to an old and unmaintained version of cvsimport... That way, people who have been happily working with linear CVS histories with the old limited tool can keep using the same set-up until their distro update their cvsps, without harming people who need to work with more complex CVS histories, who can choose to update their cvsps early themselves as $HOME/bin/cvsps earlier on their $PATH. By cvsimport (the current version), we are talking about a piece of software that has been used in the field for more than 5 years, still with a handful of patches to enhance it in the past two years. A flag-day this hot-off-the-press version is infinitely better replacement is not an option, especially when we can expect that existing users are not asking for an inifinitely better version (they rather prefer stable in the works just as before sense), even when the hot-off-the-press version *is* infinitely better in some use cases such as dealing with branchy histories. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond e...@thyrsus.com writes: Junio C Hamano gits...@pobox.com: As your version already knows how to detect the case where cvsps is too old to operate with it, I imagine it to be straight-forward to ship the old cvsimport under obscure name, git cvsimport--old or something, and spawn it from your version when necessary, perhaps after issuing a warning cvsps 3.0 not found; switching to an old and unmaintained version of cvsimport... This can be done. As this may not be the last case in which it comes up, perhaps we should have an 'obsolete' directory distinct from 'contrib'. I'll ship another patch. Alright; thanks. Don't forget to sign-off your patch ;-) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
On Wed, Jan 2, 2013 at 11:41 AM, Eric S. Raymond e...@thyrsus.com wrote: Martin Langhoff martin.langh...@gmail.com: Replacement with something more solid is welcome, but until you are extremely confident of its handling of legacy setups... I would still provide the old cvsimport, perhaps in contrib. I am extremely confident. I built a test suite so I could be. This is rather off-putting. Really. I dealt with enough CVS repos to see that the branch point could be ambiguous, and that some cases were incurably ugly and ambiguous. Off the top of my head I can recall - Files created on a branch appear on HEAD (if the cvs client was well behaved, in HEAD's attic, if the cvs client was buggy... ) - Files tagged with the branch at a much later time. Scenario is a developer opening/tagging a new branch mindlessly on a partial checkout; then trying to fix the problem later. My best guess is that you haven't dealt with enough ugly CVS repos. I used to have the old original X.org repos, but no more. Surely Mozilla's fugly old CVS repos are up somewhere, and may be therapeutic. cheers, m -- martin.langh...@gmail.com mar...@laptop.org -- Software Architect - OLPC - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Martin Langhoff martin.langh...@gmail.com: I dealt with enough CVS repos to see that the branch point could be ambiguous, and that some cases were incurably ugly and ambiguous. You are quite right, but you have misintepreted the subject of my confidence. I am under no illusion that the new cvsimport/cvsps pair is a perfect solution to the CVS-lifting problem, nor even that such a solution is possible. My best guess is that you haven't dealt with enough ugly CVS repos. I used to have the old original X.org repos, but no more. Surely Mozilla's fugly old CVS repos are up somewhere, and may be therapeutic. Thanks, but since I wrote reposurgeon in 2010 I've done more conversions of messy CVS and Subversion repositories than I can easily remember (the Subversion ones being relevant because they often have truly nasty CVS artifacts in their early history). Just off the top of my head there's been gpsd, the Network Utility Tools, Roundup, SSTK2000, the Hercules project, and robotfindskitten. And a raft of smaller projects - I sought them out as torture tests for reposurgeon. I am therefore intimately, painfully familiar with how bad CVS repos can get. I take it as given that there are still boojums that will perplex my tools lurking out there in the unexplored jungle. In fact, this very kind of prior experience had been a major motivation for reposurgeon. I became convinced several years back that the batchy design philosophy of conventional repo-conversion tools was flawed, not flexible enough to deal with the real-world messes out there. So I wrote reposurgeon to amplify human judgment rather than try to replace it. An example of the batchiness mistake close to home is the -m and -M options in the old version of cvsimport. It takes human judgment looking at the whole commit DAG in gitspace to decide what merge points would best express the (as you say, sometimes ambiguous) CVS history - what's needed is a scalpel and sutures in a surgeon's hand, not a regexp hammer. For extended discussion, see my blog post Repositories In Translation at http://esr.ibiblio.org/?p=3859 in which I argue that the process has much more in common with the ambiguity of literary translation than is normally understood. No, what I am very confident about is the performance and stability of the new cvsps/cvsimport code on *the cases the old code handled* - and a fairly well-defined larger group of many more cases. My confidence is derived from having built a test suite that incorporates and improves on the git-tree tests. I don't have to merely guess or hope that the new code works better, I can exhibit tests that demonstrate it. Among my near-term to-do items are applying those tests to cvs2git and parsecvs. But I first need to get parsecvs working again; presently, as I've inherited it, it does not correctly create a HEAD reference in the translated git repo. -- a href=http://www.catb.org/~esr/;Eric S. Raymond/a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
On Wed, Jan 2, 2013 at 5:28 PM, Eric S. Raymond e...@thyrsus.com wrote: Martin Langhoff martin.langh...@gmail.com: I dealt with enough CVS repos to see that the branch point could be ambiguous, and that some cases were incurably ugly and ambiguous. You are quite right, but you have misintepreted the subject of my confidence. I am under no illusion that the new cvsimport/cvsps pair is a perfect solution to the CVS-lifting problem, nor even that such a solution is possible. Thanks. That is a much more reassuring stance. m -- martin.langh...@gmail.com mar...@laptop.org -- Software Architect - OLPC - ask interesting questions - don't get distracted with shiny stuff - working code first - http://wiki.laptop.org/go/User:Martinlanghoff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
On Tue, Jan 1, 2013 at 11:26 AM, Eric S. Raymond e...@thyrsus.com wrote: diff --git a/git-cvsimport.py b/git-cvsimport.py new file mode 100755 index 000..6407e8a --- /dev/null +++ b/git-cvsimport.py @@ -0,0 +1,342 @@ +#!/usr/bin/env python +# +# Import CVS history into git +# +# Intended to be a near-workalike of Matthias Urlichs's Perl implementation. +# +# By Eric S. Raymond e...@thyrsus.com, December 2012 +# May be redistributed under the license of the git project. + +import sys + +if sys.hexversion 0x0206: +sys.stderr.write(git cvsimport: requires Python 2.6 or later.\n) +sys.exit(1) + +import os, getopt, subprocess, tempfile, shutil + +DEBUG_COMMANDS = 1 + +class Fatal(Exception): +Unrecoverable error. +def __init__(self, msg): +Exception.__init__(self) +self.msg = msg + +def do_or_die(dcmd, legend=): +Either execute a command or raise a fatal exception. +if legend: +legend =+ legend +if verbose = DEBUG_COMMANDS: +sys.stdout.write(git cvsimport: executing '%s'%s\n % (dcmd, legend)) +try: +retcode = subprocess.call(dcmd, shell=True) +if retcode 0: +raise Fatal(git cvsimport: child was terminated by signal %d. % -retcode) +elif retcode != 0: +raise Fatal(git cvsimport: child returned %d. % retcode) +except (OSError, IOError) as e: +raise Fatal(git cvsimport: execution of %s%s failed: %s % (dcmd, legend, e)) + +def capture_or_die(dcmd, legend=): +Either execute a command and capture its output or die. +if legend: +legend =+ legend +if verbose = DEBUG_COMMANDS: +sys.stdout.write(git cvsimport: executing '%s'%s\n % (dcmd, legend)) +try: +return subprocess.check_output(dcmd, shell=True) +except subprocess.CalledProcessError as e: +if e.returncode 0: +sys.stderr.write(git cvsimport: child was terminated by signal %d. % -e.returncode) +elif e.returncode != 0: +sys.stderr.write(git cvsimport: child returned %d. % e.returncode) +sys.exit(1) + +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 +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 +def set_exclusion(self, val): +Set a file exclusion regexp. +self.opts += -n -f '%s' % val +def set_after(self, val): +Set a date threshold for incremental import. +self.opts += -d '%s' % val +def set_revmap(self, val): +Set the file to which the engine should dump a reference map. +self.revmap = val +self.opts += -R '%s' % self.revmap +def set_module(self, val): +Set the module to query. +self.opts += + val +def command(self): +Emit the command implied by all previous options. +return cvsps --fast-export + self.opts + +class cvs2git: +Method class for cvs2git back end. +def __init__(self): +self.opts = +self.modulepath = . +def set_authormap(self, _val): +Set the author-map file. +sys.stderr.write(git cvsimport: author maping is not supported with cvs2git.\n) +sys.exit(1) +def set_repo(self, _val): +Set the repository root option. +sys.stderr.write(git cvsimport: cvs2git must run within a repository checkout directory.\n) +sys.exit(1) +def set_fuzz(self, _val): +Set the commit-similarity window. +sys.stderr.write(git cvsimport: fuzz setting is not supported with cvs2git.\n) +sys.exit(1) +def set_nokeywords(self): +Suppress CVS keyword expansion. +self.opts += --keywords-off +def add_opts(self, val): +Add options to the engine command line. +self.opts += + val +def set_exclusion(self, val): +Set a file exclusion regexp. +self.opts += --exclude='%s' % val +def set_after(self, _val): +Set a date threshold for incremental import. +sys.stderr.write(git cvsimport: incremental import is not supported with cvs2git.\n) +sys.exit(1) +def set_revmap(self, _val): +Set the file
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Chris Rorvick ch...@rorvick.com writes: outdir needs to be quoted in the formatted string, i.e.: %s | (cd '%s' /dev/null ... The issue is real, but I am afraid that the above is not sufficient because outdir can contain single quotes. I think other places that call out to external processes share the same issue of being careless about quoting in general. Doesn't Python come with a standard subprocess module that lets you spawn external programs safely, similar to the way Perl's list form open(), e.g. open($fh, -|, 'git', @args), works? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Doesn't Python come with a standard subprocess module that lets you spawn external programs safely, similar to the way Perl's list form open(), e.g. open($fh, -|, 'git', @args), works? You mean something like this: p1 = subprocess.Popen([backend.command()], stdout=subprocess.PIPE) subprocess.Popen([git, fast-import, --quiet] + gitopts, cwd=outdir, stdin=p1.stdout) Assuming gitopts is a list rather than a string. (care must be taken with backend.command() also) -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond e...@thyrsus.com writes: The combination of git-cvsimport and cvsps had serious problems. Among these were: (1) Analysis of branchy repos was buggy in multiple ways in both programs, leading to incorrect repo translations. (2) Even after a correct branch analysis, extra (redundant) fileops would often be generated on the new-branch side. (3) Inability to report more than one tag pointing to the same revision. (4) Failure in certain cases of clock-skew reported by the t9603 test. (5) Failure to use commitids for changeset ordering in cases were this would have prevented clock skew from causing incorrect grouping. Problems 2-5 and portions of problem 1 have been solved by a major rewrite of cvsps (the 3.x release series); it now emits a git fast-import stream. So..., is this a flag-day patch? After this is merged, users who have been interoperating with CVS repositories with the older cvsps have to install the updated cvsps before using a new version of Git that ships with it? As long as they update both cvsps and cvsimport, they can continue using the existing repository to get updates from the same upstream CVS repository without losing hisory continuity? I would have preferred an addition of git cvsimport-new (or rename of the existing one to git cvsimport-old), with additional tests that compare the results of these two implemenations on simple CVS history that cvsimport-old did *not* screw up, to ensure that (1) people with existing set-up can choose to keep using the old one, perhaps by tweaking their process to use cvsimport-old, and (2) the updated one will give these people the identical conversion results, as long as the history they have been interacting with do not have the corner cases that trigger bugs in older cvsps. Or am I being too conservative? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Junio C Hamano gits...@pobox.com: So..., is this a flag-day patch? After this is merged, users who have been interoperating with CVS repositories with the older cvsps have to install the updated cvsps before using a new version of Git that ships with it? Yes, they must install an updated cvsps. But this is hardly a loss, as the old version was perilously broken. There was an error or typo in the branch-analysis code, dating from 2006 and possibly earlier, that meant that branch root points would almost always be attributed to parent patchsets one patchset earlier than they should have been. Shocked me when I found it - how was this missed for six years? Because of the way the analysis is done, this fundamental bug would also cause secondary damage like file changes near the root point getting attributed to the wrong branch. In fact, this is how I first spotted the problem; my test suite exhibited this symptom. And mind you this is on top of ancestry-branch tracking not working - two separate bugs that could interact in ways I'd really rather not think about. The bottom line is that every import of a branchy CVS repo with a pre-3.x version of cvsps is probably wrong. The old git-cvsimport code was doing its part to screw things up, too. At least three of the bugs on its manual page are problems I couldn't reproduce using a bare cvsps instance, even the old broken version. As long as they update both cvsps and cvsimport, they can continue using the existing repository to get updates from the same upstream CVS repository without losing hisory continuity? Yes, but in that case I would strongly advise re-importing the entire CVS history, as the portion analyzed with 2.2b1 and earlier versions of cvsps will almost certainly have been somewhat garbled if it contains any branches. I would have preferred an addition of git cvsimport-new (or rename of the existing one to git cvsimport-old), with additional tests that compare the results of these two implemenations on simple CVS history that cvsimport-old did *not* screw up, to ensure that (1) people with existing set-up can choose to keep using the old one, perhaps by tweaking their process to use cvsimport-old, and (2) the updated one will give these people the identical conversion results, as long as the history they have been interacting with do not have the corner cases that trigger bugs in older cvsps. Or am I being too conservative? I think you are being too conservative. This patch is *not* a mere feature upgrade. The branch-analysis bug I found three days ago is not a minor problem, it is a big ugly showstopper for any case beside the simplest linear histories. Only linear histories will not break. 'People with existing set-ups' should absolutely *not* 'keep using the old one'; we should yank that choice away from them and get the old cvsimport/cvsps pair out of use *as fast as possible*, because it silently mangles branchy imports. Accordingly, giving people the idea that it's OK to use old and new versions in parallel would be an extremely bad idea. I would go so far as to call it irresponsible. Here is what I have done to ease the transition: If you try to use old git-cvsimport with new cvsps, new cvsps will detect this and ship a message to stderr telling you to upgrade If you try to use new git-cvsimport with old cvsps, old cvsps will complain of an invalid argument and git-cvsimport will quit. As for testing...cvsps now has several dozen self-tests on five different CVS repositories, including improved versions of the t960[123] tests. I will keep developing these as I work on bringing parsecvs up to snuff. I don't think there is a lot of point in git-cvsimport having its own tests any more. If you read it I think you'll see why; it's a much thinner wrapper around the conversion engine(s) than it used to be. In particular, it no longer does its own protocol transactions to the CVS server. -- a href=http://www.catb.org/~esr/;Eric S. Raymond/a -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Replace git-cvsimport with a rewrite that fixes major bugs.
Eric S. Raymond e...@thyrsus.com writes: Junio C Hamano gits...@pobox.com: So..., is this a flag-day patch? After this is merged, users who have been interoperating with CVS repositories with the older cvsps have to install the updated cvsps before using a new version of Git that ships with it? Yes, they must install an updated cvsps. But this is hardly a loss, as the old version was perilously broken. There was an error or typo in the branch-analysis code, dating from 2006 and possibly earlier, that meant that branch root points would almost always be attributed to parent patchsets one patchset earlier than they should have been. Shocked me when I found it - how was this missed for six years? Would it be that not many people use branchy history in CVS, as merging there was s painful? Or am I being too conservative? I think you are being too conservative. This patch is *not* a mere feature upgrade. The branch-analysis bug I found three days ago is not a minor problem, it is a big ugly showstopper for any case beside the simplest linear histories. Only linear histories will not break. That is exactly my point. It never worked in a branchy history, and that is an indication that people who didn't complain and used the old cvsimport with branch-incapable cvsps happily would have been working with a linear history. Either nobody uses cvsimport in the daily work, in which case a flag-day is perfectly fine, or we will have many people who are forced to update to unproven version for no immediate upside because the upstream repositories they work with, or options they use cvsimport, do not trigger the multi-branch bug. I however do understand that updating is the only sensible thing to do for them *in the longer term*, as older cvsimport and cvsps are no longer maintained, and sooner they update the better the chance the new cvsimport becomes perfect earlier. 'People with existing set-ups' should absolutely *not* 'keep using the old one'; we should yank that choice away from them and get the old cvsimport/cvsps pair out of use *as fast as possible*, because it silently mangles branchy imports. I still am not convinced, especially without a we make sure we do not regress in linear histories side-by-side test in place. That sounds irresponsible. But others may disagree, and I'd have to sleep on it. I'd prefer to hear from somebody who is *not* defending on his newer implementation, but from somebody who is actively using cvsimport as an end user. On the end-users' side, there always is this anxiety that a radical rewrite will always introduce new bugs, even when they know the rewrite is done very competently. Here is what I have done to ease the transition: If you try to use old git-cvsimport with new cvsps, new cvsps will detect this and ship a message to stderr telling you to upgrade Sounds sensible. If you try to use new git-cvsimport with old cvsps, old cvsps will complain of an invalid argument and git-cvsimport will quit. With an error message that tells the user to update cvsps, this also sounds sensible. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
