fix core-editor ignored (#414)

[pm100]

#414

clean branch this time - i think.

This fixes the following issues

• core.editor config value ignored
• config.get_str always fails (add new helper function)
• external editor fails to launch if it has spaces in it (vscode on windows I'm looking at you)

Notes

• I tried real hard to fix the 2 other config.get_str (in commit) but could not. I dont know enough about error conversions. I got it working but then clippy shot me down. I did verify that they always fail. If user.name is set this will still return "unknown".
• I did not fix the parsing of editor commands args that may have spaces in them (as per comments in the code)

[WizardOhio24]

I was thinking, could this function not just return a ConfigOptionNotFound error(a new error type that would need to be added) if the entry was not found, rather than none. Not finding a config option in Git2rs is an error, I think. With that in mind, an ultra succinct rust way to write this function could be:

    bytes2string(
        repo(repo_path)?
            .config()?
            .get_entry(key)
            .map_err(|_| error::ConfigOptionNotFound)?
            .value_bytes(),
    )

This also tests that the string is valid utf8 (using the function below), so we can throw a utf8 error, rather than a git2rs error. I have not tested this but something like that should work. This would also mean returning only Result<String> rather than Result<Option<String>>

[WizardOhio24]

With the change to get_config_string, the ? can be removed

[WizardOhio24]

Try not to use expect, this means the program will crash, should be returning an Err() here using match.

Also, perhaps a better message here would be 'Environment config variable is empty, no editor selected, check "GIT_EDITOR", "core.editor", "VISUAL" and "EDITOR" environment variables'. It would be good as well to tell the user which variable was at fault, but that's probably unnecessary given the user should really never have this error.

[WizardOhio24]

Thanks for the PR. And Thanks for fixing the commit history from the previous one :)

[pm100]

do not do .ok() thats what was wrong before. There are 3 returns from reading a config string

1 here is the value of "foo"
2 there is no value for "foo"
3 an error occurred while trying to read the config

In the original code with get_str it was actually trying to return 3 (git ALWAYS fails when using get_str) but the calling code did ok() on it and hence was always tresting this as though the value was not set (the code that read user.name and user.email). When I copied the same logic to the external editor module it did the same thing, always said it was not set. I investigated and found that get_str always fails ( see the linked git2 bug)

My view is that the 'set or not set' is a perfect Option case, and 3 should be an error return.

Hence I declared it as Result<Option<String>>

When I said I could not make the error handling for user.name and user.email. I could not work out how to convert the error potentially returned by get_config_string, I do not know enough about all the Froms for the different error types. I tried declaring get_config_string as Result<Option<String>, git2_Error>, this allows me to pass back errors from the git calls, but other failure cases do not return git2_error

[pm100]

There are no crashes in the case where things are not defined. I have tested all scenarios.

[WizardOhio24]

This can cause GitUI to crash on opening the editor,

If Err() was returned here instead, with a good error message, that would bubble up into GitUI and the program would not crash.

[WizardOhio24]

Also, with the choice between result or result<option>, I see why you would choose option if you were making an API, it makes more sense, but for standardizing errors in GitUI, it would be more useful for it to be an error. For example, if None is returned, a string will be output saying that, but it couldn't output the error, because it's just None, which wouldn't make sense to the user. So if we returned an error on config not found, GitUI could just output the error, without need to check for None or anything, it would output exactly what the error said. This is a place where None could be used, but here I think it would be easier to give an Err, which we can check where the function is called. The two options are equivalent, but a defined Err would be easier in our case. Everything else seems to handle this through a result, for example env::var and git2rs, this would keep it in line with them.

[pm100]

None is not an error , it means the value is not set, the code then carries on down the chain of or_elses

[pm100]

WHat did you do to cause that error.

[pm100]

OK. ty you found the one thing I did not test. On windows it is not possible to set a zero length env variable. But on unixen you can, I will fix

[pm100]

ok, the behavior is now what it was before. if GIT_EDITOR is set to "" (empty string) then gitui will try to launch an editor called "". This is what it used to do. Arguably it should simply move on to the next settings.

[WizardOhio24]

Could you just return Err in this case, because that can happen with any of those, if GIT_EDITOR is not set, it would happen with core.editor and that error message is less than helpful if an environment variable is set incorrectly. What you've added seems like a workaround from using Err just to keep using expect(which we should not use in GitUI so it doesn't crash (unless absolutely necessary)). Is there any reason we shouldn't use a match and return Err if the environment variable is set incorrectly? It would just provide a better error message and remove the expect from the code (one less place GitUI can fail).

[pm100]

tell me what problem you are trying to fix and what the behavior you want is. I will work out how to implement it.

[WizardOhio24]

Ok, so the error message is not helpful. The user wouldn't know what to do if they saw this. Also, the code should have as few unwraps() or expects() as possible. If there were none, GitUI could never crash, which would be the best but there are sometimes where you cannot not crash (though this is not one of those times). So what I'm saying is the expect could be removed and replace with match where Ok means there was a valid env variable, and Err means it was not valid, Err could then return Err(error...). In the first review comment I mentioned a possible better error message that could be returned, but the choice is yours, it just needs to better explain why it's erroring. This could be done with error::generic because this is probably a one-off or by making a new error. error::generic is probably better here. Doing this means the error message will be displayed in that error box, rather than ' "":no such file or directory '(which doesn't help to fix the error).

[pm100]

OK, behavior is now exactly like it was originally. If you set editor config to "" you get this

I have to say that I think "cannot run editor "" " is clearer, but dont really mind.

[WizardOhio24]

That error message isn't any better than the last one! And though "cannot run editor "" " is clearer, it is still really unhelpful. The user should be able to read the error message and understand what has gone wrong and the error message should include a way to fix the error, so the user doesn't need to ask for help. I would never expect a user for the target audience of GitUI to be able to look at "unable to read editor command" and from it imply that the VISUAL environment variable was set to "" and the others which git could use were unset.

[pm100]

well since this is the original behavior, has nothing to do with this bug nor is caused by the code in this PR why not open a new bug

[WizardOhio24]

Fair enough. But next, could we remove the expect then and use .ok_or_else(|| anyhow!("Unable to launch editor"))? in it's place. Just so GitUI won't crash at this. And I know with all the checks it won't crash right now, but we should only need one and it can be in place of where the expect is right now.

[pm100]

what crash is still there, I thought I fixed them all. Really dont want to do ok since this is the original problem, treating serious error as 'not set'

[WizardOhio24]

What do you mean, this is the original problem? What was the original problem in relation to this fix?

[WizardOhio24]

This is just replacing the expect after peak(). Just so there is only one check. It is the same as it is right now, but now only has one check. (And no expect)

[pm100]

the peek cannot fail. It only fails if the length is < 1, which I now test for

[WizardOhio24]

I know the peak cannot fail, but I'm saying there is a better way to check for it than the current way you are doing. Using .ok_or_else(|| anyhow!("Unable to launch editor"))? instead of .expect and removing the other checks.

[pm100]

'unable to launch editor ' is not very helpful either, sounds like a bug in the editor

[WizardOhio24]

"Unable to read editor command", I meant, or whatever the current one is. There really needs to be a fix for that error message to make it helpful but that can be a different PR.

[pm100]

I have lost the thread here: the code doesnt crash, does the correct thing. I think you just dont like the expect on the peek

[WizardOhio24]

Indeed, I do not. We should have as few expects as possible. If there is another way which will not cause the program to crash, we should use that.
Either way, in your code, you are now doing the same check twice. A more efficient way to write this would be by removing the expect and using .ok_or_else(). Your code does what it's supposed to, it was more the code quality I was concerned with.

[pm100]

ok, i believe this code is how you want it.

here is the error

[extrawurst]

@pm100 @WizardOhio24 wow I missed out on some fun here I see. Thanks for hanging in here. I agree with @WizardOhio24 we should reduce expect to a minimum since they are just nicer unwraps and those we already forbid via clippy. long term it would be nice to even get rid of all expect - but pick your battles. thanks for cleaning this up