JRuby on Windows

[AndyObtiva]

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

• Ensure all commits include DCO sign-off.
• Ensure that your contributions pass unit testing.
• Ensure that your contributions contain documentation if applicable.

Description

Please describe your pull request.

Hi, I encountered an issue with using git gem (via jeweler gem) on Windows running in JRuby.

Git::GitExecuteError: git '-c' 'color.ui=false' version   2>&1:git: ''-c'' is not a git command. See 'git --help'.
C:/Users/User/code/ruby-git/lib/git/lib.rb:989:in `command'
C:/Users/User/code/ruby-git/lib/git/lib.rb:895:in `current_command_version'
C:/Users/User/code/ruby-git/lib/git/lib.rb:905:in `meets_required_version?'
C:/Users/User/code/ruby-git/lib/git.rb:26:in `<main>'

It turns out it was not escaping strings properly because it was running in JRuby instead of CRuby, thus not detecting Windows correctly via RUBY_PLATFORM. I fixed it by adding a check for RUBY_DESCRIPTION too.

Cheers.

[jcouball]

In addition to the code comment, is there a test that fails without this change? The JRuby build seems to have been passing. If there isn't, can you please make one?

[jcouball]

How about shortening this to the more intention revealing:

def escape(s)
  return escape_for_windows(s) if windows_platform?

  escape_for_sh(s) # not sure what the best name for this method is: are we escaping for sh?  linux? linux_shell?
end

Then move the existing code to fill in the new methods referenced above. Then add a test (or more) for each method.

[AndyObtiva]

Sure. I agree that the suggested refactorings into intention revealing methods make the code a lot more readable/understandable.

[jcouball]

I re-ran the build and the status was updated. Not sure why it wasn't updating before.

[AndyObtiva]

In addition to the code comment, is there a test that fails without this change? The JRuby build seems to have been passing. If there isn't, can you please make one?

Good question. As you probably noticed, I've been trying to update the Travis CI build to include Windows. I think the current tests are sufficient if run on Windows as they test that escape(s) method indirectly via public methods instead. The reason the jruby build was passing is because it was running on Linux. It's a Windows compatibility problem after all on top of jruby.

I was able to add a Windows Ruby build and got 5 failures and a number of errors in it. I'm hunting them one by one on my machine right now.

The Windows JRuby build seems like no dice at the moment as Travis CI doesn't officially support it, so I'm manually scripting installation of JRuby with a Travis CI shell script, but if I don't get it working, the Windows Ruby build would probably suffice and should help quite a bit in testing Windows support in general with the only difference in JRuby being to check the RUBY_DESCRIPTION on top of RUBY_PLATFORM to ascertain Windows from non-Windows environments.

This is a fun break from some tough work I was busy with last week, so I'm more than happy to involve myself with this. I hope you're not in a rush. I'm taking my time given that I need a break from last week.

Thanks for acknowledging the bug and for providing code refactoring feedback that I agree makes the code more readable. I look forward to addressing all failing Windows Ruby tests and performing the refactoring in the foreseeable future.

Godspeed.

p.s. Once done, I'll make sure to rebase the latest code from trunk/master too since it's been a little while since I reported this..

[jcouball]

I hope you're not in a rush.

No rush, carry on at your own pace.

[jcouball]

If you want to break down this PR, you could add the new Travis CI builds first, in a separate PR. You can marking the Windows Ruby build(s) as optional so they don't impact others. It would be nice to have Ruby 2.7 added sooner than later.

[AndyObtiva]

If you want to break down this PR, you could add the new Travis CI builds first, in a separate PR. You can marking the Windows Ruby build(s) as optional so they don't impact others. It would be nice to have Ruby 2.7 added sooner than later.

Done.

I marked the Windows Ruby build(s) as optional.

I got both Ruby Windows and JRuby Windows working.

I took off Ruby 2.7 (it's a in a separate pull request now).

In summary, the changes mostly focus on removing reliance on bash shell commands that are incompatible with Windows, such as du (replaced with a pure Ruby repo size calculation), pwd (replaced with Ruby's higher-level abstract FileUtils.pwd), tar (replaced with pure Ruby gem "minitar", only needed for tests), and gzip (replaced with built-in Ruby Zlib implementation). The changes also fixed a few discovered minor test issues, including ones relating to Windows paths starting with directory letter (e.g. "C:"). Last but not least, I did the refactoring you recommended regarding escape(s).

Thanks for making the Ruby git library available to the open-source community.

Godspeed.

[jcouball]

I just had one comment about the encoding tests.

Also, can you please catch up to master and squash commits when you are ready?

[jcouball]

I don't think this is quite right. What do you think about this:

skip 'Encoding tests do not work in Windows' if windows_platform?

On other platforms, these tests should work not matter what the external encoding is.

[jcouball]

I forgot to mention... I have not ever seen rescue used in this way. Can you point me to something that explains how you are using it?

[AndyObtiva]

About your first question, if you can figure out a better way to skip tests on Windows, then power to you! After all, they do not work in their current form inside the Windows Command Prompt or Git Bash and I did not feel it was important enough for me to invest more time into it (especially given that there was zero coverage before to begin with, so any coverage now is better than nothing)

About your second question, I learned this from a senior Ruby developer called Chris Sepic more than a decade ago while working at this Chicago suburb company, Leapfrog Online.

Basically, in rare cases where you do not need to do anything with the error exception object, instead of doing this:

begin 
  try_something
rescue => e
  do_nothing_with_e
end

You can drop the error exception object:

begin 
  try_something
rescue
  do_something
end

And then finally, you can fold it into one line:

try_something rescue do_something

I don't advise using this in the majority of cases. I only used it because this was a test (not production code) and brevity helped keep things on one line.

Specifically, there is this odd quirk about JRuby where it calls the encoding constant UTF8 instead of UTF_8, so instead of me writing something very long and elaborate to check it such as:

if Encoding.constants.include?(:UTF_8)
  # do something with Encoding::UTF_8
elsif Encoding.constants.include?(:UTF8)
  # do something with Encoding::UTF8
end

I just shortened it knowing that if you call `Encoding::UTF_8` in JRuby, it bombs, and then the `rescue` keyword rescues it unto the next value to compare to, which is `Encoding::UTF8`

Again, this is not a recommended technique in general. I just did it because it was low-risk code in a test (spec).

[jcouball]

Same here.

[AndyObtiva]

Hi, it's been a month since I made the changes. I was wondering if there is anything else you need of me before merging this pull request.

I have been relying on my own fork (git-glimmer) for my project Glimmer DSL for SWT in the meantime.

I was hoping I would move back to the git gem trunk soon though.

Cheers,

Andy

p.s. Regarding skip 'Encoding tests do not work in Windows' if windows_platform?, I thought it was just a low-priority aesthetic thing that is just a matter of opinion and you could make the change yourself after merging. If you insist I make it instead, I'd be happy to if that is the hold up.