Skip to content

Manifest.Range: iterate entries (path, digest)#1966

Merged
unmultimedio merged 8 commits intomainfrom
jfigueroa/manifest-iterators
Apr 3, 2023
Merged

Manifest.Range: iterate entries (path, digest)#1966
unmultimedio merged 8 commits intomainfrom
jfigueroa/manifest-iterators

Conversation

@unmultimedio
Copy link
Member

@unmultimedio unmultimedio commented Mar 31, 2023

Range function for the manifest entries, so we can reduce the appearances of .Paths() usages like:

for _, path := range m.Paths() {
  digest, ok := m.DigestFor(path)
  if !ok {
    // this will never happen
    return nil, fmt.Errorf("digest not found for path: %s", path)
  }
  // do something with path and digest...
}

into:

m.Range(func(path string, digest manifest.Digest) error {
  // do something with path and digest...
})

@unmultimedio unmultimedio requested review from pkwarren and twilly March 31, 2023 23:48
@unmultimedio unmultimedio self-assigned this Mar 31, 2023
// IteratePaths invokes a function for all the paths in the manifest, passing
// the path and its digest. The order in which the paths are iterated is not
// guaranteed.
func (m *Manifest) IteratePaths(f func(string, Digest)) {
Copy link
Member

Choose a reason for hiding this comment

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

The paths function doesn't look too expensive - why are we introducing this variant? Is it to return the pre-calculated digest for each path?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is not because of how expensive it is, but because we have some code paths in which we want the digests, not only the paths, so we can drop unnecessary presence checkers. I'll change a few of the code paths in this same PR as well to make it more clear.

Comment on lines -72 to -75
digest, found := manifestFromCache.DigestFor(path)
if !found {
return nil, fmt.Errorf("digest not found for path: %s", path)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the check we're skipping with the new func.

// Range invokes a function for all the paths in the manifest, passing the path
// and its digest. The order in which the paths are iterated is not guaranteed.
// This func will stop iterating if an error is returned.
func (m *Manifest) Range(f func(path string, digest Digest) error) error {
Copy link
Member Author

Choose a reason for hiding this comment

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

Renaming func to follow suggestion in this comment. cc @jhump

@unmultimedio unmultimedio changed the title Manifest paths iterator Manifest range: iterate entries (path, digest) Apr 3, 2023
@unmultimedio unmultimedio changed the title Manifest range: iterate entries (path, digest) Manifest.Range: iterate entries (path, digest) Apr 3, 2023
@unmultimedio unmultimedio requested a review from pkwarren April 3, 2023 13:50
Co-authored-by: Saquib Mian <smian@buf.build>
@unmultimedio unmultimedio merged commit 2f3a1f2 into main Apr 3, 2023
@unmultimedio unmultimedio deleted the jfigueroa/manifest-iterators branch April 3, 2023 15:44
Monirul1 pushed a commit to Monirul1/buf that referenced this pull request Apr 30, 2023
Range function for the manifest entries, so we can reduce the
appearances of `.Paths()` usages like:

```go
for _, path := range m.Paths() {
  digest, ok := m.DigestFor(path)
  if !ok {
    // this will never happen
    return nil, fmt.Errorf("digest not found for path: %s", path)
  }
  // do something with path and digest...
}
```

into:

```go
m.Range(func(path string, digest manifest.Digest) error {
  // do something with path and digest...
})
```

---------

Co-authored-by: Saquib Mian <smian@buf.build>
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.

3 participants