Refactor code for finding home directory on Windows

[jdblischak]

The main change in this PR is refactoring ssh_path to create a new function home that returns the user's home directory regardless of operating system. It is currently used by ssh_path and config.

xref: #317 #320

It also has two other minor changes:

1. The empty .gitconfig is only created if the user has specified configuration options
2. I fixed a typo in my previous unit test. I passed email to config instead of user.email. This generated a warning message on my local Windows machine. Should that have been detected on AppVeyor? I used the conditional statement used by testthat::skip_on_appveyor, but I don't know if that works outside of testthat (in other words, is the environment variable "APPVEYOR" created by AppVeyor or testthat?).

[coveralls]

Coverage increased (+0.01%) to 78.531% when pulling fa9872f on jdblischak:windows-home into 95115f9 on ropensci:master.

[jdblischak]

The good news is that AppVeyor is running my tests. The bad news is that the tests are failing!

It failed on the initial check that .gitconfig does not exist after just checking the configuration. This suggests that perhaps there is already a .gitconfig file present on AppVeyor. Based on the appveyor.yml in this repo and the appveyor helper script from r-appveyor, I don't expect there to be a .gitconfig present.

I'm not sure how to debug this. The tests pass for me on my local Windows machine:

export APPVEYOR=True
make check

Any ideas?

[stewid]

I look into it tomorrow

[jennybc]

I'd consider calling this git_home() for added clarity.

[jdblischak]

But it's not specific to Git because it's currently used to find the home directory for both .gitconfig and .ssh/. What about calling it something like user_home() or home_user()?

[jdblischak]

@stewid Your opinion?

[stewid]

... code for finding home directory ...

What about home_dir?

[jennybc]

Seems like this should be exploiting the new 'git_config_files() function to detect existing config, for internal consistency.

[jdblischak]

Seems like this should be exploiting the new 'git_config_files() function to detect existing config, for internal consistency.

@jennybc Good idea! I updated the code to check for the existence of global config file using git_config_files().

[stewid]

I added artifacts to appveyor.yml to debug and found that "C:\Users\appveyor/.gitconfig" already exists.

> ## Check location of .gitconfig on Windows
> if (identical(Sys.getenv("APPVEYOR"), "True")) {
+   ## TEST
+   str(Sys.getenv("USERPROFILE"))
+   str(Sys.getenv("HOMEDRIVE"))
+   gitconfig_expected <- file.path(Sys.getenv("USERPROFILE"), ".gitconfig")
+   str(gitconfig_expected)
+   str(file.exists(gitconfig_expected))
+ 
+   config(global = TRUE, user.name = "name", email = "email")
+   gitconfig_expected <- file.path(Sys.getenv("HOMEDRIVE"), "Users",
+                                   Sys.info()["login"])
+   stopifnot(file.exists(gitconfig_expected))
+   unlink(gitconfig_expected)
+ }
 chr "C:\\Users\\appveyor"
 chr "C:"
 chr "C:\\Users\\appveyor/.gitconfig"
 logi TRUE

Maybe it works to:

1. Move "C:\Users\appveyor/.gitconfig" to a temp-config-file
2. Run tests
3. Move temp-config-file "C:\Users\appveyor/.gitconfig"

[jdblischak]

I added artifacts to appveyor.yml to debug and found that "C:\Users\appveyor/.gitconfig" already exists.

OK. That explains the problem then. Thanks!

Move "C:\Users\appveyor/.gitconfig" to a temp-config-file
Run tests
Move temp-config-file "C:\Users\appveyor/.gitconfig"

Yes, I'll temporarily move that file for these tests (and only on AppVeyor).

[jennybc]

Why do you want to move .gitconfig? Would it make more sense to write a config test that operates on local git config for a temporary repo? That would seem to require less special case-y ness.

If you're going to do such things, withr is a very handy package for doing some work in a temporarily modified state, with automatic cleanup/restoration. I don't know how much this comes up in other tests -- this would probably be a testing shift beyond the scope of this one test or PR. But worth thinking about.

[jdblischak]

Why do you want to move .gitconfig? Would it make more sense to write a config test that operates on local git config for a temporary repo? That would seem to require less special case-y ness.

Because this unit test is to test if config() creates the global .gitconfig in the correct location on Windows.

If you're going to do such things, withr is a very handy package for doing some work in a temporarily modified state, with automatic cleanup/restoration. I don't know how much this comes up in other tests -- this would probably be a testing shift beyond the scope of this one test or PR. But worth thinking about.

withr looks super useful. I am going to have to start incorporating it into the unit tests for my own packages. However, since this isn't my repo, I am trying to follow the style of the existing tests. I'm happy to use withr if you'd prefer I do.

[stewid]

I agree, withr looks very useful. However, for this single case I prefer to keep it as it is and not add a dependency.

[jdblischak]

I found the bug that is causing the test to fail (at least the test is working like it should...). I'm re-building (which takes forever on Windows) and testing locally first, but will push update soon.

[stewid]

Thanks