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

[WIP] Handle ignored files with quoted (non-ASCII) filenames #694

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

pcantrell
Copy link

@pcantrell pcantrell commented Feb 21, 2024

Your checklist for this pull request

🚨Please review the guidelines for contributing to this repository.

  • Ensure all commits include DCO sign-off.
  • Ensure that your contributions pass unit testing.
  • Ensure that your contributions contain documentation if applicable.

Description

This is a bug fix: if a repository contains ignored files with non-ASCII or otherwise “unusual” characters that trigger quotePath, then Git.open(…).lib.ignored_files currently returns the git-quoted paths, not the proper filenames.

Some pre-review questions before I add the DCO sign-off:

  • It seems like this should be tested, but there are apparently no tests for ignored_files. Should I add some? If so, where?
  • Is ignored_files the only place this unescaping behavior is missing? Does it apply only to paths, or can (for example) branches and remotes also end up quoted? Should this unescaping live in Lib#command_lines, applying to all command outputs? (Probably not, but….)

@jcouball
Copy link
Member

@pcantrell thank you for this fix. It might take a little time for me to get a proper review done, but I wanted to let you know that I have seen your PR.

@jcouball jcouball self-requested a review February 22, 2024 02:27
@pcantrell
Copy link
Author

No worries; I understand that open source is mostly run by spare time and elbow grease.

Give it a look when you can, and don't hesitate to delegate tasks to get it into shape.

@jcouball
Copy link
Member

There are (at least) two problems that need to be addressed:

  1. The core issue which you report: filenames returned by Git::Lib#ignored_files should be unquoted using the same rules as Git::Lib#ls_files.

This lib always passes the global option "-c core.quotePath=true" when shelling out to git. This will result in the filenames quoted according to the documented rules for core.quotePath.

I believe that the fix submitted in this PR will be all that is required to fix the problem, but tests are needed. Ideally, would have been nice to start with a failing test in the first commit and then add the implementation for the fix in the second commit.

I'd put these tests in tests/units/test_ls_files_ignored.rb.

  1. Create an interface in Git::Base for getting a list of ignored files

I am trying to transition the Git::Lib to be a private implementation detail of Git::Base which will give more freedom for refactoring the current implementation (which is IMHO a hot mess). Currently, there is no other way to get a list of ignored files with Git::Base. I think we should add a new method to Git::Base for that purpose and have that method call Git::Lib#ignored_files. I would name the new method Git::Base#ls_files_ignored to indicate it is just passing different flags to git ls-files.

This method should have its own minimal test and be documented with YARDoc and example(s) should be put in the README.md.

You can use this PR for just the first issue for now or if you are feeling like doing both, then you can do both in this PR. If you don't feel like doing the Git::Base changes, let me know and I will do them later.

@jcouball
Copy link
Member

Also, rebase this branch with master to make the failing CI builds go away.

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.

None yet

2 participants