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
1=== modified file 'Makefile'
2--- Makefile 2006-12-06 20:50:19 +0000
3+++ Makefile 2009-08-02 22:13:26 +0000
4@@ -15,12 +15,14 @@
5
6 build:
7
8-check: check-python
9- PYTHONPATH=$(PYTHONPATH) $(MAKE) -C tests check_nonpython
10+check: check-python check-nonpython
11
12 check-python:
13 PYTHONPATH=$(PYTHONPATH) $(PYTHON) test_all.py "$$TESTFILTER"
14
15+check-nonpython:
16+ PYTHONPATH=$(PYTHONPATH) $(MAKE) -C tests check_nonpython
17+
18 find_py_xargs_n1:=\
19 find . -name .bzr -prune -o -name '*.py' -print0 | xargs -0 -n1
20
21
22=== modified file 'modules/CVS/StorageLayer.py'
23--- modules/CVS/StorageLayer.py 2006-12-06 20:50:19 +0000
24+++ modules/CVS/StorageLayer.py 2009-08-03 01:31:02 +0000
25@@ -922,7 +922,7 @@
26 def apply(self, logger, target_tree, prune):
27 """apply this change to a target tree.
28 FIXME, currently we just mutate the target to match."""
29- if (self.predecessor != None and
30+ if (self.predecessor is None or
31 self.predecessor.type() in (Revision.PLACEHOLDER,
32 Revision.REMOVE)):
33 logger.info("nb harmless for now : FIXME: patch should never need to add %s %s", self.filename, self.revision)
34
35=== modified file 'tests/Makefile'
36--- tests/Makefile 2006-07-21 12:20:00 +0000
37+++ tests/Makefile 2009-08-03 05:57:42 +0000
38@@ -1,6 +1,6 @@
39 # TODO autoconfiscate me
40 #
41-TESTS=test-build test-branch test-head
42+TESTS=test-build test-branch test-head test-merge-creates-file test-delete-readd
43 .PHONY: all $(TESTS) unittests
44 all:
45
46@@ -18,6 +18,12 @@
47 ./test-dup-branch.sh
48 ./test-deleted-branch.sh
49
50+test-merge-creates-file:
51+ ./test-merge-creates-file.sh
52+
53+test-delete-readd:
54+ ./test-delete-readd.sh
55+
56 test-head:
57 #./test-head-1-1-dead.sh # FIXME: failing, temporarily disabled
58 ./test-head-2-dead.sh
59
60=== added file 'tests/test-delete-readd.sh'
61--- tests/test-delete-readd.sh 1970-01-01 00:00:00 +0000
62+++ tests/test-delete-readd.sh 2009-08-03 01:29:44 +0000
63@@ -0,0 +1,83 @@
64+#!/bin/sh
65+
66+set -e
67+set -x
68+
69+. ./test-support.fns
70+
71+BASE=`pwd`
72+export BZR_HOME=$BASE/working
73+export TZ=UTC
74+
75+cvs_create_repo_and_module ()
76+{
77+ mkdir -p $BASE/working/import
78+ cd $BASE/working/import
79+ touch initial
80+ cvs import -m '' module vendor release
81+ cd $BASE/working
82+ cvs co module
83+ return 0
84+}
85+
86+cvs_create_delete_file ()
87+{
88+ cd $BASE/working/module
89+ touch file
90+ cvs add file
91+ cvs commit -m 'add file'
92+ rm file
93+ cvs rm file
94+ cvs commit -m 'remove file again'
95+}
96+
97+cvs_commit_to_main ()
98+{
99+ cd $BASE/working/module
100+ echo blah > initial
101+ cvs commit -m 'change to initial'
102+}
103+
104+cvs_readd_file ()
105+{
106+ cd $BASE/working/module
107+ touch file
108+ cvs add file
109+ cvs commit -m 'add file again'
110+}
111+
112+initial_import ()
113+{
114+ cd $BASE/working/module
115+ bzr init ../bzrworking
116+ cscvs totla --strict -b ../bzrworking -SI MAIN.1 ../bzrworking
117+}
118+
119+update_import ()
120+{
121+ cd $BASE/working/module
122+ cscvs totla --strict -b ../bzrworking -SC $(bzr revno ../bzrworking):: ../bzrworking
123+}
124+
125+cleanup
126+setup_repo
127+export CVSROOT=$REPOSITORY
128+cvs_create_repo_and_module
129+cvs_create_delete_file
130+
131+# Now we need to pretend that everything that's happened in the cvs
132+# repository so far happened at least a week ago. This is done by
133+# hacking the ,v files with sed(!).
134+cd $BASE/working/cvs
135+find . -name '*,v' -print0 | xargs -0 sed -ie 's/^date\t[0-9]\{4\}\.[0-9]\{2\}\.[0-9]\{2\}\./date\t2007.01.01./'
136+initial_import
137+# We need to make sure that cscvs has seen at least one update recently.
138+cvs_commit_to_main
139+update_import
140+# Now, when the branch is merged, cscvs should recognize that the file
141+# needs to be added again, not merely patched.
142+cvs_readd_file
143+update_import
144+cd $BASE
145+cleanup
146+exit 0
147
148=== added file 'tests/test-merge-creates-file.sh'
149--- tests/test-merge-creates-file.sh 1970-01-01 00:00:00 +0000
150+++ tests/test-merge-creates-file.sh 2009-08-03 01:29:44 +0000
151@@ -0,0 +1,84 @@
152+#!/bin/sh
153+
154+set -e
155+set -x
156+
157+. ./test-support.fns
158+
159+BASE=`pwd`
160+export BZR_HOME=$BASE/working
161+export TZ=UTC
162+
163+cvs_create_repo_and_module ()
164+{
165+ mkdir -p $BASE/working/import
166+ cd $BASE/working/import
167+ touch initial
168+ cvs import -m '' module vendor release
169+ cd $BASE/working
170+ cvs co module
171+ return 0
172+}
173+
174+cvs_create_branch_with_new_file ()
175+{
176+ cd $BASE/working/module
177+ cvs tag -b branchtag
178+ cvs up -r branchtag
179+ touch branch-file
180+ cvs add branch-file
181+ cvs commit -m 'add branch-file'
182+ cvs update -A
183+}
184+
185+cvs_commit_to_main ()
186+{
187+ cd $BASE/working/module
188+ echo blah > initial
189+ cvs commit -m 'change on main'
190+}
191+
192+cvs_merge_branch ()
193+{
194+ cd $BASE/working/module
195+ cvs update -jbranchtag
196+ # I don't undertand what this file is doing here.
197+ rm .#initial.1.2
198+ cvs commit -m 'merge branch'
199+}
200+
201+initial_import ()
202+{
203+ cd $BASE/working/module
204+ bzr init ../bzrworking
205+ cscvs totla --strict -b ../bzrworking -SI MAIN.1 ../bzrworking
206+}
207+
208+update_import ()
209+{
210+ cd $BASE/working/module
211+ cscvs totla --strict -b ../bzrworking -SC $(bzr revno ../bzrworking):: ../bzrworking
212+}
213+
214+cleanup
215+setup_repo
216+export CVSROOT=$REPOSITORY
217+cvs_create_repo_and_module
218+cvs_create_branch_with_new_file
219+
220+# Now we need to pretend that everything that's happened in the cvs
221+# repository so far happened at least a week ago. This is done by
222+# hacking the ,v files with sed(!).
223+cd $BASE/working/cvs
224+find . -name '*,v' -print0 | xargs -0 sed -ie 's/^date\t[0-9]\{4\}\.[0-9]\{2\}\.[0-9]\{2\}\./date\t2007.01.01./'
225+initial_import
226+# We need to make sure that cscvs has seen at least one update recently.
227+cvs_commit_to_main
228+update_import
229+# Now, when the branch is merged, cscvs should recognize that the file
230+# needs to be added, not merely patched.
231+cvs_merge_branch
232+update_import
233+cd $BASE
234+cleanup
235+exit 0

Subscribers

People subscribed via source and target branches