Launchpad CSCVS

Merge lp:~mwhudson/launchpad-cscvs/merge-creates-file-bug-120977-attempt-2 into lp:launchpad-cscvs

  1. merge-creates-file-bug-120977-attempt-2
  2. Merge into devel
Proposed by Michael Hudson-Doyle
Status: Merged
Merged at revision: not available
Proposed branch: lp:~mwhudson/launchpad-cscvs/merge-creates-file-bug-120977-attempt-2
Merge into: lp:launchpad-cscvs
Diff against target: None lines
To merge this branch: bzr merge lp:~mwhudson/launchpad-cscvs/merge-creates-file-bug-120977-attempt-2
Reviewer Review Type Date Requested Status
Tim Penhey (community) Approve
Review via email: mp+9573@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Michael Hudson-Doyle (mwhudson) wrote :

Hi there,

This bug is the predictably tiny (a few characters!) fix for the infamous linked cvs-import-killing bug.

It turns out that the issue is rather similar to the add-delete-readd case, so the way I've fixed it very much matches how that is handled. The way this case is handled is labelled as a hack in the source, but my attempt at fixing it "properly" made the process of building the catalog more than 10 times slower, and I don't understand the catalog building code well enough to spot a better fix.

There is no unit test coverage for this code (making the patched method error unconditionally only makes two unit tests fail!) but I've added two acceptance tests that exercise this code from the shell (one exercises the add/delete/readd case and the other the case from the bug). They're quite confusing, but commented. Please point out areas that are still too confusing.

Cheers,
mwh

Revision history for this message
Tim Penhey (thumper) wrote :

This actually looks pretty good to me.

review: Approve
Revision history for this message
Robert Collins (lifeless) wrote :

The core change looks plausible to me. != None is a bad idea anyhow. /me looks at himself.

I'd write the acceptance tests in python, myself. They are more managable and introspectable. I long ago (4 years? maybe more?) stopped adding shell tests.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'Makefile'
--- Makefile 2006-12-06 20:50:19 +0000
+++ Makefile 2009-08-02 22:13:26 +0000
@@ -15,12 +15,14 @@
1515
16build:16build:
1717
18check: check-python18check: check-python check-nonpython
19 PYTHONPATH=$(PYTHONPATH) $(MAKE) -C tests check_nonpython
2019
21check-python:20check-python:
22 PYTHONPATH=$(PYTHONPATH) $(PYTHON) test_all.py "$$TESTFILTER"21 PYTHONPATH=$(PYTHONPATH) $(PYTHON) test_all.py "$$TESTFILTER"
2322
23check-nonpython:
24 PYTHONPATH=$(PYTHONPATH) $(MAKE) -C tests check_nonpython
25
24find_py_xargs_n1:=\26find_py_xargs_n1:=\
25find . -name .bzr -prune -o -name '*.py' -print0 | xargs -0 -n127find . -name .bzr -prune -o -name '*.py' -print0 | xargs -0 -n1
2628
2729
=== modified file 'modules/CVS/StorageLayer.py'
--- modules/CVS/StorageLayer.py 2006-12-06 20:50:19 +0000
+++ modules/CVS/StorageLayer.py 2009-08-03 01:31:02 +0000
@@ -922,7 +922,7 @@
922 def apply(self, logger, target_tree, prune):922 def apply(self, logger, target_tree, prune):
923 """apply this change to a target tree.923 """apply this change to a target tree.
924 FIXME, currently we just mutate the target to match."""924 FIXME, currently we just mutate the target to match."""
925 if (self.predecessor != None and925 if (self.predecessor is None or
926 self.predecessor.type() in (Revision.PLACEHOLDER,926 self.predecessor.type() in (Revision.PLACEHOLDER,
927 Revision.REMOVE)):927 Revision.REMOVE)):
928 logger.info("nb harmless for now : FIXME: patch should never need to add %s %s", self.filename, self.revision)928 logger.info("nb harmless for now : FIXME: patch should never need to add %s %s", self.filename, self.revision)
929929
=== modified file 'tests/Makefile'
--- tests/Makefile 2006-07-21 12:20:00 +0000
+++ tests/Makefile 2009-08-03 05:57:42 +0000
@@ -1,6 +1,6 @@
1# TODO autoconfiscate me1# TODO autoconfiscate me
2#2#
3TESTS=test-build test-branch test-head3TESTS=test-build test-branch test-head test-merge-creates-file test-delete-readd
4.PHONY: all $(TESTS) unittests4.PHONY: all $(TESTS) unittests
5all:5all:
66
@@ -18,6 +18,12 @@
18 ./test-dup-branch.sh18 ./test-dup-branch.sh
19 ./test-deleted-branch.sh19 ./test-deleted-branch.sh
2020
21test-merge-creates-file:
22 ./test-merge-creates-file.sh
23
24test-delete-readd:
25 ./test-delete-readd.sh
26
21test-head:27test-head:
22 #./test-head-1-1-dead.sh # FIXME: failing, temporarily disabled28 #./test-head-1-1-dead.sh # FIXME: failing, temporarily disabled
23 ./test-head-2-dead.sh29 ./test-head-2-dead.sh
2430
=== added file 'tests/test-delete-readd.sh'
--- tests/test-delete-readd.sh 1970-01-01 00:00:00 +0000
+++ tests/test-delete-readd.sh 2009-08-03 01:29:44 +0000
@@ -0,0 +1,83 @@
1#!/bin/sh
2
3set -e
4set -x
5
6. ./test-support.fns
7
8BASE=`pwd`
9export BZR_HOME=$BASE/working
10export TZ=UTC
11
12cvs_create_repo_and_module ()
13{
14 mkdir -p $BASE/working/import
15 cd $BASE/working/import
16 touch initial
17 cvs import -m '' module vendor release
18 cd $BASE/working
19 cvs co module
20 return 0
21}
22
23cvs_create_delete_file ()
24{
25 cd $BASE/working/module
26 touch file
27 cvs add file
28 cvs commit -m 'add file'
29 rm file
30 cvs rm file
31 cvs commit -m 'remove file again'
32}
33
34cvs_commit_to_main ()
35{
36 cd $BASE/working/module
37 echo blah > initial
38 cvs commit -m 'change to initial'
39}
40
41cvs_readd_file ()
42{
43 cd $BASE/working/module
44 touch file
45 cvs add file
46 cvs commit -m 'add file again'
47}
48
49initial_import ()
50{
51 cd $BASE/working/module
52 bzr init ../bzrworking
53 cscvs totla --strict -b ../bzrworking -SI MAIN.1 ../bzrworking
54}
55
56update_import ()
57{
58 cd $BASE/working/module
59 cscvs totla --strict -b ../bzrworking -SC $(bzr revno ../bzrworking):: ../bzrworking
60}
61
62cleanup
63setup_repo
64export CVSROOT=$REPOSITORY
65cvs_create_repo_and_module
66cvs_create_delete_file
67
68# Now we need to pretend that everything that's happened in the cvs
69# repository so far happened at least a week ago. This is done by
70# hacking the ,v files with sed(!).
71cd $BASE/working/cvs
72find . -name '*,v' -print0 | xargs -0 sed -ie 's/^date\t[0-9]\{4\}\.[0-9]\{2\}\.[0-9]\{2\}\./date\t2007.01.01./'
73initial_import
74# We need to make sure that cscvs has seen at least one update recently.
75cvs_commit_to_main
76update_import
77# Now, when the branch is merged, cscvs should recognize that the file
78# needs to be added again, not merely patched.
79cvs_readd_file
80update_import
81cd $BASE
82cleanup
83exit 0
084
=== added file 'tests/test-merge-creates-file.sh'
--- tests/test-merge-creates-file.sh 1970-01-01 00:00:00 +0000
+++ tests/test-merge-creates-file.sh 2009-08-03 01:29:44 +0000
@@ -0,0 +1,84 @@
1#!/bin/sh
2
3set -e
4set -x
5
6. ./test-support.fns
7
8BASE=`pwd`
9export BZR_HOME=$BASE/working
10export TZ=UTC
11
12cvs_create_repo_and_module ()
13{
14 mkdir -p $BASE/working/import
15 cd $BASE/working/import
16 touch initial
17 cvs import -m '' module vendor release
18 cd $BASE/working
19 cvs co module
20 return 0
21}
22
23cvs_create_branch_with_new_file ()
24{
25 cd $BASE/working/module
26 cvs tag -b branchtag
27 cvs up -r branchtag
28 touch branch-file
29 cvs add branch-file
30 cvs commit -m 'add branch-file'
31 cvs update -A
32}
33
34cvs_commit_to_main ()
35{
36 cd $BASE/working/module
37 echo blah > initial
38 cvs commit -m 'change on main'
39}
40
41cvs_merge_branch ()
42{
43 cd $BASE/working/module
44 cvs update -jbranchtag
45 # I don't undertand what this file is doing here.
46 rm .#initial.1.2
47 cvs commit -m 'merge branch'
48}
49
50initial_import ()
51{
52 cd $BASE/working/module
53 bzr init ../bzrworking
54 cscvs totla --strict -b ../bzrworking -SI MAIN.1 ../bzrworking
55}
56
57update_import ()
58{
59 cd $BASE/working/module
60 cscvs totla --strict -b ../bzrworking -SC $(bzr revno ../bzrworking):: ../bzrworking
61}
62
63cleanup
64setup_repo
65export CVSROOT=$REPOSITORY
66cvs_create_repo_and_module
67cvs_create_branch_with_new_file
68
69# Now we need to pretend that everything that's happened in the cvs
70# repository so far happened at least a week ago. This is done by
71# hacking the ,v files with sed(!).
72cd $BASE/working/cvs
73find . -name '*,v' -print0 | xargs -0 sed -ie 's/^date\t[0-9]\{4\}\.[0-9]\{2\}\.[0-9]\{2\}\./date\t2007.01.01./'
74initial_import
75# We need to make sure that cscvs has seen at least one update recently.
76cvs_commit_to_main
77update_import
78# Now, when the branch is merged, cscvs should recognize that the file
79# needs to be added, not merely patched.
80cvs_merge_branch
81update_import
82cd $BASE
83cleanup
84exit 0

Subscribers

People subscribed via source and target branches