Skip to content

Conversation

mickhawkins
Copy link
Member

Tries the "original" path for the files needing permission fixes, then re-tries with public/ if the file can't be found, before failing if it still can't be found.

@codecov-commenter
Copy link

codecov-commenter commented Sep 9, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.39%. Comparing base (de5e71d) to head (36a1004).

Additional details and impacted files
@@            Coverage Diff            @@
##               main     #106   +/-   ##
=========================================
  Coverage     85.39%   85.39%           
  Complexity       86       86           
=========================================
  Files             2        2           
  Lines           267      267           
=========================================
  Hits            228      228           
  Misses           39       39           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@mickhawkins mickhawkins force-pushed the fixpermissions-public branch from c4873af to 78009a3 Compare September 9, 2025 06:55
Copy link
Member

@snake snake left a comment

Choose a reason for hiding this comment

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

I can see what this is trying to do, but I wonder if it's perhaps too complex for what needs to be done.

Can we not simply make the following change at the top of the existing foreach:

$realpath = realpath("$sourcedir/public/$relname");

// Fallback, to handle legacy branches which don't use the 'public/' prefix.
if ($realpath === false) {
    $realpath = realpath("$sourcedir/$relname");
}

// Other code continues as it was..

@mickhawkins
Copy link
Member Author

Hi @snake,

Good pickup - the info about realpath() I was looking at was a bit confusing and it wasn't super clear that it checked for the existence of the file, plus there's already a file exists check in the code. It seems like that second check is actually redundant, and I don't think it's even possible to hit it. I'll do an updated patch shortly for this.

The only thing that'll be missing is my patch provided a bit more verbose output (if it failed both, it would say so, whereas this will just say the final path). I might just update the text on the error a bit so it's clear both have failed if it reaches that point.

@mickhawkins mickhawkins force-pushed the fixpermissions-public branch from 78009a3 to 36a1004 Compare September 9, 2025 09:09
@mickhawkins
Copy link
Member Author

Hi @snake,

I've pushed the updated branch. Given realpath() checks the file exists, the file_exists check was totally redundant and unreachable code so I've removed it. It was also possible to simplify the loop a bit further, setting $executables doesn't need to be in an if/else because the code exits on error.

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