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

elim warning spam from #8500 #8697

Merged
merged 1 commit into from
Jan 24, 2023
Merged

Conversation

gbaz
Copy link
Collaborator

@gbaz gbaz commented Jan 24, 2023

This removes the warning spam from #8500

Now we check if a file exists before attempting to compute the hash of it. Doh.

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 24, 2023

@Mikolaj if this works for you I think we should just give it an expedited merge and backport.

Copy link
Member

@Mikolaj Mikolaj left a comment

Choose a reason for hiding this comment

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

This accesses the filesystem again for the same package and, for a few packages, their files would not be accessed at all if not for this extra check, if my guesses in #8500 were right. However, on sensible OSes, by this point, most of the time, all the relevant filesystem info is in RAM anyway.

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 24, 2023

Before, we always just tested if the file exists, which just accessed metadata. Here, we have to do that, and on top of that, if it does exist, actually hash it. But that's the point of the check :-)

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 24, 2023

https://github.com/Mergifyio backport 3.10

@mergify
Copy link
Contributor

mergify bot commented Jan 24, 2023

backport .10

❌ No backport have been created

  • Backport to branch .10 failed: Branch not found

@mergify
Copy link
Contributor

mergify bot commented Jan 24, 2023

backport 3.10

✅ Backports have been created

@Mikolaj
Copy link
Member

Mikolaj commented Jan 24, 2023

Before, we always just tested if the file exists, which just accessed metadata. Here, we have to do that, and on top of that, if it does exist, actually hash it. But that's the point of the check :-)

I don't remember how it was before. But now doesFileExist is performed twice for the same package, once in checkRepoTarballFetched and again in the new code you just added. Fortunately, if the package is going to be hashed or, worse, downloaded, this is totally negligible.

@gbaz
Copy link
Collaborator Author

gbaz commented Jan 24, 2023

Pretty sure checkRepoTarballFetched is only called on repoTarballPkgsWithoutMetadata -- the new path is only called on the other set of packages, those which have metadata to validate :-)

mergify bot added a commit that referenced this pull request Jan 24, 2023
@Mikolaj
Copy link
Member

Mikolaj commented Jan 25, 2023

You are probably right.

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.

2 participants