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().
a375ea332b0d366 eed7c284f6a093d d59e6d0917
+ LGTM
05a41143196849d e5cc22b049b84cd 5883469724
+ LGTM
d7d06fec7f4b96e c1173446579c058 2a4d60bb87
+ LGTM
0790066a0b3bb10 9d95c1fe26d0924 75a9c433bc
+ 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
2ecc23b7c32eb2b 78287700fa6538c 217f249411 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.:
+ 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.
if not self.patches_ applied: builder. Source. to_git_ tree()) .
return self.source. to_git_ tree(repo)
pass
# Fast path if we don't need to apply patches (which is not yet
# supported by source_
try:
except:
# On any error, fallback to original method
with self.source as dsc_path: importer. dsc_to_ tree_hash(
repo,
dsc_path,
True, Oid(binascii. unhexlify( oid_str) )
oid_str = gitubuntu.
)
return pygit2.
('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().