loggerhead

Merge lp://staging/~mwhudson/loggerhead/bug-390029 into lp://staging/loggerhead

bug-390029
Merge into trunk-rich

Proposed by Michael Hudson-Doyle on 2012-01-08

Status:

Merged

Approved by:

j.c.sackett on 2012-01-20

Approved revision:

462

Merged at revision:

464

Proposed branch:

lp://staging/~mwhudson/loggerhead/bug-390029

Merge into:

lp://staging/loggerhead

Diff against target:

13 lines (+2/-1)

1 file modified

loggerhead/history.py (+2/-1)

To merge this branch:

bzr merge lp://staging/~mwhudson/loggerhead/bug-390029

Critical

Fix Released

Link a bug report

Reviewer	Review Type	Date Requested	Status
j.c.sackett (community)		2012-01-20	Approve on 2012-01-20
Richard Harding (community)	code*	2012-01-08	Approve on 2012-01-20
Review via email: mp+87878@code.staging.launchpad.net

Description of the change

This change should fix the bug, but the strange thing is that I can't reproduce it locally. It still occurs on LP though -- has something changed about whether revision_trees([revid, revid]) returns one or two trees in a recent bzr? If so, the bug will fix itself, but maybe we should apply this anyway.

Revision history for this message

Richard Harding (rharding) wrote on 2012-01-19:

Michael, in peeking at the code it seems like you'd be able to test this by creating a test like the "test_native" that sets up the linear history and then grabs the top revision off of it and feeds that to the history.get_file_changes. This might require cheating and turning the revision into an entry as in _change_from_revision, but then you'd be able to test the corner case of the revision compared to itself?

The change here makes sense, but I just want to make sure that there's not a good way to test before approving.

Thanks

review: Needs Information (code*)

Revision history for this message

Michael Hudson-Doyle (mwhudson) wrote on 2012-01-19:

Sure, creating the scenario that results in an OOPS locally is simple enough -- the thing is, it doesn't cause an error for me! Poking around and asking people suggests that bzrlib has changed behaviour here, so we can either (a) just wait until the new bzrlib is deployed on LP or (b) apply the simple patch. I'd favour (b), friendlier to people who have not upgraded bzr yet for whatever reason.

Revision history for this message

Richard Harding (rharding) wrote on 2012-01-20:

Ok, thanks for the details.

review: Approve (code*)

Revision history for this message

j.c.sackett (jcsackett) wrote on 2012-01-20:

This looks fine, I have nothing to add.

review: Approve