Skip to content

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

Merged
Enselic merged 11 commits into
sharkdp:masterfrom
cyqsimon:git-config-fix
Feb 26, 2022
Merged

Recognize files in $XDG_CONFIG_HOME/git/ and $HOME/.config/git/ better#2067
Enselic merged 11 commits into
sharkdp:masterfrom
cyqsimon:git-config-fix

Conversation

@cyqsimon

@cyqsimon cyqsimon commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

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.

Comment thread src/syntax_mapping.rs Outdated
@cyqsimon cyqsimon requested a review from Enselic February 18, 2022 02:43
Comment thread src/syntax_mapping.rs Outdated
@Enselic

Enselic commented Feb 18, 2022

Copy link
Copy Markdown
Collaborator

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

@cyqsimon

Copy link
Copy Markdown
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.

Comment thread tests/integration_tests.rs
@cyqsimon

Copy link
Copy Markdown
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.

@cyqsimon cyqsimon requested a review from Enselic February 18, 2022 11:56

@Enselic Enselic left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Contributor Author

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

@cyqsimon cyqsimon requested a review from Enselic February 20, 2022 04:05
Comment thread src/syntax_mapping.rs Outdated
Comment thread src/syntax_mapping.rs Outdated
Comment thread CHANGELOG.md Outdated
cyqsimon and others added 2 commits February 25, 2022 19:36
Co-authored-by: Martin Nordholts <enselic@gmail.com>

@sharkdp sharkdp left a comment

Copy link
Copy Markdown
Owner

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