Skip to content
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

Recognize files in $XDG_CONFIG_HOME/git/ and $HOME/.config/git/ better #2067

Merged
merged 11 commits into from
Feb 26, 2022
Merged

Recognize files in $XDG_CONFIG_HOME/git/ and $HOME/.config/git/ better #2067

merged 11 commits into from
Feb 26, 2022

Conversation

cyqsimon
Copy link
Contributor

@cyqsimon cyqsimon commented Feb 9, 2022

Related issue: #1191

When #1191 was fixed, the implementation was incomplete. More specifically, it did not cover the case when $XDG_CONFIG_HOME is not set or empty, as specified in git-config documentation.

This PR fixes that.

src/syntax_mapping.rs Outdated Show resolved Hide resolved
src/syntax_mapping.rs Outdated Show resolved Hide resolved
@Enselic
Copy link
Collaborator

Enselic commented Feb 18, 2022

Would be great if you could add a couple of integration tests for this change, see tests/integration_test.rs

@cyqsimon
Copy link
Contributor Author

I wrote a test, but I'm not quite sure why it's failing. Probably some funky stuff going on with the ANSI escape codes. Help needed.

@cyqsimon
Copy link
Contributor Author

Okay I think I fixed the test. It is some weird ANSI colour escape sequence issue that I don't really understand. Please verify that the test is effective. Thanks.

Copy link
Collaborator

@Enselic Enselic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big thanks for adding tests. Automated tests are so great to have. With automated tests in place I consider this Approved (assuming you handle remaining comments). We could argue about if we want to define functions in functions, but I don't think it matters much with automated tests in place. Because then we can easily change that later. But I have to admit the diff is nice right now.

It would be great if you could add an entry in the CHANGELOG.md before we merge. See https://github.com/sharkdp/bat/blob/master/CONTRIBUTING.md

@cyqsimon
Copy link
Contributor Author

I did a rebase against the newest master branch to resolve merge conflicts of changelog.md.

src/syntax_mapping.rs Outdated Show resolved Hide resolved
src/syntax_mapping.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
cyqsimon and others added 2 commits February 25, 2022 19:36
Co-authored-by: Martin Nordholts <enselic@gmail.com>
Copy link
Owner

@sharkdp sharkdp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much

@Enselic Enselic changed the title git global config - lookup $XDG_CONFIG_HOME faithfully Recognize files in $XDG_CONFIG_HOME/git/ and $HOME/.config/git/ better Feb 26, 2022
@Enselic Enselic merged commit 14ddda0 into sharkdp:master Feb 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants