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

Add Win32::LongPath to support long paths on Windows #686

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

RyanMcC
Copy link
Contributor

@RyanMcC RyanMcC commented Oct 14, 2022

Note: This PR doesn't fully resolve the long path issue for Windows. The File::Find call will still fail when cloc is given an input with paths > 255 chars. However, the File::Find call can be skipped by either using a --list-file or by using the --no-recurse option.


I added wrapper functions for most of the Win32::LongPath things that needed substituting and updated all the existing usages that should need it. I didn't add an explicit flag for enabling this, it just works based off of $ON_WINDOWS and $HAVE_Win32_Long_Path for now. If you think it'd be better to have a flag to enable it, I could add that instead.

The two cases where it doesn't have wrapper functions is stat/statL and opendir/opendirL. For both of those, the Win32::LongPath version returns different types that need their own handling. One of the stat usages is for $size_in_bytes in make_file_list -- I'm not sure if there was a reason stat was used over -s for this, but if there's not it could be replaced with the get_size wrapper.


While working on this I also noticed a couple issues:

  • In sub make_file_list where it iterates over $ra_arg_list, the ternary to create $ul_F was incorrectly ordered. It was $upper_lower_map{$F} ? $ON_WINDOWS : $F but I assume it was meant to be $ON_WINDOWS ? $upper_lower_map{$F} : $F, so I updated that while I was there.
  • Another possible problem I noticed is with --no-recurse. I'm not sure if it's intentional, but it only works if you run cloc from within the target dir. readdir only returns the file names and not the whole path, so unless your cwd is in that dir, it won't find any files. I didn't change this at all, and the Win32::LongPath stuff I added there behaves the same as the existing code.

Also, it's not really an issue but I was curious -- @AlDanial would you be open to adding a variant of diff-list-file (maybe diff-list-files?) that accepts separate list-files rather than a combined one? It seems non-trivial to decide which files should be diffed together, and I think it'd be a lot easier to let cloc handle that. I think it would be pretty easy to implement too, it should just involve adding a third case for the make_file_list call (in the opt_diff branch) that reads the ARGV as a list-file and gives that to make_file_list. I can make another PR for that if you'd like.

@AlDanial
Copy link
Owner

I appreciate the detailed thought and effort that went into this PR. A cursory view of the diff looks fine and I plan to merge it. However please give me a couple of weeks to get there.

AlDanial added a commit that referenced this pull request Oct 31, 2022
AlDanial added a commit that referenced this pull request Oct 31, 2022
@AlDanial AlDanial merged commit 569030b into AlDanial:master Oct 31, 2022
@AlDanial
Copy link
Owner

@RyanMcC : thanks!

@RyanMcC
Copy link
Contributor Author

RyanMcC commented Oct 31, 2022

@AlDanial yw!

Also, would you be open to adding the the diff-list-files option I mentioned above (as a way to provide separate list files when diffing, rather than a single combined one)? It should be a pretty small change and I can open another PR for it.

@AlDanial
Copy link
Owner

AlDanial commented Nov 2, 2022

Have to admit that fell off my radar, thanks for reminding me. I'll certainly look favorably at a PR for that. One minor nit: on future subroutines please add entry and exit print statements when $opt_v > 2. There are plenty of existing examples to follow. I know that's a bit odd but it has proven helpful for troubleshooting in the past.

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