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
=== modified file 'logxml.py'
--- logxml.py 2011-12-12 15:11:38 +0000
+++ logxml.py 2013-03-22 16:31:28 +0000
@@ -24,11 +24,8 @@
24 super(XMLLogFormatter, self).__init__(*args, **kwargs)24 super(XMLLogFormatter, self).__init__(*args, **kwargs)
25 self.log_count = 025 self.log_count = 0
26 self.start_with_merge = False26 self.start_with_merge = False
27 self.nested_merge_count = 0
28 self.previous_merge_depth = 027 self.previous_merge_depth = 0
29 self.debug_enabled = 'debug' in debug.debug_flags28 self.debug_enabled = 'debug' in debug.debug_flags
30 self.open_logs = 0
31 self.open_merges = 0
32 self.stack = []29 self.stack = []
3330
34 def show(self, revno, rev, delta, tags=None):31 def show(self, revno, rev, delta, tags=None):
@@ -47,89 +44,45 @@
47 self.__debug(revision)44 self.__debug(revision)
48 actions = []45 actions = []
49 # to handle merge revision as childs46 # to handle merge revision as childs
50 if revision.merge_depth > 0 and not self.start_with_merge:47 if self.previous_merge_depth < revision.merge_depth:
51 if self.previous_merge_depth < revision.merge_depth:48 if self.log_count > 0:
52 if self.log_count > 0:49 self.__open_merge(revision.merge_depth - self.previous_merge_depth)
53 merge_depth_diference = revision.merge_depth - \50 elif self.previous_merge_depth > revision.merge_depth:
54 self.previous_merge_depth51 self.__close_merge(self.previous_merge_depth - revision.merge_depth)
55 for m in range(merge_depth_diference):52 if len(self.stack) > 0 and self.stack[len(self.stack) - 1] == 'log':
56 actions.append(self.__open_merge)53 self.__close_log()
57 if merge_depth_diference > 1:54 else:
58 self.nested_merge_count += 155 self.__close_log()
59 elif self.log_count == 0:
60 # first log is inside a merge, we show it as a top level
61 # we could support a merge tag without parent log.
62 self.start_with_merge = True
63 elif self.previous_merge_depth > revision.merge_depth:
64 # TODO: testcase for more than one level of nested merges
65 actions.append({self.__close_merge:self.previous_merge_depth - \
66 revision.merge_depth})
67 if self.nested_merge_count > 0:
68 self.nested_merge_count -= 1
69 else:
70 actions.append(self.__close_log)
71 else:
72 if self.open_logs > 0:
73 actions.append(self.__close_log)
74 elif self.previous_merge_depth < revision.merge_depth:
75 actions.append({self.__close_merge:self.previous_merge_depth - \
76 revision.merge_depth})
77 if self.nested_merge_count > 0:
78 self.nested_merge_count -= 1
79 else:
80 actions.append(self.__close_log)
81 elif self.open_merges > 0:
82 actions.append({self.__close_merge:self.open_merges})
83 #actions.append(self.__close_merge)
84 actions.append(self.__close_log)
85 else:
86 actions.append(self.__close_log)
87 if self.start_with_merge:
88 # we only care about the first log, the following logs are
89 # handlend in the logic of nested merges
90 self.start_with_merge = False
91 for action in actions:
92 if type(action) == dict:
93 action.keys()[0](action[action.keys()[0]])
94 else:
95 action()
96 self.__open_log()56 self.__open_log()
97 self.__log_revision(revision)57 self.__log_revision(revision)
9858
99 self.log_count = self.log_count + 159 self.log_count = self.log_count + 1
100 self.previous_merge_depth = revision.merge_depth60 self.previous_merge_depth = revision.merge_depth
10161
102 def __open_merge(self):62 def __open_merge(self, levels):
103 self.to_file.write('<merge>')63 for i in range(levels):
104 self.open_merges += 164 self.to_file.write('<merge>')
105 self.stack.append('merge')65 self.stack.append('merge')
106
107 def __close_merge(self, num=1):
108 for item in self.stack.__reversed__():
109 self.to_file.write('</%s>' % item)
110 self.stack.pop()
111 if item == 'merge':
112 self.open_merges -= 1
113 num -= 1
114 if num == 0:
115 return
116 if item == 'log':
117 self.open_logs -= 1
11866
119 def __open_log(self):67 def __open_log(self):
120 self.to_file.write('<log>',)68 self.to_file.write('<log>',)
121 self.open_logs = self.open_logs + 1
122 self.stack.append('log')69 self.stack.append('log')
12370
71 def __close_merge(self, levels):
72 while len(self.stack) > 0:
73 item = self.stack.pop(len(self.stack) - 1)
74 self.to_file.write('</%s>' % item)
75 if item == 'merge':
76 levels -= 1
77 if levels == 0:
78 return
79
124 def __close_log(self):80 def __close_log(self):
125 for item in self.stack.__reversed__():81 while len(self.stack) > 0:
82 item = self.stack.pop(len(self.stack) - 1)
126 self.to_file.write('</%s>' % item)83 self.to_file.write('</%s>' % item)
127 self.stack.pop()
128 if item == 'log':84 if item == 'log':
129 self.open_logs -= 1
130 return85 return
131 if item == 'merge':
132 self.open_merges -= 1
13386
134 def __log_revision(self, revision):87 def __log_revision(self, revision):
135 if revision.revno is not None:88 if revision.revno is not None:
@@ -137,7 +90,7 @@
137 if revision.tags:90 if revision.tags:
138 self.to_file.write('<tags>')91 self.to_file.write('<tags>')
139 for tag in revision.tags:92 for tag in revision.tags:
140 self.to_file.write('<tag>%s</tag>' % tag)93 self.to_file.write('<tag>%s</tag>' % _escape_cdata(tag))
141 self.to_file.write('</tags>')94 self.to_file.write('</tags>')
142 if self.show_ids:95 if self.show_ids:
143 self.to_file.write('<revisionid>%s</revisionid>' %96 self.to_file.write('<revisionid>%s</revisionid>' %
@@ -181,26 +134,7 @@
181134
182 def end_log(self):135 def end_log(self):
183 #if the last logged was inside a merge (and it was only one log)136 #if the last logged was inside a merge (and it was only one log)
184 if self.open_logs > 1 and self.open_merges > 0:137 self.__close_merge(len(self.stack))
185 self.to_file.write('</log>')
186 self.open_logs = self.open_logs - 1
187 if not self.start_with_merge:
188 # In case that the last log was inside a merge we need to close it
189 if self.open_merges > 0:
190 for merge in range(self.open_merges):
191 self.to_file.write('</merge>')
192 if self.open_logs > 0:
193 self.to_file.write('</log>')
194 self.open_logs -= 1
195 self.open_merges = self.open_merges - 1
196 # to close the last opened log
197 if self.open_logs > 0:
198 self.to_file.write('</log>')
199 self.open_logs = self.open_logs - 1
200 else:
201 if self.open_logs > 0:
202 self.to_file.write('</log>')
203 self.open_logs = self.open_logs - 1
204 self.to_file.write('</logs>')138 self.to_file.write('</logs>')
205139
206140
207141
=== modified file 'tests/test_log_xml.py'
--- tests/test_log_xml.py 2012-02-21 10:14:16 +0000
+++ tests/test_log_xml.py 2013-03-22 16:31:28 +0000
@@ -184,11 +184,13 @@
184 branch1_tree = self._prepare(path='branch1', format='dirstate-tags')184 branch1_tree = self._prepare(path='branch1', format='dirstate-tags')
185 branch1 = branch1_tree.branch185 branch1 = branch1_tree.branch
186 branch2_tree = branch1_tree.bzrdir.sprout('branch2').open_workingtree()186 branch2_tree = branch1_tree.bzrdir.sprout('branch2').open_workingtree()
187 branch2 = branch2_tree.branch
187 branch1_tree.commit(message='foobar', allow_pointless=True)188 branch1_tree.commit(message='foobar', allow_pointless=True)
188 branch1.tags.set_tag('tag1', branch1.last_revision())189 branch1.tags.set_tag('tag1', branch1.last_revision())
190 branch2_tree.merge_from_branch(branch1)
191 branch1.tags.merge_to(branch2.tags)
192 branch2_tree.commit(message='merge branch 1')
189 os.chdir('branch2')193 os.chdir('branch2')
190 self.run_bzr('merge ../branch1') # tags don't propagate otherwise
191 branch2_tree.commit(message='merge branch 1')
192 log_xml = fromstring(self.run_bzr("xmllog -r-1")[0])194 log_xml = fromstring(self.run_bzr("xmllog -r-1")[0])
193 for tag in log_xml.findall('log/merge/log/tags/tag'):195 for tag in log_xml.findall('log/merge/log/tags/tag'):
194 self.assertEquals(tag.text, 'tag1')196 self.assertEquals(tag.text, 'tag1')
@@ -460,7 +462,7 @@
460 for message in log_xml.findall('log/merge/merge/log/message'):462 for message in log_xml.findall('log/merge/merge/log/message'):
461 self.assertEquals(message.text.strip(), 'merge first post')463 self.assertEquals(message.text.strip(), 'merge first post')
462464
463class TestLogEncodings(TestCaseInTempDir):465class TestLogEncodings(ExternalBase):
464466
465 _mu = u'\xb5'467 _mu = u'\xb5'
466 _message = u'Message with \xb5'468 _message = u'Message with \xb5'
@@ -482,7 +484,7 @@
482 ]484 ]
483485
484 def setUp(self):486 def setUp(self):
485 TestCaseInTempDir.setUp(self)487 ExternalBase.setUp(self)
486 self.old_user_encoding = osutils._cached_user_encoding488 self.old_user_encoding = osutils._cached_user_encoding
487 self.old_get_user_encoding = osutils.get_user_encoding489 self.old_get_user_encoding = osutils.get_user_encoding
488490
@@ -492,14 +494,21 @@
492 bzrlib.user_encoding = self.old_user_encoding494 bzrlib.user_encoding = self.old_user_encoding
493 osutils._cached_user_encoding = self.old_user_encoding495 osutils._cached_user_encoding = self.old_user_encoding
494 osutils.get_user_encoding = self.old_get_user_encoding496 osutils.get_user_encoding = self.old_get_user_encoding
495 TestCaseInTempDir.tearDown(self)497 ExternalBase.tearDown(self)
496498
497 def create_branch(self):499 def create_branch(self, path='.', format=None):
498 bzr = self.run_bzr500 tree = self.make_branch_and_tree(path, format=format)
499 bzr('init')501 self.build_tree(
500 open('a', 'wb').write('some stuff\n')502 [path + '/hello.txt'])
501 bzr('add a')503 tree.add('hello.txt')
502 bzr(['commit', '-m', self._message])504 tree.commit(message=self._message)
505
506 def create_branch2(self, path='.', format=None):
507 tree = self.make_branch_and_tree(path, format=format)
508 self.build_tree(
509 [path + '/hello.txt'])
510 tree.add('hello.txt')
511 tree.commit(message=u'\u0422\u0435\u0441\u0442')
503512
504 def try_encoding(self, encoding, fail=False):513 def try_encoding(self, encoding, fail=False):
505 bzr = self.run_bzr514 bzr = self.run_bzr
@@ -546,16 +555,13 @@
546 self.try_encoding(encoding, fail=True)555 self.try_encoding(encoding, fail=True)
547556
548 def test_stdout_encoding(self):557 def test_stdout_encoding(self):
558 self.create_branch2()
549 bzr = self.run_bzr559 bzr = self.run_bzr
550 osutils._cached_user_encoding = 'cp1251'560 osutils._cached_user_encoding = 'cp1251'
551 bzrlib.osutils.get_user_encoding = lambda: 'cp1251'561 bzrlib.osutils.get_user_encoding = lambda: 'cp1251'
552 if hasattr(bzrlib, 'user_encoding'):562 if hasattr(bzrlib, 'user_encoding'):
553 bzrlib.user_encoding = 'cp1251'563 bzrlib.user_encoding = 'cp1251'
554564
555 bzr('init')
556 self.build_tree(['a'])
557 bzr('add a')
558 bzr(['commit', '-m', u'\u0422\u0435\u0441\u0442'])
559 stdout, stderr = self.run_bzr('xmllog', encoding='cp866')565 stdout, stderr = self.run_bzr('xmllog', encoding='cp866')
560566
561 #message = stdout.splitlines()[-1]567 #message = stdout.splitlines()[-1]

Subscribers

People subscribed via source and target branches

to all changes: