New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Store can expand directories into transitive fingerprints #5331

Merged
merged 1 commit into from Jan 22, 2018

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

illicitonion commented Jan 16, 2018

When pushing Digests from local to remote, if the Digest is a Directory, the Store may want to proactively upload the files it contains (or at least ask the remote whether it needs to). To do this, it needs to be able to identify the transitive contents of a Directory.

@illicitonion illicitonion requested a review from stuhood Jan 16, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jan 16, 2018

Depends on #5330, #5329, #5328, #5332

@stuhood
Copy link
Member

stuhood left a comment

This also needs more context: either method docs for the new methods (especially since they return string values), or at the very least as a PR description.

I'm guessing that the String values are the string versions of fingerprints ready to be placed in a Directory protobuf? But passing around unaliased String usages isn't very self-documenting. A type alias might help?

A few possible resolutions here... I'll accept any of them.

@illicitonion

This comment has been minimized.

Copy link
Contributor

illicitonion commented Jan 19, 2018

Updated description. There is no public-facing API here, but the caller will get a BoxFuture<HashMap<Digest, EntryType>, String> - not sure where you're seeing Strings being returned other than as errors...

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/caswrites/5-expand-directories branch from 9b43c0e to cbb8c49 Jan 21, 2018

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/caswrites/5-expand-directories branch from cbb8c49 to 0eb6e9b Jan 22, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks, looks good. Sorry for the result dyslexia.

@illicitonion illicitonion merged commit 919c0e1 into pantsbuild:master Jan 22, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@illicitonion illicitonion deleted the twitter:dwagnerhall/caswrites/5-expand-directories branch Feb 26, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment