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 windows support for git-fastclone #65

Merged
merged 5 commits into from
Dec 14, 2023

Conversation

DEMON1A
Copy link
Contributor

@DEMON1A DEMON1A commented Dec 9, 2023

Created a new function for the path generation that validates the current operation system before generating the path, and uses underscore in-case a windows system is being used to make sure git-fastclone doesn't use a non-allowed characters for windows filenames that prevents the submodules and lock files from being created

Created a new function for the path generation that validates the current operation system before generating the path, and uses underscore in-case a windows system is being used to make sure git-fastclone doesn't use a non-allowed characters for windows filenames that prevents the submodules and lock files from being created
Copy link
Contributor

@justinseanmartin justinseanmartin left a comment

Choose a reason for hiding this comment

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

Two minor pieces of feedback, but happy to land after these changes, or happy to discuss if you have pushback.

lib/git-fastclone.rb Outdated Show resolved Hide resolved
lib/git-fastclone.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@justinseanmartin justinseanmartin left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the contribution!

@justinseanmartin
Copy link
Contributor

It looks like there are some minor rubocop violations to take care of. Once that's done, I'll merge this PR. Would you like me to cut a bugfix release with the changes?

@DEMON1A
Copy link
Contributor Author

DEMON1A commented Dec 13, 2023

I can see where it fails, should combining the if statement as a one liner solve the issue? Something like this

RbConfig::CONFIG['host_os'] =~ /mswin|msys|mingw|cygwin|bccwin|wince|emc/ ? '__' : ':'

Robucop didn't say anything about the code style when I was modifying git-fastclone, I think it's better if you correct those code style violations on your side if you don't mind, I can't really validate the code style locally

@justinseanmartin
Copy link
Contributor

The rubocop fix looks like this:

diff --git a/lib/git-fastclone.rb b/lib/git-fastclone.rb
index 82326d5..18a67a7 100644
--- a/lib/git-fastclone.rb
+++ b/lib/git-fastclone.rb
@@ -49,10 +49,10 @@ module GitFastClone
 
     def reference_filename(filename)
       separator = if RbConfig::CONFIG['host_os'] =~ /mswin|msys|mingw|cygwin|bccwin|wince|emc/
-                              '__'
-                            else
-                              ':'
-                            end
+                    '__'
+                  else
+                    ':'
+                  end
       "#{separator}#{filename}"
     end
     module_function :reference_filename

It should produce this for you automatically if you run bundle exec rubocop -a. I don't have push access to your repo, but I could PR this to your repo if you can't apply the patch of get rubocop to run successfully locally.

@justinseanmartin justinseanmartin merged commit 29e272f into square:master Dec 14, 2023
5 checks passed
@justinseanmartin
Copy link
Contributor

Ok, git-fastclone 1.4.5 has been published with this fix.

This pull request was closed.
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