loggerhead

Merge lp:~pr0gg3d/loggerhead/annotate_comment_notpresent_812583 into lp:loggerhead

  1. annotate_comment_notpresent_812583
  2. Merge into trunk-rich
Proposed by Francesco Del Degan
Status: Merged
Approved by: John A Meinel
Approved revision: 455
Merged at revision: 453
Proposed branch: lp:~pr0gg3d/loggerhead/annotate_comment_notpresent_812583
Merge into: lp:loggerhead
Diff against target: 58 lines (+17/-6)
2 files modified
loggerhead/controllers/annotate_ui.py (+5/-1)
loggerhead/tests/test_controllers.py (+12/-5)
To merge this branch: bzr merge lp:~pr0gg3d/loggerhead/annotate_comment_notpresent_812583
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+70923@code.launchpad.net

This proposal supersedes a proposal from 2011-08-09.

Description of the change

Fixes bug #812583 that raises an exception when annotating some lines where commit message is not present.

It catch that exception (IndexError) and put the message as empty string.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/9/2011 9:38 AM, Francesco Del Degan wrote:
> Francesco Del Degan has proposed merging
> lp:~pr0gg3d/loggerhead/annotate_comment_notpresent_812583 into
> lp:loggerhead.
>
> Requested reviews: Loggerhead Reviewers (loggerhead-reviewers)
> Related bugs: Bug #812583 in loggerhead: "IndexError in
> add_template_values in loggerhead annotate"
> https://bugs.launchpad.net/loggerhead/+bug/812583
>
> For more details, see:
> https://code.launchpad.net/~pr0gg3d/loggerhead/annotate_comment_notpresent_812583/+merge/70825
>
> Fixes bug #812583 that raises an exception when annotating some
> lines where commit message is not present.
>
> It catch that exception (IndexError) and put the message as empty
> string.

Would it be possible to add a test for this bug? It shouldn't be too
hard to generate a commit without a comment and make sure that annotate
doesn't explode.

 review: needsfixing

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5BAssACgkQJdeBCYSNAAMHMQCfYppjU8zVAVDBZrKwHoLkLZm5
m90AniY/yQB/+FmHSOWKfqWz93AJWSFT
=hjCf
-----END PGP SIGNATURE-----

review: Needs Fixing
Revision history for this message
Francesco Del Degan (pr0gg3d) wrote : Posted in a previous version of this proposal

Sure, i'll look into it.

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/9/2011 11:54 AM, Francesco Del Degan wrote:
> Sure, i'll look into it.

You should be able to add a test to loggerhead/tests/test_controllers.py

I think you can just set the 'rev_ids_texts' to whatever you want
(especially for your empty-file patch). You may want to add an optional
parameter to "make_annotate_ui_*" that takes a commit message, which you
can then set to be empty.

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5BLaUACgkQJdeBCYSNAAOdbQCgvVbqeRG8vy8ZXp3rH0pjR/cw
csAAnRZphxP0XfPDnAxbFElu96dzJdX/
=Zoo5
-----END PGP SIGNATURE-----

Revision history for this message
Francesco Del Degan (pr0gg3d) wrote : Posted in a previous version of this proposal

ya I refactored adding a third argument to 'history' tuple and modified the existing test adding two "." as messages and use one '' in void-comment test.

Now, i've to do the same mod into the other mp, extending "make_annotate_ui_*" and modifying the test for empty file.

I'm sorry, but i'm pretty new to bzr, i'm trying to figure out how handle this (sort of) conflict.

Revision history for this message
John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 8/9/2011 3:08 PM, Francesco Del Degan wrote:
> ya I refactored adding a third argument to 'history' tuple and
> modified the existing test adding two "." as messages and use one ''
> in void-comment test.
>
> Now, i've to do the same mod into the other mp, extending
> "make_annotate_ui_*" and modifying the test for empty file.
>
> I'm sorry, but i'm pretty new to bzr, i'm trying to figure out how
> handle this (sort of) conflict.

I'm not sure how you are defining conflict. However, I would probably
just merge this branch into the other branch, and add the new updates.

Then when proposing it, you can mark that the other branch has a
"prerequisite" branch of this one. So the diff will show just what will
be newly introduced.

I would probably reduce the amount of the test that you copied. You
don't need the "set up by __call__" comment, etc. Just call get_values,
and add a comment that you are testing it handles an empty commit
message without breaking.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAk5BMvkACgkQJdeBCYSNAAPpRgCdEU727Id3BujMxhuLp9nJbU0c
dSgAn3GmmNaOGADYgCZfDYLl0JjxJcHC
=hcFh
-----END PGP SIGNATURE-----

Revision history for this message
Francesco Del Degan (pr0gg3d) wrote : Posted in a previous version of this proposal

> I'm not sure how you are defining conflict. However, I would probably
> just merge this branch into the other branch, and add the new updates.
>
> Then when proposing it, you can mark that the other branch has a
> "prerequisite" branch of this one. So the diff will show just what will
> be newly introduced.
>
> I would probably reduce the amount of the test that you copied. You
> don't need the "set up by __call__" comment, etc. Just call get_values,
> and add a comment that you are testing it handles an empty commit
> message without breaking.

I made some cleanup on the code and I will merge this into the other branch.
When i will propose the other branch, i will mark this as a prerequisite.

Revision history for this message
John A Meinel (jameinel) wrote :

The test fails without the fix, and the fix looks good to me. Thanks!

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'loggerhead/controllers/annotate_ui.py'
2--- loggerhead/controllers/annotate_ui.py 2011-04-22 17:22:26 +0000
3+++ loggerhead/controllers/annotate_ui.py 2011-08-09 16:37:18 +0000
4@@ -46,7 +46,11 @@
5 change = self._history.get_changes([line_revid])[0]
6 change_cache[line_revid] = change
7
8- message = change.comment.splitlines()[0]
9+ try:
10+ message = change.comment.splitlines()[0]
11+ except IndexError:
12+ # Comment not present for this revision
13+ message = ""
14
15 if last_lineno:
16 # The revspan is of lines between the last revision and this one.
17
18=== modified file 'loggerhead/tests/test_controllers.py'
19--- loggerhead/tests/test_controllers.py 2011-06-28 16:06:12 +0000
20+++ loggerhead/tests/test_controllers.py 2011-08-09 16:37:18 +0000
21@@ -117,16 +117,16 @@
22 tree = self.make_branch_and_tree('.')
23 self.build_tree_contents([('filename', '')])
24 tree.add(['filename'], [file_id])
25- for rev_id, text in rev_ids_texts:
26+ for rev_id, text, message in rev_ids_texts:
27 self.build_tree_contents([('filename', text)])
28- tree.commit(rev_id=rev_id, message='.')
29+ tree.commit(rev_id=rev_id, message=message)
30 tree.branch.lock_read()
31 self.addCleanup(tree.branch.unlock)
32 branch_app = BranchWSGIApp(tree.branch, friendly_name='test_name')
33 return AnnotateUI(branch_app, branch_app.get_history)
34
35 def test_annotate_file(self):
36- history = [('rev1', 'old\nold\n'), ('rev2', 'new\nold\n')]
37+ history = [('rev1', 'old\nold\n', '.'), ('rev2', 'new\nold\n', '.')]
38 ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
39 # A lot of this state is set up by __call__, but we'll do it directly
40 # here.
41@@ -137,8 +137,15 @@
42 self.assertEqual(2, len(annotated))
43 self.assertEqual('2', annotated[1].change.revno)
44 self.assertEqual('1', annotated[2].change.revno)
45-
46-
47+
48+ def test_annotate_empty_comment(self):
49+ # Testing empty comment handling without breaking
50+ history = [('rev1', 'old\nold\n', '.'), ('rev2', 'new\nold\n', '')]
51+ ann_ui = self.make_annotate_ui_for_file_history('file_id', history)
52+ ann_ui.args = ['rev2']
53+ annotate_info = ann_ui.get_values('filename',
54+ kwargs={'file_id': 'file_id'}, headers={})
55+
56 class TestFileDiffUI(BasicTests):
57
58 def make_branch_app_for_filediff_ui(self):

Subscribers

People subscribed via source and target branches