Repository: sharkdp/bat (language: rust)

Issue:
BAT_CONFIG_PATH ignored by --generate-config-file
<!-- Hey there, thank you for creating an issue! -->

**What version of `bat` are you using?**

<!-- Output of `bat --version` -->
Newest

**Describe the bug you encountered:**
![image](https://user-images.githubusercontent.com/55729509/108675764-3694ee80-7522-11eb-9c45-3337c8d63dc7.png)


**What did you expect to happen instead?**

use custom location instead


**Windows version:**

![image](https://user-images.githubusercontent.com/55729509/108675495-d736de80-7521-11eb-81ef-032ba4309f1f.png)



Discussion / hints:
Thank you for reporting this. Looks like a bug indeed.
I can confirm this bug, which is generic. It is caused by
```
        .filter(|config_path| config_path.is_file())
```
in
```
pub fn config_file() -> PathBuf {
    env::var("BAT_CONFIG_PATH")
        .ok()
        .map(PathBuf::from)
        .filter(|config_path| config_path.is_file())
        .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
}
```
which makes sense when _reading_ config values, but of course not when _creating it_.

Made a proof of concept fix (see above), but am not happy with how it turned out. Plus, it needs integration tests. Will take care of that at some later point when I get time, unless someone beats me to it (which I encourage).
> which makes sense when _reading_ config values, but of course not when _creating it_.

Let's discuss this first. Maybe it doesn't make sense for _reading_ either.

Agreed: when *writing* with `--generate-config-file`, we should always simply write to the path returned by `config_file()` (maybe rename that function to `config_file_path()`). We should print an error if that path can not be written to (e.g. missing permission or path exists and is a directory).

When *reading* the config file, we could distinguish between two cases:
1. `BAT_CONFIG_PATH` has not been set => read from the default path, but ignore if it is not existent. Print an error in case of permission / file-type problems.
2. `BAT_CONFIG_PATH` has been set => I think we should always try to read from that path and maybe print an error/warning if it does not exist. Maybe we should even ignore it, just like in case 1. In case of permission / file-type problems, we print an error.

I guess `config_file` would be most useful if it always just returns the path where `bat` expects the config to be. Even if that path does not exist. That would mean that we could simply remove the `.filter(…)` call.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 44aa42c429..2155c15f40 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -16,6 +16,7 @@
 - If `PAGER` (but not `BAT_PAGER` or `--pager`) is `more` or `most`, silently use `less` instead to ensure support for colors, see #1063 (@Enselic)
 - If `PAGER` is `bat`, silently use `less` to prevent recursion. For `BAT_PAGER` or `--pager`, exit with error, see #1413 (@Enselic)
 - Manpage highlighting fix, see #1511 (@keith-hall)
+- `BAT_CONFIG_PATH` ignored by `bat` if non-existent, see #1550 (@sharkdp)
 
 ## Other
 
diff --git a/src/bin/bat/config.rs b/src/bin/bat/config.rs
index e7ede5a306..93ce8aaaac 100644
--- a/src/bin/bat/config.rs
+++ b/src/bin/bat/config.rs
@@ -10,13 +10,12 @@ pub fn config_file() -> PathBuf {
     env::var("BAT_CONFIG_PATH")
         .ok()
         .map(PathBuf::from)
-        .filter(|config_path| config_path.is_file())
         .unwrap_or_else(|| PROJECT_DIRS.config_dir().join("config"))
 }
 
 pub fn generate_config_file() -> bat::error::Result<()> {
     let config_file = config_file();
-    if config_file.exists() {
+    if config_file.is_file() {
         println!(
             "A config file already exists at: {}",
             config_file.to_string_lossy()
@@ -71,7 +70,14 @@ pub fn generate_config_file() -> bat::error::Result<()> {
 #--map-syntax ".ignore:Git Ignore"
 "#;
 
-    fs::write(&config_file, default_config)?;
+    fs::write(&config_file, default_config).map_err(|e| {
+        format!(
+            "Failed to create config file at '{}': {}",
+            config_file.to_string_lossy(),
+            e
+        )
+    })?;
+
     println!(
         "Success! Config file written to {}",
         config_file.to_string_lossy()


Test patch:
diff --git a/tests/integration_tests.rs b/tests/integration_tests.rs
index 87c587a47c..82473e9132 100644
--- a/tests/integration_tests.rs
+++ b/tests/integration_tests.rs
@@ -652,6 +652,13 @@ fn config_location_test() {
         .assert()
         .success()
         .stdout("bat.conf\n");
+
+    bat_with_config()
+        .env("BAT_CONFIG_PATH", "not-existing.conf")
+        .arg("--config-file")
+        .assert()
+        .success()
+        .stdout("not-existing.conf\n");
 }
 
 #[test]