Skip to content
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

Add a Git-backed storage.ReadBucket via storagegit #2114

Merged
merged 18 commits into from May 25, 2023

Conversation

saquibmian
Copy link
Contributor

No description provided.

private/pkg/git/git.go Outdated Show resolved Hide resolved
private/pkg/git/git.go Outdated Show resolved Hide resolved
// Traverse walks down a tree, following the name-path specified,
// and returns the terminal TreeEntry. If no entry is found, it returns
// ErrSubTreeNotFound.
Traverse(objectReader ObjectReader, names ...string) (TreeEntry, error)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not Walk?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could go either way, but I felt that Walk was overloaded with storage's walk (i.e, it's not walking all paths in a subtree, it's navigating to a particular descendant). Maybe Descendant is better?

private/pkg/git/git.go Outdated Show resolved Hide resolved
private/pkg/git/tree.go Show resolved Hide resolved
private/pkg/git/tree_test.go Show resolved Hide resolved
private/pkg/git/tree_entry_test.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Show resolved Hide resolved
private/pkg/storage/storagegit/storagegit.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/provider.go Show resolved Hide resolved
Copy link
Member

@unmultimedio unmultimedio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking very nice, left mainly nits

private/pkg/git/tree.go Outdated Show resolved Hide resolved
private/pkg/git/tree_entry.go Outdated Show resolved Hide resolved
private/pkg/git/tree_entry_test.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Outdated Show resolved Hide resolved
private/pkg/git/tree.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Outdated Show resolved Hide resolved
Copy link
Member

@unmultimedio unmultimedio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A bunch of rename suggestions and other nits. Nice work!

Comment on lines 36 to 46
// ModeFile is a blob that should be written as a plain file.
ModeFile FileMode = 010_0644
// ModeExec is a blob that should be written with the executable bit set.
ModeExe FileMode = 010_0755
// ModeDir is a tree to be unpacked as a subdirectory in the current
// directory.
ModeDir FileMode = 004_0000
// ModeSymlink is a blob with its content being the path linked to.
ModeSymlink FileMode = 012_0000
// ModeSubmodule is a commit that the submodule is checked out at.
ModeSubmodule FileMode = 016_0000
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the left padded zeroes in all of them? I think at least one zero can be removed in all of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, these are octal literals :)

private/pkg/git/git.go Outdated Show resolved Hide resolved
private/pkg/git/git.go Outdated Show resolved Hide resolved
private/pkg/git/object_reader.go Outdated Show resolved Hide resolved
private/pkg/git/tree.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Show resolved Hide resolved
func (b *bucket) walk(
parent git.Tree,
objectReader git.ObjectReader,
prefix string,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that partial prefixes won't work then, so for a valid dir foo/bar/baz/*, the prefix foo/b won't walk anything. Unsure how other buckets handle this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's correct, that's be design for buckets. The prefix is a segment prefix, but a path key prefix.

private/pkg/storage/storagegit/bucket.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Outdated Show resolved Hide resolved
private/pkg/storage/storagegit/bucket.go Outdated Show resolved Hide resolved
Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good to me! One comment, then defer to @unmultimedio for the rest.

private/pkg/storage/storagegit/storagegit.go Outdated Show resolved Hide resolved
@saquibmian saquibmian merged commit 45c2edc into main May 25, 2023
7 checks passed
@saquibmian saquibmian deleted the saquib/bsr-1896-implement-storagegit branch May 25, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants