itchio / butler

🎩 Command-line itch.io helper
MIT License
744 stars 53 forks source link

Manager fails to exclude 64-bit uploads on 32-bit runtimes #146

Closed 200sc closed 6 years ago

200sc commented 6 years ago

In the excludeWrongArch function, the boolean filtered flag is only set to true if runtime.Is64 is also true, and as a result a lot of work is being done in the false case to do that filtering without the appropriate results being returned.

A quick fix would be to just set the flag to be true in the false case, but this function could also be refactored to not need a filtered flag-- it seems like a premature optimization to not always return the res slice instead of sometimes returning the input argument, which (probably won't) but could lead to stealth bugs in code that doesn't expect the same input slice returned in some scenarios. (Modifications could be made to the returned uploads slice, in turn modifying the input argument as well unexpectedly).

A little bit of refactoring could also just set an allowed and restricted upload string instead of duplicating the containment check and appending block for both architectures:

allowed := "32"
restricted := "64"
if runtime.Is64 {
    allowed, restricted = restricted, allowed
}   
if anyUploadContainsString(uploads, allowed) {
    filtered = true
    for _, u := range uploads {
        if uploadContainsString(u, restricted) {
            // exclude
            continue
        }
        res = append(res, u)
    }
}
fasterthanlime commented 6 years ago

Thanks for the report! I went with an alternate approach that is more conservative wrt filtering, but is probably the best I can do with the current dataset.