New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixes #415 - branch names mangled by color escapes #416

Merged
merged 3 commits into from Jun 17, 2018

Conversation

Projects
None yet
2 participants
@rivy
Contributor

rivy commented Jun 14, 2018

  • add "-c color.branch=never" option to git branch commands, definitively disabling color branch output

.# Discussion

Branch names are mis-parsed if git branch output is colored (eg, when git is configured
with "config.branch=always"). Using the base "-c color.branch=never" option overrides any
user configuration, displaying git branch output as uncolored in all cases and fixing the
issue.

Notably, option ordering is important in this instance. The "-c ..." option is a git
base option, not a git branch subcommand option. So, as done here, it must be inserted
before the "branch" subcommand.

ref: Issue #415

Fixes #415 - branch names mangled by color escapes
- add "-c color.branch=never" option to git branch commands, definitively disabling color branch output

.# Discussion

Branch names are mis-parsed if `git branch` output is colored (eg, when git is configured
with "config.branch=always"). Using the base "-c color.branch=never" option overrides any
user configuration, displaying git branch output as uncolored in all cases and fixing the
issue.

Notably, option ordering is important in this instance. The "-c ..." option is a git
base option, not a git branch subcommand option. So, as done here, it must be inserted
before the "branch" subcommand.

ref: Issue #415
@eamodio

This comment has been minimized.

Owner

eamodio commented Jun 15, 2018

Thanks for your contribution!

I'm surprised setting color.ui=false isn't enough as it is supposed to be the master switch:
https://git-scm.com/book/en/v2/Customizing-Git-Git-Configuration#_code_color_ui_code

Here is the relevant code
https://github.com/eamodio/vscode-gitlens/blob/develop/src/git/git.ts#L102

And if color.ui is not reliable, we will probably need to add the specific overrides to all the commands, branch, diff, status, etc

@rivy

This comment has been minimized.

Contributor

rivy commented Jun 15, 2018

Yes, I was surprised that "color.ui=false" doesn't turn off all color output, as well. It may well be that it actually does turn off all color, but then is reversed by more specific overrides (such as "color.branch=always") at other configuration levels.

I, personally, have "color.branch=always", "color.diff=always", and "color.status=always" set at the global level, so that I can, by default, get colored output even for redirections (eg, to less). This does cause parsing confusion for code instrumenting git and expecting non-colored output, and I've encountered related bugs in lots of other places (eg, jisaacks/GitGutter#475).

Although this is the only issue that I'm encountering here, I do think that, ultimately, for robustness, all the verb specific color settings (branch, diff, interactive, status) should be set to "false" or "never" when parsing the git output of those verbs. Whether it should be centrally done by adding all of the config settings ("-c color.branch=false", "-c color.diff=false", "-c color.interactive=false", and "-c color.status=false") at https://github.com/eamodio/vscode-gitlens/blob/develop/src/git/git.ts#L102 or at the individual verb usages is more of a style question.

Notably, the config setting disabling color (eg, "-c color.branch=never") can be reversed by a later setting within the same command line turning it back on (eg, "-c color.branch=always"). So, being a bit more DRY and disabling them all centrally would be fine even if you want to receive colored output for some specific commands.

I don't know if there would be any performance penalty, but I doubt it given that most git commands are disk I/O bound.

I can refactor this PR to be more general and centrally place all of those config settings if you'd prefer.

@eamodio

This comment has been minimized.

Owner

eamodio commented Jun 15, 2018

While I'm slightly on the fence about centralizing them vs not. I think it might be better to keep them separate (i.e. associated with the specific command they apply to), just to keep the length of the command line down. So if you wouldn't mind adding the other settings to the relevant places I would be very grateful 😄. Also please set them to false since the git docs don't seem to mention never

Thank you!

@rivy

This comment has been minimized.

Contributor

rivy commented Jun 15, 2018

Done.

I found three uses of 'diff' and two uses of 'status', and I modified the config options of all of them. All are using "false". But, FYI, I only looked for uses within "src/git/git.ts". It appeared that that was where everything was localized. Let me know if there are other areas to change.

The final result is working correctly for me.

@eamodio eamodio changed the base branch from master to develop Jun 17, 2018

@eamodio

This comment has been minimized.

Owner

eamodio commented Jun 17, 2018

Looks great! Thank you!

@eamodio eamodio merged commit 25e75d4 into eamodio:develop Jun 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment