Add a special case for git config discovery inside tests

[aleksator]

Fixes #7469: Some tests will fail if one have a local git config user.name/user.email

[rust-highfive]

r? @ehuss

(rust_highfive has picked a reviewer for you, use r? to override)

[ehuss]

Thanks! This looks like it changes the behavior when config files fail to load. Previously errors were ignored, and I think it would be good to preserve that behavior.

[aleksator]

I have restored the previous behavior as requested. I went even further with env::current_dir()?, handing the error in place: now default git config is returned if that happens.

It bothers me a bit that the code to do that turned out to be not very elegant:

    let current_working_directory = env::current_dir();

    if current_working_directory.is_err() {
        return GitConfig::open_default().ok();
    }

    let current_working_directory = current_working_directory.unwrap();

There are probably some combinators that could be applied here to seamlessly integrate with the code that goes after it, but I couldn't come up with any. Suggestions welcome!

[ehuss]

This can be simplified with a match.

Suggested change

	let current_working_directory = env::current_dir();
	if current_working_directory.is_err() {
	return GitConfig::open_default().ok();
	}
	let current_working_directory = current_working_directory.unwrap();
	GitRepository::discover(current_working_directory)
	.and_then(|repo| repo.config())
	.or_else(|_| GitConfig::open_default())
	.ok()
	match env::current_dir() {
	Ok(cwd) => GitRepository::discover(cwd)
	.and_then(|repo| repo.config())
	.or_else(|_| GitConfig::open_default())
	.ok(),
	Err(_) => GitConfig::open_default().ok(),
	}

[aleksator]

Ah I see. I was trying to cram it into one chain to avoid duplicating GitConifg::open_default() call (is that even possible?), but your suggestion looks quite clear

[ehuss]

This as_path isn't necessary. You can pass &test_git_config to open() and the auto-deref will convert it as necessary.

[aleksator]

Thanks for the suggestion! Applied

[aleksator]

@ehuss There are now two additional commits as a result of the review, should I squash everything into one for cleaner history?

[ehuss]

Yea, if you could squash, that would be great. We're often not too careful about that here, but I do prefer a cleaner history.

[aleksator]

Done. Didn't want to do that during the review to make it easier to view the changes.

[ehuss]

Thanks! 😄

@bors r+

[bors]

📌 Commit 0f51c48 has been approved by ehuss

[bors]

⌛ Testing commit 0f51c48 with merge 6256eca...

[bors]

☀️ Test successful - checks-azure
Approved by: ehuss
Pushing 6256eca to master...