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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle branch names differing only by case #285

Merged
merged 4 commits into from Jun 19, 2019

Conversation

@hdgarrood
Copy link
Contributor

commented Jun 19, 2019

Description of the change

Escape upper case characters in branch names to ensure that branches
which only differ in case don't cause issues on case-insensitive file
systems.

Also switch to using an allowlist for escaping characters: it seems a
little safer to specify the characters which don't need escaping (and to
escape everything else), than to specify characters which do need
escaping.

Finally, add a test that getCacheVersionDir is injective.

Checklist:

  • Added the change to the "Unreleased" section of the changelog
  • Added some example of the new feature to the README
  • Added a test for the contribution (if applicable)

P.S.: the above checks are not compulsory to get a change merged, so you may skip them. However, taking care of them will result in less work for the maintainers and will be much appreciated 馃槉

hdgarrood added some commits Jun 19, 2019

Handle branch names differing only by case
Escape upper case characters in branch names to ensure that branches
which only differ in case don't cause issues on case-insensitive file
systems.

Also switch to using an allowlist for escaping characters: it seems a
little safer to specify the characters which don't need escaping (and to
escape everything else), than to specify characters which do need
escaping.

Finally, add a test that `getCacheVersionDir` is injective.
@hdgarrood

This comment has been minimized.

Copy link
Contributor Author

commented Jun 19, 2019

It turns out that using QuickCheck to check that a function is injective requires a bit more effort than I thought it would. This is the simplest and most sensible thing I could come up with which reliably fails if I modify getVersionCacheDir to no longer be injective.

@f-f

f-f approved these changes Jun 19, 2019

Copy link
Member

left a comment

Looks great, thank you! 鉂わ笍

package.yaml Outdated Show resolved Hide resolved
Update package.yaml
Co-Authored-By: Fabrizio Ferrai <fabrizio.ferrai@gmail.com>

@f-f f-f merged commit 76af253 into spacchetti:master Jun 19, 2019

2 checks passed

Travis CI - Pull Request Build Passed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

elliotdavies pushed a commit to elliotdavies/spago that referenced this pull request Jul 1, 2019

Handle branch names differing only by case (spacchetti#285)
Escape upper case characters in branch names to ensure that branches
which only differ in case don't cause issues on case-insensitive file
systems.

Also switch to using an allowlist for escaping characters: it seems a
little safer to specify the characters which don't need escaping (and to
escape everything else), than to specify characters which do need
escaping.

Finally, add a test that `getCacheVersionDir` is injective.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can鈥檛 perform that action at this time.