Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
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.

Enselic
Enselic reviewed Feb 9, 2022
View reviewed changes
@cyqsimon cyqsimon requested a review from Enselic February 18, 2022 02:43
Enselic
Enselic reviewed Feb 18, 2022
View reviewed changes
@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
cyqsimon commented Feb 18, 2022
View reviewed changes
@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.

@cyqsimon cyqsimon requested a review from Enselic February 18, 2022 11:56
Enselic
Enselic approved these changes Feb 19, 2022
View reviewed changes
Copy link
Collaborator

@Enselic Enselic left a comment •
edited
Loading

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.

@cyqsimon cyqsimon requested a review from Enselic February 20, 2022 04:05
Enselic
Enselic reviewed Feb 25, 2022
View reviewed changes
cyqsimon and others added 2 commits February 25, 2022 19:36
@cyqsimon @Enselic
Apply suggestions from code review
92c8483 
Co-authored-by: Martin Nordholts <enselic@gmail.com>
@cyqsimon
Guard against empty $HOME
2556530
sharkdp
sharkdp approved these changes Feb 26, 2022
View reviewed changes
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
Reviewers

@Enselic Enselic Enselic approved these changes

@sharkdp sharkdp sharkdp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@cyqsimon @Enselic @sharkdp