Hi Tom, thanks for the patch. I'll have to let Colin make the final
determination, both because he knows the code much better than I do and
because he has upload rights where I do not. I do have some questions
after looking at your patch from 2011-02-15.
First: since you have a branch that contains your change, why not add a
merge proposal for that? It would make it easier to review your patch,
comment on it, and apply it. Also, they're easy to do! :)
Why do you add the empty string to the suffix list in the for loop? I
see that the original code unconditionally adds the unsuffixed value of
open_tag_file() if both .bz2 and .gz fail. Is that the real source of
the bug (iow, open_tag_file(mirror, '') may not produce a non-None
value)?
Nit: it's better to spell it "if tag_file is not None"
Nit: some of the indentation is a bit inconsistent. E.g. the "raise
IOError" is too far to the right, although it doesn't break anything.
Nit: unless tag_files could be any false value, I generally like to use
a more specific test. In this case, tag_files will always be a list, so
testing for len() == 0 is better since it more closely matches your
intention.
I know you're modifying the existing logic, but it seems like the only
way to break out of the for-loop with tag_file not None is when
open_tag_file() returns some non-None value. In that case, I wonder if
this code makes more sense (completely untested of course ;):
tag_files = []
for mirror in mirrors:
for suffix in (".bz2", ".gz", ""):
tag_file = None
try:
tag_file = open_tag_file(mirror, suffix)
except (IOError, OSError):
pass
else:
if tag_file is not None:
tag_files.append(tag_file)
break
if len(tag_files) == 0:
raise IOError
return tag_files
Notice that I moved tag_files=None to inside the inner loop. I think
the way the old code was written, it was possible to add a tag_file more
than once inadvertently. Let's say mirrors=('A', 'B').
open_tag_file('A', '.gz') returns something non-None and it gets added
to tag_files. Now mirror 'B' is processed, but all its suffixes raise
IOError. When the inner loop for mirror='B' completes, tag_file is
still set to the value it had during the mirror='A' iteration and it
gets added twice.
I can't tell whether that's intended, but I kind of doubt it, thus the
rewrite moving tag_file=None to the inner loop.
Or I could be totally wrong. :) As I said though, I'll let Colin make
the final determination. Cheers, and thanks for your contribution!
--
You received this bug notification because you are a member of Ubuntu
Bugs, which is subscribed to Ubuntu.
https://bugs.launchpad.net/bugs/717879
Title:
germinate should not examine all components in PPAs
--
ubuntu-bugs mailing list
[email protected]
https://lists.ubuntu.com/mailman/listinfo/ubuntu-bugs
