Use get_entry instead of get_str

[cruessler]

Related to #79 and #228.

Beware that the additional tests most likely will fail locally because the local git config interferes with them. This PR is intended for testing whether there are issues on CI.

[extrawurst]

Beware that the additional tests most likely will fail locally

how are you planning to work around this?

This PR is intended for testing whether there are issues on CI.

looks good doesn't it?

[cruessler]

Beware that the additional tests most likely will fail locally

how are you planning to work around this?

I don’t have a solution yet, so I’ll do a little research to see what other projects are doing.

This PR is intended for testing whether there are issues on CI.

looks good doesn't it?

Yes, the git config used on CI seems to not interfere with the new tests.

[cruessler]

@extrawurst I might have found a solution. Setting $HOME to an empty directory in repo_init_empty prevents git from falling back to $HOME/.gitconfig because that is now guaranteed to be empty. git itself does not provide more granular env variables as far as I could tell. I’m not 100 % sure whether undesired side-effects can occur if we just change $HOME in the middle of a test, though.

[extrawurst]

Sorry for the delay, this would break tests that depend on $HOME to be something useful, right? and since we run tests in parallel it would be non-deterministic.

[cruessler]

I did a little research to see how other tools tackle this issue, and this is what I found:

• tig explicitly sets $HOME to a temporary directory for its tests.

  https://github.com/jonas/tig/blob/b878abf5a0da2efbdd29b8e0215d4df0c7a8109f/test/tools/libtest.sh#L44

• git also uses this technique.

  https://github.com/git/git/blob/7814e8a05a59c0cf5fb186661d1551c75d1299b5/t/test-lib.sh#L1372

• libgit2 has tests that modify $HOME to make sure everything works.

  https://github.com/libgit2/libgit2/blob/2376cd26226771dcf8ef5dfd04c83a50c50bf3d4/tests/core/env.c#L120

I share your concerns, though, and, to be honest, I was quite surprised to find that this seems to be a common practice.

To make the tests independent of their order, we could set $HOME at the beginning of the test suite so that all tests share the same environment.

[extrawurst]

@cruessler thanks for researching this. Ok lets go ahead with this approach, but let's do one more thing: Let's limit the impact of this change to the test that requires this instead of all using init_repo_empty.

We should make a utility that helps us set the env and protect it using a mutex guard that we can use to have multiple tests run in serial that depend on this behaviour, also this utility can revert the changes to env when dropped at the end of the test. Does that make sense to you?

can you make those changes and mark the PR as ready to review?

[cruessler]

@extrawurst Sounds good to me! I'll update the PR in the next few days.

[cruessler]

It looks like changing $HOME just for one test is not trivial. This is due to the following facts:

• libgit2 has some global state that is initialized only once. This happens e. g. on the first call to Repository::init.
• The initialization calls git_libgit2_init which sets up some global state.
• In particular, the current value of $HOME is read and cached.

That means that whatever value $HOME had when Repository::init was first called will be used for further calls to repo.config(), among others. Effectively, all tests within the same process run with the same value for $HOME.

I briefly tried rusty-fork, but could not get it to work quickly.

[extrawurst]

@cruessler omg why do they make it so hard on us? :)
Look like we need to call git_libgit2_shutdown (https://github.com/rust-lang/git2-rs/blob/master/libgit2-sys/lib.rs#L1829) before we run a test that is HOME sensitive cause it is supposed to wipe clean those caches, right?

[cruessler]

@extrawurst Looks like that is not an option, either. :-) We would have to depend on libgit2-sys in asyncgit which produces the following error message:

error: multiple packages link to native library `git2`, but a native library can be linked only once

I searched far and wide, but git_libgit2_shutdown does not seem to be exposed by git2 itself, so there’s no way to call it as far as I can see.

[extrawurst]

looks like u have an opportunity to become a git2-rs contributor :D

[cruessler]

@extrawurst There is already an issue at git2-rs regarding environment variables. They’re considering mentioning this behaviour in the docs, so I don’t have the impression that git2-rs will be exposing git_libgit2_shutdown anytime soon. :-)

[extrawurst]

@cruessler as far as I can see no one ever raised the idea of exposing shutdown so I did now: rust-lang/git2-rs#550 (comment)

[extrawurst]

@cruessler also maybe this change is relevant for us: rust-lang/git2-rs#656 ?

[cruessler]

@cruessler also maybe this change is relevant for us: rust-lang/git2-rs#656 ?

Thanks for the hint! I just checked locally, and on my machine the issue was solved and the new tests were green. I’ll update the branch, so we can see what CI says.

[cruessler]

There is only one downside: set_search_path is unsafe and asyncgit forbids unsafe code.

[extrawurst]

@cruessler well that's a concession I can make. can you quickly add that to the changelog aswell, then I can merge it 👍

[cruessler]

@extrawurst Great, done!

[extrawurst]

@cruessler Thanks❤️