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

Fix recursive directory nesting in copy2tmp method #126

Merged
merged 1 commit into from
Mar 9, 2024

Conversation

Foadsf
Copy link
Contributor

@Foadsf Foadsf commented Jan 25, 2024

I resolved the issue causing excessive directory nesting in the copy2tmp method. I replaced Dir.entries with Dir.children to avoid including '.' and '..' in the file list, which prevented recursive self-reference. I added checks to appropriately handle symbolic links and deeply nested directories appropriately, ensuring correct file copying without creating unnecessary nested structures. They were tested with various directory setups to confirm the fix.

@engineersmnky
Copy link

This relates to changes made in ruby 3.1. See this Commit

@fechmistrz
Copy link

What needs to happen before this gets merged? I've tested this fix in my repo with symlinks and am very happy with the results.

@reitzig
Copy link
Owner

reitzig commented Mar 6, 2024

Nothing on your end. 👍

Sorry for the delay. I'll have to find the time to get back into this context, however briefly, to give the PR appropriate attention.

Since there's no proper installation method, anyway 🙈, I hope you are fine with using the tool off the branch for the time being.

@reitzig
Copy link
Owner

reitzig commented Mar 6, 2024

To be clear, this tool has never been tested with Ruby 3 on my end. While I am happy to believe that this PR fixes the problem at end, I feel I should

  • decide whether to drop support for Ruby 2 altogether,
  • make sure it works for both major versions, and/or
  • give it a once-over to check for other regressions.

That's more than a few minutes work, unfortunately. 😅

@koppor
Copy link
Contributor

koppor commented Mar 6, 2024

Working for both major versions seems to be maintenance nightmare.

I vote for dropping Ruby 2 support and checking the tool for the latest Ruby version only.

(Even though one could run the tool using Docker etc., I think, making it "easy" to run locally is more awesome)

@koppor
Copy link
Contributor

koppor commented Mar 6, 2024

@reitzig I updated PR #121 to include Ruby 3.x versions, hope, this is helpful for you:

image

@reitzig
Copy link
Owner

reitzig commented Mar 9, 2024

Thanks, @koppor, I've set up a working workflow based on your PR. 🙏

I vote for dropping Ruby 2 support and checking the tool for the latest Ruby version only.

Agreed, and done. Even though my embarassingly outdated home machines has Ruby 2.x running, all those versions are EOL. Good riddance!

Now, to rebase this. 🍿

Resolved the issue causing excessive directory nesting in the copy2tmp method. Replaced Dir.entries with Dir.children to avoid including '.' and '..' in the file list, which prevented recursive self-reference. Added checks to handle symbolic links and deeply nested directories appropriately, ensuring correct file copying without creating unnecessary nested structures. Tested with various directory setups to confirm the fix.
@reitzig
Copy link
Owner

reitzig commented Mar 9, 2024

LGTM, thank you!

fixes #125

@reitzig reitzig merged commit 5488e67 into reitzig:master Mar 9, 2024
6 checks passed
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.

5 participants