Replace /'s for cache version dir name

[codedmart]

• Fixes Fails to install branches with / in the branch name #257

Description of the change

TODO

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 😊

[f-f]

Just one small suggestion, but otherwise looks great! 👏

[codedmart]

@f-f OK updated now. As per @hdgarrood's suggestion also escaping a few other characters that could be in branch names as well.

[hdgarrood]

I think we should include % in the list of characters to be escaped as currently I think “%2F” and “/“ will both map to “%2F“.

Also I’ve just remembered that case insensitive file systems are a thing...

[f-f]

@hdgarrood agreed, this is currently still not injective but if we add % to the list it will.

And yes, e.g. my mac is case insensitive, so two branches with different cases would still clash. I think though we can just add a TODO about this and hope no one tries to fetch two branches differing only by case 😄

Btw @codedmart if you want to have some fun you could setup Quickcheck with a property that requires that getCacheVersionDir cannot map two different names to the same output (I'll merge this PR in any case, so if you want to do this feel free to open another PR. Though it's probably more interesting if you write the test before fixing the implementation, so you can marvel at Quickcheck proving things)

[codedmart]

Yup forgot % and the case sensitivity. Will fix. I will look at the test as well. Thanks!

[hdgarrood]

I suppose one option is to say that any uppercase character needs escaping too; in my experience branch names rarely contain uppercase characters (so this scheme will probably give you nice readable branch names more often than not).

[hdgarrood]

Also if you want to write a QuickCheck property for this, I think you'd need to be a bit careful if you want to end up with one that's actually useful; if you just write

quickCheck \x y -> (x /= y) ==> (getVersionCacheDir x /= getVersionCacheDir y)

then the chance of a collision is so astronomically tiny that that test is only really slowing down your test suite. I'd recommend writing your own generator which uses a much smaller set of characters to sample from, in particular including the ones we know to be problematic. Then, instead of running lots of individual tests, we should just run a single test where we generate a large set of potential branch names, apply the getVersionCacheDir function to all of them, assemble the results into another set, and check that the result set is the same size as the input set. I think this is the only way you'll get the test to fail reliably if the function is not actually injective.

[f-f]

@codedmart this got a bit out of sync after all the ZuriHac's contributions, so I'll merge it now so we can ship it in the upcoming release later today. Thank you! 🙂