Remove calls to Dir.chdir

[fxposter]

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

In multithreaded environment Dir.chdir changes current directory of all process' threads, so other threads might misbehave.

Base#chdir, Base#with_working and Base#with_temp_working were left intact, cause they are not used internally, so it's an explicit user decision to use them.

Fixes #355

[fxposter]

I rewrote the functionality using Open3 library instead of IO.popen. This should fix the issues with jruby on windows not supporting chdir.

[fxposter]

cc @jcouball

[jcouball]

@fxposter thank you very much for this PR and #675

This PR is still failing for JRuby on Windows. I faced the same problems when trying to solve these problems and more (#355, #363, #441, #495, #516, #574).

I almost have that solution over the finish line with #617. This is (to the best I can tell) only missing one JRuby issue which is tracked in jruby/jruby#7515.

I am wondering if you gave #617 a try? I would love feedback on that one.

[fxposter]

I'll take a look, but do you really believe that you need to have "process execution" abstraction instead of using the existing ones? :)

PS. I'll get to a windows machine on the weekend, will try to check what is going on there, cause open3 specifically have code that should handle chdir/etc: https://github.com/ruby/open3/blob/master/lib/open3/jruby_windows.rb#L81

[fxposter]

@jcouball can you review and run tests again?
I have found what was wrong with windows - due to the fact that tests use Dir.chdir themselves - when we passed chdir to open3 it didn't "inherit" it from the current Dir.chdir call that was done by tests:

# imagine that we run this script in dir /x

Dir.chdir('/y') {
  Dir.chdir('.') {
    Open3.capture2('ls') # will show files in /y
  }
}

Dir.chdir('/y') {
  Open3.capture2('ls', :chdir => '.') # will show files in /x
}

to avoid that I do :chdir => File.expand_path('.') which properly expands relative paths.

[fxposter]

Also, before merging: I rewrote code like

Dir.chdir(dir) { files = Dir.glob('**/*').select { |f| File.file?(f) } }

to

files = Dir.glob('**/*', :base => dir).select { |f| File.file?(f) }

The end result is the same, but :base is only supported since ruby 2.5. If that is a problem - I can rewrite the code without using :base keyword argument.

[jcouball]

Thanks @fxposter, we are getting very close! Only one error left in the Windows JRuby build.

I see that the failure is in test_tree_ops.rb in a with_index block.

My first comment is that I am not a fan of this test case since it is just one big test case with a lot of state changing and asserts sprinkled through the method. An isolated, minimal, failing test case would be ideal but not necessary.

The first think I would try is to use Git::Base#with_temp_index instead of Git::Base#with_index. I see that there is code in that method to workaround some problem in JRuby when using Tempfile exactly the way the test does.

I honestly don't understand why the with_index block is being used in the first place. Maybe to use a "throw away" index?

Beyond that, maybe something related to the changes you are making might stick out to you.

I have changed the configuration of the GitHub actions for the CI build so that the build will run when you push changes. This way you won't have to wait for me or another contributor for feedback.

[fxposter]

:(

I hoped that the last change fixes everything on jruby on windows. The tricky part here is that locally some tests are always failing for me on both Mac and Windows (while they pass in CI), so I assumed I fixed everything.

I'll try to focus on the failing one, as I have access to windows machine during the weekend.

[jcouball]

@fxposter

Regarding your comment about which Ruby release should be supported, here is release information for some Ruby releases from the Ruby Maintenance Branches page:

Ruby 3.0
status: security maintenance
release date: 2020-12-25
normal maintenance until: 2023-04-01
EOL: 2024-03-31 (expected)

Ruby 2.7
status: eol
release date: 2019-12-25
normal maintenance until: 2022-04-01
EOL: 2023-03-31

Ruby 2.6
status: eol
release date: 2018-12-25
normal maintenance until: 2021-04-01
EOL: 2022-04-12

Ruby 2.5
status: eol
release date: 2017-12-25
normal maintenance until: 2020-04-01
EOL: 2021-04-05

I think for this PR we could change the required version of Ruby in the gemspec to 2.5.

I notice that the CI build only goes back to 2.7 so there should probably be a build added for MRI 2.5 on Linux at least.

It would be my intention to follow up with an explicit policy that this gem follows for supported Ruby version in a separate PR. The policy could be something like "Support all non-EOL Ruby minor versions" (currently 3.0, 3.1, and 3.2). We could possibly include the latest EOL minor version as well (which would add 2.7 until 3.0 is EOL'd in March 2024).

[jcouball]

The tricky part here is that locally some tests are always failing for me on both Mac and Windows (while they pass in CI)

Huh, that shouldn't be and is concerning. If the CI builds pass and the local build works for me (which it does), they I'd accept the pull request. If you want to share details of your failure(s) we can try to debug. I am on Mac so that is where I can be most helpful.

[fxposter]

sure. this is what I get on my mac by default (probably it's related to my user git's config, but I didn't try to debug that yet, since it failed on master as well):

 $ rake
ruby bin/test
Loaded suite bin/test
Started
E
=====================================================================================================================================================================================================================================================
Error: test_push(TestRemotes):
  Git::FailedError: git '--git-dir=/private/var/folders/m3/lnm6c86s2m99_bx5n8c4j_ww0000gn/T/d20231118-67431-4pwmvb/local/.git' '--work-tree=/private/var/folders/m3/lnm6c86s2m99_bx5n8c4j_ww0000gn/T/d20231118-67431-4pwmvb/local' '-c' 'core.quotePath=true' '-c' 'color.ui=false' 'push' 'testrem'  2>&1
  status: pid 70744 exit 128
  output: "fatal: You are pushing to remote 'testrem', which is not the upstream of\nyour current branch 'master', without telling me what to push\nto update which remote branch.\n"
/Users/pavelf/Projects/github/ruby-git/lib/git/lib.rb:1234:in `command'
/Users/pavelf/Projects/github/ruby-git/lib/git/lib.rb:996:in `push'
/Users/pavelf/Projects/github/ruby-git/lib/git/base.rb:396:in `push'
/Users/pavelf/Projects/github/ruby-git/tests/units/test_remotes.rb:226:in `block in test_push'
     223:       assert(!rem.status['test-file1'])
     224:       assert(!rem.status['test-file3'])
     225:
  => 226:       loc.push('testrem')
     227:
     228:       assert(rem.status['test-file1'])
     229:       assert(!rem.status['test-file3'])
/Users/pavelf/Projects/github/ruby-git/tests/test_helper.rb:68:in `block in in_temp_dir'
/Users/pavelf/Projects/github/ruby-git/tests/test_helper.rb:67:in `chdir'
/Users/pavelf/Projects/github/ruby-git/tests/test_helper.rb:67:in `in_temp_dir'
/Users/pavelf/Projects/github/ruby-git/tests/units/test_remotes.rb:205:in `test_push'

[jcouball]

@fxposter what is the output of

git --version && git config --list --show-origin

[jcouball]

Looks like you might have push.default set to upstream. I'll prepare a PR to explicitly set the push.default for this test.

[fxposter]

color.ui=auto
push.default=tracking
push.autosetupremote=true
branch.autosetuprebase=always
init.defaultbranch=master

and lots of others, probably unrelated

[jcouball]

It looks like push.default=tracking gives the same result as push.default=upstream.

[jcouball]

@fxposter if you rebase with master, test_push(TestRemotes) should pass in on your Mac now.