Merge lp:~spiv/bzr-builddeb/unbound-local-623736 into lp:bzr-builddeb

Proposed by Andrew Bennetts on 2010-08-25
Status: Rejected
Rejected by: James Westby on 2010-09-10
Proposed branch: lp:~spiv/bzr-builddeb/unbound-local-623736
Merge into: lp:bzr-builddeb
Diff against target: 23 lines (+7/-6)
1 file modified
bzrtools_import.py (+7/-6)
To merge this branch: bzr merge lp:~spiv/bzr-builddeb/unbound-local-623736
Reviewer Review Type Date Requested Status
James Westby 2010-08-25 Disapprove on 2010-09-10
Review via email: mp+33609@code.launchpad.net

Description of the change

This should fix an UnboundLocalError in the case where file_ids_from is an empty list.

I took a look at writing a test, but I couldn't find unit tests for the bzrtools_import functions, and I'm not sure how to provoke that situation via less direct APIs. I can at least say it doesn't break any tests :)

To post a comment you must log in.
Robert Collins (lifeless) wrote :

If you look back a few commits in the file you'll see a change by me;
that commit has tests.

-Rob

James Westby (james-w) wrote :

On Wed, 25 Aug 2010 05:20:55 -0000, Andrew Bennetts <email address hidden> wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr-builddeb/unbound-local-623736 into lp:bzr-builddeb.
>
> Requested reviews:
> Bzr-builddeb-hackers (bzr-builddeb-hackers)
> Related bugs:
> #623736 bzr crashed with UnboundLocalError in _import_archive()
> https://bugs.launchpad.net/bugs/623736
>
>
> This should fix an UnboundLocalError in the case where file_ids_from
> is an empty list.

Thanks.

> I took a look at writing a test, but I couldn't find unit tests for
> the bzrtools_import functions, and I'm not sure how to provoke that
> situation via less direct APIs. I can at least say it doesn't break
> any tests :)

Feel free to add a TestCase for testing bzrtools_import directly. That
would seem to be the best thing here.

It shouldn't be hard to set up the conditions, as you can import a
directory, and file_ids_from is just passed in directly.

Let me know if you would like me to do it.

Thanks,

James

James Westby (james-w) wrote :

Hi,

This has been superseded with my work on this code today.

Thanks for the contribution.

James

review: Disapprove

Unmerged revisions

476. By Andrew Bennetts on 2010-08-25

Fix UnboundLocalError in _import_archive.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrtools_import.py'
2--- bzrtools_import.py 2010-08-19 19:28:08 +0000
3+++ bzrtools_import.py 2010-08-25 05:20:55 +0000
4@@ -260,12 +260,13 @@
5 tt.delete_versioned(trans_id)
6 trans_id = tt.trans_id_file_id(found_file_id)
7 break
8- if not found_file_id and not existing_file_id:
9- # No file_id in any of the source trees and no file id in the base
10- # tree.
11- name = basename(member.name.rstrip('/'))
12- file_id = generate_ids.gen_file_id(name)
13- tt.version_file(file_id, trans_id)
14+ else:
15+ if not existing_file_id:
16+ # No file_id in any of the source trees and no file id in
17+ # the base tree.
18+ name = basename(member.name.rstrip('/'))
19+ file_id = generate_ids.gen_file_id(name)
20+ tt.version_file(file_id, trans_id)
21 path = tree.abspath(relative_path)
22 if member.name in seen:
23 if tt.final_kind(trans_id) == 'file':

Subscribers

People subscribed via source and target branches