Code review comment for ~racb/git-ubuntu:test-optimization

Revision history for this message
Bryce Harrington (bryce) wrote :

a375ea332b0d366eed7c284f6a093dd59e6d0917
  + LGTM

05a41143196849de5cc22b049b84cd5883469724
  + LGTM

d7d06fec7f4b96ec1173446579c0582a4d60bb87
  + LGTM

0790066a0b3bb109d95c1fe26d092475a9c433bc
  + sp. excecutable
  + It's good you're calling out the necessity of debian/rules being executable, however the explanation is leaving me with more questions. E.g. why must it be executable (nothing in the code seems to actually use it)? Why does it say "Exceptionally", by this do you just mean "Notably", or is an exception involved, or...? Why would the API need to represent debian/rule's executable nature? Should the code include an assertion check for execute permissions on the file? Yet I see the caller is explicitly setting the executable bit *after* the call... I'm guessing what you mean to say is that the routine does not preserve debian/rules' executable nature so callers will need to set that manually... or something like that? Perhaps you could clarify the text to explain it more precisely.
  + What should the behavior be if source/format, control, rules, or changelog were missing from the debian directory? I'm guessing it'll throw an exception on None.encode()
  + Technically the rtype is an generator, although I don't know if we make the documentation reflect that, but it can matter depending on how the routine's called

2ecc23b7c32eb2b78287700fa6538c217f249411
  + 98s -> 32s nice!
  + "But I'll leave this for another time." -> sounds like a separate bug report should be filed?
  + to_git_tree() needs its repo param documented
  + self.source.to_git_tree(repo) can fail from an assert in the internal write(). You might want to consider revising the logic so it falls back to the original method if the git tree write fails for whatever reason. I.e.:

        if not self.patches_applied:
            # Fast path if we don't need to apply patches (which is not yet
            # supported by source_builder.Source.to_git_tree()).
            try:
                return self.source.to_git_tree(repo)
            except:
                # On any error, fallback to original method
                pass

        with self.source as dsc_path:
            oid_str = gitubuntu.importer.dsc_to_tree_hash(
                repo,
                dsc_path,
                True,
            )
        return pygit2.Oid(binascii.unhexlify(oid_str))

('unhexlify' is a fun word to say.)

I notice there are no new or expanded test cases accompanying this. Do you feel the existing test cases still adequately exercise the code? to_git_tree() and (maybe) _to_path_bytes_pairs() are new, and there's a new code path in _obj_to_oid().

review: Needs Fixing

« Back to merge proposal