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

remove some unneeded normalizePath calls #41116

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

icewind1991
Copy link
Member

While normalizing isn't that expensive, it does add up if we do it multiple times for every item in a folder

@icewind1991 icewind1991 requested review from a team, ArtificialOwl, Altahrim and nfebe and removed request for a team October 25, 2023 14:44
@solracsf solracsf added this to the Nextcloud 28 milestone Oct 27, 2023
@icewind1991 icewind1991 force-pushed the normlize-less branch 4 times, most recently from 662ae14 to 9301120 Compare October 31, 2023 15:21
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
@icewind1991 icewind1991 force-pushed the normlize-less branch 2 times, most recently from e4120e2 to 4f2abbd Compare November 1, 2023 12:46
This was referenced Nov 6, 2023
@blizzz blizzz mentioned this pull request Nov 14, 2023
@@ -491,7 +491,7 @@ public static function getStorageInfo($path, $rootInfo = null, $includeMountPoin
if (!$view) {
throw new \OCP\Files\NotFoundException();
}
$fullPath = Filesystem::normalizePath($view->getAbsolutePath($path));
$fullPath = rtrim($view->getAbsolutePath($path), '/');
Copy link
Member

Choose a reason for hiding this comment

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

I'm not entirely sure if we should skip the normalization here as $path might get passed from public API and is used as a cache key which could trigger the uncached calculation then more often

Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

I'm uncomfortable getting this merged in a draft state as I cannot tell about any potentially leftovers and the change itself feels a bit risky to do as I fear there might be side effects whenever a non-normalized path is used in other code parts.

@blizzz blizzz mentioned this pull request Nov 20, 2023
5 tasks
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv added the 2. developing Work in progress label Feb 21, 2024
This was referenced Mar 12, 2024
@Altahrim Altahrim mentioned this pull request Mar 20, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
@skjnldsv skjnldsv modified the milestones: Nextcloud 30, Nextcloud 31 Aug 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants