Comment 7 for bug 717879

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!