bzr-xmloutput

Merge lp:~piastucki/bzr-xmloutput/xml-log-fix into lp:bzr-xmloutput

  1. xml-log-fix
  2. Merge into trunk
Proposed by Piotr Piastucki
Status: Merged
Approved by: Guillermo Gonzalez
Approved revision: 170
Merged at revision: 172
Proposed branch: lp:~piastucki/bzr-xmloutput/xml-log-fix
Merge into: lp:bzr-xmloutput
Diff against target: 252 lines (+48/-108)
2 files modified
logxml.py (+26/-92)
tests/test_log_xml.py (+22/-16)
To merge this branch: bzr merge lp:~piastucki/bzr-xmloutput/xml-log-fix
Reviewer Review Type Date Requested Status
Guillermo Gonzalez Approve
Review via email: mp+154886@code.launchpad.net

Description of the change

The branch contains the following changes:
1) fixes for failing tests in test_log_xml.py module
2) refactoring of logxml.py module - since we already have a stack of open tags there does not seem to be any reason to keep additional flags/counters and logic to update them. I removed the counters completely and used the stack to properly close open tags.
3) added escaping for tag names

Changes in 2) should fix 517937 - tets case:
1) bzr branch lp:bzr-eclipse (rev 255)
2) cd bzr-eclispe
3) bzr xmllog --limit 230 org.vcs.bazaar.eclipse.ui/
-> malformed XML (http://piastucki.eu/log1.xml)
4) apply fix
5) bzr xmllog --limit 230 org.vcs.bazaar.eclipse.ui/
-> correct XML (http://piastucki.eu/log2.xml)

To post a comment you must log in.
lp:~piastucki/bzr-xmloutput/xml-log-fix updated
167. By Piotr Piastucki

Fix failing tests in test_log_xml

168. By Piotr Piastucki

Refactor and simplify logxml.
Remove unneeded counters and flags as having a stack of open tags is enough.

169. By Piotr Piastucki

Make sure merge tag is inside log tag

170. By Piotr Piastucki

Avoid closing log in case of nested merges

Revision history for this message
Guillermo Gonzalez (verterok) wrote :

Thanks for fixing this.

It would be ideal to have a testcase to reproduce the bad xml, but I wasn't able to write one yet (can't reproduce the nested merges in the test to trigger this condition).

Please let me know if can do it, if not I'll try to ask one of the bzr gurus @ #bzr.

Cheers,

Revision history for this message
Piotr Piastucki (piastucki) wrote :
Download full text (5.0 KiB)

Actually I think this is a combination of nested merges, selected resources and --limit option. The issue is observed when some inner merges affecting the given resource are removed from the result due to --limit option.
I was not able to reproduce it with a simple test case but the following scenario is relatively simple:
1) bzr branch lp:bzr-eclipse -r 213
2) cd bzr-eclipse
3) bzr xmllog --include-merged --limit 5 org.vcs.bazaar.eclipse.ui/ -> invalid xml

It looks like a bug in bzr.
Try the following bzr log commands in the above mentioned scenario:
1) bzr log --include-merged --limit 7
2) bzr log --include-merged --limit 5 org.vcs.bazaar.eclipse.ui/
See the results attached below and note that revision 211.1.2 is missing in the second case.
I hope this results can be used by some bzr gurus to diagnose the problem.
However, the proposed patch simplifies the code in bzr-xmloutput and IMHO is still worth applying even if the actual issue is fixed somehow in bzr.

1) bzr log --include-merged --limit 7
------------------------------------------------------------
revno: 213 [merge]
committer: Guillermo Gonzalez <email address hidden>
branch nick: trunk
timestamp: Thu 2009-07-09 17:40:43 -0300
message:
  merge lp:~verterok/bzr-eclipse/add-ignore
    ------------------------------------------------------------
    revno: 212.1.4
    committer: Guillermo Gonzalez <email address hidden>
    branch nick: add-ignore
    timestamp: Thu 2009-07-09 17:29:33 -0300
    message:
      fix IgnoredCommand to be compatible with eclipse >= 3.3
      include commons-logging in core.tests classpath
    ------------------------------------------------------------
    revno: 212.1.3
    committer: Guillermo Gonzalez <email address hidden>
    branch nick: add-ignore
    timestamp: Thu 2009-07-09 15:33:49 -0300
    message:
      update bzr-java-lib deps
    ------------------------------------------------------------
    revno: 212.1.2
    committer: Guillermo Gonzalez <email address hidden>
    branch nick: add-ignore
    timestamp: Thu 2009-07-09 02:51:20 -0300
    message:
      Use IResource and IBzrResource while handling Ignore/d actions, all resources handling is now in the core
      IgnoredCommand.getIgnored() now returns a Map<IResource, String>
      Fix ignored and ignore (commands and actions) to handle projects that aren't the branch root
    ------------------------------------------------------------
    revno: 212.1.1 [merge]
    committer: Guillermo Gonzalez <email address hidden>
    branch nick: add-ignore
    timestamp: Wed 2009-07-01 00:52:46 -0300
    message:
      merge lp:~javierder/bzr-eclipse/add-ignore
        ------------------------------------------------------------
        revno: 211.1.2 [merge]
        committer: Javier Derderian <email address hidden>
        branch nick: bzr-eclipse-tomerge
        timestamp: Mon 2009-06-01 12:52:23 -0300
        message:
          Merged with lp:~javierder/bzr-eclipse/add-ignore
            ------------------------------------------------------------
            revno: 210.4.6 [merge]
            committer: Javier Derderian <email address hidden>
            branch nick: bzr-eclipse-...

Read more...

Revision history for this message
Guillermo Gonzalez (verterok) wrote :

Thanks for working on this.

I'll talk with the bzr devs to see if this is a bzr core issue.

Cheers

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'logxml.py'
2--- logxml.py 2011-12-12 15:11:38 +0000
3+++ logxml.py 2013-03-22 16:31:28 +0000
4@@ -24,11 +24,8 @@
5 super(XMLLogFormatter, self).__init__(*args, **kwargs)
6 self.log_count = 0
7 self.start_with_merge = False
8- self.nested_merge_count = 0
9 self.previous_merge_depth = 0
10 self.debug_enabled = 'debug' in debug.debug_flags
11- self.open_logs = 0
12- self.open_merges = 0
13 self.stack = []
14
15 def show(self, revno, rev, delta, tags=None):
16@@ -47,89 +44,45 @@
17 self.__debug(revision)
18 actions = []
19 # to handle merge revision as childs
20- if revision.merge_depth > 0 and not self.start_with_merge:
21- if self.previous_merge_depth < revision.merge_depth:
22- if self.log_count > 0:
23- merge_depth_diference = revision.merge_depth - \
24- self.previous_merge_depth
25- for m in range(merge_depth_diference):
26- actions.append(self.__open_merge)
27- if merge_depth_diference > 1:
28- self.nested_merge_count += 1
29- elif self.log_count == 0:
30- # first log is inside a merge, we show it as a top level
31- # we could support a merge tag without parent log.
32- self.start_with_merge = True
33- elif self.previous_merge_depth > revision.merge_depth:
34- # TODO: testcase for more than one level of nested merges
35- actions.append({self.__close_merge:self.previous_merge_depth - \
36- revision.merge_depth})
37- if self.nested_merge_count > 0:
38- self.nested_merge_count -= 1
39- else:
40- actions.append(self.__close_log)
41- else:
42- if self.open_logs > 0:
43- actions.append(self.__close_log)
44- elif self.previous_merge_depth < revision.merge_depth:
45- actions.append({self.__close_merge:self.previous_merge_depth - \
46- revision.merge_depth})
47- if self.nested_merge_count > 0:
48- self.nested_merge_count -= 1
49- else:
50- actions.append(self.__close_log)
51- elif self.open_merges > 0:
52- actions.append({self.__close_merge:self.open_merges})
53- #actions.append(self.__close_merge)
54- actions.append(self.__close_log)
55- else:
56- actions.append(self.__close_log)
57- if self.start_with_merge:
58- # we only care about the first log, the following logs are
59- # handlend in the logic of nested merges
60- self.start_with_merge = False
61- for action in actions:
62- if type(action) == dict:
63- action.keys()[0](action[action.keys()[0]])
64- else:
65- action()
66+ if self.previous_merge_depth < revision.merge_depth:
67+ if self.log_count > 0:
68+ self.__open_merge(revision.merge_depth - self.previous_merge_depth)
69+ elif self.previous_merge_depth > revision.merge_depth:
70+ self.__close_merge(self.previous_merge_depth - revision.merge_depth)
71+ if len(self.stack) > 0 and self.stack[len(self.stack) - 1] == 'log':
72+ self.__close_log()
73+ else:
74+ self.__close_log()
75 self.__open_log()
76 self.__log_revision(revision)
77
78 self.log_count = self.log_count + 1
79 self.previous_merge_depth = revision.merge_depth
80
81- def __open_merge(self):
82- self.to_file.write('<merge>')
83- self.open_merges += 1
84- self.stack.append('merge')
85-
86- def __close_merge(self, num=1):
87- for item in self.stack.__reversed__():
88- self.to_file.write('</%s>' % item)
89- self.stack.pop()
90- if item == 'merge':
91- self.open_merges -= 1
92- num -= 1
93- if num == 0:
94- return
95- if item == 'log':
96- self.open_logs -= 1
97+ def __open_merge(self, levels):
98+ for i in range(levels):
99+ self.to_file.write('<merge>')
100+ self.stack.append('merge')
101
102 def __open_log(self):
103 self.to_file.write('<log>',)
104- self.open_logs = self.open_logs + 1
105 self.stack.append('log')
106
107+ def __close_merge(self, levels):
108+ while len(self.stack) > 0:
109+ item = self.stack.pop(len(self.stack) - 1)
110+ self.to_file.write('</%s>' % item)
111+ if item == 'merge':
112+ levels -= 1
113+ if levels == 0:
114+ return
115+
116 def __close_log(self):
117- for item in self.stack.__reversed__():
118+ while len(self.stack) > 0:
119+ item = self.stack.pop(len(self.stack) - 1)
120 self.to_file.write('</%s>' % item)
121- self.stack.pop()
122 if item == 'log':
123- self.open_logs -= 1
124 return
125- if item == 'merge':
126- self.open_merges -= 1
127
128 def __log_revision(self, revision):
129 if revision.revno is not None:
130@@ -137,7 +90,7 @@
131 if revision.tags:
132 self.to_file.write('<tags>')
133 for tag in revision.tags:
134- self.to_file.write('<tag>%s</tag>' % tag)
135+ self.to_file.write('<tag>%s</tag>' % _escape_cdata(tag))
136 self.to_file.write('</tags>')
137 if self.show_ids:
138 self.to_file.write('<revisionid>%s</revisionid>' %
139@@ -181,26 +134,7 @@
140
141 def end_log(self):
142 #if the last logged was inside a merge (and it was only one log)
143- if self.open_logs > 1 and self.open_merges > 0:
144- self.to_file.write('</log>')
145- self.open_logs = self.open_logs - 1
146- if not self.start_with_merge:
147- # In case that the last log was inside a merge we need to close it
148- if self.open_merges > 0:
149- for merge in range(self.open_merges):
150- self.to_file.write('</merge>')
151- if self.open_logs > 0:
152- self.to_file.write('</log>')
153- self.open_logs -= 1
154- self.open_merges = self.open_merges - 1
155- # to close the last opened log
156- if self.open_logs > 0:
157- self.to_file.write('</log>')
158- self.open_logs = self.open_logs - 1
159- else:
160- if self.open_logs > 0:
161- self.to_file.write('</log>')
162- self.open_logs = self.open_logs - 1
163+ self.__close_merge(len(self.stack))
164 self.to_file.write('</logs>')
165
166
167
168=== modified file 'tests/test_log_xml.py'
169--- tests/test_log_xml.py 2012-02-21 10:14:16 +0000
170+++ tests/test_log_xml.py 2013-03-22 16:31:28 +0000
171@@ -184,11 +184,13 @@
172 branch1_tree = self._prepare(path='branch1', format='dirstate-tags')
173 branch1 = branch1_tree.branch
174 branch2_tree = branch1_tree.bzrdir.sprout('branch2').open_workingtree()
175+ branch2 = branch2_tree.branch
176 branch1_tree.commit(message='foobar', allow_pointless=True)
177 branch1.tags.set_tag('tag1', branch1.last_revision())
178+ branch2_tree.merge_from_branch(branch1)
179+ branch1.tags.merge_to(branch2.tags)
180+ branch2_tree.commit(message='merge branch 1')
181 os.chdir('branch2')
182- self.run_bzr('merge ../branch1') # tags don't propagate otherwise
183- branch2_tree.commit(message='merge branch 1')
184 log_xml = fromstring(self.run_bzr("xmllog -r-1")[0])
185 for tag in log_xml.findall('log/merge/log/tags/tag'):
186 self.assertEquals(tag.text, 'tag1')
187@@ -460,7 +462,7 @@
188 for message in log_xml.findall('log/merge/merge/log/message'):
189 self.assertEquals(message.text.strip(), 'merge first post')
190
191-class TestLogEncodings(TestCaseInTempDir):
192+class TestLogEncodings(ExternalBase):
193
194 _mu = u'\xb5'
195 _message = u'Message with \xb5'
196@@ -482,7 +484,7 @@
197 ]
198
199 def setUp(self):
200- TestCaseInTempDir.setUp(self)
201+ ExternalBase.setUp(self)
202 self.old_user_encoding = osutils._cached_user_encoding
203 self.old_get_user_encoding = osutils.get_user_encoding
204
205@@ -492,14 +494,21 @@
206 bzrlib.user_encoding = self.old_user_encoding
207 osutils._cached_user_encoding = self.old_user_encoding
208 osutils.get_user_encoding = self.old_get_user_encoding
209- TestCaseInTempDir.tearDown(self)
210-
211- def create_branch(self):
212- bzr = self.run_bzr
213- bzr('init')
214- open('a', 'wb').write('some stuff\n')
215- bzr('add a')
216- bzr(['commit', '-m', self._message])
217+ ExternalBase.tearDown(self)
218+
219+ def create_branch(self, path='.', format=None):
220+ tree = self.make_branch_and_tree(path, format=format)
221+ self.build_tree(
222+ [path + '/hello.txt'])
223+ tree.add('hello.txt')
224+ tree.commit(message=self._message)
225+
226+ def create_branch2(self, path='.', format=None):
227+ tree = self.make_branch_and_tree(path, format=format)
228+ self.build_tree(
229+ [path + '/hello.txt'])
230+ tree.add('hello.txt')
231+ tree.commit(message=u'\u0422\u0435\u0441\u0442')
232
233 def try_encoding(self, encoding, fail=False):
234 bzr = self.run_bzr
235@@ -546,16 +555,13 @@
236 self.try_encoding(encoding, fail=True)
237
238 def test_stdout_encoding(self):
239+ self.create_branch2()
240 bzr = self.run_bzr
241 osutils._cached_user_encoding = 'cp1251'
242 bzrlib.osutils.get_user_encoding = lambda: 'cp1251'
243 if hasattr(bzrlib, 'user_encoding'):
244 bzrlib.user_encoding = 'cp1251'
245
246- bzr('init')
247- self.build_tree(['a'])
248- bzr('add a')
249- bzr(['commit', '-m', u'\u0422\u0435\u0441\u0442'])
250 stdout, stderr = self.run_bzr('xmllog', encoding='cp866')
251
252 #message = stdout.splitlines()[-1]

Subscribers

People subscribed via source and target branches

to all changes: